diff --git a/.changeset/mysql-upsert-cross-row-identity-merge.md b/.changeset/mysql-upsert-cross-row-identity-merge.md new file mode 100644 index 0000000000..1d868ee7d2 --- /dev/null +++ b/.changeset/mysql-upsert-cross-row-identity-merge.md @@ -0,0 +1,45 @@ +--- +"@objectstack/driver-sql": minor +"@objectstack/spec": patch +--- + +fix(driver-sql): refuse — and roll back — a MySQL upsert that merges onto a row the caller never identified (#8807) + +`ON DUPLICATE KEY UPDATE` carries no conflict target, so on MySQL a merge lands on +whichever UNIQUE key the row collides with first. `#8621` closed the half where +nothing backed a caller-named target; `#8755` closed the half where a rival key +could absorb a caller-named one. This closes the residue those two left by +construction: the `conflictKeys`-less call and the `['id']` call, which compile +byte-identically and which no pre-flight can judge, because neither names anything. + +Measured on live MySQL 8.0.46, `email` and `tax_id` both `unique: true`, **no** +`conflictKeys`: seeding `{email:'d@b.com', tax_id:'T-9'}` inserted one row, and +`{email:'e@b.com', tax_id:'T-9'}` then resolved with no error — one row, the +*seeded* one, its `email` rewritten `d@b.com` to `e@b.com`, and the id the caller +was handed back present in no row at all. The identical pair on SQLite raises +`UNIQUE constraint failed: …tax_id` and leaves the seeded row untouched. + +Per the maintainer ruling on #8807 this enforces a contract principle, not a MySQL +detail: *an `upsert` must never modify a row whose identity the caller did not +supply and whose conflict key it did not name.* + +**Accept-set change, MySQL only.** After the statement and inside the same +transaction, the driver checks whether the row it landed on is the one the call +supplied. If it is not, the write is **rolled back** and the call refuses with +`code: 'VALIDATION_ERROR'`, `status: 400`, naming the UNIQUE key that absorbed the +merge and stating that nothing was changed. + +The check is exact rather than heuristic — `id` is insert-only on the merge path +(#8622), so a row merged on the primary key always still carries the supplied id +and a row merged on any other key never does — which is why it has no false +refusals. + +Deliberately unchanged: tables whose only key is the primary key are not verified +and open no transaction, so the ordinary upsert keeps its single round trip; every +insert and every re-upsert of the same row still merges; the caller-named +single-unique-key fast path is untouched; and SQLite and PostgreSQL are unaffected, +because `ON CONFLICT (...)` already honours the named arbiter. The lifecycle +archiver's hot→cold copy passes by construction — it supplies each row's own id — +and of the two objects declaring `lifecycle.archive`, neither carries a +non-primary unique field. The dialect limit is documented under +*Database Drivers → MySQL*. diff --git a/content/docs/data-modeling/drivers.mdx b/content/docs/data-modeling/drivers.mdx index 996e72c66c..8158a8d3f2 100644 --- a/content/docs/data-modeling/drivers.mdx +++ b/content/docs/data-modeling/drivers.mdx @@ -339,8 +339,8 @@ before any row is written and before any auto-number is reserved: | Call, on MySQL | Result | | :--- | :--- | -| `upsert(o, row)` — no `conflictKeys` | Merges. Not pre-flighted (see the residue below). | -| `upsert(o, row, ['id'])` — the primary key | Merges. Compiles identically to the line above. | +| `upsert(o, row)` — no `conflictKeys` | Merges on the primary key. **Refused** if the merge lands on a different row (see below). | +| `upsert(o, row, ['id'])` — the primary key | Identical to the line above — same statement, same answer. | | `upsert(o, row, ['email'])`, the table's only UNIQUE key being on `email` | Merges on `email`. **The common shape is unaffected.** | | `upsert(o, row, ['email'])`, the table also carrying `UNIQUE(tax_id)` | **Refused.** The message names `tax_id`'s index and the workarounds. | | `upsert(o, row, ['email'])`, no unique index on `email` at all | **Refused** on every dialect ([#8621](https://github.com/objectstack-ai/objectstack/issues/8621)). | @@ -353,13 +353,43 @@ Two ways out, both stated in the error message: appropriate when both keys are genuine business constraints, since one of them must otherwise be given up. +### The merge that lands on a row you never identified + +The pre-flight above covers a *caller-named* non-primary target. It cannot cover +the `conflictKeys`-less default or an explicitly named primary key, because +neither call names anything a pre-flight could check — and those two compile to +the same statement. On a table with several UNIQUE keys that statement can still +collide on a key you did not name, and MySQL will merge there: + +```text +seed upsert({ email: 'd@b.com', tax_id: 'T-9', title: 'first' }) -- no conflictKeys + -> inserted, id = iVvD35rMk4BIayYc +B upsert({ email: 'e@b.com', tax_id: 'T-9', title: 'second' }) -- no conflictKeys + -> the fresh id did not collide; `tax_id` did, so MySQL merged onto the + SEEDED row — rewriting an `email` this call never asked to touch. +``` + +So the driver checks, after the statement and inside the same transaction, +whether the row it landed on is the one the call supplied. If it is not, the +write is **rolled back** and the call is refused with `code: 'VALIDATION_ERROR'` +and `status: 400`. Nothing is left changed. + +| Call, on MySQL, table carrying a rival UNIQUE key | Result | +| :--- | :--- | +| The row is new, or matches an existing row's `id` | **Merges**, exactly as before. | +| The row collides on a UNIQUE key you did not name | **Refused**, and the write is rolled back. | + -**The residue, stated rather than hidden.** The refusal covers a *caller-named* -non-primary target. It does not cover the `conflictKeys`-less default or an -explicitly named primary key: those two compile to the same statement, and on a -table with several UNIQUE keys that statement can still merge on one you did not -name. If you need the target honoured exactly, name it — and on MySQL, keep one -UNIQUE key per table. +**This check is selective, and only MySQL pays for it.** A table whose only key +is its primary key can never exhibit the condition, so nothing is verified and +no transaction is opened there. SQLite and PostgreSQL compile +`ON CONFLICT (id)`, which honours the arbiter and raises a unique violation on +any other key — they already behave this way and are unchanged. + +If you meant to merge on a business key, **name it** (`conflictKeys`), which +makes the intent checkable. On MySQL, a table with more than one UNIQUE key +cannot have every merge honoured — keep one UNIQUE key per table, or run the +object on SQLite/PostgreSQL. ## MongoDB diff --git a/packages/drivers/driver-sql/src/sql-driver-upsert-conflict-target-dialects.test.ts b/packages/drivers/driver-sql/src/sql-driver-upsert-conflict-target-dialects.test.ts index f4311b3eb0..18bd5ea92f 100644 --- a/packages/drivers/driver-sql/src/sql-driver-upsert-conflict-target-dialects.test.ts +++ b/packages/drivers/driver-sql/src/sql-driver-upsert-conflict-target-dialects.test.ts @@ -65,9 +65,17 @@ * a second UNIQUE key on the table can absorb the conflict. That call is now * refused too, with its OWN sentence (#5240 — one condition, one wording; this * is a different condition from "no index backs your target" and every remedy it - * names is different). What is deliberately left merging, and documented rather - * than silent, is the PRIMARY-KEY-targeted call and the `conflictKeys`-less - * default — see {@link WRONG_KEY} and the residue pins below. + * names is different). + * + * ✅ **[#8755]'s residue is closed by [#8807]** — the PRIMARY-KEY-targeted call + * and the `conflictKeys`-less default, which compile byte-identically and which + * no pre-flight can judge because neither names anything. Those are now checked + * AFTER the statement and inside a transaction: if the merge landed on a row the + * caller never identified, the write is rolled back and the call refuses. What + * is deliberately left merging is the legitimate half of the same shape — every + * insert, and every merge onto the identity the caller did supply — pinned as + * this file's positive controls, which is what keeps the rule selective rather + * than a ban on merging over MySQL. * * ✅ **[#8622] has since repaired the primary-key half**, and only that half. * `id` is now insert-only on the merge path for every dialect, so the pin that @@ -131,6 +139,28 @@ const PLAIN = { }, } as any; +/** + * [#8807] Two unique business keys — the shape on which MySQL merges a + * `conflictKeys`-less upsert onto a row the caller never identified. + * + * Declared for the ON CONFLICT dialects so the card's claim is pinned as a + * DIALECT DIFFERENCE rather than as a MySQL anecdote: the ruled principle + * ("never modify a row whose identity the caller did not supply") is + * dialect-independent, and the reason SQLite and PostgreSQL needed no code + * change is that they already uphold it — `ON CONFLICT (id)` merges on the + * named arbiter alone and raises a unique violation on anything else. That is + * the control, and it is what makes the MySQL fix a convergence rather than a + * new per-dialect rule. + */ +const TWO_KEYS = { + name: 'os8807_two_keys', + fields: { + email: { type: 'string', unique: true }, + tax_id: { type: 'string', unique: true }, + title: { type: 'string' }, + }, +} as any; + const captureError = async (run: () => Promise): Promise => { try { await run(); @@ -168,12 +198,14 @@ function declareRefusalSweep(cell: DialectCell): void { // Live cells reuse one database, so the sweep starts from dropped tables. await knexInstance.schema.dropTableIfExists(BACKED.name); await knexInstance.schema.dropTableIfExists(PLAIN.name); - await driver.initObjects([BACKED, PLAIN]); + await knexInstance.schema.dropTableIfExists(TWO_KEYS.name); + await driver.initObjects([BACKED, PLAIN, TWO_KEYS]); }); afterAll(async () => { await knexInstance?.schema.dropTableIfExists(BACKED.name).catch(() => {}); await knexInstance?.schema.dropTableIfExists(PLAIN.name).catch(() => {}); + await knexInstance?.schema.dropTableIfExists(TWO_KEYS.name).catch(() => {}); await driver?.disconnect?.(); }); @@ -272,6 +304,68 @@ function declareRefusalSweep(cell: DialectCell): void { expect(rows[0].title).toBe('second'); }); + /** + * ✅ **[#8807] THE DIALECT CONTROL — the refused branch, on the dialects + * that need no code change.** + * + * The identical call sequence that merged silently onto an unnamed key on + * MySQL (pinned in the MySQL section below) must NOT modify the seeded row + * here. `ON CONFLICT (id)` names its arbiter, so a collision on `tax_id` + * cannot be absorbed by it — the server raises a unique violation and the + * stored row is left alone. + * + * This case is why #8807's fix is a convergence rather than a new MySQL + * rule: the ruled principle already held on two of three dialects, and the + * assertion here is on the STORED ROW, not on the error, because that is + * what the principle is about. (The error's shape is the server's, not this + * driver's, and pinning its text would pin the server's wording on two + * dialects for no gain.) + */ + it('[#8807] a conflictKeys-less upsert never merges onto an unnamed UNIQUE key here', async () => { + const seeded = await driver.upsert(TWO_KEYS.name, { email: 'd@b.com', tax_id: 'T-9', title: 'first' }); + + const err = await captureError(() => + driver.upsert(TWO_KEYS.name, { email: 'e@b.com', tax_id: 'T-9', title: 'second' }), + ); + + expect( + err, + 'this dialect honours the ON CONFLICT arbiter, so the rival key must raise rather than absorb', + ).not.toBeNull(); + + // Scoped by the colliding key: this suite shares one table across cases + // and one database across dialects, so an unscoped read would make the + // assertion depend on declaration order. + const after = await driver.find(TWO_KEYS.name, { where: { tax_id: 'T-9' } }); + expect(after).toHaveLength(1); + expect(after[0].id).toBe(seeded.id); + expect( + after[0].email, + 'a row the caller never identified was modified — the principle #8807 rules on is ' + + 'dialect-independent and this dialect was supposed to already uphold it', + ).toBe('d@b.com'); + expect(after[0].title).toBe('first'); + }); + + /** + * ✅ **[#8807] the still-merging branch, same dialect, same table.** The + * caller supplies the identity, so the merge lands on it — the positive + * control that keeps the case above from being satisfied by a driver that + * simply stopped merging on tables with two unique keys. + */ + it('[#8807] still merges onto the row whose identity the caller DID supply', async () => { + await driver.upsert(TWO_KEYS.name, { id: 'os8807_ctl', email: 'c@b.com', tax_id: 'T-1', title: 'first' }); + const err = await captureError(() => + driver.upsert(TWO_KEYS.name, { id: 'os8807_ctl', email: 'c@b.com', tax_id: 'T-1', title: 'second' }), + ); + expect(err, 'the supplied-identity merge is the legitimate shape and must never be refused').toBeNull(); + + const after = await driver.find(TWO_KEYS.name, { where: { id: 'os8807_ctl' } }); + expect(after).toHaveLength(1); + expect(after[0].id).toBe('os8807_ctl'); + expect(after[0].title).toBe('second'); + }); + /** * The specificity control, and the reason the predicate reads the message * rather than the code on BOTH cells: Postgres answers `42P10` here too for @@ -700,6 +794,46 @@ const SINGLE_KEY = { }, } as any; +/** + * [#8807] The table with NO rival key at all — the primary key is the only + * thing a row can collide on. + * + * This is the control that separates #8807's post-hoc identity check from the + * blanket refusal the ruling excluded. Every pin below that asserts a refusal + * is also satisfied by a driver that simply stopped merging on MySQL; this + * fixture is where such a driver goes red. It is also the shape that must not + * pay for the check — no rival key means no cross-row merge is possible, so the + * statement keeps its single autocommitted round trip. + */ +const NO_RIVAL = { + name: 'os8807_no_rival', + fields: { + title: { type: 'string' }, + note: { type: 'string' }, + }, +} as any; + +/** + * [#8807] The same hostile shape, TENANTED — an `organization_id` field, so + * `resolveTenantField` resolves and the identity read's tenant scope is live. + * + * The identity check issues a READ, and `check:tenant-chokepoint` requires every + * read door to route through `applyTenantScope`. That is not bookkeeping here: + * the scope decides which rows the probe can see, and the probe's answer is what + * refuses or permits a write. These pins fix the decision that was made — the + * read is scoped to the tenant the row was WRITTEN under, not to the caller's + * active org — and the third of them is the one that distinguishes the two. + */ +const TENANTED = { + name: 'os8807_tenanted', + fields: { + organization_id: { type: 'string' }, + email: { type: 'string', unique: true }, + tax_id: { type: 'string', unique: true }, + title: { type: 'string' }, + }, +} as any; + declareDialectCell( MYSQL_CELL, 'unbacked conflict-target refusal (pre-flight, MySQL)', @@ -722,13 +856,17 @@ function declareMysqlPreflightRefusal(cell: DialectCell): void { await knexInstance.schema.dropTableIfExists(MISMATCHED.name); await knexInstance.schema.dropTableIfExists(WRONG_KEY.name); await knexInstance.schema.dropTableIfExists(SINGLE_KEY.name); - await driver.initObjects([MISMATCHED, WRONG_KEY, SINGLE_KEY]); + await knexInstance.schema.dropTableIfExists(NO_RIVAL.name); + await knexInstance.schema.dropTableIfExists(TENANTED.name); + await driver.initObjects([MISMATCHED, WRONG_KEY, SINGLE_KEY, NO_RIVAL, TENANTED]); }); afterAll(async () => { await knexInstance?.schema.dropTableIfExists(MISMATCHED.name).catch(() => {}); await knexInstance?.schema.dropTableIfExists(WRONG_KEY.name).catch(() => {}); await knexInstance?.schema.dropTableIfExists(SINGLE_KEY.name).catch(() => {}); + await knexInstance?.schema.dropTableIfExists(NO_RIVAL.name).catch(() => {}); + await knexInstance?.schema.dropTableIfExists(TENANTED.name).catch(() => {}); await driver?.disconnect?.(); }); @@ -738,6 +876,8 @@ function declareMysqlPreflightRefusal(cell: DialectCell): void { await knexInstance(MISMATCHED.name).delete(); await knexInstance(WRONG_KEY.name).delete(); await knexInstance(SINGLE_KEY.name).delete(); + await knexInstance(NO_RIVAL.name).delete(); + await knexInstance(TENANTED.name).delete(); }); /** @@ -921,13 +1061,28 @@ function declareMysqlPreflightRefusal(cell: DialectCell): void { * implementing #8755. The assertion, its strength and its failure message * are untouched: deleting it, or weakening it to fit the refusal, would have * dropped #8622's only MySQL-cell coverage of a landed fix. + * + * ⚠️ **[#8807] moved the fixture a THIRD time — same reason, and the last + * time it can happen.** The `conflictKeys`-less wrong-key merge is now + * refused and rolled back, so this pin went red on the fixture #8755 left it + * on; that red is the fix landing, not a regression, and the honest response + * is another fixture move rather than a weakened assertion. #8622's claim is + * *a merged row keeps its own primary key when the incoming payload carries + * a different (freshly minted) one*, and that needs a merge which still + * HAPPENS. The one shape left on MySQL where a caller-supplied identity is + * absent and the merge is still legitimate is a caller-NAMED target on a + * table whose only UNIQUE key IS that target — {@link SINGLE_KEY}, the shape + * #8755's ruling protects by name. No wrong-key merge survives anywhere on + * this dialect to host it, which is precisely what #8807 changed. */ - it('KEEPS the surviving row’s primary key, even merging on that wrong key', async () => { - await driver.upsert(WRONG_KEY.name, { email: 'a@b.com', tax_id: 'T-1', title: 'first' }); - const seededId = (await rows(WRONG_KEY.name))[0].id; + it('KEEPS the merged row’s primary key when the payload carries a freshly minted one', async () => { + await driver.upsert(SINGLE_KEY.name, { email: 'm@b.com', title: 'first' }, ['email']); + const seededId = (await rows(SINGLE_KEY.name))[0].id; - await driver.upsert(WRONG_KEY.name, { email: 'other@b.com', tax_id: 'T-1', title: 'second' }); - const merged = (await rows(WRONG_KEY.name))[0]; + // No `id` in the payload, so `upsert` mints one — the losing insert's id, + // which is what #8622 stopped writing over the stored row. + await driver.upsert(SINGLE_KEY.name, { email: 'm@b.com', title: 'second' }, ['email']); + const merged = (await rows(SINGLE_KEY.name))[0]; expect( merged.id, @@ -935,12 +1090,11 @@ function declareMysqlPreflightRefusal(cell: DialectCell): void { 'relationship, audit record and external id mapping pointing at the old row now dangles', ).toBe(seededId); - // The wrong-key merge itself is UNCHANGED and still wrong: one row, and - // the columns that are not insert-only still took the losing insert's - // values. Without this half, the pin above would also pass if the merge - // had simply stopped happening. + // Without this half the pin above would also pass if the merge had simply + // stopped happening — which, on this table, would mean #8807's check had + // spread to the single-key fast path the ruling protects. expect(merged.title, 'the mergeable columns must still merge — only identity is excluded').toBe('second'); - expect(merged.email).toBe('other@b.com'); + expect(merged.email).toBe('m@b.com'); }); /** @@ -1060,19 +1214,22 @@ function declareMysqlPreflightRefusal(cell: DialectCell): void { }); /** - * [#8755] The residue this card deliberately does NOT refuse, pinned so it is - * documented behaviour rather than an accident nobody measured: an - * explicitly named PRIMARY KEY on a table that also carries UNIQUE keys. + * ✅ **[#8807]'s FIRST positive control**, and the pin #8755 wrote as its + * residue. The behaviour it asserts is unchanged; only its meaning moved. + * + * An explicitly named PRIMARY KEY on a table that also carries UNIQUE keys, + * re-upserting THE SAME row: `id`, `email` and `tax_id` all match the seeded + * row, so whichever key MySQL picks it lands on that one row — the identity + * the caller supplied. #8807's check passes it untouched, which is the whole + * claim: the accept set moves for cross-row merges, not for primary-key + * merges on tables that happen to carry a business key. * - * The reasoning is on `refuseAmbiguousConflictTarget` in `sql-driver.ts`. In - * one line: this call compiles byte-identically to the `conflictKeys`-less - * default that no pre-flight has ever probed, so refusing the explicit - * spelling while merging the implicit one would make the accept set a - * property of how the caller typed the same statement — and the only - * `conflictKeys` the platform itself issues is exactly this one (the - * lifecycle archiver's hot→cold copy). + * ⚠️ This is no longer "the residue #8755 declines to refuse". The residue + * (a merge landing on a row the caller never identified) IS refused now — + * see the #8807 pins below. What survives here is the legitimate half of the + * same shape, and it must stay green or the refusal has become a ban. */ - it('[#8755] leaves an explicitly named PRIMARY KEY merging, UNIQUE keys or not', async () => { + it('[#8755 → #8807] an explicitly named PRIMARY KEY still MERGES onto the supplied row', async () => { const err = await captureError(() => driver.upsert(WRONG_KEY.name, { id: 'os8755_pk', email: 'pk@b.com', tax_id: 'T-4', title: 'first' }, ['id']), ); @@ -1086,6 +1243,294 @@ function declareMysqlPreflightRefusal(cell: DialectCell): void { expect(after[0].title).toBe('second'); }); + // ─────────────────────────────────────────────────────────────────── + // [#8807] The merge that lands on a row the caller never identified. + // + // Ruled principle (maintainer, 2026-08-15): *an `upsert` must never modify + // a row whose identity the caller did not supply and whose conflict key it + // did not name.* Enforcement was delegated to this lane; the shape chosen + // is the post-hoc identity check, not the pre-flight refusal — see the + // reasoning on `refuseCrossRowIdentityMerge` in `sql-driver.ts`. + // + // Measured on live MySQL 8.0.46 through the same knex + `mysql2` path, on + // `origin/main` @ 716ac9bf8 BEFORE this fix: + // + // seed upsert({email:'d@b.com', tax_id:'T-9', title:'first'}) -> id=iVvD35rMk4BIayYc + // B upsert({email:'e@b.com', tax_id:'T-9', title:'second'}) -> RESOLVED + // ONE row, and it is the SEEDED one, its `email` rewritten d@b.com -> e@b.com. + // The id B was handed back (F-Fp1OGCQB-l5XRu) is in no row at all. + // + // The identical pair on SQLite raises `UNIQUE constraint failed: ….tax_id` + // and leaves the seeded row untouched — pinned as the dialect control in + // the ON CONFLICT sweep at the top of this file. + // ─────────────────────────────────────────────────────────────────── + + /** + * ① The envelope. `code` AND `status`, never a bare `rejects.toThrow()`: + * before the fix this call RESOLVED, so a bare throw assertion would have + * been green for the wrong reason on any unrelated failure (a dead + * connection, an unknown column) while the accept set had not moved. + */ + it('[#8807] REFUSES a conflictKeys-less upsert that would merge onto an unnamed UNIQUE key', async () => { + await driver.upsert(WRONG_KEY.name, { email: 'd@b.com', tax_id: 'T-9', title: 'first' }); + + const err = await captureError(() => + driver.upsert(WRONG_KEY.name, { email: 'e@b.com', tax_id: 'T-9', title: 'second' }), + ); + + expect( + err, + 'MySQL merged a conflictKeys-less upsert onto a UNIQUE key the caller never named — the ' + + 'identity check did not run (#8807)', + ).not.toBeNull(); + expect(err!.code).toBe(StandardErrorCode.enum.VALIDATION_ERROR); + expect(err!.status).toBe(400); + }); + + /** + * ② **The pin that carries the ruling.** Refusing after the fact is not + * enough — "must never MODIFY" is a claim about the stored row, so the + * wrong write has to be gone. This is what the transaction around the + * statement exists for, and a fix that reported the violation without + * rolling it back would satisfy pin ① and fail here. + */ + it('[#8807] ROLLS BACK the wrong write — the row it landed on is byte-for-byte untouched', async () => { + const seeded = await driver.upsert(WRONG_KEY.name, { email: 'd@b.com', tax_id: 'T-9', title: 'first' }); + + await captureError(() => + driver.upsert(WRONG_KEY.name, { email: 'e@b.com', tax_id: 'T-9', title: 'second' }), + ); + + const after = await rows(WRONG_KEY.name); + expect(after, 'the refused call must not have inserted a row either').toHaveLength(1); + expect(after[0].id).toBe(seeded.id); + expect( + after[0].email, + 'the seeded row survived with the OVERWRITTEN email — the refusal was reported but not ' + + 'rolled back, so the corruption this card exists to stop still happened', + ).toBe('d@b.com'); + expect(after[0].title).toBe('first'); + expect(after[0].tax_id).toBe('T-9'); + }); + + /** + * ③ The two spellings are one statement, so they get one answer. #8755 + * declined to separate them in the merging direction; this card must not + * re-introduce the split in the refusing direction, or the accept set + * becomes a property of how the caller typed the same call. + */ + it("[#8807] answers the explicit `['id']` spelling identically to the default", async () => { + await driver.upsert(WRONG_KEY.name, { email: 'p@b.com', tax_id: 'T-5', title: 'first' }, ['id']); + + const err = await captureError(() => + driver.upsert(WRONG_KEY.name, { email: 'q@b.com', tax_id: 'T-5', title: 'second' }, ['id']), + ); + + expect(err, 'naming the primary key explicitly must not buy a different accept set').not.toBeNull(); + expect(err!.code).toBe(StandardErrorCode.enum.VALIDATION_ERROR); + expect(err!.status).toBe(400); + + const after = await rows(WRONG_KEY.name); + expect(after).toHaveLength(1); + expect(after[0].email).toBe('p@b.com'); + expect(after[0].title).toBe('first'); + }); + + /** + * ④ The message is the deliverable — the defect is SILENT, so the whole + * value of the fix is a sentence an author can act on. It must name the key + * that actually absorbed the merge, say the write was undone (or an + * operator will go hunting for damage that is not there), and state the way + * out. Row values stay off it, and off `cause`: schema identifiers are what + * an operator acts on, the same payload contract both refusals above keep. + */ + it('[#8807] names the colliding key and the rollback, and leaks no row values', async () => { + await driver.upsert(WRONG_KEY.name, { email: 'seed@b.com', tax_id: 'T-8', title: 'first' }); + + const err = await captureError(() => + driver.upsert(WRONG_KEY.name, { email: 'leaked@example.com', tax_id: 'T-8', title: 'secret-title' }), + ); + + expect(err!.message).toContain('uniq_os8621_wrong_key_tax_id'); + expect(err!.message).toContain('tax_id'); + expect(err!.message).toContain(WRONG_KEY.name); + expect(err!.message).toMatch(/ON DUPLICATE KEY UPDATE/); + // The fact an operator needs first: nothing was changed. + expect(err!.message).toMatch(/rolled back/i); + // The three ways out, named rather than alluded to. + expect(err!.message).toMatch(/conflictKeys/); + expect(err!.message).toMatch(/drop(ping)? or renam/i); + expect(err!.message).toMatch(/SQLite and PostgreSQL/); + + expect(err!.message).not.toContain('leaked@example.com'); + expect(err!.message).not.toContain('secret-title'); + expect(err!.message).not.toMatch(/insert into/i); + + const causeText = String((err!.cause as Error | undefined)?.message); + expect(causeText).toContain('uniq_os8621_wrong_key_tax_id'); + expect(causeText).not.toContain('leaked@example.com'); + expect(causeText).not.toContain('secret-title'); + }); + + /** + * ✅ **[#8807]'s SECOND positive control, and the sharpest one.** A table + * whose only key is the primary key: the `conflictKeys`-less default must + * merge there exactly as it always has, with no verification owed and no + * transaction opened. + * + * Without this case every refusal pin above is equally satisfied by a driver + * that stopped merging `conflictKeys`-less upserts on MySQL altogether — + * which is the blanket option the ruling excluded by name, because it would + * refuse the platform's own lifecycle archiver. Selectivity is the reason + * this enforcement was choosable at all, so it is pinned rather than argued. + */ + it('[#8807] a table with NO rival UNIQUE key merges untouched — the check is selective', async () => { + const err = await captureError(() => + driver.upsert(NO_RIVAL.name, { id: 'os8807_plain', title: 'first', note: 'a' }), + ); + expect(err, 'a table with no rival UNIQUE key has nothing to verify and must never refuse').toBeNull(); + + await driver.upsert(NO_RIVAL.name, { id: 'os8807_plain', title: 'second', note: 'b' }); + + const after = await driver.find(NO_RIVAL.name, {}); + expect(after).toHaveLength(1); + expect(after[0].id).toBe('os8807_plain'); + expect(after[0].title).toBe('second'); + }); + + /** + * ✅ **[#8807]'s THIRD positive control — the archiver's shape.** + * + * The ruling required the lifecycle archiver be handled first-party in the + * same PR. Measured: of the two objects in this repo that declare + * `lifecycle.archive` (`sys_audit_log`, `sys_metadata_audit`), ZERO declare + * a non-primary unique field, so the archiver never even reaches the check. + * But it must also survive a customer object that DOES carry one, since the + * archiver upserts arbitrary objects — so its exact call shape is pinned on + * the hostile table: `cold.upsert(object, row, ['id'])`, copying a row that + * keeps its own id, first as an insert and then idempotently re-run. + * + * This is the case that would go red if the enforcement had been the + * pre-flight refusal instead, and it is why the post-hoc check was chosen. + */ + /** + * [#8807] **Tenanted, refusing branch.** The identity read is tenant-scoped + * (`check:tenant-chokepoint` requires every read door to be), so this pin + * exists to prove the scope did not BLIND the check: the caller's own rows, + * inside the caller's own org, still refuse the cross-row merge. + */ + it('[#8807] still refuses the cross-row merge on a TENANTED object', async () => { + const seeded = await driver.upsert( + TENANTED.name, + { email: 't1@b.com', tax_id: 'T-T1', title: 'first' }, + undefined, + { tenantId: 'org_a' } as any, + ); + + const err = await captureError(() => + driver.upsert( + TENANTED.name, + { email: 't2@b.com', tax_id: 'T-T1', title: 'second' }, + undefined, + { tenantId: 'org_a' } as any, + ), + ); + + expect( + err, + 'the tenant scope blinded the identity probe — it can no longer see the row it must judge', + ).not.toBeNull(); + expect(err!.code).toBe(StandardErrorCode.enum.VALIDATION_ERROR); + expect(err!.status).toBe(400); + + const after = await driver.find(TENANTED.name, { where: { tax_id: 'T-T1' } }); + expect(after).toHaveLength(1); + expect(after[0].id).toBe(seeded.id); + expect(after[0].email, 'the wrong write was not rolled back on the tenanted path').toBe('t1@b.com'); + }); + + /** + * [#8807] **Tenanted, merging branch.** The ordinary tenanted upsert — the + * overwhelmingly common shape — must be untouched. `injectTenantOnInsert` + * stamps the caller's org on the row, so the scoped read finds it. + */ + it('[#8807] a TENANTED supplied-identity merge is untouched', async () => { + const err = await captureError(() => + driver.upsert( + TENANTED.name, + { id: 'os8807_t', email: 't3@b.com', tax_id: 'T-T3', title: 'first' }, + undefined, + { tenantId: 'org_a' } as any, + ), + ); + expect(err, 'the ordinary tenanted upsert must never be refused').toBeNull(); + + await driver.upsert( + TENANTED.name, + { id: 'os8807_t', email: 't3@b.com', tax_id: 'T-T3', title: 'second' }, + undefined, + { tenantId: 'org_a' } as any, + ); + + const after = await driver.find(TENANTED.name, { where: { id: 'os8807_t' } }); + expect(after).toHaveLength(1); + expect(after[0].title).toBe('second'); + expect(after[0].organization_id).toBe('org_a'); + }); + + /** + * ✅ **[#8807] THE pin that fixes the scoping DECISION**, and the one that + * separates the two readings of "tenant-scope the identity read". + * + * `injectTenantOnInsert` never overwrites an explicit value — "admins + * writing to a specific tenant via raw row data keep that authority". So a + * caller whose active org is `org_a` can deliberately land a row in + * `org_b`, and that write is CORRECT. + * + * Scoping the identity read to the caller's **active org** would then read + * `(organization_id = 'org_a' OR IS NULL)`, miss the row that was really + * written, and refuse a correct write — a false refusal, the expensive + * direction this file names throughout. Scoping to the tenant the row was + * **written under** does not. This pin goes RED under the first reading and + * green under the one implemented. + */ + it('[#8807] does not falsely refuse an admin write that lands in another org', async () => { + const err = await captureError(() => + driver.upsert( + TENANTED.name, + { organization_id: 'org_b', email: 't4@b.com', tax_id: 'T-T4', title: 'first' }, + undefined, + { tenantId: 'org_a' } as any, + ), + ); + + expect( + err, + "the identity read is scoped to the CALLER's active org rather than the tenant the row " + + 'was written under, so a deliberate cross-org admin write reads as a cross-row merge', + ).toBeNull(); + + const after = await driver.find(TENANTED.name, { where: { tax_id: 'T-T4' } }); + expect(after).toHaveLength(1); + expect(after[0].organization_id).toBe('org_b'); + }); + + it("[#8807] the lifecycle archiver's hot->cold copy still lands, and re-runs idempotently", async () => { + const row = { id: 'os8807_archived', email: 'arch@b.com', tax_id: 'T-ARCH', title: 'copied' }; + + const first = await captureError(() => driver.upsert(WRONG_KEY.name, row, ['id'])); + expect(first, "the archiver copy must not be refused — it supplies the row's own identity").toBeNull(); + + // The sweep re-runs: same row, same id, already present cold. + const second = await captureError(() => driver.upsert(WRONG_KEY.name, row, ['id'])); + expect(second, 'the archiver is idempotent by design — the re-copy must merge, not refuse').toBeNull(); + + const after = await rows(WRONG_KEY.name); + expect(after).toHaveLength(1); + expect(after[0].id).toBe('os8807_archived'); + expect(after[0].email).toBe('arch@b.com'); + }); + /** * The control, and the reason the pins above are readable as a refusal * rather than as a broken cell: the primary-key merge path — the one whose diff --git a/packages/drivers/driver-sql/src/sql-driver.ts b/packages/drivers/driver-sql/src/sql-driver.ts index c8b8ab59e0..d52d0bbed9 100644 --- a/packages/drivers/driver-sql/src/sql-driver.ts +++ b/packages/drivers/driver-sql/src/sql-driver.ts @@ -1304,17 +1304,23 @@ function refuseUnbackedConflictTarget(object: string, mergeKeys: string[], cause * out is impossible is worse than no refusal. * - **As the named target** — `upsert(…, ['id'])` is the driver's own identity * path, byte-identical in compilation to the default `conflictKeys`-less - * call, which this pre-flight deliberately never probes. Refusing the - * explicit spelling while merging the implicit one would make the accept set - * a property of how the caller typed the same statement. It is also the only - * `conflictKeys` shape the platform itself issues (the lifecycle archiver's - * hot→cold copy), whose remedy would read "drop the unique constraint you - * declared on your own business column". - * - * Both residues are real and are documented as the dialect limit in - * `content/docs/data-modeling/drivers.mdx` rather than left for a reader to - * discover: on MySQL a merge can still land on an unnamed unique key whenever no - * target was named at all, or when the named target is the primary key. + * call. Refusing the explicit spelling while merging the implicit one would + * make the accept set a property of how the caller typed the same statement. + * It is also the only `conflictKeys` shape the platform itself issues (the + * lifecycle archiver's hot→cold copy), whose remedy would read "drop the + * unique constraint you declared on your own business column". + * + * ✅ **[#8807] closed both residues, and NOT by widening this refusal.** The + * paragraph above is still the reason the primary key is not refused *here* — + * what changed is that "not refused here" no longer means "unprotected". A + * primary-key-targeted merge on a table carrying a rival UNIQUE key is now + * verified AFTER the statement, inside a transaction, and rolled back if it + * landed on a row the caller never identified + * ({@link refuseCrossRowIdentityMerge}). That shape was chosen over extending + * this pre-flight precisely because of the first bullet: on the primary-key + * path every non-primary UNIQUE key is a rival, so a pre-flight refusal there + * could not have been narrowed at all and would have been the blanket ban the + * #8755 ruling and the #8807 ruling both excluded. */ function refuseAmbiguousConflictTarget( object: string, @@ -1347,6 +1353,99 @@ function refuseAmbiguousConflictTarget( return err; } +/** + * What the upsert pre-flight tells {@link SqlDriver.upsert} once it has judged + * the table's physical keys — a value rather than the bare `void` it returned + * while every verdict it could reach was either "proceed" or a throw. + * + * `verifyIdentity` is [#8807]'s third answer: the statement may run, but the row + * it lands on has to be checked afterwards. `rivals` and `tableName` are carried + * so the refusal can name the keys the introspection actually read instead of + * introspecting a second time to say the same thing. + */ +interface ConflictTargetVerdict { + verifyIdentity: boolean; + rivals?: PhysicalIndex[]; + tableName?: string; +} + +/** + * [#8807] The merge landed on a row the caller never identified. + * + * # Why this is a different condition from both refusals above, per #5240 + * + * {@link refuseUnbackedConflictTarget} answers *no index backs your target* and + * {@link refuseAmbiguousConflictTarget} answers *your target is backed but + * cannot be honoured* — both to a caller who NAMED a target, both before the + * statement is compiled. This one answers a caller who named nothing at all + * (or named `id`, the identity the driver itself supplies), and it can only be + * answered AFTER the statement has run, because the question is which row the + * server picked. Reusing either sentence would instruct an author to change a + * `conflictKeys` argument they never wrote. + * + * The ruled principle it enforces (maintainer, 2026-08-15): *an `upsert` must + * never modify a row whose identity the caller did not supply and whose conflict + * key it did not name.* + * + * # Why the write is rolled back rather than reported + * + * "Never modify" is not satisfied by noticing afterwards. The check therefore + * runs inside a transaction with the statement, and throwing here is what undoes + * the wrong write — see {@link SqlDriver.upsert}. Reporting a completed + * cross-row overwrite would leave the corruption and merely narrate it. + * + * # Why `VALIDATION_ERROR` / 400 + * + * Same classifier, same answer as the two refusals above, and for the same + * reason: this is conditional on the TABLE, not on the dialect. The identical + * statement against the identical MySQL server is honoured the moment the table + * stops carrying a rival UNIQUE key, so the remedy is a schema change and not + * "wait for the backend". 400 also keeps the sentence on the wire — `rest` + * withholds the body of any 5xx, and this message is the deliverable. + * + * Measured on live MySQL 8.0.46 through the same knex + `mysql2` path, `email` + * and `tax_id` both `unique: true`, NO `conflictKeys` at all: + * + * ``` + * seed upsert({email:'d@b.com', tax_id:'T-9', title:'first'}) -> id=iVvD35rMk4BIayYc + * B upsert({email:'e@b.com', tax_id:'T-9', title:'second'}) -> RESOLVED, id=F-Fp1OGCQB-l5XRu + * ONE row, and it is the SEEDED one: iVvD35rMk4BIayYc, its `email` rewritten + * d@b.com -> e@b.com. The id the caller was handed back does not exist in the + * table at all. + * ``` + */ +function refuseCrossRowIdentityMerge( + object: string, + tableName: string, + id: unknown, + rivals: PhysicalIndex[], +): Error { + const named = rivals.map((i) => `${i.name}(${i.columns.join(', ')})`).join(', '); + const err = new Error( + `Cannot upsert into "${object}": the merge landed on a row this call never identified. No ` + + `conflict target was named, so the merge target is the primary key — but this backend is ` + + `MySQL, whose only merge statement is ON DUPLICATE KEY UPDATE, and that statement carries ` + + `no conflict target. The row did not collide on its primary key; it collided on another ` + + `UNIQUE key on "${tableName}" — ${named} — and MySQL merged there instead, overwriting a ` + + `DIFFERENT row than the one this call supplied. The write has been rolled back; nothing was ` + + `changed. Fix by naming the business key you meant to merge on (pass it as conflictKeys, so ` + + `the intent is checkable), by dropping or renaming the extra UNIQUE key(s) so the primary ` + + `key is the only one the row can collide on, or by running this object on a dialect that ` + + `honours the target — SQLite and PostgreSQL compile ON CONFLICT (...), which raises a ` + + `unique violation here instead of merging.`, + ) as Error & { code?: string; status?: number; cause?: unknown }; + err.code = StandardErrorCode.enum.VALIDATION_ERROR; + err.status = 400; + // Schema identifiers plus the id this call supplied — the same payload + // contract the two refusals above keep. The id is the driver's OWN value + // (minted here, or the caller's `id`/`_id`), never a business column value. + err.cause = new Error( + `no row on "${tableName}" carries id ${JSON.stringify(id)} after the merge; UNIQUE keys that ` + + `can absorb a primary-key-targeted merge: ${named}`, + ); + return err; +} + /** * [#5158] A `FilterArray` reached the driver unlowered. * @@ -5548,7 +5647,11 @@ export class SqlDriver implements IDataDriver { * it is now one of two, and a name that describes half of what a guard refuses * is how the other half gets deleted by someone tidying up. */ - protected async assertConflictTargetHonoured(object: string, mergeKeys: string[]): Promise { + protected async assertConflictTargetHonoured( + object: string, + mergeKeys: string[], + callerNamed: boolean, + ): Promise { // The table the statement will actually hit — same resolution `getBuilder` // performs for the insert, rotation shard included. const target = this.rotationWriteTarget(object) ?? object; @@ -5561,15 +5664,36 @@ export class SqlDriver implements IDataDriver { type Verdict = | { kind: 'honoured' } | { kind: 'unbacked' } - | { kind: 'ambiguous'; rivals: PhysicalIndex[] }; + | { kind: 'ambiguous'; rivals: PhysicalIndex[] } + | { kind: 'identity-at-risk'; rivals: PhysicalIndex[] }; const judge = (keys: PhysicalIndex[]): Verdict => { if (!keys.some(covers)) return { kind: 'unbacked' }; - // [#8755] The named target is the PRIMARY KEY: the driver's own identity - // path, and the one shape whose refusal would differ from the - // byte-identical default call this pre-flight never probes. Left to merge - // — the residue is documented, not silent (see the refusal's docblock). - if (keys.some((i) => i.primary === true && covers(i))) return { kind: 'honoured' }; + // The target is the PRIMARY KEY — the driver's own identity path, and the + // shape the `conflictKeys`-less default compiles to byte-identically. + // + // [#8755] left this merging and documented the residue. [#8807] takes it, + // and the reason it is a THIRD verdict rather than a fourth caller of the + // ambiguous refusal is the whole of that card: here the caller named + // nothing (or named the identity the driver supplies), so there is no + // promised target to break. What the platform owes is narrower and + // dialect-independent — *an upsert must never modify a row whose identity + // the caller did not supply and whose conflict key it did not name* + // (maintainer ruling 2026-08-15) — and that is a statement about the row + // the statement LANDED on, which no pre-flight can know. + // + // So this verdict does not refuse. It reports that a rival key exists and + // the landing row must therefore be verified after the fact; see + // {@link upsert} for the check and {@link refuseCrossRowIdentityMerge} + // for the sentence. Refusing here instead would be the blanket option the + // ruling excluded: on this path every non-primary UNIQUE key is a rival, + // so "narrowed to tables carrying a rival key" and "every table with a + // business unique key" are THE SAME SET — the narrowing that made the + // refusal proportionate for a caller-named target does not exist here. + if (keys.some((i) => i.primary === true && covers(i))) { + const rivals = keys.filter((i) => i.primary !== true && !covers(i)); + return rivals.length > 0 ? { kind: 'identity-at-risk', rivals } : { kind: 'honoured' }; + } // A UNIQUE key that is neither the named target nor the primary key can // absorb the conflict instead of it. An index over the SAME columns as the // target is not a rival: colliding on it is colliding on the target. @@ -5577,8 +5701,20 @@ export class SqlDriver implements IDataDriver { return rivals.length > 0 ? { kind: 'ambiguous', rivals } : { kind: 'honoured' }; }; + // Only the two REFUSING verdicts re-read the cache. [#8807]'s + // `identity-at-risk` deliberately does not, and the asymmetry is the reason: + // a re-read exists so a stale cache can never produce a FALSE REFUSAL, and + // this verdict refuses nothing. Both staleness directions are safe here — a + // rival created since the read means the check is skipped (exactly today's + // behaviour, nothing lost), and a rival DROPPED since the read means the + // check runs and passes, because the merge really does land on the primary + // key. Re-reading anyway would put a round trip on the hot path of every + // ordinary upsert to sharpen an answer that cannot be wrong in the + // expensive direction. + const refusing = (v: Verdict): boolean => v.kind === 'unbacked' || v.kind === 'ambiguous'; + let keys = await this.introspectKeyIndexes(tableName); - if (keys !== null && keys.length > 0 && judge(keys).kind !== 'honoured') { + if (keys !== null && keys.length > 0 && refusing(judge(keys))) { // A cache filled before the index was created is the only way a real key // can be missing here, and a cache filled before one was DROPPED is the // only way a rival key can be reported that no longer exists. Both are @@ -5587,10 +5723,23 @@ export class SqlDriver implements IDataDriver { // both verdicts. keys = await this.introspectKeyIndexes(tableName, { fresh: true }); } - if (keys === null || keys.length === 0) return; + if (keys === null || keys.length === 0) return { verifyIdentity: false }; const verdict = judge(keys); - if (verdict.kind === 'honoured') return; + if (verdict.kind === 'honoured') return { verifyIdentity: false }; + + if (verdict.kind === 'identity-at-risk') { + return { verifyIdentity: true, rivals: verdict.rivals, tableName }; + } + + // [#8807] A target the caller did NOT name is never refused. `['id']` is + // what this driver substitutes when `conflictKeys` is absent, so a refusal + // reached from there would be refusing the driver's own default — and the + // two spellings compile byte-identically, which is precisely why #8755 + // declined to separate them. Both refusals below answer a caller who named + // something the dialect cannot honour; with nothing named there is no such + // caller, and the post-hoc identity check above is the whole remedy. + if (!callerNamed) return { verifyIdentity: false }; if (verdict.kind === 'ambiguous') { throw refuseAmbiguousConflictTarget(object, mergeKeys, tableName, verdict.rivals); @@ -5612,6 +5761,103 @@ export class SqlDriver implements IDataDriver { ); } + /** + * [#8807] After a primary-key-targeted merge on a table that carries a rival + * UNIQUE key: did the statement land on the identity this call supplied? + * + * # Why the absence of the row is an exact verdict, not a heuristic + * + * The statement returned without error, so it either INSERTED (the row now + * carries this id) or MERGED into an existing row. `id` is insert-only on the + * merge path since #8622 — it is stripped from the merge set — so a merge + * never writes this id onto the row it lands on. Therefore: + * + * row with this id exists ⟺ the merge landed on the primary key + * row with this id absent ⟺ it landed on some other UNIQUE key + * + * That is a biconditional, not an inference from a symptom, which is what + * makes this check free of false refusals — the property the ruling's + * excluded blanket option could not have. + * + * # Why the PRIMARY KEY row wins a multi-key collision — measured, not assumed + * + * The biconditional needs one more fact than #8622 to be exact: when the + * incoming row collides on the primary key AND on a rival UNIQUE key at the + * same time, the row carrying our id exists — so "row present" must still mean + * "the statement landed on it", or a pre-existing row with our id would mask a + * merge that went elsewhere. MySQL updates only the FIRST matched index, and + * *which* one that is decides the question. Measured on live MySQL 8.0.46, + * `PRIMARY KEY (id)` + `UNIQUE (email)` + `UNIQUE (tax_id)`, rows + * `R1(id=R1, tax_id=T-1)` and `R2(id=R2, tax_id=T-2)`, inserting + * `(id=R1, tax_id=T-2)` — which collides with R1 on the primary key and with + * R2 on `uniq_tax`: + * + * ``` + * ROW_COUNT() = 2 (an update) + * R1 -> title='MERGED' ← the PRIMARY KEY row won + * R2 -> untouched + * ``` + * + * So the primary key is matched first and the masking case does not arise. + * + * # Tenant scope — applied, and scoped to the tenant the row was WRITTEN under + * + * The read routes through {@link applyTenantScope} like every other read door + * in this class (`check:tenant-chokepoint` asserts exactly that, on a bound + * builder). The subtlety is *which* tenant to scope to, and using the caller's + * active org would be wrong in one real case: + * + * - **Ordinary tenanted call** — nothing supplied a tenant on the row, so + * `injectTenantOnInsert` stamped `options.tenantId` on it. Scoping to the + * written tenant IS scoping to the caller's org: identical to the wall + * every sibling door applies. + * - **Admin writing to a specific tenant via raw row data** — a documented + * authority ({@link injectTenantOnInsert}: "explicit values are never + * overwritten"). The row lands under the tenant the caller named, so a read + * scoped to the caller's *active* org would miss a row that really was + * written and refuse a correct write. Scoping to the written tenant does + * not. + * - **No tenancy field, or no tenant context at all** — `applyTenantScope` + * returns the builder untouched when `tenantId` is empty, by its own + * contract. That is what keeps the lifecycle archiver working: it calls + * `cold.upsert(object, row, ['id'])` with NO options, so this read is + * unscoped there, exactly as before. + * + * The verdict itself is tenant-independent regardless: `id` is the PRIMARY + * KEY, so at most one row in the table can carry it. + * + * # The write target, not the object + * + * A rotation-sharded write lands in the current shard, so that is where the + * row must be looked for. Reading the unsharded name would report every + * rotated write as a cross-row merge. + */ + private async assertMergeLandedOnSuppliedIdentity( + object: string, + writeTable: string, + id: string | number, + rivals: PhysicalIndex[], + writtenTenant: unknown, + options?: DriverOptions, + ): Promise { + const builder = this.getBuilder(writeTable, options); + // Scoped to the tenant this row was written under (see the docblock): the + // caller's org on an ordinary call, the explicitly named one on an admin + // cross-tenant write, and a no-op when neither exists. `tenantIds` is + // dropped deliberately — the group-union posture widens a READ to a + // membership set, and this is an identity probe for ONE row, not a read. + const scopeOptions: DriverOptions = { + ...options, + tenantId: typeof writtenTenant === 'string' && writtenTenant !== '' ? writtenTenant : options?.tenantId, + tenantIds: undefined, + }; + this.applyTenantScope(builder, object, scopeOptions); + const landed = await builder.where('id', id).first('id'); + if (landed) return; + const tableName = this.physicalTableByObject[writeTable] ?? writeTable; + throw refuseCrossRowIdentityMerge(object, tableName, id, rivals); + } + async upsert(object: string, data: Record, conflictKeys?: string[], options?: DriverOptions): Promise> { const { _id, ...rest } = data; const toUpsert = { ...rest }; @@ -5627,23 +5873,42 @@ export class SqlDriver implements IDataDriver { const mergeKeys = conflictKeys && conflictKeys.length > 0 ? conflictKeys : ['id']; - // [#8621, #8755] Pre-flight the conflict target — see + // [#8621, #8755, #8807] Pre-flight the conflict target — see // {@link assertConflictTargetHonoured} for the mechanism and why it is // MySQL-only. Two placement facts, both load-bearing: // - // - It runs only when the CALLER named a target. The default `['id']` is - // this driver's own primary key on every table it creates, so a probe - // there could only ever confirm what the driver just built — a round - // trip added to the hot path of every ordinary upsert to answer a - // question that has no other answer. The defect, the card and the ruling - // are all about a caller-named target. + // - ⚠️ It now runs whether or not the CALLER named a target, and the + // reasoning that used to stop at the caller-named case is superseded + // rather than merely widened. That reasoning was *"a probe on the default + // path could only ever confirm what the driver just built"*, and it was + // right about the question #8621/#8755 were asking — "is `['id']` backed?" + // has one possible answer on a table this driver created. #8807 asks a + // different question of the same introspection — "does anything ELSE on + // this table absorb a collision?" — and that one is genuinely unknown + // until read. The read is cached per table + // ({@link physicalKeyIndexes}), so the hot path pays it once, and the + // verdict it returns is not a refusal: see below. // - It runs BEFORE the retry loop, therefore before // `fillAutoNumberFields`. Refusing after it would burn an autonumber // reservation for a statement that never executes — a visible gap in an // externally meaningful sequence, handed out for a rejected call. - if (conflictKeys && conflictKeys.length > 0 && this.isMysql) { - await this.assertConflictTargetHonoured(object, mergeKeys); - } + const callerNamed = !!(conflictKeys && conflictKeys.length > 0); + const preflight = this.isMysql + ? await this.assertConflictTargetHonoured(object, mergeKeys, callerNamed) + : { verifyIdentity: false }; + + // [#8807] The primary-key-targeted merge on a table that carries a rival + // UNIQUE key: MySQL may land it on a row this call never identified, and + // nothing before the statement can tell whether it will. So the statement + // and the identity check run as ONE unit of work, and the check's failure + // is what rolls the wrong write back. + // + // The accept set moves for exactly the calls that corrupt data: a merge + // that lands on the supplied identity — every insert, and every legitimate + // re-upsert of the same row — passes the check untouched, because `id` is + // insert-only (#8622) and therefore a row merged on the primary key always + // still carries it. + const verifyIdentity = preflight.verifyIdentity === true; // #6943. Measured: `upsert` does NOT share `bulkCreate`'s shape. It is // single-row, so a stale counter costs it exactly one burned number per @@ -5666,7 +5931,7 @@ export class SqlDriver implements IDataDriver { // Rotation: conflict-merge is scoped to the CURRENT shard (telemetry is // effectively append-only; a cross-shard upsert would need a probe-first // strategy nothing on the platform requires today). - const builder = this.getBuilder(this.rotationWriteTarget(object) ?? object, options); + const writeTable = this.rotationWriteTarget(object) ?? object; // `created_at` is insert-only — never overwrite it when an existing row is // merged on conflict (the stamped/seeded value belongs to the original // insert). [#7011] `auto_number` columns are insert-only for the same @@ -5710,7 +5975,6 @@ export class SqlDriver implements IDataDriver { // that updates a row still advances `updated_at`. const insertOnlyColumns = this.insertOnlyUpsertColumns(object); const mergeColumns = Object.keys(formatted).filter((c) => !insertOnlyColumns.has(c)); - const insertion = builder.insert(formatted).onConflict(mergeKeys); // [#8622] Excluding `id` made a branch REACHABLE that could not fire // before, and the bare `merge()` below is merge-ALL: it re-admits every @@ -5740,8 +6004,57 @@ export class SqlDriver implements IDataDriver { const noopMergeColumns = Object.keys(formatted).filter((c) => mergeKeys.includes(c)); const columnsToMerge = mergeColumns.length > 0 ? mergeColumns : noopMergeColumns; - try { + // The statement, plus [#8807]'s identity check when one is owed. The + // builder is built HERE rather than above the comment block so it can be + // bound to whichever transaction this write runs on — the caller's, the + // one opened just below, or none at all. The emitted INSERT is otherwise + // unchanged. + const runStatement = async (writeOptions?: DriverOptions): Promise => { + // Bound in two steps, not chained: this is the INSERT door, and + // `check:tenant-chokepoint` classifies a builder as a write by seeing + // `.insert` used on the BINDING. Collapsing these into one expression + // hides that from the gate and the door reads as an unscoped read — + // measured when this closure was first extracted. Insert-side tenancy + // is `injectTenantOnInsert`'s job (called above), which is exactly why + // this one is deliberately not scoped. + const builder = this.getBuilder(writeTable, writeOptions); + const insertion = builder.insert(formatted).onConflict(mergeKeys); await (columnsToMerge.length > 0 ? insertion.merge(columnsToMerge) : insertion.merge()); + if (verifyIdentity) { + // The tenant the row was WRITTEN under, read off the payload after + // `injectTenantOnInsert` has run — the caller's org on an ordinary + // call, an explicitly supplied one on an admin cross-tenant write. + const tenantField = this.resolveTenantField(object); + await this.assertMergeLandedOnSuppliedIdentity( + object, + writeTable, + toUpsert.id, + preflight.rivals ?? [], + tenantField ? toUpsert[tenantField] : undefined, + writeOptions, + ); + } + }; + + try { + if (verifyIdentity && options?.transaction === undefined) { + // [#8807] No caller transaction, so the driver opens one: the check + // is only worth making if its failure can UNDO the write it judged, + // and an autocommitted statement is already permanent by the time the + // row can be read back. Scoped to `verifyIdentity` so the ordinary + // upsert — every dialect but MySQL, and every MySQL table with no + // rival UNIQUE key — keeps its single autocommitted round trip. + // + // Inside a caller transaction the wrapper is deliberately NOT added: + // the statement is already transactional, and throwing hands the + // rollback decision to the owner of that transaction, exactly as the + // autonumber path above reasons about the same boundary. + await this.knex.transaction(async (trx) => { + await runStatement({ ...options, transaction: trx }); + }); + } else { + await runStatement(options); + } break; } catch (error) { // [#8445] Classified BEFORE the autonumber retry logic, for three diff --git a/packages/objectql/src/lifecycle/lifecycle-service.ts b/packages/objectql/src/lifecycle/lifecycle-service.ts index b64aa9ead8..fb6b40b254 100644 --- a/packages/objectql/src/lifecycle/lifecycle-service.ts +++ b/packages/objectql/src/lifecycle/lifecycle-service.ts @@ -1119,6 +1119,32 @@ export class LifecycleService { limit: ARCHIVE_BATCH_SIZE, }); if (!rows.length) break; + // [#8807] The conflict target stays `['id']`, and that is a measured + // conclusion rather than an untouched line. + // + // #8807 ruled that an upsert must never modify a row whose identity the + // caller did not supply and whose conflict key it did not name — the + // MySQL `ON DUPLICATE KEY UPDATE` merge can land on any UNIQUE key, + // including one nobody named. The enforcement options included refusing + // this exact call shape on any table carrying a non-primary UNIQUE key, + // which would have refused archival wholesale, so the ruling required the + // blast radius on THIS caller be measured before anything shipped. + // + // Measured: of the objects in this repo that declare `lifecycle.archive` + // — `sys_audit_log` and `sys_metadata_audit`, the only two — ZERO declare + // a non-primary unique field or a `unique` index. The archiver therefore + // never even reaches the check today. + // + // It is also correct by construction for a customer object that DOES + // carry one, which is why no opt-out is threaded through here: this loop + // copies a row that already has an `id` and re-copies it idempotently, so + // the merge lands on the identity it supplied and the check passes. The + // one case it would refuse is a cold row that collides on a business key + // while carrying a DIFFERENT id — which is not archival working, it is + // archival about to overwrite an unrelated archived record. Refusing + // there is the Archiver's own safety rule ("hot-delete only what the cold + // store has taken"): the upsert throws, `bulkDelete` below never runs, + // and the hot rows survive for the next sweep. for (const row of rows) { await cold.upsert(object, row, ['id']); } diff --git a/packages/spec/src/migrations/entries/semantic/18.driver-sql-upsert-cross-row-identity-merge-refused.ts b/packages/spec/src/migrations/entries/semantic/18.driver-sql-upsert-cross-row-identity-merge-refused.ts new file mode 100644 index 0000000000..d8b53f78a7 --- /dev/null +++ b/packages/spec/src/migrations/entries/semantic/18.driver-sql-upsert-cross-row-identity-merge-refused.ts @@ -0,0 +1,70 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +import type { SemanticMigration } from '../../types.js'; + +export const entry: SemanticMigration = { + id: 'driver-sql-upsert-cross-row-identity-merge-refused', + surface: + 'an `upsert` with no `conflictKeys` — or naming the primary key — on a MySQL table that ' + + 'carries a non-primary UNIQUE key, in `driver-sql` (and its `TursoDriver` / ' + + '`SqliteWasmDriver` subclasses). It merged onto whichever UNIQUE key the row collided ' + + 'with, silently rewriting a DIFFERENT row; when that happens the write is now rolled ' + + 'back and the call refuses with `VALIDATION_ERROR` / 400', + replacement: + 'name the business key you meant to merge on (`conflictKeys`), so the intent is checkable ' + + 'and the pre-flight can answer for it; or drop/rename the extra UNIQUE key so the ' + + 'primary key is the only thing a row can collide on; or run the object on SQLite / ' + + 'PostgreSQL, which compile `ON CONFLICT (...)` and honour the named arbiter. There is ' + + 'no spelling of "merge onto whatever key happens to collide" that was ever correct — ' + + 'the old behaviour rewrote a row the caller never identified', + reason: + 'MySQL\'s only merge statement is `ON DUPLICATE KEY UPDATE`, which carries NO conflict ' + + 'target: knex drops the named keys before the statement leaves the process, so the ' + + 'merge lands on whichever UNIQUE index the row collides with first. #8621 closed the ' + + 'half where nothing backed a caller-named target and #8755 the half where a rival key ' + + 'could absorb a caller-named one. This entry closes the residue those two left by ' + + 'construction: the `conflictKeys`-less call and the `[\'id\']` call, which compile ' + + 'byte-identically and which no pre-flight can judge, because neither names anything.\n\n' + + 'Measured on live MySQL 8.0.46 through the same knex + `mysql2` path `upsert` takes, ' + + '`email` and `tax_id` both `unique: true`, NO `conflictKeys` at all: seeding ' + + "`{email:'d@b.com', tax_id:'T-9', title:'first'}` inserted id `iVvD35rMk4BIayYc`, and " + + "`{email:'e@b.com', tax_id:'T-9', title:'second'}` then RESOLVED with no error — one " + + 'row, the SEEDED one, its `email` rewritten `d@b.com` -> `e@b.com`. The id the caller ' + + 'was handed back was in no row at all. The identical pair on SQLite raises `UNIQUE ' + + 'constraint failed: ….tax_id` and leaves the seeded row untouched.\n\n' + + 'Ruled 2026-08-15 on #8807, as a contract principle rather than a MySQL detail: *an ' + + '`upsert` must never modify a row whose identity the caller did not supply and whose ' + + 'conflict key it did not name.* Enforcement was delegated to the drivers lane with ' + + 'blanket refusal excluded by name — refusing every `conflictKeys`-less upsert on any ' + + 'table with a business unique key would refuse the platform\'s own lifecycle archiver. ' + + 'Measured before choosing: on this path the merge target is always the primary key, so ' + + 'EVERY non-primary UNIQUE key is a rival and "narrowed to tables carrying a rival key" ' + + 'and "every table with a business unique key" are the same set — the narrowing that ' + + 'made a pre-flight refusal proportionate for a caller-named target does not exist ' + + 'here.\n\n' + + 'So the enforcement is a post-hoc identity check instead, and it is exact rather than ' + + 'heuristic: `id` is insert-only on the merge path since #8622, so a row merged on the ' + + 'primary key always still carries the id the call supplied, and a row merged on any ' + + 'other key never does. Absence of that row after the statement is therefore a ' + + 'biconditional for "this landed on a row the caller never identified", which is why the ' + + 'refusal has no false positives. It runs inside a transaction with the statement — ' + + '"never modify" is not satisfied by noticing afterwards — and only on MySQL tables that ' + + 'carry a rival UNIQUE key, so a table whose only key is its primary key keeps its ' + + 'single autocommitted round trip unchanged.\n\n' + + 'This is a CODE-path API, not stored metadata, so — like ' + + '`driver-sql-unresolvable-where-column-refused` — there is no `sys_metadata` row for ' + + 'the D2 chain to rewrite and this entry is the notification channel. No mechanical ' + + 'rewrite exists: the platform cannot know which business key an unnamed merge meant, ' + + 'and guessing one would merge onto a row the caller never named, which is the defect. ' + + '#8807, #8755, #8621, #8622, #8592, ADR-0112.', + acceptanceCriteria: + 'On MySQL deployments only. For every object whose rows are written with `upsert` and ' + + 'whose table carries a UNIQUE key besides the primary key, confirm the writer either ' + + 'supplies the `id` of the row it means to update or passes that business key as ' + + '`conflictKeys`. Sweeps, imports and archival copies complete with no ' + + '`VALIDATION_ERROR` whose message says "the merge landed on a row this call never ' + + 'identified". Where such a refusal appears, the old behaviour was silently overwriting ' + + 'an unrelated row on that table — audit the object for rows whose business key is ' + + 'correct but whose other columns belong to a different record, since no error was ever ' + + 'raised for those writes.', +}; diff --git a/packages/spec/src/migrations/registry.ts b/packages/spec/src/migrations/registry.ts index cb1ac9c70c..36225ed646 100644 --- a/packages/spec/src/migrations/registry.ts +++ b/packages/spec/src/migrations/registry.ts @@ -5062,6 +5062,72 @@ const step18: MigrationStep = { + 'object — the driver never resolved such a path and answered `[]`, so any list that ' + 'looked correct under one was already showing nothing.', }, + { + id: 'driver-sql-upsert-cross-row-identity-merge-refused', + surface: + 'an `upsert` with no `conflictKeys` — or naming the primary key — on a MySQL table that ' + + 'carries a non-primary UNIQUE key, in `driver-sql` (and its `TursoDriver` / ' + + '`SqliteWasmDriver` subclasses). It merged onto whichever UNIQUE key the row collided ' + + 'with, silently rewriting a DIFFERENT row; when that happens the write is now rolled ' + + 'back and the call refuses with `VALIDATION_ERROR` / 400', + replacement: + 'name the business key you meant to merge on (`conflictKeys`), so the intent is checkable ' + + 'and the pre-flight can answer for it; or drop/rename the extra UNIQUE key so the ' + + 'primary key is the only thing a row can collide on; or run the object on SQLite / ' + + 'PostgreSQL, which compile `ON CONFLICT (...)` and honour the named arbiter. There is ' + + 'no spelling of "merge onto whatever key happens to collide" that was ever correct — ' + + 'the old behaviour rewrote a row the caller never identified', + reason: + 'MySQL\'s only merge statement is `ON DUPLICATE KEY UPDATE`, which carries NO conflict ' + + 'target: knex drops the named keys before the statement leaves the process, so the ' + + 'merge lands on whichever UNIQUE index the row collides with first. #8621 closed the ' + + 'half where nothing backed a caller-named target and #8755 the half where a rival key ' + + 'could absorb a caller-named one. This entry closes the residue those two left by ' + + 'construction: the `conflictKeys`-less call and the `[\'id\']` call, which compile ' + + 'byte-identically and which no pre-flight can judge, because neither names anything.\n\n' + + 'Measured on live MySQL 8.0.46 through the same knex + `mysql2` path `upsert` takes, ' + + '`email` and `tax_id` both `unique: true`, NO `conflictKeys` at all: seeding ' + + "`{email:'d@b.com', tax_id:'T-9', title:'first'}` inserted id `iVvD35rMk4BIayYc`, and " + + "`{email:'e@b.com', tax_id:'T-9', title:'second'}` then RESOLVED with no error — one " + + 'row, the SEEDED one, its `email` rewritten `d@b.com` -> `e@b.com`. The id the caller ' + + 'was handed back was in no row at all. The identical pair on SQLite raises `UNIQUE ' + + 'constraint failed: ….tax_id` and leaves the seeded row untouched.\n\n' + + 'Ruled 2026-08-15 on #8807, as a contract principle rather than a MySQL detail: *an ' + + '`upsert` must never modify a row whose identity the caller did not supply and whose ' + + 'conflict key it did not name.* Enforcement was delegated to the drivers lane with ' + + 'blanket refusal excluded by name — refusing every `conflictKeys`-less upsert on any ' + + 'table with a business unique key would refuse the platform\'s own lifecycle archiver. ' + + 'Measured before choosing: on this path the merge target is always the primary key, so ' + + 'EVERY non-primary UNIQUE key is a rival and "narrowed to tables carrying a rival key" ' + + 'and "every table with a business unique key" are the same set — the narrowing that ' + + 'made a pre-flight refusal proportionate for a caller-named target does not exist ' + + 'here.\n\n' + + 'So the enforcement is a post-hoc identity check instead, and it is exact rather than ' + + 'heuristic: `id` is insert-only on the merge path since #8622, so a row merged on the ' + + 'primary key always still carries the id the call supplied, and a row merged on any ' + + 'other key never does. Absence of that row after the statement is therefore a ' + + 'biconditional for "this landed on a row the caller never identified", which is why the ' + + 'refusal has no false positives. It runs inside a transaction with the statement — ' + + '"never modify" is not satisfied by noticing afterwards — and only on MySQL tables that ' + + 'carry a rival UNIQUE key, so a table whose only key is its primary key keeps its ' + + 'single autocommitted round trip unchanged.\n\n' + + 'This is a CODE-path API, not stored metadata, so — like ' + + '`driver-sql-unresolvable-where-column-refused` — there is no `sys_metadata` row for ' + + 'the D2 chain to rewrite and this entry is the notification channel. No mechanical ' + + 'rewrite exists: the platform cannot know which business key an unnamed merge meant, ' + + 'and guessing one would merge onto a row the caller never named, which is the defect. ' + + '#8807, #8755, #8621, #8622, #8592, ADR-0112.', + acceptanceCriteria: + 'On MySQL deployments only. For every object whose rows are written with `upsert` and ' + + 'whose table carries a UNIQUE key besides the primary key, confirm the writer either ' + + 'supplies the `id` of the row it means to update or passes that business key as ' + + '`conflictKeys`. Sweeps, imports and archival copies complete with no ' + + '`VALIDATION_ERROR` whose message says "the merge landed on a row this call never ' + + 'identified". Where such a refusal appears, the old behaviour was silently overwriting ' + + 'an unrelated row on that table — audit the object for rows whose business key is ' + + 'correct but whose other columns belong to a different record, since no error was ever ' + + 'raised for those writes.', + }, { id: 'engine-dotted-filter-refused', surface: