Add createOrUpdateDocuments method and filter hooks for document upserts #663
Conversation
WalkthroughAdds Mirror::createOrUpdateDocuments to mirror batch upserts from source to destination with lifecycle filters and upgrade checks; extends Filter with before/after upsert hooks; adds an e2e test validating createOrUpdateDocuments and updateDocuments emit and return correct counts. Changes
Sequence Diagram(s)sequenceDiagram
participant C as Caller
participant M as Mirror
participant S as Source DB
participant F as Filters
participant D as Destination DB
C->>M: createOrUpdateDocuments(collection, docs, batchSize, onNext)
M->>S: createOrUpdateDocuments(...)
loop each processed doc
S-->>M: onNext(doc)
M->>C: onNext(doc)
M->>M: increment modified count
end
M->>M: check SOURCE_ONLY / destination / upgrade status
alt upgraded && destination available
M->>F: beforeCreateOrUpdateDocument(cloned doc) for each doc
M->>D: withPreserveDates(createOrUpdateDocuments(cloned docs))
D-->>M: ack/result
M->>F: afterCreateOrUpdateDocument(cloned doc) for each doc
else
M->>M: skip destination write
end
M-->>C: return modified count
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested reviewers
Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ 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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
Status, 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 (5)
tests/e2e/Adapter/Scopes/DocumentTests.php (2)
5797-5800: Also guard on batch-operations support to avoid false positivesThis test calls updateDocuments, which some adapters gate behind getSupportForBatchOperations. Since you already guard on upsert support, add the batch-ops guard to keep the test portable across adapters.
Apply this diff:
if (!$database->getAdapter()->getSupportForUpserts()) { return; } + + if (!$database->getAdapter()->getSupportForBatchOperations()) { + $this->expectNotToPerformAssertions(); + return; + }
5838-5846: Optional: Assert one updated field to increase test confidenceRight now we only assert counts. Adding a quick check that 'value' actually changed to 'test' improves the signal of what the count represents without much overhead.
For example (after Line 5846):
$found = $database->find($collectionName); foreach ($found as $doc) { $this->assertSame('test', $doc->getAttribute('value')); }src/Database/Mirroring/Filter.php (1)
363-397: Suppress PHPMD unused-parameter noise for no-op defaultsPHPMD flags these parameters as unused, which is expected for default no-op implementations in an abstract filter. Suppress to keep CI quiet.
Apply this diff to annotate both methods:
/** * Called before document is upserted in the destination database * * @param Database $source * @param Database $destination * @param string $collectionId * @param Document $document * @return Document */ + /** @SuppressWarnings(PHPMD.UnusedFormalParameter) */ public function beforeCreateOrUpdateDocument( Database $source, Database $destination, string $collectionId, Document $document, ): Document { return $document; } /** * Called after document is upserted in the destination database * * @param Database $source * @param Database $destination * @param string $collectionId * @param Document $document * @return Document */ + /** @SuppressWarnings(PHPMD.UnusedFormalParameter) */ public function afterCreateOrUpdateDocument( Database $source, Database $destination, string $collectionId, Document $document, ): Document { return $document; }src/Database/Mirror.php (2)
784-785: Unify signature style and default constant with existing bulk APIsFor consistency with createDocuments/updateDocuments in this class, prefer self::INSERT_BATCH_SIZE and the short nullable form for the callback.
Apply this diff:
- public function createOrUpdateDocuments(string $collection, array $documents, int $batchSize = Database::INSERT_BATCH_SIZE, callable|null $onNext = null): int + public function createOrUpdateDocuments(string $collection, array $documents, int $batchSize = self::INSERT_BATCH_SIZE, ?callable $onNext = null): int
847-849: Fix incorrect error action string in loggingThe action name should match the method for accurate diagnostics.
Apply this diff:
- $this->logError('createDocuments', $err); + $this->logError('createOrUpdateDocuments', $err);
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
src/Database/Mirror.php(1 hunks)src/Database/Mirroring/Filter.php(1 hunks)tests/e2e/Adapter/Scopes/DocumentTests.php(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
tests/e2e/Adapter/Scopes/DocumentTests.php (9)
tests/e2e/Adapter/Base.php (1)
getDatabase(34-34)src/Database/Database.php (7)
getDatabase(681-684)getAdapter(1103-1106)createCollection(1232-1350)createAttribute(1580-1631)Database(36-6957)count(6286-6322)createOrUpdateDocuments(4959-4972)src/Database/Adapter.php (6)
getDatabase(155-158)getSupportForUpserts(1002-1002)createCollection(505-505)createAttribute(537-537)count(805-805)createOrUpdateDocuments(731-735)src/Database/Adapter/MariaDB.php (3)
getSupportForUpserts(1885-1888)createCollection(77-222)count(1528-1588)src/Database/Adapter/Postgres.php (4)
getSupportForUpserts(2047-2050)createCollection(189-329)createAttribute(440-459)count(1618-1679)src/Database/Adapter/Pool.php (5)
getSupportForUpserts(403-406)createCollection(153-156)createAttribute(168-171)count(273-276)createOrUpdateDocuments(248-251)src/Database/Adapter/SQLite.php (2)
getSupportForUpserts(941-944)createCollection(141-226)src/Database/Adapter/SQL.php (2)
createAttribute(217-232)createOrUpdateDocuments(1957-2107)src/Database/Helpers/Role.php (2)
Role(5-178)any(159-162)
src/Database/Mirror.php (5)
src/Database/Database.php (4)
createOrUpdateDocuments(4959-4972)Database(36-6957)silent(518-537)withPreserveDates(1029-1039)src/Database/Adapter/SQL.php (1)
createOrUpdateDocuments(1957-2107)src/Database/Adapter.php (1)
createOrUpdateDocuments(731-735)src/Database/Document.php (1)
getAttribute(224-231)src/Database/Mirroring/Filter.php (2)
beforeCreateOrUpdateDocument(372-379)afterCreateOrUpdateDocument(390-397)
🪛 Gitleaks (8.27.2)
tests/e2e/Adapter/Scopes/DocumentTests.php
5813-5813: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
5818-5818: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
5823-5823: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
5828-5828: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
🪛 PHPMD (2.15.0)
src/Database/Mirroring/Filter.php
373-373: Avoid unused parameters such as '$source'. (Unused Code Rules)
(UnusedFormalParameter)
374-374: Avoid unused parameters such as '$destination'. (Unused Code Rules)
(UnusedFormalParameter)
375-375: Avoid unused parameters such as '$collectionId'. (Unused Code Rules)
(UnusedFormalParameter)
391-391: Avoid unused parameters such as '$source'. (Unused Code Rules)
(UnusedFormalParameter)
392-392: Avoid unused parameters such as '$destination'. (Unused Code Rules)
(UnusedFormalParameter)
393-393: Avoid unused parameters such as '$collectionId'. (Unused Code Rules)
(UnusedFormalParameter)
🔇 Additional comments (2)
src/Database/Mirroring/Filter.php (1)
363-397: Upsert hooks look goodThe new beforeCreateOrUpdateDocument/afterCreateOrUpdateDocument hooks align with the existing create/update hooks and provide the right extension points for mirroring upserts. Default pass-through implementations are appropriate.
src/Database/Mirror.php (1)
827-836: Confirm intended count semantics (source vs. destination)Here, like createDocuments, you ignore the destination return value and keep the source-modified count. In updateDocuments you overwrite $modified with the destination result. If this asymmetry is intentional, all good—otherwise consider aligning for predictability.
Would you like me to normalize all three bulk methods to return the source count only (and ignore the destination), or unify on destination count? I can prep a follow-up patch accordingly.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
tests/e2e/Adapter/Scopes/DocumentTests.php (4)
5797-5800: Guard skipped test with expectNotToPerformAssertions()Several other tests in this suite use expectNotToPerformAssertions() when feature-gating. This one returns early without it, which can trip PHPUnit configs that fail on tests with zero assertions.
Apply this diff:
- if (!$database->getAdapter()->getSupportForUpserts()) { - return; - } + if (!$database->getAdapter()->getSupportForUpserts()) { + $this->expectNotToPerformAssertions(); + return; + }
5838-5846: Strengthen assertions: verify mutation result and persistenceCurrently we only assert counts. Add assertions to:
- ensure onNext returns the updated documents with the new value
- verify the update persisted to storage (e.g., via a follow-up find)
This aligns with the style used by testUpdateDocuments and similar tests above.
Apply this diff:
$updates = new Document(['value' => 'test']); $newDocs = []; $count = $database->updateDocuments($collectionName, $updates, onNext:function ($doc) use (&$newDocs) { $newDocs[] = $doc; }); $this->assertCount(4, $newDocs); $this->assertEquals(4, $count); + + // Ensure updated documents have the expected value + foreach ($newDocs as $doc) { + $this->assertEquals('test', $doc->getAttribute('value')); + } + // Ensure persistence layer reflects the update + $persisted = $database->find($collectionName, [ + Query::equal('value', ['test']) + ]); + $this->assertCount(4, $persisted);
5804-5829: Ignore Gitleaks “Generic API Key” false positives for test dataStatic analysis flagged “Generic API Key” due to the attribute named “key” and sample values (bulk_upsert*_initial). These are harmless test fixtures and not secrets.
Consider adding an allowlist entry in your gitleaks config for:
- this test file path, or
- a regex like
"bulk_upsert[0-9]+_initial"and/or attribute"\"key\"\s*:"within tests.If you prefer to avoid allowlists, renaming the attribute to something like test_key would also quell the detector.
5807-5829: Remove redundant permissions in test fixturesYou’re granting both write(any) and update(any). write implies create+update+delete, so update is redundant.
You can simplify:
- $permissions = [Permission::read(Role::any()), Permission::write(Role::any()),Permission::update(Role::any())]; + $permissions = [Permission::read(Role::any()), Permission::write(Role::any())];
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
src/Database/Mirror.php(1 hunks)tests/e2e/Adapter/Scopes/DocumentTests.php(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/Database/Mirror.php
🧰 Additional context used
🧬 Code Graph Analysis (1)
tests/e2e/Adapter/Scopes/DocumentTests.php (9)
tests/e2e/Adapter/Base.php (1)
getDatabase(34-34)src/Database/Adapter.php (7)
getDatabase(155-158)getSupportForUpserts(1002-1002)createCollection(505-505)createAttribute(537-537)count(805-805)createOrUpdateDocuments(731-735)updateDocuments(719-719)src/Database/Adapter/MariaDB.php (3)
getSupportForUpserts(1885-1888)createCollection(77-222)count(1528-1588)src/Database/Adapter/Postgres.php (4)
getSupportForUpserts(2047-2050)createCollection(189-329)createAttribute(440-459)count(1618-1679)src/Database/Adapter/SQLite.php (2)
getSupportForUpserts(941-944)createCollection(141-226)src/Database/Adapter/Pool.php (6)
getSupportForUpserts(403-406)createCollection(153-156)createAttribute(168-171)count(273-276)createOrUpdateDocuments(248-251)updateDocuments(243-246)src/Database/Adapter/SQL.php (3)
createAttribute(217-232)createOrUpdateDocuments(1957-2107)updateDocuments(413-646)src/Database/Helpers/Permission.php (4)
Permission(9-264)read(186-195)write(254-263)update(220-229)src/Database/Helpers/Role.php (2)
Role(5-178)any(159-162)
🪛 Gitleaks (8.27.2)
tests/e2e/Adapter/Scopes/DocumentTests.php
5813-5813: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
5818-5818: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
5823-5823: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
5828-5828: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
Summary by CodeRabbit
New Features
Tests