fix - replacement style update for schemaless #747
Conversation
…al updates in case of schemaless
WalkthroughDatabase and Mongo adapter update paths now distinguish attribute-supporting vs schemaless modes: Database adds schemaless-aware Changes
Sequence Diagram(s)sequenceDiagram
rect rgba(200,230,255,0.35)
participant App
participant DB as Database
participant Adapter
participant MongoDB
end
App->>DB: updateDocument(oldDoc, newDoc)
DB->>DB: mergeDocuments(oldDoc,newDoc)
DB->>DB: Check adapter->getSupportForAttributes()
alt supports attributes
DB->>Adapter: updateDocument(filter, {'$set': merged})
Adapter->>MongoDB: bulk update with '$set' (multi:true)
MongoDB-->>Adapter: result
else schemaless
DB->>DB: diff non-internal keys -> decide shouldUpdate
DB->>Adapter: updateDocuments(filters, updates)
Adapter->>Adapter: updateDocumentsForSchemaless(...) (build '$set' & '$unset')
Adapter->>MongoDB: per-doc multi update(s) with '$set' + optional '$unset'
MongoDB-->>Adapter: result counts
end
Adapter-->>DB: result/count
DB-->>App: updated document(s)/result
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~30 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
tests/e2e/Adapter/Scopes/DocumentTests.php (1)
6564-6568: Schemaless guard is right; tidy the comment and confirm batch-test intentGood call adding the guard; defaults rely on attribute metadata, so this test isn’t meaningful when attributes aren’t supported. Two nits and a consistency check:
- Clean up the comment phrasing/double slashes.
- Optional: prefer markTestSkipped for clearer intent (project uses expectNotToPerformAssertions elsewhere, so up to you).
- Please confirm whether testUpdateDocumentsSuccessiveCallsDoNotResetDefaults should also be skipped under schemaless to avoid default-value assertions on adapters that don’t consult attributes.
Apply either tweak:
- // // skipping as schemaless doesn't look to the attributes metadata + // Skip in schemaless: adapter doesn't consult attribute metadata if (!$database->getAdapter()->getSupportForAttributes()) { $this->expectNotToPerformAssertions(); return; }Or (alternative):
- if (!$database->getAdapter()->getSupportForAttributes()) { - $this->expectNotToPerformAssertions(); - return; - } + if (!$database->getAdapter()->getSupportForAttributes()) { + $this->markTestSkipped('Adapter does not support attributes; defaults metadata not applicable.'); + }src/Database/Database.php (1)
5032-5042: Schemaless delta check: use getInternalAttributes() and symmetric set compare.Use the runtime internal list and compare sets both ways for clarity and correctness.
- if (!$this->adapter->getSupportForAttributes()) { - $internalFields = array_map(fn ($attr) => $attr['$id'], self::INTERNAL_ATTRIBUTES); - - $oldKeys = array_keys(array_diff_key($old->getArrayCopy(), array_flip($internalFields))); - $newKeys = array_keys(array_diff_key($document->getArrayCopy(), array_flip($internalFields))); - if (count($oldKeys) !== count($newKeys) || array_diff($oldKeys, $newKeys)) { - $shouldUpdate = true; - } - } + if (!$this->adapter->getSupportForAttributes()) { + $internalFields = array_map(fn ($attr) => $attr['$id'], $this->getInternalAttributes()); + $mask = array_fill_keys($internalFields, true); + $oldKeys = array_keys(array_diff_key($old->getArrayCopy(), $mask)); + $newKeys = array_keys(array_diff_key($document->getArrayCopy(), $mask)); + if ( + \count($oldKeys) !== \count($newKeys) || + !empty(\array_diff($oldKeys, $newKeys)) || + !empty(\array_diff($newKeys, $oldKeys)) + ) { + $shouldUpdate = true; + } + }I can add a focused test asserting $id/$sequence immutability and schemaless field add/remove triggers an update. Want a PR note + tests?
tests/e2e/Adapter/Scopes/SchemalessTests.php (2)
1164-1164: Clarify the comment to accurately describe replacement behavior.The current comment mentions "default attribute automatically in the encode," which is misleading. The actual behavior is that schemaless updates use a replacement strategy where only internal attributes (like
$id,$permissions,$createdAt,$updatedAt) are preserved from the old document, and custom attributes not included in the update payload are removed.Consider revising to something like:
- // in schemaless, if attributes are created and then if values are not provided then they are replaced with the default attribute automatically in the encode + // In schemaless mode, updates use replacement-style behavior: only internal attributes + // are preserved from the original document, custom attributes not in the update are removed
1159-1200: Strengthen test by verifying internal attributes are preserved.The test correctly validates that custom attributes are removed during updates, but it should also verify that internal attributes (like
$permissions,$createdAt,$updatedAt) are preserved to fully document the replacement behavior.Add assertions after line 1193 and 1197 to verify internal attributes are preserved:
$doc = $database->updateDocument($col, 'docS', new Document(['$id' => 'docS','key' => 'single2'])); $this->assertEquals('single2', $doc->getAttribute('key')); $this->assertArrayNotHasKey('extra', $doc); +$this->assertNotEmpty($doc->getPermissions(), 'Internal attribute $permissions should be preserved'); +$this->assertNotEmpty($doc->getAttribute('$createdAt'), 'Internal attribute $createdAt should be preserved'); +$this->assertNotEmpty($doc->getAttribute('$updatedAt'), 'Internal attribute $updatedAt should be preserved'); $doc = $database->getDocument($col, 'docS'); $this->assertEquals('single2', $doc->getAttribute('key')); $this->assertArrayNotHasKey('extra', $doc); +$this->assertNotEmpty($doc->getPermissions(), 'Internal attribute $permissions should be preserved'); +$this->assertNotEmpty($doc->getAttribute('$createdAt'), 'Internal attribute $createdAt should be preserved'); +$this->assertNotEmpty($doc->getAttribute('$updatedAt'), 'Internal attribute $updatedAt should be preserved');Optional: Remove the redundant
'$id' => 'docS'from line 1191 since the ID is already specified as a parameter:-$doc = $database->updateDocument($col, 'docS', new Document(['$id' => 'docS','key' => 'single2'])); +$doc = $database->updateDocument($col, 'docS', new Document(['key' => 'single2']));
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/Database/Adapter/Mongo.php(1 hunks)src/Database/Database.php(2 hunks)tests/e2e/Adapter/Scopes/DocumentTests.php(1 hunks)tests/e2e/Adapter/Scopes/SchemalessTests.php(3 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-03T02:04:17.803Z
Learnt from: abnegate
PR: utopia-php/database#721
File: tests/e2e/Adapter/Scopes/DocumentTests.php:6418-6439
Timestamp: 2025-10-03T02:04:17.803Z
Learning: In tests/e2e/Adapter/Scopes/DocumentTests::testSchemalessDocumentInvalidInteralAttributeValidation (PHP), when the adapter reports getSupportForAttributes() === false (schemaless), the test should not expect exceptions from createDocuments for “invalid” internal attributes; remove try/catch and ensure the test passes without exceptions, keeping at least one assertion.
Applied to files:
src/Database/Database.phptests/e2e/Adapter/Scopes/DocumentTests.phptests/e2e/Adapter/Scopes/SchemalessTests.php
🧬 Code graph analysis (4)
src/Database/Database.php (5)
src/Database/Adapter/Mongo.php (1)
getSupportForAttributes(2645-2648)src/Database/Adapter/SQL.php (1)
getSupportForAttributes(896-899)src/Database/Adapter.php (1)
getSupportForAttributes(921-921)src/Database/Adapter/Pool.php (1)
getSupportForAttributes(328-331)src/Database/Document.php (1)
getArrayCopy(423-458)
tests/e2e/Adapter/Scopes/DocumentTests.php (5)
src/Database/Database.php (1)
getAdapter(1251-1254)src/Database/Adapter/Mongo.php (1)
getSupportForAttributes(2645-2648)src/Database/Adapter/SQL.php (1)
getSupportForAttributes(896-899)src/Database/Adapter.php (1)
getSupportForAttributes(921-921)src/Database/Adapter/Pool.php (1)
getSupportForAttributes(328-331)
tests/e2e/Adapter/Scopes/SchemalessTests.php (2)
src/Database/Database.php (3)
getAdapter(1251-1254)getDocument(3600-3724)updateDocument(4887-5113)src/Database/Adapter/Mongo.php (3)
getSupportForAttributes(2645-2648)getDocument(1113-1148)updateDocument(1432-1459)
src/Database/Adapter/Mongo.php (3)
src/Database/Adapter/SQL.php (1)
getSupportForAttributes(896-899)src/Database/Adapter.php (1)
getSupportForAttributes(921-921)src/Database/Adapter/Pool.php (1)
getSupportForAttributes(328-331)
⏰ 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). (1)
- GitHub Check: Setup & Build Docker Image
🔇 Additional comments (3)
src/Database/Adapter/Mongo.php (1)
1450-1452: Code is correct—Utopia\Mongo\Client supports plain document replacements.The Utopia Mongo client treats plain documents (without $-operators) passed to update() as full replacements of the matched document. The conditional logic at lines 1450–1452 properly leverages this:
- Schema-aware mode (
getSupportForAttributes() = true):['$set' => $record]performs a partial update.- Schemaless mode (
getSupportForAttributes() = false):$record(plain document) performs a full replacement.The
_idis correctly unset before the update, error handling is in place, and the pattern aligns with the library's documented behavior.tests/e2e/Adapter/Scopes/SchemalessTests.php (2)
348-349: LGTM: Assertions correctly reflect replacement-style updates in schemaless mode.These modified assertions now verify that custom attributes not included in the update payload are removed, which aligns with the new schemaless update behavior where only internal attributes (like
$id,$permissions,$createdAt,$updatedAt) are preserved during updates.
364-364: LGTM: Consistent with replacement-style update behavior.This assertion is consistent with the changes at lines 348-349, correctly verifying that attributes not included in the update are removed.
src/Database/Database.php
Outdated
| if ($this->adapter->getSupportForAttributes()) { | ||
| $document = \array_merge($old->getArrayCopy(), $document->getArrayCopy()); | ||
| } else { | ||
| $oldArray = $old->getArrayCopy(); | ||
| $newArray = $document->getArrayCopy(); | ||
| $internalKeys = array_map(fn ($attr) => $attr['$id'], self::INTERNAL_ATTRIBUTES); | ||
| $internalAttrs = array_intersect_key($oldArray, array_flip($internalKeys)); | ||
| $document = array_merge($internalAttrs, $newArray); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
Lock down $id and $sequence; prefer dynamic internal-attr list.
- After merge, user input can override $id and $sequence. Preserve identity and row pointer explicitly to avoid primary-key/key-update surprises.
- Use getInternalAttributes() (respects sharedTables) instead of self::INTERNAL_ATTRIBUTES.
Apply:
$createdAt = $document->getCreatedAt();
if ($this->adapter->getSupportForAttributes()) {
$document = \array_merge($old->getArrayCopy(), $document->getArrayCopy());
} else {
$oldArray = $old->getArrayCopy();
$newArray = $document->getArrayCopy();
- $internalKeys = array_map(fn ($attr) => $attr['$id'], self::INTERNAL_ATTRIBUTES);
+ $internalKeys = array_map(fn ($attr) => $attr['$id'], $this->getInternalAttributes());
$internalAttrs = array_intersect_key($oldArray, array_flip($internalKeys));
$document = array_merge($internalAttrs, $newArray);
}
+ // Ensure identity and sequencing are immutable on update
+ $document['$id'] = $old->getId();
+ $document['$sequence'] = $old->getSequence();
$document['$collection'] = $old->getAttribute('$collection'); // Make sure user doesn't switch collection ID
$document['$createdAt'] = ($createdAt === null || !$this->preserveDates) ? $old->getCreatedAt() : $createdAt;
if ($this->adapter->getSharedTables()) {
$document['$tenant'] = $old->getTenant(); // Make sure user doesn't switch tenant
}If adapters ignore payload $id during update it’s benign; otherwise this is required. Want me to scan adapters to confirm whether $id is ever written on update? I can provide a script.
🏁 Script executed:
#!/bin/bash
# 1. Find getInternalAttributes() method to verify it exists and its behavior
echo "=== Checking for getInternalAttributes() method ==="
rg -n "function\s+getInternalAttributes|getInternalAttributes\s*\(" src/Database/Database.php -A5
echo -e "\n=== Checking current code state around lines 4916-4924 ==="
# Get lines around the area to verify current state
sed -n '4910,4930p' src/Database/Database.php
echo -e "\n=== Searching for adapters and their update methods that write to $id ==="
# Look for adapter implementations that might write $id during updates
rg -n "\\$.*\[.*\$id.*\\]|->update|setDocument|writeId|write.*\\\$id" src/ -g "*.php" -A2 -B2 | head -100
echo -e "\n=== Check self::INTERNAL_ATTRIBUTES definition ==="
rg -n "INTERNAL_ATTRIBUTES\s*=" src/Database/Database.php -A10 | head -30Length of output: 10217
🏁 Script executed:
#!/bin/bash
# 2. Search for adapter classes and their update/insert patterns
echo "=== Finding adapter interface and implementations ==="
fd -e php -path "*/Adapter*" src/ | head -20
echo -e "\n=== Checking if adapters implement update that touches $id ==="
rg -n "function\s+update|class.*Adapter" src/ -g "*.php" -B2 -A15 | grep -A15 "function update" | head -80Length of output: 5526
🏁 Script executed:
#!/bin/bash
# 3. Specific check: look for where $id might be set in adapter update flows
echo "=== Searching for any direct $id assignment in adapter/document update paths ==="
rg -n '\[\s*["\']?\$id["\']?\s*\].*=' src/ -g "*.php" -B3 -A3 | head -60
echo -e "\n=== Check what getInternalAttributes returns vs INTERNAL_ATTRIBUTES ==="
rg -n "getInternalAttributes\(\)" src/ -g "*.php" -B2 -A8 | head -50Length of output: 146
🏁 Script executed:
#!/bin/bash
# Corrected regex to search for $id field assignments without quote issues
echo "=== Check if $id field is written in adapter update operations ==="
rg -n '\$id|"\\$id"' src/Database/Adapter/ -g "*.php" -B3 -A3 | grep -i "update\|write\|\[" | head -80
echo -e "\n=== Look for existing $id/$sequence protection in updateDocument ==="
sed -n '4870,4950p' src/Database/Database.php | grep -n "\\$id\|\\$sequence\|updateDocument"
echo -e "\n=== Verify getInternalAttributes filtering logic ==="
sed -n '7764,7780p' src/Database/Database.phpLength of output: 8841
🏁 Script executed:
#!/bin/bash
# Search for documentation or comments about adapter update behavior
echo "=== Check adapter interface for update method signature ==="
rg -n "interface.*Adapter|abstract.*Adapter|function.*update.*Document" src/Database/Adapter/ -g "*.php" -A10 | head -60Length of output: 4180
Use dynamic internal attributes and lock down $id/$sequence consistently.
The code has two issues:
self::INTERNAL_ATTRIBUTESis static and ignores adapter configuration; use$this->getInternalAttributes()which respectssharedTablessetting (removes$tenantwhen appropriate).- After merge, user input can override
$idand$sequence. The codebase already protects$collection,$createdAt, and$tenantthis way;$idand$sequenceshould follow the same pattern to prevent identity/row-pointer mutation.
Apply:
$oldArray = $old->getArrayCopy();
$newArray = $document->getArrayCopy();
- $internalKeys = array_map(fn ($attr) => $attr['$id'], self::INTERNAL_ATTRIBUTES);
+ $internalKeys = array_map(fn ($attr) => $attr['$id'], $this->getInternalAttributes());
$internalAttrs = array_intersect_key($oldArray, array_flip($internalKeys));
$document = array_merge($internalAttrs, $newArray);
}
+ $document['$id'] = $old->getId();
+ $document['$sequence'] = $old->getSequence();
$document['$collection'] = $old->getAttribute('$collection');
$document['$createdAt'] = ($createdAt === null || !$this->preserveDates) ? $old->getCreatedAt() : $createdAt;🤖 Prompt for AI Agents
In src/Database/Database.php around lines 4916 to 4924, the merge logic uses the
static self::INTERNAL_ATTRIBUTES and allows user-supplied $id/$sequence to
override existing values; replace self::INTERNAL_ATTRIBUTES with
$this->getInternalAttributes() so internal attributes respect
adapter/sharedTables configuration, and after merging ensure $id and $sequence
are locked down by taking their values from the existing $old record (i.e.,
explicitly preserve $oldArray['$id'] and $oldArray['$sequence'] when building
the final $document, consistent with how $collection/$createdAt/$tenant are
protected).
There was a problem hiding this comment.
Since its replacement style , taking all the mandatory system fields from the old doc
There was a problem hiding this comment.
@ArnabChatterjee20k Thanks! One small but important guard is still needed.
Because array_merge prefers the right-hand array, if the incoming payload includes $id or $sequence, it will overwrite the preserved copies from the old doc. We’re currently re-applying $collection/$createdAt/$tenant after the merge, but not $id/$sequence. Also, for the schemaless path we should derive internal keys from getInternalAttributes() so $tenant is handled correctly when sharedTables is off.
Suggested minimal patch:
diff --git a/src/Database/Database.php b/src/Database/Database.php
@@ public function updateDocument(string $collection, string $id, Document $document): Document
- $oldArray = $old->getArrayCopy();
- $newArray = $document->getArrayCopy();
- $internalKeys = array_map(fn ($attr) => $attr['$id'], self::INTERNAL_ATTRIBUTES);
+ $oldArray = $old->getArrayCopy();
+ $newArray = $document->getArrayCopy();
+ // Use dynamic internal attributes (respects sharedTables / $tenant)
+ $internalKeys = array_map(fn ($attr) => $attr['$id'], $this->getInternalAttributes());
$internalAttrs = array_intersect_key($oldArray, array_flip($internalKeys));
$document = array_merge($internalAttrs, $newArray);
}
+ // Ensure identity and sequencing remain immutable on update
+ $document['$id'] = $old->getId();
+ $document['$sequence'] = $old->getSequence();
$document['$collection'] = $old->getAttribute('$collection'); // Make sure user doesn't switch collection ID
$document['$createdAt'] = ($createdAt === null || !$this->preserveDates) ? $old->getCreatedAt() : $createdAt;
if ($this->adapter->getSharedTables()) {
$document['$tenant'] = $old->getTenant(); // Make sure user doesn't switch tenant
}This keeps replacement semantics intact while preventing any accidental or malicious changes to $id/$sequence.
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/e2e/Adapter/Scopes/SchemalessTests.php (1)
130-151: Schemaless createDocuments should not expect exceptions for "invalid" internal attributesIn schemaless mode, these try/catch blocks should be removed; createDocuments should succeed without throwing, with at least one assertion remaining. This test currently contradicts prior guidance. Replace both try/catch blocks with a positive assertion on the insert count. Based on learnings.
Apply this diff:
@@ - try { - $docs = [ - new Document(['$id' => true, 'freeA' => 'doc1']), - new Document(['$id' => true, 'freeB' => 'test']), - new Document(['$id' => true]), - ]; - $database->createDocuments($colName, $docs); - } catch (\Throwable $e) { - $this->assertInstanceOf(StructureException::class, $e); - } - - try { - $docs = [ - new Document(['$createdAt' => true, 'freeA' => 'doc1']), - new Document(['$updatedAt' => true, 'freeB' => 'test']), - new Document(['$permissions' => 12]), - ]; - $database->createDocuments($colName, $docs); - } catch (\Throwable $e) { - $this->assertInstanceOf(StructureException::class, $e); - } + $docs = [ + new Document(['$createdAt' => true, 'freeA' => 'doc1']), + new Document(['$updatedAt' => true, 'freeB' => 'test']), + new Document(['$permissions' => 12]), + ]; + $this->assertEquals(3, $database->createDocuments($colName, $docs));
♻️ Duplicate comments (2)
src/Database/Database.php (2)
4925-4931: Lock down $id and $sequence after merge.Set these from $old to avoid accidental/malicious changes in payload. Keep existing $collection/$tenant guards.
- $document['$collection'] = $old->getAttribute('$collection'); // Make sure user doesn't switch collection ID + // Guard identity first + $document['$id'] = $old->getId(); + $document['$sequence'] = $old->getSequence(); + $document['$collection'] = $old->getAttribute('$collection'); // Make sure user doesn't switch collection ID
4916-4924: Prevent identity override; use dynamic internal-attr list.Right-side merge can overwrite $id/$sequence; and self::INTERNAL_ATTRIBUTES ignores sharedTables. Use getInternalAttributes() and lock identity after merge.
Apply:
- if ($this->adapter->getSupportForAttributes()) { + if ($this->adapter->getSupportForAttributes()) { $document = \array_merge($old->getArrayCopy(), $document->getArrayCopy()); } else { $oldArray = $old->getArrayCopy(); $newArray = $document->getArrayCopy(); - $internalKeys = array_map(fn ($attr) => $attr['$id'], self::INTERNAL_ATTRIBUTES); + $internalKeys = array_map(fn ($attr) => $attr['$id'], $this->getInternalAttributes()); $internalAttrs = array_intersect_key($oldArray, array_flip($internalKeys)); $document = array_merge($internalAttrs, $newArray); }Then (next hunk) lock identity:
+ // Ensure identity & sequencing are immutable on update + $document['$id'] = $old->getId(); + $document['$sequence'] = $old->getSequence();
🧹 Nitpick comments (2)
tests/e2e/Adapter/Scopes/SchemalessTests.php (2)
348-350: Prefer asserting on getAttributes() for clarityUse the attributes array rather than relying on ArrayAccess on Document for these key-absence checks.
Apply this diff:
- $this->assertArrayNotHasKey('score', $updatedDoc); - $this->assertArrayNotHasKey('type', $updatedDoc); + $this->assertArrayNotHasKey('score', $updatedDoc->getAttributes()); + $this->assertArrayNotHasKey('type', $updatedDoc->getAttributes()); @@ - $this->assertArrayNotHasKey('type', $updatedDoc2); + $this->assertArrayNotHasKey('type', $updatedDoc2->getAttributes());Also applies to: 364-364
1199-1263: Wrap bulk update/remove section with batch-support guard (optional)For consistency with other tests in this trait, guard only the bulk update/remove portion so single-document assertions still run on adapters that report no batch support.
Apply this diff:
@@ - // Test updateDocuments - bulk update with attribute removal + // Test updateDocuments - bulk update with attribute removal + if (!$database->getAdapter()->getSupportForBatchOperations()) { + // Skip bulk part; single-doc assertions above already validated behavior + $database->deleteCollection($col); + return; + }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/Database/Adapter/Mongo.php(2 hunks)src/Database/Database.php(3 hunks)tests/e2e/Adapter/Scopes/SchemalessTests.php(3 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-03T02:04:17.803Z
Learnt from: abnegate
PR: utopia-php/database#721
File: tests/e2e/Adapter/Scopes/DocumentTests.php:6418-6439
Timestamp: 2025-10-03T02:04:17.803Z
Learning: In tests/e2e/Adapter/Scopes/DocumentTests::testSchemalessDocumentInvalidInteralAttributeValidation (PHP), when the adapter reports getSupportForAttributes() === false (schemaless), the test should not expect exceptions from createDocuments for “invalid” internal attributes; remove try/catch and ensure the test passes without exceptions, keeping at least one assertion.
Applied to files:
tests/e2e/Adapter/Scopes/SchemalessTests.phpsrc/Database/Database.php
🧬 Code graph analysis (3)
tests/e2e/Adapter/Scopes/SchemalessTests.php (4)
src/Database/Database.php (5)
getAdapter(1251-1254)getDocument(3600-3724)updateDocument(4887-5113)find(6925-7065)updateDocuments(5136-5345)src/Database/Adapter/Mongo.php (5)
getSupportForAttributes(2702-2705)getDocument(1113-1148)updateDocument(1432-1459)find(1916-2078)updateDocuments(1474-1563)src/Database/Document.php (2)
getId(63-66)getAttributes(194-212)src/Database/Query.php (4)
Query(8-1169)startsWith(700-703)notEqual(464-467)equal(452-455)
src/Database/Adapter/Mongo.php (5)
src/Database/Adapter/SQL.php (1)
getSupportForAttributes(896-899)src/Database/Adapter.php (1)
getSupportForAttributes(921-921)src/Database/Adapter/Pool.php (1)
getSupportForAttributes(328-331)src/Database/Query.php (2)
Query(8-1169)equal(452-455)src/Database/Document.php (2)
getSequence(71-80)getArrayCopy(423-458)
src/Database/Database.php (5)
src/Database/Adapter/Mongo.php (1)
getSupportForAttributes(2702-2705)src/Database/Adapter/SQL.php (1)
getSupportForAttributes(896-899)src/Database/Adapter.php (1)
getSupportForAttributes(921-921)src/Database/Adapter/Pool.php (1)
getSupportForAttributes(328-331)src/Database/Document.php (2)
getArrayCopy(423-458)Document(12-470)
⏰ 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). (14)
- GitHub Check: Adapter Tests (SharedTables/SQLite)
- GitHub Check: Adapter Tests (SharedTables/MySQL)
- GitHub Check: Adapter Tests (MySQL)
- GitHub Check: Adapter Tests (SharedTables/MariaDB)
- GitHub Check: Adapter Tests (Schemaless/MongoDB)
- GitHub Check: Adapter Tests (SQLite)
- GitHub Check: Adapter Tests (SharedTables/Postgres)
- GitHub Check: Adapter Tests (SharedTables/MongoDB)
- GitHub Check: Adapter Tests (Mirror)
- GitHub Check: Adapter Tests (Postgres)
- GitHub Check: Adapter Tests (MongoDB)
- GitHub Check: Adapter Tests (Pool)
- GitHub Check: Adapter Tests (MariaDB)
- GitHub Check: Unit Test
ArnabChatterjee20k
force-pushed
the
dat-821
branch
from
53bfc16 to
9a1496e
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (4)
src/Database/Database.php (3)
4916-4923: Keep $id and $sequence immutable after merge.User payload can still override identity. Re-assert them from $old immediately after merge.
$createdAt = $document->getCreatedAt(); - $document = $this->mergeDocuments($old, $document); + $document = $this->mergeDocuments($old, $document); + // Keep identity immutable + $document['$id'] = $old->getId(); + $document['$sequence'] = $old->getSequence(); $document['$collection'] = $old->getAttribute('$collection'); // Make sure user doesn't switch collection ID
5024-5034: Schemaless key-diff: use dynamic internal attrs and symmetric set comparison.Respect sharedTables via getInternalAttributes() and detect both additions and removals.
- // to check addition and removal of fields in case of schemaless - if (!$this->adapter->getSupportForAttributes()) { - $internalFields = array_map(fn ($attr) => $attr['$id'], self::INTERNAL_ATTRIBUTES); - - $oldKeys = array_keys(array_diff_key($old->getArrayCopy(), array_flip($internalFields))); - $newKeys = array_keys(array_diff_key($document->getArrayCopy(), array_flip($internalFields))); - if (count($oldKeys) !== count($newKeys) || array_diff($oldKeys, $newKeys)) { - $shouldUpdate = true; - } - } + // Detect addition and removal of non-internal fields in schemaless mode + if (!$this->adapter->getSupportForAttributes()) { + $internalFields = array_map(fn ($attr) => $attr['$id'], $this->getInternalAttributes()); + $oldKeys = array_keys(array_diff_key($old->getArrayCopy(), array_flip($internalFields))); + $newKeys = array_keys(array_diff_key($document->getArrayCopy(), array_flip($internalFields))); + if ( + count($oldKeys) !== count($newKeys) + || array_diff($oldKeys, $newKeys) + || array_diff($newKeys, $oldKeys) + ) { + $shouldUpdate = true; + } + }
8373-8395: mergeDocuments(): preserve internals correctly and use dynamic internal attrs.Payload can override $id/$sequence as written. Let persisted internals win and derive internals dynamically.
- $internalKeys = array_map(fn ($attr) => $attr['$id'], self::INTERNAL_ATTRIBUTES); + $internalKeys = array_map(fn ($attr) => $attr['$id'], $this->getInternalAttributes()); $internalAttrs = array_intersect_key($oldArray, array_flip($internalKeys)); - - return array_merge($internalAttrs, $newArray); + // Preserve internal attributes from the existing document + return array_merge($newArray, $internalAttrs);src/Database/Adapter/Mongo.php (1)
1490-1495: Potential issue: _id may be included in schemaless updates.Line 1490 converts the updates document to an array and replaces
$sequencewith_id. When this$recordis passed toupdateDocumentsForSchemaless, it may contain the_idfield, which will be included in the$setoperation. MongoDB forbids modifying_idand will throw "Mod on _id not allowed" errors.Apply this diff to exclude
_idbefore calling the schemaless update method:$record = $this->replaceChars('$', '_', $updates->getArrayCopy()); try { if (!$this->getSupportForAttributes()) { + unset($record['_id']); // Never attempt to $set the primary key return $this->updateDocumentsForSchemaless($name, $documents, $record, $filters, $options); }Based on learnings (past review comments flagged this same issue).
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/Database/Adapter/Mongo.php(3 hunks)src/Database/Database.php(3 hunks)tests/e2e/Adapter/Scopes/DocumentTests.php(2 hunks)tests/e2e/Adapter/Scopes/SchemalessTests.php(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/e2e/Adapter/Scopes/SchemalessTests.php
- tests/e2e/Adapter/Scopes/DocumentTests.php
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-03T02:04:17.803Z
Learnt from: abnegate
PR: utopia-php/database#721
File: tests/e2e/Adapter/Scopes/DocumentTests.php:6418-6439
Timestamp: 2025-10-03T02:04:17.803Z
Learning: In tests/e2e/Adapter/Scopes/DocumentTests::testSchemalessDocumentInvalidInteralAttributeValidation (PHP), when the adapter reports getSupportForAttributes() === false (schemaless), the test should not expect exceptions from createDocuments for “invalid” internal attributes; remove try/catch and ensure the test passes without exceptions, keeping at least one assertion.
Applied to files:
src/Database/Database.php
🧬 Code graph analysis (2)
src/Database/Adapter/Mongo.php (4)
src/Database/Adapter/SQL.php (1)
getSupportForAttributes(896-899)src/Database/Adapter.php (1)
getSupportForAttributes(921-921)src/Database/Document.php (3)
getArrayCopy(423-458)removeAttribute(287-293)getAttributes(194-212)src/Database/Database.php (1)
Database(37-8396)
src/Database/Database.php (5)
src/Database/Adapter/Mongo.php (1)
getSupportForAttributes(2649-2652)src/Database/Adapter/SQL.php (1)
getSupportForAttributes(896-899)src/Database/Adapter.php (1)
getSupportForAttributes(921-921)src/Database/Adapter/Pool.php (1)
getSupportForAttributes(328-331)src/Database/Document.php (2)
getArrayCopy(423-458)Document(12-470)
⏰ 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). (14)
- GitHub Check: Adapter Tests (Schemaless/MongoDB)
- GitHub Check: Adapter Tests (SharedTables/MongoDB)
- GitHub Check: Adapter Tests (Pool)
- GitHub Check: Adapter Tests (SharedTables/SQLite)
- GitHub Check: Adapter Tests (SharedTables/MariaDB)
- GitHub Check: Adapter Tests (SharedTables/Postgres)
- GitHub Check: Adapter Tests (SharedTables/MySQL)
- GitHub Check: Adapter Tests (Mirror)
- GitHub Check: Adapter Tests (SQLite)
- GitHub Check: Adapter Tests (MySQL)
- GitHub Check: Adapter Tests (Postgres)
- GitHub Check: Adapter Tests (MariaDB)
- GitHub Check: Adapter Tests (MongoDB)
- GitHub Check: Unit Test
🔇 Additional comments (1)
src/Database/Adapter/Mongo.php (1)
3197-3241: Logic looks correct for schemaless attribute removal.The method properly identifies attributes that exist in old documents but not in the updates, excluding internal attributes, and constructs a
$unsetoperation for them. This ensures that when updating documents in schemaless mode, removed attributes are properly deleted from MongoDB.However, this method assumes the
$updatesparameter does not contain_id. Please ensure that the caller (line 1494) removes_idfrom$recordbefore invoking this method, as suggested in the previous comment.
| $updates = match (true) { | ||
| $this->getSupportForAttributes() => ['$set' => $record], | ||
| default => $record, | ||
| }; | ||
| $updateQuery = $updates; |
There was a problem hiding this comment.
Critical: Invalid MongoDB update syntax for schemaless mode.
When getSupportForAttributes() returns false (schemaless mode), the code sets $updateQuery = $record without any MongoDB update operator. MongoDB's update command requires operators like $set, $unset, etc. Passing a plain document will cause a MongoDB error.
Apply this diff to fix the issue by using $set in both cases:
- $updates = match (true) {
- $this->getSupportForAttributes() => ['$set' => $record],
- default => $record,
- };
- $updateQuery = $updates;
+ $updateQuery = ['$set' => $record];If the intent is to perform a full document replacement in schemaless mode, you should use MongoDB's replaceOne operation instead, which requires a separate implementation.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| $updates = match (true) { | |
| $this->getSupportForAttributes() => ['$set' => $record], | |
| default => $record, | |
| }; | |
| $updateQuery = $updates; | |
| $updateQuery = ['$set' => $record]; |
🤖 Prompt for AI Agents
In src/Database/Adapter/Mongo.php around lines 1449-1453, the code sets
$updateQuery = $record when getSupportForAttributes() is false which produces
invalid MongoDB update syntax; change the logic to always wrap the document in
an update operator (e.g., '$set' => $record) so MongoDB receives a valid update
document, and if the intended behavior was to replace the entire document
instead of updating fields, implement a separate replaceOne path rather than
passing a plain document to an update operation.
There was a problem hiding this comment.
its intentional for performing replacement instead of update
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for the MongoDB client update method implementation
echo "=== Searching for MongoDB Client update method ==="
ast-grep --pattern $'class Client {
$$$
update($$$) {
$$$
}
$$$
}'
echo -e "\n=== Searching for update method calls in tests ==="
rg -n "client->update\(" --type=php -C2
echo -e "\n=== Looking for replaceOne usage ==="
rg -n "replaceOne" --type=php -C2
echo -e "\n=== Checking tests for schemaless update behavior ==="
rg -n "getSupportForAttributes.*false" tests/ --type=php -C3
Length of output: 1540
🏁 Script executed:
#!/bin/bash
# Check the MongoDB client dependency
echo "=== Checking composer.json for mongo dependency ==="
cat composer.json | jq '.require' 2>/dev/null || cat composer.json | grep -A5 '"require"'
echo -e "\n=== Looking at the import statements in Mongo.php ==="
head -30 src/Database/Adapter/Mongo.php | grep -E "^use"
echo -e "\n=== Checking schemaless test file for update behavior ==="
cat tests/e2e/Adapter/Scopes/SchemalessTests.php | grep -A20 "testSchemalessUpdateDocumentWithQuery"
echo -e "\n=== Look for how getSupportForAttributes is controlled ==="
rg -n "setSupportForAttributes\(false\)" --type=php -C2
echo -e "\n=== Check if updateDocumentsForSchemaless uses same syntax ==="
sed -n '3220,3250p' src/Database/Adapter/Mongo.php
Length of output: 2768
@ArnabChatterjee20k, I appreciate the clarification about the intention. However, I've noticed an important inconsistency in the implementation:
In updateDocument (single document, schemaless mode):
$updates = match (true) {
$this->getSupportForAttributes() => ['$set' => $record],
default => $record, // Raw document for replacement
};But in updateDocumentsForSchemaless (batch update, schemaless mode):
$updateQuery = [
'$set' => $updates, // Still uses $set operator
];If the intention is to perform replacement (using a raw document without $set) in schemaless mode, shouldn't the batch update path (updateDocumentsForSchemaless) also use the raw document for consistency?
Currently, the single document update attempts replacement while the batch update performs a field update with $set. This creates inconsistent behavior between single and batch operations.
Could you clarify:
- Should both paths use replacement (raw document) in schemaless mode?
- Or should both use
$setfor updates? - Does the
utopia-php/mongoClient wrapper handle raw documents specially in itsupdate()method to perform replacement instead of update?
Understanding this will help ensure the implementation is consistent and aligns with the intended behavior.
✏️ Learnings added
Learnt from: ArnabChatterjee20k
PR: utopia-php/database#747
File: src/Database/Adapter/Mongo.php:1449-1453
Timestamp: 2025-10-29T12:27:57.045Z
Learning: In src/Database/Adapter/Mongo.php, when getSupportForAttributes() returns false (schemaless mode), the updateDocument method intentionally uses a raw document without $set operator for replacement-style updates, as confirmed by the repository maintainer ArnabChatterjee20k.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
tests/e2e/Adapter/Scopes/SchemalessTests.php (1)
1159-1327: Comprehensive test coverage for schemaless attribute removal.This test thoroughly validates replacement-style update behavior in schemaless mode, covering single updates, bulk updates, null value preservation, internal attribute preservation, and edge cases. The test logic is sound and correctly verifies that:
- Attributes not in the update payload are removed
- Null values are explicitly preserved as keys with null values
- Internal attributes remain intact across updates
- Empty document updates remove all non-internal attributes
Optional improvements:
The comment on line 1164 could be clearer. Consider revising:
- // in schemaless, if attributes are created and then if values are not provided then they are replaced with the default attribute automatically in the encode + // In schemaless mode, update operations use replacement-style semantics: attributes not provided in the update payload are removed from the documentAdditionally, while the comprehensive nature of this test is valuable, consider whether splitting it into focused test methods (e.g.,
testSchemalessUpdateSingleDocumentRemovesAttributes,testSchemalessUpdateBulkRemovesAttributes,testSchemalessUpdatePreservesNullValues) would improve maintainability and make test failures easier to diagnose.Based on learnings.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/e2e/Adapter/Scopes/SchemalessTests.php(3 hunks)
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: ArnabChatterjee20k
PR: utopia-php/database#747
File: src/Database/Adapter/Mongo.php:1449-1453
Timestamp: 2025-10-29T12:27:57.045Z
Learning: In src/Database/Adapter/Mongo.php, when getSupportForAttributes() returns false (schemaless mode), the updateDocument method intentionally uses a raw document without $set operator for replacement-style updates, as confirmed by the repository maintainer ArnabChatterjee20k.
📚 Learning: 2025-10-03T02:04:17.803Z
Learnt from: abnegate
PR: utopia-php/database#721
File: tests/e2e/Adapter/Scopes/DocumentTests.php:6418-6439
Timestamp: 2025-10-03T02:04:17.803Z
Learning: In tests/e2e/Adapter/Scopes/DocumentTests::testSchemalessDocumentInvalidInteralAttributeValidation (PHP), when the adapter reports getSupportForAttributes() === false (schemaless), the test should not expect exceptions from createDocuments for “invalid” internal attributes; remove try/catch and ensure the test passes without exceptions, keeping at least one assertion.
Applied to files:
tests/e2e/Adapter/Scopes/SchemalessTests.php
📚 Learning: 2025-10-29T12:27:57.045Z
Learnt from: ArnabChatterjee20k
PR: utopia-php/database#747
File: src/Database/Adapter/Mongo.php:1449-1453
Timestamp: 2025-10-29T12:27:57.045Z
Learning: In src/Database/Adapter/Mongo.php, when getSupportForAttributes() returns false (schemaless mode), the updateDocument method intentionally uses a raw document without $set operator for replacement-style updates, as confirmed by the repository maintainer ArnabChatterjee20k.
Applied to files:
tests/e2e/Adapter/Scopes/SchemalessTests.php
🧬 Code graph analysis (1)
tests/e2e/Adapter/Scopes/SchemalessTests.php (3)
src/Database/Database.php (6)
getDocument(3600-3724)updateDocument(4887-5105)find(6909-7049)updateDocuments(5128-5329)count(7146-7197)getCollection(1572-1588)src/Database/Adapter/Mongo.php (6)
getSupportForAttributes(2649-2652)getDocument(1113-1148)updateDocument(1432-1460)find(1863-2025)updateDocuments(1475-1511)count(2097-2184)src/Database/Document.php (4)
getId(63-66)getAttributes(194-212)getSequence(71-80)getPermissions(93-96)
⏰ 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). (14)
- GitHub Check: Adapter Tests (SharedTables/SQLite)
- GitHub Check: Adapter Tests (Postgres)
- GitHub Check: Adapter Tests (MariaDB)
- GitHub Check: Adapter Tests (Schemaless/MongoDB)
- GitHub Check: Adapter Tests (SharedTables/Postgres)
- GitHub Check: Adapter Tests (SharedTables/MongoDB)
- GitHub Check: Adapter Tests (Mirror)
- GitHub Check: Adapter Tests (Pool)
- GitHub Check: Adapter Tests (SharedTables/MariaDB)
- GitHub Check: Adapter Tests (SharedTables/MySQL)
- GitHub Check: Adapter Tests (SQLite)
- GitHub Check: Adapter Tests (MongoDB)
- GitHub Check: Adapter Tests (MySQL)
- GitHub Check: Unit Test
🔇 Additional comments (2)
tests/e2e/Adapter/Scopes/SchemalessTests.php (2)
348-349: LGTM! Correctly validates replacement-style update behavior.The test now properly verifies that attributes not included in the update payload ('score' and 'type') are removed rather than preserved. This aligns with the schemaless replacement-style update behavior where the entire document is replaced rather than merged.
Based on learnings.
364-364: LGTM! Consistent with replacement-style update validation.The assertion correctly verifies that 'type' is removed when not included in the update payload, consistent with the changes on lines 348-349.
Based on learnings.
|
closing this pr as it is decided to use upsert for a full replacement type update |
New Update behaviour for schemaless
Single document update: Replaces old document fields with new ones, removing any missing attributes.
Bulk update: Applies replacement-style updates to all matched documents, removing unspecified fields.
Filtered bulk update: Updates only matching documents while leaving others untouched.
Explicit null values: Keeps attributes set to null instead of removing them.
Internal attributes preservation: Always preserves system fields like $id, $sequence, $permissions, etc.
Empty document update: Deletes all non-internal attributes, retaining only system fields.
Non-matching document safety: Documents not included in the update query remain unchanged.
Summary by CodeRabbit
Bug Fixes
Tests
Chores