Skip to content

Partial update #886

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

Open
fogelito wants to merge 1 commit into
base: main
Choose a base branch
from
+6 −1
Open

Partial update #886

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
7 changes: 6 additions & 1 deletion src/Database/Database.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6159,6 +6159,7 @@ public function updateDocument(string $collection, string $id, Document $documen
$skipPermissionsUpdate = ($originalPermissions === $currentPermissions);
}
$createdAt = $document->getCreatedAt();
$inputKeys = \array_keys($document->getArrayCopy());

@greptile-apps greptile-apps Bot Jun 9, 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.

P2 No dedicated tests for the partial-update path

$inputKeys is captured here to restrict which columns the adapter writes. The existing testUpdateDocument test always passes a fully-populated document, so $inputKeys equals the full attribute set and the new filtering is never exercised. A test that passes a document with only a subset of attributes and then asserts the omitted columns retain their previous DB values would prevent regressions in this new code path.

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!


$document = \array_merge($old->getArrayCopy(), $document->getArrayCopy());
$document['$collection'] = $old->getAttribute('$collection'); // Make sure user doesn't switch collection ID
Expand Down Expand Up @@ -6334,7 +6335,11 @@ public function updateDocument(string $collection, string $id, Document $documen

$document = $this->adapter->castingBefore($collection, $document);

$this->adapter->updateDocument($collection, $id, $document, $skipPermissionsUpdate);
$internalKeys = \array_column(self::INTERNAL_ATTRIBUTES, '$id');
$allowedKeys = \array_flip(\array_merge($inputKeys, $internalKeys));
$adapterDocument = new Document(\array_intersect_key($document->getArrayCopy(), $allowedKeys));

$this->adapter->updateDocument($collection, $id, $adapterDocument, $skipPermissionsUpdate);
Comment on lines +6338 to +6342

@greptile-apps greptile-apps Bot Jun 9, 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.

P2 Stale return value for non-updated fields in high-concurrency scenarios

$adapterDocument correctly limits what the adapter writes to the DB (only $inputKeys + internal fields), but $document — which is returned to the caller — is the in-memory merged snapshot assembled at the start of the transaction (old values merged over by the user's input). If another transaction writes to a non-input field between this transaction's getDocument fetch and the adapter write, the caller will receive stale values for those non-updated fields in the returned document, even though the cache is purged and subsequent reads would be accurate.

This diverges from the previous behaviour where writing the full merged document meant the caller's return value always matched what was just persisted. Consider re-fetching the document (or merging in $adapterDocument's updated fields into the final return) to guarantee the caller receives a consistent snapshot.


$document = $this->adapter->castingAfter($collection, $document);

Expand Down
Loading