Catch query exception on bulk update/delete#10517
Conversation
📝 WalkthroughWalkthroughAdds QueryException handling to bulk document Delete and Update actions. In Delete.php and Update.php, a new catch block wraps deleteDocuments/updateDocuments calls and maps QueryException to Exception::GENERAL_QUERY_INVALID using the original message. Existing catches (ConflictException, RestrictedException/RelationshipException, StructureException) remain unchanged. No modifications to collection logic, onNext handling, metadata, metrics, response building, or event emission. No public interfaces are altered. Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests
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 |
Security Scan Results for PRDocker Image Scan Results
Source Code Scan Results🎉 No vulnerabilities found! |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Documents/Bulk/Update.php (1)
153-155: Chain original QueryException; verify message exposurePass the caught exception as previous for better logs/observability, and double‑check that returning the raw DB error message is acceptable to expose. If not, drop the message to use the standardized one for GENERAL_QUERY_INVALID. Optionally apply the same previous chaining to the earlier parseQueries catch for consistency.
- } catch (QueryException $e) { - throw new Exception(Exception::GENERAL_QUERY_INVALID, $e->getMessage()); + } catch (QueryException $e) { + throw new Exception( + Exception::GENERAL_QUERY_INVALID, + $e->getMessage(), + previous: $e + ); }src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Documents/Bulk/Delete.php (1)
127-129: Preserve exception chain; confirm client‑facing message policySame suggestion here: include the original exception as previous for diagnostics, and verify whether the raw QueryException message should be surfaced to clients or replaced by the default mapped message.
- } catch (QueryException $e) { - throw new Exception(Exception::GENERAL_QUERY_INVALID, $e->getMessage()); + } catch (QueryException $e) { + throw new Exception( + Exception::GENERAL_QUERY_INVALID, + $e->getMessage(), + previous: $e + ); }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Documents/Bulk/Delete.php(1 hunks)src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Documents/Bulk/Update.php(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Documents/Bulk/Delete.php (1)
src/Appwrite/Extend/Exception.php (1)
Exception(7-447)
src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Documents/Bulk/Update.php (1)
src/Appwrite/Extend/Exception.php (1)
Exception(7-447)
⏰ 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). (3)
- GitHub Check: Benchmark
- GitHub Check: Setup & Build Appwrite Image
- GitHub Check: scan
✨ Benchmark results
⚡ Benchmark Comparison
|
What does this PR do?
(Provide a description of what this PR does and why it's needed.)
Test Plan
(Write your test plan here. If you changed any code, please provide us with clear instructions on how you verified your changes work. Screenshots may also be helpful.)
Related PRs and Issues
Checklist