Throw BulkWriteException with write results on write errors#35
Throw BulkWriteException with write results on write errors#35premtsd-code wants to merge 3 commits intomainfrom
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughReplaced generic exceptions with Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
Greptile SummaryThis PR enhances error handling in bulk write operations by throwing Key changes:
Confidence Score: 5/5Safe to merge — the change is narrowly scoped, backwards-compatible (BulkWriteException extends Exception), and well-tested. No P0 or P1 issues found. The exception type change is backwards-compatible since No files require special attention.
|
| Filename | Overview |
|---|---|
| src/Client.php | Throws BulkWriteException with writeErrors and nInserted ($result->n ?? 0) instead of a generic Exception on bulk write errors — logic is correct for ordered: false insert scenarios. |
| src/Exception.php | Forwards writeErrors to the parent constructor so getWriteErrors() is now functional on BulkWriteException; clean fix that eliminates a latent inconsistency. |
| tests/MongoTest.php | New integration test verifies BulkWriteException is thrown with correct code and result structure, and that non-duplicate documents persist; assertions are precise. |
Reviews (2): Last reviewed commit: "Populate parent writeErrors in BulkWrite..." | Re-trigger Greptile
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/MongoTest.php (1)
509-509: TightennInsertedassertion to avoid false positives.
assertGreaterThanOrEqual(1, ...)can pass even if inserted count is wrong for this fixture (expected exactly 1).Proposed test assertion tightening
- self::assertGreaterThanOrEqual(1, $result['nInserted']); + self::assertSame(1, $result['nInserted']);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/MongoTest.php` at line 509, The current assertion uses assertGreaterThanOrEqual(1, $result['nInserted']) which can produce false positives; update the assertion to require exactly one inserted document by replacing it with an equality check against $result['nInserted'] (e.g., assertSame(1, $result['nInserted']) or assertEquals(1, $result['nInserted'])) so the test enforces the expected single insert.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@tests/MongoTest.php`:
- Line 509: The current assertion uses assertGreaterThanOrEqual(1,
$result['nInserted']) which can produce false positives; update the assertion to
require exactly one inserted document by replacing it with an equality check
against $result['nInserted'] (e.g., assertSame(1, $result['nInserted']) or
assertEquals(1, $result['nInserted'])) so the test enforces the expected single
insert.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: d6ebc86b-866f-4373-a791-3cb1a09c1454
📒 Files selected for processing (2)
src/Client.phptests/MongoTest.php
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
|
|
||
| $result = $e->getResult(); | ||
| self::assertArrayHasKey('nInserted', $result); | ||
| self::assertArrayHasKey('writeErrors', $result); |
There was a problem hiding this comment.
nInserted count
The assertion assertGreaterThanOrEqual(1, $result['nInserted']) would pass even if nInserted is 100. Since the setup inserts exactly two documents — one duplicate that must fail and one new one that must succeed — the count should be precisely 1 with ordered: false.
| self::assertArrayHasKey('writeErrors', $result); | |
| self::assertSame(1, $result['nInserted']); |
There was a problem hiding this comment.
Both fixed in 56fe485:
nInsertedassertion tightened toassertSame(1, ...)BulkWriteExceptionnow passeswriteErrorsto parent constructor, sogetWriteErrors()is consistent withgetResult()['writeErrors']
|
Tip: Greploops — Automatically fix all review issues by running Use the Greptile plugin for Claude Code to query reviews, search comments, and manage custom context directly from your terminal. |
1 participant
Summary
BulkWriteException(already exists but was unused) instead of genericExceptionwhenwriteErrorsare returned from MongoDBnInsertedcount and fullwriteErrorsarray in the exception result, accessible via$e->getResult()utopia-php/databaseMongo adapter) to recover partial success info frominsertManywithordered: falseTest plan
testInsertManyDuplicateThrowsBulkWriteException— verifiesBulkWriteExceptionis thrown with correctnInsertedandwriteErrors, and that non-duplicate docs persist withordered: falseException, so catches still work)Summary by CodeRabbit