From 26a612dd3e33b811b855e61eb2d2733eb2e250eb Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 14 Aug 2026 15:32:09 +0000 Subject: [PATCH 1/2] fix(types): require a violation phrasing in isUniqueViolationError's message limb (#8590) The `unique constraint` limb matched a word pair, not a condition, so every sentence saying a unique constraint is ABSENT was claimed as a violation of one. Measured on live servers across all three supported dialect families (SQLite via better-sqlite3, PostgreSQL 16.13 via pg 8.22.0, MariaDB 10.11.14 via mysql2, all through knex 3.3.0), in both directions plus the NOT NULL / FOREIGN KEY near misses. The dialect sweep found a SECOND instance the card did not know about: PG 42830 (`there is no unique constraint matching given keys for referenced table`), raised by a FOREIGN KEY referencing a non-unique column, puts the pair adjacent in Postgres' own absence sentence. That rules out the negative-lookahead candidate, which is a blocklist keyed on SQLite's wording and still answers true there. The limb is now an allowlist of violation phrasings, restoring the module's stated default (unrecognised is false) to the message channel. Both spellings the retired limb covered are preserved: SQLite's `UNIQUE constraint failed: t.c` and Postgres' `violates unique constraint "..."`. The code/errno channels and the duplicate key/entry limbs are untouched. #8567's pin is inverted rather than deleted, and the absence sentences are pinned per dialect in a new suite covering the code channel too. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01NaS1PAHJcPfAA2acnV53Tn --- ...-violation-absence-sentence-superstring.md | 67 ++++ .../src/unbacked-conflict-target.test.ts | 69 ++-- ...unique-violation-absence-sentences.test.ts | 338 ++++++++++++++++++ packages/types/src/unique-violation.ts | 66 +++- 4 files changed, 498 insertions(+), 42 deletions(-) create mode 100644 .changeset/unique-violation-absence-sentence-superstring.md create mode 100644 packages/types/src/unique-violation-absence-sentences.test.ts diff --git a/.changeset/unique-violation-absence-sentence-superstring.md b/.changeset/unique-violation-absence-sentence-superstring.md new file mode 100644 index 0000000000..308f96a959 --- /dev/null +++ b/.changeset/unique-violation-absence-sentence-superstring.md @@ -0,0 +1,67 @@ +--- +"@objectstack/types": patch +--- + +fix(types): `isUniqueViolationError` stops claiming the sentences that say a unique constraint is ABSENT (#8590) + +The shared predicate's message limb was a bare `unique constraint`, and a word +pair is not a condition. Every dialect that can say "this row violated a unique +constraint" can also say "there is no unique constraint here", and the same two +words sit adjacent in both — so the predicate answered **true** for errors +meaning the exact opposite of what it detects. `rest-server.ts` maps that +verdict to `409 UNIQUE_VIOLATION`, which tells a client to change a value when +nothing was ever compared, on a status an SDK will not retry. + +**Measured on live servers for this fix, all three supported dialect families** +— SQLite via better-sqlite3, PostgreSQL 16.13 via `pg` 8.22.0, MariaDB 10.11.14 +via `mysql2` 3.23.1, all through knex 3.3.0 — driving each dialect through both +conditions plus the NOT NULL / FOREIGN KEY near misses: + +``` +sqlite ON CONFLICT clause does not match any PRIMARY KEY or UNIQUE constraint + -> was true, WRONG (the reported defect, #8590) +postgres there is no unique constraint matching given keys for referenced table "t" + -> was true, WRONG (42830 — found by this fix's dialect sweep) +postgres there is no unique or exclusion constraint matching the ON CONFLICT specification + -> false (the pair is not adjacent here) +mysql the condition cannot arise: knex compiles to ON DUPLICATE KEY UPDATE, + which carries no conflict target (confirmed against a live server) +``` + +**Postgres was not clean either, and that chose the fix.** #8590 was filed +reading the collision as SQLite-only, with Postgres escaping "by luck of word +order". The sweep raised **42830** — a `FOREIGN KEY` referencing a non-unique +column — where Postgres puts `unique constraint` adjacent in its own absence +sentence. The card offered two candidate fixes; only one survives 42830. A +negative lookahead on SQLite's missing-index sentence is a blocklist that can +only enumerate absence sentences somebody already tripped over, and it answers +`true` on 42830. So the limb now requires a **violation phrasing** — +`unique constraint failed` (SQLite) or `violates unique constraint` (Postgres) — +which restores the module's own stated default, *unrecognised is `false`*, to +the message channel. + +**Both spellings the retired limb covered are preserved exactly**, which was the +constraint on the fix: the limb was inherited verbatim from the REST branch +#6250 replaced and covered SQLite's `UNIQUE constraint failed: t.c` *and* +Postgres' `... violates unique constraint "..."`. The `unique violation`, +`duplicate key` and `duplicate entry` limbs are untouched, as are the `code` and +`errno` channels — MySQL's `Duplicate entry` path never went through the +narrowed limb at all. + +**No user-visible behaviour changes today; this closes a latent inversion.** The +one site compiling a caller-supplied conflict target (`SqlDriver.upsert`) +recognises the unbacked target *first* in its catch and throws a refusal +declaring `status: 400`, and `mapDataError` reads `declaredHttpStatus` before it +reaches the unique-violation branch — so the 409 was gated off the wire by +ordering, not by the verdict. That ordering was the only thing standing between +this and a wrong status, which is why the verdict is now pinned rather than left +to it. A repo-wide scan of every string literal whose verdict moves found no +consumer relying on the old answer: all of them are prose, a different +predicate's vocabulary (`looksLikeInternalErrorLeak` keeps its own list), or +fixtures asserted through the status-passthrough path. + +`unbacked-conflict-target.test.ts`'s pin — written by #8567 to point at itself +rather than go quietly green — is **inverted, not deleted**, and +`unique-violation-absence-sentences.test.ts` pins the absence sentences per +dialect in both directions, including the code channel, so re-reading `code` +cannot undo the message-side fix from the other side. diff --git a/packages/types/src/unbacked-conflict-target.test.ts b/packages/types/src/unbacked-conflict-target.test.ts index 22b28a0543..74e5e9a897 100644 --- a/packages/types/src/unbacked-conflict-target.test.ts +++ b/packages/types/src/unbacked-conflict-target.test.ts @@ -30,13 +30,16 @@ * vocabulary growing an `ON CONFLICT` one, because that is the file people * extend. * - * ⚠️ Running it that way is what found **#8590**: on SQLite the separation is - * ALREADY broken in the pre-existing direction — `isUniqueViolationError` - * claims the unbacked-target error, because SQLite's missing-index sentence - * ends `…PRIMARY KEY or UNIQUE constraint` and that vocabulary matches the word - * pair `unique constraint` wherever it appears. Not fixed here (it moves - * verdicts in six packages); pinned as measured, per dialect, so the fix - * announces itself. See the suite below. + * ⚠️ Running it that way is what found **#8590**: on SQLite the separation was + * broken in the pre-existing direction — `isUniqueViolationError` claimed the + * unbacked-target error, because SQLite's missing-index sentence ends + * `…PRIMARY KEY or UNIQUE constraint` and that vocabulary matched the word pair + * `unique constraint` wherever it appeared. #8567 pinned it as measured rather + * than fixing it (the fix moves verdicts in six consuming packages); **#8590 + * has since closed it** by requiring a violation phrasing in that limb, and the + * pin below was inverted rather than deleted — which is what a pin written to + * point at itself is for. The separation is now clean on both dialects, in both + * directions, and the suite below is what keeps it that way. */ import { describe, expect, it } from 'vitest'; @@ -157,29 +160,38 @@ describe('[#8567] the `code` channel is deliberately unread — measured over-ma describe('[#8567] ⚠️ separation from isUniqueViolationError — the inverse condition', () => { /** * ⚠️ This suite was written expecting clean disjointness in both - * directions. It went RED on the first run, and the measurement won: on - * SQLite, `isUniqueViolationError` ALREADY claims the unbacked-target - * error. Filed as **#8590**, deliberately not fixed here — narrowing that - * predicate moves verdicts in six consuming packages and needs its own - * measured pass. + * directions. It went RED on the first run and the measurement won: on + * SQLite, `isUniqueViolationError` claimed the unbacked-target error. + * #8567 filed that as **#8590** and pinned the wrong verdict as measured + * rather than fixing it, because narrowing that predicate moves verdicts in + * six consuming packages and needed its own measured pass. * - * The cause is a superstring collision, not a judgement call. Its message - * limb is `/unique constraint|…/i`, and SQLite's sentence for the MISSING - * index ends `…any PRIMARY KEY or UNIQUE constraint` — the two words sit - * adjacent inside a sentence that says the constraint is absent. Postgres - * escapes only on word order (`unique or exclusion constraint` is not - * adjacent), which is the tell that a word pair is being matched rather - * than a condition. + * **#8590 has since landed, and this pin was INVERTED — that is the pin + * working, not an obstacle to route around.** The cause was a superstring + * collision, not a judgement call: the limb was a bare `unique constraint`, + * and SQLite's sentence for the MISSING index ends `…any PRIMARY KEY or + * UNIQUE constraint`, so the two words sit adjacent inside a sentence that + * says the constraint is ABSENT. The limb now requires a violation + * phrasing (`unique constraint failed` / `violates unique constraint`), so + * mentioning a unique constraint is no longer enough to be claimed as one. * - * So the pins below record the state as MEASURED, per dialect, rather than - * as hoped. When #8590 lands, the SQLite row goes red and points straight - * at itself — which is the entire reason to pin a known defect instead of - * leaving the direction untested. + * ⚠️ Postgres was believed to escape "by luck of word order" — its + * `unique or exclusion constraint` is not adjacent. That reading was too + * kind: #8590's own dialect sweep raised PG 42830, + * `there is no unique constraint matching given keys for referenced table`, + * where Postgres puts the pair adjacent in its own ABSENCE sentence. Both + * dialects had the collision; only SQLite's instance was on the path this + * file measures. The absence sentences are pinned per dialect in + * `unique-violation-absence-sentences.test.ts`. + * + * Both rows are therefore `false` now, and the map is kept per dialect + * rather than collapsed to a constant so a regression names the dialect it + * came back on. */ const UNIQUE_VIOLATION_VERDICT_ON_UNBACKED: Record = { - // ⚠️ THE DEFECT (#8590). Correct value is `false`; flip it when #8590 lands. - sqlite: true, - // Correct today, and only by luck of word order — see above. + // [#8590] Was `true` — the defect. Inverted when the fix landed. + sqlite: false, + // Correct before #8590 on this sentence, and now correct by rule. postgres: false, }; @@ -188,8 +200,9 @@ describe('[#8567] ⚠️ separation from isUniqueViolationError — the inverse expect(isUnbackedConflictTargetError(new Error(dialect.knexPrefixed))).toBe(true); expect( isUniqueViolationError(new Error(dialect.knexPrefixed)), - 'if this changed, #8590 either landed (SQLite → false: delete the exception) or ' + - 'regressed (Postgres → true: a new limb is matching the missing-index sentence)', + 'both dialects are `false` since #8590. A `true` here means the unique-violation ' + + 'vocabulary has regrown a limb that matches a sentence saying the constraint is ' + + 'ABSENT — the superstring collision #8590 closed, back on this dialect', ).toBe(UNIQUE_VIOLATION_VERDICT_ON_UNBACKED[key]); }); } diff --git a/packages/types/src/unique-violation-absence-sentences.test.ts b/packages/types/src/unique-violation-absence-sentences.test.ts new file mode 100644 index 0000000000..e6ea737b26 --- /dev/null +++ b/packages/types/src/unique-violation-absence-sentences.test.ts @@ -0,0 +1,338 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * [#8590] `isUniqueViolationError` vs the sentences that say a unique + * constraint is **ABSENT** — the superstring class, pinned per dialect. + * + * # The defect this file closes + * + * The predicate's message limb was a bare `unique constraint`. A word pair is + * not a condition: every dialect that can say "this row violated a unique + * constraint" can also say "there is no unique constraint here", and the same + * two words sit adjacent in both. So the predicate answered `true` for errors + * meaning the exact opposite of what it claims to detect — and 409 + * `UNIQUE_VIOLATION` is what `rest-server.ts` maps that verdict to, telling a + * client to change a value when nothing was ever compared. + * + * # Everything below was raised on a live server, never transcribed + * + * Same rule as `unbacked-conflict-target.test.ts`: a fixture nobody observed a + * server emit is not evidence. Measured for #8590 on: + * + * - **SQLite** via better-sqlite3, knex 3.3.0 + * - **PostgreSQL 16.13** via `pg` 8.22.0, knex 3.3.0 + * - **MariaDB 10.11.14** via `mysql2` 3.23.1, knex 3.3.0 — the MySQL-wire + * family the `Duplicate entry` / `ER_DUP_ENTRY` / errno 1062 vocabulary was + * written for, and the dialect #8590's triage required measuring before the + * limb was narrowed. + * + * Each dialect was driven through BOTH conditions — a unique index that exists + * and was violated, and a unique constraint that is absent — plus the + * NOT NULL / FOREIGN KEY near misses that share the wording. + * + * # ⚠️ Postgres was NOT clean either — the finding that chose the fix + * + * #8590 was filed reading the collision as SQLite-only, with Postgres escaping + * "by luck of word order" because its ON CONFLICT sentence says `unique or + * exclusion constraint` (not adjacent). Sweeping the dialects for the fix found + * {@link PG_FK_ABSENCE}: PostgreSQL 42830, raised when a FOREIGN KEY references + * a non-unique column, puts the pair **adjacent** in its own absence sentence. + * + * That is what decided the fix's shape. The card offered two candidates: + * + * 1. a negative lookahead on SQLite's missing-index sentence, and + * 2. requiring a violation phrasing. + * + * Both close the SQLite case; only (2) closes 42830, because (1) is a blocklist + * and can only enumerate absence sentences somebody already tripped over. The + * limb is now an allowlist of violation phrasings, which restores the module's + * own stated default — **unrecognised is `false`** — to the message channel. + * + * The suites below pin both halves: absence sentences stay `false`, and every + * measured violation spelling stays `true`, because a narrowing that also drops + * a real conflict would trade this bug for a worse one. + */ + +import { describe, expect, it } from 'vitest'; +import { isUniqueViolationError, uniqueViolationColumn } from './unique-violation.js'; + +/** + * PostgreSQL 42830. Raised by `ALTER TABLE ... ADD CONSTRAINT ... FOREIGN KEY` + * and by an inline `REFERENCES` at CREATE TABLE, both measured. The words + * `unique constraint` are adjacent, and the sentence says there is none. + */ +const PG_FK_ABSENCE = 'there is no unique constraint matching given keys for referenced table "xa_parent"'; + +/** + * The measured ABSENCE sentences: a unique constraint is missing, nothing + * collided, and no client should ever be told to change a value for these. + * + * `knexPrefixed` is what a caller actually catches — knex builds the message as + * STATEMENT + ` - ` + the server's sentence — and `bare` is the server's + * sentence alone. Both are pinned, or the verdict would depend on how many + * layers the error passed through. + */ +const ABSENCE = [ + { + label: 'sqlite: ON CONFLICT target with no backing unique index', + bare: 'ON CONFLICT clause does not match any PRIMARY KEY or UNIQUE constraint', + knexPrefixed: + 'insert into `xtalk_plain` (`email`, `id`, `req`, `title`) values ' + + "('a@b.com', '1', 'r', 'x') on conflict (`email`) do update set " + + '`title` = excluded.`title` - ON CONFLICT clause does not match any ' + + 'PRIMARY KEY or UNIQUE constraint', + }, + { + label: 'postgres: ON CONFLICT target with no backing unique index', + bare: 'there is no unique or exclusion constraint matching the ON CONFLICT specification', + knexPrefixed: + 'insert into "xtalk_plain" ("email", "id", "req", "title") values ($1, $2, $3, $4) ' + + 'on conflict ("email") do update set "title" = excluded."title" - there is no unique ' + + 'or exclusion constraint matching the ON CONFLICT specification', + }, + { + label: 'postgres 42830: FOREIGN KEY referencing a non-unique column', + bare: PG_FK_ABSENCE, + knexPrefixed: + 'alter table xa_child add constraint fk_pe foreign key (parent_email) references ' + + `xa_parent(email) - ${PG_FK_ABSENCE}`, + }, + { + label: 'postgres 42830: the same absence, inline REFERENCES at CREATE TABLE', + bare: PG_FK_ABSENCE, + knexPrefixed: + 'create table xa_child2 (id text primary key, pe text references xa_parent(email)) - ' + + PG_FK_ABSENCE, + }, +] as const; + +/** + * The measured VIOLATION spellings — an index EXISTS and a row broke it. Every + * one of these was `true` before #8590 and must stay `true` after it: the limb + * was narrowed, and the whole risk of narrowing is dropping a real conflict. + * + * `column` is what {@link uniqueViolationColumn} may resolve, `undefined` where + * the dialect names an index rather than a column (#6544's contract). + */ +const VIOLATIONS = [ + { + label: 'sqlite: UNIQUE constraint failed, names the column', + bare: 'UNIQUE constraint failed: xtalk_uniq.email', + knexPrefixed: + 'insert into `xtalk_uniq` (`email`, `id`, `title`) values ' + + "('a@b.com', '2', 'y') - UNIQUE constraint failed: xtalk_uniq.email", + column: 'email', + }, + { + label: 'sqlite: the same on a PRIMARY KEY', + bare: 'UNIQUE constraint failed: xtalk_uniq.id', + knexPrefixed: + 'insert into `xtalk_uniq` (`email`, `id`, `title`) values ' + + "('other@b.com', '1', 'q') - UNIQUE constraint failed: xtalk_uniq.id", + column: 'id', + }, + { + label: 'postgres: duplicate key value violates unique constraint', + bare: 'duplicate key value violates unique constraint "xtalk_uniq_email_unique"', + knexPrefixed: + 'insert into "xtalk_uniq" ("email", "id", "title") values ($1, $2, $3) - ' + + 'duplicate key value violates unique constraint "xtalk_uniq_email_unique"', + column: undefined, + }, + { + label: 'mysql: Duplicate entry for a unique key', + bare: "Duplicate entry 'a@b.com' for key 'xtalk_uniq_email_unique'", + knexPrefixed: + 'insert into `xtalk_uniq` (`email`, `id`, `title`) values ' + + "('a@b.com', '2', 'y') - Duplicate entry 'a@b.com' for key 'xtalk_uniq_email_unique'", + column: undefined, + }, + { + label: 'mysql: the same on a PRIMARY KEY', + bare: "Duplicate entry '1' for key 'PRIMARY'", + knexPrefixed: + 'insert into `xtalk_uniq` (`email`, `id`, `title`) values ' + + "('other@b.com', '1', 'q') - Duplicate entry '1' for key 'PRIMARY'", + column: undefined, + }, +] as const; + +/** + * The near misses. Every one shares vocabulary with a positive above — SQLite's + * `constraint failed`, Postgres' `violates ... constraint` — which is exactly + * why the limb may not key on either half alone. + */ +const NEAR_MISSES = [ + ['sqlite NOT NULL', 'NOT NULL constraint failed: xtalk_plain.req'], + ['sqlite FOREIGN KEY', 'foreign key mismatch - "xa_child" referencing "xa_parent"'], + ['postgres not-null', 'null value in column "req" of relation "xtalk_plain" violates not-null constraint'], + ['postgres foreign key', 'insert or update on table "xa_child" violates foreign key constraint "fk_pe"'], + ['mysql not-null', "Column 'req' cannot be null"], + [ + 'mysql foreign key', + "Can't create table `os8590`.`xa_child` (errno: 150 \"Foreign key constraint is incorrectly formed\")", + ], +] as const; + +describe('[#8590] an ABSENT unique constraint is never a unique violation', () => { + for (const sentence of ABSENCE) { + it(`${sentence.label} — bare server sentence`, () => { + expect(isUniqueViolationError(new Error(sentence.bare))).toBe(false); + }); + + it(`${sentence.label} — knex-prefixed`, () => { + expect(isUniqueViolationError(new Error(sentence.knexPrefixed))).toBe(false); + }); + + it(`${sentence.label} — as a plain string`, () => { + expect(isUniqueViolationError(sentence.bare)).toBe(false); + }); + + /** + * The column extractor is gated on the predicate, so a `false` verdict + * must take the column answer with it. A caller that got `email` here + * would render "a record with this email already exists" for a table + * that has no unique index on `email` at all. + */ + it(`${sentence.label} — names no conflicting column either`, () => { + expect(uniqueViolationColumn(new Error(sentence.knexPrefixed))).toBeUndefined(); + }); + } + + /** + * The discriminator, stated as a test rather than only in prose: this is + * the sentence a negative lookahead on SQLite's wording would still claim. + * It is the reason the limb is an allowlist of violation phrasings. + */ + it('postgres 42830 is the case that rules out a lookahead on SQLite’s sentence', () => { + expect(PG_FK_ABSENCE).toMatch(/unique constraint/i); + expect(PG_FK_ABSENCE).not.toMatch(/PRIMARY KEY or/i); + expect(isUniqueViolationError(new Error(PG_FK_ABSENCE))).toBe(false); + }); +}); + +describe('[#8590] every measured violation spelling still answers true', () => { + for (const violation of VIOLATIONS) { + it(`${violation.label} — bare server sentence`, () => { + expect(isUniqueViolationError(new Error(violation.bare))).toBe(true); + }); + + it(`${violation.label} — knex-prefixed`, () => { + expect(isUniqueViolationError(new Error(violation.knexPrefixed))).toBe(true); + }); + + it(`${violation.label} — as a plain string`, () => { + expect(isUniqueViolationError(violation.bare)).toBe(true); + }); + + it(`${violation.label} — the column answer is unchanged`, () => { + expect(uniqueViolationColumn(new Error(violation.knexPrefixed))).toBe(violation.column); + }); + } + + /** + * Ruling on #8590: the narrowing had to keep BOTH dialects' genuine + * spellings, because the limb it replaced was inherited verbatim from the + * REST branch and covered both. Asserted as one statement so a future + * narrowing that keeps only one of them cannot pass. + */ + it('keeps both spellings the retired `unique constraint` limb covered', () => { + expect(isUniqueViolationError('UNIQUE constraint failed: sys_user.email')).toBe(true); + expect( + isUniqueViolationError('duplicate key value violates unique constraint "sys_user_email_key"'), + ).toBe(true); + }); +}); + +describe('[#8590] the near misses that share the vocabulary', () => { + for (const [label, message] of NEAR_MISSES) { + it(`${label} is not a unique violation`, () => { + expect(isUniqueViolationError(new Error(message))).toBe(false); + }); + } +}); + +describe('[#8590] the code and errno channels are untouched by the narrowing', () => { + /** + * The narrowing was to the MESSAGE channel only. These are the channels a + * driver sets when it has a real conflict, and a message-limb change must + * not have moved them — measured `code` / `errno` values, message + * deliberately uninformative so only the channel under test can answer. + */ + const CHANNELS: Array<[string, Record]> = [ + ['postgres SQLSTATE 23505', { code: '23505' }], + ['mysql ER_DUP_ENTRY', { code: 'ER_DUP_ENTRY' }], + ['mysql errno 1062', { errno: 1062 }], + ['mysql numeric code 1062', { code: 1062 }], + ['sqlite SQLITE_CONSTRAINT_UNIQUE', { code: 'SQLITE_CONSTRAINT_UNIQUE' }], + ]; + + for (const [label, channel] of CHANNELS) { + it(`${label} still answers true on the code channel alone`, () => { + expect(isUniqueViolationError(Object.assign(new Error('insert failed'), channel))).toBe(true); + }); + } + + /** + * ⚠️ The absence sentences carry codes too, and those codes are NOT in the + * vocabulary — SQLite answers the generic `SQLITE_ERROR` and Postgres + * answers 42830 / 42P10 (`invalid_column_reference`). Pinned because a + * later "let's also read the code" change is exactly how the message-side + * fix would be undone from the other channel. + */ + it('does not claim an absence sentence through its code channel', () => { + expect( + isUniqueViolationError(Object.assign(new Error(ABSENCE[0].bare), { code: 'SQLITE_ERROR' })), + ).toBe(false); + expect( + isUniqueViolationError(Object.assign(new Error(ABSENCE[1].bare), { code: '42P10' })), + ).toBe(false); + expect( + isUniqueViolationError(Object.assign(new Error(PG_FK_ABSENCE), { code: '42830' })), + ).toBe(false); + }); +}); + +describe('[#8590] the driver refusal that wraps the raw error as `cause`', () => { + /** + * `SqlDriver.upsert` recognises the unbacked target and throws a refusal + * that keeps the raw driver error as its own `cause` — and this predicate + * walks `cause`. So before #8590 the refusal ITSELF answered `true` here, + * one step down, even though its own message says the index is missing. + * + * Nothing user-visible depended on that: the refusal declares + * `status: 400`, and `rest-server.ts` reads `declaredHttpStatus` before it + * reaches the unique-violation branch, so the 400 wins. That gate is the + * only thing that stood between this and a 409 on the wire, which is why + * the verdict is pinned here rather than left to it. + */ + const refusal = () => + Object.assign( + new Error( + '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 the database refuses ' + + 'the statement.', + ), + { code: 'VALIDATION_ERROR', status: 400, cause: new Error(ABSENCE[0].knexPrefixed) }, + ); + + it('is not a unique violation, and neither is the cause it carries', () => { + expect(isUniqueViolationError(refusal())).toBe(false); + }); + + it('its own prose is not a unique violation either', () => { + expect(isUniqueViolationError(refusal().message)).toBe(false); + }); + + /** + * The other direction, unchanged: a refusal wrapping a REAL conflict is + * still recognised through the same `cause` walk. The walk was never the + * defect — the vocabulary it applied was. + */ + it('still reaches a real conflict one step down the cause chain', () => { + const wrapped = Object.assign(new Error('Write failed'), { + cause: new Error('duplicate key value violates unique constraint "sys_user_email_key"'), + }); + expect(isUniqueViolationError(wrapped)).toBe(true); + }); +}); diff --git a/packages/types/src/unique-violation.ts b/packages/types/src/unique-violation.ts index 662d6890e2..1d360f653c 100644 --- a/packages/types/src/unique-violation.ts +++ b/packages/types/src/unique-violation.ts @@ -75,16 +75,17 @@ * "add a unique index" sends an operator after an index that is already there. * Neither predicate may grow a limb belonging to the other. * - * ⚠️ This predicate is ALREADY on the wrong side of that line for one dialect: - * `message`'s `unique constraint` limb matches SQLite's *missing*-index - * sentence, which ends `…any PRIMARY KEY or UNIQUE constraint`, so an unbacked - * conflict target is reported here as a violation of a constraint that does not - * exist. Measured on the real driver error and filed as **#8590** — read it - * before touching `UNIQUE_VIOLATION.message`, because the naive narrowing also - * drops Postgres' `violates unique constraint "..."`, which this limb has - * covered since it was inherited verbatim from the REST branch it replaced. - * `unbacked-conflict-target.test.ts` pins both predicates' verdicts per dialect - * so the fix cannot land silently in either direction. + * ⚠️ This predicate WAS on the wrong side of that line, and #8590 moved it + * back. The `message` limb used to read `unique constraint` as a bare word + * pair, which matched every sentence containing those two words **including + * the ones saying the constraint is absent**. Since #8590 the limb requires a + * VIOLATION phrasing — `unique constraint failed` (SQLite) or + * `violates unique constraint` (Postgres) — so a sentence that merely mentions + * a unique constraint no longer answers yes. The reasoning, and the measured + * sentences that forced it, are on {@link UNIQUE_VIOLATION} below; + * `unique-violation-absence-sentences.test.ts` pins the absence sentences and + * `unbacked-conflict-target.test.ts` pins both predicates' verdicts per dialect, + * so neither the fix nor a fresh drift can land silently in either direction. */ /** @@ -125,9 +126,12 @@ interface UniqueViolationSignature { * 409**, which is what makes routing REST through this predicate incapable of * narrowing a verdict a client relies on today: * - * - `unique constraint` — SQLite's `UNIQUE constraint failed: t.c` *and* - * Postgres' `... violates unique constraint "..."`. Inherited verbatim from - * the REST limb being replaced. + * - `unique constraint failed` — SQLite's `UNIQUE constraint failed: t.c`. + * - `violates unique constraint` — Postgres' `... violates unique constraint + * "..."`. These two replaced a single bare `unique constraint` limb that was + * inherited verbatim from the REST branch; see "Why a VIOLATION phrasing" + * below for the sentences that forced the split. Both dialects' genuine + * spellings are preserved exactly — that was the constraint on the fix. * - `unique violation` — inherited verbatim from the same limb (SQLSTATE * 23505's condition name, which some transports render as prose). * - `duplicate key` — Postgres' `duplicate key value violates ...` @@ -141,11 +145,45 @@ interface UniqueViolationSignature { * worse bug than the one being fixed — a not-null violation answered as * `409 UNIQUE_VIOLATION` tells the client to change a value that is not the * problem, and 409 is a status an SDK will not retry. + * + * ## Why a VIOLATION phrasing, not the word pair (#8590) + * + * The limb used to be a bare `unique constraint`, and a word pair is not a + * condition: databases put those two words in sentences that say a unique + * constraint is **ABSENT** just as readily as in ones that say a row broke it. + * Both spellings below were raised on real servers — SQLite via + * better-sqlite3, PostgreSQL 16.13 via `pg` 8.22.0, both through knex 3.3.0 — + * and the bare limb answered `true` to every one of them: + * + * ``` + * # SQLITE — no unique index backs the ON CONFLICT target (#8445, #8590) + * ON CONFLICT clause does not match any PRIMARY KEY or UNIQUE constraint + * + * # POSTGRES 42830 — a FOREIGN KEY referencing a non-unique column + * there is no unique constraint matching given keys for referenced table "t" + * ``` + * + * The Postgres sentence is why this is an allowlist of violation phrasings and + * not a negative lookahead on SQLite's sentence. #8590 was filed believing the + * collision was SQLite-only and that Postgres escaped "by luck of word order"; + * measuring the dialects for the fix found 42830, where Postgres puts the same + * two words adjacent in its own absence sentence. A lookahead keyed on the + * SQLite wording answers `true` there — it is a blocklist, and it can only ever + * enumerate the absence sentences somebody already tripped over. Requiring a + * violation phrasing inverts the default to match this module's stated one: + * **unrecognised is `false`**, so a sentence nobody has measured is not a + * conflict until a limb says it is. + * + * ⚠️ The three supported dialect families are exactly sqlite / postgres / mysql + * (`sql-driver.ts` recognises no others), and all three were measured on live + * servers for #8590, in both directions, including MySQL's `Duplicate entry` + * path. A dialect added later needs its violation spelling added HERE, measured + * off a thrown error — not a loosened limb. */ const UNIQUE_VIOLATION: UniqueViolationSignature = { codes: new Set(['23505', 'ER_DUP_ENTRY', 'SQLITE_CONSTRAINT_UNIQUE']), errnos: new Set([1062]), - message: /unique constraint|unique violation|duplicate key|duplicate entry/i, + message: /unique constraint failed|violates unique constraint|unique violation|duplicate key|duplicate entry/i, }; /** How far to follow an `error.cause` chain — drivers wrap, but not deeply. */ From 3ebd8608e7308a8a555ca17d3eff8eb1beb52355 Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 14 Aug 2026 16:45:50 +0000 Subject: [PATCH 2/2] docs(types): correct unbacked-conflict-target's module head after #8590 (#8590) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The paragraph still described the superstring collision as live and reasoned from it, and conditioned the disjointness prohibition on "while #8590 is open". All three claims are now false: the predicate no longer claims that error, the issue is closed, and the prohibition was never meant to expire — the two predicates answer inverse questions, so a limb travelling between them produces a confident inverted answer permanently, not until some card lands. Also records what the dialect sweep disproved: Postgres did not escape the collision "by luck of word order". PG 42830 puts `unique constraint` adjacent in its own absence sentence, which is why the fix is an allowlist of violation phrasings rather than a negative lookahead on SQLite's wording. Prose only — no emitted code changes, so no changeset. Folds in #8732. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01NaS1PAHJcPfAA2acnV53Tn --- .../types/src/unbacked-conflict-target.ts | 39 ++++++++++++++----- 1 file changed, 30 insertions(+), 9 deletions(-) diff --git a/packages/types/src/unbacked-conflict-target.ts b/packages/types/src/unbacked-conflict-target.ts index fc1ac6dbf8..8d67767c95 100644 --- a/packages/types/src/unbacked-conflict-target.ts +++ b/packages/types/src/unbacked-conflict-target.ts @@ -20,17 +20,38 @@ * warning is repeated at both call sites and in `unique-violation.ts` because * it is the most expensive mistake available anywhere near this question. * - * ⚠️ The separation is **not clean today, in the pre-existing direction**, and - * pinning it is what found that: `isUniqueViolationError` claims SQLite's + * ⛔ **Nothing below may take a limb from that vocabulary, or give one to it.** + * Unconditional, and permanent: the two predicates answer inverse questions, so + * a limb that travels between them produces a confident inverted answer. This + * prohibition was once written as holding "while #8590 is open", which was + * wrong twice over — it reads as expiring, and #8590 has since closed. + * + * ⚠️ The separation **was** broken in the pre-existing direction, and pinning + * it is what found that: `isUniqueViolationError` claimed SQLite's * unbacked-target error, because that sentence ends `…PRIMARY KEY or UNIQUE - * constraint` and its vocabulary matches the word pair `unique constraint` - * wherever it appears — including inside a sentence saying the constraint is - * ABSENT. Filed as #8590; not fixed by #8567, which would have moved verdicts - * in six consuming packages on a card that measured a different question. + * constraint` and its vocabulary matched the word pair `unique constraint` + * wherever it appeared — including inside a sentence saying the constraint is + * ABSENT. #8567 filed that as #8590 and pinned it rather than fixing it, which + * would have moved verdicts in six consuming packages on a card that measured a + * different question. **#8590 has since closed it**: that predicate's message + * limb now requires a VIOLATION phrasing — `unique constraint failed` (SQLite) + * or `violates unique constraint` (Postgres) — so merely mentioning a unique + * constraint no longer answers yes. + * + * ⚠️ Postgres was believed to escape that collision "by luck of word order", + * its `unique or exclusion constraint` not being adjacent. #8590's dialect + * sweep disproved it: PG **42830**, `there is no unique constraint matching + * given keys for referenced table "t"` — a FOREIGN KEY referencing a non-unique + * column — puts the pair adjacent in Postgres' own ABSENCE sentence. Both + * dialects had the collision; only SQLite's instance sat on the path this file + * measures. That is why the fix is an allowlist of violation phrasings and not + * a negative lookahead on SQLite's sentence, which would still answer `true` + * there. + * * `unbacked-conflict-target.test.ts` records both predicates' verdicts on every - * measured text, per dialect, so neither the fix nor a fresh drift can land - * silently. Nothing below may take a limb from that vocabulary, or give one to - * it, while #8590 is open. + * measured text, per dialect, and `unique-violation-absence-sentences.test.ts` + * pins the absence sentences on both sides — so neither a fix nor a fresh drift + * can land silently in either direction. * * ## What each dialect actually says — measured, never transcribed *