Pass old to update on next, rename to upsert #685
Conversation
|
Warning Rate limit exceeded@abnegate has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 15 minutes and 45 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (2)
WalkthroughPublic API rename from createOrUpdateDocuments() to upsertDocuments() across Adapter, Pool, SQL adapter, Database, and Mirror. Database adds an optional onError callback, introduces upsertDocument helper, and changes onNext to receive (new, old). Mirror and tests updated to use the new names and callback shape. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Client
participant DB as Database
participant AD as Adapter (SQL/Pool/…)
Client->>DB: upsertDocuments(collection, documents, batchSize, onNext?, onError?)
loop For each batch
DB->>DB: snapshot old state per document
DB->>AD: upsertDocuments(collectionDoc, attribute?, changes)
AD-->>DB: results[]
loop For each result
DB->>DB: try onNext(newDoc, oldDoc)
alt onNext throws and onError provided
DB->>Client: onError(error, newDoc, oldDoc)
else onNext throws and no onError
DB-->>Client: throw error
end
end
end
DB-->>Client: totalCount
note right of DB: upsertDocument delegates to upsertDocumentsWithIncrease for single doc
sequenceDiagram
autonumber
participant SRC as Mirror (source DB)
participant DST as Mirror (destination DB)
participant AD as Adapter
participant Hooks as Filters
SRC->>SRC: upsertDocuments(collection, docs, batch, onNext?, onError?)
SRC->>DST: upsertDocuments(collection, docs, batch, onNext?, onError?)
loop Each document
DST->>Hooks: beforeCreateOrUpdateDocument(doc)
DST->>AD: upsertDocuments(collectionDoc, attribute?, changes)
AD-->>DST: newDoc
DST->>Hooks: afterCreateOrUpdateDocument(newDoc)
DST->>SRC: onNext(newDoc, oldDoc)
alt onNext throws
SRC->>SRC: onError?(error, newDoc, oldDoc) or throw
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
✨ 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: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
tests/e2e/Adapter/Scopes/DocumentTests.php (1)
535-541: Make onNext callback arity-aware in upsertDocuments
In both src/Database/Mirror.php (around lines 796–799) and src/Database/Database.php’s upsertDocumentsWithIncrease implementation (around line 5337), the code currently always calls onNext($doc, $old). That will throw an ArgumentCountError for existing one-parameter closures in tests (e.g. tests/e2e/Adapter/Scopes/DocumentTests.php at lines 640–643, 5569–5571, 5600–5602, etc.). Update those sites to inspect the callable’s parameter count and invoke it with only the required arguments (one or two) to maintain backward compatibility.src/Database/Database.php (1)
4621-4641: Bug: timestamp conflict check uses the new document’s updatedAt instead of the original.
This can falsely throw ConflictException (especially when $updates sets $updatedAt to now). Capture/check the original updatedAt before overwriting $document.Apply this diff inside the foreach ($batch as $index => $document) loop:
- $new = new Document(\array_merge($document->getArrayCopy(), $updates->getArrayCopy())); + $new = new Document(\array_merge($document->getArrayCopy(), $updates->getArrayCopy())); + $originalUpdatedAt = $document->getUpdatedAt(); @@ - $document = $new; - - // Check if document was updated after the request timestamp - try { - $oldUpdatedAt = new \DateTime($document->getUpdatedAt()); + $document = $new; + // Check if document was updated after the request timestamp (use original value) + try { + $oldUpdatedAt = new \DateTime($originalUpdatedAt); } catch (Exception $e) { throw new DatabaseException($e->getMessage(), $e->getCode(), $e); }
🧹 Nitpick comments (4)
src/Database/Adapter/SQL.php (1)
2093-2249: Rename implemented; consider stabilizing columns across changes and handling spatial WKT explicitly.Two optional hardening tweaks:
- Ensure every Change has identical, sorted attribute keys; otherwise $columns (from the last item) may not match earlier bind tuples.
- For spatial attributes, bind WKT (convertArrayToWKT) rather than JSON to avoid relying on engine-specific coercion.
Apply minimal validation and WKT binding:
@@ - $attributes = []; + $attributes = []; $bindIndex = 0; $batchKeys = []; $bindValues = []; + $firstKeys = null; + $columnsSQL = ''; @@ - \ksort($attributes); + \ksort($attributes); + $keys = \array_keys($attributes); + if ($firstKeys === null) { + $firstKeys = $keys; + $cols = []; + foreach ($firstKeys as $key => $attr) { + /** @var string $attr */ + $cols[$key] = "{$this->quote($this->filter($attr))}"; + } + $columnsSQL = '(' . \implode(', ', $cols) . ')'; + } elseif ($keys !== $firstKeys) { + throw new DatabaseException('All upsert changes must have identical attribute sets'); + } - - $columns = []; - foreach (\array_keys($attributes) as $key => $attr) { - /** - * @var string $attr - */ - $columns[$key] = "{$this->quote($this->filter($attr))}"; - } - $columns = '(' . \implode(', ', $columns) . ')'; @@ - foreach ($attributes as $attributeKey => $attrValue) { - if (\is_array($attrValue)) { - $attrValue = \json_encode($attrValue); - } - - if (in_array($attributeKey, $spatialAttributes)) { + foreach ($attributes as $attributeKey => $attrValue) { + if (in_array($attributeKey, $spatialAttributes)) { + // Convert array geometry to WKT for ST_GeomFromText + if (\is_array($attrValue)) { + $attrValue = $this->convertArrayToWKT($attrValue); + } $bindKey = 'key_' . $bindIndex; $bindKeys[] = "ST_GeomFromText(:" . $bindKey . ")"; } else { + if (\is_array($attrValue)) { + $attrValue = \json_encode($attrValue); + } $attrValue = (\is_bool($attrValue)) ? (int)$attrValue : $attrValue; $bindKey = 'key_' . $bindIndex; $bindKeys[] = ':' . $bindKey; } @@ - $stmt = $this->getUpsertStatement($name, $columns, $batchKeys, $attributes, $bindValues, $attribute); + $stmt = $this->getUpsertStatement($name, $columnsSQL, $batchKeys, $attributes, $bindValues, $attribute);To validate uniform attribute keys across generated Change objects, inspect the builder in Database::upsertDocumentsWithIncrease and ensure it normalizes/ksorts keys for all changes.
src/Database/Mirror.php (2)
788-790: Optional: propagate onError to destination too.If you want parity with Database API, pass $onError to destination as well. Today destination errors are only logged via logError.
Example:
- $this->destination->upsertDocuments( + $this->destination->upsertDocuments( $collection, $clones, $batchSize, - null, + null, + $onError, )
832-840: Optional: align returned count source vs destination across Mirror methods.createDocuments returns source count; update/delete/upsert return destination count. Consider standardizing on source count to avoid surprises.
src/Database/Database.php (1)
4663-4666: Limit handling: prefer >= to avoid edge cases.
Use >= like deleteDocuments does; prevents overshoot/looping if counts diverge.- } elseif ($originalLimit && $modified == $originalLimit) { + } elseif ($originalLimit && $modified >= $originalLimit) {
📜 Review details
Configuration used: Path: .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 ignored due to path filters (1)
composer.lockis excluded by!**/*.lock
📒 Files selected for processing (8)
src/Database/Adapter.php(1 hunks)src/Database/Adapter/Pool.php(1 hunks)src/Database/Adapter/SQL.php(1 hunks)src/Database/Database.php(21 hunks)src/Database/Mirror.php(4 hunks)tests/e2e/Adapter/Scopes/DocumentTests.php(28 hunks)tests/e2e/Adapter/Scopes/GeneralTests.php(3 hunks)tests/e2e/Adapter/Scopes/SpatialTests.php(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (8)
src/Database/Adapter/Pool.php (2)
src/Database/Adapter.php (1)
upsertDocuments(752-756)src/Database/Adapter/SQL.php (1)
upsertDocuments(2093-2250)
tests/e2e/Adapter/Scopes/GeneralTests.php (5)
src/Database/Database.php (1)
upsertDocuments(5110-5125)src/Database/Mirror.php (1)
upsertDocuments(784-856)src/Database/Adapter.php (1)
upsertDocuments(752-756)src/Database/Adapter/Pool.php (1)
upsertDocuments(248-251)src/Database/Adapter/SQL.php (1)
upsertDocuments(2093-2250)
src/Database/Adapter/SQL.php (4)
src/Database/Database.php (1)
upsertDocuments(5110-5125)src/Database/Mirror.php (1)
upsertDocuments(784-856)src/Database/Adapter.php (1)
upsertDocuments(752-756)src/Database/Adapter/Pool.php (1)
upsertDocuments(248-251)
tests/e2e/Adapter/Scopes/SpatialTests.php (5)
src/Database/Database.php (1)
upsertDocuments(5110-5125)src/Database/Mirror.php (1)
upsertDocuments(784-856)src/Database/Adapter.php (1)
upsertDocuments(752-756)src/Database/Adapter/Pool.php (1)
upsertDocuments(248-251)src/Database/Adapter/SQL.php (1)
upsertDocuments(2093-2250)
src/Database/Adapter.php (4)
src/Database/Database.php (1)
upsertDocuments(5110-5125)src/Database/Mirror.php (1)
upsertDocuments(784-856)src/Database/Adapter/Pool.php (1)
upsertDocuments(248-251)src/Database/Adapter/SQL.php (1)
upsertDocuments(2093-2250)
tests/e2e/Adapter/Scopes/DocumentTests.php (5)
src/Database/Database.php (3)
count(6450-6486)upsertDocuments(5110-5125)upsertDocumentsWithIncrease(5141-5356)src/Database/Adapter.php (2)
count(825-825)upsertDocuments(752-756)src/Database/Adapter/Pool.php (2)
count(273-276)upsertDocuments(248-251)src/Database/Adapter/SQL.php (2)
count(2521-2583)upsertDocuments(2093-2250)src/Database/Mirror.php (1)
upsertDocuments(784-856)
src/Database/Database.php (7)
src/Database/Document.php (5)
getPermissions(93-96)removeAttribute(287-293)getId(63-66)Document(12-470)isEmpty(396-399)src/Database/Mirror.php (2)
upsertDocuments(784-856)onError(78-81)src/Database/Adapter.php (2)
upsertDocuments(752-756)withTransaction(377-425)src/Database/Adapter/Pool.php (1)
upsertDocuments(248-251)src/Database/Adapter/SQL.php (1)
upsertDocuments(2093-2250)src/Database/Validator/Authorization.php (2)
Authorization(7-225)skip(160-170)src/Database/Change.php (1)
getOld(13-16)
src/Database/Mirror.php (4)
src/Database/Database.php (2)
upsertDocuments(5110-5125)Database(37-7274)src/Database/Adapter.php (1)
upsertDocuments(752-756)src/Database/Adapter/Pool.php (1)
upsertDocuments(248-251)src/Database/Adapter/SQL.php (1)
upsertDocuments(2093-2250)
🪛 PHPMD (2.15.0)
src/Database/Mirror.php
789-789: Avoid unused parameters such as '$onError'. (Unused Code Rules)
(UnusedFormalParameter)
🔇 Additional comments (21)
tests/e2e/Adapter/Scopes/GeneralTests.php (3)
515-520: Rename to upsertDocuments looks correct.Call site matches the new Database API and existing gating by getSupportForUpserts().
553-562: LGTM on API rename (batch upsert for multiple tenants).Consistent with the new method. No behavioral changes introduced here.
585-593: LGTM on API rename (updates via upsert).Call site aligns with the new upsertDocuments API.
tests/e2e/Adapter/Scopes/SpatialTests.php (1)
1584-1584: Comment update matches the API rename.No action needed.
src/Database/Adapter/Pool.php (1)
248-251: Delegation method renamed correctly.Matches Adapter::upsertDocuments and routes via FUNCTION as expected.
src/Database/Adapter.php (1)
752-756: Abstract rename aligns with the new API.Signature and docblock remain coherent with “upsert” semantics.
src/Database/Mirror.php (1)
820-827: Guard new filter hooks to prevent undefined method errors
Ensure calls to beforeCreateOrUpdateDocument and afterCreateOrUpdateDocument are wrapped in method_exists checks, falling back to the legacy beforeCreateDocument/afterCreateDocument if the new methods aren’t implemented:--- a/src/Database/Mirror.php +++ b/src/Database/Mirror.php @@ lines 820-827 - foreach ($this->writeFilters as $filter) { - $clone = $filter->beforeCreateOrUpdateDocument( + foreach ($this->writeFilters as $filter) { + if (method_exists($filter, 'beforeCreateOrUpdateDocument')) { + $clone = $filter->beforeCreateOrUpdateDocument( source: $this->source, destination: $this->destination, collectionId: $collection, document: $clone, - ); + ); + } else { + $clone = $filter->beforeCreateDocument( + source: $this->source, + destination: $this->destination, + collectionId: $collection, + document: $clone, + ); + } } @@ lines 843-850 - foreach ($this->writeFilters as $filter) { - $filter->afterCreateOrUpdateDocument( + foreach ($this->writeFilters as $filter) { + if (method_exists($filter, 'afterCreateOrUpdateDocument')) { + $filter->afterCreateOrUpdateDocument( source: $this->source, destination: $this->destination, collectionId: $collection, document: $clone, - ); + ); + } else { + $filter->afterCreateDocument( + source: $this->source, + destination: $this->destination, + collectionId: $collection, + document: $clone, + ); + } }Applies to both the create-or-update and update-or-create loops.
src/Database/Database.php (14)
63-63: Formatting nit: SPATIAL_TYPES list looks good.
No behavior change; consistent with surrounding constants.
3829-3829: Docblock matches callback shape in createDocuments.
The parameter type for onNext reflects the actual callback usage.
4286-4292: Permissions comparison guard is correct.
Sorting and comparing arrays to decide skipPermissionsUpdate is fine and matches similar logic elsewhere.
4476-4495: UpdateDocuments: new callback signatures are clear.
Adding onError and passing $old to onNext is a sensible API improvement.
4650-4658: onNext/onError wrapping is correct.
Cloned $old by index and try/catch with onError is good.
5110-5125: upsertDocuments delegating to withIncrease (with onError) is clean.
Signature and forwarding order look correct.
5141-5150: upsertDocumentsWithIncrease: API docs and parameters look good.
onNext($new, ?$old) and optional onError are clear.
5308-5313: Authorization::skip within transaction is appropriate here.
Adapter upsert executes atomically without read permission requirements.
5324-5345: Post-upsert callback semantics are correct.
Decoding before invoking onNext and providing nullable $old matches the contract; try/catch routes to onError properly.
6012-6026: deleteDocuments: improved callbacks (onNext with $old, onError).
API symmetry with update/upsert; looks good.
6105-6155: Cloning $old and purging cache per-tenant is sound.
Index alignment and error handling are fine.
6562-6576: Encode: normalize empty internal date strings to null.
Good robustness improvement; avoids invalid dates leaking into storage.
6978-6992: convertQuery: guard on empty attribute is correct.
Prevents accessing properties on empty Document; keeps datetime normalization intact.
7236-7260: decodeSpatialData: safer closing-paren handling.
Using strrpos avoids mismatches for nested parentheses; OK for POINT/LINESTRING/POLYGON.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/Database/Database.php (1)
5086-5093: Past concern resolved: closure arity and non-null return are fixed.
- Closure now accepts two args; null-result fallback ensures a Document is returned.
Also applies to: 5095-5100
🧹 Nitpick comments (2)
src/Database/Database.php (2)
5190-5199: Improve no-op detection by filling optionals before comparing with old.
Currently, the equality check may miss no-ops when caller omits optional fields present in $old. Move the “Force matching optional parameter sets” block before the comparison.Apply:
- if ( - empty($attribute) - && $skipPermissionsUpdate - && $old->getAttributes() == $document->getAttributes() - ) { - // If not updating a single attribute and the - // document is the same as the old one, skip it - unset($documents[$key]); - continue; - } + // Normalize optionals first to compare apples-to-apples + foreach ($collectionAttributes as $attr) { + if (!$attr->getAttribute('required') && !\array_key_exists($attr['$id'], (array)$document)) { + $document->setAttribute( + $attr['$id'], + $old->getAttribute($attr['$id'], ($attr['default'] ?? null)) + ); + } + } + if ( + empty($attribute) + && $skipPermissionsUpdate + && $old->getAttributes() == $document->getAttributes() + ) { + unset($documents[$key]); + continue; + } @@ - // Force matching optional parameter sets - // Doesn't use decode as that intentionally skips null defaults to reduce payload size - foreach ($collectionAttributes as $attr) { - if (!$attr->getAttribute('required') && !\array_key_exists($attr['$id'], (array)$document)) { - $document->setAttribute( - $attr['$id'], - $old->getAttribute($attr['$id'], ($attr['default'] ?? null)) - ); - } - }Also applies to: 5237-5246
7241-7241: Minor parsing robustness for decodeSpatialData.
Use preg_split('/\s+/') for coords to tolerate extra whitespace; trim() around substrings.Apply (illustrative):
- $coords = explode(' ', trim($inside)); + $coords = preg_split('/\s+/', trim($inside)); @@ - $points = explode(',', $inside); + $points = array_map('trim', explode(',', $inside)); @@ - $rings = explode('),(', $inside); + $rings = array_map('trim', explode('),(', $inside));Also applies to: 7251-7251, 7264-7264
📜 Review details
Configuration used: Path: .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 (1)
src/Database/Database.php(21 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/Database/Database.php (6)
src/Database/Document.php (4)
getPermissions(93-96)removeAttribute(287-293)getId(63-66)Document(12-470)src/Database/Adapter.php (2)
upsertDocuments(752-756)withTransaction(377-425)src/Database/Adapter/SQL.php (1)
upsertDocuments(2093-2250)src/Database/Mirror.php (2)
upsertDocuments(784-856)onError(78-81)src/Database/Validator/Authorization.php (2)
Authorization(7-225)skip(160-170)src/Database/Change.php (1)
getOld(13-16)
🪛 PHPMD (2.15.0)
src/Database/Database.php
5090-5090: Avoid unused parameters such as '$_old'. (Unused Code Rules)
(UnusedFormalParameter)
⏰ 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 (Mirror)
- GitHub Check: Adapter Tests (MySQL)
- GitHub Check: Adapter Tests (Postgres)
- GitHub Check: Adapter Tests (SharedTables/Postgres)
- GitHub Check: Adapter Tests (SharedTables/MySQL)
- GitHub Check: Adapter Tests (MariaDB)
- GitHub Check: Adapter Tests (Pool)
- GitHub Check: Adapter Tests (SharedTables/SQLite)
- GitHub Check: Adapter Tests (SharedTables/MariaDB)
- GitHub Check: Adapter Tests (SQLite)
- GitHub Check: Unit Test
🔇 Additional comments (9)
src/Database/Database.php (9)
63-63: LGTM: SPATIAL_TYPES formatting is fine.
3829-3829: Docblock OK: createDocuments onNext(Document) type is accurate.
4476-4478: Callback shape standardization (onNext(new, old), optional onError) looks good.
- Matches usages later in method bodies; consistent across update/upsert/delete.
Please verify all call sites across the repo now pass two params to onNext for updateDocuments, upsertDocuments, upsertDocumentsWithIncrease, and deleteDocuments.
Also applies to: 5108-5110, 5114-5120, 5137-5151, 6016-6018
4600-4601: Good: clone old state and invoke onNext(updated, old) with error routing.Also applies to: 4650-4656
5114-5129: LGTM: upsertDocuments delegates correctly including onError and batchSize.
5312-5316: Transaction + per-doc callbacks look solid; confirm adapter order guarantee.
- Mapping $batch[$index] to $chunk[$index] assumes adapter preserves order and cardinality.
Confirm the adapter contract specifies stable ordering. If not guaranteed, consider associating old by ID to decouple from ordering.
Also applies to: 5329-5349
6565-6579: Nice: normalize empty date strings to null before encoding.
6982-6996: Safer convertQuery: only transform when attribute is known.
6016-6018: LGTM: deleteDocuments onNext(old included), cloning old state, error routing.Also applies to: 6110-6110, 6146-6156
Summary by CodeRabbit
New Features
Breaking Changes
Improvements
Tests