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;