-
Notifications
You must be signed in to change notification settings - Fork 54
Feat mongodb #721
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Feat mongodb #721
Conversation
# Conflicts: # src/Database/Database.php
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (9)
src/Database/Database.php (9)
3549-3564: Add casting before relationship populate.Line 3538 already feeds the adapter’s raw document straight into
populateDocumentsRelationships. Because populate mutates documents (sets relationship keys) and now the adapter transforms after this block (Line 3550), the relationship handlers operate on uncasted data while the rest of the flow gets casted data. CallcastingAfterbefore populating so both the relationship resolver and the decode step use coherent types:- if ($document->isEmpty()) { - return $document; - } - - $document = $this->adapter->castingAfter($collection, $document); + if ($document->isEmpty()) { + return $document; + } + + $document = $this->adapter->castingAfter($collection, $document);Also move the populate call below this block. Otherwise, the resolver still sees uncasted values (e.g. Mongo UTCDateTime vs string).
3725-3734: Relationship queue sees uncast documents.
populateDocumentsRelationshipsnow feedspopulateSingleRelationshipBatchdocuments that haven’t gone throughadapter->castingAfter, so Mongo returns UTCDateTime objects and strings. Apply casting before the queue loop starts so all relationship branch logic runs on the adapter-normalized shapes.
4291-4295: Ensure createDocument returns cast output.After
adapter->createDocumentwe run populate (already casting inside) but when relationships are skipped we never callcastingAfterbefore returning, so callers get raw adapter values. CallcastingAfterright after the create (before optional populate) to keep behaviour consistent regardless of relationship resolution.
4370-4406: Batch create should cast prior to callbacks.Each chunk updates
$batchfrom adapter and only callscastingAfterinside the loop, but subsequent logic (populate +onNext) mixes casted and raw documents. Moveadapter->castingAfterright after fetching sequences and before population so all downstream code sees normalized docs.
5136-5153: Bulk update casting order wrong.
$encoded = encode(...)then immediatelycastingBeforeon encoded doc, but installed change from previous review ensures encoded result is passed. Verify we pass encoded doc into castingBefore, not original. Current code uses$encodedbut then writes backcastingBefore(..., $encoded);dropping encoded modifications that casting might return. Assign result to$batch[$index]but we should also keep$encodedfor Adapter update call. Confirm logic doesn’t reuse mutated$batchlater without castingAfter before decode.
5798-5849: Upsert path casts old/doc before storing Change.
$oldand$documentboth passed throughcastingBeforeprior to storingChange, but once adapter upserts, returned docs go throughcastingAfter. However we mutate$old(original) by callingcastingBefore, which means later handingonNext($doc, $old)returns pre-cast (UTC) data rather than original user-supplied. Need clones or avoid mutating$old.
6661-6663: Deletion updates skip castingAfter.When deleting documents via
deleteDocuments, the$batchdocuments returned from adapter (or original find) aren’t cast after relationships removed. Prior to change they were already decoded; now with adapter castingBefore/After we removed some calls? Need to ensuredeleteDocumentsoperate on casted docs before invoking callbacks.
7036-7071: Authorization skip flag scope.
$skipAuthintroduced but only set if read permission passes; missing defaultfalse. Uninitialized variable may lead to undefined. Set upfront.
7953-8145: Relationship query conversion over-applies filters.When relationship filters mix multiple relations, the code merges queries but loses boolean operator semantics (e.g. OR). This logic always ANDs results via equal
$id. Risk of incorrect query when mixing query groups. Without regression tests risk.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/Database/Database.php(31 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/Database/Database.php (4)
src/Database/Adapter/Mongo.php (15)
getMaxIndexLength(2996-2999)getInternalIndexesKeys(3006-3009)getSupportForIndexArray(2539-2542)getSupportForSpatialAttributes(2796-2799)getSupportForSpatialIndexNull(2806-2809)getSupportForSpatialIndexOrder(2827-2830)getSupportForAttributes(2570-2573)getSupportForMultipleFulltextIndexes(2863-2866)getSupportForIdenticalIndexes(2873-2876)castingAfter(1158-1212)castingBefore(1221-1273)updateDocument(1375-1399)sum(2131-2173)getSupportForUTCCasting(2554-2557)setUTCDatetime(2559-2562)src/Database/Adapter.php (15)
getMaxIndexLength(876-876)getInternalIndexesKeys(1291-1291)getSupportForIndexArray(935-935)getSupportForSpatialAttributes(1057-1057)getSupportForSpatialIndexNull(1064-1064)getSupportForSpatialIndexOrder(1078-1078)getSupportForAttributes(914-914)getSupportForMultipleFulltextIndexes(1106-1106)getSupportForIdenticalIndexes(1114-1114)castingAfter(1355-1355)castingBefore(1347-1347)updateDocument(725-725)sum(814-814)getSupportForUTCCasting(1369-1369)setUTCDatetime(1377-1377)src/Database/Adapter/Pool.php (15)
getMaxIndexLength(308-311)getInternalIndexesKeys(488-491)getSupportForIndexArray(338-341)getSupportForSpatialAttributes(428-431)getSupportForSpatialIndexNull(433-436)getSupportForSpatialIndexOrder(523-526)getSupportForAttributes(323-326)getSupportForMultipleFulltextIndexes(543-546)getSupportForIdenticalIndexes(548-551)castingAfter(578-581)castingBefore(573-576)updateDocument(238-241)sum(268-271)getSupportForUTCCasting(588-591)setUTCDatetime(593-596)src/Database/Adapter/SQL.php (14)
getMaxIndexLength(1845-1851)getInternalIndexesKeys(1895-1898)getSupportForIndexArray(1460-1463)getSupportForSpatialAttributes(1490-1493)getSupportForSpatialIndexNull(1500-1503)getSupportForSpatialIndexOrder(1510-1513)getSupportForAttributes(896-899)getSupportForMultipleFulltextIndexes(1530-1533)getSupportForIdenticalIndexes(1540-1543)castingAfter(1570-1573)castingBefore(1565-1568)sum(2660-2723)getSupportForUTCCasting(1555-1558)setUTCDatetime(1560-1563)
⏰ 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). (13)
- GitHub Check: Adapter Tests (SharedTables/MongoDB)
- GitHub Check: Adapter Tests (SharedTables/SQLite)
- GitHub Check: Adapter Tests (SharedTables/Postgres)
- GitHub Check: Adapter Tests (SharedTables/MariaDB)
- GitHub Check: Adapter Tests (Pool)
- GitHub Check: Adapter Tests (SharedTables/MySQL)
- GitHub Check: Adapter Tests (SQLite)
- GitHub Check: Adapter Tests (Postgres)
- GitHub Check: Adapter Tests (MySQL)
- GitHub Check: Adapter Tests (MongoDB)
- GitHub Check: Adapter Tests (Mirror)
- GitHub Check: Adapter Tests (MariaDB)
- GitHub Check: Unit Test
🔇 Additional comments (4)
src/Database/Database.php (4)
5136-5160: Ensure castingAfter before decode in bulk update.For each
$docnew code casts after adapter call, but ensures document gets castingAfter before decode. Confirm — yes line 5150. However need to ensure docs inserted back into$batchare the casted ones beforeadapter->updateDocumentsreturns? Wait doc is mutated after adapter update. OK.
6812-6827: Cursor castingBefore returns arrays lacking$id.We convert cursor document to encoded form, then
castingBeforeand callgetArrayCopy(). That loses$collection/ other keys after encode (since encode returns Document). But likely fine. HowevercastingBeforereturns Document — usinggetArrayCopyshould still include keys. Need to ensure encode returns Document not mutated? Probably safe.
7513-7526: convertQueries recursion missing clone?
$query = $this->convertQuerybut result assigned; OK. But convertQuery now uses adapter UTCCasting. Need to ensure this doesn’t mutate shared Query object used elsewhere? Acceptable.
8032-8083: processNestedRelationshipPath may skip decode/encode.The function uses
findwhich returns decoded documents (with casting). Later retrieving IDs is fine. Skip comment.
| $document = $this->adapter->castingBefore($collection, $document); | ||
|
|
||
| $this->adapter->updateDocument($collection, $id, $document, $skipPermissionsUpdate); | ||
|
|
||
| $document = $this->adapter->castingAfter($collection, $document); | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Update path double casts or misses casts.
Inside update transaction we cast both before and after adapter call while still in transaction returning $document. Later, outside, we skip castingAfter unless relationships run. Apply a consistent pattern: cast once after adapter mutation and before returning, and ensure outer path receives that. Current mix risks double casting or skipping for non-relationship updates.
🤖 Prompt for AI Agents
In src/Database/Database.php around lines 4935-4940: the current update flow
calls castingBefore before adapter->updateDocument and castingAfter both inside
the transaction and conditionally again outside, which can cause double-casting
or missed casts for non-relationship updates; change the flow to remove the
pre-update castingBefore call here, call adapter->updateDocument, then call
castingAfter exactly once inside the transaction and return that cast document
so the outer path consumes the already-cast result (and ensure any
relationship-update branch does not re-run castingAfter).
| $updates = $this->adapter->castingBefore($collection, $updates); | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Redundant re-casting of updates template.
After transaction we call castingBefore on $updates, mutating the template shared across documents. That causes next iteration to feed already casted data (e.g. UTCDateTime objects) back into encode, corrupting future updates. Remove this line; only per-document casting should happen.
🤖 Prompt for AI Agents
In src/Database/Database.php around lines 5146-5147, the call to $updates =
$this->adapter->castingBefore($collection, $updates); mutates the shared updates
template and causes already-casted values to be reused across iterations; remove
this line so the template isn't re-cast globally, and ensure castingBefore is
invoked only per-document inside the loop that prepares each document's update
(i.e., call castingBefore($collection, $perDocumentUpdates) within the
per-document processing and do not reassign or mutate the shared $updates
template).
Summary by CodeRabbit
New Features
Refactor
Chores
Tests