LoginSignup
0
0

Delete article

Deleted articles cannot be recovered.

Draft of this article would be also deleted.

Are you sure you want to delete this article?

@sho_fcafein株式会社エフカフェ

【どう実装する？シリーズ #8】削除対象の自動検出、コードを読まないと分からない挙動になってない？

Posted at

前回の記事では、バリデーションをクラスに集約し、すべてのエラーを一度に返せるようにしました。

今回は、まだ残っている問題の1つ「暗黙的な副作用」に取り組みます。

問題の再確認

現在のLP保存処理には、「コードを読まないと分からない挙動」があります。

class LandingPageService
{
    public function update(int $lpId, array $data, int $userId): int
    {
        // バリデーション
        // ...

        DB::beginTransaction();
        try {
            // LP本体を保存
            $lp = Lp::updateOrCreate(['id' => $lpId], [
                'name' => $data['name'],
                'status' => $data['status'],
                'template' => $data['template'],
            ]);

            // データ整形
            $formattedData = $this->formatData($data);

            // ステップ・CTAを保存
            $this->saveStepsAndCtas($formattedData, $lp);

            // 暗黙的な削除対象の検出とマージ ← ここ
            $currentStepIds = $this->collectStepIds($formattedData);
            $orphanedStepIds = $lp->steps()
                ->whereNotIn('id', $currentStepIds)
                ->pluck('id')
                ->toArray();
            $allDeleteIds = array_merge(
                $data['delete_steps'] ?? [],
                $orphanedStepIds
            );

            // ステップ削除(関連CTAも連鎖削除) ← さらにここ
            if (!empty($allDeleteIds)) {
                $relatedCtaIds = $lp->steps()
                    ->whereIn('id', $allDeleteIds)
                    ->pluck('related_cta_id')
                    ->toArray();
                $lp->steps()
                    ->whereIn('id', array_merge($allDeleteIds, $relatedCtaIds))
                    ->delete();
            }

            // ...
        }
    }
}

2つの暗黙的な挙動があります：

  1. リクエストに含まれないステップは自動的に削除される
  2. 削除されるステップに紐づくCTAも連鎖削除される

具体例で見る問題

シナリオ

現在のLP:

ステップ1 (id: 100)
ステップ2 (id: 200) → 個別CTA (id: 201)
ステップ3 (id: 300)

更新リクエスト:

{
  "steps": [
    {"id": 100, "content": "..."},
    {"id": 300, "content": "..."}
  ]
}

ステップ2（id: 200）が含まれていません。

何が起きるか

// 現在のステップID: [100, 300]
$currentStepIds = [100, 300];

// DBに存在するが、リクエストにないステップ: [200]
$orphanedStepIds = [200];

// 削除対象: [] (明示的) + [200] (暗黙的) = [200]
$allDeleteIds = [200];

// ステップ200に紐づくCTA: [201]
$relatedCtaIds = [201];

// 最終的に削除: [200, 201]
$lp->steps()->whereIn('id', [200, 201])->delete();

結果:

  • ステップ2（id: 200）が削除される
  • 個別CTA（id: 201）も連鎖削除される

リクエストには「削除してほしい」とは書いていないのに、削除されます。

何が問題か

1. 挙動がコードを読まないと分からない

APIの呼び出し側（フロントエンド）は、この挙動を知っている必要があります。

// フロントエンド開発者の認識
// 「ステップ1と3だけを更新する」
await api.updateLP({
  steps: [
    { id: 100, content: "..." },
    { id: 300, content: "..." }
  ]
});

// 実際の挙動
// 「ステップ1と3を更新し、ステップ2を削除する」

ドキュメントやコードを読まない限り、この挙動は分かりません。

2. 意図しない削除が起きる可能性

// 部分的な更新をしたいケース
// 「ステップ1だけ更新したい」
await api.updateLP({
  steps: [
    { id: 100, content: "新しい内容" }
  ]
});

// → ステップ2, 3が削除されてしまう！

部分更新のつもりが、全削除になります。

3. 関連CTAの連鎖削除も暗黙的

// 削除されるステップに紐づくCTAも削除
$relatedCtaIds = $lp->steps()
    ->whereIn('id', $allDeleteIds)
    ->pluck('related_cta_id')
    ->toArray();

ステップを削除すると、関連CTAも消えます。これもコードを読まないと分かりません。

4. テストで網羅しづらい

public function test_ステップ更新()
{
    // 既存データ
    $lp = Lp::factory()->create();
    $step1 = Step::factory()->create(['lp_id' => $lp->id, 'id' => 100]);
    $step2 = Step::factory()->create(['lp_id' => $lp->id, 'id' => 200]);
    $cta = Step::factory()->create(['lp_id' => $lp->id, 'id' => 201, 'related_cta_id' => 200]);
    
    // ステップ1だけを送信
    $this->service->update($lp->id, [
        'steps' => [
            ['id' => 100, 'content' => '...']
        ]
    ], 1);
    
    // ステップ2が削除されていることをテストする必要がある
    $this->assertDatabaseMissing('steps', ['id' => 200]);
    // CTAも削除されていることをテストする必要がある
    $this->assertDatabaseMissing('steps', ['id' => 201]);
}

「送信していないものが削除される」という挙動をテストしないといけません。

5. デバッグが困難

お客様: 「ステップが消えたんですけど...」
開発者: 「削除操作はしましたか？」
お客様: 「してないです」
開発者: 「...」（コードを読んで初めて原因が分かる）

明示的にする

暗黙的な挙動を、明示的にしましょう。

方針1: 削除は明示的に指定する

リクエストに含まれないステップを自動削除するのではなく、削除したいステップは明示的に指定する。

Before（暗黙的）

// リクエストに含まれないステップは自動削除
await api.updateLP({
  steps: [
    { id: 100, content: "..." },
    { id: 300, content: "..." }
  ]
  // ステップ2(id: 200)は含まれていない → 自動削除される
});

After（明示的）

// 削除したいステップは明示的に指定
await api.updateLP({
  steps: [
    { id: 100, content: "..." },
    { id: 300, content: "..." }
  ],
  delete_steps: [200]  // 削除したいステップを明示
});

方針2: 同期処理をやめる

「リクエストの内容 = 最終的な状態」という同期処理ではなく、個別の操作として扱う。

// 追加
await api.addStep({ lp_id: 1, content: "..." });

// 更新
await api.updateStep({ id: 100, content: "..." });

// 削除
await api.deleteStep({ id: 200 });

操作が明示的になります。

今回は**方針1（削除を明示的に指定）**で実装します。

実装

Service

class LandingPageService
{
    public function update(int $lpId, array $data, int $userId): int
    {
        // バリデーション
        // ...

        DB::beginTransaction();
        try {
            // LP本体を保存
            $lp = Lp::updateOrCreate(['id' => $lpId], [
                'name' => $data['name'],
                'status' => $data['status'],
                'template' => $data['template'],
            ]);

            // データ整形
            $formattedData = $this->formatData($data);

            // ステップ・CTAを保存
            $this->saveStepsAndCtas($formattedData, $lp);

            // 明示的に指定された削除対象のみ処理
            if (!empty($data['delete_steps'])) {
                $this->deleteSteps($lp, $data['delete_steps']);
            }

            // ...
        }
    }

    private function deleteSteps(Lp $lp, array $stepIds): void
    {
        // 削除対象のステップに紐づくCTAも取得
        $relatedCtaIds = $lp->steps()
            ->whereIn('id', $stepIds)
            ->pluck('related_cta_id')
            ->filter() // nullを除外
            ->toArray();

        // ステップと関連CTAを削除
        $allDeleteIds = array_merge($stepIds, $relatedCtaIds);
        $lp->steps()->whereIn('id', $allDeleteIds)->delete();

        // 削除内容をログに記録
        Log::info('Steps deleted', [
            'lp_id' => $lp->id,
            'step_ids' => $stepIds,
            'related_cta_ids' => $relatedCtaIds,
        ]);
    }
}

変更点:

  • 暗黙的な削除対象検出コードを削除
  • delete_stepsが指定された場合のみ削除
  • 削除処理をdeleteSteps()メソッドに分離
  • ログで削除内容を記録

deleteSteps() は public? private?

deleteSteps()をprivateにするかpublicにするかは、設計判断が必要です。

public にするメリット:

  • 単独でテストできる
  • 他の箇所から再利用できる
  • 将来的に削除専用APIとして提供できる

public にするデメリット:

  • トランザクション管理が曖昧になる（呼び出し側が管理？メソッド内で管理？）
  • バリデーションの責任が不明確（呼び出し側がチェック？メソッド内でチェック？）
  • 部分的な状態変更を許してしまう（LPだけ更新して、後からステップ削除、など）
  • 契約（事前条件・事後条件）が複雑になる

今回の判断: private

現状の設計では：

  • update()内で一連の処理として実行される
  • トランザクション管理はupdate()が担当
  • バリデーションもupdate()経由

deleteSteps()はupdate()の内部実装の詳細として、privateにしておく方が安全です。

もし将来的に削除専用APIが必要になったら、その時点で：

  1. publicに変更
  2. トランザクション管理を明示（メソッド内で完結させる、など）
  3. バリデーションを追加
  4. 契約を明確にする

という対応をすればいいでしょう。

フロントエンド

// 明示的な削除指定
async function updateLP(lpData) {
  const currentStepIds = lpData.steps.map(s => s.id).filter(id => id);
  const existingStepIds = await fetchExistingStepIds(lpData.id);
  
  // 削除対象を計算（リクエストに含まれないステップ）
  const deleteSteps = existingStepIds.filter(id => !currentStepIds.includes(id));

  await api.updateLP({
    ...lpData,
    delete_steps: deleteSteps  // 明示的に指定
  });
}

削除の意図が明確になります。

APIドキュメント

POST /api/lp/update
parameters:
  steps:
    type: array
    description: 更新するステップ一覧
  delete_steps:
    type: array
    description: 削除するステップのID一覧（オプション）
    example: [200, 201]

ドキュメントを読めば、挙動が分かります。

連鎖削除も明示的にする

関連CTAの連鎖削除も、より明示的にできます。

Before（暗黙的）

private function deleteSteps(Lp $lp, array $stepIds): void
{
    // 関連CTAも自動的に削除される
    $relatedCtaIds = $lp->steps()
        ->whereIn('id', $stepIds)
        ->pluck('related_cta_id')
        ->filter()
        ->toArray();
    
    $allDeleteIds = array_merge($stepIds, $relatedCtaIds);
    $lp->steps()->whereIn('id', $allDeleteIds)->delete();
}

After（明示的）

オプション1: フラグで制御

private function deleteSteps(Lp $lp, array $stepIds, bool $deleteRelatedCtas = true): void
{
    $deleteIds = $stepIds;
    
    if ($deleteRelatedCtas) {
        $relatedCtaIds = $lp->steps()
            ->whereIn('id', $stepIds)
            ->pluck('related_cta_id')
            ->filter()
            ->toArray();
        
        $deleteIds = array_merge($deleteIds, $relatedCtaIds);
        
        Log::info('Deleting steps with related CTAs', [
            'step_ids' => $stepIds,
            'related_cta_ids' => $relatedCtaIds,
        ]);
    }
    
    $lp->steps()->whereIn('id', $deleteIds)->delete();
}

オプション2: DB側で制御（外部キー制約）

// マイグレーション
Schema::create('steps', function (Blueprint $table) {
    $table->id();
    $table->foreignId('related_cta_id')
        ->nullable()
        ->constrained('steps')
        ->onDelete('cascade');  // DB側で連鎖削除
});

// Service
private function deleteSteps(Lp $lp, array $stepIds): void
{
    // DB側で自動的に連鎖削除される
    $lp->steps()->whereIn('id', $stepIds)->delete();
    
    Log::info('Steps deleted (CTAs cascaded by DB)', [
        'step_ids' => $stepIds,
    ]);
}

DB側で連鎖削除を定義すれば、コード側はシンプルになります。

明示的にすることのメリット

1. 挙動が分かりやすい

// コードを見れば、何が起きるか明確
await api.updateLP({
  steps: [...],
  delete_steps: [200]  // ステップ200を削除する意図が明確
});

2. 意図しない削除を防げる

// 部分更新も安全
await api.updateLP({
  steps: [
    { id: 100, content: "新しい内容" }
  ]
  // delete_stepsを指定しないので、他のステップは削除されない
});

3. テストがシンプルになる

public function test_指定されたステップだけ削除される()
{
    $lp = Lp::factory()->create();
    $step1 = Step::factory()->create(['lp_id' => $lp->id, 'id' => 100]);
    $step2 = Step::factory()->create(['lp_id' => $lp->id, 'id' => 200]);
    
    // ステップ2を削除
    $this->service->update($lp->id, [
        'steps' => [['id' => 100, 'content' => '...']],
        'delete_steps' => [200]  // 明示的に指定
    ], 1);
    
    // ステップ2だけが削除されている
    $this->assertDatabaseHas('steps', ['id' => 100]);
    $this->assertDatabaseMissing('steps', ['id' => 200]);
}

public function test_delete_stepsがない場合は削除されない()
{
    $lp = Lp::factory()->create();
    $step1 = Step::factory()->create(['lp_id' => $lp->id, 'id' => 100]);
    $step2 = Step::factory()->create(['lp_id' => $lp->id, 'id' => 200]);
    
    // delete_stepsを指定しない
    $this->service->update($lp->id, [
        'steps' => [['id' => 100, 'content' => '...']]
    ], 1);
    
    // 両方とも残っている
    $this->assertDatabaseHas('steps', ['id' => 100]);
    $this->assertDatabaseHas('steps', ['id' => 200]);
}

テストケースが明確になります。

4. デバッグが簡単

お客様: 「ステップが消えたんですけど...」
開発者: 「ログを確認します」

[ログ]
Steps deleted
- lp_id: 123
- step_ids: [200]
- related_cta_ids: [201]
- user_id: 456
- timestamp: 2024-01-15 10:30:00

開発者: 「10:30にユーザーID 456が削除操作を行いました」

削除操作がログに残るので、原因が特定しやすくなります。

5. API仕様が明確になる

POST /api/lp/update

parameters:
  steps: 更新するステップ一覧
  delete_steps: 削除するステップID一覧（オプション）
    - 指定したステップとその関連CTAが削除されます
    - 指定しない場合、既存ステップは削除されません

暗黙的な挙動を見つけるヒント

コードの中で、こんなパターンがあったら要注意：

パターン1: 差分を計算して削除

// リクエストにないものを削除
$existing = Model::all()->pluck('id');
$requested = collect($data)->pluck('id');
$toDelete = $existing->diff($requested);

Model::whereIn('id', $toDelete)->delete();

パターン2: 先に全削除してから再作成

// 既存を全削除
$lp->steps()->delete();

// 新しく作成
foreach ($data['steps'] as $step) {
    $lp->steps()->create($step);
}

パターン3: whereNotIn で削除

// リクエストに含まれないものを削除
Model::whereNotIn('id', $requestedIds)->delete();

これらは「リクエストに含まれない = 削除したい」という暗黙の前提があります。

まとめ

暗黙的な副作用には、以下の問題がありました：

問題点

  • 挙動がコードを読まないと分からない
  • 意図しない削除が起きる可能性
  • テストで網羅しづらい
  • デバッグが困難

削除を明示的に指定することで：

メリット

  • 挙動が分かりやすい
  • 意図しない削除を防げる
  • テストがシンプルになる
  • デバッグが簡単
  • API仕様が明確になる

「リクエストに含まれない = 削除したい」という暗黙の前提ではなく、「削除したいものは明示的に指定する」設計にすることで、コードの意図が明確になり、バグも減ります。

次回の記事では、まだ残っている問題「データ整形ロジックの混在」に取り組みます。リクエストのデータ構造と内部で扱うデータ構造が異なる場合、その変換処理をどこに置くべきか、どう設計すべきかを考えます。

0
0
0

Register as a new user and use Qiita more conveniently

  1. You get articles that match your needs
  2. You can efficiently read back useful information
  3. You can use dark theme
What you can do with signing up
Sign upLogin
0
0

Delete article

Deleted articles cannot be recovered.

Draft of this article would be also deleted.

Are you sure you want to delete this article?