Feat mongodb #721
Feat mongodb #721
Conversation
✅ Actions performedFull review triggered. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
tests/e2e/Adapter/Scopes/GeneralTests.php (1)
94-101: Restore authorization before early-return guards
Authorization::disable()runs before the capability guard. When the guard short-circuits, we return without ever callingAuthorization::reset(), so the rest of the suite executes with authorization disabled. Move the guard before disabling authorization (and likewise intestPreserveDatesCreate) so we only disable when the test actually runs.- Authorization::disable(); - /** @var Database $database */ $database = static::getDatabase(); if (!$database->getAdapter()->getSupportForAttributes()) { $this->expectNotToPerformAssertions(); return; } + + Authorization::disable();Apply the same pattern to
testPreserveDatesCreate.Also applies to: 193-200
tests/e2e/Adapter/Scopes/PermissionTests.php (1)
936-949: Avoid leaking Authorization state when relationships unsupportedHere we set the active role to
Role::users()before checking the capability. If the adapter lacks relationship support, the guard returns immediately and never restores the previous authorization context, so subsequent tests keep running withRole::users()set. Place the guard before mutating authorization (and do the same intestCollectionPermissionsRelationshipsGetWorks).- Authorization::cleanRoles(); - Authorization::setRole(Role::users()->toString()); - /** @var Database $database */ $database = static::getDatabase(); if (!$database->getAdapter()->getSupportForRelationships()) { $this->expectNotToPerformAssertions(); return; } + + Authorization::cleanRoles(); + Authorization::setRole(Role::users()->toString());src/Database/Database.php (3)
2047-2056: Bug: fall-through makes spatial defaults always throw.Missing
break;after spatial cases causes thedefaultbranch to throw “Unknown attribute type”.case self::VAR_POINT: case self::VAR_LINESTRING: case self::VAR_POLYGON: // Spatial types expect arrays as default values if ($defaultType !== 'array') { throw new DatabaseException('Default value for spatial type ' . $type . ' must be an array'); } - // no break + break; default: throw new DatabaseException('Unknown attribute type: ' . $type . '. Must be one of ' . self::VAR_STRING . ', ' . self::VAR_INTEGER . ', ' . self::VAR_FLOAT . ', ' . self::VAR_BOOLEAN . ', ' . self::VAR_DATETIME . ', ' . self::VAR_RELATIONSHIP . ', ' . self::VAR_POINT . ', ' . self::VAR_LINESTRING . ', ' . self::VAR_POLYGON);
4714-4812:$updatespre‑casting occurs after DB write; move it before.
$updates = $this->adapter->castingBefore(...);happens postupdateDocuments(), so it has no effect.$updates = $this->encode($collection, $updates); // Check new document structure $validator = new PartialStructure( $collection, $this->adapter->getIdAttributeType(), $this->adapter->getMinDateTime(), $this->adapter->getMaxDateTime(), $this->adapter->getSupportForAttributes() ); @@ - $this->withTransaction(function () use ($collection, $updates, &$batch, $currentPermissions) { + // Pre-cast updates for adapter before write + $updatesPre = $this->adapter->castingBefore($collection, $updates); + $this->withTransaction(function () use ($collection, $updatesPre, &$batch, $currentPermissions) { foreach ($batch as $index => $document) { @@ - $this->adapter->updateDocuments( + $this->adapter->updateDocuments( $collection, - $updates, + $updatesPre, $batch ); }); @@ - $updates = $this->adapter->castingBefore($collection, $updates); + // Already pre-cast above; no-op removed
4001-4043:createDocuments: result ofcastingBeforenot persisted.Iterates by value; if
castingBeforereturns a new instance, it’s lost. Iterate with index and assign back.- foreach ($documents as $document) { + foreach ($documents as $i => $document) { $createdAt = $document->getCreatedAt(); $updatedAt = $document->getUpdatedAt(); @@ - $document = $this->encode($collection, $document); + $document = $this->encode($collection, $document); @@ - $document = $this->adapter->castingBefore($collection, $document); + $document = $this->adapter->castingBefore($collection, $document); + $documents[$i] = $document; }
♻️ Duplicate comments (5)
src/Database/Validator/Index.php (1)
415-453: Fix order-sensitive comparison and add self-comparison skip.The bidirectional
array_diffcomparisons on lines 424-428 (attributes) and 430-434 (orders) are order-insensitive, but index column order matters. For example, an index on['name', 'age']is functionally different from['age', 'name'].Additionally, when updating an existing index, this check may self-conflict (similar to the issue in
checkMultipleFulltextIndex). Add a guard to skip comparison when$existingIndex->getId() === $index->getId().Apply this diff to fix both issues:
foreach ($this->indexes as $existingIndex) { + if ($existingIndex->getId() === $index->getId()) { + continue; + } + $existingAttributes = $existingIndex->getAttribute('attributes', []); $existingOrders = $existingIndex->getAttribute('orders', []); $existingType = $existingIndex->getAttribute('type', ''); - $attributesMatch = false; - if (empty(array_diff($existingAttributes, $indexAttributes)) && - empty(array_diff($indexAttributes, $existingAttributes))) { - $attributesMatch = true; - } + $attributesMatch = ($existingAttributes === $indexAttributes); - $ordersMatch = false; - if (empty(array_diff($existingOrders, $indexOrders)) && - empty(array_diff($indexOrders, $existingOrders))) { - $ordersMatch = true; - } + $ordersMatch = ($existingOrders === $indexOrders); if ($attributesMatch && $ordersMatch) {src/Database/Validator/Query/Filter.php (1)
98-106: Still enforce spatial guard before early-returnWe still let spatial methods through when the attribute isn’t in the schema and
supportForAttributesis false, so invalid distance/geometry queries slip past every safety check. Please perform the spatial guard (and only returntrueafterwards) even when attributes aren’t supported.- if (!$this->supportForAttributes && !isset($this->schema[$attribute])) { - if (count($values) > $this->maxValuesCount) { - $this->message = 'Query on attribute has greater than ' . $this->maxValuesCount . ' values: ' . $attribute; - return false; - } - - return true; - } + if (!isset($this->schema[$attribute])) { + if (count($values) > $this->maxValuesCount) { + $this->message = 'Query on attribute has greater than ' . $this->maxValuesCount . ' values: ' . $attribute; + return false; + } + + $query = new Query($method); + if ($query->isSpatialQuery()) { + $this->message = 'Spatial query "' . $method . '" cannot be applied on unknown attribute: ' . $attribute; + return false; + } + + if (!$this->supportForAttributes) { + return true; + } + }docker-compose.yml (1)
108-110: Advertise the Mongo service hostname in rs.initiateRegistering the member as
localhost:27017causes every other container to connect to itself instead of the Mongo service, breaking transactions/change streams. Use the Compose service hostname instead.- rs.initiate({_id: 'rs0', members: [{_id: 0, host: 'localhost:27017'}]}) + rs.initiate({_id: 'rs0', members: [{_id: 0, host: 'mongo:27017'}]})tests/e2e/Adapter/SharedTables/MongoDBTest.php (1)
55-60: Generate a per-run namespace to avoid cross-test collisions.
We previously flagged this: using the static namespace'my_shared_tables'lets concurrent/dirty runs stomp on each other. Randomize the suffix (e.g.'my_shared_tables_' . uniqid()), so each test run remains isolated.- ->setNamespace(static::$namespace = 'my_shared_tables'); + ->setNamespace(static::$namespace = 'my_shared_tables_' . uniqid());tests/e2e/Adapter/Scopes/DocumentTests.php (1)
6149-6225: Drop attribute-only calls from the schemaless test.
This test runs only whengetSupportForAttributes()is false, yet it still callscreateAttribute()/increaseDocumentAttribute()/decreaseDocumentAttribute(). Those APIs are guaranteed to throw on adapters without attribute support, so the test will fail before exercising schemaless behavior. Remove or guard all attribute-specific code so the schemaless adapters can execute the flow.- $database->createAttribute($colName, 'key', Database::VAR_STRING, 50, true); - $database->createAttribute($colName, 'value', Database::VAR_STRING, 50, false, 'value'); … - $database->createAttribute($colName, 'counter', Database::VAR_INTEGER, 0, false, 0); - $docS = $database->getDocument($colName, 'docS'); - $this->assertEquals(0, $docS->getAttribute('counter')); - $docS = $database->increaseDocumentAttribute($colName, 'docS', 'counter', 5); - $this->assertEquals(5, $docS->getAttribute('counter')); - $docS = $database->decreaseDocumentAttribute($colName, 'docS', 'counter', 3); - $this->assertEquals(2, $docS->getAttribute('counter')); - - $deletedByCounter = $database->deleteDocuments($colName, [Query::equal('counter', [2])]); - $this->assertEquals(1, $deletedByCounter);(Keep the rest of the schemaless assertions intact.)
🧹 Nitpick comments (6)
composer.json (2)
51-51: Reconsider wildcard version for laravel/pint in require-dev.Changing
laravel/pintfrom a fixed version to"*"removes version constraints, which can lead to:
- Unpredictable CI/local environment behavior if breaking changes are introduced in new versions.
- Difficulty reproducing builds and debugging formatting issues.
Apply this diff to use a permissive but bounded version range:
- "laravel/pint": "*", + "laravel/pint": "^1.0",
57-59: Clarify the distinction between ext-pdo and mongodb/mongodb in suggests.The
suggestssection now includes bothext-pdoandmongodb/mongodb. However:
ext-mongodbis already required on line 38, making themongodb/mongodbsuggestion potentially redundant or confusing.- The description for
ext-pdomentions MongoDB, which may mislead users.Consider clarifying the distinction:
- "ext-pdo": "Needed to support MariaDB, MySQL or SQLite Database Adapter", - "mongodb/mongodb": "Needed to support MongoDB Database Adapter" + "ext-pdo": "Needed to support MariaDB, MySQL, or SQLite Database Adapters", + "mongodb/mongodb": "Provides additional MongoDB utilities (ext-mongodb is already required)"tests/e2e/Adapter/MongoDBTest.php (3)
38-42: AvoidflushAll(); preferflushDB()or a scoped cache name.
flushAll()wipes all Redis DBs and can race with other tests. UseflushDB()or set a unique cache name/namespace and purge that.- $redis->flushAll(); + // Limit impact to current Redis DB + $redis->flushDB();
79-97: Mark placeholders as incomplete instead of assertingtrue.Use PHPUnit’s
markTestIncomplete()to signal pending work and avoid false greens.- public function testRenameAttribute(): void - { - $this->assertTrue(true); - } + public function testRenameAttribute(): void + { + $this->markTestIncomplete('MongoDB: implement rename attribute test'); + } - public function testRenameAttributeExisting(): void - { - $this->assertTrue(true); - } + public function testRenameAttributeExisting(): void + { + $this->markTestIncomplete('MongoDB: implement rename attribute (existing) test'); + } - public function testUpdateAttributeStructure(): void - { - $this->assertTrue(true); - } + public function testUpdateAttributeStructure(): void + { + $this->markTestIncomplete('MongoDB: implement update attribute structure test'); + } - public function testKeywords(): void - { - $this->assertTrue(true); - } + public function testKeywords(): void + { + $this->markTestIncomplete('MongoDB: implement keywords test'); + }
99-107: Silence PHPMD unused-parameter warnings.Either prefix with underscores or suppress via annotations.
- protected static function deleteColumn(string $collection, string $column): bool + protected static function deleteColumn(string $_collection, string $_column): bool { return true; } - protected static function deleteIndex(string $collection, string $index): bool + protected static function deleteIndex(string $_collection, string $_index): bool { return true; }src/Database/Database.php (1)
2347-2357: Minor: method name casing.
setattribute('key', ...)is inconsistent withsetAttribute. PHP is case-insensitive here, but consistency helps.- ->setattribute('key', $newKey ?? $id) + ->setAttribute('key', $newKey ?? $id)
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
composer.lockis excluded by!**/*.lock
📒 Files selected for processing (35)
.github/workflows/tests.yml(1 hunks)Dockerfile(2 hunks)composer.json(1 hunks)docker-compose.yml(3 hunks)src/Database/Adapter.php(3 hunks)src/Database/Adapter/Pool.php(3 hunks)src/Database/Adapter/SQL.php(2 hunks)src/Database/Database.php(25 hunks)src/Database/Validator/Index.php(6 hunks)src/Database/Validator/Key.php(2 hunks)src/Database/Validator/Label.php(1 hunks)src/Database/Validator/Queries/Documents.php(2 hunks)src/Database/Validator/Query/Filter.php(3 hunks)src/Database/Validator/Query/Order.php(2 hunks)src/Database/Validator/Query/Select.php(2 hunks)src/Database/Validator/Sequence.php(1 hunks)src/Database/Validator/Structure.php(4 hunks)src/Database/Validator/UID.php(1 hunks)tests/e2e/Adapter/MongoDBTest.php(1 hunks)tests/e2e/Adapter/Scopes/AttributeTests.php(18 hunks)tests/e2e/Adapter/Scopes/CollectionTests.php(1 hunks)tests/e2e/Adapter/Scopes/DocumentTests.php(32 hunks)tests/e2e/Adapter/Scopes/GeneralTests.php(2 hunks)tests/e2e/Adapter/Scopes/IndexTests.php(5 hunks)tests/e2e/Adapter/Scopes/PermissionTests.php(2 hunks)tests/e2e/Adapter/Scopes/SpatialTests.php(15 hunks)tests/e2e/Adapter/SharedTables/MongoDBTest.php(1 hunks)tests/resources/mongo/entrypoint.sh(1 hunks)tests/resources/mongo/mongo-keyfile(1 hunks)tests/unit/QueryTest.php(0 hunks)tests/unit/Validator/IndexTest.php(8 hunks)tests/unit/Validator/KeyTest.php(1 hunks)tests/unit/Validator/LabelTest.php(1 hunks)tests/unit/Validator/PermissionsTest.php(3 hunks)tests/unit/Validator/StructureTest.php(2 hunks)
💤 Files with no reviewable changes (1)
- tests/unit/QueryTest.php
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-07-30T19:17:53.630Z
Learnt from: ArnabChatterjee20k
PR: utopia-php/database#642
File: src/Database/Validator/PartialStructure.php:43-52
Timestamp: 2025-07-30T19:17:53.630Z
Learning: In PartialStructure validator, when filtering for required attributes validation using the $requiredAttributes parameter, $this->attributes should be used instead of the merged $attributes array because this validation is specifically for internal attributes like $createdAt and $updatedAt that are defined in the base Structure class, not collection-specific attributes.
Applied to files:
src/Database/Validator/Structure.php
🪛 Checkov (3.2.334)
docker-compose.yml
[medium] 129-130: Basic Auth Credentials
(CKV_SECRET_4)
🪛 PHPMD (2.15.0)
tests/e2e/Adapter/MongoDBTest.php
99-99: Avoid unused parameters such as '$collection'. (undefined)
(UnusedFormalParameter)
99-99: Avoid unused parameters such as '$column'. (undefined)
(UnusedFormalParameter)
104-104: Avoid unused parameters such as '$collection'. (undefined)
(UnusedFormalParameter)
104-104: Avoid unused parameters such as '$index'. (undefined)
(UnusedFormalParameter)
src/Database/Adapter/SQL.php
1565-1565: Avoid unused parameters such as '$collection'. (undefined)
(UnusedFormalParameter)
1570-1570: Avoid unused parameters such as '$collection'. (undefined)
(UnusedFormalParameter)
tests/e2e/Adapter/SharedTables/MongoDBTest.php
102-102: Avoid unused parameters such as '$collection'. (undefined)
(UnusedFormalParameter)
102-102: Avoid unused parameters such as '$column'. (undefined)
(UnusedFormalParameter)
107-107: Avoid unused parameters such as '$collection'. (undefined)
(UnusedFormalParameter)
107-107: Avoid unused parameters such as '$index'. (undefined)
(UnusedFormalParameter)
🔇 Additional comments (33)
src/Database/Validator/Label.php (1)
7-7: LGTM! Past feedback addressed.The message now references
self::KEY_MAX_LENGTHdynamically instead of hard-coding "255", aligning with the parent class pattern and ensuring consistency across validators.src/Database/Validator/UID.php (1)
16-16: LGTM!The description correctly references
self::KEY_MAX_LENGTHfor dynamic max-length messaging, ensuring consistency with the updated constant.tests/unit/Validator/KeyTest.php (1)
69-71: LGTM!The boundary tests correctly validate the 255-character maximum length, aligning with the new
KEY_MAX_LENGTHconstant.tests/unit/Validator/LabelTest.php (1)
61-63: LGTM!The boundary tests correctly reflect the 255-character limit, consistent with the updated
KEY_MAX_LENGTHconstant.src/Database/Validator/Key.php (3)
11-14: LGTM!The new
KEY_MAX_LENGTHconstant centralizes the maximum key length value, enabling dynamic references in error messages and validation logic across validators.
19-19: LGTM!The error message correctly references
self::KEY_MAX_LENGTHdynamically, ensuring consistency and maintainability.
84-86: LGTM!The length check correctly enforces the 255-character maximum using
self::KEY_MAX_LENGTH, replacing the previous hard-coded limit.tests/unit/Validator/PermissionsTest.php (2)
251-251: LGTM!The error message assertions correctly reflect the updated 255-character maximum length, aligning with the new
KEY_MAX_LENGTHconstant in the Key validator.Also applies to: 253-253, 255-255, 259-259, 261-261, 263-263, 269-269, 281-281, 291-291, 293-293
265-269: LGTM!The boundary tests correctly validate the 255-character maximum length for role identifiers, consistent with the updated constant.
tests/resources/mongo/entrypoint.sh (1)
1-12: LGTM! Keyfile setup and MongoDB startup are correct.The script properly handles the MongoDB keyfile setup:
- Copies the keyfile from
/tmpto/etc/mongo-keyfilewith restricted permissions (400).- Sets ownership to
mongodb:mongodbfor security.- Uses
set -eto ensure the script exits on any error.- Delegates to MongoDB's standard
docker-entrypoint.shwith appropriate arguments for replica set, authentication, and keyfile.The logic is sound and follows MongoDB security best practices.
.github/workflows/tests.yml (1)
73-87: LGTM! Test matrix expansion for MongoDB is appropriate.The addition of
MongoDBandSharedTables/MongoDBto the adapter test matrix ensures comprehensive coverage of the new MongoDB support. The change is straightforward and aligns with the PR objectives.Dockerfile (2)
36-40: LGTM! MongoDB extension installation is correct.The MongoDB extension installation and enablement are properly integrated:
- Uses
pecl install mongodb-$PHP_MONGODB_VERSIONto install the extension.- Enables the extension with
docker-php-ext-enable mongodb.- Cleans up build dependencies and cache with
apk delandrm -rf /var/cache/apk/*.The approach follows Docker best practices for minimizing image size.
20-20: MongoDB PHP extension version is up-to-date
PHP_MONGODB_VERSION="2.1.1"is the latest stable release (June 13, 2025) and supports PHP 8.3.19.src/Database/Validator/Structure.php (4)
109-110: LGTM! Feature flag for attribute validation is well-designed.The addition of the
$supportForAttributesparameter enables adapters without traditional attribute schemas (e.g., MongoDB) to bypass semantic attribute validation. The default value oftruepreserves existing behavior for other adapters, ensuring backward compatibility.
255-272: LGTM! Short-circuit logic for required values is correct.When
$supportForAttributesisfalse, the method correctly returnstrueimmediately, bypassing the required attribute checks. This aligns with the design for schema-less adapters like MongoDB.
284-295: LGTM! Short-circuit logic for unknown attributes is correct.When
$supportForAttributesisfalse, the method correctly returnstrueimmediately, bypassing the unknown attribute detection. This is appropriate for schema-less adapters.
368-372: LGTM! Conditional error reporting for unknown types is correct.The error message for unknown attribute types is only generated when
$supportForAttributesistrue. This prevents false positives for schema-less adapters that may use custom or flexible types.src/Database/Validator/Index.php (6)
33-90: LGTM! New capability flags and constructor parameters are well-structured.The addition of
$supportForAttributes,$multipleFulltextIndexSupport,$identicalIndexSupport, and$indexesprovides fine-grained control over index validation based on adapter capabilities. The constructor signature is clear, with sensible defaults that preserve backward compatibility.
109-114: LGTM! Attribute existence check respects supportForAttributes flag.When
$supportForAttributesisfalse, the validator skips checking if attributes exist in the collection, which is appropriate for schema-less adapters like MongoDB.
157-170: LGTM! Fulltext index type validation respects supportForAttributes flag.The check correctly short-circuits when
$supportForAttributesisfalse, bypassing type validation for fulltext indexes. This is appropriate for adapters that handle types differently.
178-223: LGTM! Array index validation respects supportForAttributes flag.The method correctly short-circuits when
$supportForAttributesisfalse, bypassing array-specific index validation. This is appropriate for schema-less adapters.
345-352: LGTM! New validation checks are properly integrated.The calls to
checkMultipleFulltextIndexandcheckIdenticalIndexare correctly placed in the validation flow, ensuring these checks are performed when validating indexes.
384-403: LGTM! Multiple fulltext index validation is correct.The
checkMultipleFulltextIndexmethod properly:
- Short-circuits when
$multipleFulltextIndexSupportistrue.- Skips self-comparison by checking
$existingIndex->getId() === $index->getId()(lines 392-394).- Rejects additional fulltext indexes when the adapter doesn't support them.
This addresses the previous review concern about self-conflict.
tests/e2e/Adapter/Scopes/IndexTests.php (4)
165-177: LGTM! Index validator instantiation updated correctly.The instantiation of
Indexvalidator now includes the new$indexesarray and three capability flags (getSupportForIndexArray,getSupportForSpatialAttributes, etc.), aligning with the updated validator signature.
247-277: LGTM! Conditional validation logic handles adapter capabilities correctly.The test properly:
- Instantiates the validator with adapter capability flags.
- Uses conditional assertions (lines 263-267, 271-276) to verify behavior based on
$supportForAttributes.- Expects different error messages depending on adapter capabilities.
This ensures the test suite accommodates both schema-based and schema-less adapters.
329-347: LGTM! Index length tests use adapter's max length.The tests correctly reference
$database->getAdapter()->getMaxIndexLength()to validate index length constraints, ensuring tests respect adapter-specific limits.
510-553: LGTM! Multiple fulltext index validation test is comprehensive.The test properly:
- Skips when fulltext is not supported.
- Creates a collection with two string attributes.
- Creates the first fulltext index.
- Attempts to create a second fulltext index and verifies success/failure based on
getSupportForMultipleFulltextIndexes().The test logic is sound and exercises the new
checkMultipleFulltextIndexvalidation.tests/resources/mongo/mongo-keyfile (1)
1-17: LGTM! MongoDB keyfile is appropriate for test infrastructure.The keyfile contains random data suitable for MongoDB replica set authentication in a test environment. This is correctly referenced by
docker-compose.ymland used byentrypoint.shto enable authentication.Important: Ensure this keyfile is only used in test/development environments and never in production.
composer.json (1)
38-43: Verifyutopia-php/mongopackage and ext-mongodb requirements.
- Confirm that
utopia-php/mongo:0.10.*exists as the latest stable release on Packagist and is compatible with PHP 8.1+ (GH repo lookup returned 404).- Verify that
utopia-php/pools:0.8.*aligns with the latest v0.8.2.- Ensure installation docs note the
ext-mongodbextension requirement.src/Database/Database.php (4)
6483-6489: Cursor pre‑encode and pre‑cast: LGTM.Encoding and applying
castingBeforeto the cursor aligns it with adapter expectations.Ensure adapter’s
castingBeforeaccepts a Document and returns a Document consistently across adapters.
6705-6714: Auth skip insum(): LGTM.Mirrors behavior in
count(). Good consistency.
46-46: Public constant rename is a breaking change: no internal usages ofDatabase::VAR_OBJECT_IDor static calls toDatabase::convertQuery/convertQuerieswere found. Confirm any external consumers or extensions referencing these are updated toVAR_UUID7and the new instance‐method calls.
7140-7154: No static invocations of convertQuery/convertQueries detected — automated search found noDatabase::convertQueryorDatabase::convertQueriescalls; all usages already use instance methods.
# Conflicts: # src/Database/Database.php
There was a problem hiding this comment.
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.
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.
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