Skip to content

Skip unnecessary getDocument() for increment upserts #830

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
premtsd-code wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Skip unnecessary getDocument() for increment upserts #830

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
45d733a
Skip unnecessary getDocument() for increment upserts
premtsd-code Mar 8, 2026
637f0e4
Add non-increment attribute assertions to skip pre-read test
premtsd-code Mar 8, 2026
bbfbf81
Only skip pre-read when authorization is disabled
premtsd-code Mar 8, 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
17 changes: 9 additions & 8 deletions src/Database/Adapter/Mongo.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1655,20 +1655,21 @@ public function upsertDocuments(Document $collection, string $attribute, array $
$unsetFields = $this->getUpsertAttributeRemovals($oldDocument, $document, $record);

if (!empty($attribute)) {
// Get the attribute value before removing it from $set
// Get the attribute value before removing it from the record
$attributeValue = $record[$attribute] ?? 0;

// Remove the attribute from $set since we're incrementing it
// it is requierd to mimic the behaver of SQL on duplicate key update
unset($record[$attribute]);

// Also remove from unset if it was there
unset($unsetFields[$attribute]);

// Increment the specific attribute and update all other fields
// For increment upserts, only update _updatedAt on existing docs.
// All other fields use $setOnInsert (only applied on insert, not update),
// matching SQL ON DUPLICATE KEY UPDATE which only touches value + _updatedAt.
$updatedAt = $record['_updatedAt'];
unset($record['_updatedAt']);

$update = [
'$inc' => [$attribute => $attributeValue],
'$set' => $record
'$set' => ['_updatedAt' => $updatedAt],
'$setOnInsert' => $record
];

if (!empty($unsetFields)) {
Expand Down
10 changes: 8 additions & 2 deletions src/Database/Database.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7054,8 +7054,11 @@ public function upsertDocumentsWithIncrease(
$created = 0;
$updated = 0;
$seenIds = [];
$skipPreRead = !empty($attribute);
foreach ($documents as $key => $document) {
if ($this->getSharedTables() && $this->getTenantPerDocument()) {
if ($skipPreRead) {
$old = new Document();
} elseif ($this->getSharedTables() && $this->getTenantPerDocument()) {
$old = $this->authorization->skip(fn () => $this->withTenant($document->getTenant(), fn () => $this->silent(fn () => $this->getDocument(
$collection->getId(),
$document->getId(),
Expand All @@ -7080,9 +7083,12 @@ public function upsertDocumentsWithIncrease(

$regularUpdatesUserOnly = \array_diff_key($regularUpdates, \array_flip($internalKeys));

// When skipping pre-read (increment upserts), always skip permission updates.
// The SQL ON DUPLICATE KEY UPDATE only touches the increment column + _updatedAt,
// so existing permissions are preserved by the DB, and new docs get empty permissions.
$skipPermissionsUpdate = true;

if ($document->offsetExists('$permissions')) {
if (!$skipPreRead && $document->offsetExists('$permissions')) {
$originalPermissions = $old->getPermissions();
Comment on lines +7086 to 7092
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai bot Mar 8, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

Explicit permissions are dropped on create-via-increment-upsert.

This path now forces $skipPermissionsUpdate = true, and the later backfill uses $old->getPermissions(). With skipPreRead, $old is always empty, so getPermissions() resolves to []. Any caller that creates a document through increment upsert with explicit $permissions will silently lose them.

🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In `@src/Database/Database.php` around lines 7086 - 7092, The code forces
$skipPermissionsUpdate = true for increment upserts which then backfills
permissions from $old->getPermissions(), but when $skipPreRead is true $old is
empty and explicit $permissions on the incoming $document are lost; update the
logic in the increment-upsert path (around the variables $skipPermissionsUpdate,
$skipPreRead, $document, $old and the call to $old->getPermissions()) to
preserve explicit permissions by either (a) not forcing $skipPermissionsUpdate
when the incoming $document contains an explicit '$permissions' field, or (b)
when $skipPreRead is true, read permissions from
$document->offsetGet('$permissions') and use those instead of
$old->getPermissions() so explicit permissions are retained on
create-via-increment-upsert.

Copy link
Copy Markdown
Contributor Author

@premtsd-code premtsd-code Mar 8, 2026

Choose a reason for hiding this comment

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

That's a contradictory usage — you don't set permissions on documents when auth is disabled, because there's no auth layer to enforce them.

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai bot Mar 8, 2026

Choose a reason for hiding this comment

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

@premtsd-code Agreed. Given skipPreRead is only enabled for increment upserts when authorization is disabled ($skipPreRead = !empty($attribute) && !$this->authorization->getStatus()), ignoring any incoming $permissions in this path is intentional and correct. The existing comment already notes that new docs get empty permissions in this mode; if you’d like, I can add a short clarifier that explicit incoming $permissions are ignored specifically because auth is disabled to prevent future confusion. Otherwise, we can consider this concern resolved.

✏️ Learnings added 
Learnt from: premtsd-code
Repo: utopia-php/database PR: 830
File: src/Database/Database.php:7086-7092
Timestamp: 2026-03-08T22:55:17.883Z
Learning: In src/Database/Database.php, Database::upsertDocumentsWithIncrease() sets $skipPreRead = !empty($attribute) && !$this->authorization->getStatus(), so the pre-read-skip path only runs when authorization is disabled. In this path, explicit $permissions on incoming documents are intentionally ignored; updates preserve existing permissions, and creates default to empty permissions ([]), since no auth layer enforces them. Stated by maintainer premtsd-code on PR `#830`.

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.

🧠 Learnings used 
Learnt from: ArnabChatterjee20k
Repo: utopia-php/database PR: 747
File: src/Database/Adapter/Mongo.php:1449-1453
Timestamp: 2025-10-29T12:27:57.071Z
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.

Learnt from: abnegate
Repo: utopia-php/database PR: 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.

$currentPermissions = $document->getPermissions();

Expand Down
112 changes: 112 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 @@ -874,6 +874,118 @@ public function testUpsertDocumentsInc(): void
}
}

public function testUpsertDocumentsIncSkipPreRead(): void
{
/** @var Database $database */
$database = $this->getDatabase();

if (!$database->getAdapter()->getSupportForUpserts()) {
$this->expectNotToPerformAssertions();
return;
}

// Mirrors the stats workflow: no permissions, authorization disabled
$database->getAuthorization()->disable();

try {
$database->createCollection(__FUNCTION__);
$database->createAttribute(__FUNCTION__, 'value', Database::VAR_INTEGER, 0, false);
$database->createAttribute(__FUNCTION__, 'metric', Database::VAR_STRING, 128, false);

// Step 1: Create-via-upsert (documents don't exist yet — no pre-read needed)
$documents = [
new Document([
'$id' => 'stat_a',
'value' => 10,
'metric' => 'network.requests',
]),
new Document([
'$id' => 'stat_b',
'value' => 20,
'metric' => 'documents.count',
]),
];

$count = $database->upsertDocumentsWithIncrease(
collection: __FUNCTION__,
attribute: 'value',
documents: $documents
);
$this->assertSame(2, $count);

$results = $database->find(__FUNCTION__);
$this->assertCount(2, $results);

$byId = [];
foreach ($results as $doc) {
$byId[$doc->getId()] = $doc;
}

$this->assertSame(10, $byId['stat_a']->getAttribute('value'));
$this->assertSame(20, $byId['stat_b']->getAttribute('value'));
$this->assertSame('network.requests', $byId['stat_a']->getAttribute('metric'));
$this->assertSame('documents.count', $byId['stat_b']->getAttribute('metric'));

$createdAtA = $byId['stat_a']->getCreatedAt();
$createdAtB = $byId['stat_b']->getCreatedAt();
$this->assertNotNull($createdAtA);
$this->assertNotNull($createdAtB);

// Step 2: Increment existing documents (update-via-upsert)
$documents[0]->setAttribute('value', 5);
$documents[1]->setAttribute('value', 3);

$count = $database->upsertDocumentsWithIncrease(
collection: __FUNCTION__,
attribute: 'value',
documents: $documents
);
$this->assertSame(2, $count);

$results = $database->find(__FUNCTION__);
$byId = [];
foreach ($results as $doc) {
$byId[$doc->getId()] = $doc;
}

$this->assertSame(15, $byId['stat_a']->getAttribute('value'));
$this->assertSame(23, $byId['stat_b']->getAttribute('value'));

// Non-increment attribute must not change on duplicate key update
$this->assertSame('network.requests', $byId['stat_a']->getAttribute('metric'));
$this->assertSame('documents.count', $byId['stat_b']->getAttribute('metric'));

// Step 3: Verify $createdAt is preserved after increment
$this->assertSame($createdAtA, $byId['stat_a']->getCreatedAt());
$this->assertSame($createdAtB, $byId['stat_b']->getCreatedAt());

// Step 4: Third increment to confirm repeated upserts work
$documents[0]->setAttribute('value', 1);
$documents[1]->setAttribute('value', 1);

$database->upsertDocumentsWithIncrease(
collection: __FUNCTION__,
attribute: 'value',
documents: $documents
);

$results = $database->find(__FUNCTION__);
$byId = [];
foreach ($results as $doc) {
$byId[$doc->getId()] = $doc;
}

$this->assertSame(16, $byId['stat_a']->getAttribute('value'));
$this->assertSame(24, $byId['stat_b']->getAttribute('value'));
$this->assertSame('network.requests', $byId['stat_a']->getAttribute('metric'));
$this->assertSame('documents.count', $byId['stat_b']->getAttribute('metric'));
$this->assertSame($createdAtA, $byId['stat_a']->getCreatedAt());
$this->assertSame($createdAtB, $byId['stat_b']->getCreatedAt());
} finally {
$database->getAuthorization()->reset();
}
}

public function testUpsertDocumentsPermissions(): void
{
/** @var Database $database */
Expand Down