Skip to content

Conversation

@abnegate
Copy link
Member

@abnegate abnegate commented Sep 30, 2025

Summary by CodeRabbit

  • New Features

    • MongoDB support: native adapter, PHP mongodb extension, composer requirements, and local mongo + mongo-express services.
  • Refactor

    • Default ID format switched to UUIDv7.
    • Increased key/label/permission length limits to 255.
    • New adapter capability flags and casting lifecycle hooks.
  • Chores

    • Updated MySQL and Redis images; docker/dev config extended with volumes/secrets for Mongo.
  • Tests

    • Expanded adapter matrix (Mongo/SharedTables variants) and added capability-aware e2e/unit tests.

Copy link
Contributor

@coderabbitai coderabbitai bot left a 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. Call castingAfter before 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.

populateDocumentsRelationships now feeds populateSingleRelationshipBatch documents that haven’t gone through adapter->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->createDocument we run populate (already casting inside) but when relationships are skipped we never call castingAfter before returning, so callers get raw adapter values. Call castingAfter right 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 $batch from adapter and only calls castingAfter inside the loop, but subsequent logic (populate + onNext) mixes casted and raw documents. Move adapter->castingAfter right after fetching sequences and before population so all downstream code sees normalized docs.


5136-5153: Bulk update casting order wrong.

$encoded = encode(...) then immediately castingBefore on 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 $encoded but then writes back castingBefore(..., $encoded); dropping encoded modifications that casting might return. Assign result to $batch[$index] but we should also keep $encoded for Adapter update call. Confirm logic doesn’t reuse mutated $batch later without castingAfter before decode.


5798-5849: Upsert path casts old/doc before storing Change.

$old and $document both passed through castingBefore prior to storing Change, but once adapter upserts, returned docs go through castingAfter. However we mutate $old (original) by calling castingBefore, which means later handing onNext($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 $batch documents 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 ensure deleteDocuments operate on casted docs before invoking callbacks.


7036-7071: Authorization skip flag scope.

$skipAuth introduced but only set if read permission passes; missing default false. 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

📥 Commits

Reviewing files that changed from the base of the PR and between aef4c2b and 73d2c29.

📒 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 $doc new code casts after adapter call, but ensures document gets castingAfter before decode. Confirm — yes line 5150. However need to ensure docs inserted back into $batch are the casted ones before adapter->updateDocuments returns? Wait doc is mutated after adapter update. OK.


6812-6827: Cursor castingBefore returns arrays lacking $id.

We convert cursor document to encoded form, then castingBefore and call getArrayCopy(). That loses $collection / other keys after encode (since encode returns Document). But likely fine. However castingBefore returns Document — using getArrayCopy should still include keys. Need to ensure encode returns Document not mutated? Probably safe.


7513-7526: convertQueries recursion missing clone?

$query = $this->convertQuery but 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 find which returns decoded documents (with casting). Later retrieving IDs is fine. Skip comment.

Comment on lines +4935 to +4940
$document = $this->adapter->castingBefore($collection, $document);

$this->adapter->updateDocument($collection, $id, $document, $skipPermissionsUpdate);

$document = $this->adapter->castingAfter($collection, $document);

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

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).

Comment on lines +5146 to +5147
$updates = $this->adapter->castingBefore($collection, $updates);

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

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).

@abnegate abnegate requested a review from fogelito October 5, 2025 04:52
@abnegate abnegate merged commit 8475957 into main Oct 6, 2025
17 checks passed
@abnegate abnegate deleted the feat-mongodb branch October 6, 2025 03:09
@abnegate abnegate restored the feat-mongodb branch October 6, 2025 04:22
@abnegate abnegate mentioned this pull request Oct 6, 2025
abnegate added a commit that referenced this pull request Oct 6, 2025
This reverts commit 8475957, reversing
changes made to e318e54.
@abnegate abnegate mentioned this pull request Oct 6, 2025
This was referenced Nov 20, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants