Fix/delete unexisting resource #194
Conversation
WalkthroughAdds per-item existence validation and distinct check for bulk delete requests by constructing the resource model in DestroyRequest::destroyRules; adds a feature test verifying a 422 response and no deletions when any provided ID does not exist. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant C as Client
participant Ctl as Delete Controller
participant Req as DestroyRequest (validator)
participant DB as Database
C->>Ctl: DELETE /resource { resources: [id_missing, id_existing] }
Ctl->>Req: Validate payload
Req->>DB: For each resources.* => check distinct and exists(table, key)
alt Any ID missing
Req-->>Ctl: Validation fails (422, errors.resources.*)
Ctl-->>C: 422 Unprocessable Entity
else All IDs exist
Ctl->>DB: Delete rows
Ctl-->>C: 200/204 Success
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (12)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
…omkit/laravel-rest-api into fix/delete-unexisting-resource
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (6)
src/Http/Requests/DestroyRequest.php (3)
36-44: Good existence check; add distinct/min:1 and prefer model-aware exists to avoid connection/table drift.
- Add min:1 so empty arrays fail fast.
- Add distinct to prevent duplicate IDs.
- Use Rule::exists(get_class($model), …) so the model’s connection/table are honored.
- Optional: bail on each item to skip DB lookups if a duplicate is detected first.
Apply:
- return [ - 'resources' => [ - 'required', 'array', - ], - 'resources.*' => [ - Rule::exists($model->getTable(), $model->getKeyName()), - ], - ]; + return [ + 'resources' => [ + 'required', 'array', 'min:1', + ], + 'resources.*' => [ + 'bail', 'distinct', + Rule::exists(get_class($model), $model->getKeyName()), + ], + ];
36-44: Consider scoping the exists rule to the resource’s visibility to avoid ID enumeration.If this API is multi‑tenant or otherwise scoped, plain exists can leak whether an ID exists outside the caller’s scope. If your Resource exposes a query scope (e.g., indexQuery or similar), apply it to the Exists rule via using()/where().
Would you like me to wire this to the appropriate scope on Resource (if available) so validation only passes for records visible to the caller?
23-33: Docblock param type mismatch.The docblock says “@param resource” (PHP built‑in) but the signature is Resource. Update for accuracy.
- * @param resource $resource + * @param Resource $resourcetests/Feature/Controllers/DeleteOperationsTest.php (3)
59-79: Naming nit: “non_existing” reads cleaner than “not_existing”.Purely cosmetic; feel free to skip.
- public function test_deleting_a_not_existing_model(): void + public function test_deleting_a_non_existing_model(): void
59-79: Optional: add a duplicate‑ID case once distinct is enforced.If you adopt distinct, add a test to assert a validation error for duplicate IDs and that no deletions occur.
I can add a focused test method mirroring this one with a duplicated $model->getKey().
59-79: Prefer built‑in validation assertions (use assertInvalid when available)Replace assertExactJsonStructure with assertInvalid(['resources.0']) in this test; assertExactJsonStructure is used across many tests — perform a project‑wide replacement only after confirming your Laravel version supports assertInvalid (fallback: assertJsonValidationErrors).
- $response->assertStatus(422); - $response->assertExactJsonStructure(['message', 'errors' => ['resources.0']]); + $response->assertStatus(422); + $response->assertInvalid(['resources.0']);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/Http/Requests/DestroyRequest.php(2 hunks)tests/Feature/Controllers/DeleteOperationsTest.php(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
tests/Feature/Controllers/DeleteOperationsTest.php (3)
tests/Support/Database/Factories/ModelFactory.php (1)
ModelFactory(8-29)tests/Support/Models/Model.php (1)
Model(10-112)tests/Support/Policies/GreenPolicy.php (1)
GreenPolicy(8-100)
src/Http/Requests/DestroyRequest.php (1)
src/Http/Resource.php (1)
newModel(61-67)
🔇 Additional comments (1)
src/Http/Requests/DestroyRequest.php (1)
5-5: Import for Rule is correct.Needed for the new validation; no issues here.
| $model = $resource::newModel(); | ||
|
|
||
| return [ | ||
| 'resources' => [ | ||
| 'required', 'array', | ||
| ], | ||
| 'resources.*' => [ | ||
| Rule::exists($model->getTable(), $model->getKeyName()), | ||
| ], |
There was a problem hiding this comment.
🧩 Analysis chain
422 vs 404 semantics — confirm product intent.
This change makes non‑existent IDs return 422 (validation). The linked issue mentions “e.g., 404”. For bulk DELETE via body, 422 is reasonable; just confirm this is the intended contract.
🏁 Script executed:
#!/bin/bash
# Find any docs/tests expecting 404 for non-existent deletes
rg -n -C2 -i 'delete.+(404|not found)|assertStatus\(\s*404\s*\)|assertNotFound' --type=phpLength of output: 336
422 vs 404 semantics — confirm product intent.
Tests assert 404 for non‑existent deletes, but this change causes a 422 validation response; reconcile contract.
- Tests: tests/Feature/Controllers/ActionsOperationsTest.php:103 (assertStatus(404)).
- Code: src/Http/Requests/DestroyRequest.php (lines 36–44) — 'resources.*' uses Rule::exists(...) causing validation failure -> 422.
Either restore 404 semantics (handle missing IDs and return 404) or update tests/docs to accept 422.
closes #178
Summary by CodeRabbit
New Features
Tests