From 4a64e5fd1a4a2a82fc0aaacfb0960e123bd75425 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 13 Aug 2026 22:03:40 +0000 Subject: [PATCH] fix(driver-sql): envelope the unbacked-conflict-target upsert refusal (#8445) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `SqlDriver.upsert` let SQLite's error escape as raised when `conflictKeys` named columns with no backing PRIMARY KEY or UNIQUE index: `code: 'SQLITE_ERROR'`, `status: undefined`, and a message that is the statement with its bound values. `mapDataError` reads `code`/`status` to build the response envelope, so with neither set it fell through to its default branch and shipped that SQL text to the caller as the whole body. The condition is now recognised at the throw site and re-raised as `VALIDATION_ERROR` / 400 with the original error kept as `cause`. The wording is `driver-turso`'s remote refusal (#8413), first sentence for first sentence (#5240 — one condition, one wording). Recognition matches SQLite's own sentence, the only channel SQLite fills for this condition. Postgres/MySQL wording is unmeasured and deliberately not guessed at; generalising is tracked on #8567. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01VoxQqG5FiUHZKCST7KDoZC --- ...psert-unbacked-conflict-target-envelope.md | 48 ++++ ...er-upsert-conflict-target-envelope.test.ts | 228 ++++++++++++++++++ packages/drivers/driver-sql/src/sql-driver.ts | 125 ++++++++++ 3 files changed, 401 insertions(+) create mode 100644 .changeset/upsert-unbacked-conflict-target-envelope.md create mode 100644 packages/drivers/driver-sql/src/sql-driver-upsert-conflict-target-envelope.test.ts diff --git a/.changeset/upsert-unbacked-conflict-target-envelope.md b/.changeset/upsert-unbacked-conflict-target-envelope.md new file mode 100644 index 0000000000..8fa7adc110 --- /dev/null +++ b/.changeset/upsert-unbacked-conflict-target-envelope.md @@ -0,0 +1,48 @@ +--- +"@objectstack/driver-sql": patch +--- + +fix(driver-sql): an `upsert` whose `conflictKeys` have no backing unique index refuses with an ADR-0112 envelope instead of a raw `SqliteError` (#8445) + +`SqlDriver.upsert` let SQLite's error escape exactly as raised when the named +conflict keys were not backed by a PRIMARY KEY or UNIQUE index. Measured on the +local face (knex + better-sqlite3): + +``` +upsert('plain', { email: 'a@b.com', title: 'x' }, ['email']) + -> THREW name=SqliteError code=SQLITE_ERROR status=undefined + msg=insert into `plain` (...) values ('a@b.com', ...) on conflict (`email`) + do update set ... - ON CONFLICT clause does not match any PRIMARY KEY + or UNIQUE constraint +``` + +**The payload was the larger half of the defect.** `mapDataError` builds the +response envelope from `error.code` / `error.status`; with neither set it falls +through to its default branch and serves the thrown message as the entire body — +and that message is the **statement**, bound values inlined. So a caller got no +`code` to branch on *and* the SQL text of the write it attempted. + +The condition is now recognised at the throw site and re-raised as +`VALIDATION_ERROR` / 400, carrying the original error as `cause` so the SQLite +text an operator debugging the table needs is preserved rather than destroyed. +The wording is `driver-turso`'s remote refusal (#8413), first sentence for first +sentence — `TursoDriver` picks its face from `url`, so one condition answered in +two wordings would make the answer a property of the connection string (#5240). + +**No call that worked before fails now, and no call that failed before +succeeds.** The same upserts are refused; they are refused legibly. A +`conflictKeys` upsert whose target *is* backed by a declared `unique: true` +still merges, and the default `id` merge key is untouched — both pinned as +controls beside the refusal, because an implementation that refused every +`conflictKeys` upsert would satisfy the refusal assertion while having broken +the capability. + +**Recognition is SQLite-first, by decision rather than by oversight.** SQLite +fills exactly one channel for this condition — the message; it raises a plain +`SQLITE_ERROR`, the same generic code a syntax error carries, so `code` cannot +discriminate. Postgres and MySQL wording for the same condition is unmeasured +(no server for either was available to raise it), so those dialects keep the +behaviour they have today rather than being matched on transcribed-from-memory +text. Measuring them, and deciding whether the recognition then belongs in a +shared predicate in `@objectstack/types` beside `isUniqueViolationError`, is +tracked on #8567. diff --git a/packages/drivers/driver-sql/src/sql-driver-upsert-conflict-target-envelope.test.ts b/packages/drivers/driver-sql/src/sql-driver-upsert-conflict-target-envelope.test.ts new file mode 100644 index 0000000000..225e9944e9 --- /dev/null +++ b/packages/drivers/driver-sql/src/sql-driver-upsert-conflict-target-envelope.test.ts @@ -0,0 +1,228 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * [#8445] A `conflictKeys` upsert whose target no unique index backs refuses in + * the ADR-0112 envelope — the LOCAL twin of #8413's remote-face refusal. + * + * # What was measured, before the fix + * + * `SqlDriver.upsert` let SQLite's error escape exactly as raised. Measured on + * this face (knex + better-sqlite3), on `origin/main` @ `719a21bf`: + * + * ``` + * upsert('plain', { email: 'a@b.com', title: 'x' }, ['email']) + * -> THREW name=SqliteError code=SQLITE_ERROR status=undefined + * msg=insert into `plain` (`created_at`, `email`, `id`, `title`, `updated_at`) + * values ('2026-08-13T…', 'a@b.com', 'ib21mSZ…', 'x', '2026-08-13T…') + * on conflict (`email`) do update set … - ON CONFLICT clause does not + * match any PRIMARY KEY or UNIQUE constraint + * ``` + * + * # Why the payload matters as much as the missing code + * + * `mapDataError` builds the envelope from `error.code` / `error.status`. With + * neither set it falls through to its default branch and serves the thrown + * message as the entire body — and that message is the STATEMENT, bound values + * included. So the defect is two things at once: no `code` for any client to + * branch on, and the SQL text (with row data in it) shipped to the caller. + * Hence the leak pin below, beside the envelope pin: asserting `code`/`status` + * alone would let a refusal that still echoed the statement pass. + * + * # Every case asserts `code` AND `status` + * + * Never a bare `toThrow()` — the un-fixed driver threw for this input too, so + * `rejects.toThrow()` was green before and after and could not see the defect + * at all. This is the same rule `sql-driver-date-bucket.test.ts` records for + * the `code`/`status`-`undefined` fall-through it pins. + * + * # The positive control is not optional + * + * An implementation that refused EVERY `conflictKeys` upsert would satisfy the + * refusal pin while having destroyed the capability. The control asserts the + * same `conflictKeys` upsert MERGES when a declared `unique: true` does back + * the target — and it is the half that caught the equivalent risk on #8413. + * The specificity control beside it is its mirror: an unrelated statement + * failure must still come back as itself, because the recognition is a narrow + * message match and not a catch-all over `SQLITE_ERROR`. + * + * # Reverse verification — direction predicted BEFORE it was run + * + * Predicted, with the two helpers' call site removed from `upsert`'s catch (the + * fix committed first, so the file is restored from a commit that exists): + * the envelope pin and the leak pin go RED — the envelope pin on its first + * assertion (`code` → `'SQLITE_ERROR'`, not through any "it resolved" branch, + * because the un-fixed driver refused this input all along), the leak pin on + * the statement text reappearing in the caller-visible message. The wording pin + * goes red with them. Everything else stays GREEN, and that is the half worth + * predicting: the positive control, the `id`-merge-key control and the + * specificity control never depended on the fix — they describe behaviour this + * card does not change, which is precisely what makes them controls. + */ + +import { describe, it, expect, beforeEach, afterEach } from 'vitest'; +import { StandardErrorCode } from '@objectstack/spec/api'; +import { SqlDriver } from '../src/index.js'; + +/** The shape `mapDataError` / `sendError` read off a thrown driver error. */ +interface WireBearingError extends Error { + code?: string; + status?: number; + cause?: unknown; +} + +/** + * The card's object: one business key beside an ordinary column, and no tenant + * column — so `uniqueIndexesFromFields` resolves to the single-column `(email)` + * form rather than a tenant-scoped composite. + */ +const CONTACT = { + name: 'crm_contact', + fields: { + email: { type: 'string', unique: true }, + title: { type: 'string' }, + }, +} as any; + +/** The same object with the declaration REMOVED — the un-backed conflict target. */ +const PLAIN = { + name: 'crm_contact_plain', + fields: { + email: { type: 'string' }, + title: { type: 'string' }, + }, +} as any; + +const captureError = async (run: () => Promise): Promise => { + try { + await run(); + return null; + } catch (e) { + return e as WireBearingError; + } +}; + +describe('[#8445] SqlDriver.upsert refuses an unbacked conflict target in the ADR-0112 envelope', () => { + let driver: SqlDriver; + + beforeEach(async () => { + driver = new SqlDriver({ + client: 'better-sqlite3', + connection: { filename: ':memory:' }, + useNullAsDefault: true, + }); + await driver.initObjects([CONTACT, PLAIN]); + }); + + afterEach(async () => { + await driver.disconnect(); + }); + + // ─────────────────────────────────────────────────────────────────── + // Pin 1 — the envelope + // ─────────────────────────────────────────────────────────────────── + + it('answers a real `code` and `status` — never a raw SqliteError', async () => { + const err = await captureError(() => driver.upsert(PLAIN.name, { email: 'a@b.com', title: 'x' }, ['email'])); + + expect(err).not.toBeNull(); + // The measured defect: `code: 'SQLITE_ERROR'`, `status: undefined`. + expect(err!.code).toBe(StandardErrorCode.enum.VALIDATION_ERROR); + expect(err!.status).toBe(400); + expect(err!.code).not.toBe('SQLITE_ERROR'); + + // The message names what an operator has to act on: which object, which + // keys, and that the remedy is the index rather than a retry. + expect(err!.message).toMatch(/crm_contact_plain/); + expect(err!.message).toMatch(/email/); + expect(err!.message).toMatch(/unique/i); + + // The SQLite ground truth is preserved rather than destroyed — the refusal + // adds a classification, it does not replace what a DBA needs. + expect(String((err!.cause as Error | undefined)?.message)).toMatch(/ON CONFLICT clause does not match/i); + }); + + // ─────────────────────────────────────────────────────────────────── + // Pin 2 — the payload: the statement must not travel with the refusal + // ─────────────────────────────────────────────────────────────────── + + it('keeps the SQL statement and its bound values out of the caller-visible message', async () => { + const err = await captureError(() => + driver.upsert(PLAIN.name, { email: 'leaked@example.com', title: 'secret-title' }, ['email']), + ); + + expect(err).not.toBeNull(); + // `mapDataError` serves this string as the whole body for a `code`-less + // throw; before the fix it was the INSERT, values inlined. + expect(err!.message).not.toMatch(/insert into/i); + expect(err!.message).not.toMatch(/excluded\./i); + expect(err!.message).not.toContain('leaked@example.com'); + expect(err!.message).not.toContain('secret-title'); + + // …while the statement is still reachable from the `cause`, which no error + // mapper puts on the wire. + expect(String((err!.cause as Error | undefined)?.message)).toMatch(/insert into/i); + }); + + // ─────────────────────────────────────────────────────────────────── + // Pin 3 — one condition, one wording (#5240) + // ─────────────────────────────────────────────────────────────────── + + /** + * The first sentence is `driver-turso`'s `refuseUnbackedConflictTarget` + * (#8413), verbatim: `TursoDriver` picks its face from `url`, so this one + * condition can be answered by either compiler in a single deployment and a + * second wording would make the answer a property of the connection string. + * + * ⚠️ Read what this pin can and cannot do. It is ONE-WAY: it fails if THIS + * face is reworded, and cannot see a reword of the remote face. The two-way + * form compares the two RUNTIME messages (the shape + * `remote-transport-aggregate-function-refusal.test.ts` uses), which needs a + * package that can import both faces — `driver-sql` cannot import + * `driver-turso`, and `remote-transport.ts` is deliberately free of knex and + * of `SqlDriver`, so neither source can hold it. That pin belongs in + * `driver-turso`, outside this card's declared file surface, and is filed as + * #8568 rather than smuggled in here. + */ + it('opens with the remote refusal’s first sentence, word for word', async () => { + const err = await captureError(() => driver.upsert(PLAIN.name, { email: 'a@b.com', title: 'x' }, ['email'])); + + expect(err!.message.split('. ')[0] + '.').toBe( + 'Cannot upsert into "crm_contact_plain" on conflict keys ("email"): no PRIMARY KEY or UNIQUE ' + + 'index backs them, so the merge target does not exist and SQLite refuses the statement.', + ); + }); + + // ─────────────────────────────────────────────────────────────────── + // Pin 4 — the controls: what this card must NOT have changed + // ─────────────────────────────────────────────────────────────────── + + it('MERGES when a declared unique index does back the conflict target', async () => { + await driver.upsert(CONTACT.name, { email: 'a@b.com', title: 'first' }, ['email']); + await driver.upsert(CONTACT.name, { email: 'a@b.com', title: 'second' }, ['email']); + + const rows = await driver.find(CONTACT.name, {}); + expect(rows).toHaveLength(1); + expect(rows[0].title).toBe('second'); + }); + + it('leaves the default `id` merge key working — no conflictKeys, no refusal', async () => { + const created = await driver.upsert(PLAIN.name, { id: 'fixed_id', email: 'a@b.com', title: 'first' }); + expect(created.id).toBe('fixed_id'); + await driver.upsert(PLAIN.name, { id: 'fixed_id', email: 'a@b.com', title: 'second' }); + + const rows = await driver.find(PLAIN.name, {}); + expect(rows).toHaveLength(1); + expect(rows[0].title).toBe('second'); + }); + + it('does not swallow an unrelated statement failure as this refusal', async () => { + // A table that was never created: a different `SQLITE_ERROR` entirely. The + // recognition matches SQLite's ON CONFLICT sentence, not the generic code, + // so this must come back as itself. + const err = await captureError(() => driver.upsert('never_created', { id: 'x' })); + + expect(err).not.toBeNull(); + expect(err!.message).not.toMatch(/Cannot upsert into/); + expect(err!.code).not.toBe(StandardErrorCode.enum.VALIDATION_ERROR); + }); +}); diff --git a/packages/drivers/driver-sql/src/sql-driver.ts b/packages/drivers/driver-sql/src/sql-driver.ts index 83549b70d5..7427409804 100644 --- a/packages/drivers/driver-sql/src/sql-driver.ts +++ b/packages/drivers/driver-sql/src/sql-driver.ts @@ -967,6 +967,120 @@ function refuseDateBucketedGroupBy(granularity: string, bucketedHere: string[], throw err; } +/** + * [#8445] Is this the "the conflict target is not a key" failure? + * + * # Which channel answers, and why it is the message + * + * SQLite fills exactly one: the MESSAGE. It raises a plain `SQLITE_ERROR` — + * the same generic code a syntax error carries — so `code` cannot discriminate, + * and a `code`-based test would swallow every other statement failure as an + * unbacked conflict target. Measured on this face, through knex + + * better-sqlite3, before the fix: + * + * ``` + * upsert('plain', { email: 'a@b.com', title: 'x' }, ['email']) + * -> THREW name=SqliteError code=SQLITE_ERROR status=undefined + * msg=insert into `plain` (...) values (...) on conflict (`email`) do update set ... + * - ON CONFLICT clause does not match any PRIMARY KEY or UNIQUE constraint + * ``` + * + * Note what knex does to the message: it prefixes the STATEMENT and keeps + * SQLite's own sentence after ` - `. So the tail this predicate matches is + * present on the local face too, and the regex is anchored on that tail rather + * than on the whole string. The text is SQLite's own, stable since `ON + * CONFLICT` arrived in 3.24, and narrow enough that no other failure shares it. + * + * # What it deliberately does NOT cover — measured, not assumed + * + * `driver-sql` serves three dialects, and only SQLite's wording for this + * condition has been measured (this container has no Postgres or MySQL server + * to raise the other two). Recognition is therefore SQLite-first by decision: + * a Postgres deployment still gets its raw error here, exactly as before, which + * is no worse than today and is a condition nobody has to guess at. Widening it + * — most likely as a dialect-spanning predicate in `@objectstack/types` beside + * `isUniqueViolationError`, whose vocabulary table is the shape it would take — + * is #8567, and it is gated on measuring the other two dialects' text rather + * than transcribing it from memory. + * + * ⚠️ Deliberately NOT `isUniqueViolationError`: that predicate answers the + * OPPOSITE condition (a unique index exists and the row violated it). + * Confusing the two would report a working constraint as a missing one. + * + * The twin lives in `driver-turso`'s `remote-transport.ts` + * ({@link https://github.com/objectstack-ai/objectstack/issues/8413}); the two + * faces state this recognition once each, for the reason that file's header + * gives — the remote transport is deliberately free of knex and of `SqlDriver`, + * so it cannot import this one. + */ +function isUnbackedConflictTargetError(error: unknown): boolean { + const message = error instanceof Error ? error.message : String(error ?? ''); + return /ON CONFLICT clause does not match any PRIMARY KEY or UNIQUE constraint/i.test(message); +} + +/** + * [#8445] A `conflictKeys` upsert whose target no unique index backs. + * + * # What the raw throw did + * + * `mapDataError` builds the response envelope from `error.code` / `error.status`. + * With neither set it falls through to its default branch and ships the thrown + * message as the whole body — and the message above is the STATEMENT, values + * included: `{ "error": "insert into `plain` (...) values ('a@b.com', ...) ..." }`. + * Unbranchable by any client, and an information leak of the statement text. + * That fall-through is not a guess about `mapDataError`; it is the shape + * `sql-driver-date-bucket.test.ts` already records for a `code`/`status`-less + * driver throw. + * + * # Why `VALIDATION_ERROR` / 400 + * + * Reused, deliberately, from the remote twin — the rationale is written out in + * full at `driver-turso`'s `refuseUnbackedConflictTarget` (#8413) and is not + * re-derived here. In one line: a client branching on why its call failed needs + * two facts — **do not retry** (the next attempt fails identically; nothing is + * transient, so a 5xx would invite a retry storm) and **nothing collided** (the + * merge could not be attempted at all, which is why it is not + * `RESOURCE_CONFLICT`/409 and not `DUPLICATE_VALUE`). `conflictKeys` is a + * request argument that did not validate against the target, which is exactly + * what the catalogued generic covers. `status: 400` puts the rejection on + * `@objectstack/rest`'s expected-rejection list, so a caller's un-satisfiable + * upsert stops being logged once per request as an unhandled SERVER fault. + * + * # The wording is the remote refusal's, first sentence for first sentence + * + * #5240 — one condition, one wording. `TursoDriver` picks its face from `url`, + * so this condition can be answered by either compiler in one deployment, and a + * second wording would make the answer a property of the connection string. + * The text below is #8413's, verbatim, including the "table created before its + * `unique` declaration was emitted as DDL" clause: that leg is not remote-only + * — this face creates declared indexes, but it cannot create one over a table + * that already holds duplicates (`syncDeclaredIndexes` degrades rather than + * rewriting rows), so the same table arrives here with the same missing index. + * + * ⚠️ The original error is kept as `cause` rather than discarded — the SQLite + * text is the ground truth an operator debugging the table wants, and nothing + * above this layer can recover it once replaced. It is ASSIGNED rather than + * passed to the `Error` constructor, matching the twin: the two-argument form + * needs the ES2022 `ErrorOptions` overload. + */ +function refuseUnbackedConflictTarget(object: string, mergeKeys: string[], cause: unknown): Error { + const keys = mergeKeys.map((k) => `"${k}"`).join(', '); + const err = new Error( + `Cannot upsert into "${object}" on conflict keys (${keys}): no PRIMARY KEY or UNIQUE index ` + + `backs them, so the merge target does not exist and SQLite refuses the statement. This is ` + + `usually a table created before its "unique" declaration was emitted as DDL, or conflict ` + + `keys naming columns that were never declared unique. Fix by declaring the column(s) ` + + `"unique: true" and re-running schema sync so the unique index is created — if the table ` + + `already holds duplicate values the index cannot be created until they are resolved, and ` + + `this driver does not rewrite stored rows to force it. Alternatively upsert on the primary ` + + `key by supplying "id" and omitting conflictKeys.`, + ) as Error & { code?: string; status?: number; cause?: unknown }; + err.code = StandardErrorCode.enum.VALIDATION_ERROR; + err.status = 400; + err.cause = cause; + return err; +} + /** * [#5158] A `FilterArray` reached the driver unlowered. * @@ -4948,6 +5062,17 @@ export class SqlDriver implements IDataDriver { await (mergeColumns.length > 0 ? insertion.merge(mergeColumns) : insertion.merge()); break; } catch (error) { + // [#8445] Classified BEFORE the autonumber retry logic, for three + // reasons that all point the same way. It is not an autonumber + // collision — `collidingAutoNumberReservations` asks + // `isUniqueViolationError`, which is false for this error, so the + // reservation probe would query the sequences table for nothing and + // then rethrow the raw error anyway. It is not transient — the same + // statement fails identically on every attempt, so a retry is a wasted + // round trip by construction. And it is the only placement that + // envelopes BOTH exits below: a refusal recognised after the branch + // would still escape raw whenever `mayRetry` is false. + if (isUnbackedConflictTargetError(error)) throw refuseUnbackedConflictTarget(object, mergeKeys, error); if (!mayRetry || attempt >= AUTONUMBER_COLLISION_RETRIES) throw error; const colliding = await this.collidingAutoNumberReservations(error, reservations, options); if (colliding.length === 0) throw error;