Uh oh!
There was an error while loading. Please reload this page.
Skip unnecessary getDocument() for increment upserts - #830
Open
premtsd-code wants to merge 3 commits into
Open
Conversation
When upsertDocumentsWithIncrease() is called with an attribute (stats increment-only), the pre-read SELECT per document is completely unnecessary. SQL ON DUPLICATE KEY UPDATE and Mongo $inc handle insert-or-increment atomically without needing the old document. The old document was fetched but never meaningfully used in the increment path — change detection short-circuits on !empty($attribute), auth is disabled in workers, and the UPDATE clause only touches the increment column + _updatedAt. Also fixes Mongo adapter to use $setOnInsert for non-increment fields during increment upserts, matching SQL behavior where ON DUPLICATE KEY UPDATE only modifies the specified columns.
Contributor
📝 WalkthroughWalkthroughSeparates incrementing upsert payloads in the Mongo adapter ($inc, $set {_updatedAt}, $setOnInsert remaining fields) and adds a skipPreRead path in Database for increment-upserts that bypasses pre-read and permission updates when appropriate, plus change-detection to avoid unnecessary writes. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Database
participant MongoAdapter
participant MongoDB
rect rgba(200, 150, 255, 0.5)
Note over Database,MongoAdapter: Increment-upsert flow (skipPreRead = true)
Client->>Database: upsertDocuments(request with increment attribute)
Database->>Database: detect increment -> set skipPreRead=true, skipPermissionsUpdate=true
Database->>Database: oldDoc := empty
Database->>Database: compute hasChanges (operators, updatedAt)
Database->>MongoAdapter: upsertDocuments(payload, skipPreRead)
MongoAdapter->>MongoAdapter: build update: $inc, $set {_updatedAt}, $setOnInsert
MongoAdapter->>MongoDB: updateOne(filter, {$inc,$set,$setOnInsert})
MongoDB-->>MongoAdapter: ack (inserted/updated)
MongoAdapter-->>Database: result
Database-->>Client: response (permissions unchanged)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/Database/Adapter/Mongo.php (1)
1657-1677:⚠️ Potential issue | 🟡 MinorDrop
$unsetfrom the increment-upsert path.Line 1675 still lets increment upserts remove unrelated fields on conflict in schemaless mode when
$oldDocumentis populated. That breaks the SQL-aligned contract here, where conflict updates should only touch the incremented field and_updatedAt.Suggested fix
if (!empty($attribute)) { // Get the attribute value before removing it from the record $attributeValue = $record[$attribute] ?? 0; unset($record[$attribute]); unset($unsetFields[$attribute]); // 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' => ['_updatedAt' => $updatedAt], '$setOnInsert' => $record ]; -- if (!empty($unsetFields)) {- $update['$unset'] = $unsetFields;- } } else { // Update all fields $update = [ '$set' => $record ]; if (!empty($unsetFields)) { $update['$unset'] = $unsetFields; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Database/Adapter/Mongo.php` around lines 1657 - 1677, The increment-upsert branch is still attaching $unset (via $unsetFields) which allows removal of unrelated fields; update the logic in the increment-upsert path (the block that builds $update using $inc, $set => ['_updatedAt'], and '$setOnInsert' => $record, referencing $attribute, $attributeValue, $updatedAt and $setOnInsert) to not include $unset at all for increment upserts—i.e., remove or guard the code that sets $update['$unset'] = $unsetFields so $unsetFields is ignored in this path and only $inc and _updatedAt are touched on conflict.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/Database/Database.php`:
- Around line 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.
In `@tests/e2e/Adapter/Scopes/DocumentTests.php`:
- Line 888: The authorization override currently calls
$database->getAuthorization()->disable() without ensuring it gets reset on
failure; wrap the override so it always resets by replacing the direct disable()
call with a scoped call to $database->getAuthorization()->skip(function () { ...
}) around the code that needs auth disabled, or if skip(...) is unavailable,
surround the relevant block with try { $database->getAuthorization()->disable();
/* work */ } finally { $database->getAuthorization()->enable(); } to guarantee
re-enabling; target the calls on getAuthorization(), disable(), and
enable()/skip() in DocumentTests.php so the authorization state is always
restored even on exceptions.
- Around line 891-902: Add a second non-increment attribute and include the
$setOnInsert regression path: after the existing createAttribute call for
'value', call createAttribute for a new field (e.g., 'meta' or 'tag') as a
non-incrementable field; when building the initial upsert document set that new
field to an initial value, then perform subsequent upserts that change that new
field value while still incrementing 'value'; finally assert that the stored
non-increment field (the new attribute) did not change across upserts while
'value' increased. Use the same Document objects/array and the existing
upsert/increment flow in DocumentTests.php so assertions verify the
non-increment insert-only behavior on conflict.
---
Outside diff comments:
In `@src/Database/Adapter/Mongo.php`:
- Around line 1657-1677: The increment-upsert branch is still attaching $unset
(via $unsetFields) which allows removal of unrelated fields; update the logic in
the increment-upsert path (the block that builds $update using $inc, $set =>
['_updatedAt'], and '$setOnInsert' => $record, referencing $attribute,
$attributeValue, $updatedAt and $setOnInsert) to not include $unset at all for
increment upserts—i.e., remove or guard the code that sets $update['$unset'] =
$unsetFields so $unsetFields is ignored in this path and only $inc and
_updatedAt are touched on conflict.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: df14899c-dc64-47e9-9c3c-17d3e1571391
📒 Files selected for processing (3)
src/Database/Adapter/Mongo.phpsrc/Database/Database.phptests/e2e/Adapter/Scopes/DocumentTests.php
Uh oh!
There was an error while loading. Please reload this page.
Comment on lines
+7086
to
7092
| // 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(); |
Contributor
There was a problem hiding this comment.
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.
ContributorAuthor
There was a problem hiding this comment.
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.
Contributor
There was a problem hiding this comment.
@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.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
tests/e2e/Adapter/Scopes/DocumentTests.php (1)
934-981:⚠️ Potential issue | 🟡 MinorMutate
metricon later upserts to actually cover the$setOnInsertregression.Right now
metricis only asserted, never changed after the insert. Because the later upsert payload keeps the samemetricvalue, an implementation that incorrectly updates non-increment fields on conflict would still pass this test.🧪 Suggested test tightening
// Step 2: Increment existing documents (update-via-upsert) $documents[0]->setAttribute('value', 5); $documents[1]->setAttribute('value', 3); + $documents[0]->setAttribute('metric', 'network.requests.changed');+ $documents[1]->setAttribute('metric', 'documents.count.changed'); $count = $database->upsertDocumentsWithIncrease( collection: __FUNCTION__, attribute: 'value', documents: $documents @@ // Step 4: Third increment to confirm repeated upserts work $documents[0]->setAttribute('value', 1); $documents[1]->setAttribute('value', 1); + $documents[0]->setAttribute('metric', 'network.requests.changed-again');+ $documents[1]->setAttribute('metric', 'documents.count.changed-again'); $database->upsertDocumentsWithIncrease( collection: __FUNCTION__, attribute: 'value', documents: $documents🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/e2e/Adapter/Scopes/DocumentTests.php` around lines 934 - 981, The test never changes the non-increment attribute 'metric' in subsequent upserts, so implementations that wrongly update non-increment fields on conflict still pass; modify the later upsert payloads passed to upsertDocumentsWithIncrease (the documents in the $documents array used in Steps 2 and 4) to set a different 'metric' value (e.g., change $documents[0]/[1]->setAttribute('metric', 'new.value')) before calling upsertDocumentsWithIncrease and keep the existing assertions that 'metric' remains the original values for stat_a and stat_b to ensure $setOnInsert behavior is enforced.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@tests/e2e/Adapter/Scopes/DocumentTests.php`:
- Around line 934-981: The test never changes the non-increment attribute
'metric' in subsequent upserts, so implementations that wrongly update
non-increment fields on conflict still pass; modify the later upsert payloads
passed to upsertDocumentsWithIncrease (the documents in the $documents array
used in Steps 2 and 4) to set a different 'metric' value (e.g., change
$documents[0]/[1]->setAttribute('metric', 'new.value')) before calling
upsertDocumentsWithIncrease and keep the existing assertions that 'metric'
remains the original values for stat_a and stat_b to ensure $setOnInsert
behavior is enforced.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 1d609933-713a-40fa-bd00-d4ca0cc70a00
📒 Files selected for processing (1)
tests/e2e/Adapter/Scopes/DocumentTests.php
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (2)
src/Database/Database.php (2)
7086-7092:⚠️ Potential issue | 🟠 MajorPreserve explicit
$permissionswhen create-via-upsert skips the pre-read.In the skip-pre-read path,
$skipPermissionsUpdatestaystrue, and the later backfill at Line 7191 pulls permissions from the empty$old. That means a document created through increment-upsert will insert[]even when the caller provided explicit$permissions.💡 Minimal fix
- if ($skipPermissionsUpdate) {+ if ($skipPermissionsUpdate && !($skipPreRead && $document->offsetExists('$permissions'))) { $document->setAttribute('$permissions', $old->getPermissions()); }🤖 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, When skipping pre-read ($skipPreRead true) the current logic leaves $skipPermissionsUpdate true and later backfill reads empty permissions from $old; change this by detecting when $document->offsetExists('$permissions') in the skip-pre-read path and capture those permissions into $originalPermissions (from $document) and unset $skipPermissionsUpdate (or otherwise ensure the backfill uses the captured permissions). Update the block around $skipPermissionsUpdate/$skipPreRead/$document/$old so explicit $permissions provided by the caller are preserved for the later backfill logic (the section that currently reads permissions from $old).
7057-7061:⚠️ Potential issue | 🟠 MajorKeep “pre-read skipped” separate from “document does not exist”.
Using
new Document()here still makes every conflicting increment-upsert look like a create to the rest of this method. That breaks thecreated/updatedbreakdown at Lines 7268-7274, passesnullas the old document toonNextat Line 7315, and sends the update through the insert-styleStructurepath at Lines 7210-7217. This needs a separate sentinel/flag instead of reusing an empty old document.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Database/Database.php` around lines 7057 - 7061, The loop currently sets $old = new Document() when pre-read is skipped ($skipPreRead), which incorrectly makes skipped pre-reads indistinguishable from a real "document does not exist" and breaks created/updated logic and downstream paths (e.g., the onNext handling and insert-style Structure path). Instead introduce a distinct sentinel flag (e.g., $preReadSkipped) and keep $old as null when pre-read is skipped; update the downstream logic that branches on $old (the created/updated breakdown, the onNext invocation, and the Structure insert/update path) to explicitly check $preReadSkipped versus a truly missing $old so skipped pre-reads are handled differently from non-existent documents.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/Database/Database.php`:
- Around line 7086-7092: When skipping pre-read ($skipPreRead true) the current
logic leaves $skipPermissionsUpdate true and later backfill reads empty
permissions from $old; change this by detecting when
$document->offsetExists('$permissions') in the skip-pre-read path and capture
those permissions into $originalPermissions (from $document) and unset
$skipPermissionsUpdate (or otherwise ensure the backfill uses the captured
permissions). Update the block around
$skipPermissionsUpdate/$skipPreRead/$document/$old so explicit $permissions
provided by the caller are preserved for the later backfill logic (the section
that currently reads permissions from $old).
- Around line 7057-7061: The loop currently sets $old = new Document() when
pre-read is skipped ($skipPreRead), which incorrectly makes skipped pre-reads
indistinguishable from a real "document does not exist" and breaks
created/updated logic and downstream paths (e.g., the onNext handling and
insert-style Structure path). Instead introduce a distinct sentinel flag (e.g.,
$preReadSkipped) and keep $old as null when pre-read is skipped; update the
downstream logic that branches on $old (the created/updated breakdown, the
onNext invocation, and the Structure insert/update path) to explicitly check
$preReadSkipped versus a truly missing $old so skipped pre-reads are handled
differently from non-existent documents.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: f242e0ee-5430-4173-8618-fefa3acaab44
📒 Files selected for processing (1)
src/Database/Database.php
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Skip the per-document
getDocument()SELECT inupsertDocumentsWithIncrease()when$attributeis set and authorization is disabled (stats increment upserts).$setOnInsertfor non-increment fields, matching SQLON DUPLICATE KEY UPDATEbehaviorContext
Appwrite's stats worker calls
upsertDocumentsWithIncrease('stats', 'value', $documents)on a timer to record usage metrics. Before this change, every document in the batch triggered agetDocument()SELECT to fetch the old row — even though the result is never meaningfully used in the increment path.The optimization only activates when authorization is disabled, preserving correct behavior for any caller with auth enabled.
Impact
~3.3 million eliminated SELECTs/day across production (conservative 10% estimate). Micro-benchmark: 2.5x faster at the library level.
Test plan
testUpsertDocumentsIncSkipPreRead— create-via-upsert, increment, repeated increments,$createdAtpreservation, non-increment field preservationtestUpsertDocumentsIncandtestUpsertDocumentsPermissionsstill passSummary by CodeRabbit
Performance Improvements
Bug Fixes
Tests