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

今回は、まだ残っている問題の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つの暗黙的な挙動があります：

リクエストに含まれないステップは自動的に削除される
削除されるステップに紐づく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が必要になったら、その時点で：

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

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

フロントエンド

// 明示的な削除指定
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仕様が明確になる

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

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