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/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 * 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. */