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 all commits
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
72 changes: 61 additions & 11 deletions src/Database/Adapter/Mongo.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1445,11 +1445,12 @@ public function updateDocument(Document $collection, string $id, Document $docum

try {
unset($record['_id']); // Don't update _id

$options = $this->getTransactionOptions();
$updateQuery = [
'$set' => $record,
];
$updates = match (true) {
$this->getSupportForAttributes() => ['$set' => $record],
default => $record,
};
$updateQuery = $updates;
Comment on lines +1449 to +1453
Copy link
Contributor

@coderabbitai coderabbitai bot Oct 29, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🔴 Critical

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.

Suggested change
$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.

Copy link
Contributor Author

@ArnabChatterjee20k ArnabChatterjee20k Oct 29, 2025

Choose a reason for hiding this comment

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

its intentional for performing replacement instead of update

Copy link
Contributor

@coderabbitai coderabbitai bot Oct 29, 2025

Choose a reason for hiding this comment

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

🧩 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:

  1. Should both paths use replacement (raw document) in schemaless mode?
  2. Or should both use $set for updates?
  3. Does the utopia-php/mongo Client wrapper handle raw documents specially in its update() 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.

$this->client->update($name, $filters, $updateQuery, $options);
} catch (MongoException $e) {
throw $this->processException($e);
Expand Down Expand Up @@ -1486,20 +1487,23 @@ public function updateDocuments(Document $collection, Document $updates, array $
$filters['_tenant'] = $this->getTenantFilters($collection->getId());
}

$record = $updates->getArrayCopy();
$record = $this->replaceChars('$', '_', $record);

$updateQuery = [
'$set' => $record,
];
$record = $this->replaceChars('$', '_', $updates->getArrayCopy());

try {
if (!$this->getSupportForAttributes()) {
return $this->updateDocumentsForSchemaless($name, $documents, $record, $filters, $options);
}

$updateQuery = [
'$set' => $record,
];

return $this->client->update(
$name,
$filters,
$updateQuery,
options: $options,
multi: true,
multi: true
);
} catch (MongoException $e) {
throw $this->processException($e);
Expand Down Expand Up @@ -3189,4 +3193,50 @@ public function getTenantQuery(string $collection, string $alias = ''): string
{
return '';
}

/**
* @param string $collection
* @param array<Document> $oldDocuments
* @param array<mixed> $updates
* @param array<mixed> $filters
* @param array<mixed> $options
* @return int
*/
private function updateDocumentsForSchemaless(string $collection, array $oldDocuments, array $updates, array $filters, array $options): int
{
$internalKeys = array_map(
fn ($attr) => str_replace('$', '_', $attr['$id']),
Database::INTERNAL_ATTRIBUTES
);
$unsetUnion = [];

foreach ($oldDocuments as $doc) {
if ($doc->offsetExists('$skipPermissionsUpdate')) {
$doc->removeAttribute('$skipPermissionsUpdate');
}
$attrs = $doc->getAttributes();
$attributes = $this->replaceChars('$', '_', $attrs);
foreach (array_keys($attributes) as $attributeKey) {
if (!array_key_exists($attributeKey, $updates) && !in_array($attributeKey, $internalKeys, true)) {
$unsetUnion[$attributeKey] = 1;
}
}
}

$updateQuery = [
'$set' => $updates,
];

if (!empty($unsetUnion)) {
$updateQuery['$unset'] = $unsetUnion;
}

return $this->client->update(
$collection,
$filters,
$updateQuery,
options: $options,
multi: true
);
}
}
37 changes: 35 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,7 @@ public function updateDocument(string $collection, string $id, Document $documen
$skipPermissionsUpdate = ($originalPermissions === $currentPermissions);
}
$createdAt = $document->getCreatedAt();

$document = \array_merge($old->getArrayCopy(), $document->getArrayCopy());
$document = $this->mergeDocuments($old, $document);
$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 +5021,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;
}
}

$updatePermissions = [
...$collection->getUpdate(),
...($documentSecurity ? $old->getUpdate() : [])
Expand Down Expand Up @@ -8360,4 +8370,27 @@ protected function encodeSpatialData(mixed $value, string $type): string
throw new DatabaseException('Unknown spatial type: ' . $type);
}
}

/**
* @param Document $old
* @param Document $new
* @return array<mixed>
*/
protected function mergeDocuments(Document $old, Document $new): array
{
switch (true) {
case $this->adapter->getSupportForAttributes():
return \array_merge($old->getArrayCopy(), $new->getArrayCopy());

// schemaless behaviour
default:
$oldArray = $old->getArrayCopy();
$newArray = $new->getArrayCopy();

$internalKeys = array_map(fn ($attr) => $attr['$id'], self::INTERNAL_ATTRIBUTES);
$internalAttrs = array_intersect_key($oldArray, array_flip($internalKeys));

return array_merge($internalAttrs, $newArray);
}
}
}
22 changes: 19 additions & 3 deletions tests/e2e/Adapter/Scopes/DocumentTests.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6170,14 +6170,26 @@ public function testCreateUpdateDocumentsMismatch(): void
$this->assertEquals(2, $database->updateDocuments($colName, new Document(['key' => 'new doc'])));
$doc = $database->getDocument($colName, 'doc4');
$this->assertEquals('doc4', $doc->getId());
$this->assertEquals('value', $doc->getAttribute('value'));
if (!$database->getAdapter()->getSupportForAttributes()) {
// not assertArrayNotHasKey as createAttribute is done previously
// value would be returned in attributes but schemaless updateDocuments do replace the old document with new document
// so null
$this->assertNull($doc->getAttribute('value'));
} else {
$this->assertEquals('value', $doc->getAttribute('value'));

}

$addedDocs = $database->find($colName);
$this->assertCount(2, $addedDocs);
foreach ($addedDocs as $doc) {
$this->assertNotEmpty($doc->getPermissions());
$this->assertCount(3, $doc->getPermissions());
$this->assertEquals('value', $doc->getAttribute('value'));
if (!$database->getAdapter()->getSupportForAttributes()) {
$this->assertNull($doc->getAttribute('value'));
} else {
$this->assertEquals('value', $doc->getAttribute('value'));
}
}
$database->deleteCollection($colName);
}
Expand Down Expand Up @@ -6561,7 +6573,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
176 changes: 173 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,174 @@ 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);

// Test updateDocuments - bulk update with attribute removal
$docs = [
new Document(['$id' => 'docA', 'key' => 'keepA', 'extra' => 'removeA', '$permissions' => $permissions]),
new Document(['$id' => 'docB', 'key' => 'keepB', 'extra' => 'removeB', 'more' => 'data', '$permissions' => $permissions]),
new Document(['$id' => 'docC', 'key' => 'keepC', 'extra' => 'removeC', '$permissions' => $permissions]),
];
$this->assertEquals(3, $database->createDocuments($col, $docs));

// Verify all documents have both 'key' and 'extra'
$allDocs = $database->find($col);
$this->assertCount(4, $allDocs); // 3 new + 1 old (docS)

foreach ($allDocs as $doc) {
if (in_array($doc->getId(), ['docA', 'docB', 'docC'])) {
$this->assertArrayHasKey('extra', $doc->getAttributes());
}
}

// Bulk update all new docs: keep 'key', remove 'extra' and 'more'
$updatedCount = $database->updateDocuments($col, new Document(['key' => 'updatedBulk']), [Query::startsWith('$id', 'doc'),Query::notEqual('$id', 'docS')]);
$this->assertEquals(3, $updatedCount);

// Verify 'extra' and 'more' fields are removed from all updated docs
$updatedDocs = $database->find($col, [Query::startsWith('$id', 'doc'),Query::notEqual('$id', 'docS')]);
$this->assertCount(3, $updatedDocs);

foreach ($updatedDocs as $doc) {
$this->assertEquals('updatedBulk', $doc->getAttribute('key'));
$this->assertArrayNotHasKey('extra', $doc->getAttributes());
if ($doc->getId() === 'docB') {
$this->assertArrayNotHasKey('more', $doc->getAttributes());
}
}

// Verify docS (not in update query) is unchanged
$docS = $database->getDocument($col, 'docS');
$this->assertEquals('single2', $docS->getAttribute('key'));
$this->assertArrayNotHasKey('extra', $docS);

// Update documents with query filter - partial update
$database->updateDocuments(
$col,
new Document(['label' => 'tagged']),
[Query::equal('$id', ['docA', 'docC'])]
);

$docA = $database->getDocument($col, 'docA');
$docB = $database->getDocument($col, 'docB');
$docC = $database->getDocument($col, 'docC');

$this->assertEquals('tagged', $docA->getAttribute('label'));
$this->assertArrayNotHasKey('label', $docB->getAttributes());
$this->assertEquals('tagged', $docC->getAttribute('label'));

// Verify 'key' is still preserved in docB and not in others
$this->assertEquals('updatedBulk', $docB->getAttribute('key'));

foreach (['docA','docC'] as $doc) {
$this->assertArrayNotHasKey('key', $database->getDocument($col, $doc)->getAttributes());
}

// verify docs is still preserved(untouched)
$docS = $database->getDocument($col, 'docS');
$this->assertEquals('single2', $docS->getAttribute('key'));

// 1) Null Value Preservation vs Removal
$docNull = $database->createDocument($col, new Document([
'$id' => 'docNull',
'$permissions' => $permissions,
'keepMe' => 'k1',
'removeMe' => 'r1',
'nullable' => 'n1'
]));
$this->assertEquals('docNull', $docNull->getId());

$docNullUpdated = $database->updateDocument($col, 'docNull', new Document([
'keepMe' => 'k2',
'nullable' => null // explicitly set null should be preserved as key with null value
]));
$this->assertEquals('k2', $docNullUpdated->getAttribute('keepMe'));
$this->assertArrayHasKey('nullable', $docNullUpdated->getAttributes());
$this->assertNull($docNullUpdated->getAttribute('nullable'));
$this->assertArrayNotHasKey('removeMe', $docNullUpdated->getAttributes());

$docNullRefetch = $database->getDocument($col, 'docNull');
$this->assertEquals('k2', $docNullRefetch->getAttribute('keepMe'));
$this->assertArrayHasKey('nullable', $docNullRefetch->getAttributes());
$this->assertNull($docNullRefetch->getAttribute('nullable'));
$this->assertArrayNotHasKey('removeMe', $docNullRefetch->getAttributes());

// 2) Internal attributes preservation ($id, $sequence, $permissions)
$before = $database->getDocument($col, 'docS');
$this->assertEquals('docS', $before->getId());
$beforeSequence = $before->getSequence();
$beforePermissions = $before->getPermissions();
$this->assertNotEmpty($beforeSequence);
$this->assertGreaterThanOrEqual(1, count($beforePermissions));

$after = $database->updateDocument($col, 'docS', new Document(['key' => 'single3']));
$this->assertEquals('docS', $after->getId());
$this->assertEquals('single3', $after->getAttribute('key'));
$this->assertEquals($beforeSequence, $after->getSequence());
foreach ($beforePermissions as $perm) {
$this->assertContains($perm, $after->getPermissions());
}

$afterRefetch = $database->getDocument($col, 'docS');
$this->assertEquals('docS', $afterRefetch->getId());
$this->assertEquals($beforeSequence, $afterRefetch->getSequence());
foreach ($beforePermissions as $perm) {
$this->assertContains($perm, $afterRefetch->getPermissions());
}

// 3) Update with empty document (removes all non-internal attributes)
$noOp = $database->updateDocument($col, 'docS', new Document([]));
$this->assertEquals('docS', $noOp->getId());
$this->assertArrayNotHasKey('key', $noOp->getAttributes());

$noOpRefetch = $database->getDocument($col, 'docS');
$this->assertArrayNotHasKey('key', $noOpRefetch->getAttributes());
// Internal attributes should still be present
$this->assertEquals('docS', $noOpRefetch->getId());
$this->assertNotEmpty($noOpRefetch->getSequence());
$this->assertEquals($col, $noOpRefetch->getCollection());
$this->assertTrue(is_string($noOpRefetch->getAttribute('$createdAt')));
$this->assertTrue(is_string($noOpRefetch->getAttribute('$updatedAt')));
$this->assertGreaterThanOrEqual(1, count($noOpRefetch->getPermissions()));
$database->deleteCollection($col);
}
}