Skip to content

fix - replacement style update for schemaless #747

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
ArnabChatterjee20k wants to merge 3 commits into from
Closed

fix - replacement style update for schemaless #747

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
0598fe7
fix - update document replacing the exisint document instead of parti…
ArnabChatterjee20k Oct 27, 2025
9a1496e
added bulk update support for document replacement in the updateDocum…
ArnabChatterjee20k Oct 29, 2025
ef4809c
added few more tests for edge cases
ArnabChatterjee20k Oct 29, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions src/Database/Adapter/Mongo.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1447,9 +1447,9 @@ public function updateDocument(Document $collection, string $id, Document $docum
unset($record['_id']); // Don't update _id

$options = $this->getTransactionOptions();
$updateQuery = [
$updateQuery = $this->getSupportForAttributes() ? [
'$set' => $record,
];
] : $record;
$this->client->update($name, $filters, $updateQuery, $options);
} catch (MongoException $e) {
throw $this->processException($e);
Expand Down
22 changes: 20 additions & 2 deletions src/Database/Database.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4913,8 +4913,15 @@ public function updateDocument(string $collection, string $id, Document $documen
$skipPermissionsUpdate = ($originalPermissions === $currentPermissions);
}
$createdAt = $document->getCreatedAt();

$document = \array_merge($old->getArrayCopy(), $document->getArrayCopy());
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);
}
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot Oct 27, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

🛠️ 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 -30

Length 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 -80

Length 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 -50

Length 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.php

Length 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 -60

Length of output: 4180

Use dynamic internal attributes and lock down $id/$sequence consistently.

The code has two issues:

  1. self::INTERNAL_ATTRIBUTES is static and ignores adapter configuration; use $this->getInternalAttributes() which respects sharedTables setting (removes $tenant when appropriate).
  2. After merge, user input can override $id and $sequence. The codebase already protects $collection, $createdAt, and $tenant this way; $id and $sequence should 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).

Copy link
Copy Markdown
Contributor Author

@ArnabChatterjee20k ArnabChatterjee20k Oct 27, 2025

Choose a reason for hiding this comment

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

Since its replacement style , taking all the mandatory system fields from the old doc

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot Oct 27, 2025

Choose a reason for hiding this comment

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

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

$document['$collection'] = $old->getAttribute('$collection'); // Make sure user doesn't switch collection ID
$document['$createdAt'] = ($createdAt === null || !$this->preserveDates) ? $old->getCreatedAt() : $createdAt;

Expand Down Expand Up @@ -5022,6 +5029,17 @@ public function updateDocument(string $collection, string $id, Document $documen
}
}

// 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;
}
}

Comment thread
ArnabChatterjee20k marked this conversation as resolved.
$updatePermissions = [
...$collection->getUpdate(),
...($documentSecurity ? $old->getUpdate() : [])
Expand Down
6 changes: 5 additions & 1 deletion tests/e2e/Adapter/Scopes/DocumentTests.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6561,7 +6561,11 @@ public function testUpdateDocumentSuccessiveCallsDoNotResetDefaults(): void
{
/** @var Database $database */
$database = static::getDatabase();

// // skipping as schemaless doesn't look to the attributes metadata
if (!$database->getAdapter()->getSupportForAttributes()) {
$this->expectNotToPerformAssertions();
return;
}
$collectionId = 'successive_update_single';
Authorization::cleanRoles();
Authorization::setRole(Role::any()->toString());
Expand Down
49 changes: 46 additions & 3 deletions tests/e2e/Adapter/Scopes/SchemalessTests.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -345,8 +345,8 @@ public function testSchemalessUpdateDocumentWithQuery(): void
$this->assertEquals('updated', $updatedDoc->getAttribute('status'));
$this->assertEquals('2023-01-01', $updatedDoc->getAttribute('lastModified'));
$this->assertEquals('added', $updatedDoc->getAttribute('newAttribute'));
$this->assertEquals('user', $updatedDoc->getAttribute('type')); // Existing attributes preserved
$this->assertEquals(100, $updatedDoc->getAttribute('score'));
$this->assertArrayNotHasKey('score', $updatedDoc);
$this->assertArrayNotHasKey('type', $updatedDoc);

$retrievedDoc = $database->getDocument($colName, 'doc1');
$this->assertEquals('updated', $retrievedDoc->getAttribute('status'));
Expand All @@ -361,7 +361,7 @@ public function testSchemalessUpdateDocumentWithQuery(): void
$this->assertEquals('value1', $updatedDoc2->getAttribute('customField1'));
$this->assertEquals(42, $updatedDoc2->getAttribute('customField2'));
$this->assertEquals(['array', 'of', 'values'], $updatedDoc2->getAttribute('customField3'));
$this->assertEquals('admin', $updatedDoc2->getAttribute('type')); // Original attributes preserved
$this->assertArrayNotHasKey('type', $updatedDoc2);

$database->deleteCollection($colName);
}
Expand Down Expand Up @@ -1155,4 +1155,47 @@ public function testSchemalessDates(): void

$database->deleteCollection($col);
}

public function testSchemalessRemoveAttributesByUpdate(): void
{
/** @var Database $database */
$database = static::getDatabase();

// 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
if ($database->getAdapter()->getSupportForAttributes()) {
$this->expectNotToPerformAssertions();
return;
}

$col = uniqid('sl_update_remove');
$database->createCollection($col);

$permissions = [
Permission::read(Role::any()),
Permission::write(Role::any()),
Permission::update(Role::any()),
Permission::delete(Role::any())
];

$single = $database->createDocument($col, new Document(['$id' => 'docS', 'key' => 'single', 'extra' => 'yes', '$permissions' => $permissions]));
$this->assertEquals('docS', $single->getId());
$this->assertEquals('yes', $single->getAttribute('extra'));
$this->assertEquals('single', $single->getAttribute('key'));

// before removing attribute
$doc = $database->getDocument($col, 'docS');
$this->assertEquals('yes', $doc->getAttribute('extra'));
$this->assertEquals('single', $doc->getAttribute('key'));

// removing attribute
$doc = $database->updateDocument($col, 'docS', new Document(['$id' => 'docS','key' => 'single2']));
$this->assertEquals('single2', $doc->getAttribute('key'));
$this->assertArrayNotHasKey('extra', $doc);

$doc = $database->getDocument($col, 'docS');
$this->assertEquals('single2', $doc->getAttribute('key'));
$this->assertArrayNotHasKey('extra', $doc);

$database->deleteCollection($col);
}
}