Skip to content

Catch query exception on bulk update/delete#10517

Merged
abnegate merged 1 commit into1.8.xfrom
fix-miss-catch
Sep 19, 2025
Merged

Catch query exception on bulk update/delete#10517
abnegate merged 1 commit into1.8.xfrom
fix-miss-catch

Conversation

@abnegate
Copy link
Copy Markdown
Member

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

  • (Related PR or issue)

Checklist

  • Have you read the Contributing Guidelines on issues?
  • If the PR includes a change to an API's metadata (desc, label, params, etc.), does it also include updated API specs and example docs?

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai bot commented Sep 19, 2025

📝 Walkthrough

Walkthrough

Adds 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)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
Description Check ❓ Inconclusive The PR description contains only the default contributor template and does not describe the implemented changes, test plan, or related issues, so it is generic and provides insufficient context for reviewers. Please update the description with a concise summary of what was changed (files/behavior), why the change was made, how it was tested (test plan or verification steps), and links to any related issues or PRs to help reviewers and maintainers.
✅ Passed checks (1 passed)
Check name Status Explanation
Title Check ✅ Passed The title "Catch query exception on bulk update/delete" is concise, focused, and accurately reflects the primary code change (adding QueryException handling to bulk update and delete flows), so it clearly summarizes the main intent of the changeset for reviewers.
✨ Finishing touches
  • 📝 Generate Docstrings
🧪 Generate unit tests
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch fix-miss-catch

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@github-actions
Copy link
Copy Markdown

Security Scan Results for PR

Docker Image Scan Results

Package Version Vulnerability Severity
golang.org/x/crypto v0.31.0 CVE-2025-22869 HIGH
golang.org/x/oauth2 v0.24.0 CVE-2025-22868 HIGH
stdlib 1.22.10 CVE-2025-47906 HIGH
stdlib 1.22.10 CVE-2025-47907 HIGH

Source Code Scan Results

🎉 No vulnerabilities found!

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 exposure

Pass 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 policy

Same 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

📥 Commits

Reviewing files that changed from the base of the PR and between df27669 and f944ad3.

📒 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

@github-actions
Copy link
Copy Markdown

✨ Benchmark results

  • Requests per second: 1,091
  • Requests with 200 status code: 196,385
  • P99 latency: 0.176766673

⚡ Benchmark Comparison

Metric This PR Latest version
RPS 1,091 893
200 196,385 160,786
P99 0.176766673 0.211381761

@abnegate abnegate merged commit 0b5fc8f into 1.8.x Sep 19, 2025
41 checks passed
@abnegate abnegate deleted the fix-miss-catch branch September 19, 2025 21:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant