feat: replace skipDuplicates with OnDuplicate enum (Fail/Skip/Upsert)#861
Open
premtsd-code wants to merge 10 commits intomainfrom
Open
feat: replace skipDuplicates with OnDuplicate enum (Fail/Skip/Upsert)#861premtsd-code wants to merge 10 commits intomainfrom
premtsd-code wants to merge 10 commits intomainfrom
Conversation
- Add Utopia\Database\OnDuplicate enum with Fail/Skip/Upsert cases - Replace bool $skipDuplicates flag with OnDuplicate $onDuplicate on Database and Adapter, scoped via new withOnDuplicate(mode, callable) - Remove skipDuplicates(callable) — callers use withOnDuplicate(Skip) - Adapters (MariaDB via SQL, SQLite, Postgres, Mongo) dispatch on enum via match; INSERT IGNORE / OR IGNORE / ON CONFLICT DO NOTHING for Skip; REPLACE / OR REPLACE for Upsert where natively supported - Pool + Mirror updated to propagate the enum state - Tests updated to call withOnDuplicate(OnDuplicate::Skip, $cb) Prepares the library for Upsert row semantics and schema-level skip/upsert handling in a follow-up commit.
…ATE / bulkWrite $set - SQL.getInsertSuffix now accepts column list so Upsert can generate SET clause - MariaDB override: 'ON DUPLICATE KEY UPDATE col = VALUES(col), ...' (skipping _uid/_id keys) - Postgres override: extends ON CONFLICT branch to DO UPDATE SET using EXCLUDED.col (skipping _uid/_id/_tenant conflict keys) - SQLite: already correct via INSERT OR REPLACE keyword (no suffix change) - Mongo bulk createDocuments: Upsert uses $set operator instead of $setOnInsert, making matched documents reflect incoming values; Skip keeps $setOnInsert semantics (insert-only no-op) Single-row createDocument upsert path + schema-level (createCollection / createAttribute / createIndex) dispatch remain for the next commit.
…Skip/Upsert
- createCollection:
* MariaDB/Postgres/SQLite: emit CREATE TABLE IF NOT EXISTS (and matching
CREATE INDEX IF NOT EXISTS for Postgres internal indexes) when onDuplicate
is Skip or Upsert; keep plain CREATE TABLE for Fail
* Mongo: extend the existing shared-tables/metadata 'skip if exists' branch
to also apply when onDuplicate is Skip or Upsert
- createAttribute (SQL base): catch DuplicateException, return true in
Skip/Upsert. Type/size changes go through updateAttribute — auto-applying
them during re-migration is unsafe.
- createIndex:
* MariaDB: catch DuplicateException in Skip/Upsert (MySQL doesn't support
IF NOT EXISTS on ALTER TABLE ADD INDEX)
* Postgres: emit CREATE INDEX IF NOT EXISTS natively
* SQLite: already has sqlite_master pre-check
- Upsert on schema resources behaves the same as Skip (don't auto-modify
existing schema). Upsert semantics apply to rows only.
…t tests - SQL base: add getInsertPermissionsKeyword() — keeps INSERT IGNORE on Skip/Upsert for the `_perms` side-table so row-Upsert doesn't try to DO-UPDATE-SET permissions (they already match on the composite unique key) - SQLite override: return INSERT OR IGNORE INTO for Skip/Upsert, and override getInsertSuffix to return empty — SQLite realizes Upsert via the `INSERT OR REPLACE` keyword and must not inherit MariaDB's `ON DUPLICATE KEY UPDATE` suffix (syntax error on SQLite) - Postgres override: return plain INSERT INTO for permissions; conflict handling goes through the ON CONFLICT suffix, not INSERT IGNORE (syntax error on Postgres) - tests/DocumentTests: add testCreateDocsUpsertOverwrites and testCreateDocsUpsertAll covering the happy paths; refresh three stale `skipDuplicates` comments to reference OnDuplicate::Skip - Mirror / MirrorTest: refresh stale `skipDuplicates` comment wording
Contributor
📝 WalkthroughWalkthroughReplaces the boolean skip-duplicates scope with a typed OnDuplicate enum (Fail, Skip, Upsert) and a nestable scoped API Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
Greptile SummaryThis PR replaces the boolean Several issues from prior review rounds remain open and should be resolved before merge:
Confidence Score: 4/5Not safe to merge until the MongoDB Three P1 issues from prior threads remain unresolved: MongoDB upsert will throw on any existing document,
Important Files Changed
Reviews (7): Last reviewed commit: "Move schema Skip/Upsert to Database laye..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| * OnDuplicate::Upsert — existing rows are overwritten with the incoming | ||
| * values; new rows are inserted. The returned count reflects every input. | ||
| */ | ||
| public function testCreateDocsUpsertOverwrites(): void |
There was a problem hiding this comment.
getSupportForUpserts() guard
testCreateDocsUpsertOverwrites and testCreateDocsUpsertAll run against all adapters, including SQLite which declares getSupportForUpserts() = false. Every other upsert-related test in this file (e.g. testUpsertDocumentsNoop, testUpsertDuplicateIds, testUpsertMixedPermissionDelta) begins with an early return when getSupportForUpserts() is false.
SQLite uses INSERT OR REPLACE INTO (full row delete+reinsert) which differs from ON DUPLICATE KEY UPDATE / ON CONFLICT DO UPDATE. If the intent is that OnDuplicate::Upsert is a supported semantic on SQLite, getSupportForUpserts() should be changed to return true there. If it isn't, the tests should guard accordingly, matching the rest of the test suite's convention.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 10
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/SQL.php (1)
2633-2652:⚠️ Potential issue | 🔴 CriticalKeep permission side-table writes consistent with duplicate handling.
After a duplicate-tolerant main insert/upsert, the permissions insert still runs for every input document. Under
Skip, this can add incoming permissions to an existing skipped document. UnderUpsert, the main_permissionscolumn can be updated while_permsonly inserts additions and never removes stale permissions, so authorization can diverge from the document.Gate permission writes to newly inserted rows when preserving existing permissions, or fully reconcile
_permstransactionally when Upsert is meant to replace permissions.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Database/Adapter/SQL.php` around lines 2633 - 2652, The permissions side-table insert currently runs unconditionally after the main insert/upsert (see the main statement built with getInsertKeyword(), getSQLTable(), getInsertSuffix() and subsequent execute($stmt)), causing stale or duplicate perms; change the logic so that for "Skip" duplicate-handling you only INSERT into the _perms table for rows that were actually inserted (capture inserted primary keys/rows via RETURNING or the driver-specific insert-result and filter $permissions to those new ids), and for "Upsert" either fully reconcile permissions transactionally by DELETing existing perms for the affected document ids before inserting the new set (all inside the same transaction and using getInsertPermissionsKeyword()/getInsertPermissionsSuffix()), or detect replaced rows and replace their perms rather than blindly inserting; ensure this uses the same transaction boundary as execute($stmt) and respects $this->sharedTables when building the permissions SQL.
🤖 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/Adapter/MariaDB.php`:
- Around line 2392-2419: The getInsertSuffix method currently only skips
'`_uid`' and '`_id`', causing `_createdAt` and `_tenant` to be overwritten on
upsert; update getInsertSuffix (used when $this->onDuplicate ===
OnDuplicate::Upsert) to also skip '`_createdAt`' and '`_tenant`'—aligning its
skip list with getUpsertStatement() and Postgres::getInsertSuffix()—so the
foreach over $columns does not add assignments for these four columns and thus
the returned 'ON DUPLICATE KEY UPDATE ...' string omits `_createdAt` and
`_tenant`.
In `@src/Database/Adapter/Mongo.php`:
- Around line 451-457: The DuplicateException branch in processException()
currently returns true unconditionally, which makes OnDuplicate::Fail behave
like idempotent success; change the logic in the DuplicateException handling
inside Mongo.php so it returns true only when $tolerateExisting is true or when
the target is explicitly a shared-tables/metadata case (preserve the "no-op"
behavior for shared-tables/metadata), and otherwise do not return early so the
subsequent OnDuplicate::Fail handling runs (i.e., let the code fall through to
the duplicate-path at the bottom instead of returning true).
- Around line 1527-1537: In OnDuplicate::Upsert where $operator is $set, remove
any '_id' from the $payload before adding the update and, if an '_id' existed in
the original $record, inject it into the $filter so the upsert supplies _id via
the query filter rather than via $set/$setOnInsert; update the code that builds
$operations (the ['filter'=> $filter, 'update'=> [$operator => $payload]] entry)
to ensure '_id' is only present in $filter and never in the $payload, preserving
the existing unset of '_uid' and '_tenant' and skipping when $payload becomes
empty.
In `@src/Database/Adapter/Postgres.php`:
- Around line 246-254: The create-block builds "IF NOT EXISTS" variants when
$this->onDuplicate !== OnDuplicate::Fail, but the exception handler later
unconditionally drops tables (the rollback that drops both tables on
PDOException), which can delete pre-existing tables; change the logic so you
either (A) track whether the CREATE TABLE/INDEX statements actually created
anything (e.g., set a boolean like $createdTable/$createdIndexes when the CREATE
without IF NOT EXISTS ran or when the CREATE reports creation) and only perform
the rollback DROP when those flags are true, or (B) skip the rollback entirely
when $this->onDuplicate !== OnDuplicate::Fail; update the code paths around
$createTable/$createIndex/$createUniqueIndex and the PDOException handler that
currently drops the tables so that drops happen only if the code created them
(or are skipped for non-Fail onDuplicate modes).
- Around line 2395-2399: The current logic in Postgres.php scopes
OnDuplicate::Skip to a specific conflict target via $conflictTarget, causing
only _uid/_tenant conflicts to be ignored; change the branch for
$this->onDuplicate === OnDuplicate::Skip to return a bare "ON CONFLICT DO
NOTHING" so all unique constraint violations (including user-defined unique
indexes and _id) are ignored, and keep the explicit conflict target behavior
only for the Upsert path that builds the targeted ON CONFLICT (... ) DO UPDATE
clause.
In `@src/Database/Adapter/SQL.php`:
- Around line 259-263: Postgres::createAttribute currently rethrows
duplicate-column errors after calling processException; mirror the
SQL::createAttribute behavior by detecting when processException($e) returns a
DuplicateException and, if $this->onDuplicate !== OnDuplicate::Fail, return true
to tolerate existing columns for withOnDuplicate(Skip/Upsert); keep
schema-evolution changes delegated to updateAttribute and do not auto-apply
type/size changes. Ensure you reference Postgres::createAttribute,
processException, DuplicateException, $this->onDuplicate and OnDuplicate::Fail
when making the change.
In `@src/Database/Adapter/SQLite.php`:
- Around line 1947-1969: Replace SQLite's current "INSERT OR REPLACE" upsert
approach with a proper "ON CONFLICT (...) DO UPDATE SET ..." pattern: change the
branch that currently returns 'INSERT OR REPLACE INTO' (the OnDuplicate::Upsert
case in the match) to use a plain insert keyword (e.g., 'INSERT INTO') and
implement getInsertSuffix(string $table, array $columns = []) to build an ON
CONFLICT clause that lists the table's primary/unique conflict columns and
produces "DO UPDATE SET col = excluded.col" assignments for each column except
_uid, _id, and _tenant; keep getInsertPermissionsKeyword behavior for Fail vs
others. Locate and update the match returning upsert SQL and the protected
function getInsertSuffix to generate the ON CONFLICT ... DO UPDATE fragment
while skipping the three special columns.
In `@src/Database/Database.php`:
- Around line 855-864: withOnDuplicate currently only sets $this->onDuplicate
but doesn’t propagate that mode to single-row/adapter-level create calls, so
createDocument(), createCollection(), createAttribute(), and createIndex() may
still perform Database-level duplicate checks or call the adapter and throw;
update withOnDuplicate to also run the callback inside the adapter scope
(propagate the duplicate mode into the adapter, e.g. adapter->withScope or
equivalent) and modify the schema/single-row precheck logic used by
createDocument/createCollection/createAttribute/createIndex to consult
$this->onDuplicate before raising an error so Skip/Upsert behaviors are honored
across all create-style operations (keep createDocuments() usage intact).
In `@tests/e2e/Adapter/MirrorTest.php`:
- Around line 16-17: The import list is out of PSR-12 alphabetical order: move
the Utopia\Database\OnDuplicate import so imports are alphabetized (e.g., ensure
Mirror comes before OnDuplicate, and OnDuplicate appears before PDO and before
Exception\Structure); update the use statements in MirrorTest.php to reorder the
use lines accordingly to satisfy the ordered_imports rule.
In `@tests/e2e/Adapter/Scopes/DocumentTests.php`:
- Line 7908: The calls to withOnDuplicate are missing the required space after
the comma between arguments (e.g., currently
"withOnDuplicate(OnDuplicate::Skip,function ..." ); update each withOnDuplicate
call to insert a space after the comma so they read
"withOnDuplicate(OnDuplicate::Skip, function ...)" — apply the same spacing fix
for all occurrences of withOnDuplicate that pass OnDuplicate::Skip and a closure
(references: withOnDuplicate, OnDuplicate::Skip and the closure signatures using
use ($database, $collection, &$emittedIds) and similar).
---
Outside diff comments:
In `@src/Database/Adapter/SQL.php`:
- Around line 2633-2652: The permissions side-table insert currently runs
unconditionally after the main insert/upsert (see the main statement built with
getInsertKeyword(), getSQLTable(), getInsertSuffix() and subsequent
execute($stmt)), causing stale or duplicate perms; change the logic so that for
"Skip" duplicate-handling you only INSERT into the _perms table for rows that
were actually inserted (capture inserted primary keys/rows via RETURNING or the
driver-specific insert-result and filter $permissions to those new ids), and for
"Upsert" either fully reconcile permissions transactionally by DELETing existing
perms for the affected document ids before inserting the new set (all inside the
same transaction and using
getInsertPermissionsKeyword()/getInsertPermissionsSuffix()), or detect replaced
rows and replace their perms rather than blindly inserting; ensure this uses the
same transaction boundary as execute($stmt) and respects $this->sharedTables
when building the permissions SQL.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 005c0ad6-c31c-4f49-b7c1-7358fbb48e2c
📒 Files selected for processing (12)
src/Database/Adapter.phpsrc/Database/Adapter/MariaDB.phpsrc/Database/Adapter/Mongo.phpsrc/Database/Adapter/Pool.phpsrc/Database/Adapter/Postgres.phpsrc/Database/Adapter/SQL.phpsrc/Database/Adapter/SQLite.phpsrc/Database/Database.phpsrc/Database/Mirror.phpsrc/Database/OnDuplicate.phptests/e2e/Adapter/MirrorTest.phptests/e2e/Adapter/Scopes/DocumentTests.php
Comment on lines
+2392
to
+2419
| /** | ||
| * MariaDB/MySQL Upsert: append `ON DUPLICATE KEY UPDATE col = VALUES(col), ...` | ||
| * so the INSERT replaces the matched row's columns instead of throwing. | ||
| * | ||
| * @param array<string> $columns | ||
| */ | ||
| protected function getInsertSuffix(string $table, array $columns = []): string | ||
| { | ||
| if ($this->onDuplicate !== OnDuplicate::Upsert || empty($columns)) { | ||
| return ''; | ||
| } | ||
|
|
||
| $assignments = []; | ||
| foreach ($columns as $col) { | ||
| // Skip the primary unique key (_uid) — no need to "update" what matched. | ||
| // Safe to skip _id too since it's auto-increment in INSERT and untouched on update. | ||
| if (\in_array($col, ['`_uid`', '`_id`'], true)) { | ||
| continue; | ||
| } | ||
| $assignments[] = "{$col} = VALUES({$col})"; | ||
| } | ||
|
|
||
| if (empty($assignments)) { | ||
| return ''; | ||
| } | ||
|
|
||
| return 'ON DUPLICATE KEY UPDATE ' . \implode(', ', $assignments); | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find all callers and check column formatting
rg -nP --type=php -C3 '\bgetInsertSuffix\s*\('
# Look at how columns are assembled at call sites
ast-grep --pattern $'$_->getInsertSuffix($$$)'
# Verify there are existing assertions locking _createdAt immutability across upsert
rg -nP --type=php -C2 '_createdAt' tests | rg -i 'upsert|on.?duplicate'Repository: utopia-php/database
Length of output: 2686
🏁 Script executed:
#!/bin/bash
# 1. Get full Postgres::getInsertSuffix() implementation
echo "=== Postgres::getInsertSuffix() ==="
sed -n '2389,2420p' src/Database/Adapter/Postgres.php
# 2. Get getUpsertStatement() implementation at line 1296
echo -e "\n=== MariaDB::getUpsertStatement() ==="
sed -n '1296,1350p' src/Database/Adapter/MariaDB.php
# 3. Check how columnList is formatted when passed to getInsertSuffix
echo -e "\n=== columnList assembly in SQL.php ==="
sed -n '2620,2640p' src/Database/Adapter/SQL.php
# 4. Search for _createdAt exclusion patterns in upsert
echo -e "\n=== _createdAt patterns in MariaDB ==="
rg -n '_createdAt' src/Database/Adapter/MariaDB.php -C2 | head -50Repository: utopia-php/database
Length of output: 5517
🏁 Script executed:
#!/bin/bash
# Search for test files that verify _createdAt behavior in upserts
echo "=== Test files checking _createdAt immutability ==="
fd -e '.php' tests | xargs grep -l '_createdAt' | head -10
# Check for specific upsert+_createdAt tests
echo -e "\n=== Tests with upsert and _createdAt ==="
rg -l 'upsert' tests --type=php | xargs rg '_createdAt' -C3 | head -100Repository: utopia-php/database
Length of output: 156
🏁 Script executed:
#!/bin/bash
# Find all callers of getUpsertStatement
echo "=== Callers of getUpsertStatement() ==="
rg -n 'getUpsertStatement' src/Database --type=php -B2 -A2
# Check if getInsertSuffix was recently added
echo -e "\n=== Recent additions to getInsertSuffix ==="
git log --oneline -p -- src/Database/Adapter/MariaDB.php | grep -A20 'getInsertSuffix' | head -60Repository: utopia-php/database
Length of output: 3247
🏁 Script executed:
#!/bin/bash
# Verify when batch inserts with OnDuplicate::Upsert are actually used
echo "=== OnDuplicate::Upsert usage ==="
rg -n 'OnDuplicate::Upsert' src/Database --type=php -B3 -A3 | head -100
# Check if there are high-level upsert methods in Database class
echo -e "\n=== Database class upsert methods ==="
rg -n 'def.*upsert|function.*upsert' src/Database/Database.php --type=php -iRepository: utopia-php/database
Length of output: 2284
getInsertSuffix overwrites _createdAt and _tenant on Upsert — diverges from the existing upsert path.
The new batch insert path via getInsertSuffix() skips only `_uid` and `_id`, so the generated SQL will include:
ON DUPLICATE KEY UPDATE `_createdAt` = VALUES(`_createdAt`), `_tenant` = VALUES(`_tenant`), ...Issues:
_createdAtregression. The existinggetUpsertStatement()(line 1296) intentionally excludes_createdAtso the original creation timestamp is preserved on collision. The new batch path silently overwrites it with the incoming value — user-visible data loss for anyone relying on_createdAtbeing immutable across upserts._tenantinconsistency.Postgres::getInsertSuffix()skips_uid,_id, and_tenant, but this MariaDB variant includes_tenantin updates. In shared-tables mode the unique key is(_uid, _tenant), so this assignment is a no-op on conflict but misaligns with the other adapter.
Align the skip list with getUpsertStatement() to exclude both _createdAt and _tenant:
🛡️ Proposed fix
protected function getInsertSuffix(string $table, array $columns = []): string
{
if ($this->onDuplicate !== OnDuplicate::Upsert || empty($columns)) {
return '';
}
$assignments = [];
foreach ($columns as $col) {
- // Skip the primary unique key (_uid) — no need to "update" what matched.
- // Safe to skip _id too since it's auto-increment in INSERT and untouched on update.
- if (\in_array($col, ['`_uid`', '`_id`'], true)) {
+ // Skip the conflict key (_uid), the surrogate PK (_id), the tenant
+ // discriminator (_tenant), and _createdAt — the original creation
+ // timestamp must be preserved on an upsert collision.
+ if (\in_array($col, ['`_uid`', '`_id`', '`_tenant`', '`_createdAt`'], true)) {
continue;
}
$assignments[] = "{$col} = VALUES({$col})";
}
if (empty($assignments)) {
return '';
}
return 'ON DUPLICATE KEY UPDATE ' . \implode(', ', $assignments);
}🧰 Tools
🪛 PHPMD (2.15.0)
[warning] 2398-2398: Avoid unused parameters such as '$table'. (undefined)
(UnusedFormalParameter)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/Database/Adapter/MariaDB.php` around lines 2392 - 2419, The
getInsertSuffix method currently only skips '`_uid`' and '`_id`', causing
`_createdAt` and `_tenant` to be overwritten on upsert; update getInsertSuffix
(used when $this->onDuplicate === OnDuplicate::Upsert) to also skip
'`_createdAt`' and '`_tenant`'—aligning its skip list with getUpsertStatement()
and Postgres::getInsertSuffix()—so the foreach over $columns does not add
assignments for these four columns and thus the returned 'ON DUPLICATE KEY
UPDATE ...' string omits `_createdAt` and `_tenant`.
Comment on lines
451
to
457
| if ($e instanceof DuplicateException) { | ||
| // Also tolerate client-reported duplicates in Skip/Upsert mode. | ||
| if ($tolerateExisting) { | ||
| return true; | ||
| } | ||
| // Keep existing shared-tables/metadata behavior — no-op there. | ||
| return true; |
Contributor
There was a problem hiding this comment.
Preserve Fail semantics for collection duplicates.
After processException(), a code-48 collection collision becomes DuplicateException, but Line 457 still returns true even when $tolerateExisting is false. That makes default OnDuplicate::Fail silently idempotent and bypasses the duplicate path mentioned below at Lines 459-467.
🐛 Proposed fix
if ($e instanceof DuplicateException) {
// Also tolerate client-reported duplicates in Skip/Upsert mode.
if ($tolerateExisting) {
return true;
}
- // Keep existing shared-tables/metadata behavior — no-op there.
- return true;
+ throw $e;
}🤖 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 451 - 457, The
DuplicateException branch in processException() currently returns true
unconditionally, which makes OnDuplicate::Fail behave like idempotent success;
change the logic in the DuplicateException handling inside Mongo.php so it
returns true only when $tolerateExisting is true or when the target is
explicitly a shared-tables/metadata case (preserve the "no-op" behavior for
shared-tables/metadata), and otherwise do not return early so the subsequent
OnDuplicate::Fail handling runs (i.e., let the code fall through to the
duplicate-path at the bottom instead of returning true).
Comment on lines
+1527
to
+1537
| // Filter fields can't reappear in $setOnInsert/$set (mongo path-conflict error). | ||
| $payload = $record; | ||
| unset($payload['_uid'], $payload['_tenant']); | ||
|
|
||
| if (empty($setOnInsert)) { | ||
| if (empty($payload)) { | ||
| continue; | ||
| } | ||
|
|
||
| $operations[] = [ | ||
| 'filter' => $filter, | ||
| 'update' => ['$setOnInsert' => $setOnInsert], | ||
| 'update' => [$operator => $payload], |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
MongoDB update $set _id immutable field and $setOnInsert _id upsert
💡 Result:
In MongoDB, the _id field is immutable and cannot be modified once a document exists. Attempting to use $set on _id in an update operation (with or without upsert) will fail with an error like "Performing an update on the path '_id' would modify the immutable field '_id'". For upsert operations (upsert: true), the _id is determined by the query filter if it specifies _id. For example: db.collection.updateOne( { _id: ObjectId("...") }, { $set: { field: "value" } }, { upsert: true } ) If no document matches, MongoDB inserts a new document using the _id from the query filter, applying the update operators (like $set). $setOnInsert operators are also applied only on insert. Do not include _id in $set or $setOnInsert in the update document, as MongoDB will reject attempts to set _id via update operators even with $setOnInsert (historical behavior confirmed in sources, fixed in later versions but still not recommended). Instead, specify _id in the query filter for controlled upserts. Example of correct upsert without setting _id in update: db.collection.updateOne( { _id: 123 }, { $set: { status: "active" }, $setOnInsert: { createdAt: new Date } }, { upsert: true } ) This sets _id: 123 on insert from the query, applies both $set and $setOnInsert on insert, or only $set on update. If the query doesn't specify _id, MongoDB auto-generates ObjectId for _id on insert.
Citations:
- 1: https://www.mongodb.com/docs/v8.0/reference/operator/update/setoninsert/
- 2: https://stackoverflow.com/questions/16217306/mongodb-update-setoninsert-mod-on-id-not-allowed
- 3: https://www.mongodb.com/docs/manual/reference/operator/update/setoninsert/
- 4: https://www.mongodb.com/docs/manual/core/document/%23the-_id-field
- 5: https://www.mongodb.com/docs/manual/tutorial/update-documents/
🏁 Script executed:
# Find and examine the upsert operation context in Mongo.php around lines 1527-1537
cd src/Database/Adapter && head -1560 Mongo.php | tail -50Repository: utopia-php/database
Length of output: 1846
🏁 Script executed:
# Get broader context of the upsert method to understand filter and payload construction
cd src/Database/Adapter && sed -n '1480,1560p' Mongo.phpRepository: utopia-php/database
Length of output: 2845
Strip _id from Mongo $set upserts and specify it in the filter instead.
In OnDuplicate::Upsert, $operator is $set, which will fail because MongoDB rejects any attempt to set _id via the $set operator. MongoDB's _id field is immutable—it cannot be modified after document creation, and it also cannot be set via $setOnInsert. Instead, _id must be specified in the query filter. For new inserts, MongoDB uses the _id from the filter; for existing documents, only $set fields outside of _id are applied.
🔧 Suggested approach
Include _id in the filter when constructing the upsert operation, and exclude it from the $set payload. For example:
$filter = ['_uid' => $record['_uid'] ?? ''];
+ if (\array_key_exists('_id', $record)) {
+ $filter['_id'] = $record['_id'];
+ }
if ($this->sharedTables) {
$filter['_tenant'] = $record['_tenant'] ?? $this->getTenant();
}
// Filter fields can't reappear in $setOnInsert/$set (mongo path-conflict error).
$payload = $record;
- unset($payload['_uid'], $payload['_tenant']);
+ unset($payload['_uid'], $payload['_tenant'], $payload['_id']);This way, _id is supplied via the filter (where MongoDB expects it for upserts), not via the update operators.
🤖 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 1527 - 1537, In
OnDuplicate::Upsert where $operator is $set, remove any '_id' from the $payload
before adding the update and, if an '_id' existed in the original $record,
inject it into the $filter so the upsert supplies _id via the query filter
rather than via $set/$setOnInsert; update the code that builds $operations (the
['filter'=> $filter, 'update'=> [$operator => $payload]] entry) to ensure '_id'
is only present in $filter and never in the $payload, preserving the existing
unset of '_uid' and '_tenant' and skipping when $payload becomes empty.
Comment on lines
+1947
to
+1969
| return match ($this->onDuplicate) { | ||
| OnDuplicate::Skip => 'INSERT OR IGNORE INTO', | ||
| OnDuplicate::Upsert => 'INSERT OR REPLACE INTO', | ||
| OnDuplicate::Fail => 'INSERT INTO', | ||
| }; | ||
| } | ||
|
|
||
| protected function getInsertPermissionsKeyword(): string | ||
| { | ||
| return $this->onDuplicate === OnDuplicate::Fail | ||
| ? 'INSERT INTO' | ||
| : 'INSERT OR IGNORE INTO'; | ||
| } | ||
|
|
||
| /** | ||
| * SQLite realizes Upsert via the `INSERT OR REPLACE` keyword, so no SUFFIX | ||
| * clause is needed. Override MariaDB's `ON DUPLICATE KEY UPDATE` suffix. | ||
| * | ||
| * @param array<string> $columns | ||
| */ | ||
| protected function getInsertSuffix(string $table, array $columns = []): string | ||
| { | ||
| return ''; |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
SQLite INSERT OR REPLACE delete insert semantics and ON CONFLICT DO UPDATE support
💡 Result:
SQLite INSERT OR REPLACE implements the REPLACE conflict resolution algorithm: on UNIQUE/PRIMARY KEY constraint violation, it deletes the existing row(s) causing the conflict, then inserts the new row. This is a "delete then insert" semantic. REPLACE (alias for INSERT OR REPLACE) triggers DELETE triggers (if recursive triggers enabled) but not UPDATE hooks, and does not increment the changes counter for the delete. Unspecified columns in the INSERT are set to NULL (or default if allowed), potentially violating NOT NULL constraints and rolling back. ON CONFLICT DO UPDATE (UPSERT, added in SQLite 3.24.0 2018) provides finer control: on specified uniqueness conflict, skip insert and perform targeted UPDATE using original column values and 'excluded.' table alias for proposed insert values. No delete occurs; it's pure UPDATE on conflict. DO UPDATE uses ABORT resolution internally—if UPDATE violates constraints, entire statement rolls back. Multiple ON CONFLICT clauses possible, checked in order. Key differences: OR REPLACE always deletes/replaces entire row (risking NULLs in unspecified columns, affecting triggers/FK cascades); ON CONFLICT DO UPDATE updates specific columns without delete, preserving unspecified columns, no delete triggers/FK effects from deletion. Examples: INSERT OR REPLACE INTO t(id, val) VALUES(1, 'new'); -- Deletes row id=1, inserts new (val=new, other cols NULL/default) INSERT INTO t(id, val) VALUES(1, 'new') ON CONFLICT(id) DO UPDATE SET val=excluded.val; -- Updates val to 'new', keeps other columns unchanged
Citations:
- 1: https://www2.sqlite.org/lang_insert.html
- 2: https://www.sqlitetutorial.net/sqlite-replace-statement/
- 3: https://www.sqlite.org/lang_insert.html
- 4: https://www2.sqlite.org/lang_conflict.html
- 5: https://sqlite.org/lang_conflict.html
- 6: https://sqlite.org/lang_upsert.html
🏁 Script executed:
# Find and check minimum SQLite version requirement
fd -t f composer.json package.json | head -5Repository: utopia-php/database
Length of output: 162
🏁 Script executed:
# Check MariaDB and PostgreSQL adapter implementations
fd -t f "Adapter" src/ | grep -E "(MariaDB|Postgres)" | head -10Repository: utopia-php/database
Length of output: 45
🏁 Script executed:
# Search for getInsertSuffix usage in SQL.php
rg "getInsertSuffix" --type php -B 3 -A 3Repository: utopia-php/database
Length of output: 3422
🏁 Script executed:
# Look at the full context around line 2633 in SQL.php
fd -t f "SQL.php" src/ | head -1Repository: utopia-php/database
Length of output: 93
🏁 Script executed:
# Get full MariaDB getInsertSuffix implementation
sed -n '1800,1850p' src/Database/Adapter/MariaDB.phpRepository: utopia-php/database
Length of output: 1082
🏁 Script executed:
# Get full PostgreSQL getInsertSuffix implementation
rg "protected function getInsertSuffix" -A 50 src/Database/Adapter/Postgres.phpRepository: utopia-php/database
Length of output: 1776
🏁 Script executed:
# Check for SQLite version requirement in composer.json
cat composer.json | grep -A 5 -B 5 sqliteRepository: utopia-php/database
Length of output: 45
🏁 Script executed:
# Search for _id, _uid handling and type info
rg "(\`_id\`|\`_uid\`|\`_tenant\`)" src/ -t php | head -20Repository: utopia-php/database
Length of output: 807
🏁 Script executed:
# Get full MariaDB getInsertSuffix implementation
rg "protected function getInsertSuffix" -A 50 src/Database/Adapter/MariaDB.phpRepository: utopia-php/database
Length of output: 829
🏁 Script executed:
# Search for SQLite minimum version or compatibility notes
find . -name "README*" -o -name "*.md" | head -10 | xargs grep -l -i sqlite 2>/dev/nullRepository: utopia-php/database
Length of output: 74
🏁 Script executed:
# Check composer.json for any version constraints
cat composer.jsonRepository: utopia-php/database
Length of output: 2302
🏁 Script executed:
# Look for any tests or docs mentioning SQLite version
rg "sqlite.*version|3\.(24|25|3[0-9])" -i --type php --type md | head -20Repository: utopia-php/database
Length of output: 404
🏁 Script executed:
# Check if there's any duplicate exception handling that might differ between adapters
rg "DuplicateException|UniqueException" src/Database/Adapter/ -B 2 -A 2 | head -50Repository: utopia-php/database
Length of output: 3531
Replace SQLite's INSERT OR REPLACE upsert with ON CONFLICT DO UPDATE.
SQLite's REPLACE conflict algorithm deletes the conflicting row and inserts a new one, which can change _id values and fire delete/insert side effects instead of updating in place. Since SQLite 3.38 (the project minimum) supports ON CONFLICT DO UPDATE (available since 3.24.0), use the same pattern as PostgreSQL: skip _uid, _id, and _tenant in the update assignments.
Proposed fix
protected function getInsertKeyword(): string
{
return match ($this->onDuplicate) {
OnDuplicate::Skip => 'INSERT OR IGNORE INTO',
- OnDuplicate::Upsert => 'INSERT OR REPLACE INTO',
+ OnDuplicate::Upsert => 'INSERT INTO',
OnDuplicate::Fail => 'INSERT INTO',
};
}
@@
protected function getInsertSuffix(string $table, array $columns = []): string
{
- return '';
+ if ($this->onDuplicate !== OnDuplicate::Upsert || empty($columns)) {
+ return '';
+ }
+
+ $assignments = [];
+ foreach ($columns as $col) {
+ if (\in_array($col, ['`_uid`', '`_id`', '`_tenant`'], true)) {
+ continue;
+ }
+ $assignments[] = "{$col} = excluded.{$col}";
+ }
+
+ if (empty($assignments)) {
+ return '';
+ }
+
+ $conflictTarget = $this->sharedTables ? '(`_uid`, `_tenant`)' : '(`_uid`)';
+
+ return "ON CONFLICT {$conflictTarget} DO UPDATE SET " . \implode(', ', $assignments);
}🧰 Tools
🪛 PHPMD (2.15.0)
[warning] 1967-1967: Avoid unused parameters such as '$table'. (undefined)
(UnusedFormalParameter)
[warning] 1967-1967: Avoid unused parameters such as '$columns'. (undefined)
(UnusedFormalParameter)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/Database/Adapter/SQLite.php` around lines 1947 - 1969, Replace SQLite's
current "INSERT OR REPLACE" upsert approach with a proper "ON CONFLICT (...) DO
UPDATE SET ..." pattern: change the branch that currently returns 'INSERT OR
REPLACE INTO' (the OnDuplicate::Upsert case in the match) to use a plain insert
keyword (e.g., 'INSERT INTO') and implement getInsertSuffix(string $table, array
$columns = []) to build an ON CONFLICT clause that lists the table's
primary/unique conflict columns and produces "DO UPDATE SET col = excluded.col"
assignments for each column except _uid, _id, and _tenant; keep
getInsertPermissionsKeyword behavior for Fail vs others. Locate and update the
match returning upsert SQL and the protected function getInsertSuffix to
generate the ON CONFLICT ... DO UPDATE fragment while skipping the three special
columns.
Comment on lines
16
to
17
| use Utopia\Database\OnDuplicate; | ||
| use Utopia\Database\Exception\Structure; |
Contributor
There was a problem hiding this comment.
Fix import order to satisfy PSR-12 ordered_imports (lint failure).
Utopia\Database\OnDuplicate is inserted between Exception\Limit and Exception\Structure, but alphabetically it belongs after Mirror (and before PDO). This is what Pint is flagging.
🧹 Proposed fix
use Utopia\Database\Exception\Limit;
-use Utopia\Database\OnDuplicate;
use Utopia\Database\Exception\Structure;
use Utopia\Database\Helpers\Permission;
use Utopia\Database\Helpers\Role;
use Utopia\Database\Mirror;
+use Utopia\Database\OnDuplicate;
use Utopia\Database\PDO;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/e2e/Adapter/MirrorTest.php` around lines 16 - 17, The import list is
out of PSR-12 alphabetical order: move the Utopia\Database\OnDuplicate import so
imports are alphabetized (e.g., ensure Mirror comes before OnDuplicate, and
OnDuplicate appears before PDO and before Exception\Structure); update the use
statements in MirrorTest.php to reorder the use lines accordingly to satisfy the
ordered_imports rule.
…entTests) Pint violations flagged by CI: - MirrorTest: OnDuplicate import alphabetized after Mirror - DocumentTests: add missing space after ',' in withOnDuplicate(...) calls
Comment on lines
+1527
to
+1529
| // Filter fields can't reappear in $setOnInsert/$set (mongo path-conflict error). | ||
| $payload = $record; | ||
| unset($payload['_uid'], $payload['_tenant']); |
There was a problem hiding this comment.
_id not stripped from $set payload on Upsert
Every call to $set in this adapter (updateDocument, upsertDocument) explicitly does unset($record['_id']) before building the update — MongoDB treats _id as an immutable field and rejects $set: {_id: …} on an existing document. The new bulk-write Upsert path only unsets _uid and _tenant, leaving _id (set from $record['_id'] = $sequence or from the $ → _ substitution of $id) in the $set payload. For any existing document this will throw "Mod on _id not allowed" at the driver level.
The _id must be removed from the $set payload and moved into $setOnInsert so that new documents get their ID on insert while existing documents are not touched:
$payload = $record;
unset($payload['_uid'], $payload['_tenant']);
$idValue = $payload['_id'] ?? null;
unset($payload['_id']);
$update = [$operator => $payload];
if ($operator === '$set' && $idValue !== null) {
$update['$setOnInsert'] = ['_id' => $idValue];
}
$operations[] = [
'filter' => $filter,
'update' => $update,
];… for indexes Upsert on createAttribute now introspects the existing column and only drops+recreates when the declared type/size differs. Matching columns are a cheap no-op — migration data stays intact. Skip keeps pure tolerate. - SQL base: new attributeMatches() helper (INFORMATION_SCHEMA.COLUMN_TYPE + normalizeColumnType strips MySQL legacy display widths) wired into createAttribute's Upsert branch - Postgres: override attributeMatches() to use pg_catalog.format_type() + Postgres-specific normalization (`character varying` → VARCHAR, strip `without time zone`). Fix createAttribute to tolerate/drop+recreate on duplicate per mode (was previously rethrowing) - SQLite: override attributeMatches() to query PRAGMA table_info - Mongo: createAttribute is already a no-op (schemaless), no change createIndex now drops+recreates on Upsert (no cheap spec comparison; a rebuild is the cost of getting the new definition in place): - MariaDB/Postgres: catch DuplicateException in Upsert → deleteIndex + retry - SQLite: pre-check sqlite_master → if hit + Upsert, deleteIndex + continue; closeCursor before DDL to avoid "database table is locked" - Mongo: catch IndexKeySpecsConflict (code 86) as DuplicateException so the drop+recreate branch fires (85 was already mapped) - Postgres createIndex: Skip uses IF NOT EXISTS, Upsert uses plain CREATE so the duplicate error bubbles up and triggers the drop+recreate branch Database::withOnDuplicate() now delegates to adapter->withOnDuplicate() so schema-level create* operations running directly against the adapter observe the mode. Previously only createDocuments propagated it. Tests (Scopes/DocumentTests): - testUpsertAttributeSameTypeNoop: matching type → data preserved - testUpsertAttrTypeChangedRecreates: widened VARCHAR → drop+recreate (asserted via subsequent same-type Upsert being a no-op + deleteAttribute) - testUpsertIndexRebuilds: index pointing at different columns is rebuilt - testSkipSchemaTolerates: Skip leaves existing column/index untouched even when the declared spec is wider
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/Database/Database.php (1)
5685-5688:⚠️ Potential issue | 🔴 CriticalRequire update authorization before row Upsert updates existing documents.
createDocuments()only checks collection create permission, butOnDuplicate::Upsertcan turn duplicate inserts into updates. With document security enabled, this can modify existing documents without the per-document update authorization enforced byupsertDocumentsWithIncrease()/updateDocument().Please either route Upsert mode through the existing upsert authorization flow or prefetch duplicates and validate update permissions before invoking adapter Upsert.
Also applies to: 5740-5745
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Database/Database.php` around lines 5685 - 5688, createDocuments() currently only checks create permission (Input(self::PERMISSION_CREATE, ...)) and does not enforce update authorization when OnDuplicate::Upsert would convert inserts into updates, allowing modification of existing documents; route Upsert flows through the existing upsert/update authorization path (e.g., call upsertDocumentsWithIncrease() or updateDocument()) or prefetch duplicate ids and call authorization->isValid(new Input(self::PERMISSION_UPDATE, $existingDoc)) for each duplicate before invoking the adapter Upsert; ensure you still skip metadata collection check (self::METADATA) and reuse $this->authorization->getDescription() in thrown AuthorizationException for consistency.tests/e2e/Adapter/Scopes/DocumentTests.php (1)
8216-8251:⚠️ Potential issue | 🟡 MinorMake the duplicate child assert detect overwrites.
The duplicate child uses the same
nameas the seeded child, so this test still passes ifOnDuplicate::Skipaccidentally overwrites the existing child row. Give the incoming duplicate a different value and assert the stored child kept the original value.Suggested test hardening
new Document([ '$id' => 'existingChild', - 'name' => 'ExistingChild', + 'name' => 'ShouldNotOverwriteChild', '$permissions' => $permissions, ]), @@ $this->assertSame(['existingChild', 'retryChild'], $childIds); + + $existingChild = $database->getDocument($child, 'existingChild'); + $this->assertSame('ExistingChild', $existingChild->getAttribute('name'));🤖 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 8216 - 8251, The test currently uses a duplicate child with the same name as the seeded child so an accidental overwrite would still pass; update the duplicate child Document (the one with '$id' => 'retryChild' created before calling $database->withOnDuplicate(OnDuplicate::Skip, fn () => $database->createDocuments($parent, $batch))) to use a different 'name' value, then after fetching $existing via $database->getDocument($parent, 'existingParent') assert that the stored child (found via $existing->getAttribute('children') and its Document->getId()) still has the original name (e.g., assert the child's getAttribute('name') equals 'ExistingChild'), ensuring OnDuplicate::Skip did not overwrite the existing row.
♻️ Duplicate comments (7)
src/Database/Adapter/Mongo.php (2)
451-457:⚠️ Potential issue | 🟠 MajorPreserve
Failsemantics for duplicate collections.Line 457 still returns
trueafter aDuplicateExceptioneven when$tolerateExistingis false, so defaultOnDuplicate::Failsilently succeeds instead of surfacing the duplicate.🐛 Proposed fix
if ($e instanceof DuplicateException) { // Also tolerate client-reported duplicates in Skip/Upsert mode. if ($tolerateExisting) { return true; } - // Keep existing shared-tables/metadata behavior — no-op there. - return true; + throw $e; }🤖 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 451 - 457, The current DuplicateException handling in Mongo.php returns true even when $tolerateExisting is false, which breaks OnDuplicate::Fail semantics; update the catch branch that checks ($e instanceof DuplicateException) so that it returns true only when $tolerateExisting is truthy, and otherwise rethrows the DuplicateException (or propagate the error) instead of returning true, referencing the DuplicateException type and the $tolerateExisting/OnDuplicate::Fail logic to locate the code to change.
1530-1545:⚠️ Potential issue | 🟠 MajorKeep immutable
_idand existing permissions out of Mongo$setupserts.In
OnDuplicate::Upsert,$payloadis used under$set, but it still contains_id, which Mongo rejects as immutable. It also updates_permissionson existing rows, while the SQL path now avoids updating existing permission rows in the side table, creating cross-adapter authorization drift.🐛 Proposed adjustment
$filter = ['_uid' => $record['_uid'] ?? '']; + if (\array_key_exists('_id', $record)) { + $filter['_id'] = $record['_id']; + } if ($this->sharedTables) { $filter['_tenant'] = $record['_tenant'] ?? $this->getTenant(); } // Filter fields can't reappear in $setOnInsert/$set (mongo path-conflict error). $payload = $record; - unset($payload['_uid'], $payload['_tenant']); + unset($payload['_uid'], $payload['_tenant'], $payload['_id']); - if (empty($payload)) { + $update = []; + if ($this->onDuplicate === OnDuplicate::Upsert) { + if (\array_key_exists('_permissions', $payload)) { + $update['$setOnInsert']['_permissions'] = $payload['_permissions']; + unset($payload['_permissions']); + } + if (!empty($payload)) { + $update['$set'] = $payload; + } + } elseif (!empty($payload)) { + $update['$setOnInsert'] = $payload; + } + + if (empty($update)) { continue; } $operations[] = [ 'filter' => $filter, - 'update' => [$operator => $payload], + 'update' => $update, ];Please verify the MongoDB
_idupdate-operator behavior against the driver/server versions in CI:MongoDB update operators $set immutable _id upsert filter _id behavior🤖 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 1530 - 1545, The upsert payload currently can include immutable _id and will overwrite existing _permissions; before building $operations in OnDuplicate::Upsert remove $_id from $payload (unset($payload['_id'])), extract and remove _permissions from $payload (e.g. $perm = $payload['_permissions'] ?? null; unset($payload['_permissions'])), then build the update so $payload goes under the intended $operator while any extracted _permissions are applied only on insert (merge into $update['$setOnInsert']['_permissions'] = $perm when $perm is not null) so existing rows are not modified; ensure you continue to use the same symbols ($payload, $operator, $operations, 'filter', 'update') so the change is localized and test CI against Mongo driver/server behavior.src/Database/Adapter/MariaDB.php (1)
2410-2417:⚠️ Potential issue | 🟠 MajorPreserve immutable/internal columns in the Upsert suffix.
This still updates
`_createdAt`and`_tenant`on duplicate rows, unlikegetUpsertStatement()which skips them. That can mutate creation timestamps and diverge shared-table behavior.Proposed fix
foreach ($columns as $col) { // Skip the primary unique key (_uid) — no need to "update" what matched. // Safe to skip _id too since it's auto-increment in INSERT and untouched on update. - if (\in_array($col, ['`_uid`', '`_id`'], true)) { + if (\in_array($col, ['`_uid`', '`_id`', '`_tenant`', '`_createdAt`'], true)) { continue; } $assignments[] = "{$col} = VALUES({$col})"; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Database/Adapter/MariaDB.php` around lines 2410 - 2417, The upsert assignment builder in MariaDB.php is currently skipping only '`_uid`' and '`_id`', causing immutable/internal columns like '`_createdAt`' and '`_tenant`' to be updated on duplicate key; in the method that contains the foreach ($columns as $col) loop (the Upsert suffix/assignment generator—compare behavior with getUpsertStatement()), extend the skip check to include '`_createdAt`' and '`_tenant`' (add them to the in_array list) so those columns are not added to $assignments and thus are preserved on duplicate updates.src/Database/Adapter/Postgres.php (2)
2463-2482:⚠️ Potential issue | 🟠 MajorKeep Skip broad and preserve
_createdAton Upsert.
Skipis scoped to_uidconflicts only, so user-defined unique indexes can still throw. Upsert also updates"_createdAt", diverging fromgetUpsertStatement()and mutating creation timestamps.Proposed fix
- $conflictTarget = $this->sharedTables ? '("_uid", "_tenant")' : '("_uid")'; - if ($this->onDuplicate === OnDuplicate::Skip) { - return "ON CONFLICT {$conflictTarget} DO NOTHING"; + return 'ON CONFLICT DO NOTHING'; } + $conflictTarget = $this->sharedTables ? '("_uid", "_tenant")' : '("_uid")'; + // Upsert: DO UPDATE SET col = EXCLUDED.col for every column except the conflict key. $assignments = []; foreach ($columns as $col) { - if (\in_array($col, ['"_uid"', '"_id"', '"_tenant"'], true)) { + if (\in_array($col, ['"_uid"', '"_id"', '"_tenant"', '"_createdAt"'], true)) { continue; } $assignments[] = "{$col} = EXCLUDED.{$col}"; }Please verify the bare
ON CONFLICT DO NOTHINGbehavior against the supported PostgreSQL version:PostgreSQL INSERT ON CONFLICT DO NOTHING without conflict target ignores all unique constraint conflicts🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Database/Adapter/Postgres.php` around lines 2463 - 2482, The current logic makes Skip too broad by using $conflictTarget based on $this->sharedTables and may emit a bare ON CONFLICT target that doesn’t match intended semantics; also the upsert branch wrongly updates the creation timestamp. Change the Skip branch (when $this->onDuplicate === OnDuplicate::Skip) to always use the _uid-only conflict target (i.e. '("_uid")') so Skip remains scoped to _uid conflicts regardless of $this->sharedTables/$conflictTarget, and modify the assignment-generation loop that builds $assignments (used in getUpsertStatement()/this method) to also exclude '"_createdAt"' from updates so creation timestamps are preserved; keep all other exclusions (\"_id\", \"_tenant\") as-is and ensure the returned ON CONFLICT strings reflect these fixes.
246-254:⚠️ Potential issue | 🔴 CriticalAvoid dropping pre-existing collections after idempotent create.
With
IF NOT EXISTS, this method may operate on tables that already existed; the catch block can still drop both tables after a later non-duplicate failure.Proposed fix
- if (!($e instanceof DuplicateException)) { + if ($this->onDuplicate === OnDuplicate::Fail && !($e instanceof DuplicateException)) { $this->execute($this->getPDO() ->prepare("DROP TABLE IF EXISTS {$this->getSQLTable($id)}, {$this->getSQLTable($id . '_perms')};")); }Also applies to: 358-364
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Database/Adapter/Postgres.php` around lines 246 - 254, The current creation logic uses "IF NOT EXISTS" but the catch block still unconditionally drops tables/indexes, which can remove pre-existing objects; update the flow around $createTable, $createIndex, and $createUniqueIndex (and the similar block at the 358-364 area) to only drop objects that this operation actually created: before running CREATE statements, record existence (e.g., query information_schema/pg_indexes or pg_tables) or set created flags based on whether the object existed; in the catch handler, check those flags or re-check existence and only DROP the tables/indexes that were created by this run (do not drop if they pre-existed), and keep the behavior tied to the existing onDuplicate / OnDuplicate::Fail logic.src/Database/Adapter/SQLite.php (1)
1956-1980:⚠️ Potential issue | 🟠 MajorUse SQLite UPSERT instead of
INSERT OR REPLACE.
INSERT OR REPLACEhas delete-then-insert semantics, so row Upsert can change_idand fire delete/insert side effects instead of updating in place.Proposed fix
protected function getInsertKeyword(): string { return match ($this->onDuplicate) { OnDuplicate::Skip => 'INSERT OR IGNORE INTO', - OnDuplicate::Upsert => 'INSERT OR REPLACE INTO', + OnDuplicate::Upsert => 'INSERT INTO', OnDuplicate::Fail => 'INSERT INTO', }; } @@ protected function getInsertSuffix(string $table, array $columns = []): string { - return ''; + if ($this->onDuplicate !== OnDuplicate::Upsert || empty($columns)) { + return ''; + } + + $assignments = []; + foreach ($columns as $col) { + if (\in_array($col, ['`_uid`', '`_id`', '`_tenant`', '`_createdAt`'], true)) { + continue; + } + $assignments[] = "{$col} = excluded.{$col}"; + } + + if (empty($assignments)) { + return ''; + } + + $conflictTarget = $this->sharedTables + ? '(`_tenant` COLLATE NOCASE, `_uid` COLLATE NOCASE)' + : '(`_uid` COLLATE NOCASE)'; + + return "ON CONFLICT {$conflictTarget} DO UPDATE SET " . \implode(', ', $assignments); }Please verify the conflict target matches SQLite’s unique
_uidindex definition, including column order/collation:SQLite UPSERT ON CONFLICT target matching unique index column order and COLLATE NOCASE requirements🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Database/Adapter/SQLite.php` around lines 1956 - 1980, The code currently uses INSERT OR REPLACE in getInsertKeyword (and returns empty getInsertSuffix), which performs delete-then-insert; change the Upsert branch in getInsertKeyword (and adjust getInsertPermissionsKeyword if needed) to use plain 'INSERT INTO' and implement getInsertSuffix(string $table, array $columns) to return a proper SQLite UPSERT clause using ON CONFLICT(col1, col2[, ...] ) DO UPDATE SET ... instead of REPLACE; ensure the ON CONFLICT target exactly matches the unique index for the table (name, column order and any COLLATE NOCASE on _uid) so the conflict target columns and order/collation in getInsertSuffix match the database unique `_uid` index definition and produce correct DO UPDATE assignments for the provided $columns.src/Database/Database.php (1)
855-866:⚠️ Potential issue | 🟠 MajorDatabase-level duplicate guards still block schema Skip/Upsert.
withOnDuplicate()now propagates the mode to the adapter, but schema paths still throw before the adapter can observe it: existing collections, metadata-backed attributes, and indexes all raiseDuplicateExceptionunconditionally. That makeswithOnDuplicate(OnDuplicate::Skip|Upsert, ...)non-idempotent for these create-style schema operations.Please make these prechecks consult
$this->onDuplicatebefore throwing; e.g. Skip should no-op/return the existing result, and Upsert should perform the intended schema reconciliation path.Also applies to: 1721-1725, 2198-2200, 4530-4533
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Database/Database.php` around lines 855 - 866, The precheck logic in Database::withOnDuplicate (and the similar precheck sites noted) throws DuplicateException for existing collections/attributes/indexes before the adapter sees the propagated mode; update the precheck branches in withOnDuplicate and the schema create paths (e.g., the checks in createCollection/createAttribute/createIndex or wherever DuplicateException is raised) to consult $this->onDuplicate: if $this->onDuplicate is OnDuplicate::Skip, return the existing result/no-op instead of throwing; if OnDuplicate::Upsert, proceed into the reconciliation path (do not throw) so the adapter-level upsert logic runs; ensure DuplicateException is still thrown only when $this->onDuplicate is the default/Fail.
🤖 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/Adapter/Mongo.php`:
- Around line 1121-1128: The Upsert rebuild path (check using DuplicateException
and OnDuplicate::Upsert in the block that calls deleteIndex and
$this->client->createIndexes) recreates the index but skips the unique-index
readiness wait used after the normal createIndexes flow; extract the existing
readiness polling loop (the logic that waits for unique index readiness) into a
private helper method (eg. waitForIndexReady or ensureUniqueIndexReady) and
invoke that helper both after the normal $this->client->createIndexes(...) call
and after the rebuild path that calls deleteIndex(...) followed by
$this->client->createIndexes(...), ensuring callers only proceed once the unique
constraint is ready.
In `@src/Database/Adapter/SQL.php`:
- Around line 260-271: attributeMatches(...) is falsely reporting mismatches for
spatial types because normalizeColumnType() leaves declaration-only nullability
on getSQLType() (getSpatialSQLType() returns e.g. "POINT SRID 4326 NOT NULL")
while INFORMATION_SCHEMA returns just the base type; update
normalizeColumnType() to strip trailing nullability clauses before comparison
(remove case-insensitive trailing "NOT NULL" or "NULL" and any extra whitespace)
so attributeMatches() compares the base type only and avoids the unnecessary
deleteAttribute(...) + getPDO()->prepare(...)->execute() path.
In `@tests/e2e/Adapter/Scopes/DocumentTests.php`:
- Around line 8372-8374: The test is calling the adapter directly
(getAdapter()->createAttribute/createIndex) inside the withOnDuplicate block,
which can mask failures in Database::createAttribute()/createIndex(); change the
tests to call $database->createAttribute(...) and $database->createIndex(...)
while still using $database->withOnDuplicate(OnDuplicate::Upsert, ...) so the
Database-layer logic is exercised; after doing so assert the final
schema/behavior (e.g. inspect metadata via $database->getCollectionSchema(...)
or attempt a 200-char insert to verify VARCHAR was widened and verify the
rebuilt index targets the expected columns) instead of only checking that an
index named 'idx' exists.
---
Outside diff comments:
In `@src/Database/Database.php`:
- Around line 5685-5688: createDocuments() currently only checks create
permission (Input(self::PERMISSION_CREATE, ...)) and does not enforce update
authorization when OnDuplicate::Upsert would convert inserts into updates,
allowing modification of existing documents; route Upsert flows through the
existing upsert/update authorization path (e.g., call
upsertDocumentsWithIncrease() or updateDocument()) or prefetch duplicate ids and
call authorization->isValid(new Input(self::PERMISSION_UPDATE, $existingDoc))
for each duplicate before invoking the adapter Upsert; ensure you still skip
metadata collection check (self::METADATA) and reuse
$this->authorization->getDescription() in thrown AuthorizationException for
consistency.
In `@tests/e2e/Adapter/Scopes/DocumentTests.php`:
- Around line 8216-8251: The test currently uses a duplicate child with the same
name as the seeded child so an accidental overwrite would still pass; update the
duplicate child Document (the one with '$id' => 'retryChild' created before
calling $database->withOnDuplicate(OnDuplicate::Skip, fn () =>
$database->createDocuments($parent, $batch))) to use a different 'name' value,
then after fetching $existing via $database->getDocument($parent,
'existingParent') assert that the stored child (found via
$existing->getAttribute('children') and its Document->getId()) still has the
original name (e.g., assert the child's getAttribute('name') equals
'ExistingChild'), ensuring OnDuplicate::Skip did not overwrite the existing row.
---
Duplicate comments:
In `@src/Database/Adapter/MariaDB.php`:
- Around line 2410-2417: The upsert assignment builder in MariaDB.php is
currently skipping only '`_uid`' and '`_id`', causing immutable/internal columns
like '`_createdAt`' and '`_tenant`' to be updated on duplicate key; in the
method that contains the foreach ($columns as $col) loop (the Upsert
suffix/assignment generator—compare behavior with getUpsertStatement()), extend
the skip check to include '`_createdAt`' and '`_tenant`' (add them to the
in_array list) so those columns are not added to $assignments and thus are
preserved on duplicate updates.
In `@src/Database/Adapter/Mongo.php`:
- Around line 451-457: The current DuplicateException handling in Mongo.php
returns true even when $tolerateExisting is false, which breaks
OnDuplicate::Fail semantics; update the catch branch that checks ($e instanceof
DuplicateException) so that it returns true only when $tolerateExisting is
truthy, and otherwise rethrows the DuplicateException (or propagate the error)
instead of returning true, referencing the DuplicateException type and the
$tolerateExisting/OnDuplicate::Fail logic to locate the code to change.
- Around line 1530-1545: The upsert payload currently can include immutable _id
and will overwrite existing _permissions; before building $operations in
OnDuplicate::Upsert remove $_id from $payload (unset($payload['_id'])), extract
and remove _permissions from $payload (e.g. $perm = $payload['_permissions'] ??
null; unset($payload['_permissions'])), then build the update so $payload goes
under the intended $operator while any extracted _permissions are applied only
on insert (merge into $update['$setOnInsert']['_permissions'] = $perm when $perm
is not null) so existing rows are not modified; ensure you continue to use the
same symbols ($payload, $operator, $operations, 'filter', 'update') so the
change is localized and test CI against Mongo driver/server behavior.
In `@src/Database/Adapter/Postgres.php`:
- Around line 2463-2482: The current logic makes Skip too broad by using
$conflictTarget based on $this->sharedTables and may emit a bare ON CONFLICT
target that doesn’t match intended semantics; also the upsert branch wrongly
updates the creation timestamp. Change the Skip branch (when $this->onDuplicate
=== OnDuplicate::Skip) to always use the _uid-only conflict target (i.e.
'("_uid")') so Skip remains scoped to _uid conflicts regardless of
$this->sharedTables/$conflictTarget, and modify the assignment-generation loop
that builds $assignments (used in getUpsertStatement()/this method) to also
exclude '"_createdAt"' from updates so creation timestamps are preserved; keep
all other exclusions (\"_id\", \"_tenant\") as-is and ensure the returned ON
CONFLICT strings reflect these fixes.
- Around line 246-254: The current creation logic uses "IF NOT EXISTS" but the
catch block still unconditionally drops tables/indexes, which can remove
pre-existing objects; update the flow around $createTable, $createIndex, and
$createUniqueIndex (and the similar block at the 358-364 area) to only drop
objects that this operation actually created: before running CREATE statements,
record existence (e.g., query information_schema/pg_indexes or pg_tables) or set
created flags based on whether the object existed; in the catch handler, check
those flags or re-check existence and only DROP the tables/indexes that were
created by this run (do not drop if they pre-existed), and keep the behavior
tied to the existing onDuplicate / OnDuplicate::Fail logic.
In `@src/Database/Adapter/SQLite.php`:
- Around line 1956-1980: The code currently uses INSERT OR REPLACE in
getInsertKeyword (and returns empty getInsertSuffix), which performs
delete-then-insert; change the Upsert branch in getInsertKeyword (and adjust
getInsertPermissionsKeyword if needed) to use plain 'INSERT INTO' and implement
getInsertSuffix(string $table, array $columns) to return a proper SQLite UPSERT
clause using ON CONFLICT(col1, col2[, ...] ) DO UPDATE SET ... instead of
REPLACE; ensure the ON CONFLICT target exactly matches the unique index for the
table (name, column order and any COLLATE NOCASE on _uid) so the conflict target
columns and order/collation in getInsertSuffix match the database unique `_uid`
index definition and produce correct DO UPDATE assignments for the provided
$columns.
In `@src/Database/Database.php`:
- Around line 855-866: The precheck logic in Database::withOnDuplicate (and the
similar precheck sites noted) throws DuplicateException for existing
collections/attributes/indexes before the adapter sees the propagated mode;
update the precheck branches in withOnDuplicate and the schema create paths
(e.g., the checks in createCollection/createAttribute/createIndex or wherever
DuplicateException is raised) to consult $this->onDuplicate: if
$this->onDuplicate is OnDuplicate::Skip, return the existing result/no-op
instead of throwing; if OnDuplicate::Upsert, proceed into the reconciliation
path (do not throw) so the adapter-level upsert logic runs; ensure
DuplicateException is still thrown only when $this->onDuplicate is the
default/Fail.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: b391a52f-664b-4fa4-b908-d51a153fa39a
📒 Files selected for processing (8)
src/Database/Adapter/MariaDB.phpsrc/Database/Adapter/Mongo.phpsrc/Database/Adapter/Postgres.phpsrc/Database/Adapter/SQL.phpsrc/Database/Adapter/SQLite.phpsrc/Database/Database.phptests/e2e/Adapter/MirrorTest.phptests/e2e/Adapter/Scopes/DocumentTests.php
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/e2e/Adapter/MirrorTest.php
Comment on lines
+1121
to
+1128
| $err = $this->processException($e); | ||
| if ($err instanceof DuplicateException && $this->onDuplicate === OnDuplicate::Upsert) { | ||
| // Mongo raises IndexKeySpecsConflict when re-creating an index | ||
| // with a name that points to different keys/options. Drop the | ||
| // existing one and recreate with the new spec. | ||
| $this->deleteIndex($collection, $id); | ||
| return $this->client->createIndexes($name, [$indexes], $options); | ||
| } |
Contributor
There was a problem hiding this comment.
Reuse the unique-index readiness wait after an Upsert rebuild.
The Upsert conflict path drops and recreates the index, then returns immediately. For unique indexes this bypasses the readiness polling at Lines 1077-1117, so callers can proceed before the rebuilt unique constraint is ready.
Consider extracting the existing wait loop into a helper and calling it after both the normal createIndexes() and this rebuild path.
🤖 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 1121 - 1128, The Upsert rebuild
path (check using DuplicateException and OnDuplicate::Upsert in the block that
calls deleteIndex and $this->client->createIndexes) recreates the index but
skips the unique-index readiness wait used after the normal createIndexes flow;
extract the existing readiness polling loop (the logic that waits for unique
index readiness) into a private helper method (eg. waitForIndexReady or
ensureUniqueIndexReady) and invoke that helper both after the normal
$this->client->createIndexes(...) call and after the rebuild path that calls
deleteIndex(...) followed by $this->client->createIndexes(...), ensuring callers
only proceed once the unique constraint is ready.
Comment on lines
+260
to
+271
| if ($err instanceof DuplicateException && $this->onDuplicate !== OnDuplicate::Fail) { | ||
| if ($this->onDuplicate === OnDuplicate::Skip) { | ||
| return true; | ||
| } | ||
| // Upsert: drop + recreate only if the declared type differs from | ||
| // what's already in the DB — otherwise the existing column is | ||
| // already correct and re-creation would pointlessly lose data. | ||
| if ($this->attributeMatches($collection, $id, $type, $size, $signed, $array, $required) === true) { | ||
| return true; | ||
| } | ||
| $this->deleteAttribute($collection, $id, $array); | ||
| return $this->getPDO()->prepare($sql)->execute(); |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Inspect SQL type declarations returned by adapter getSQLType implementations.
# Expectation: If getSQLType appends NOT NULL/NULL/DEFAULT, normalizeColumnType should strip those before comparing.
rg -n -C 6 'function getSQLType|NOT NULL| DEFAULT | NULL' src/Database/AdapterRepository: utopia-php/database
Length of output: 27862
🏁 Script executed:
# Locate attributeMatches() method and understand the comparison logic
rg -n -A 20 'protected function attributeMatches' src/Database/Adapter/SQL.phpRepository: utopia-php/database
Length of output: 983
🏁 Script executed:
# Check normalizeColumnType() method and its current implementation
rg -n -A 15 'protected function normalizeColumnType' src/Database/Adapter/SQL.phpRepository: utopia-php/database
Length of output: 705
🏁 Script executed:
# Review lines 1935-1952 to understand the "Also applies to" section
sed -n '1930,1960p' src/Database/Adapter/SQL.phpRepository: utopia-php/database
Length of output: 1343
🏁 Script executed:
# Check if non-spatial getSQLType() methods also append NOT NULL/NULL
rg -n -B 5 -A 20 'protected function getSQLType' src/Database/Adapter/MySQL.phpRepository: utopia-php/database
Length of output: 45
🏁 Script executed:
# Check for MariaDB getSQLType and all spatial handling
rg -n -A 50 'protected function getSQLType' src/Database/Adapter/MariaDB.php | head -100Repository: utopia-php/database
Length of output: 2240
🏁 Script executed:
# Check Postgres getSQLType to see if it also appends NOT NULL/NULL
sed -n '2025,2070p' src/Database/Adapter/Postgres.phpRepository: utopia-php/database
Length of output: 1527
🏁 Script executed:
# Search for spatial type handling and what metadata queries return
rg -n 'POINT|LINESTRING|POLYGON' src/Database/Adapter | grep -E '(getSQLType|SRID|spatial)' -iRepository: utopia-php/database
Length of output: 1133
🏁 Script executed:
# Check if there are any tests showing what INFORMATION_SCHEMA returns for spatial columns
fd -e php test | xargs rg -l 'spatial|POINT|attributeMatches' 2>/dev/null | head -3Repository: utopia-php/database
Length of output: 180
🏁 Script executed:
# Look at the actual DELETE + recreate code path (line 270) to understand the consequence
sed -n '258,275p' src/Database/Adapter/SQL.phpRepository: utopia-php/database
Length of output: 953
🏁 Script executed:
# Check what INFORMATION_SCHEMA.COLUMNS actually returns for nullable columns
# and whether COLUMN_TYPE includes nullability
rg -n 'INFORMATION_SCHEMA|IS_NULLABLE|COLUMN_TYPE' src/Database/AdapterRepository: utopia-php/database
Length of output: 1593
🏁 Script executed:
# Check if there are any adapter-specific attributeMatches() overrides
rg -n 'protected function attributeMatches' src/Database/AdapterRepository: utopia-php/database
Length of output: 295
🏁 Script executed:
# Check getSQLType for Postgres and SQLite to see if they append NOT NULL/NULL
sed -n '2025,2100p' src/Database/Adapter/Postgres.php | grep -A 50 'case Database::VAR_POINT'Repository: utopia-php/database
Length of output: 1255
🏁 Script executed:
# Verify the createAttribute signature and calls to understand $required parameter flow
rg -n -B 5 -A 10 'deleteAttribute' src/Database/Adapter/SQL.php | head -60Repository: utopia-php/database
Length of output: 1336
🏁 Script executed:
# Verify Postgres and SQLite attributeMatches() overrides to confirm they handle their metadata correctly
sed -n '1965,2010p' src/Database/Adapter/Postgres.phpRepository: utopia-php/database
Length of output: 2115
🏁 Script executed:
# Check SQLite attributeMatches override
sed -n '1989,2030p' src/Database/Adapter/SQLite.phpRepository: utopia-php/database
Length of output: 940
Add nullability clause normalization to prevent false column type mismatches for spatial types.
attributeMatches() compares INFORMATION_SCHEMA.COLUMNS.COLUMN_TYPE against getSQLType() output. MySQL/MariaDB's getSpatialSQLType() appends NOT NULL or NULL based on the $required parameter, but metadata queries return only the base type (e.g., POINT SRID 4326 vs. POINT SRID 4326 NOT NULL). The current normalizeColumnType() doesn't strip these clauses, causing false mismatches that trigger unnecessary column drop/recreate at line 270, with data loss risk.
Update normalizeColumnType() to strip declaration-only clauses before comparison:
Proposed fix
protected function normalizeColumnType(string $sql): string
{
+ $sql = \preg_replace('/\s+DEFAULT\s+(?:\'[^\']*\'|"[^"]*"|[^\s]+)/i', '', $sql);
+ $sql = \preg_replace('/\s+NOT\s+NULL\b/i', '', $sql);
+ $sql = \preg_replace('/\s+NULL\b/i', '', $sql);
$sql = \preg_replace('/\b(BIGINT|INT|SMALLINT|MEDIUMINT)\s*\(\s*\d+\s*\)/i', '$1', $sql);
$sql = \preg_replace('/\s+/', ' ', $sql);
return \strtoupper(\trim($sql));
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/Database/Adapter/SQL.php` around lines 260 - 271, attributeMatches(...)
is falsely reporting mismatches for spatial types because normalizeColumnType()
leaves declaration-only nullability on getSQLType() (getSpatialSQLType() returns
e.g. "POINT SRID 4326 NOT NULL") while INFORMATION_SCHEMA returns just the base
type; update normalizeColumnType() to strip trailing nullability clauses before
comparison (remove case-insensitive trailing "NOT NULL" or "NULL" and any extra
whitespace) so attributeMatches() compares the base type only and avoids the
unnecessary deleteAttribute(...) + getPDO()->prepare(...)->execute() path.
…xMatches
Replaces exception-driven control flow in createAttribute and createIndex
with pre-check helpers (attributeMatches / indexMatches). The previous
pattern had three growth-cost problems:
- Error-code classification drift across DB versions (MariaDB vs MySQL vs
MySQL compat modes) could silently shift a duplicate to a rethrow path.
- On Postgres, a failed CREATE INDEX / ALTER TABLE inside a transaction
aborts the outer transaction — catch-and-retry is unsafe.
- Layering new concerns (type-check, deadlock retry) stacks branches inside
the same catch instead of reading top-to-bottom.
Pre-check pattern (mirrors attributeMatches):
$match = $this->indexMatches(...);
if ($match === true) return true; // spec unchanged, no-op
if ($match === false) {
if (Skip) return true; // tolerate
deleteIndex(...); // Upsert: rebuild
}
// null: doesn't exist, fall through to CREATE
indexMatches implementations:
- SQL base (MariaDB/MySQL): INFORMATION_SCHEMA.STATISTICS, compares column
list (ordered) + NON_UNIQUE flag + INDEX_TYPE (BTREE/FULLTEXT/SPATIAL).
- Postgres override: pg_catalog via unnest(indkey) for column ordering,
indisunique for uniqueness, pg_am.amname for index method (btree/gist/
gin/hnsw).
- SQLite override: PRAGMA index_list + PRAGMA index_info. Closes cursors
before returning — SQLite holds a table-level lock while a read cursor
is open which would block subsequent DDL.
SQLite.createIndex runs the pre-check in every mode (including Fail): the
method has historically been unconditionally idempotent because
createCollection intentionally issues the same CREATE INDEX twice for
_perms side-tables. Matching the contract keeps that path working.
Per-column length and sort order are intentionally ignored in the
comparison — a difference there yields a conservative false negative
(unnecessary rebuild), never a false positive.
New test: testUpsertIndexSameSpecNoop — re-declaring an index with the
same spec under OnDuplicate::Upsert is a pure no-op (no DROP / CREATE
DDL fires), symmetric with the pre-existing
testUpsertAttributeSameTypeNoop.
Row Upsert (createDocuments) is intentionally NOT changed: DB-native
conflict primitives (INSERT IGNORE / ON DUPLICATE KEY UPDATE / ON CONFLICT
DO UPDATE / $set+upsert:true) resolve duplicates atomically per-row in
one statement. A find()-then-insert pre-check would be slower, race-prone,
and miss conflicts on user-defined unique indexes.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (3)
src/Database/Adapter/SQLite.php (1)
1949-1974:⚠️ Potential issue | 🟠 MajorSQLite Upsert still uses
INSERT OR REPLACE(delete-then-insert) semantics.Previously flagged, still present.
INSERT OR REPLACEdeletes the conflicting row (firing delete triggers, cascading FKs, re-assigning_idvia AUTOINCREMENT) and inserts a new one withNULL/default for any column not listed — losing columns that weren't part of the upsert payload. This diverges from the MariaDB/Postgres Upsert contract (targeted UPDATE of provided columns,_id/_tenantpreserved).Since the project already requires SQLite ≥ 3.38,
ON CONFLICT (...) DO UPDATE SET col = excluded.colhas been available since 3.24 and is the direct equivalent to the other adapters.🛠️ Proposed fix
protected function getInsertKeyword(): string { return match ($this->onDuplicate) { OnDuplicate::Skip => 'INSERT OR IGNORE INTO', - OnDuplicate::Upsert => 'INSERT OR REPLACE INTO', + OnDuplicate::Upsert => 'INSERT INTO', OnDuplicate::Fail => 'INSERT INTO', }; } @@ - protected function getInsertSuffix(string $table, array $columns = []): string - { - return ''; - } + protected function getInsertSuffix(string $table, array $columns = []): string + { + if ($this->onDuplicate !== OnDuplicate::Upsert || empty($columns)) { + return ''; + } + + $assignments = []; + foreach ($columns as $col) { + if (\in_array($col, ['`_uid`', '`_id`', '`_tenant`'], true)) { + continue; + } + $assignments[] = "{$col} = excluded.{$col}"; + } + + if (empty($assignments)) { + return ''; + } + + $conflictTarget = $this->sharedTables ? '(`_uid`, `_tenant`)' : '(`_uid`)'; + + return "ON CONFLICT {$conflictTarget} DO UPDATE SET " . \implode(', ', $assignments); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Database/Adapter/SQLite.php` around lines 1949 - 1974, The current SQLite adapter uses INSERT OR REPLACE (in getInsertKeyword and getInsertPermissionsKeyword) which causes delete-then-insert semantics; change the implementation to emit a plain INSERT INTO (not INSERT OR REPLACE/IGNORE) and implement a proper ON CONFLICT(...) DO UPDATE SET ... clause via getInsertSuffix so upserts use "ON CONFLICT(conflict_cols) DO UPDATE SET col = excluded.col" (preserve primary key/_id and _tenant, only update provided columns, and avoid firing delete triggers or changing AUTOINCREMENT); use the $columns parameter in getInsertSuffix to build the conflict target and the excluded assignments for all non-PK columns, and ensure getInsertPermissionsKeyword follows the same INSERT INTO semantics for permission inserts.src/Database/Adapter/SQL.php (1)
1945-1951:⚠️ Potential issue | 🟠 Major
normalizeColumnType()still doesn't strip nullability/default clauses.This was flagged previously but the helper still only normalizes integer display widths and whitespace. Since MariaDB/MySQL
getSpatialSQLType()appendsNOT NULL/NULLbased on$required, whileINFORMATION_SCHEMA.COLUMN_TYPEreturns only the base type (e.g.POINT SRID 4326),attributeMatches()will report a falsefalsefor every spatial column and trigger the destructivedeleteAttribute→ ADD COLUMN path underOnDuplicate::Upsert, dropping data.🛠️ Proposed fix
protected function normalizeColumnType(string $sql): string { + $sql = \preg_replace('/\s+DEFAULT\s+(?:\'[^\']*\'|"[^"]*"|[^\s]+)/i', '', $sql); + $sql = \preg_replace('/\s+NOT\s+NULL\b/i', '', $sql); + $sql = \preg_replace('/\s+NULL\b/i', '', $sql); $sql = \preg_replace('/\b(BIGINT|INT|SMALLINT|MEDIUMINT)\s*\(\s*\d+\s*\)/i', '$1', $sql); $sql = \preg_replace('/\s+/', ' ', $sql); return \strtoupper(\trim($sql)); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Database/Adapter/SQL.php` around lines 1945 - 1951, normalizeColumnType() currently only normalizes integer widths and whitespace but must also strip nullability and default clauses so comparisons with INFORMATION_SCHEMA types (e.g. from getSpatialSQLType()) match; update normalizeColumnType to remove trailing NULL/NOT NULL tokens and any DEFAULT ... clause (and optionally other modifiers like AUTO_INCREMENT/UNSIGNED if present) before trimming and uppercasing. Target the normalizeColumnType method and adjust its regexes to strip patterns like /\s+(NOT\s+NULL|NULL)\b/i and /\s+DEFAULT\s+[^,\s)]+/i (and additional modifiers as needed) so attributeMatches() no longer false-positive triggers deleteAttribute → ADD COLUMN in OnDuplicate::Upsert.tests/e2e/Adapter/Scopes/DocumentTests.php (1)
8371-8507:⚠️ Potential issue | 🟠 MajorExercise the public schema APIs and verify the final schema spec.
These schema tests still call
$database->getAdapter()directly insidewithOnDuplicate(), so they can pass even ifDatabase::createAttribute()/Database::createIndex()do not honor the new scope. They also mostly prove the resource exists, not that the requested spec was applied or preserved. Prefer calling$database->createAttribute()/$database->createIndex()and asserting behavior/metadata, e.g. a widened string accepts a 200-char value and the rebuilt index targets the requested attribute.🤖 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 8371 - 8507, These tests call $database->getAdapter() inside withOnDuplicate(), bypassing the Database scope handling; replace those adapter-level createAttribute/createIndex calls with the public $database->createAttribute(...) and $database->createIndex(...) inside the withOnDuplicate blocks (e.g. in testUpsertAttrTypeChangedRecreates, testUpsertIndexRebuilds, testUpsertIndexSameSpecNoop, testSkipSchemaTolerates) and then assert the actual schema/behavior: for attributes assert metadata via $database->getAttribute(...) or insert a 200-char string into the widened VARCHAR to confirm it accepts the new size; for indexes assert the index targets the requested column (via $database->getIndex/...metadata or by querying using the index) and that deleteIndex still succeeds. Ensure all assertions verify the spec was applied, not just that the resource exists.
🧹 Nitpick comments (1)
src/Database/Adapter/SQLite.php (1)
207-230: Redundant_permsindex creation at lines 229–230.The exact same two
_permsindexes (_index_1unique and_index_2key) are created at lines 211–212 and again at 229–230. Before this PR the second pair may have served as a defensive fallback, but nowcreateIndex→indexMatcheswill match-and-no-op the second pair in every mode, making them pure dead work (two extra PRAGMA round-trips percreateCollection). Safe to drop one of the pairs.♻️ Proposed cleanup
foreach ($indexes as $index) { ... $this->createIndex($id, $indexId, $indexType, $indexAttributes, $indexLengths, $indexOrders, [], [], $indexTtl); } - - $this->createIndex("{$id}_perms", '_index_1', Database::INDEX_UNIQUE, ['_document', '_type', '_permission'], [], []); - $this->createIndex("{$id}_perms", '_index_2', Database::INDEX_KEY, ['_permission', '_type'], [], []);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Database/Adapter/SQLite.php` around lines 207 - 230, Duplicate creation of the "_perms" indexes (_index_1 unique and _index_2 key) occurs twice in SQLite.php causing redundant PRAGMA calls; remove the second pair (the calls to createIndex that pass "{$id}_perms" with '_index_1' and '_index_2') after the foreach so only the first pair remains; verify createIndex and indexMatches still behave as expected and run tests for createCollection to ensure no functional change.
🤖 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/Adapter/Postgres.php`:
- Around line 2044-2101: indexMatches() currently returns null when pg_attribute
join drops expression-only index columns; instead, when $rows is empty, run a
lightweight existence check against pg_catalog.pg_class (filtered by schema and
index name) to determine if the index object exists: if pg_class has no row
return null (truly absent), if pg_class has a row return false (index exists but
is expression-only/uninspectable). Update indexMatches() accordingly (affects
behavior when callers like createIndex()/buildJsonbPath and OnDuplicate::Skip
expect Skip to be a no-op vs DuplicateException), so Skip treats
present-but-unintrospectable indexes as "matches" => no CREATE or as
conservative false to avoid duplicate errors per desired policy. Ensure you
reference $keyName, $this->getDatabase(), and use pg_catalog.pg_class lookup to
distinguish absent vs uninspectable.
In `@src/Database/Adapter/SQL.php`:
- Around line 247-273: The createAttribute method currently executes the ALTER
TABLE without mapping PDOExceptions through processException(), so non-duplicate
DDL errors escape as raw PDOException; modify createAttribute to wrap the final
DDL execution in try/catch like the sibling methods (createAttributes,
renameAttribute, deleteAttribute): call
$this->getPDO()->prepare($sql)->execute() inside try { ... } and in
catch(PDOException $e) rethrow throw $this->processException($e); ensuring
existing pre-check logic (attributeMatches, deleteAttribute) remains unchanged.
In `@tests/e2e/Adapter/Scopes/DocumentTests.php`:
- Around line 8270-8277: The upsert row tests (e.g.,
testCreateDocsUpsertOverwrites and the similar test that calls
withOnDuplicate(OnDuplicate::Upsert)) must be skipped for adapters that don't
support upserts; add a guard at the start of each such test using
$this->getSupportForUpserts() and call $this->markTestSkipped('Adapter does not
support upserts') when it returns false so the test will not run against
unsupported adapters. Ensure you place this check inside the test method before
creating collections/attributes or calling withOnDuplicate(OnDuplicate::Upsert).
---
Duplicate comments:
In `@src/Database/Adapter/SQL.php`:
- Around line 1945-1951: normalizeColumnType() currently only normalizes integer
widths and whitespace but must also strip nullability and default clauses so
comparisons with INFORMATION_SCHEMA types (e.g. from getSpatialSQLType()) match;
update normalizeColumnType to remove trailing NULL/NOT NULL tokens and any
DEFAULT ... clause (and optionally other modifiers like AUTO_INCREMENT/UNSIGNED
if present) before trimming and uppercasing. Target the normalizeColumnType
method and adjust its regexes to strip patterns like /\s+(NOT\s+NULL|NULL)\b/i
and /\s+DEFAULT\s+[^,\s)]+/i (and additional modifiers as needed) so
attributeMatches() no longer false-positive triggers deleteAttribute → ADD
COLUMN in OnDuplicate::Upsert.
In `@src/Database/Adapter/SQLite.php`:
- Around line 1949-1974: The current SQLite adapter uses INSERT OR REPLACE (in
getInsertKeyword and getInsertPermissionsKeyword) which causes
delete-then-insert semantics; change the implementation to emit a plain INSERT
INTO (not INSERT OR REPLACE/IGNORE) and implement a proper ON CONFLICT(...) DO
UPDATE SET ... clause via getInsertSuffix so upserts use "ON
CONFLICT(conflict_cols) DO UPDATE SET col = excluded.col" (preserve primary
key/_id and _tenant, only update provided columns, and avoid firing delete
triggers or changing AUTOINCREMENT); use the $columns parameter in
getInsertSuffix to build the conflict target and the excluded assignments for
all non-PK columns, and ensure getInsertPermissionsKeyword follows the same
INSERT INTO semantics for permission inserts.
In `@tests/e2e/Adapter/Scopes/DocumentTests.php`:
- Around line 8371-8507: These tests call $database->getAdapter() inside
withOnDuplicate(), bypassing the Database scope handling; replace those
adapter-level createAttribute/createIndex calls with the public
$database->createAttribute(...) and $database->createIndex(...) inside the
withOnDuplicate blocks (e.g. in testUpsertAttrTypeChangedRecreates,
testUpsertIndexRebuilds, testUpsertIndexSameSpecNoop, testSkipSchemaTolerates)
and then assert the actual schema/behavior: for attributes assert metadata via
$database->getAttribute(...) or insert a 200-char string into the widened
VARCHAR to confirm it accepts the new size; for indexes assert the index targets
the requested column (via $database->getIndex/...metadata or by querying using
the index) and that deleteIndex still succeeds. Ensure all assertions verify the
spec was applied, not just that the resource exists.
---
Nitpick comments:
In `@src/Database/Adapter/SQLite.php`:
- Around line 207-230: Duplicate creation of the "_perms" indexes (_index_1
unique and _index_2 key) occurs twice in SQLite.php causing redundant PRAGMA
calls; remove the second pair (the calls to createIndex that pass "{$id}_perms"
with '_index_1' and '_index_2') after the foreach so only the first pair
remains; verify createIndex and indexMatches still behave as expected and run
tests for createCollection to ensure no functional change.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 9b89e28d-3619-4c6a-9b23-229b7bc1aaff
📒 Files selected for processing (5)
src/Database/Adapter/MariaDB.phpsrc/Database/Adapter/Postgres.phpsrc/Database/Adapter/SQL.phpsrc/Database/Adapter/SQLite.phptests/e2e/Adapter/Scopes/DocumentTests.php
Comment on lines
+2044
to
+2101
| $stmt = $this->getPDO()->prepare(" | ||
| SELECT a.attname AS column_name, i.indisunique AS is_unique, am.amname AS method | ||
| FROM pg_catalog.pg_index i | ||
| JOIN pg_catalog.pg_class c ON c.oid = i.indexrelid | ||
| JOIN pg_catalog.pg_namespace n ON n.oid = c.relnamespace | ||
| JOIN pg_catalog.pg_am am ON am.oid = c.relam | ||
| CROSS JOIN LATERAL unnest(i.indkey) WITH ORDINALITY AS k(attnum, pos) | ||
| JOIN pg_catalog.pg_attribute a ON a.attrelid = i.indrelid AND a.attnum = k.attnum | ||
| WHERE n.nspname = :schema | ||
| AND c.relname = :index | ||
| ORDER BY k.pos | ||
| "); | ||
| $stmt->bindValue(':schema', $this->getDatabase(), PDO::PARAM_STR); | ||
| $stmt->bindValue(':index', $keyName, PDO::PARAM_STR); | ||
| $stmt->execute(); | ||
| $rows = $stmt->fetchAll(PDO::FETCH_ASSOC); | ||
|
|
||
| if (empty($rows)) { | ||
| return null; | ||
| } | ||
|
|
||
| if ((bool) $rows[0]['is_unique'] !== ($type === Database::INDEX_UNIQUE)) { | ||
| return false; | ||
| } | ||
|
|
||
| // Map declared index type to Postgres access method (pg_am.amname) | ||
| $targetMethod = match ($type) { | ||
| Database::INDEX_SPATIAL => 'gist', | ||
| Database::INDEX_HNSW_EUCLIDEAN, | ||
| Database::INDEX_HNSW_COSINE, | ||
| Database::INDEX_HNSW_DOT => 'hnsw', | ||
| Database::INDEX_OBJECT, | ||
| Database::INDEX_TRIGRAM => 'gin', | ||
| default => 'btree', // INDEX_KEY, INDEX_UNIQUE, INDEX_FULLTEXT | ||
| }; | ||
| if (\strtolower((string) $rows[0]['method']) !== $targetMethod) { | ||
| return false; | ||
| } | ||
|
|
||
| // Target column list — mirror createIndex's transformation including | ||
| // shared-tables tenant prefix for KEY/UNIQUE indexes. | ||
| $targetCols = []; | ||
| foreach ($attributes as $attr) { | ||
| $targetCols[] = match ($attr) { | ||
| '$id' => '_uid', | ||
| '$createdAt' => '_createdAt', | ||
| '$updatedAt' => '_updatedAt', | ||
| default => $this->filter($attr), | ||
| }; | ||
| } | ||
| if ($this->sharedTables && \in_array($type, [Database::INDEX_KEY, Database::INDEX_UNIQUE])) { | ||
| \array_unshift($targetCols, '_tenant'); | ||
| } | ||
|
|
||
| $existingCols = \array_map(fn ($r) => (string) $r['column_name'], $rows); | ||
|
|
||
| return $targetCols === $existingCols; | ||
| } |
Contributor
There was a problem hiding this comment.
indexMatches() breaks Skip for expression-only indexes (INDEX_OBJECT on nested JSON paths).
The join pg_attribute a ON a.attrelid = i.indrelid AND a.attnum = k.attnum silently drops expression components (indkey entry of 0). For indexes built via buildJsonbPath() in createIndex() (i.e. INDEX_OBJECT with a nested 'a.b' path — a fully expression-based index), every component is an expression, so $rows is empty and the method returns null — "index doesn't exist".
Caller at line 985 then falls through to plain CREATE INDEX (no IF NOT EXISTS), which raises DuplicateException under OnDuplicate::Skip. That contradicts the Skip contract.
Distinguish "index absent" from "index present but unintrospectable" — e.g. first confirm existence via pg_class on the short key, and treat a present-but-no-regular-columns index as a conservative false (Skip → no-op; Upsert → rebuild).
🛠 Sketch
$stmt->execute();
$rows = $stmt->fetchAll(PDO::FETCH_ASSOC);
- if (empty($rows)) {
- return null;
- }
+ if (empty($rows)) {
+ // Index may genuinely be absent, or it may be expression-only
+ // (e.g. INDEX_OBJECT on a nested JSON path) — re-query existence
+ // to disambiguate before reporting "does not exist".
+ $exists = $this->getPDO()->prepare('
+ SELECT 1 FROM pg_catalog.pg_class c
+ JOIN pg_catalog.pg_namespace n ON n.oid = c.relnamespace
+ WHERE c.relkind = \'i\' AND n.nspname = :schema AND c.relname = :index
+ ');
+ $exists->bindValue(':schema', $this->getDatabase(), PDO::PARAM_STR);
+ $exists->bindValue(':index', $keyName, PDO::PARAM_STR);
+ $exists->execute();
+ return $exists->fetchColumn() !== false ? false : null;
+ }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/Database/Adapter/Postgres.php` around lines 2044 - 2101, indexMatches()
currently returns null when pg_attribute join drops expression-only index
columns; instead, when $rows is empty, run a lightweight existence check against
pg_catalog.pg_class (filtered by schema and index name) to determine if the
index object exists: if pg_class has no row return null (truly absent), if
pg_class has a row return false (index exists but is
expression-only/uninspectable). Update indexMatches() accordingly (affects
behavior when callers like createIndex()/buildJsonbPath and OnDuplicate::Skip
expect Skip to be a no-op vs DuplicateException), so Skip treats
present-but-unintrospectable indexes as "matches" => no CREATE or as
conservative false to avoid duplicate errors per desired policy. Ensure you
reference $keyName, $this->getDatabase(), and use pg_catalog.pg_class lookup to
distinguish absent vs uninspectable.
Comment on lines
+8270
to
+8277
| public function testCreateDocsUpsertOverwrites(): void | ||
| { | ||
| /** @var Database $database */ | ||
| $database = $this->getDatabase(); | ||
|
|
||
| $database->createCollection(__FUNCTION__); | ||
| $database->createAttribute(__FUNCTION__, 'name', Database::VAR_STRING, 128, true); | ||
| $database->createAttribute(__FUNCTION__, 'tag', Database::VAR_STRING, 128, false); |
Contributor
There was a problem hiding this comment.
Guard row-Upsert tests for adapters without upsert support.
Most existing upsert tests in this file skip when getSupportForUpserts() is false. These new withOnDuplicate(OnDuplicate::Upsert) row tests should do the same to avoid failing unsupported adapters in the full matrix.
🧪 Proposed guard
public function testCreateDocsUpsertOverwrites(): void
{
/** `@var` Database $database */
$database = $this->getDatabase();
+ if (!$database->getAdapter()->getSupportForUpserts()) {
+ $this->expectNotToPerformAssertions();
+ return;
+ }
+
$database->createCollection(__FUNCTION__);
$database->createAttribute(__FUNCTION__, 'name', Database::VAR_STRING, 128, true);
$database->createAttribute(__FUNCTION__, 'tag', Database::VAR_STRING, 128, false);
@@
public function testCreateDocsUpsertAll(): void
{
/** `@var` Database $database */
$database = $this->getDatabase();
+ if (!$database->getAdapter()->getSupportForUpserts()) {
+ $this->expectNotToPerformAssertions();
+ return;
+ }
+
$database->createCollection(__FUNCTION__);
$database->createAttribute(__FUNCTION__, 'name', Database::VAR_STRING, 128, true);Also applies to: 8314-8320
🤖 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 8270 - 8277, The
upsert row tests (e.g., testCreateDocsUpsertOverwrites and the similar test that
calls withOnDuplicate(OnDuplicate::Upsert)) must be skipped for adapters that
don't support upserts; add a guard at the start of each such test using
$this->getSupportForUpserts() and call $this->markTestSkipped('Adapter does not
support upserts') when it returns false so the test will not run against
unsupported adapters. Ensure you place this check inside the test method before
creating collections/attributes or calling withOnDuplicate(OnDuplicate::Upsert).
The pre-check refactor (a42f4cb) accidentally dropped the try/catch { throw processException($e) } wrap around the final execute() in createAttribute / createIndex. Duplicate handling moved to the pre-check, but other DDL errors (row-size limit, timeout, lock wait, connection drop, NOT NULL violation on table with existing rows, etc.) were leaking as raw PDOException instead of being mapped to the adapter domain exceptions (LimitException, TimeoutException, StructureException, ...). CI caught this via testCreateSpatialColumnWithExistingData on MySQL / SharedTables-MySQL / Pool — the test expects a Structure exception when adding a NOT NULL spatial column to a table that already has rows, and the raw PDOException failed the instanceof assertion. Flagged in review as greptile P1 + coderabbit Major. Sibling DDL methods (createAttributes, renameAttribute, deleteAttribute, deleteIndex) wrap the same way; we were the outliers. Affects: - SQL.createAttribute - Postgres.createAttribute - MariaDB.createIndex - Postgres.createIndex - SQLite.createIndex
…reate)
Design change after review: drop+recreate for schema reconciliation belongs
in the migration package, not in the database library. Migration compares
source vs destination _metadata._updatedAt and issues deleteAttribute /
deleteIndex itself before re-creating — the library just needs to not throw
when the resource already exists.
Consequences:
1. Adapter-level pre-check pattern and introspection helpers are all gone —
~400 lines of dialect-specific INFORMATION_SCHEMA / pg_catalog / PRAGMA
code reverted. Schema introspection was the fragile part; removing it
shrinks the PR's surface area significantly.
- SQL.php: createAttribute back to plain try/catch → processException;
attributeMatches, normalizeColumnType, indexMatches all deleted.
- Postgres.php: createAttribute + createIndex reverted; IF NOT EXISTS
variants in createCollection removed; overrides for attributeMatches /
normalizeColumnType / indexMatches deleted.
- MariaDB.php: createIndex back to plain try/catch; CREATE TABLE IF NOT
EXISTS removed (always plain CREATE).
- SQLite.php: createCollection back to plain CREATE TABLE; createIndex
back to historical "always idempotent" pre-check via sqlite_master
(no mode-aware branches); attributeMatches / indexMatches deleted.
- Mongo.php: createIndex Upsert catch-retry removed; createCollection's
tolerateExisting no longer includes onDuplicate != Fail.
2. Database.php now owns schema Skip/Upsert dispatch. All three create
methods check _metadata and return early on existing resources:
- createCollection: returns the existing metadata document.
- createAttribute: scans _metadata.attributes JSON for the $id.
- createIndex: scans _metadata.indexes JSON for the $id.
Upsert == Skip at schema level — both tolerate. Callers that need to
change a spec drop+recreate themselves.
3. Mirror.php forwards the OnDuplicate scope on every schema op
(createCollection / createAttribute / createIndex) to both source and
destination so their Database-layer dispatch observes it. Previously the
mode only propagated on createDocuments.
4. Tests rewritten to route through $database-> (not the adapter directly),
exercising the full Database-layer flow:
- testCreateCollSkipUpsertTolerates
- testCreateAttrSkipUpsertTolerates (asserts: wider-spec re-declare is
ignored, metadata unchanged)
- testCreateIdxSkipUpsertTolerates (same — different-column re-declare
is tolerated, metadata unchanged)
Row-level Skip/Upsert is unchanged — DB-native conflict primitives
(INSERT IGNORE / ON CONFLICT / bulkWrite+$setOnInsert|$set) stay in the
adapters. The OnDuplicate enum + withOnDuplicate scope on Database +
Adapter + Mirror are also unchanged.
Comment on lines
8262
to
8460
| \sort($allChildIds); | ||
| $this->assertSame(['existingChild', 'newChild', 'retryChild'], $allChildIds); | ||
| } | ||
|
|
||
| /** | ||
| * OnDuplicate::Upsert — existing rows are overwritten with the incoming | ||
| * values; new rows are inserted. The returned count reflects every input. | ||
| */ | ||
| public function testCreateDocsUpsertOverwrites(): void | ||
| { | ||
| /** @var Database $database */ | ||
| $database = $this->getDatabase(); | ||
|
|
||
| $database->createCollection(__FUNCTION__); | ||
| $database->createAttribute(__FUNCTION__, 'name', Database::VAR_STRING, 128, true); | ||
| $database->createAttribute(__FUNCTION__, 'tag', Database::VAR_STRING, 128, false); | ||
|
|
||
| $permissions = [ | ||
| Permission::read(Role::any()), | ||
| Permission::create(Role::any()), | ||
| Permission::update(Role::any()), | ||
| ]; | ||
|
|
||
| // Seed two docs. | ||
| $database->createDocuments(__FUNCTION__, [ | ||
| new Document(['$id' => 'a', 'name' => 'original-A', 'tag' => 'keep', '$permissions' => $permissions]), | ||
| new Document(['$id' => 'b', 'name' => 'original-B', 'tag' => 'keep', '$permissions' => $permissions]), | ||
| ]); | ||
|
|
||
| // Upsert: overwrite 'a', leave 'b' untouched (not in batch), insert 'c'. | ||
| $collection = __FUNCTION__; | ||
| $count = $database->withOnDuplicate(OnDuplicate::Upsert, function () use ($database, $collection, $permissions) { | ||
| return $database->createDocuments($collection, [ | ||
| new Document(['$id' => 'a', 'name' => 'replaced-A', 'tag' => 'new', '$permissions' => $permissions]), | ||
| new Document(['$id' => 'c', 'name' => 'inserted-C', 'tag' => 'new', '$permissions' => $permissions]), | ||
| ]); | ||
| }); | ||
| $this->assertSame(2, $count); | ||
|
|
||
| $docs = $database->find(__FUNCTION__, [Query::orderAsc('$id')]); | ||
| $this->assertCount(3, $docs); | ||
| $this->assertSame('replaced-A', $docs[0]->getAttribute('name')); | ||
| $this->assertSame('new', $docs[0]->getAttribute('tag')); | ||
| $this->assertSame('original-B', $docs[1]->getAttribute('name')); | ||
| $this->assertSame('keep', $docs[1]->getAttribute('tag')); | ||
| $this->assertSame('inserted-C', $docs[2]->getAttribute('name')); | ||
| } | ||
|
|
||
| /** | ||
| * OnDuplicate::Upsert — a batch composed entirely of duplicates overwrites | ||
| * every existing row; zero rows are skipped. | ||
| */ | ||
| public function testCreateDocsUpsertAll(): void | ||
| { | ||
| /** @var Database $database */ | ||
| $database = $this->getDatabase(); | ||
|
|
||
| $database->createCollection(__FUNCTION__); | ||
| $database->createAttribute(__FUNCTION__, 'name', Database::VAR_STRING, 128, true); | ||
|
|
||
| $permissions = [ | ||
| Permission::read(Role::any()), | ||
| Permission::create(Role::any()), | ||
| Permission::update(Role::any()), | ||
| ]; | ||
|
|
||
| $database->createDocuments(__FUNCTION__, [ | ||
| new Document(['$id' => 'x', 'name' => 'v1', '$permissions' => $permissions]), | ||
| new Document(['$id' => 'y', 'name' => 'v1', '$permissions' => $permissions]), | ||
| ]); | ||
|
|
||
| $collection = __FUNCTION__; | ||
| $count = $database->withOnDuplicate(OnDuplicate::Upsert, function () use ($database, $collection, $permissions) { | ||
| return $database->createDocuments($collection, [ | ||
| new Document(['$id' => 'x', 'name' => 'v2', '$permissions' => $permissions]), | ||
| new Document(['$id' => 'y', 'name' => 'v2', '$permissions' => $permissions]), | ||
| ]); | ||
| }); | ||
| $this->assertSame(2, $count); | ||
|
|
||
| $docs = $database->find(__FUNCTION__, [Query::orderAsc('$id')]); | ||
| $this->assertCount(2, $docs); | ||
| $this->assertSame('v2', $docs[0]->getAttribute('name')); | ||
| $this->assertSame('v2', $docs[1]->getAttribute('name')); | ||
| } | ||
|
|
||
| /** | ||
| * OnDuplicate::Skip and Upsert tolerate an existing collection — they | ||
| * return the current metadata document instead of throwing. Collections | ||
| * are never destructive at the library layer; callers that need to | ||
| * reconcile schema drop/recreate the individual attributes / indexes. | ||
| */ | ||
| public function testCreateCollSkipUpsertTolerates(): void | ||
| { | ||
| /** @var Database $database */ | ||
| $database = $this->getDatabase(); | ||
|
|
||
| $database->createCollection(__FUNCTION__); | ||
| $database->createAttribute(__FUNCTION__, 'name', Database::VAR_STRING, 128, true); | ||
| $database->createDocument(__FUNCTION__, new Document([ | ||
| '$id' => 'doc', | ||
| 'name' => 'keep', | ||
| '$permissions' => [Permission::read(Role::any())], | ||
| ])); | ||
|
|
||
| $collection = __FUNCTION__; | ||
| $database->withOnDuplicate(OnDuplicate::Skip, fn () => $database->createCollection($collection)); | ||
| $this->assertSame('keep', $database->getDocument(__FUNCTION__, 'doc')->getAttribute('name')); | ||
|
|
||
| $database->withOnDuplicate(OnDuplicate::Upsert, fn () => $database->createCollection($collection)); | ||
| $this->assertSame('keep', $database->getDocument(__FUNCTION__, 'doc')->getAttribute('name')); | ||
| } | ||
|
|
||
| /** | ||
| * OnDuplicate::Skip and Upsert tolerate an existing attribute. Caller is | ||
| * responsible for dropping first if the spec needs to change (migration | ||
| * consults source vs destination _metadata._updatedAt to decide). | ||
| */ | ||
| public function testCreateAttrSkipUpsertTolerates(): void | ||
| { | ||
| /** @var Database $database */ | ||
| $database = $this->getDatabase(); | ||
|
|
||
| $database->createCollection(__FUNCTION__); | ||
| $database->createAttribute(__FUNCTION__, 'name', Database::VAR_STRING, 128, true); | ||
|
|
||
| $database->createDocument(__FUNCTION__, new Document([ | ||
| '$id' => 'doc', | ||
| 'name' => 'keep', | ||
| '$permissions' => [Permission::read(Role::any())], | ||
| ])); | ||
|
|
||
| $collection = __FUNCTION__; | ||
|
|
||
| // Skip: same-spec re-declare is a no-op. | ||
| $this->assertTrue($database->withOnDuplicate( | ||
| OnDuplicate::Skip, | ||
| fn () => $database->createAttribute($collection, 'name', Database::VAR_STRING, 128, true) | ||
| )); | ||
|
|
||
| // Skip: even a wider-spec re-declare is tolerated (not applied). | ||
| $this->assertTrue($database->withOnDuplicate( | ||
| OnDuplicate::Skip, | ||
| fn () => $database->createAttribute($collection, 'name', Database::VAR_STRING, 512, true) | ||
| )); | ||
|
|
||
| // Upsert: same — tolerate existing. Migration handles drop+recreate itself. | ||
| $this->assertTrue($database->withOnDuplicate( | ||
| OnDuplicate::Upsert, | ||
| fn () => $database->createAttribute($collection, 'name', Database::VAR_STRING, 512, true) | ||
| )); | ||
|
|
||
| // Metadata still reflects the ORIGINAL spec — library didn't touch it. | ||
| $nameAttr = null; | ||
| foreach ($database->getCollection(__FUNCTION__)->getAttribute('attributes', []) as $attr) { | ||
| if ($attr->getAttribute('key') === 'name') { | ||
| $nameAttr = $attr; | ||
| break; | ||
| } | ||
| } | ||
| $this->assertNotNull($nameAttr); | ||
| $this->assertSame(128, (int) $nameAttr->getAttribute('size')); | ||
| $this->assertSame('keep', $database->getDocument(__FUNCTION__, 'doc')->getAttribute('name')); | ||
| } | ||
|
|
||
| /** | ||
| * OnDuplicate::Skip and Upsert tolerate an existing index. End state is | ||
| * always the first-declared spec; callers that need a different spec | ||
| * deleteIndex() first. | ||
| */ | ||
| public function testCreateIdxSkipUpsertTolerates(): void | ||
| { | ||
| /** @var Database $database */ | ||
| $database = $this->getDatabase(); | ||
|
|
||
| $database->createCollection(__FUNCTION__); | ||
| $database->createAttribute(__FUNCTION__, 'a', Database::VAR_STRING, 64, true); | ||
| $database->createAttribute(__FUNCTION__, 'b', Database::VAR_STRING, 64, true); | ||
| $database->createIndex(__FUNCTION__, 'idx', Database::INDEX_KEY, ['a']); | ||
|
|
||
| $collection = __FUNCTION__; | ||
|
|
||
| $this->assertTrue($database->withOnDuplicate( | ||
| OnDuplicate::Skip, | ||
| fn () => $database->createIndex($collection, 'idx', Database::INDEX_KEY, ['b']) | ||
| )); | ||
|
|
||
| $this->assertTrue($database->withOnDuplicate( | ||
| OnDuplicate::Upsert, | ||
| fn () => $database->createIndex($collection, 'idx', Database::INDEX_KEY, ['b']) | ||
| )); | ||
|
|
||
| // Metadata still reflects the original column — library tolerates, | ||
| // caller must drop first to change spec. | ||
| $indexes = $database->getCollection(__FUNCTION__)->getAttribute('indexes', []); | ||
| $this->assertCount(1, $indexes); | ||
| $this->assertSame(['a'], $indexes[0]->getAttribute('attributes')); | ||
| } | ||
| } |
There was a problem hiding this comment.
getSupportForUpserts() guard on new upsert tests
testCreateDocsUpsertOverwrites and testCreateDocsUpsertAll are the only new OnDuplicate::Upsert tests that do not begin with:
if (!$this->getDatabase()->getAdapter()->getSupportForUpserts()) {
$this->expectNotToPerformAssertions();
return;
}Every existing upsert test in this file (e.g. testUpsertDocumentsNoop, testUpsertDuplicateIds, testUpsertMixedPermissionDelta) guards against SQLite (getSupportForUpserts() === false) before executing. SQLite emits INSERT OR REPLACE INTO, which is full delete-then-reinsert semantics rather than a true in-place update, and the adapter itself still declares getSupportForUpserts(): false. Running these two tests against SQLite will either fail, produce different row counts, or silently assert incorrect data.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (6)
src/Database/Adapter/MariaDB.php (1)
2387-2408:⚠️ Potential issue | 🟠 MajorSkip list still diverges from
getUpsertStatement()andPostgres::getInsertSuffix().The batch Upsert path continues to emit
_createdAt = VALUES(_createdAt)and_tenant = VALUES(_tenant)because only`_uid`and`_id`are skipped.getUpsertStatement()in this same file (lines 1283-1289) already excludes_uid,_id,_createdAt,_tenant— users relying on_createdAtbeing immutable across upserts will see it silently overwritten on the batch path. Please align the skip list.🛡️ Proposed fix
$assignments = []; foreach ($columns as $col) { - // Skip the primary unique key (_uid) — no need to "update" what matched. - // Safe to skip _id too since it's auto-increment in INSERT and untouched on update. - if (\in_array($col, ['`_uid`', '`_id`'], true)) { + // Skip the conflict key (_uid), surrogate PK (_id), tenant + // discriminator (_tenant), and _createdAt — the original creation + // timestamp must be preserved on an upsert collision. + if (\in_array($col, ['`_uid`', '`_id`', '`_tenant`', '`_createdAt`'], true)) { continue; } $assignments[] = "{$col} = VALUES({$col})"; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Database/Adapter/MariaDB.php` around lines 2387 - 2408, getInsertSuffix currently only skips '`_uid`' and '`_id`', causing `_createdAt` and `_tenant` to be updated during batch upserts; update the skip list in MariaDB::getInsertSuffix to also exclude '`_createdAt`' and '`_tenant`' so it matches getUpsertStatement() and Postgres::getInsertSuffix(), i.e., add those column names to the in_array check (or otherwise extend the conditional that continues the loop) in the getInsertSuffix method to prevent emitting assignments for those columns.src/Database/Adapter/SQLite.php (1)
1942-1967:⚠️ Potential issue | 🟠 Major
INSERT OR REPLACEUpsert still performs a delete-then-insert, mutating_idand_createdAt.
OnDuplicate::Upsert → 'INSERT OR REPLACE INTO'retains the REPLACE conflict algorithm: on_uidcollision SQLite deletes the existing row and inserts a new one. Side effects that diverge from MariaDB/Postgres Upsert semantics in this same PR:
- The surrogate PK
_id(AUTOINCREMENT rowid) gets a new value — external callers holding the prior$sequenceare silently stale._createdAtis lost (contradicts the Postgres/MariaDB path that intentionally preserves it).- DELETE triggers fire instead of UPDATE; no UPDATE hooks.
Since the project targets SQLite ≥ 3.24 (UPSERT available), switching Upsert to plain
INSERT INTO+ anON CONFLICT ... DO UPDATE SET ...suffix (mirroring Postgres) would give true in-place update semantics. This is the same concern raised previously and still applies to the current code.SQLite INSERT OR REPLACE vs ON CONFLICT DO UPDATE rowid preservation🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Database/Adapter/SQLite.php` around lines 1942 - 1967, The current SQLite adapter maps OnDuplicate::Upsert to "INSERT OR REPLACE INTO", which deletes-and-inserts (changing _id and _createdAt and firing DELETE triggers); update getInsertKeyword and getInsertSuffix so that OnDuplicate::Upsert uses a plain "INSERT INTO" plus an "ON CONFLICT (...) DO UPDATE SET ..." suffix that mirrors Postgres/MariaDB upsert semantics (preserve _id and _createdAt and perform an in-place UPDATE); specifically change the match in getInsertKeyword to return "INSERT INTO" for OnDuplicate::Upsert and implement getInsertSuffix to generate the ON CONFLICT clause (targeting the unique key such as _uid) that updates only non-key columns while preserving _id and _createdAt, ensuring this runs only when SQLite >= 3.24 and referencing the onDuplicate property and OnDuplicate::Upsert in your changes.src/Database/Adapter/Postgres.php (1)
2381-2385:⚠️ Potential issue | 🟠 MajorSkip still limited to
_uidconflicts — diverges from MariaDBINSERT IGNORE.For
OnDuplicate::Skip, the suffix still emits an explicitON CONFLICT ("_uid"[, "_tenant"]) DO NOTHING. Conflicts on_id(PK) or any user-defined unique index will still raise23505/DuplicateException, whereas MariaDB'sINSERT IGNOREswallows all unique violations. Cross-adapter Skip semantics are inconsistent.Prefer a bare
ON CONFLICT DO NOTHINGfor Skip, and keep the explicit target only for Upsert (which requires a target forDO UPDATE).- $conflictTarget = $this->sharedTables ? '("_uid", "_tenant")' : '("_uid")'; - if ($this->onDuplicate === OnDuplicate::Skip) { - return "ON CONFLICT {$conflictTarget} DO NOTHING"; + return "ON CONFLICT DO NOTHING"; } + + $conflictTarget = $this->sharedTables ? '("_uid", "_tenant")' : '("_uid")';🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Database/Adapter/Postgres.php` around lines 2381 - 2385, The current Postgres adapter emits a targeted ON CONFLICT ("_uid"[, "_tenant"]) DO NOTHING for OnDuplicate::Skip which still allows other unique violations to raise errors; change the logic in Postgres.php so that when $this->onDuplicate === OnDuplicate::Skip you return a bare "ON CONFLICT DO NOTHING" (no $conflictTarget), and reserve the explicit conflict target ($conflictTarget) only for the Upsert/DO UPDATE path (where a target is required). Update the branch that checks OnDuplicate::Skip to return the untargeted clause and leave the existing $conflictTarget usage for the DO UPDATE case.tests/e2e/Adapter/Scopes/DocumentTests.php (1)
8270-8293:⚠️ Potential issue | 🟠 MajorGuard the row-Upsert scope tests for unsupported adapters.
These two tests exercise row-level
OnDuplicate::Upsert, so they should follow the existing upsert-test pattern and return early whengetSupportForUpserts()is false; otherwise the full adapter matrix can fail on adapters that do not support upserts.🧪 Proposed guard
public function testCreateDocsUpsertOverwrites(): void { /** `@var` Database $database */ $database = $this->getDatabase(); + if (!$database->getAdapter()->getSupportForUpserts()) { + $this->expectNotToPerformAssertions(); + return; + } + $database->createCollection(__FUNCTION__); $database->createAttribute(__FUNCTION__, 'name', Database::VAR_STRING, 128, true); $database->createAttribute(__FUNCTION__, 'tag', Database::VAR_STRING, 128, false); @@ public function testCreateDocsUpsertAll(): void { /** `@var` Database $database */ $database = $this->getDatabase(); + if (!$database->getAdapter()->getSupportForUpserts()) { + $this->expectNotToPerformAssertions(); + return; + } + $database->createCollection(__FUNCTION__); $database->createAttribute(__FUNCTION__, 'name', Database::VAR_STRING, 128, true);Also applies to: 8314-8334
🤖 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 8270 - 8293, Guard the row-level upsert tests by returning early when the adapter doesn't support upserts: in testCreateDocsUpsertOverwrites (and the sibling upsert test), call $this->getSupportForUpserts() at the start and if it returns false do an early return so the test is skipped for adapters without upsert support; mirror the existing upsert-test pattern used elsewhere and place the check before creating the collection/attributes or calling withOnDuplicate/OnDuplicate::Upsert.src/Database/Adapter/Mongo.php (2)
1514-1531:⚠️ Potential issue | 🟠 MajorKeep
_idout of Mongo update operators.When a document has a sequence, Line 1522 copies
_idinto$payload; inUpsertmode this becomes'$set' => ['_id' => ...], which can fail because MongoDB treats_idas immutable. Put_idin the equality filter for insert identity and remove it from the operator payload.🐛 Proposed fix
$operations = []; foreach ($records as $record) { $filter = ['_uid' => $record['_uid'] ?? '']; + if (\array_key_exists('_id', $record)) { + $filter['_id'] = $record['_id']; + } if ($this->sharedTables) { $filter['_tenant'] = $record['_tenant'] ?? $this->getTenant(); } // Filter fields can't reappear in $setOnInsert/$set (mongo path-conflict error). $payload = $record; - unset($payload['_uid'], $payload['_tenant']); + unset($payload['_uid'], $payload['_tenant'], $payload['_id']);Verification references: MongoDB documents
_idas immutable in update flows, and its$setOnInsertupsert docs show_idcan be derived from the query filter:
- https://www.mongodb.com/docs/v6.2/tutorial/update-documents/
- https://www.mongodb.com/docs/manual/reference/operator/update/setoninsert/
MongoDB update operator _id immutable upsert $set $setOnInsert official documentation🤖 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 1514 - 1531, The payload construction in the upsert loop (where $operations is built) can copy _id into the update operator, causing Mongo errors; modify the logic around $payload in the foreach (the block handling $record and $payload) to remove any '_id' from $payload (e.g., unset $payload['_id']) and, if $record['_id'] exists, ensure it's used as part of the equality filter (add to $filter) so the insert identity comes from the query rather than the update document; keep the existing removal of '_uid' and '_tenant' but also strip '_id' before pushing the ['filter'=>..., 'update'=>[$operator=>$payload]] entry.
449-461:⚠️ Potential issue | 🟠 MajorPreserve
Failsemantics for physical collection duplicates.Line 451 still returns
truefor every processedDuplicateException, so a normal Mongo code-48 collection collision is silently tolerated even when$tolerateExistingis false.🐛 Proposed fix
if ($e instanceof DuplicateException) { // Keep existing shared-tables/metadata behavior — no-op there. - return true; + if ($tolerateExisting) { + return true; + } + + throw $e; }🤖 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 449 - 461, The handler in src/Database/Adapter/Mongo.php currently returns true for any DuplicateException (losing Fail semantics); change the DuplicateException handling so it only no-ops when $tolerateExisting is true and otherwise re-throws the DuplicateException (preserving the existing shared-tables/metadata no-op behavior). Specifically, in the catch that checks "if ($e instanceof DuplicateException)" use the $tolerateExisting flag to decide between "return true" and re-throwing $e so normal Mongo code-48 collisions propagate and Database::createCollection() can perform orphan reconciliation.
🧹 Nitpick comments (1)
src/Database/Mirror.php (1)
206-265: Optional: extract the$forwardclosure into a private helper.The same 4-line closure is redefined verbatim in
createCollection(211-214),createAttribute(320-323), andcreateIndex(508-511), andcreateDocumentsopen-codes the same branch inline (640-645, 679-695). A single helper would de-duplicate and make the semantics (“Fail short-circuits, Skip/Upsert forwards the scope”) easier to evolve.♻️ Suggested helper
/** * Forward the current OnDuplicate scope to a target database, unless the * mode is Fail (in which case the call is invoked directly). */ private function forwardOnDuplicate(Database $target, callable $call): mixed { return $this->onDuplicate !== OnDuplicate::Fail ? $target->withOnDuplicate($this->onDuplicate, $call) : $call(); }Then each call site becomes
$this->forwardOnDuplicate($this->source, fn () => ...), and the source/destination branches increateDocumentscollapse to a single path.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Database/Mirror.php` around lines 206 - 265, Extract the repeated $forward closure into a private helper named forwardOnDuplicate(Database $target, callable $call): mixed and replace the inline closure usages in createCollection, createAttribute, createIndex and the source/destination branches inside createDocuments with calls to $this->forwardOnDuplicate(...). The helper should implement the same logic: if $this->onDuplicate !== OnDuplicate::Fail call $target->withOnDuplicate($this->onDuplicate, $call) else invoke $call() directly, and ensure each call site uses the same callable wrapping the original create* invocation so behavior is unchanged.
🤖 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/Adapter/Postgres.php`:
- Around line 2387-2401: getInsertSuffix() currently allows _createdAt and
_permissions to be updated on conflict, which conflicts with updateDocument()
and the permissions table behavior; modify getInsertSuffix() (the assignment
loop that currently excludes ['"_uid"','"_id"','"_tenant"']) to also exclude
'"_createdAt"' and '"_permissions"' so those columns are not included in the DO
UPDATE SET list, and leave getInsertPermissionsSuffix() as DO NOTHING so both
main-table upserts and the permissions-table upserts treat creation timestamp
and permissions as immutable on conflict.
In `@src/Database/Adapter/SQL.php`:
- Around line 1047-1058: The current permission-side-table logic relies on
INSERT IGNORE (see getInsertPermissionsKeyword and use of _perms/_permissions),
which lets skipped duplicates or upserts leave _perms out of sync and over-grant
access; change the permission insert flow so that for OnDuplicate::Skip you only
insert _perms for rows that were actually inserted (i.e., derive permission
inserts from the INSERT ... SELECT of newly inserted IDs or use the DB's
inserted-rows output rather than blind INSERT IGNORE), and for
OnDuplicate::Upsert you either fully sync/replace _perms for the affected
_document IDs in the same operation (DELETE/REPLACE or transactional DELETE then
INSERT for those document IDs) or preserve existing _perms when the _permissions
JSON remains unchanged; stop relying on INSERT IGNORE in
getInsertPermissionsKeyword for these modes and implement explicit conditional
logic to insert/sync permissions based on the actual insert/upsert outcome.
In `@src/Database/Database.php`:
- Around line 847-866: withOnDuplicate sets $this->onDuplicate and mirrors it to
the adapter but createAttributes() currently ignores that scope and rethrows
metadata-duplicate errors; update createAttributes() to respect the scoped mode
by using $this->onDuplicate (or wrapping adapter calls in
adapter->withOnDuplicate($this->onDuplicate, ...)) when creating each attribute
so duplicates are handled according to the mode (Skip/Upsert) and restore
previous mode semantics already provided by withOnDuplicate; ensure you
reference createAttributes(), createAttribute()/adapter->createAttribute(),
onDuplicate and adapter->withOnDuplicate() when making the change.
- Around line 5770-5772: The current createDocuments() branch lets
OnDuplicate::Upsert reach the adapter via
$this->adapter->withOnDuplicate($this->onDuplicate, $insert) which only checks
create permission and can update existing rows; change this so Upsert paths are
handled by the existing upsertDocuments() flow (or, if you must keep native
adapter upsert, perform the same pre-read + per-document update/security
authorization that upsertDocuments() does before calling
$this->adapter->withOnDuplicate). Specifically, detect when $this->onDuplicate
=== OnDuplicate::Upsert inside createDocuments(), and forward to
upsertDocuments() (or replicate its authorization/read steps) instead of
directly invoking $insert() or withOnDuplicate.
---
Duplicate comments:
In `@src/Database/Adapter/MariaDB.php`:
- Around line 2387-2408: getInsertSuffix currently only skips '`_uid`' and
'`_id`', causing `_createdAt` and `_tenant` to be updated during batch upserts;
update the skip list in MariaDB::getInsertSuffix to also exclude '`_createdAt`'
and '`_tenant`' so it matches getUpsertStatement() and
Postgres::getInsertSuffix(), i.e., add those column names to the in_array check
(or otherwise extend the conditional that continues the loop) in the
getInsertSuffix method to prevent emitting assignments for those columns.
In `@src/Database/Adapter/Mongo.php`:
- Around line 1514-1531: The payload construction in the upsert loop (where
$operations is built) can copy _id into the update operator, causing Mongo
errors; modify the logic around $payload in the foreach (the block handling
$record and $payload) to remove any '_id' from $payload (e.g., unset
$payload['_id']) and, if $record['_id'] exists, ensure it's used as part of the
equality filter (add to $filter) so the insert identity comes from the query
rather than the update document; keep the existing removal of '_uid' and
'_tenant' but also strip '_id' before pushing the ['filter'=>...,
'update'=>[$operator=>$payload]] entry.
- Around line 449-461: The handler in src/Database/Adapter/Mongo.php currently
returns true for any DuplicateException (losing Fail semantics); change the
DuplicateException handling so it only no-ops when $tolerateExisting is true and
otherwise re-throws the DuplicateException (preserving the existing
shared-tables/metadata no-op behavior). Specifically, in the catch that checks
"if ($e instanceof DuplicateException)" use the $tolerateExisting flag to decide
between "return true" and re-throwing $e so normal Mongo code-48 collisions
propagate and Database::createCollection() can perform orphan reconciliation.
In `@src/Database/Adapter/Postgres.php`:
- Around line 2381-2385: The current Postgres adapter emits a targeted ON
CONFLICT ("_uid"[, "_tenant"]) DO NOTHING for OnDuplicate::Skip which still
allows other unique violations to raise errors; change the logic in Postgres.php
so that when $this->onDuplicate === OnDuplicate::Skip you return a bare "ON
CONFLICT DO NOTHING" (no $conflictTarget), and reserve the explicit conflict
target ($conflictTarget) only for the Upsert/DO UPDATE path (where a target is
required). Update the branch that checks OnDuplicate::Skip to return the
untargeted clause and leave the existing $conflictTarget usage for the DO UPDATE
case.
In `@src/Database/Adapter/SQLite.php`:
- Around line 1942-1967: The current SQLite adapter maps OnDuplicate::Upsert to
"INSERT OR REPLACE INTO", which deletes-and-inserts (changing _id and _createdAt
and firing DELETE triggers); update getInsertKeyword and getInsertSuffix so that
OnDuplicate::Upsert uses a plain "INSERT INTO" plus an "ON CONFLICT (...) DO
UPDATE SET ..." suffix that mirrors Postgres/MariaDB upsert semantics (preserve
_id and _createdAt and perform an in-place UPDATE); specifically change the
match in getInsertKeyword to return "INSERT INTO" for OnDuplicate::Upsert and
implement getInsertSuffix to generate the ON CONFLICT clause (targeting the
unique key such as _uid) that updates only non-key columns while preserving _id
and _createdAt, ensuring this runs only when SQLite >= 3.24 and referencing the
onDuplicate property and OnDuplicate::Upsert in your changes.
In `@tests/e2e/Adapter/Scopes/DocumentTests.php`:
- Around line 8270-8293: Guard the row-level upsert tests by returning early
when the adapter doesn't support upserts: in testCreateDocsUpsertOverwrites (and
the sibling upsert test), call $this->getSupportForUpserts() at the start and if
it returns false do an early return so the test is skipped for adapters without
upsert support; mirror the existing upsert-test pattern used elsewhere and place
the check before creating the collection/attributes or calling
withOnDuplicate/OnDuplicate::Upsert.
---
Nitpick comments:
In `@src/Database/Mirror.php`:
- Around line 206-265: Extract the repeated $forward closure into a private
helper named forwardOnDuplicate(Database $target, callable $call): mixed and
replace the inline closure usages in createCollection, createAttribute,
createIndex and the source/destination branches inside createDocuments with
calls to $this->forwardOnDuplicate(...). The helper should implement the same
logic: if $this->onDuplicate !== OnDuplicate::Fail call
$target->withOnDuplicate($this->onDuplicate, $call) else invoke $call()
directly, and ensure each call site uses the same callable wrapping the original
create* invocation so behavior is unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 0d58bfd7-a373-44bb-be23-c034ea55f108
📒 Files selected for processing (8)
src/Database/Adapter/MariaDB.phpsrc/Database/Adapter/Mongo.phpsrc/Database/Adapter/Postgres.phpsrc/Database/Adapter/SQL.phpsrc/Database/Adapter/SQLite.phpsrc/Database/Database.phpsrc/Database/Mirror.phptests/e2e/Adapter/Scopes/DocumentTests.php
Comment on lines
+2387
to
2401
| // Upsert: DO UPDATE SET col = EXCLUDED.col for every column except the conflict key. | ||
| $assignments = []; | ||
| foreach ($columns as $col) { | ||
| if (\in_array($col, ['"_uid"', '"_id"', '"_tenant"'], true)) { | ||
| continue; | ||
| } | ||
| $assignments[] = "{$col} = EXCLUDED.{$col}"; | ||
| } | ||
|
|
||
| if (empty($assignments)) { | ||
| return "ON CONFLICT {$conflictTarget} DO NOTHING"; | ||
| } | ||
|
|
||
| return "ON CONFLICT {$conflictTarget} DO UPDATE SET " . \implode(', ', $assignments); | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm how SQL::createDocuments passes the columns list into getInsertSuffix
# (expecting quoted identifiers like `"_uid"` to match the exclusion set),
# and check how other adapters (MariaDB) exclude _createdAt on Upsert.
rg -nP -C5 'getInsertSuffix\s*\(' --type=php
rg -nP -C3 "'_createdAt'|\"_createdAt\"" src/Database/Adapter --type=phpRepository: utopia-php/database
Length of output: 19136
🏁 Script executed:
# Extract the full getInsertSuffix() implementation in Postgres.php
sed -n '2375,2410p' src/Database/Adapter/Postgres.php
# Check MariaDB's implementation for comparison
sed -n '2387,2420p' src/Database/Adapter/MariaDB.php
# Search for getInsertPermissionsSuffix() to understand permissions handling
rg -nP 'getInsertPermissionsSuffix' --type=php -A5Repository: utopia-php/database
Length of output: 3171
🏁 Script executed:
# Get full getInsertPermissionsSuffix() implementations
sed -n '2403,2430p' src/Database/Adapter/Postgres.php
echo "---"
rg -nP 'getInsertPermissionsSuffix' src/Database/Adapter/MariaDB.php -A15
# Check how columns list is built in createDocuments (search for where $columnList is created)
sed -n '2550,2600p' src/Database/Adapter/SQL.php | head -60
# Verify what columns are included in the upsert
rg -nP '\$columnList' src/Database/Adapter/SQL.php -B2 -A2Repository: utopia-php/database
Length of output: 3518
Upsert incorrectly updates _createdAt — conflicts with updateDocument() semantics.
In getInsertSuffix(), the assignment loop excludes only "_uid", "_id", "_tenant", but not "_createdAt". This means on a conflict, the incoming document's _createdAt will overwrite the original creation timestamp. However, in updateDocument() at line 1428, the code explicitly excludes _createdAt from updates, confirming this column should be immutable on both paths.
Additionally, the _permissions column will be updated via DO UPDATE SET in the main table, but getInsertPermissionsSuffix() uses DO NOTHING for the permissions table, causing the two to diverge on upsert.
🛠 Proposed fix
- // Upsert: DO UPDATE SET col = EXCLUDED.col for every column except the conflict key.
+ // Upsert: DO UPDATE SET col = EXCLUDED.col for every column except the
+ // conflict/identity/immutable columns.
$assignments = [];
foreach ($columns as $col) {
- if (\in_array($col, ['"_uid"', '"_id"', '"_tenant"'], true)) {
+ if (\in_array($col, ['"_uid"', '"_id"', '"_tenant"', '"_createdAt"'], true)) {
continue;
}
$assignments[] = "{$col} = EXCLUDED.{$col}";
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/Database/Adapter/Postgres.php` around lines 2387 - 2401,
getInsertSuffix() currently allows _createdAt and _permissions to be updated on
conflict, which conflicts with updateDocument() and the permissions table
behavior; modify getInsertSuffix() (the assignment loop that currently excludes
['"_uid"','"_id"','"_tenant"']) to also exclude '"_createdAt"' and
'"_permissions"' so those columns are not included in the DO UPDATE SET list,
and leave getInsertPermissionsSuffix() as DO NOTHING so both main-table upserts
and the permissions-table upserts treat creation timestamp and permissions as
immutable on conflict.
Comment on lines
+1047
to
+1058
| /** | ||
| * Returns the INSERT keyword for the `_perms` side-table. Permissions have | ||
| * their own composite unique constraint (_document, _type, _permission), | ||
| * so on row Upsert we don't want to ON-DUPLICATE-KEY-UPDATE them — they're | ||
| * already there. Both Skip and Upsert modes should just silently ignore | ||
| * pre-existing permission rows. | ||
| */ | ||
| protected function getInsertPermissionsKeyword(): string | ||
| { | ||
| return $this->onDuplicate === OnDuplicate::Fail | ||
| ? 'INSERT INTO' | ||
| : 'INSERT IGNORE INTO'; |
Contributor
There was a problem hiding this comment.
Keep _perms synchronized with skipped/upserted rows.
Line 2644 still inserts permission rows for every input document. INSERT IGNORE only ignores identical triples, so a skipped duplicate can still gain new _perms grants, and an upsert can replace _permissions JSON while leaving old grants in _perms. Since reads authorize through _perms, this can over-grant access.
For Skip, only insert permissions for rows that were actually inserted. For Upsert, either replace/sync _perms for affected document IDs in the same operation, or preserve existing permissions consistently in both the row and side table.
Also applies to: 2639-2646
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/Database/Adapter/SQL.php` around lines 1047 - 1058, The current
permission-side-table logic relies on INSERT IGNORE (see
getInsertPermissionsKeyword and use of _perms/_permissions), which lets skipped
duplicates or upserts leave _perms out of sync and over-grant access; change the
permission insert flow so that for OnDuplicate::Skip you only insert _perms for
rows that were actually inserted (i.e., derive permission inserts from the
INSERT ... SELECT of newly inserted IDs or use the DB's inserted-rows output
rather than blind INSERT IGNORE), and for OnDuplicate::Upsert you either fully
sync/replace _perms for the affected _document IDs in the same operation
(DELETE/REPLACE or transactional DELETE then INSERT for those document IDs) or
preserve existing _perms when the _permissions JSON remains unchanged; stop
relying on INSERT IGNORE in getInsertPermissionsKeyword for these modes and
implement explicit conditional logic to insert/sync permissions based on the
actual insert/upsert outcome.
Comment on lines
+847
to
+866
| /** | ||
| * Run $callback within a scope where create-style operations apply the | ||
| * given OnDuplicate mode. Nestable — previous mode is restored on return. | ||
| * | ||
| * @template T | ||
| * @param callable(): T $callback | ||
| * @return T | ||
| */ | ||
| public function withOnDuplicate(OnDuplicate $mode, callable $callback): mixed | ||
| { | ||
| $previous = $this->skipDuplicates; | ||
| $this->skipDuplicates = true; | ||
| $previous = $this->onDuplicate; | ||
| $this->onDuplicate = $mode; | ||
|
|
||
| try { | ||
| return $callback(); | ||
| // Mirror the mode onto the adapter so schema-level operations | ||
| // (createAttribute / createIndex / createCollection) that run | ||
| // directly against the adapter can observe it. createDocuments | ||
| // still goes through its own adapter->withOnDuplicate dispatch, | ||
| // which is nestable and idempotent with this outer scope. | ||
| return $this->adapter->withOnDuplicate($mode, $callback); |
Contributor
There was a problem hiding this comment.
Apply the duplicate scope to createAttributes() too.
withOnDuplicate() now scopes create-style operations, but public createAttributes() still rethrows metadata duplicates and ignores $this->onDuplicate. A batch schema reconciliation using OnDuplicate::Skip/Upsert can still fail if any attribute already exists.
Suggested direction
foreach ($attributes as $attribute) {
if (!isset($attribute['$id'])) {
throw new DatabaseException('Missing attribute key');
}
@@
if (!isset($attribute['filters'])) {
$attribute['filters'] = [];
}
+
+ if ($this->onDuplicate !== OnDuplicate::Fail) {
+ foreach ($collection->getAttribute('attributes', []) as $existing) {
+ if (\strtolower($existing->getAttribute('key', $existing->getId())) === \strtolower($attribute['$id'])) {
+ continue 2;
+ }
+ }
+ }
$existsInSchema = false;
@@
}
+
+if (empty($attributeDocuments)) {
+ return true;
+}📝 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
| /** | |
| * Run $callback within a scope where create-style operations apply the | |
| * given OnDuplicate mode. Nestable — previous mode is restored on return. | |
| * | |
| * @template T | |
| * @param callable(): T $callback | |
| * @return T | |
| */ | |
| public function withOnDuplicate(OnDuplicate $mode, callable $callback): mixed | |
| { | |
| $previous = $this->skipDuplicates; | |
| $this->skipDuplicates = true; | |
| $previous = $this->onDuplicate; | |
| $this->onDuplicate = $mode; | |
| try { | |
| return $callback(); | |
| // Mirror the mode onto the adapter so schema-level operations | |
| // (createAttribute / createIndex / createCollection) that run | |
| // directly against the adapter can observe it. createDocuments | |
| // still goes through its own adapter->withOnDuplicate dispatch, | |
| // which is nestable and idempotent with this outer scope. | |
| return $this->adapter->withOnDuplicate($mode, $callback); | |
| foreach ($attributes as $attribute) { | |
| if (!isset($attribute['$id'])) { | |
| throw new DatabaseException('Missing attribute key'); | |
| } | |
| if (!isset($attribute['filters'])) { | |
| $attribute['filters'] = []; | |
| } | |
| if ($this->onDuplicate !== OnDuplicate::Fail) { | |
| foreach ($collection->getAttribute('attributes', []) as $existing) { | |
| if (\strtolower($existing->getAttribute('key', $existing->getId())) === \strtolower($attribute['$id'])) { | |
| continue 2; | |
| } | |
| } | |
| } | |
| $existsInSchema = false; | |
| } | |
| if (empty($attributeDocuments)) { | |
| return true; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/Database/Database.php` around lines 847 - 866, withOnDuplicate sets
$this->onDuplicate and mirrors it to the adapter but createAttributes()
currently ignores that scope and rethrows metadata-duplicate errors; update
createAttributes() to respect the scoped mode by using $this->onDuplicate (or
wrapping adapter calls in adapter->withOnDuplicate($this->onDuplicate, ...))
when creating each attribute so duplicates are handled according to the mode
(Skip/Upsert) and restore previous mode semantics already provided by
withOnDuplicate; ensure you reference createAttributes(),
createAttribute()/adapter->createAttribute(), onDuplicate and
adapter->withOnDuplicate() when making the change.
Comment on lines
+5770
to
5772
| $batch = $this->onDuplicate !== OnDuplicate::Fail | ||
| ? $this->adapter->withOnDuplicate($this->onDuplicate, $insert) | ||
| : $insert(); |
Contributor
There was a problem hiding this comment.
Do not let createDocuments() Upsert bypass update authorization.
OnDuplicate::Upsert can update existing rows here, but this path only checks create permission and never verifies per-document update/document-security permissions. Route Upsert mode through the existing upsertDocuments() flow, or add equivalent pre-read authorization before native adapter upserts.
Safer starting point
public function createDocuments(
string $collection,
array $documents,
int $batchSize = self::INSERT_BATCH_SIZE,
?callable $onNext = null,
?callable $onError = null,
): int {
@@
if (empty($documents)) {
return 0;
}
+
+ if ($this->onDuplicate === OnDuplicate::Upsert) {
+ return $this->upsertDocuments(
+ $collection,
+ $documents,
+ $batchSize,
+ $onNext === null
+ ? null
+ : fn (Document $doc, ?Document $_old = null) => $onNext($doc),
+ $onError
+ );
+ }
$batchSize = \min(Database::INSERT_BATCH_SIZE, \max(1, $batchSize));📝 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
| $batch = $this->onDuplicate !== OnDuplicate::Fail | |
| ? $this->adapter->withOnDuplicate($this->onDuplicate, $insert) | |
| : $insert(); | |
| public function createDocuments( | |
| string $collection, | |
| array $documents, | |
| int $batchSize = self::INSERT_BATCH_SIZE, | |
| ?callable $onNext = null, | |
| ?callable $onError = null, | |
| ): int { | |
| if (empty($documents)) { | |
| return 0; | |
| } | |
| if ($this->onDuplicate === OnDuplicate::Upsert) { | |
| return $this->upsertDocuments( | |
| $collection, | |
| $documents, | |
| $batchSize, | |
| $onNext === null | |
| ? null | |
| : fn (Document $doc, ?Document $_old = null) => $onNext($doc), | |
| $onError | |
| ); | |
| } | |
| $batchSize = \min(Database::INSERT_BATCH_SIZE, \max(1, $batchSize)); | |
| // ... rest of function continues unchanged ... | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/Database/Database.php` around lines 5770 - 5772, The current
createDocuments() branch lets OnDuplicate::Upsert reach the adapter via
$this->adapter->withOnDuplicate($this->onDuplicate, $insert) which only checks
create permission and can update existing rows; change this so Upsert paths are
handled by the existing upsertDocuments() flow (or, if you must keep native
adapter upsert, perform the same pre-read + per-document update/security
authorization that upsertDocuments() does before calling
$this->adapter->withOnDuplicate). Specifically, detect when $this->onDuplicate
=== OnDuplicate::Upsert inside createDocuments(), and forward to
upsertDocuments() (or replicate its authorization/read steps) instead of
directly invoking $insert() or withOnDuplicate.
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 free
to 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
Replaces the
skipDuplicates(callable)scope guard with a richerwithOnDuplicate(OnDuplicate $mode, callable)API. The newOnDuplicateenum has three cases —Fail(default),Skip(silently ignore duplicates), andUpsert(overwrite existing rows) — and every adapter translates the mode into its dialect-native idempotent operation.Motivation
On top of existing Skip support (merged in #852 and used by the migration package for re-runnable transfers), migration consumers also need Upsert semantics so a re-run can bring stale rows up-to-date rather than just skip them.
skipDuplicatesis a boolean dressed up as a method — it can't express the third case without adding a siblingupsertDuplicatesand leaving us with two near-identical parallel scopes forever. The enum cleans that up and gives us one extension point for any future mode.Adapter matrix
INSERT IGNORE INTOINSERT ... ON DUPLICATE KEY UPDATE col = VALUES(col), ...INSERT IGNORE INTOINSERT ... ON DUPLICATE KEY UPDATE col = VALUES(col), ...INSERT ... ON CONFLICT DO NOTHINGINSERT ... ON CONFLICT ... DO UPDATE SET col = EXCLUDED.colINSERT OR IGNORE INTOINSERT OR REPLACE INTObulkWritewith$setOnInsertbulkWritewith$set(bothupsert: true)Schema resources (
createCollection,createAttribute,createIndex) become idempotent under both Skip and Upsert — Upsert semantics apply to rows only; schema Upsert is too dangerous and isn't done here. Mongo, Mirror, and Pool each follow suit.Row Upsert — permissions are special
The
_permsside-table has its own composite unique key(_document, _type, _permission), so on row Upsert we don't want toON DUPLICATE KEY UPDATE— the matching permission row is already there. A newgetInsertPermissionsKeyword()hook keeps the permissions insert atINSERT IGNORE INTO(or its dialect equivalent) while the main document insert uses the full Upsert.Breaking changes
skipDuplicates()is removed. Callers that usedskipDuplicates(fn)should now callwithOnDuplicate(OnDuplicate::Skip, fn).Adapter::$skipDuplicates/Database::$skipDuplicatesproperties are replaced by$onDuplicate(anOnDuplicateenum).skipDuplicateswas only merged in 5.3.22 and the only known consumer is utopia-php/migration (which will be updated in a companion release). No major bump needed — the caller surface is tiny.Test plan
testCreateDocumentsIgnoreDuplicatesand related Skip tests green on every adapter (MariaDB, MySQL, SQLite, Postgres, MongoDB) + shared-tables + schemaless + Mirror + Pool.testCreateDocsUpsertOverwrites— seed 2 docs, upsert 1 existing + 1 new, assert overwrite + preservation + insert. Green on all 13 adapter variants.testCreateDocsUpsertAll— all-duplicate batch upsert, all rows replaced. Green on all 13 adapter variants.Summary by CodeRabbit
New Features
Improvements
Breaking Changes
Tests