From 2de808050137cc279bcd31ff4bb167ea9ad93fa5 Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 15 Aug 2026 02:30:49 +0000 Subject: [PATCH 1/2] fix(driver-sql): refuse a MySQL upsert whose named conflict target another unique key can absorb (#8755) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `ON DUPLICATE KEY UPDATE` carries no conflict target, so on MySQL the merge lands on whichever UNIQUE key the row collides with first -- including one the caller never named, and including when the named target IS backed. #8621 closed the unbacked half; this closes the rest. Measured on live MySQL 8.0.46 through the same knex + mysql2 path `upsert` takes, both business columns `unique: true`, caller naming `email`: seed upsert({email:'a@b.com', tax_id:'T-1'}, ['email']) -> RESOLVED B upsert({email:'other@b.com', tax_id:'T-1'}, ['email']) -> RESOLVED, ONE row merged on `tax_id`, across two different values of the named key. The identical call on SQLite raises `UNIQUE constraint failed: ….tax_id` and leaves the seeded row untouched -- a legible error rather than a silent wrong write. (Measured; the card's body says "inserts a second row", which its own next clause contradicts and this measurement disproves.) Per the maintainer ruling on #8755 this takes option A -- preflight refusal -- and rejects option B's probe-then-write emulation. The pre-flight extends #8621's introspection rather than adding a second one: the same `physicalKeyIndexes` read, the same cache and invalidation, the same stale-cache re-read before any refusal, now answering two questions instead of one. `assertConflictTargetBacked` is renamed `assertConflictTargetHonoured` because "backed" now describes half of what it refuses. The refusal gets its own wording, `code` and `status` are VALIDATION_ERROR / 400: #5240 is one condition one wording, and this is a different condition from "no index backs your target" -- reusing that sentence would tell an author to declare a `unique: true` they already declared. Not 501: the #5907 classifier this file applies twice sorts by what the caller must change, and this is conditional on the TABLE (a single-unique-key table merges fine on the same server), not a capability gap in the backend. 400 also keeps the message on the wire, and the message is the deliverable -- the ruling requires it to name the colliding key and the way out. Two shapes are deliberately left merging and documented as the dialect's residue rather than silently narrowed: the `conflictKeys`-less default (never probed by any pre-flight), and an explicitly named PRIMARY KEY -- which compiles byte-identically to that default, and is the only `conflictKeys` shape the platform itself issues (the lifecycle archiver's hot->cold copy). Counting the primary key as a rival would refuse every `conflictKeys` upsert on MySQL, since every table this driver creates carries an `id` PRIMARY KEY, and its stated remedy ("drop or rename the extra key") is not available for one. Both branches pinned on the live MySQL cell, as the ruling requires: the single-unique-key upsert still merges, and the two-unique-key upsert refuses with the second key's name and both workarounds asserted as text. #8622's identity pin moves fixture once more, to the default-path merge that still exhibits the wrong-key merge it measures. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01XeQRiAa7vYRVX5Fog7Zby8 --- ...er-upsert-conflict-target-dialects.test.ts | 243 +++++++++++++++--- packages/drivers/driver-sql/src/sql-driver.ts | 197 ++++++++++++-- 2 files changed, 376 insertions(+), 64 deletions(-) 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 4a568935ba..f4311b3eb0 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 @@ -58,8 +58,16 @@ * UNIQUE indexes before compiling, and answers the same sentence this sweep * asserts on the other two dialects. The last section of this file is that * refusal's pins — rewritten from #8592's characterization, as that card - * instructed, not relaxed. What remains un-refused there is the narrower #8755: - * `ON DUPLICATE KEY UPDATE` has no target even when the named one IS backed. + * instructed, not relaxed. + * + * ✅ **[#8755] has since closed the second half**, on the same introspection: + * `ON DUPLICATE KEY UPDATE` has no target even when the named one IS backed, so + * 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. * * ✅ **[#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 @@ -553,6 +561,16 @@ describe('[#8567] MySQL: `onConflict().merge()` compiles the conflict target awa * backed and the pre-flight passes it; see #8755). Same claim, same strength, * same failure message; a fixture that can still exhibit the behaviour. * + * ⚠️ **[#8755] moved that same pin a SECOND time, one step further, and for the + * same reason.** #8755 refuses the two-unique-key call as well, so naming + * `email` on {@link WRONG_KEY} no longer merges either. The pin now runs on the + * `conflictKeys`-LESS default call against that table — the shape no pre-flight + * has ever probed, where MySQL still merges on whichever UNIQUE key collides. + * Measured on live MySQL 8.0.46 while implementing #8755, and it is the same + * phenomenon: one row, merged on `tax_id`, the stored `id` surviving. If a later + * card removes THAT wrong-key merge too, this pin moves again rather than being + * deleted — it is #8622's only MySQL-cell coverage of a landed fix. + * * # How this was measured * * #8567 left the MySQL half as an inference from compiled SQL — "merges on @@ -640,12 +658,19 @@ const MISMATCHED = { } as any; /** - * [#8621] Both business columns unique — so the named target `email` IS backed, - * the pre-flight passes the call, and MySQL merges it on whichever unique index - * the row actually collides with. This is the table where a wrong-key merge - * still happens after this card (#8755), which is why #8622's identity pin now - * runs here: that pin measures identity ACROSS a wrong-key merge, and needs a - * fixture that can still produce one. + * [#8621 → #8755] Both business columns unique. The named target `email` IS + * backed, so #8621's pre-flight passes the call — and MySQL then merges it on + * whichever unique index the row actually collides with, which is the whole of + * #8755. + * + * **This is now the REFUSAL fixture for #8755**: naming a non-primary target on + * this table is refused before compiling, because `uniq_os8621_wrong_key_tax_id` + * can absorb the conflict instead of the named `uniq_os8621_wrong_key_email`. + * + * It remains the wrong-key-MERGE fixture too, on the two shapes #8755 + * deliberately does not refuse and documents as the dialect's residue: the + * `conflictKeys`-less default, and an explicitly named PRIMARY KEY. That is + * where #8622's identity pin lives now. */ const WRONG_KEY = { name: 'os8621_wrong_key', @@ -656,6 +681,25 @@ const WRONG_KEY = { }, } as any; +/** + * [#8755] The DISCRIMINATING CONTROL the ruling names first: one unique key + * besides the primary, and the caller names exactly it. + * + * This is the common shape — a business object with one natural key — and it + * must keep merging, or the refusal is a blanket ban on `conflictKeys` upserts + * over MySQL rather than the narrow rule that was ruled. It is a table of its + * own rather than a reuse of {@link MISMATCHED} with `['tax_id']` (which has the + * same physical shape today) precisely so the control cannot be weakened by a + * later edit to a fixture that exists to be mismatched. + */ +const SINGLE_KEY = { + name: 'os8755_single_key', + fields: { + email: { type: 'string', unique: true }, + title: { type: 'string' }, + }, +} as any; + declareDialectCell( MYSQL_CELL, 'unbacked conflict-target refusal (pre-flight, MySQL)', @@ -677,12 +721,14 @@ function declareMysqlPreflightRefusal(cell: DialectCell): void { knexInstance = (driver as any).knex; await knexInstance.schema.dropTableIfExists(MISMATCHED.name); await knexInstance.schema.dropTableIfExists(WRONG_KEY.name); - await driver.initObjects([MISMATCHED, WRONG_KEY]); + await knexInstance.schema.dropTableIfExists(SINGLE_KEY.name); + await driver.initObjects([MISMATCHED, WRONG_KEY, SINGLE_KEY]); }); 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 driver?.disconnect?.(); }); @@ -691,6 +737,7 @@ function declareMysqlPreflightRefusal(cell: DialectCell): void { beforeEach(async () => { await knexInstance(MISMATCHED.name).delete(); await knexInstance(WRONG_KEY.name).delete(); + await knexInstance(SINGLE_KEY.name).delete(); }); /** @@ -863,18 +910,23 @@ function declareMysqlPreflightRefusal(cell: DialectCell): void { * ⚠️ **[#8621] moved this pin's FIXTURE, and nothing else.** It measures * identity preservation ACROSS a merge on a key the caller never named, and * on {@link MISMATCHED} that call is now refused before it runs — the - * phenomenon is gone from that table, so the pin cannot live there. It runs - * on {@link WRONG_KEY} instead, where MySQL still merges on the wrong key - * (both columns unique, so the named target passes the pre-flight — #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. + * phenomenon is gone from that table, so the pin cannot live there. + * + * ⚠️ **[#8755] moved the fixture again, for the same reason and no other.** + * Naming `email` on {@link WRONG_KEY} is refused now too, so the call that + * used to exhibit the wrong-key merge here is gone as well. The surviving + * shape is the `conflictKeys`-LESS default: no pre-flight has ever probed it + * (the driver's own `['id']`), the minted id cannot collide, and MySQL + * merges on whichever UNIQUE key does — measured on live MySQL 8.0.46 while + * 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. */ 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' }, ['email']); + 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; - await driver.upsert(WRONG_KEY.name, { email: 'other@b.com', tax_id: 'T-1', title: 'second' }, ['email']); + await driver.upsert(WRONG_KEY.name, { email: 'other@b.com', tax_id: 'T-1', title: 'second' }); const merged = (await rows(WRONG_KEY.name))[0]; expect( @@ -892,35 +944,146 @@ function declareMysqlPreflightRefusal(cell: DialectCell): void { }); /** - * [#8755] The scope boundary, pinned so nobody reads #8621 as more than it - * is. `ON DUPLICATE KEY UPDATE` carries no conflict target even when the - * named one IS backed, so a second unique index can still absorb the - * conflict. The pre-flight does not refuse this — the target it was given is - * genuinely backed — and refusing it would mean refusing every - * `conflictKeys` upsert on any MySQL table with more than one unique index, - * an accept-set change far past this card's ruling. + * ✅ **[#8755] The condition #8621 left standing, now refused.** This was a + * characterization pin ("still merges on another unique key when the NAMED + * target is backed") whose failure message said that if it ever went red the + * pin — not the behaviour — was the thing to rewrite. That is this rewrite. + * + * `email` IS backed here, so #8621's arm passes the call. What refuses it is + * the second arm: `uniq_os8621_wrong_key_tax_id` is a UNIQUE key outside the + * named target, `ON DUPLICATE KEY UPDATE` carries no target, and MySQL would + * therefore merge on whichever of the two collided first — measured on live + * MySQL 8.0.46 before the fix as ONE row, merged on `tax_id`, across two + * different values of the key the caller named. * - * Pinned rather than left implicit because the alternative is a reader - * concluding from the pins above that MySQL now honours `conflictKeys` as a - * target. It does not, and this is where that stops being true. + * `code` AND `status`, never a bare `rejects.toThrow()`: an unrelated failure + * (a dead connection, an unknown column) would satisfy a bare throw while the + * accept set had not moved at all. */ - it('[#8755] still merges on another unique key when the NAMED target is backed', async () => { - await driver.upsert(WRONG_KEY.name, { email: 'a@b.com', tax_id: 'T-1', title: 'first' }, ['email']); - // `email` is unique here, so the pre-flight passes the call. The row that - // follows collides on `tax_id` — a different unique index — and MySQL - // merges on it, across two different values of the key the caller named. + it('[#8755] REFUSES the upsert when a second UNIQUE key can absorb the conflict', async () => { const err = await captureError(() => - driver.upsert(WRONG_KEY.name, { email: 'other@b.com', tax_id: 'T-1', title: 'second' }, ['email']), + driver.upsert(WRONG_KEY.name, { email: 'a@b.com', tax_id: 'T-1', title: 'first' }, ['email']), + ); + + expect( + err, + 'MySQL accepted a conflict target another UNIQUE key can absorb — the second arm of the ' + + 'pre-flight did not run (#8755)', + ).not.toBeNull(); + expect(err!.code).toBe(StandardErrorCode.enum.VALIDATION_ERROR); + expect(err!.status).toBe(400); + expect( + await rows(WRONG_KEY.name), + 'the refused upsert still wrote — the second arm is running after the statement, not before it', + ).toHaveLength(0); + }); + + /** + * [#8755] The ruling requires the message to NAME the colliding key and to + * state the way out. Asserted as text because an untested message drifts + * into uselessness — and because the whole reason A was ruled over B is that + * a refusal an author can read beats a merge they cannot see. + */ + it('[#8755] names the second UNIQUE key and both workarounds in the message', async () => { + const err = await captureError(() => + driver.upsert(WRONG_KEY.name, { email: 'a@b.com', tax_id: 'T-1', title: 'first' }, ['email']), + ); + + // The colliding key, by the name an operator will find in SHOW INDEXES — + // and its column, since the name alone is not actionable on a table whose + // indexes were created by hand. + expect(err!.message).toContain('uniq_os8621_wrong_key_tax_id'); + expect(err!.message).toContain('tax_id'); + // The named target, so the sentence says which call is being refused. + expect(err!.message).toContain('"email"'); + expect(err!.message).toContain(WRONG_KEY.name); + // Workaround ①: drop or rename the extra key. Workaround ②: a dialect + // without the limitation, named rather than alluded to. + expect(err!.message).toMatch(/drop(ping)? or renam/i); + expect(err!.message).toMatch(/SQLite and PostgreSQL/); + expect(err!.message).toMatch(/ON DUPLICATE KEY UPDATE/); + // And the primary-key path, which this refusal deliberately leaves open. + expect(err!.message).toMatch(/primary key is unaffected/i); + }); + + /** + * [#8755] The payload contract, on the new arm: schema identifiers are the + * ground truth an operator acts on, row values are not. Same claim #8621's + * `cause` pin makes for the unbacked arm, asserted separately because this + * arm builds a different `cause`. + */ + it('[#8755] keeps row values out of the refusal, and puts the rival keys on `cause`', async () => { + const err = await captureError(() => + driver.upsert( + WRONG_KEY.name, + { email: 'leaked@example.com', tax_id: 'T-9', title: 'secret-title' }, + ['email'], + ), + ); + + 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'); + }); + + /** + * ✅ **[#8755] THE discriminating control.** One unique key besides the + * primary, named by the caller: the common shape, and it must still merge. + * + * Without this case every pin above is satisfied by a pre-flight that + * refuses every `conflictKeys` upsert on MySQL — which is option C + * un-narrowed, the accept-set change the ruling explicitly did not make + * ("the single-key fast path stays untouched"). Narrowness is the entire + * reason A was ruled over C, so it is pinned rather than argued. + */ + it('[#8755] single-unique-key upsert still MERGES — the fast path is untouched', async () => { + await driver.upsert(SINGLE_KEY.name, { email: 'one@b.com', title: 'first' }, ['email']); + const err = await captureError(() => + driver.upsert(SINGLE_KEY.name, { email: 'one@b.com', title: 'second' }, ['email']), ); - expect(err, 'a backed conflict target must not be refused').toBeNull(); - const after = await rows(WRONG_KEY.name); expect( - after, - 'two rows would mean MySQL had started honouring the named target — good news, but it ' + - 'would mean #8755 was fixed and this pin is the one to rewrite', - ).toHaveLength(1); - expect(after[0].email).toBe('other@b.com'); + err, + 'a table whose only UNIQUE key IS the conflict target must never be refused — this is the ' + + 'shape the ruling protects, and refusing it turns A into a blanket ban', + ).toBeNull(); + + const after = await rows(SINGLE_KEY.name); + expect(after).toHaveLength(1); + expect(after[0].title).toBe('second'); + expect(after[0].email).toBe('one@b.com'); + }); + + /** + * [#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. + * + * 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). + */ + it('[#8755] leaves an explicitly named PRIMARY KEY merging, UNIQUE keys or not', async () => { + const err = await captureError(() => + driver.upsert(WRONG_KEY.name, { id: 'os8755_pk', email: 'pk@b.com', tax_id: 'T-4', title: 'first' }, ['id']), + ); + expect(err, 'the primary-key fast path must not be refused').toBeNull(); + + await driver.upsert(WRONG_KEY.name, { id: 'os8755_pk', email: 'pk@b.com', tax_id: 'T-4', title: 'second' }, ['id']); + + const after = await rows(WRONG_KEY.name); + expect(after).toHaveLength(1); + expect(after[0].id).toBe('os8755_pk'); + expect(after[0].title).toBe('second'); }); /** diff --git a/packages/drivers/driver-sql/src/sql-driver.ts b/packages/drivers/driver-sql/src/sql-driver.ts index fcd0dde74c..6d9b681744 100644 --- a/packages/drivers/driver-sql/src/sql-driver.ts +++ b/packages/drivers/driver-sql/src/sql-driver.ts @@ -1068,6 +1068,104 @@ function refuseUnbackedConflictTarget(object: string, mergeKeys: string[], cause return err; } +/** + * [#8755] A `conflictKeys` upsert whose target IS backed, on a MySQL table that + * carries another UNIQUE key — the one that key can absorb the conflict instead. + * + * # A different condition from the one above, so a different sentence + * + * #5240 is "one condition, one wording", not "one function, one wording". + * {@link refuseUnbackedConflictTarget} answers *no index backs your target*, and + * every remedy it names is about creating that index. Here the index exists and + * the target is perfectly well formed; what cannot be honoured is the TARGETING, + * because `ON DUPLICATE KEY UPDATE` carries no target and MySQL merges on + * whichever unique key the row collides with first. Reusing the unbacked wording + * would tell an author to declare a `unique: true` they already declared, and + * send them to re-run a schema sync that would change nothing. + * + * Measured on live MySQL 8.0.46, `email` and `tax_id` both `unique: true`, the + * caller naming `email` — reproduced for this card through the same knex + + * `mysql2` path `upsert` takes: + * + * ``` + * upsert({email:'a@b.com', tax_id:'T-1', title:'first'}, ['email']) -> seeded + * upsert({email:'other@b.com', tax_id:'T-1', title:'second'}, ['email']) + * -> RESOLVED, ONE row: merged on `tax_id`, across two different `email` values. + * ``` + * + * # Why `VALIDATION_ERROR` / 400 and not `NOT_IMPLEMENTED` / 501 + * + * The #5907 classifier this file already applies twice + * ({@link uncompilableAggregateFunctionError}, {@link refuseDateBucketedGroupBy}) + * sorts a refusal by *what the caller would have to change*: a CAPABILITY gap in + * the backend — the request is spelled correctly and no schema anywhere makes it + * work — is 501, and a request that does not validate against the target it was + * given is 400. This one is the second: it is conditional on the TABLE, not on + * the dialect. The identical statement against the identical MySQL server is + * honoured the moment the table carries a single unique key, which is why the + * remedy in the message is a schema change and not "wait for the backend to + * implement it". 400 also keeps it off the retry-inviting 5xx band (nothing here + * is transient — the next attempt fails identically) and keeps the sentence + * itself on the wire: `@objectstack/rest` withholds the message body of any 5xx, + * and this message is the deliverable — the ruling requires it to name the + * colliding key and the way out. + * + * # What is NOT refused, and why the PRIMARY KEY never counts as a rival + * + * The primary key is excluded from the keys that trigger this refusal, in both + * directions, and neither is an oversight: + * + * - **As a rival** — every table this driver creates carries an `id` PRIMARY + * KEY, so counting it would refuse *every* `conflictKeys` upsert on MySQL. + * The ruling's "the single-key fast path stays untouched" would then describe + * nothing, and the remedy this message states ("drop or rename the extra + * key") is not available for a primary key — a refusal whose only stated way + * 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. + */ +function refuseAmbiguousConflictTarget( + object: string, + mergeKeys: string[], + tableName: string, + rivals: PhysicalIndex[], +): Error { + const keys = mergeKeys.map((k) => `"${k}"`).join(', '); + const named = rivals.map((i) => `${i.name}(${i.columns.join(', ')})`).join(', '); + const err = new Error( + `Cannot upsert into "${object}" on conflict keys (${keys}): a UNIQUE key other than the ` + + `conflict target exists on "${tableName}" — ${named} — and this backend is MySQL, whose only ` + + `merge statement is ON DUPLICATE KEY UPDATE. That statement carries no conflict target, so ` + + `the merge lands on whichever UNIQUE key the row collides with FIRST: the conflict target is ` + + `backed, but a collision on ${named} would silently merge a row the caller never targeted, ` + + `across two different values of the named key. Fix by dropping or renaming the extra UNIQUE ` + + `key(s) so the conflict target is the only one on the table, or by running this object on a ` + + `dialect that honours the target — SQLite and PostgreSQL compile ON CONFLICT (...), which ` + + `merges on the named key alone. Upserting on the primary key is unaffected: supply "id" and ` + + `omit conflictKeys.`, + ) as Error & { code?: string; status?: number; cause?: unknown }; + err.code = StandardErrorCode.enum.VALIDATION_ERROR; + err.status = 400; + // The introspected keys, exactly as the unbacked refusal attaches them: schema + // identifiers are the ground truth an operator acts on, and row values are not. + err.cause = new Error( + `conflict target (${mergeKeys.join(', ')}) on "${tableName}" is backed, but these UNIQUE keys ` + + `can absorb the conflict instead: ${named}`, + ); + return err; +} + /** * [#5158] A `FilterArray` reached the driver unlowered. * @@ -3375,7 +3473,7 @@ export class SqlDriver implements IDataDriver { /** * [#8621] PHYSICAL key indexes per table (tableName → the PRIMARY KEY and - * UNIQUE indexes that actually exist), for {@link assertConflictTargetBacked}. + * UNIQUE indexes that actually exist), for {@link assertConflictTargetHonoured}. * * Deliberately not `managedObjectIndexes`, which records what metadata * DECLARES. The pre-flight answers "can this conflict target resolve to a key @@ -5060,9 +5158,14 @@ export class SqlDriver implements IDataDriver { } /** - * [#8621] Refuse an upsert whose `conflictKeys` no PRIMARY KEY or UNIQUE index - * backs — BEFORE the statement is compiled, on the dialect where the server - * will never say so itself. + * Refuse an upsert whose named `conflictKeys` this dialect will not honour as + * the merge target — BEFORE the statement is compiled, on the dialect where + * the server will never say so itself. Two conditions, one introspection: + * + * - **[#8621] unbacked** — no PRIMARY KEY or UNIQUE index covers the named + * target, so there is no merge target at all; + * - **[#8755] ambiguous** — the target IS covered, but another UNIQUE key on + * the table can absorb the conflict instead of it. * * # Why a pre-flight exists at all, when a refusal already did * @@ -5108,13 +5211,16 @@ export class SqlDriver implements IDataDriver { * "the compiler drops the conflict target, so the server can never be asked", * and MySQL is the dialect that meets it. * - * # Why it refuses only what it can PROVE is unbacked + * # Why it refuses only what it can PROVE * * A false refusal breaks a working merge, which is the expensive direction — * the same asymmetry `isUnbackedConflictTargetError` records. So every * uncertain answer proceeds: a failed introspection, a table with no keys at * all (see {@link introspectKeyIndexes}), and a possibly stale cache, which is - * re-read from the database before any refusal is thrown. + * re-read from the database before any refusal is thrown. The stale-cache leg + * covers BOTH verdicts and in opposite directions — an index created since the + * read would make a backed target read as unbacked, and one DROPPED since the + * read would make a rival key exist that no longer does. * * The comparison is against the conflict target as EMITTED. `onConflict()` * receives `mergeKeys` verbatim — the write column map is applied to the row, @@ -5124,11 +5230,12 @@ export class SqlDriver implements IDataDriver { * dialects infer an arbiter index, and the only reading under which a * composite key means anything). * - * ⚠️ **This closes the unbacked-target hole, and not the whole MySQL gap.** + * # [#8755] The SECOND condition this pre-flight answers, added later + * * `ON DUPLICATE KEY UPDATE` carries no target even when the target IS backed, * so a table with a second unique index can still merge on a key the caller - * never named. Measured here on live MySQL 8.0.46, both columns unique, - * caller naming `email`: + * never named. Measured on live MySQL 8.0.46, both columns unique, caller + * naming `email` — re-measured for #8755 before it was fixed: * * ``` * upsert({email:'a@b.com', tax_id:'T-1', title:'first'}, ['email']) -> seeded @@ -5136,29 +5243,71 @@ export class SqlDriver implements IDataDriver { * -> ONE row, merged on `tax_id`. The named target was backed the whole time. * ``` * - * That is a different condition with a different fix and is filed separately - * (#8755); it is deliberately NOT smuggled in here, because refusing it would - * mean refusing every `conflictKeys` upsert on any MySQL table carrying more - * than one unique index — an accept-set change far past what this card rules. - */ - protected async assertConflictTargetBacked(object: string, mergeKeys: string[]): Promise { + * #8621 deliberately left that standing — it is a different condition, and + * refusing it narrows the accept set past what that card ruled. #8755's + * maintainer ruling then took it, choosing refusal (its option A) over + * emulating a target-honouring statement on MySQL (its option B, rejected: + * a SELECT-then-branch with a race window and an execution path unlike every + * other dialect — *"dressing 'the dialect can't' up as 'it did'"*). + * + * The two conditions share this method because they share the QUESTION — + * "what keys does this table physically carry?" — asked once, cached once + * ({@link physicalKeyIndexes}), invalidated in one place. They do NOT share an + * answer: see {@link refuseAmbiguousConflictTarget} for why the second gets + * its own sentence, and for why the PRIMARY KEY is never a rival key. + * + * ⚠️ **Renamed from `assertConflictTargetBacked` when the second condition + * landed.** "Backed" was the whole question while #8621 was the whole method; + * 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 { // The table the statement will actually hit — same resolution `getBuilder` // performs for the insert, rotation shard included. const target = this.rotationWriteTarget(object) ?? object; const tableName = this.physicalTableByObject[target] ?? target; const wanted = new Set(mergeKeys); - const backs = (keys: PhysicalIndex[]): boolean => - keys.some((i) => i.columns.length === wanted.size && i.columns.every((c) => wanted.has(c))); + const covers = (i: PhysicalIndex): boolean => + i.columns.length === wanted.size && i.columns.every((c) => wanted.has(c)); + + type Verdict = + | { kind: 'honoured' } + | { kind: 'unbacked' } + | { kind: 'ambiguous'; 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' }; + // 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. + const rivals = keys.filter((i) => i.primary !== true && !covers(i)); + return rivals.length > 0 ? { kind: 'ambiguous', rivals } : { kind: 'honoured' }; + }; let keys = await this.introspectKeyIndexes(tableName); - if (keys !== null && keys.length > 0 && !backs(keys)) { + if (keys !== null && keys.length > 0 && judge(keys).kind !== 'honoured') { // A cache filled before the index was created is the only way a real key - // can be missing here, and it is cheaper to re-read once on the refusing - // path than to risk refusing a call the database would have merged. + // 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 + // cheaper to re-read once on the refusing path than to risk refusing a + // call the database would have merged — the same asymmetry, now covering + // both verdicts. keys = await this.introspectKeyIndexes(tableName, { fresh: true }); } - if (keys === null || keys.length === 0 || backs(keys)) return; + if (keys === null || keys.length === 0) return; + + const verdict = judge(keys); + if (verdict.kind === 'honoured') return; + + if (verdict.kind === 'ambiguous') { + throw refuseAmbiguousConflictTarget(object, mergeKeys, tableName, verdict.rivals); + } // The introspected keys stand where the server's sentence stands on the // other two dialects: the caller-visible message is the shared wording, and @@ -5191,8 +5340,8 @@ export class SqlDriver implements IDataDriver { const mergeKeys = conflictKeys && conflictKeys.length > 0 ? conflictKeys : ['id']; - // [#8621] Pre-flight the conflict target — see - // {@link assertConflictTargetBacked} for the mechanism and why it is + // [#8621, #8755] 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 @@ -5206,7 +5355,7 @@ export class SqlDriver implements IDataDriver { // 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.assertConflictTargetBacked(object, mergeKeys); + await this.assertConflictTargetHonoured(object, mergeKeys); } // #6943. Measured: `upsert` does NOT share `bulkCreate`'s shape. It is From 967916b0573450ced97cf63e64d767644f1d97e3 Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 15 Aug 2026 03:24:07 +0000 Subject: [PATCH 2/2] docs(drivers): record the MySQL upsert conflict-target limit where driver differences live (#8755) The other half of #8755's ruling, which folded in option C's documentation: the dialect limit is now stated on the Database Drivers page beside the PostgreSQL, MongoDB and SQLite sections, which had no MySQL section at all. States what was measured rather than what was assumed: SQLite and PostgreSQL compile `ON CONFLICT (email)`, so a collision on another unique key raises `UNIQUE constraint failed: ...tax_id` and leaves the seeded row untouched; MySQL compiles `ON DUPLICATE KEY UPDATE`, which carries no target at all. (The card's body says the identical call "inserts a second row" on those two dialects -- its own next clause says otherwise, and the measurement agrees with the next clause.) Carries the accept-set table per call shape, both workarounds, and -- explicitly rather than by omission -- the residue this card does not refuse: the `conflictKeys`-less default and an explicitly named primary key still merge on whichever unique key collides. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01XeQRiAa7vYRVX5Fog7Zby8 --- .../mysql-upsert-ambiguous-conflict-target.md | 29 +++++++ content/docs/data-modeling/drivers.mdx | 80 +++++++++++++++++++ 2 files changed, 109 insertions(+) create mode 100644 .changeset/mysql-upsert-ambiguous-conflict-target.md diff --git a/.changeset/mysql-upsert-ambiguous-conflict-target.md b/.changeset/mysql-upsert-ambiguous-conflict-target.md new file mode 100644 index 0000000000..16149d5518 --- /dev/null +++ b/.changeset/mysql-upsert-ambiguous-conflict-target.md @@ -0,0 +1,29 @@ +--- +"@objectstack/driver-sql": minor +--- + +fix(driver-sql): refuse a MySQL upsert whose named conflict target another UNIQUE key can absorb (#8755) + +`ON DUPLICATE KEY UPDATE` — the only merge statement MySQL compiles — carries no +conflict target, so the merge lands on whichever UNIQUE key the row collides with +first. `#8621` closed the half where nothing backed the named target; this closes +the half where the target IS backed and a *second* UNIQUE key absorbs the +conflict instead. + +Measured on live MySQL 8.0.46, `email` and `tax_id` both `unique: true`, the +caller naming `email`: the second upsert merged on `tax_id`, across two different +values of the named key, leaving one row and no error. The identical call on +SQLite and PostgreSQL raises `UNIQUE constraint failed: …tax_id` and leaves the +seeded row untouched. + +**Accept-set change, MySQL only.** An `upsert(object, data, conflictKeys)` naming +a non-primary target on a table that carries any other UNIQUE key is now refused +before the statement is compiled — `code: 'VALIDATION_ERROR'`, `status: 400`, +nothing written and no auto-number reserved. The message names the colliding +index and both workarounds: drop or rename the extra UNIQUE key, or run the +object on a dialect that honours the target. + +Deliberately unchanged: a table whose only UNIQUE key IS the conflict target (the +common shape) merges exactly as before, as do the `conflictKeys`-less default and +an explicitly named primary key. The MySQL dialect limit and that residue are +documented under *Database Drivers → MySQL*. diff --git a/content/docs/data-modeling/drivers.mdx b/content/docs/data-modeling/drivers.mdx index d11aaf0520..996e72c66c 100644 --- a/content/docs/data-modeling/drivers.mdx +++ b/content/docs/data-modeling/drivers.mdx @@ -282,6 +282,86 @@ is moved into the client's URL slot (`connectionString` for pg, `uri` for mysql2) before reaching Knex. This affects only what Knex receives — the config you passed is preserved as-is on the driver. +## MySQL (via `@objectstack/driver-sql`) + +```bash +pnpm add @objectstack/driver-sql mysql2 +``` + +```typescript +import { SqlDriver } from '@objectstack/driver-sql'; + +new SqlDriver({ + client: 'mysql2', + connection: 'mysql://admin:secret@db.example.com:3306/myapp', +}); +``` + +Everything on this page's PostgreSQL section applies unchanged — the same +`SqlDriver`, the same connect-timeout defaults, the same tenant scoping. One +behaviour genuinely differs, and it is a limit of the dialect rather than of this +driver. + +### `upsert` conflict targets: the one dialect limit + + +On MySQL, `upsert(object, data, conflictKeys)` cannot promise that the merge +happens on `conflictKeys`. Where the table carries a UNIQUE key *outside* the +named target, the driver **refuses the call** rather than let it merge into a row +the caller never targeted +([#8755](https://github.com/objectstack-ai/objectstack/issues/8755)). + + +The same `upsert` call compiles differently per dialect, and only two of the +three can carry a conflict target at all: + +| Dialect | Compiles to | Honours the named target? | +| :--- | :--- | :--- | +| SQLite / PostgreSQL | `INSERT … ON CONFLICT (email) DO UPDATE …` | **Yes.** The named index is the arbiter. A collision on any *other* unique key raises a unique violation — a legible error. | +| MySQL | `INSERT … ON DUPLICATE KEY UPDATE …` | **No.** The statement carries no target at all, so the merge lands on whichever UNIQUE key the row collides with first. | + +Measured on MySQL 8.0.46, a table with `email` and `tax_id` both declared +`unique: true`, the caller naming `email`: + +```text +upsert({ email: 'a@b.com', tax_id: 'T-1', title: 'first' }, ['email']) -> seeded +upsert({ email: 'other@b.com', tax_id: 'T-1', title: 'second' }, ['email']) + -> ONE row. `email` did not collide; `tax_id` did, and MySQL merged on it — + rewriting a row whose `email` the caller never asked to touch. +``` + +The identical second call on SQLite and PostgreSQL fails with +`UNIQUE constraint failed: …tax_id` and leaves the seeded row untouched. + +So on MySQL the driver checks the table's physical keys *before* compiling, and +refuses what it cannot honour — with `code: 'VALIDATION_ERROR'` and `status: 400`, +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, ['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)). | + +Two ways out, both stated in the error message: + +1. **Drop or rename the extra UNIQUE key** so the conflict target is the only one + on the table — appropriate when the second key was incidental. +2. **Run the object on a dialect that honours the target** (SQLite, PostgreSQL) — + appropriate when both keys are genuine business constraints, since one of them + must otherwise be given up. + + +**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. + + ## MongoDB Configuration properties for the MongoDB driver.