Skip to content

Bug update document uid #890

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
fogelito wants to merge 7 commits into from
+144 −5
Closed

Bug update document uid #890

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
0b6c76a
uid change
fogelito Jun 14, 2026
20dd6e4
Database.php
fogelito Jun 14, 2026
9898ed1
Add tests
fogelito Jun 14, 2026
b9d2a24
Uid change
fogelito Jun 14, 2026
083161e
try finally
fogelito Jun 14, 2026
ab0996d
repoint perms
fogelito Jun 14, 2026
49d63d6
Get current permissions from the database
fogelito Jun 14, 2026
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
37 changes: 32 additions & 5 deletions src/Database/Adapter/MariaDB.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -980,11 +980,15 @@ public function updateDocument(Document $collection, string $id, Document $docum
$attributes['_createdAt'] = $document->getCreatedAt();
$attributes['_updatedAt'] = $document->getUpdatedAt();
$attributes['_permissions'] = json_encode($document->getPermissions());
$attributes['_uid'] = $document->getId();

$name = $this->filter($collection);
$columns = '';

if (!$skipPermissions) {
$newUid = $document->offsetExists('$id') ? $document->getId() : $id;
Comment thread
greptile-apps[bot] marked this conversation as resolved.
$uidChanged = $newUid !== $id;

$sql = "
SELECT _type, _permission
FROM {$this->getSQLTable($name . '_perms')}
Expand All @@ -998,7 +1002,8 @@ public function updateDocument(Document $collection, string $id, Document $docum
* Get current permissions from the database
*/
$sqlPermissions = $this->getPDO()->prepare($sql);
$sqlPermissions->bindValue(':_uid', $document->getId());

$sqlPermissions->bindValue(':_uid', $id);

if ($this->sharedTables) {
$sqlPermissions->bindValue(':_tenant', $this->tenant);
Expand Down Expand Up @@ -1041,6 +1046,26 @@ public function updateDocument(Document $collection, string $id, Document $docum
}
}

/**
* Query to re-point existing permissions to the new UID
*/
if ($uidChanged) {
$sql = "
UPDATE {$this->getSQLTable($name . '_perms')}
SET _document = :_newUid
WHERE _document = :_uid
{$this->getTenantQuery($collection)}
";

$stmtRepointPermissions = $this->getPDO()->prepare($sql);
$stmtRepointPermissions->bindValue(':_uid', $id);
$stmtRepointPermissions->bindValue(':_newUid', $newUid);

if ($this->sharedTables) {
$stmtRepointPermissions->bindValue(':_tenant', $this->tenant);
}
}

/**
* Query to remove permissions
*/
Expand Down Expand Up @@ -1071,7 +1096,7 @@ public function updateDocument(Document $collection, string $id, Document $docum
$removeQuery = $this->trigger(Database::EVENT_PERMISSIONS_DELETE, $removeQuery);

$stmtRemovePermissions = $this->getPDO()->prepare($removeQuery);
$stmtRemovePermissions->bindValue(':_uid', $document->getId());
$stmtRemovePermissions->bindValue(':_uid', $newUid);

if ($this->sharedTables) {
$stmtRemovePermissions->bindValue(':_tenant', $this->tenant);
Expand Down Expand Up @@ -1119,7 +1144,7 @@ public function updateDocument(Document $collection, string $id, Document $docum

$stmtAddPermissions = $this->getPDO()->prepare($sql);

$stmtAddPermissions->bindValue(":_uid", $document->getId());
$stmtAddPermissions->bindValue(":_uid", $newUid);

if ($this->sharedTables) {
$stmtAddPermissions->bindValue(":_tenant", $this->tenant);
Expand Down Expand Up @@ -1168,7 +1193,7 @@ public function updateDocument(Document $collection, string $id, Document $docum

$sql = "
UPDATE {$this->getSQLTable($name)}
SET {$columns} _uid = :_newUid
SET " . \rtrim($columns, ',') . "
WHERE _id=:_sequence
{$this->getTenantQuery($collection)}
";
Expand All @@ -1178,7 +1203,6 @@ public function updateDocument(Document $collection, string $id, Document $docum
$stmt = $this->getPDO()->prepare($sql);

$stmt->bindValue(':_sequence', $document->getSequence());
$stmt->bindValue(':_newUid', $document->getId());

if ($this->sharedTables) {
$stmt->bindValue(':_tenant', $this->tenant);
Expand Down Expand Up @@ -1209,6 +1233,9 @@ public function updateDocument(Document $collection, string $id, Document $docum

$stmt->execute();

if (isset($stmtRepointPermissions)) {
$stmtRepointPermissions->execute();
}
if (isset($stmtRemovePermissions)) {
$stmtRemovePermissions->execute();
}
Expand Down
6 changes: 6 additions & 0 deletions src/Database/Database.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6170,6 +6170,12 @@ public function updateDocument(string $collection, string $id, Document $documen

$skipPermissionsUpdate = ($originalPermissions === $currentPermissions);
}

// UID change
if ($document->offsetExists('$id') && $document->getId() !== $id) {
$skipPermissionsUpdate = false;
}

$createdAt = $document->getCreatedAt();

$document = \array_merge($old->getArrayCopy(), $document->getArrayCopy());
Expand Down
106 changes: 106 additions & 0 deletions tests/e2e/Adapter/Scopes/DocumentTests.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1572,6 +1572,112 @@ public function testCreateDocumentDefaults(): void
$database->deleteCollection('defaults');
}

/**
* When a document's UID changes on update, its permission rows in the
* collection's _perms table must follow the new UID. Otherwise the old
* rows are orphaned and the renamed document is left with no permissions,
* even when the permission set itself was not changed.
*/
public function testUpdateDocumentChangeIdMigratesPermissions(): void
{
/** @var Database $database */
$database = $this->getDatabase();

$auth = $database->getAuthorization();

$collection = 'update_change_id_perms';

try {
// documentSecurity with no collection-level permissions: reads are
// governed purely by the document's rows in the _perms table.
$database->createCollection($collection, permissions: [], documentSecurity: true);
$this->assertEquals(true, $database->createAttribute($collection, 'name', Database::VAR_STRING, 128, false));

// Create a document whose read permission is scoped to a single role,
// so that find() must consult the _perms table to return it.
$document = $auth->skip(fn () => $database->createDocument($collection, new Document([
'$id' => 'old_id',
'name' => 'test',
'$permissions' => [
Permission::read(Role::user('alice')),
Permission::update(Role::user('alice')),
Permission::delete(Role::user('alice')),
],
])));
$this->assertEquals('old_id', $document->getId());

// Sanity: as alice the document is visible via the _perms table.
$auth->addRole(Role::user('alice')->toString());
$this->assertCount(1, $database->find($collection));

// Rename the document WITHOUT changing its permission set.
$renamed = $auth->skip(fn () => $database->updateDocument($collection, 'old_id', new Document(\array_merge(
$document->getArrayCopy(),
['$id' => 'new_id'],
))));
$this->assertEquals('new_id', $renamed->getId());

// The old UID must no longer resolve to a document.
$this->assertTrue($auth->skip(fn () => $database->getDocument($collection, 'old_id'))->isEmpty());

// The new UID must exist and keep its permissions on the main row.
$newDoc = $auth->skip(fn () => $database->getDocument($collection, 'new_id'));
$this->assertFalse($newDoc->isEmpty());
$this->assertContains(Permission::read(Role::user('alice')), $newDoc->getPermissions());

// The crucial check: the permission rows must have migrated to the new
// UID in the _perms table. As alice, find() (which joins _perms) must
// still return exactly the renamed document. With orphaned rows under
// the old UID this returns 0.
$found = $database->find($collection);
$this->assertCount(1, $found);
$this->assertEquals('new_id', $found[0]->getId());

/**
* Second scenario: change the UID AND the permission set in the same
* update. Drop alice's access and grant bob instead. The removed rows
* must be gone, the added rows must land under the new UID, and nothing
* may be left orphaned under the old UID.
*/
$rekeyed = $auth->skip(fn () => $database->updateDocument($collection, 'new_id', new Document(\array_merge(
$newDoc->getArrayCopy(),
[
'$id' => 'final_id',
'$permissions' => [
Permission::read(Role::user('bob')),
Permission::update(Role::user('bob')),
Permission::delete(Role::user('bob')),
],
],
))));
$this->assertEquals('final_id', $rekeyed->getId());

// The old UID must no longer resolve to a document.
$this->assertTrue($auth->skip(fn () => $database->getDocument($collection, 'new_id'))->isEmpty());

// The main row must reflect the new permission set.
$finalDoc = $auth->skip(fn () => $database->getDocument($collection, 'final_id'));
$this->assertFalse($finalDoc->isEmpty());
$this->assertContains(Permission::read(Role::user('bob')), $finalDoc->getPermissions());
$this->assertNotContains(Permission::read(Role::user('alice')), $finalDoc->getPermissions());

// alice's permission rows were removed: as alice nothing is returned.
$this->assertCount(0, $database->find($collection));

// bob's permission rows landed under the new UID: as bob the renamed
// document is returned via the _perms join.
$auth->addRole(Role::user('bob')->toString());
$foundAsBob = $database->find($collection);
$this->assertCount(1, $foundAsBob);
$this->assertEquals('final_id', $foundAsBob[0]->getId());
Comment on lines +1628 to +1672

@coderabbitai coderabbitai Bot Jun 14, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion | 🟠 Major | 🏗️ Heavy lift

Assert that stale _perms rows are removed, not just made unreachable.

These checks prove the renamed document is readable under the new UID, but they would still pass if the adapter copied permissions to new_id / final_id and left orphaned rows behind on the previous UIDs. That misses the exact regression this PR is trying to lock down. Please add a direct assertion on the underlying permission rows after each rename so old_id/new_id have zero _perms entries and only the live UID keeps the expected set.

🤖 Prompt for AI Agents 
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@tests/e2e/Adapter/Scopes/DocumentTests.php` around lines 1628 - 1672, The
test verifies that documents are readable after rename operations but doesn't
check that orphaned permission rows in the _perms table are actually deleted.
Add direct assertions on the underlying _perms table entries after each rename:
after the first rename to 'new_id', assert that 'old_id' has zero _perms
entries, and after the second rename to 'final_id', assert that 'new_id' has
zero _perms entries while 'final_id' contains the expected permission set. These
assertions will catch the regression where the adapter copies permissions to the
new UID but fails to clean up orphaned rows under the old UIDs.

} finally {
$auth->removeRole(Role::user('alice')->toString());
$auth->removeRole(Role::user('bob')->toString());

$auth->skip(fn () => $database->deleteCollection($collection));
}
}

public function testIncreaseDecrease(): Document
{
/** @var Database $database */
Expand Down
Loading