Add error handling for document updates with onError callback #621
Conversation
WalkthroughThe Changes
Estimated code review effort4 (~90 minutes) Poem
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (7)
src/Database/Database.php (1)
4442-4442: Remove unused loop variable.The loop variable
$iis not used within the loop body and should be removed to improve code clarity.Apply this diff:
- foreach ($batch as $i => &$document) { + foreach ($batch as &$document) {tests/e2e/Adapter/Scopes/RelationshipTests.php (4)
1794-1803: Refactor duplicated error handling logic.The error handling callbacks are duplicated across multiple test cases. Additionally, the
instanceofcheck is redundant sinceassertInstanceOfalready handles type verification.Extract the common error handling logic into a helper method:
+ private function createErrorHandlingCallbacks(): array + { + return [ + 'onNext' => function () { + throw new Exception("Error thrown to test update doesn't stopped and error is caught"); + }, + 'onError' => function ($e) { + $this->assertInstanceOf(Exception::class, $e); + $this->assertEquals("Error thrown to test update doesn't stopped and error is caught", $e->getMessage()); + } + ]; + }Then use it in the test:
- ]), onNext: function () { - throw new Exception("Error thrown to test update doesn't stopped and error is caught"); - }, onError:function ($e) { - if ($e instanceof Exception) { - $this->assertInstanceOf(Exception::class, $e); - $this->assertEquals("Error thrown to test update doesn't stopped and error is caught", $e->getMessage()); - } else { - $this->fail("Caught value is not an Exception."); - } - }); + ]), ...$this->createErrorHandlingCallbacks());
1841-1850: Apply the same refactoring as the previous callback.This is a duplicate of the error handling logic from the first test case.
Replace with the extracted helper method:
- ]), onNext: function () { - throw new Exception("Error thrown to test update doesn't stopped and error is caught"); - }, onError:function ($e) { - if ($e instanceof Exception) { - $this->assertInstanceOf(Exception::class, $e); - $this->assertEquals("Error thrown to test update doesn't stopped and error is caught", $e->getMessage()); - } else { - $this->fail("Caught value is not an Exception."); - } - }); + ]), ...$this->createErrorHandlingCallbacks());
1854-1863: Apply the same refactoring as the previous callbacks.This is another duplicate of the error handling logic. Also note the missing space after
onError:.Replace with the extracted helper method:
- ]), onNext: function () { - throw new Exception("Error thrown to test update doesn't stopped and error is caught"); - }, onError:function ($e) { - if ($e instanceof Exception) { - $this->assertInstanceOf(Exception::class, $e); - $this->assertEquals("Error thrown to test update doesn't stopped and error is caught", $e->getMessage()); - } else { - $this->fail("Caught value is not an Exception."); - } - }); + ]), ...$this->createErrorHandlingCallbacks());
1794-1863: Consider testing more realistic error scenarios.The current tests artificially throw exceptions in
onNextto test error handling. While this verifies the mechanism works, consider adding tests that simulate actual error conditions that could occur during document updates.For example, test with invalid document data or permission issues:
// Test with invalid relationship data $this->getDatabase()->updateDocuments('testUpdateDocumentsRelationships1', new Document([ 'testUpdateDocumentsRelationships2' => 'nonexistent-id' ]), onNext: function($document) { // This would naturally fail due to invalid relationship return $document; }, onError: function($e) { $this->assertInstanceOf(RelationshipException::class, $e); });tests/e2e/Adapter/Scopes/DocumentTests.php (2)
3558-3567: Fix grammatical error in test message and improve code formatting.The error message contains a grammatical error: "doesn't stopped" should be "doesn't stop". Also, there's missing spacing around the colon in the
onErrorcallback definition, and there's a redundant assertion.- throw new Exception("Error thrown to test update doesn't stopped and error is caught"); - }, onError:function ($e) { + throw new Exception("Error thrown to test update doesn't stop and error is caught"); + }, onError: function ($e) { if ($e instanceof Exception) { - $this->assertInstanceOf(Exception::class, $e); - $this->assertEquals("Error thrown to test update doesn't stopped and error is caught", $e->getMessage()); + $this->assertEquals("Error thrown to test update doesn't stop and error is caught", $e->getMessage()); } else { $this->fail("Caught value is not an Exception."); }
3599-3608: Fix grammatical error and improve consistency with the first test case.Same issues as the first test case: grammatical error in the message, missing space in function definition, and redundant assertion.
- ]), onNext: function () { - throw new DatabaseException("Error thrown to test update doesn't stopped and error is caught"); - }, onError:function ($e) { + ]), onNext: function () { + throw new DatabaseException("Error thrown to test update doesn't stop and error is caught"); + }, onError: function ($e) { if ($e instanceof DatabaseException) { - $this->assertInstanceOf(DatabaseException::class, $e); - $this->assertEquals("Error thrown to test update doesn't stopped and error is caught", $e->getMessage()); + $this->assertEquals("Error thrown to test update doesn't stop and error is caught", $e->getMessage()); } else { - $this->fail("Caught value is not an Exception."); + $this->fail("Caught value is not a DatabaseException."); }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/Database/Database.php(3 hunks)tests/e2e/Adapter/Scopes/DocumentTests.php(2 hunks)tests/e2e/Adapter/Scopes/RelationshipTests.php(2 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
tests/e2e/Adapter/Scopes/DocumentTests.php (2)
src/Database/Exception.php (1)
Exception(7-21)src/Database/Mirror.php (1)
onError(78-81)
🪛 PHPMD (2.15.0)
src/Database/Database.php
4442-4442: Avoid unused local variables such as '$i'. (Unused Code Rules)
(UnusedLocalVariable)
⏰ 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). (1)
- GitHub Check: Setup & Build Docker Image
🔇 Additional comments (4)
src/Database/Database.php (3)
4321-4321: LGTM! Well-designed error handling parameter.The addition of the optional
$onErrorcallback parameter provides a clean way to handle exceptions during batch updates without breaking the processing flow.
4441-4448: Excellent transactional improvement for relationship consistency.Moving the relationship update logic inside the transaction ensures that both related documents and original documents are updated atomically, preventing partial updates that could leave the database in an inconsistent state.
The explicit
unset($document)after the loop is good practice to prevent memory leaks from the reference.
4459-4463: Perfect error handling implementation.The try-catch wrapper around the
$onNextcallback elegantly handles exceptions without halting the entire batch operation. This allows the system to continue processing other documents while gracefully handling errors through the$onErrorcallback.The design correctly:
- Catches exceptions from user-provided callbacks
- Continues batch processing for remaining documents
- Provides error details to the caller via
$onErrortests/e2e/Adapter/Scopes/DocumentTests.php (1)
3558-3608: Excellent test coverage for the new error handling functionality.These tests effectively verify the new
onErrorcallback mechanism works correctly for both regular exceptions and database-specific exceptions. The test pattern ensures that:
- Exceptions thrown in
onNextcallbacks are properly caught- The exception type and message are preserved
- The update operation continues despite exceptions
- Both
ExceptionandDatabaseExceptiontypes are handledThis aligns well with the PR objectives of preventing exceptions from halting entire update operations.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/Database/Mirror.php (2)
716-716: Add PHPDoc for the new parameter.Consider adding documentation for the new
$onErrorparameter to maintain consistency with the codebase's documentation standards.* @param int $batchSize * @param callable|null $onNext + * @param callable|null $onError * @return int
728-730: Consider error handling consistency for destination operations.The
$onErrorcallback is forwarded to the source database, but destination operations (lines 744-781) use the existinglogErrormethod for error handling. This creates an inconsistency where source errors use the callback but destination errors use the internal error logging mechanism.Consider whether destination operation errors should also trigger the
$onErrorcallback:} catch (\Throwable $err) { $this->logError('updateDocuments', $err); + $onError && $onError($err); }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/Database/Mirror.php(2 hunks)
⏰ 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). (11)
- GitHub Check: Adapter Tests (SQLite)
- GitHub Check: Adapter Tests (MySQL)
- GitHub Check: Adapter Tests (Mirror)
- GitHub Check: Adapter Tests (SharedTables/Postgres)
- GitHub Check: Adapter Tests (SharedTables/SQLite)
- GitHub Check: Adapter Tests (Pool)
- GitHub Check: Adapter Tests (Postgres)
- GitHub Check: Adapter Tests (MariaDB)
- GitHub Check: Adapter Tests (SharedTables/MariaDB)
- GitHub Check: Adapter Tests (SharedTables/MySQL)
- GitHub Check: Unit Test
…tions as they are dependent
ArnabChatterjee20k
force-pushed
the
dat-573
branch
from
2842365 to
5310a9a
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/e2e/Adapter/Scopes/DocumentTests.php(1 hunks)
🧬 Code Graph Analysis (1)
tests/e2e/Adapter/Scopes/DocumentTests.php (7)
src/Database/Database.php (6)
getDatabase(673-676)Database(36-6799)create(1119-1138)delete(1177-1191)count(6163-6198)updateDocuments(4288-4451)tests/e2e/Adapter/Base.php (1)
getDatabase(34-34)src/Database/Validator/Authorization.php (3)
Authorization(7-225)cleanRoles(109-112)setRole(83-86)src/Database/Helpers/Role.php (2)
Role(5-178)any(159-162)src/Database/Query.php (4)
toString(339-346)Query(8-730)greaterThanEqual(415-418)getAttribute(120-123)src/Database/Document.php (2)
Document(12-460)getAttribute(212-219)src/Database/Helpers/Permission.php (3)
Permission(9-264)read(186-195)update(220-229)
🧰 Additional context used
🧬 Code Graph Analysis (1)
tests/e2e/Adapter/Scopes/DocumentTests.php (7)
src/Database/Database.php (6)
getDatabase(673-676)Database(36-6799)create(1119-1138)delete(1177-1191)count(6163-6198)updateDocuments(4288-4451)tests/e2e/Adapter/Base.php (1)
getDatabase(34-34)src/Database/Validator/Authorization.php (3)
Authorization(7-225)cleanRoles(109-112)setRole(83-86)src/Database/Helpers/Role.php (2)
Role(5-178)any(159-162)src/Database/Query.php (4)
toString(339-346)Query(8-730)greaterThanEqual(415-418)getAttribute(120-123)src/Database/Document.php (2)
Document(12-460)getAttribute(212-219)src/Database/Helpers/Permission.php (3)
Permission(9-264)read(186-195)update(220-229)
🔇 Additional comments (1)
tests/e2e/Adapter/Scopes/DocumentTests.php (1)
3431-3523: Excellent test coverage for callback and error handling functionality.This test method provides comprehensive coverage of the new callback support in
updateDocuments:
- Proper setup: Guards against unsupported adapters, sets up authorization, and creates test data systematically
- Error scenarios: Tests both exception propagation (without
onError) and error handling (withonError)- Verification: Properly verifies both callback execution and database state changes
- Good practices: Follows testing conventions with clear assertions and cleanup
The test effectively validates the PR objectives for enhanced error handling during document updates.
ArnabChatterjee20k
force-pushed
the
dat-573
branch
from
145c9f8 to
fd0e2c8
Compare
ArnabChatterjee20k
force-pushed
the
dat-573
branch
from
fd0e2c8 to
b92b7aa
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
tests/e2e/Adapter/Scopes/Relationships/ManyToManyTests.php (2)
1612-1628: Consider clarifying the relationship ID naming.The relationship uses
id: 'parentNumber'but this creates a many-to-many relationship where the attribute name might be confusing since it suggests a single parent number rather than a collection of related documents.Consider using a more descriptive relationship ID like
'children'or'relatedChildren'to better reflect the many-to-many nature.$database->createRelationship( collection: $parentCollection, relatedCollection: $childCollection, type: Database::RELATION_MANY_TO_MANY, - id: 'parentNumber' + id: 'children' );
1620-1622: Remove unnecessary blank line.There's an extra blank line that should be removed for consistency.
$database->createAttribute($childCollection, 'parentNumber', Database::VAR_INTEGER, 0, false); - - + $database->createRelationship(tests/e2e/Adapter/Scopes/Relationships/OneToOneTests.php (1)
2292-2373: Well-structured test for one-to-one relationship batch updates.This test effectively verifies batch update operations and error handling for one-to-one relationships. The test structure is comprehensive and follows good testing practices.
However, consider these improvements:
Test the new error callback feature: The PR introduces an
$onErrorcallback parameter forupdateDocuments, but this test doesn't exercise this new functionality.Verify relationship functionality: The test creates a relationship but doesn't verify that documents are actually linked through the relationship.
Consider adding a test case that demonstrates the new error callback functionality:
+ // Test error callback functionality + $errorMessages = []; + $database->updateDocuments( + $childCollection, + new Document(['parentNumber' => 'not-a-number']), + [Query::equal('$id', ['child1'])], + onNext: function($document) { + // This should not be called due to validation error + }, + onError: function($error, $document) use (&$errorMessages) { + $errorMessages[] = $error->getMessage(); + } + ); + + $this->assertCount(1, $errorMessages, 'Error callback should capture validation errors'); + $this->assertInstanceOf(StructureException::class, $errorMessages[0]);Also consider testing the actual relationship linking:
+ // Test relationship linking + $database->updateDocuments( + $childCollection, + new Document(['parentNumber' => 1]), // Assuming parent1 has sequence number 1 + [Query::equal('$id', ['child1'])] + ); + + $parentDoc = $database->getDocument($parentCollection, 'parent1'); + $childDoc = $database->getDocument($childCollection, 'child1'); + + $this->assertNotNull($parentDoc->getAttribute('parentNumber'), 'Parent should have related child'); + $this->assertEquals(1, $childDoc->getAttribute('parentNumber'), 'Child should reference parent');
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
tests/e2e/Adapter/Scopes/Relationships/ManyToManyTests.php(2 hunks)tests/e2e/Adapter/Scopes/Relationships/ManyToOneTests.php(2 hunks)tests/e2e/Adapter/Scopes/Relationships/OneToManyTests.php(2 hunks)tests/e2e/Adapter/Scopes/Relationships/OneToOneTests.php(1 hunks)
🧬 Code Graph Analysis (1)
tests/e2e/Adapter/Scopes/Relationships/ManyToOneTests.php (5)
src/Database/Database.php (1)
Database(36-6799)src/Database/Exception.php (1)
Exception(7-21)src/Database/Helpers/Permission.php (3)
Permission(9-264)read(186-195)update(220-229)src/Database/Helpers/Role.php (2)
Role(5-178)any(159-162)src/Database/Query.php (1)
equal(355-358)
🧰 Additional context used
🧬 Code Graph Analysis (1)
tests/e2e/Adapter/Scopes/Relationships/ManyToOneTests.php (5)
src/Database/Database.php (1)
Database(36-6799)src/Database/Exception.php (1)
Exception(7-21)src/Database/Helpers/Permission.php (3)
Permission(9-264)read(186-195)update(220-229)src/Database/Helpers/Role.php (2)
Role(5-178)any(159-162)src/Database/Query.php (1)
equal(355-358)
🔇 Additional comments (13)
tests/e2e/Adapter/Scopes/Relationships/ManyToOneTests.php (2)
9-9: LGTM - Import added for Structure exception handling.The import is correctly added to support the new test method's exception handling for invalid batch updates.
1681-1761: Excellent comprehensive test for batch update error handling.This test method effectively validates the new error handling functionality for batch updates in many-to-one relationships. The test covers:
- Setup verification: Properly checks adapter support for both relationships and batch operations
- Successful batch update: Verifies that valid updates work correctly and don't affect unrelated documents
- Error isolation: Confirms that failed batch updates (Structure exceptions) don't impact unrelated documents
- Proper cleanup: Ensures test collections are deleted after execution
The test aligns perfectly with the PR objectives of improving error handling during document updates, specifically ensuring that exceptions in batch operations are properly contained and don't affect other documents.
tests/e2e/Adapter/Scopes/Relationships/ManyToManyTests.php (6)
9-9: LGTM! Appropriate import for validation testing.The
Structureexception import is correctly added to support the data validation test scenario in the new test method.
1599-1610: LGTM! Proper adapter capability checks.The test correctly checks for both relationship and batch operation support before proceeding, with appropriate early return.
1630-1650: LGTM! Proper document creation for test setup.The test documents are created with appropriate permissions and initial data for the relationship testing scenario.
1651-1662: LGTM! Effective batch update testing.The test properly validates that batch updates work correctly and that unrelated documents remain unchanged, which is essential for relationship integrity testing.
1663-1673: LGTM! Robust error handling validation.The test correctly validates that invalid data types throw the appropriate
Structureexception, which is crucial for data integrity in relationships.
1675-1681: LGTM! Proper verification and cleanup.The test appropriately verifies that failed updates don't affect other documents and includes proper cleanup by deleting the test collections.
tests/e2e/Adapter/Scopes/Relationships/OneToManyTests.php (5)
2076-2087: LGTM! Proper adapter support validation.The method correctly checks for both relationship and batch operation support before proceeding with the test, following the established pattern used throughout the test suite.
2127-2137: LGTM! Valid batch update test is well-structured.The test correctly verifies that batch updates on the parent collection work as expected and don't inadvertently affect related child documents.
2139-2149: Excellent error handling validation.This test segment properly validates that invalid batch updates (setting an integer field to a string) correctly throw a
Structureexception, which aligns perfectly with the PR objectives of improving error handling in batch operations.
2151-2157: LGTM! Proper verification and cleanup.The test correctly verifies that the parent document remains unaffected by failed child updates, demonstrating transactional integrity. The cleanup properly removes the test collections.
2089-2104: Verify one-to-many relationship ID usageIt looks like you’re using
parentNumberfor both the child-collection attribute and as theidin yourcreateRelationshipcall. Since this pattern doesn’t appear elsewhere in the tests, please confirm that:
parentNumberonchild_combined_o2mis intended to serve as the foreign-key field- Utopia Database will correctly interpret this attribute as the one-to-many relationship key
File: tests/e2e/Adapter/Scopes/Relationships/OneToManyTests.php (lines 2089–2104)
onNextcallableonNextpassing them to theonErrorcallbackSummary by CodeRabbit
New Features
Refactor