From 3751cdd017d0727a6db5fa1acfa24ce03e051822 Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 14 Aug 2026 18:40:16 +0000 Subject: [PATCH 1/2] fix(driver-sql): refuse an unbacked upsert conflict target on MySQL before compiling (#8621) SQLite and Postgres already refuse a `conflictKeys` upsert whose target no PRIMARY KEY or UNIQUE index backs: the server cannot infer an arbiter index and raises, and `isUnbackedConflictTargetError` turns that into a VALIDATION_ERROR / 400. On MySQL the same call resolved. knex compiles `onConflict(...).merge(...)` to `ON DUPLICATE KEY UPDATE`, which takes no conflict target at all, so the named keys are dropped before the statement leaves the process and the server is never asked to find an index for them. The reactive catch has nothing to catch. Measured on live MySQL 8.0.46 (`email` unnamed by any index, `tax_id` carrying the only unique key): the call merged on `tax_id`, across two different `email` values, and separately left two rows sharing the `email` the caller had asked to merge on. A wrong write, with no error. `upsert` now consults the table's physical PRIMARY KEY and UNIQUE indexes before compiling on MySQL and refuses with the wording, `code` and `status` the other two dialects already answer (#5240 -- one condition, one wording). It runs on MySQL alone by design: SQLite and Postgres refuse this from the server, with the server's own sentence attached as `cause`, and no pre-flight can reconstruct that. Only what can be PROVEN unbacked is refused -- a failed introspection, a table reporting no keys at all, and a possibly stale cache all proceed, the last after a fresh re-read. Not closed by this change, and filed as #8755: `ON DUPLICATE KEY UPDATE` carries no target even when the named one IS backed, so a second unique index can still absorb the conflict on MySQL. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01XeQRiAa7vYRVX5Fog7Zby8 --- ...er-upsert-conflict-target-dialects.test.ts | 351 ++++++++++++++---- packages/drivers/driver-sql/src/sql-driver.ts | 200 ++++++++++ 2 files changed, 470 insertions(+), 81 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 a5376c7eb8..4a568935ba 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 @@ -48,13 +48,18 @@ * ⚠️ What MySQL does INSTEAD of refusing was left un-asserted by #8567 — * correctly, since nobody had watched a MySQL server do it and an assertion * written from the compiled SQL alone would be exactly the inferred evidence - * that card existed to stop accepting. **[#8592] has now observed it on a live - * MySQL 8.0.46**, and the last section of this file pins what the server - * actually did: it merges on a unique key the caller never named, rewrote the - * merged row's primary key, and does not merge on the key it was given. Those - * pins describe a defect and are marked as such — the fix that makes them go - * red is #8621, deliberately a separate card because it moves MySQL's accept - * set. + * that card existed to stop accepting. **[#8592] observed it on a live MySQL + * 8.0.46**: it merged on a unique key the caller never named, rewrote the merged + * row's primary key, and did not merge on the key it was given. + * + * ✅ **[#8621] has since closed that gap from the other end.** MySQL cannot be + * made to refuse this (the target never reaches the server), so the driver + * refuses it first: `upsert` consults the table's physical PRIMARY KEY and + * 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. * * ✅ **[#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 @@ -519,17 +524,34 @@ describe('[#8567] MySQL: `onConflict().merge()` compiles the conflict target awa // ───────────────────────────────────────────────────────────────────────── /** - * ⚠️⚠️ **These pins record a DEFECT, not a contract.** Every assertion below is - * a characterization of what MySQL 8.0 does today, written down so it stops - * being an inference. Do not read any of them as behaviour worth keeping: when - * the pre-flight refusal lands (#8621 — deliberately NOT this card, it moves - * MySQL's accept set and is a `minor` with its own argument), these tests go red - * and must be REWRITTEN to the refusal, not relaxed to keep them green. + * ✅ **[#8621] has landed: these are CONTRACT pins now, not characterization.** + * #8592 wrote this section as a record of what MySQL 8.0 does with a conflict + * target no index backs, and said in as many words that the pins would go red + * when the pre-flight refusal landed and must then be rewritten to the refusal + * rather than relaxed. That is this rewrite. `SqlDriver.upsert` now consults the + * table's physical PRIMARY KEY and UNIQUE indexes BEFORE compiling on MySQL, and + * answers the same `VALIDATION_ERROR` / 400 sentence SQLite and Postgres already + * answer for the same mistake (#5240 — one condition, one wording). + * + * Why a pre-flight and not the existing catch: that catch classifies an error + * the SERVER raised, and on MySQL no error is ever raised — knex compiles the + * conflict target away entirely (the compile pin above proves it with no server + * at all), so there is nothing to classify. The full mechanism argument, and why + * the pre-flight runs on MySQL alone, is on `assertConflictTargetBacked` in + * `sql-driver.ts`. The negative control for "MySQL alone" is in the sweep at the + * top of this file: on SQLite and Postgres the refusal still arrives with the + * SERVER's own sentence attached as `cause`, which only the reactive path can + * produce. * - * ✅ **One exception, as of [#8622]: the primary-key pin is now a CONTRACT.** - * The rewrite it characterized was the driver's own merge set, not the server's - * doing, and it is fixed — so that pin asserts preservation and is not #8621's - * to move. The rest of this suite is unchanged characterization. + * ✅ **[#8622]'s primary-key pin was already a contract before this card**, and + * it stays one, with its assertion untouched. What moved is the FIXTURE it runs + * on, of necessity: it pinned identity preservation across a merge on a key the + * caller never named, and on the mismatched table that call is now REFUSED, so + * the phenomenon it measures no longer occurs there. It runs on + * {@link WRONG_KEY} below instead — the table where a wrong-key merge still + * happens after this card (both business columns unique, so the named target is + * backed and the pre-flight passes it; see #8755). Same claim, same strength, + * same failure message; a fixture that can still exhibit the behaviour. * * # How this was measured * @@ -582,16 +604,28 @@ describe('[#8567] MySQL: `onConflict().merge()` compiles the conflict target awa * NOT MySQL-specific and did not need a live MySQL to find: the same rewrite * reproduced on SQLite and live Postgres 16.13 against a *backed* conflict * target, because `id` was in the merge set this driver builds before any server - * is involved. The other two facts stand exactly as recorded. + * is involved. + * + * ✅ **[#8621] then removed facts one and two from this table**, by refusing the + * call before it is compiled. The transcript above stays verbatim for the same + * reason: it is what was measured on 2026-08-14, and what the refusal now + * prevents. Re-measured on the same live MySQL 8.0.46 while implementing #8621 — + * every line of it reproduced against `main` before the fix, and the suite below + * is the after. * * # Reverse verification — direction predicted BEFORE running it * - * Predicted: deleting the `measure` argument from `declareDialectCell` below - * cannot reproduce the original one-way gap, because `measure` is a REQUIRED - * parameter — the failure is a TypeScript error at the call site rather than a - * silently absent suite. That is the point of making the testkit helper total: - * the hole #8592 found is no longer expressible. Measured; it matched (tsc: - * `Expected 3 arguments, but got 2`). + * Predicted, with the fix committed first and then the `this.isMysql` guard on + * the pre-flight call site inverted to `!this.isMysql`: the four MySQL pins + * below go RED (the refusal stops arriving, the wrong-key merge and the + * duplicate-`email` rows come back), and — the half that makes it a *direction* + * rather than a tautology — the SQLite sweep at the top of this file goes red + * TOO, but differently: its refusal pins stay green (the reactive catch still + * answers) while its `cause` pin fails, because the pre-flight now answers first + * and its cause is the driver's own introspection sentence, not the server's + * `insert into … on conflict …`. That asymmetry is the evidence the MySQL-only + * gate is doing something: one dialect loses the refusal, the other loses only + * the server's text. Measured; it matched. */ const MYSQL_CELL = DIALECT_CELLS.find((c) => c.id === 'mysql')!; @@ -605,19 +639,36 @@ 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. + */ +const WRONG_KEY = { + name: 'os8621_wrong_key', + fields: { + email: { type: 'string', unique: true }, + tax_id: { type: 'string', unique: true }, + title: { type: 'string' }, + }, +} as any; + declareDialectCell( MYSQL_CELL, - 'unbacked conflict-target refusal (MySQL merges on the wrong key instead)', - declareMysqlObservedBehaviour, + 'unbacked conflict-target refusal (pre-flight, MySQL)', + declareMysqlPreflightRefusal, ); -function declareMysqlObservedBehaviour(cell: DialectCell): void { - describe(`[#8592] SqlDriver.upsert — what MySQL does instead of refusing (${cell.label})`, () => { +function declareMysqlPreflightRefusal(cell: DialectCell): void { + describe(`[#8621] SqlDriver.upsert — MySQL refuses an unbacked conflict target (${cell.label})`, () => { let driver: SqlDriver; let knexInstance: any; - const rows = async (): Promise => { - const found = await driver.find(MISMATCHED.name, {}); + const rows = async (object: string = MISMATCHED.name): Promise => { + const found = await driver.find(object, {}); return [...found].sort((a: any, b: any) => String(a.tax_id).localeCompare(String(b.tax_id))); }; @@ -625,11 +676,13 @@ function declareMysqlObservedBehaviour(cell: DialectCell): void { driver = new SqlDriver(cell.config()); knexInstance = (driver as any).knex; await knexInstance.schema.dropTableIfExists(MISMATCHED.name); - await driver.initObjects([MISMATCHED]); + await knexInstance.schema.dropTableIfExists(WRONG_KEY.name); + await driver.initObjects([MISMATCHED, WRONG_KEY]); }); afterAll(async () => { await knexInstance?.schema.dropTableIfExists(MISMATCHED.name).catch(() => {}); + await knexInstance?.schema.dropTableIfExists(WRONG_KEY.name).catch(() => {}); await driver?.disconnect?.(); }); @@ -637,42 +690,152 @@ function declareMysqlObservedBehaviour(cell: DialectCell): void { // so each case starts from an empty table rather than from its neighbour. beforeEach(async () => { await knexInstance(MISMATCHED.name).delete(); + await knexInstance(WRONG_KEY.name).delete(); }); - it('does NOT refuse the conflict target that SQLite and Postgres refuse', async () => { + /** + * ① of the three consequences #8592 recorded: the identical call that is + * `VALIDATION_ERROR` / 400 on SQLite and Postgres RESOLVED here. + * + * `code` AND `status`, never a bare `rejects.toThrow()`: a driver that threw + * some other error for this input — an unknown column, a dead connection — + * would satisfy a bare throw assertion while the accept set had not moved at + * all. The negative assertions name what the refusal replaces on this cell, + * which is not an error code but the absence of one. + */ + it('refuses the unbacked conflict target, with the same `code` and `status` as the other dialects', async () => { const err = await captureError(() => driver.upsert(MISMATCHED.name, { email: 'a@b.com', tax_id: 'T-1', title: 'first' }, ['email']), ); - // Not a bare `resolves` — the sweep above proves this exact call is a - // `VALIDATION_ERROR`/400 on the other two dialects, and THAT asymmetry is - // the finding. If this ever starts throwing, the pre-flight refusal has - // landed and this whole suite is what needs rewriting. expect( err, - 'MySQL accepted an unbacked conflict target here when this was measured — a change ' + - 'means the accept set moved (#8621), and these characterization pins are now stale', - ).toBeNull(); + 'MySQL accepted an unbacked conflict target — the pre-flight did not run, or judged a ' + + 'target backed that no PRIMARY KEY or UNIQUE index covers (#8621)', + ).not.toBeNull(); + expect(err!.code).toBe(StandardErrorCode.enum.VALIDATION_ERROR); + expect(err!.status).toBe(400); + expect(err!.message).toMatch(new RegExp(MISMATCHED.name)); + expect(err!.message).toMatch(/email/); + expect(err!.message).toMatch(/unique/i); + }); + + /** + * #5240 — one condition, one wording, and the sentence must not name an + * engine. This is the assertion that makes the card's claim ("all three + * dialects answer the same sentence for the same mistake") checkable: it is + * the identical string the sweep at the top of this file asserts on SQLite + * and Postgres, with only the object name differing. + */ + it('answers the same sentence SQLite and Postgres answer — no dialect named', async () => { + const err = await captureError(() => + driver.upsert(MISMATCHED.name, { email: 'a@b.com', tax_id: 'T-1', title: 'first' }, ['email']), + ); + + expect(err!.message.split('. ')[0] + '.').toBe( + `Cannot upsert into "${MISMATCHED.name}" on conflict keys ("email"): no PRIMARY KEY or UNIQUE ` + + 'index backs them, so the merge target does not exist and the database refuses the statement.', + ); + expect(err!.message).not.toMatch(/SQLite|Postgres|MySQL/i); }); - it('MERGES on a unique key the caller never named — a wrong write, with no error', async () => { - await driver.upsert(MISMATCHED.name, { email: 'a@b.com', tax_id: 'T-1', title: 'first' }, ['email']); - const seeded = await rows(); - expect(seeded).toHaveLength(1); + /** + * ② and ③ of #8592's consequences, killed at the root: the refusal happens + * BEFORE the statement is compiled, so nothing is written at all. + * + * This is the assertion that distinguishes a pre-flight from a post-hoc + * classification. A refusal thrown after the write would satisfy every + * envelope pin above while the wrong row sat in the table — which is + * precisely the shape of the defect, since MySQL's own answer was a + * successful wrong write. + */ + it('writes NOTHING — the refusal lands before the statement is compiled', async () => { + await captureError(() => + driver.upsert(MISMATCHED.name, { email: 'a@b.com', tax_id: 'T-1', title: 'first' }, ['email']), + ); + + expect( + await rows(), + 'the refused upsert still inserted a row — the pre-flight is running after the write, ' + + 'not before it', + ).toHaveLength(0); + }); - // Same `tax_id` (the unique index), DIFFERENT `email` (the named target). - // A merge keyed on `email` cannot match; a merge keyed on `tax_id` does. + /** + * ③, stated as the observable #8592 named: two rows sharing the `email` the + * caller asked to merge on. It is gone because the call is REFUSED — not + * because merging changed — and this case asserts both halves so a future + * change that silently starts merging on `email` cannot pass it either. + */ + it('cannot produce duplicates on the named key — both calls are refused', async () => { + const first = await captureError(() => + driver.upsert(MISMATCHED.name, { email: 'a@b.com', tax_id: 'T-1', title: 'first' }, ['email']), + ); + const second = await captureError(() => + driver.upsert(MISMATCHED.name, { email: 'a@b.com', tax_id: 'T-2', title: 'second' }, ['email']), + ); + + expect(first!.code).toBe(StandardErrorCode.enum.VALIDATION_ERROR); + expect(second!.code).toBe(StandardErrorCode.enum.VALIDATION_ERROR); + expect( + await rows(), + 'this is the two-rows-on-one-email observable #8592 measured; after #8621 the table must ' + + 'be empty, because neither call was ever executed', + ).toHaveLength(0); + }); + + /** + * The payload contract the refusal already keeps on the other two dialects, + * asserted here because this cell's `cause` is BUILT rather than caught: the + * pre-flight has no server error to attach, so it attaches the introspected + * keys instead. Schema identifiers are the ground truth an operator acts on + * — the MySQL counterpart of the server sentence — and row values are not. + */ + it('keeps row values out of the message, and puts the introspected keys on `cause`', async () => { const err = await captureError(() => - driver.upsert(MISMATCHED.name, { email: 'other@b.com', tax_id: 'T-1', title: 'second' }, ['email']), + driver.upsert(MISMATCHED.name, { email: 'leaked@example.com', tax_id: 'T-9', title: 'secret-title' }, ['email']), ); - expect(err).toBeNull(); + + 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).toMatch(/uniq_os8592_mismatched_tax_id/); + expect(causeText).toMatch(/PRIMARY/); + expect(causeText).not.toContain('leaked@example.com'); + expect(causeText).not.toContain('secret-title'); + }); + + /** + * The control that stops "refuse more" from passing trivially: a conflict + * target a UNIQUE index really does back must still merge, on the same + * table, through the same pre-flight. Without it, a pre-flight that refused + * every `conflictKeys` upsert would satisfy every pin above while having + * destroyed the capability the driver exists to provide. + */ + it('MERGES when a declared unique index does back the conflict target', async () => { + await driver.upsert(MISMATCHED.name, { email: 'a@b.com', tax_id: 'T-1', title: 'first' }, ['tax_id']); + await driver.upsert(MISMATCHED.name, { email: 'a@b.com', tax_id: 'T-1', title: 'second' }, ['tax_id']); const after = await rows(); - expect( - after, - 'two rows would mean MySQL treated these as distinct; one means it merged them on `tax_id`', - ).toHaveLength(1); - expect(after[0].email).toBe('other@b.com'); + expect(after, 'the backed conflict target must still merge').toHaveLength(1); + expect(after[0].title).toBe('second'); + }); + + /** + * The second control: the PRIMARY KEY, named EXPLICITLY. The pre-flight only + * runs when the caller supplies `conflictKeys`, so this is the case that + * proves the primary key is recognised by introspection rather than skipped + * by the default-path short circuit below. + */ + it('accepts the primary key as an explicit conflict target', async () => { + await driver.upsert(MISMATCHED.name, { id: 'os8621_pk', email: 'pk@b.com', tax_id: 'T-5', title: 'first' }, ['id']); + await driver.upsert(MISMATCHED.name, { id: 'os8621_pk', email: 'pk@b.com', tax_id: 'T-5', title: 'second' }, ['id']); + + const after = await rows(); + expect(after).toHaveLength(1); + expect(after[0].id).toBe('os8621_pk'); expect(after[0].title).toBe('second'); }); @@ -687,27 +850,32 @@ function declareMysqlObservedBehaviour(cell: DialectCell): void { * id = values(id)` wrote the LOSING insert's fresh id over the stored row. * `id` is now insert-only (`insertOnlyUpsertColumns`), on every dialect. * - * ⚠️ **Un-run where it was written.** The #8622 container could raise SQLite - * and Postgres 16.13 but not MySQL — no `mysqld` — so the id-preservation - * assertion below was measured on those two cells (in the sweep above, whose - * pins are the same claim on a *backed* target) and is carried here for the - * MySQL cell on the driver-level argument: the merge set is built in this - * process, before any dialect sees a statement. CI's - * `Temporal Conformance (live PG + MySQL)` job is the first runner to - * actually execute it. If it is red there, the driver-level reasoning is - * wrong for MySQL specifically and that is worth a card of its own — do not - * relax it back to `not.toBe` without one. + * ⚠️ **Un-run where it was written, and RUN here.** The #8622 container + * could raise SQLite and Postgres 16.13 but not MySQL — no `mysqld` — so + * this assertion was carried for the MySQL cell on the driver-level argument + * (the merge set is built in this process, before any dialect sees a + * statement) and its docblock named CI as the first runner that would + * actually execute it. #8621's container raised MySQL 8.0.46 for real and + * executed it: **green**, the driver-level reasoning holds on MySQL too. The + * standing instruction survives unchanged — if CI ever disagrees, that is a + * card of its own; do not relax this back to `not.toBe` without one. * - * The two pins around it are untouched: MySQL still merges on a key the - * caller never named, and still does not merge on the key it was given. - * Those are #8621's to move, and this card deliberately does not. + * ⚠️ **[#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. */ it('KEEPS the surviving row’s primary key, even merging on that wrong key', async () => { - await driver.upsert(MISMATCHED.name, { email: 'a@b.com', tax_id: 'T-1', title: 'first' }, ['email']); - const seededId = (await rows())[0].id; + await driver.upsert(WRONG_KEY.name, { email: 'a@b.com', tax_id: 'T-1', title: 'first' }, ['email']); + const seededId = (await rows(WRONG_KEY.name))[0].id; - await driver.upsert(MISMATCHED.name, { email: 'other@b.com', tax_id: 'T-1', title: 'second' }, ['email']); - const merged = (await rows())[0]; + await driver.upsert(WRONG_KEY.name, { email: 'other@b.com', tax_id: 'T-1', title: 'second' }, ['email']); + const merged = (await rows(WRONG_KEY.name))[0]; expect( merged.id, @@ -723,23 +891,44 @@ function declareMysqlObservedBehaviour(cell: DialectCell): void { expect(merged.email).toBe('other@b.com'); }); - it('does NOT merge on the key it WAS told to merge on — duplicates on `email`', async () => { - await driver.upsert(MISMATCHED.name, { email: 'a@b.com', tax_id: 'T-1', title: 'first' }, ['email']); - // Same `email` (the named merge key), different `tax_id` (nothing unique - // collides). The caller asked for a merge on `email`; it does not happen. - await driver.upsert(MISMATCHED.name, { email: 'a@b.com', tax_id: 'T-2', title: 'second' }, ['email']); + /** + * [#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. + * + * 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. + */ + 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. + const err = await captureError(() => + driver.upsert(WRONG_KEY.name, { email: 'other@b.com', tax_id: 'T-1', title: 'second' }, ['email']), + ); + expect(err, 'a backed conflict target must not be refused').toBeNull(); - const after = await rows(); - expect(after).toHaveLength(2); - expect(after.map((r: any) => r.email)).toEqual(['a@b.com', 'a@b.com']); - expect(after.map((r: any) => r.title)).toEqual(['first', 'second']); + 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'); }); /** - * The control, and the reason the three pins above are readable as a defect + * 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 - * target MySQL's `ON DUPLICATE KEY UPDATE` really does honour — still works - * on this same driver and this same table. + * target MySQL's `ON DUPLICATE KEY UPDATE` really does honour, and the one + * the pre-flight deliberately does not probe — still works on this same + * driver and this same table. */ it('still merges correctly on the primary key — no conflictKeys, one row', async () => { await driver.upsert(MISMATCHED.name, { id: 'os8592_fixed', email: 'id@b.com', tax_id: 'T-7', title: 'first' }); diff --git a/packages/drivers/driver-sql/src/sql-driver.ts b/packages/drivers/driver-sql/src/sql-driver.ts index 9399dc3ce5..fcd0dde74c 100644 --- a/packages/drivers/driver-sql/src/sql-driver.ts +++ b/packages/drivers/driver-sql/src/sql-driver.ts @@ -3373,6 +3373,23 @@ export class SqlDriver implements IDataDriver { /** Declared indexes per managed table (tableName → indexes[]), captured in `initObjects`. Used to recreate indexes after a SQLite table rebuild. */ protected managedObjectIndexes = new Map(); + /** + * [#8621] PHYSICAL key indexes per table (tableName → the PRIMARY KEY and + * UNIQUE indexes that actually exist), for {@link assertConflictTargetBacked}. + * + * Deliberately not `managedObjectIndexes`, which records what metadata + * DECLARES. The pre-flight answers "can this conflict target resolve to a key + * on the database in front of me", and the two differ in both directions: a + * table created before its `unique` declaration was emitted as DDL is declared + * and unbacked (the case the refusal wording already names), and a federated + * or hand-indexed table is backed while declaring nothing here at all. + * + * Invalidated per table in {@link initObjects} — the one seam that creates + * indexes — and re-read from the database before any refusal, so a stale entry + * can cost a redundant read but can never produce a false refusal. + */ + private physicalKeyIndexes = new Map(); + /** De-dup set for boot-time drift warnings (keyed by {@link driftKey}). */ protected driftWarned = new Set(); @@ -4999,6 +5016,166 @@ export class SqlDriver implements IDataDriver { return columns; } + /** + * The PRIMARY KEY and UNIQUE indexes a table PHYSICALLY carries, or `null` + * when the database could not be asked. + * + * Partial indexes are excluded: a `WHERE`-restricted unique index constrains + * only the rows its predicate admits, so it cannot serve as a conflict target + * for an arbitrary row. (MySQL — the only dialect the pre-flight runs on + * today — has no partial indexes at all, so this arm is defensive rather than + * exercised; it is written here so extending the pre-flight to SQLite or + * Postgres does not have to rediscover it.) + * + * `null` and `[]` are different answers and both mean "do not refuse": a read + * that threw is unknown, and a table that reports NO keys whatsoever is a + * table this driver did not create — every managed table has an `id` PRIMARY + * KEY — most likely one that does not exist yet. An empty result is + * indistinguishable from a missing table on MySQL, whose `information_schema` + * answers zero rows either way, so treating it as "unbacked" would answer + * *"no unique index backs your conflict keys"* to a caller whose real problem + * is a typo in the table name. + */ + private async introspectKeyIndexes( + tableName: string, + opts: { fresh?: boolean } = {}, + ): Promise { + if (!opts.fresh) { + const cached = this.physicalKeyIndexes.get(tableName); + if (cached) return cached; + } + let indexes: PhysicalIndex[]; + try { + indexes = await this.introspectIndexes(tableName); + } catch { + // #7332's direction, for the same reason: a failed read is not evidence + // of an absent index, and this caller's whole output is a refusal. + return null; + } + const keys = indexes.filter( + (i) => (i.unique === true || i.primary === true) && i.partial !== true && i.columns.length > 0, + ); + this.physicalKeyIndexes.set(tableName, keys); + return keys; + } + + /** + * [#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. + * + * # Why a pre-flight exists at all, when a refusal already did + * + * The refusal this throws is the one #8445/#8567 already landed, and the + * catch that raises it (`isUnbackedConflictTargetError` in {@link upsert}) is + * REACTIVE — it classifies an error the server raised. On SQLite and Postgres + * the server does raise one: `ON CONFLICT (email)` names an arbiter index, the + * planner fails to find it, and the statement is refused before it writes. + * + * On MySQL no error is ever raised, because the conflict target never reaches + * the server. knex compiles the driver's exact call to `ON DUPLICATE KEY + * UPDATE`, which takes no target at all — pinned with no server needed in + * `sql-driver-upsert-conflict-target-dialects.test.ts`: + * + * ``` + * knex('t').insert({...}).onConflict(['email']).merge(['title']).toSQL() + * mysql2 -> insert into `t` (…) values (?, ?, ?) + * on duplicate key update `title` = values(`title`) ← no `email` + * ``` + * + * So the reactive catch cannot fire there, and what MySQL does instead is + * strictly worse than the illegible errors the other two dialects were fixed + * for — measured on live MySQL 8.0.46 (#8592, re-measured here): it merges on + * whichever unique index the row happens to collide with, and does NOT merge + * on the key it was given. A wrong write, with no error. + * + * Consulting the declared indexes before compiling is therefore a genuinely + * different mechanism, not a re-use of the existing path. What IS re-used is + * the answer: the same `refuseUnbackedConflictTarget` wording, `code` and + * `status`, per #5240 — one condition, one wording — so all three dialects + * now answer the same sentence for the same mistake. + * + * # Why it runs on MySQL ONLY + * + * Not a dialect exemption — the opposite. SQLite and Postgres already refuse + * this exact call with this exact sentence, and they refuse it with the + * SERVER's own text attached as `cause`, which is the ground truth an operator + * debugging the table wants and which no pre-flight can reconstruct. Running + * the pre-flight there would replace a server verdict with an introspection + * verdict, discard that `cause`, and make the accept set depend on this + * driver's reading of `pg_index` rather than on the planner's — a strictly + * worse trade on the two dialects that are already correct. The criterion is + * "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 + * + * 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. + * + * The comparison is against the conflict target as EMITTED. `onConflict()` + * receives `mergeKeys` verbatim — the write column map is applied to the row, + * never to the target — so the identifiers compared here are the identifiers + * the statement would carry, and a key set matches an index when the two hold + * the same columns, order-insensitively (which is how both `ON CONFLICT` + * 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.** + * `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`: + * + * ``` + * 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, 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 { + // 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))); + + let keys = await this.introspectKeyIndexes(tableName); + if (keys !== null && keys.length > 0 && !backs(keys)) { + // 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. + keys = await this.introspectKeyIndexes(tableName, { fresh: true }); + } + if (keys === null || keys.length === 0 || backs(keys)) return; + + // The introspected keys stand where the server's sentence stands on the + // other two dialects: the caller-visible message is the shared wording, and + // `cause` carries the ground truth an operator needs to act. Schema + // identifiers only — no row values, matching the payload contract the + // refusal already keeps. + const found = keys.map((i) => `${i.name}(${i.columns.join(', ')})`).join(', '); + throw refuseUnbackedConflictTarget( + object, + mergeKeys, + new Error( + `no PRIMARY KEY or UNIQUE index on "${tableName}" covers (${mergeKeys.join(', ')}); ` + + `keys present: ${found}`, + ), + ); + } + async upsert(object: string, data: Record, conflictKeys?: string[], options?: DriverOptions): Promise> { const { _id, ...rest } = data; const toUpsert = { ...rest }; @@ -5014,6 +5191,24 @@ 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 + // 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 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.assertConflictTargetBacked(object, mergeKeys); + } + // #6943. Measured: `upsert` does NOT share `bulkCreate`'s shape. It is // single-row, so a stale counter costs it exactly one burned number per // call — the same shape `create()` had before #5495, for the same reason @@ -6350,6 +6545,11 @@ export class SqlDriver implements IDataDriver { } else { this.managedObjectIndexes.delete(tableName); } + // [#8621] This call may create the table, or add a unique index to one + // that already exists, so whatever the upsert pre-flight introspected + // before it is stale. Dropping the entry is enough — the entry is + // re-read lazily, and a refusal re-reads unconditionally. + this.physicalKeyIndexes.delete(tableName); const jsonCols: string[] = []; const booleanCols: string[] = []; From 991ab981acbadf10afa0d20f5f8cafd3ce74663b Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 14 Aug 2026 18:58:33 +0000 Subject: [PATCH 2/2] =?UTF-8?q?docs(changeset):=20driver-sql=20minor=20?= =?UTF-8?q?=E2=80=94=20MySQL=20upsert=20accept-set=20narrowing=20(#8621)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01XeQRiAa7vYRVX5Fog7Zby8 --- ...ysql-unbacked-conflict-target-preflight.md | 68 +++++++++++++++++++ 1 file changed, 68 insertions(+) create mode 100644 .changeset/mysql-unbacked-conflict-target-preflight.md diff --git a/.changeset/mysql-unbacked-conflict-target-preflight.md b/.changeset/mysql-unbacked-conflict-target-preflight.md new file mode 100644 index 0000000000..f05981d3fa --- /dev/null +++ b/.changeset/mysql-unbacked-conflict-target-preflight.md @@ -0,0 +1,68 @@ +--- +"@objectstack/driver-sql": minor +--- + +fix(driver-sql): MySQL refuses an upsert whose `conflictKeys` no PRIMARY KEY or UNIQUE index backs — calls that previously "resolved" now throw (#8621) + +**This narrows MySQL's accept set.** A `SqlDriver.upsert(object, data, conflictKeys)` +call on MySQL whose conflict target is backed by no PRIMARY KEY and no UNIQUE +index used to resolve; it now throws `VALIDATION_ERROR` / 400. That is why this +is a `minor` and not a patch: code that ran without error against MySQL will +start failing, deliberately, and the rows it was writing were not the rows the +caller asked for. + +SQLite and Postgres have refused this exact call since #8445 / #8567, with this +exact sentence. MySQL did not, and could not: knex compiles +`onConflict([...]).merge(...)` on `mysql2` to `ON DUPLICATE KEY UPDATE`, which +takes **no conflict target at all**, so the named keys are dropped before the +statement leaves the process and the server is never asked to find an index for +them. The existing refusal classifies an error the server raised, so on MySQL it +had nothing to classify. + +Measured on live MySQL 8.0.46 — `email` is the column the caller names, `tax_id` +carries the only unique index: + +``` +seed upsert({email:'a@b.com', tax_id:'T-1', title:'first'}, ['email']) -> resolved +B upsert({email:'other@b.com', tax_id:'T-1', title:'second'}, ['email']) -> resolved + ONE row: merged on `tax_id`, which the caller never named, across two + different `email` values. +D seed, then upsert({email:'a@b.com', tax_id:'T-2'}, ['email']) -> resolved + TWO rows, both `email='a@b.com'`: the merge that WAS asked for did not + happen either. +``` + +So the failure being replaced is not an illegible error — it is a silent wrong +write. `upsert` now consults the table's physical keys before compiling on MySQL +and answers the wording, `code` and `status` the other two dialects already +answer (#5240 — one condition, one wording). + +**What this means for an existing MySQL deployment.** The calls that change are +exactly those naming a conflict target no key covers — the same calls that have +always been errors on SQLite and Postgres. The most likely one to surface is a +tenant-scoped `unique: true` field: its index materializes as the composite +`(COALESCE(organization_id, '__global__'), field)` (ADR-0120 D3), so +`conflictKeys: ['field']` alone is not backed by it. The remedy is the one the +refusal already prints: declare the column(s) `unique: true` and re-run schema +sync, name the full composite, or upsert on the primary key. + +Deliberately unchanged: + +- **SQLite and Postgres.** They already refuse this from the server, and they + attach the server's own sentence as `cause` — ground truth a pre-flight cannot + reconstruct. Running the pre-flight there would replace a planner verdict with + an introspection verdict for no gain. +- **The default `['id']` path.** The pre-flight runs only when the caller names + a target; the default is this driver's own primary key on every table it + creates, so probing it would add a round trip to every ordinary upsert to + answer a question with only one possible answer. +- **Anything the pre-flight cannot prove.** A failed introspection, a table + reporting no keys at all (indistinguishable from a table that does not exist), + and a possibly stale cache all proceed rather than refuse — the cache is + re-read from the database before any refusal is thrown. + +**Not fixed here, and filed as #8755:** `ON DUPLICATE KEY UPDATE` carries no +conflict target even when the named one IS backed, so on MySQL a second unique +index can still absorb the conflict and merge on a key the caller never named. +This change closes the unbacked-target hole; it does not make MySQL honour +`conflictKeys` as a target.