From d4c4fd80bcec879eabf717c9dbe2d9872f3e49d1 Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 8 Aug 2026 02:31:25 +0000 Subject: [PATCH] =?UTF-8?q?fix(types,rest):=20one=20named=20unique-violati?= =?UTF-8?q?on=20predicate=20=E2=80=94=20MySQL=20conflicts=20are=20409,=20n?= =?UTF-8?q?ot=20500=20(#6250)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `mapDataError`'s 409 `UNIQUE_VIOLATION` branch was nested inside the `looksLikeInternalErrorLeak()` true-branch and keyed on the substrings `unique constraint` / `unique violation`. MySQL says `ER_DUP_ENTRY: Duplicate entry '…' for key '…'`, which matches no limb of that heuristic, so a MySQL conflict never reached the `if` and fell out of `UNCLASSIFIED_FAULT` as 500 INTERNAL_ERROR — on every unique conflict, against a contract that registers `UNIQUE_VIOLATION`. Introduces `isUniqueViolationError` in `@objectstack/types` (the home all four consumers already depend on) reading `code` / `errno` / `message` / `cause`, seeded from the four hand-written implementations the issue inventories, and routes the REST mapping through it — hoisted ABOVE the leak classifier rather than widening it, so no unrelated driver text is reclassified as safe to expose. The 409 body stays fixed text: MySQL embeds the offending user value and Postgres the index/column names. Measured before/after through the real mapper: mysql bare 500 INTERNAL_ERROR, mysql knex-wrapped 500 DATABASE_ERROR and postgres SQLSTATE-only 500 INTERNAL_ERROR all become 409 UNIQUE_VIOLATION; sqlite and postgres message spellings were already 409 and are unchanged. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_017uFVNMmTxLpmfQYiuKM1Yx --- .../unique-violation-shared-predicate.md | 58 +++ packages/rest/src/rest-server.ts | 76 +++- .../rest-unique-violation-dialects.test.ts | 413 ++++++++++++++++++ packages/types/src/index.ts | 4 + packages/types/src/unique-violation.test.ts | 94 ++++ packages/types/src/unique-violation.ts | 174 ++++++++ 6 files changed, 807 insertions(+), 12 deletions(-) create mode 100644 .changeset/unique-violation-shared-predicate.md create mode 100644 packages/rest/src/rest-unique-violation-dialects.test.ts create mode 100644 packages/types/src/unique-violation.test.ts create mode 100644 packages/types/src/unique-violation.ts diff --git a/.changeset/unique-violation-shared-predicate.md b/.changeset/unique-violation-shared-predicate.md new file mode 100644 index 0000000000..64e5076cff --- /dev/null +++ b/.changeset/unique-violation-shared-predicate.md @@ -0,0 +1,58 @@ +--- +"@objectstack/types": patch +"@objectstack/rest": patch +--- + +fix(types,rest): one named unique-violation predicate — a MySQL conflict is 409 UNIQUE_VIOLATION, not 500 (#6250) + +**On MySQL, every unique-constraint conflict came back as `500 INTERNAL_ERROR`.** +The API contract registers `UNIQUE_VIOLATION` as a 409 code +(`packages/spec/src/api/error-code-ledger.zod.ts`), so a front end had no way to +tell "this email is already taken" from "the server fell over" — no retry advice, +no field to point at, and a 5xx in the operator's dashboards for what is an +ordinary client outcome. SQLite and Postgres deployments never saw it, which is +why it survived: their conflict prose happens to contain the words the mapping +looked for. + +**Cause: the conflict verdict was nested inside a leak heuristic.** REST's 409 +branch lived inside the true-branch of `looksLikeInternalErrorLeak()`, keyed on +the substrings `unique constraint` / `unique violation`. MySQL says +`ER_DUP_ENTRY: Duplicate entry '…' for key '…'`, which matches no limb of that +heuristic, so the conflict never reached the `if` at all and fell out of the +terminal `UNCLASSIFIED_FAULT`. Two unrelated questions — "is this a conflict?" +and "would echoing this text leak internals?" — had been fused into one, and +MySQL is where they disagree. + +Measured on the previous release, through the real error mapper: + +``` +mysql, bare message 500 INTERNAL_ERROR → 409 UNIQUE_VIOLATION +mysql, knex-wrapped SQL 500 DATABASE_ERROR → 409 UNIQUE_VIOLATION +postgres, SQLSTATE only 500 INTERNAL_ERROR → 409 UNIQUE_VIOLATION +sqlite, message 409 UNIQUE_VIOLATION (unchanged) +postgres, message 409 UNIQUE_VIOLATION (unchanged) +``` + +So the hole was never MySQL-only: the mapping read one of the two channels +drivers use. A Postgres error carrying SQLSTATE `23505` with unremarkable prose +was a 500 as well. + +**New: `isUniqueViolationError(error)`, exported from `@objectstack/types`.** One +named predicate replaces the substring test, reading every channel a driver +uses — `code` (`23505` / `ER_DUP_ENTRY` / `SQLITE_CONSTRAINT_UNIQUE`), `errno` +(`1062`), the message, and one step down the `cause` chain that pool and +query-builder layers wrap with. Its vocabulary is the union of the four +hand-written copies the repo already carried, so routing REST through it cannot +narrow any verdict clients rely on today; an unrecognised error is never a +conflict, because a false 409 tells an SDK not to retry and points the user at a +value that is fine. + +**The internal-leak classifier is byte-identical.** The fix hoists the conflict +question out of it rather than widening its criteria, so nothing else it guards +is reclassified as safe-to-expose. And the 409 body is fixed text: MySQL embeds +the offending user data in its message (`Duplicate entry 'a@b.com' …`) and +Postgres the index and column names, none of which reaches the client. The full +driver text still reaches the server log. + +No action needed. Clients that already handled `409 UNIQUE_VIOLATION` on SQLite +and Postgres now receive it on MySQL too. diff --git a/packages/rest/src/rest-server.ts b/packages/rest/src/rest-server.ts index 165b323622..1fdaf67c4d 100644 --- a/packages/rest/src/rest-server.ts +++ b/packages/rest/src/rest-server.ts @@ -7,6 +7,7 @@ import { import { isMcpServerEnabled, looksLikeInternalErrorLeak, + isUniqueViolationError, declaresServerFault, INTERNAL_ERROR_MESSAGE, } from '@objectstack/types'; @@ -743,6 +744,62 @@ export function mapDataError(error: any, object?: string): { status: number; bod }; } + // [#6250] Unique-constraint conflict → 409 `UNIQUE_VIOLATION`. + // + // The verdict is the shared `isUniqueViolationError` predicate + // (`@objectstack/types`), and BOTH halves of that sentence are the fix. + // + // **Why it moved up here.** This branch used to live *inside* the + // `looksLikeInternalErrorLeak(raw)` true-branch below, so a conflict was + // recognised only if the message first looked like a server-internals leak + // — two unrelated questions, one nested inside the other. MySQL is where + // they disagree. `ER_DUP_ENTRY: Duplicate entry 'a@b.com' for key + // 'idx_email_unique'` matches not one of the leak heuristic's limbs + // (`sqlite_` / `sqlstate` / `constraint failed` / `unique constraint` / + // `foreign key` / a leading `insert into `/`update `/`select `/`delete + // from `), so it never reached the `if` at all and fell out of + // `UNCLASSIFIED_FAULT` as `500 INTERNAL_ERROR` — on EVERY unique conflict + // in a MySQL deployment, against an API contract that registers + // `UNIQUE_VIOLATION` (`error-code-ledger.zod.ts`). The front end could not + // tell "this email is taken" from "the server fell over". SQLite and + // Postgres hid it: their prose happens to contain `unique constraint`. + // + // The fix is deliberately NOT to teach the leak heuristic about MySQL. + // That heuristic decides what text is unsafe to echo; widening it to reach + // a status mapping would make an information-disclosure rule depend on a + // conflict vocabulary, and every future dialect would have to be taught to + // both. Asking the conflict question by name, first and independently, is + // the #5841 `isMissingTableError` move — and it leaves the leak classifier + // byte-identical, so nothing else it guards is reclassified. + // + // **Why the predicate rather than more substrings.** The message is only + // one of the two channels drivers use. Postgres surfaces SQLSTATE `23505` + // and mysql2 an `ER_DUP_ENTRY` / `errno 1062` — measured, a Postgres error + // carrying the code but a plain message was also a 500 here. The predicate + // reads code, errno, message and one step of `cause`; a substring added to + // this file would have been the fifth private vocabulary, which is the + // defect #6250 is named for. + // + // **The body says nothing the driver said.** The message is a fixed + // sentence and the only interpolated value is the object name the ROUTE + // supplied. That is load-bearing, not incidental: MySQL's text embeds the + // offending USER DATA (`Duplicate entry 'acme@example.com' …`) and + // Postgres' embeds the index and column names, so echoing the driver here + // would trade a status-code bug for an information-disclosure one. Pinned + // in `rest-unique-violation-dialects.test.ts`. The full text still reaches + // the operator: `handleRouteError` / `logWithheldServerFault` log the + // original error untouched. + if (isUniqueViolationError(error)) { + return { + status: 409, + body: { + error: 'A record with this value already exists', + code: 'UNIQUE_VIOLATION', + ...(object ? { object } : {}), + }, + }; + } + const raw = String(error?.message ?? error ?? ''); const lower = raw.toLowerCase(); @@ -945,18 +1002,13 @@ export function mapDataError(error: any, object?: string): { status: number; bod // returned raw SQL to clients. Behaviour here is unchanged; only the // predicate's home moved. if (looksLikeInternalErrorLeak(raw)) { - // Surface unique-constraint violations as a structured 409 so - // the UI can map them to "this value already exists". - if (lower.includes('unique constraint') || lower.includes('unique violation')) { - return { - status: 409, - body: { - error: 'A record with this value already exists', - code: 'UNIQUE_VIOLATION', - ...(object ? { object } : {}), - }, - }; - } + // [#6250] The unique-constraint 409 used to be nested HERE, keyed on + // `unique constraint` / `unique violation`. Both substrings are now + // limbs of the shared `isUniqueViolationError` predicate, which runs + // far above this line and unconditionally — so this branch cannot + // narrow the verdict, and a conflict no longer has to look like a leak + // to be recognised as one. What is left here is the original job: + // withhold text that would ship driver internals. return DATA_STORE_FAULT(); } return UNCLASSIFIED_FAULT(); diff --git a/packages/rest/src/rest-unique-violation-dialects.test.ts b/packages/rest/src/rest-unique-violation-dialects.test.ts new file mode 100644 index 0000000000..4a9d702d23 --- /dev/null +++ b/packages/rest/src/rest-unique-violation-dialects.test.ts @@ -0,0 +1,413 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * #6250 — one dialect table, driven through BOTH faces of the unique-violation + * verdict. + * + * The defect was a fork: four hand-written vocabularies answered "is this a + * unique-constraint violation?" and no two agreed, so the REST mapping's copy + * could miss MySQL entirely without any other copy noticing. A fix that left + * each face with its own fixtures would have rebuilt exactly that — the next + * dialect added to the predicate but not to the mapping (or the reverse) would + * still ship green. + * + * So {@link DIALECT_SAMPLES} below is the single source, and every case runs + * twice: + * + * face 1 — `isUniqueViolationError` (`@objectstack/types`), the predicate; + * face 2 — `mapDataError` (this package), the wire envelope a client sees. + * + * A dialect that regresses on either face goes red here, and adding a dialect + * to only one of them cannot go green. + * + * **This file lives in `@objectstack/rest` and not in the predicate's own + * package** for the one reason that matters: `@objectstack/types` cannot import + * `@objectstack/rest` (that is the dependency direction), so this is the only + * package that can see both faces at once. `packages/types/src/unique-violation.test.ts` + * is its complement, not a second copy — it pins the shapes a table of realistic + * driver errors cannot express (non-object throws, `cause` depth, null-safety) + * and deliberately does NOT restate the dialect vocabulary. + * + * Samples are seeded from the four implementations #6250 inventories — between + * them they encode what the drivers we ship actually emit — plus the two + * spellings the premise re-measurement turned up (a knex-prefixed MySQL message, + * and a Postgres error whose only unique-violation signal is SQLSTATE on `code`). + */ + +import { describe, it, expect } from 'vitest'; +import { isUniqueViolationError, looksLikeInternalErrorLeak } from '@objectstack/types'; +import { mapDataError } from './rest-server.js'; + +/** + * The user data value MySQL embeds in its conflict message. Kept as a named + * constant because two different assertions stand on it: it is what makes the + * sample realistic, and it is what the 409 body must never echo. + */ +const OFFENDING_VALUE = 'acme@example.com'; + +/** The index name Postgres and MySQL name in their conflict text. */ +const OFFENDING_INDEX = 'idx_email_unique'; + +interface DialectSample { + /** Which driver family emits this text. */ + readonly dialect: 'postgres' | 'mysql' | 'sqlite'; + /** Human label — also the test name. */ + readonly label: string; + /** Which channel carries the signal, so a regression names the channel it lost. */ + readonly channel: 'code' | 'message' | 'errno' | 'cause'; + /** Built fresh per assertion so no test can mutate another's fixture. */ + readonly build: () => unknown; + /** Face 1: what the shared predicate must answer. */ + readonly unique: boolean; + /** Face 2: the exact wire envelope `mapDataError` must produce. */ + readonly rest: { readonly status: number; readonly code: string }; +} + +/** Attach driver metadata to an `Error` the way a real driver does. */ +function driverError(message: string, extra: Record = {}): Error { + return Object.assign(new Error(message), extra); +} + +/** + * The shared table. + * + * Positives cover each dialect in BOTH spellings the issue names — the + * machine-readable code and the message substring — because the pre-fix mapping + * read only the second, and only for two of the three dialects. + * + * Negatives are the other constraint failures each dialect can raise on the very + * same INSERT. They matter as much as the positives: a predicate that answers + * "unique" too often is a worse bug than the one being fixed, because 409 tells + * an SDK not to retry and points the user at a value that is not the problem. + * Their expected REST envelopes are the *status quo* — this change must not move + * them. + */ +const DIALECT_SAMPLES: readonly DialectSample[] = [ + // ---------------------------------------------------------------- MySQL + // The reported defect. Before #6250 this was `500 INTERNAL_ERROR`: the + // message matches no limb of `looksLikeInternalErrorLeak`, so it never + // reached the 409 branch nested inside it. + { + dialect: 'mysql', + label: 'ER_DUP_ENTRY — bare driver message (the #6250 report)', + channel: 'message', + build: () => + driverError( + `ER_DUP_ENTRY: Duplicate entry '${OFFENDING_VALUE}' for key '${OFFENDING_INDEX}'`, + { code: 'ER_DUP_ENTRY', errno: 1062 }, + ), + unique: true, + rest: { status: 409, code: 'UNIQUE_VIOLATION' }, + }, + // The same conflict as knex actually re-throws it — the failing statement + // prefixed to the driver's reason. Before #6250 this was `500 + // DATABASE_ERROR`: `insert into ` DID trip the leak heuristic, but the + // nested substring test still did not recognise MySQL, so the conflict was + // reported as a database fault. Same hole, second spelling. + { + dialect: 'mysql', + label: 'ER_DUP_ENTRY — knex-prefixed statement + reason', + channel: 'message', + build: () => + driverError( + 'insert into `sys_user` (`email`) values (?) - ' + + `ER_DUP_ENTRY: Duplicate entry '${OFFENDING_VALUE}' for key '${OFFENDING_INDEX}'`, + { code: 'ER_DUP_ENTRY', errno: 1062 }, + ), + unique: true, + rest: { status: 409, code: 'UNIQUE_VIOLATION' }, + }, + // `code` alone, with prose that says nothing recognisable. mysql2 sets both + // fields; this isolates the code channel so losing it cannot hide behind + // the message channel still passing. + { + dialect: 'mysql', + label: 'ER_DUP_ENTRY — code channel only', + channel: 'code', + build: () => driverError('insert failed', { code: 'ER_DUP_ENTRY' }), + unique: true, + rest: { status: 409, code: 'UNIQUE_VIOLATION' }, + }, + // The numeric twin of the same condition, isolated the same way. + { + dialect: 'mysql', + label: 'errno 1062 — numeric channel only', + channel: 'errno', + build: () => driverError('insert failed', { errno: 1062 }), + unique: true, + rest: { status: 409, code: 'UNIQUE_VIOLATION' }, + }, + // Pool/query-builder layers re-throw with the original attached. + { + dialect: 'mysql', + label: 'ER_DUP_ENTRY — wrapped as `cause`', + channel: 'cause', + build: () => + driverError('Write failed', { + cause: driverError( + `ER_DUP_ENTRY: Duplicate entry '${OFFENDING_VALUE}' for key '${OFFENDING_INDEX}'`, + { code: 'ER_DUP_ENTRY', errno: 1062 }, + ), + }), + unique: true, + rest: { status: 409, code: 'UNIQUE_VIOLATION' }, + }, + // Negative: MySQL's NOT NULL. Stays the structured 400 the field-level + // branch already produces — a required value, not a taken one. + { + dialect: 'mysql', + label: 'ER_BAD_NULL_ERROR — not-null is NOT a unique violation', + channel: 'message', + build: () => + driverError("ER_BAD_NULL_ERROR: Column 'email' cannot be null", { + code: 'ER_BAD_NULL_ERROR', + errno: 1048, + }), + unique: false, + rest: { status: 400, code: 'VALIDATION_FAILED' }, + }, + // Negative: MySQL's foreign key. Note the near-miss wording — it carries + // both "constraint" and a quoted key name, and must still not read as unique. + { + dialect: 'mysql', + label: 'ER_NO_REFERENCED_ROW_2 — foreign key is NOT a unique violation', + channel: 'message', + build: () => + driverError( + 'ER_NO_REFERENCED_ROW_2: Cannot add or update a child row: a foreign key constraint fails ' + + '(`app`.`sys_order`, CONSTRAINT `fk_customer` FOREIGN KEY (`customer_id`) REFERENCES `sys_user` (`id`))', + { code: 'ER_NO_REFERENCED_ROW_2', errno: 1452 }, + ), + unique: false, + rest: { status: 500, code: 'DATABASE_ERROR' }, + }, + + // ------------------------------------------------------------- PostgreSQL + { + dialect: 'postgres', + label: 'duplicate key value violates unique constraint — message', + channel: 'message', + build: () => + driverError( + `duplicate key value violates unique constraint "${OFFENDING_INDEX}"`, + { code: '23505' }, + ), + unique: true, + rest: { status: 409, code: 'UNIQUE_VIOLATION' }, + }, + // SQLSTATE alone. Before #6250 this was `500 INTERNAL_ERROR` too — the + // mapping read no code channel at all, so the hole was never MySQL-only. + { + dialect: 'postgres', + label: 'SQLSTATE 23505 — code channel only', + channel: 'code', + build: () => driverError('insert failed', { code: '23505' }), + unique: true, + rest: { status: 409, code: 'UNIQUE_VIOLATION' }, + }, + // The `DETAIL:` line Postgres appends names the column AND the value. + // Included because it is the shape the 409 body must not forward. + { + dialect: 'postgres', + label: 'duplicate key with DETAIL naming column and value', + channel: 'message', + build: () => + driverError( + `duplicate key value violates unique constraint "${OFFENDING_INDEX}" - ` + + `DETAIL: Key (email)=(${OFFENDING_VALUE}) already exists.`, + { code: '23505' }, + ), + unique: true, + rest: { status: 409, code: 'UNIQUE_VIOLATION' }, + }, + // Negative: Postgres NOT NULL (23502). + { + dialect: 'postgres', + label: '23502 not-null — NOT a unique violation', + channel: 'message', + build: () => + driverError( + 'null value in column "email" of relation "sys_user" violates not-null constraint', + { code: '23502' }, + ), + unique: false, + rest: { status: 400, code: 'VALIDATION_FAILED' }, + }, + // Negative: Postgres foreign key (23503). + { + dialect: 'postgres', + label: '23503 foreign key — NOT a unique violation', + channel: 'message', + build: () => + driverError( + 'insert or update on table "sys_order" violates foreign key constraint "sys_order_customer_fkey"', + { code: '23503' }, + ), + unique: false, + rest: { status: 500, code: 'DATABASE_ERROR' }, + }, + + // ------------------------------------------------------------------ SQLite + // Already 409 before #6250 — its prose happens to contain the substring the + // old mapping looked for. Pinned so the migration cannot narrow it. + { + dialect: 'sqlite', + label: 'UNIQUE constraint failed — message', + channel: 'message', + build: () => + driverError('UNIQUE constraint failed: sys_user.email', { + code: 'SQLITE_CONSTRAINT_UNIQUE', + }), + unique: true, + rest: { status: 409, code: 'UNIQUE_VIOLATION' }, + }, + { + dialect: 'sqlite', + label: 'SQLITE_CONSTRAINT_UNIQUE — code channel only', + channel: 'code', + build: () => driverError('insert failed', { code: 'SQLITE_CONSTRAINT_UNIQUE' }), + unique: true, + rest: { status: 409, code: 'UNIQUE_VIOLATION' }, + }, + // Negative: SQLite NOT NULL. Shares the words "constraint failed" with the + // positive above, which is exactly why the predicate must not key on them. + { + dialect: 'sqlite', + label: 'NOT NULL constraint failed — NOT a unique violation', + channel: 'message', + build: () => + driverError('NOT NULL constraint failed: sys_user.email', { + code: 'SQLITE_CONSTRAINT_NOTNULL', + }), + unique: false, + rest: { status: 400, code: 'VALIDATION_FAILED' }, + }, + // Negative: SQLite foreign key — the other "constraint failed" sibling. + { + dialect: 'sqlite', + label: 'FOREIGN KEY constraint failed — NOT a unique violation', + channel: 'message', + build: () => + driverError('FOREIGN KEY constraint failed', { + code: 'SQLITE_CONSTRAINT_FOREIGNKEY', + }), + unique: false, + rest: { status: 500, code: 'DATABASE_ERROR' }, + }, +]; + +/** The positives, for the assertions that only concern conflicts. */ +const CONFLICTS = DIALECT_SAMPLES.filter((s) => s.unique); + +describe('#6250 face 1 — the shared predicate (@objectstack/types)', () => { + it.each(DIALECT_SAMPLES.map((s) => [`${s.dialect}/${s.channel}: ${s.label}`, s] as const))( + '%s', + (_name, sample) => { + expect(isUniqueViolationError(sample.build())).toBe(sample.unique); + }, + ); + + it('covers all three dialects on both channels — the fork this predicate retires', () => { + const covered = new Set(CONFLICTS.map((s) => `${s.dialect}:${s.channel}`)); + expect(covered).toContain('mysql:message'); + expect(covered).toContain('mysql:code'); + expect(covered).toContain('postgres:message'); + expect(covered).toContain('postgres:code'); + expect(covered).toContain('sqlite:message'); + expect(covered).toContain('sqlite:code'); + }); +}); + +describe('#6250 face 2 — the REST wire envelope (mapDataError)', () => { + /** + * `code` AND `status`, never "it stopped being a 500". Asserting the status + * alone would stay green if the conflict came back as, say, 409 + * `DELETE_RESTRICTED` — and `UNIQUE_VIOLATION` is the registered code the + * API contract promises (`packages/spec/src/api/error-code-ledger.zod.ts`), + * which is the whole of what a front end keys on to say "already taken". + */ + it.each(DIALECT_SAMPLES.map((s) => [`${s.dialect}/${s.channel}: ${s.label}`, s] as const))( + '%s', + (_name, sample) => { + const r = mapDataError(sample.build(), 'sys_user'); + expect(r.status).toBe(sample.rest.status); + expect(r.body.code).toBe(sample.rest.code); + }, + ); + + it('names the requested object on a conflict, and omits it when the route had none', () => { + const sample = CONFLICTS[0]; + expect(mapDataError(sample.build(), 'sys_user').body.object).toBe('sys_user'); + expect(mapDataError(sample.build()).body).not.toHaveProperty('object'); + }); +}); + +/** + * The status-code fix must not become an information-disclosure regression. + * + * MySQL interpolates the OFFENDING USER DATA into its conflict message + * (`Duplicate entry 'acme@example.com' …`) and Postgres interpolates the index + * name and, in its `DETAIL:` line, the column and the value. A 409 that + * forwarded any of that would hand one tenant another tenant's data through an + * error body — while the pre-#6250 500 that this replaces disclosed nothing. + */ +describe('#6250 — the 409 body echoes nothing the driver said', () => { + it.each(CONFLICTS.map((s) => [`${s.dialect}: ${s.label}`, s] as const))( + 'withholds the driver text for %s', + (_name, sample) => { + const err = sample.build(); + const r = mapDataError(err, 'sys_user'); + const wire = JSON.stringify(r.body); + + expect(r.status).toBe(409); + expect(wire).not.toContain(OFFENDING_VALUE); + expect(wire).not.toContain(OFFENDING_INDEX); + expect(wire).not.toContain((err as Error).message); + // The physical column, and the SQL a knex-wrapped error carries. + expect(wire.toLowerCase()).not.toContain('insert into'); + expect(wire.toLowerCase()).not.toContain('sys_user.email'); + }, + ); + + it('says the same sentence for every dialect — the body is fixed text, not driver prose', () => { + const bodies = new Set( + CONFLICTS.map((s) => String(mapDataError(s.build(), 'sys_user').body.error)), + ); + expect(bodies).toEqual(new Set(['A record with this value already exists'])); + }); +}); + +/** + * The leak classifier was deliberately left byte-identical (#6250's security + * flag): the fix hoists the conflict question OUT of it rather than widening + * its criteria, so nothing else it guards can be reclassified as safe-to-expose + * as a side effect. These pin the two halves of that. + */ +describe('#6250 — the fix did not widen the internal-leak classifier', () => { + it('a MySQL conflict is still not classified as a leak — it no longer has to be', () => { + const err = driverError( + `ER_DUP_ENTRY: Duplicate entry '${OFFENDING_VALUE}' for key '${OFFENDING_INDEX}'`, + { code: 'ER_DUP_ENTRY', errno: 1062 }, + ); + // Unchanged: the heuristic still does not recognise this phrasing… + expect(looksLikeInternalErrorLeak(err.message)).toBe(false); + // …and that no longer decides whether the conflict is seen. + expect(mapDataError(err, 'sys_user').status).toBe(409); + }); + + it('driver text that is a leak but NOT a conflict still gets the sanitised 500', () => { + // A genuine SQL dump with no conflict in it: must stay DATABASE_ERROR, + // and must not be swept into 409 by a predicate that says yes too often. + const r = mapDataError( + driverError('select * from `sys_user` where `id` = ? - SQLITE_ERROR: syntax error'), + 'sys_user', + ); + expect(r.status).toBe(500); + expect(r.body.code).toBe('DATABASE_ERROR'); + }); + + it('a deliberate business message is untouched by the new branch', () => { + const r = mapDataError(driverError('删除被阻断:该客户下仍有未结订单'), 'sys_user'); + expect(r.status).not.toBe(409); + }); +}); diff --git a/packages/types/src/index.ts b/packages/types/src/index.ts index 6329f56da3..8e6402c016 100644 --- a/packages/types/src/index.ts +++ b/packages/types/src/index.ts @@ -8,6 +8,10 @@ export * from './error-leak.js'; export * from './keyset-walk.js'; export * from './module-not-found.js'; export * from './response-envelope.js'; +// [#6250] The one named "is this a unique-constraint violation?" predicate. +// Four hand-written vocabularies used to answer it and disagreed about MySQL, +// which is why every MySQL conflict came back 500 instead of 409. +export * from './unique-violation.js'; // [ADR-0120 D5e] The `isolated`-posture install gate for `'global'` uniques — // the pure enumerator both the hard stop (install seam) and the advisories // (`os doctor` / `os migrate plan`) read, so the three cannot drift apart. diff --git a/packages/types/src/unique-violation.test.ts b/packages/types/src/unique-violation.test.ts new file mode 100644 index 0000000000..82f83cfd75 --- /dev/null +++ b/packages/types/src/unique-violation.test.ts @@ -0,0 +1,94 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * #6250 — the predicate's own package-local pins. + * + * **Deliberately NOT a second dialect table.** The dialect vocabulary — every + * Postgres / MySQL / SQLite sample, on both the code and the message channel, + * with its negatives — lives in exactly one place, + * `@objectstack/rest`'s `rest-unique-violation-dialects.test.ts`, where it is + * driven through the predicate AND the REST envelope in the same run. Restating + * it here would rebuild the very fork this predicate exists to retire: two + * tables that can be taught a dialect independently, which is how four + * implementations came to disagree about MySQL in the first place. (The table + * cannot live here instead: `@objectstack/types` cannot import + * `@objectstack/rest`.) + * + * What this file covers is what a table of realistic driver errors cannot + * express — the shapes the predicate is handed by CALLERS rather than by + * drivers, and the boundaries of its search. + */ + +import { describe, it, expect } from 'vitest'; +import { isUniqueViolationError } from './unique-violation.js'; + +describe('isUniqueViolationError — input shapes', () => { + it('accepts a bare string, for callers that already unwrapped `err.message`', () => { + expect(isUniqueViolationError('UNIQUE constraint failed: sys_user.email')).toBe(true); + expect(isUniqueViolationError('NOT NULL constraint failed: sys_user.email')).toBe(false); + }); + + it.each([ + ['undefined', undefined], + ['null', null], + ['a number', 42], + ['a boolean', false], + ['an empty object', {}], + ['an error with no message', new Error()], + ])('is not a conflict: %s', (_label, value) => { + expect(isUniqueViolationError(value)).toBe(false); + }); + + it('reads a numeric `code` as MySQL\'s errno wearing the other field\'s name', () => { + expect(isUniqueViolationError({ code: 1062 })).toBe(true); + expect(isUniqueViolationError({ code: 1452 })).toBe(false); + }); +}); + +describe('isUniqueViolationError — the `cause` chain', () => { + /** + * Pool and query-builder layers re-throw with the original attached, so the + * signal is often one or more steps down. The walk is bounded: an error + * whose `cause` chain is longer than the depth limit is not searched to the + * end, and the conservative default (`false`) is what it falls to. + */ + const nest = (depth: number): unknown => { + let err: unknown = Object.assign(new Error('Duplicate entry'), { code: 'ER_DUP_ENTRY' }); + for (let i = 0; i < depth; i += 1) err = Object.assign(new Error('Write failed'), { cause: err }); + return err; + }; + + it.each([1, 2, 3, 4])('finds a conflict wrapped %i level(s) deep', (depth) => { + expect(isUniqueViolationError(nest(depth))).toBe(true); + }); + + it('stops rather than walking an unbounded chain', () => { + expect(isUniqueViolationError(nest(5))).toBe(false); + }); + + it('a self-referential `cause` terminates instead of recursing forever', () => { + const err: { message: string; cause?: unknown } = { message: 'Write failed' }; + err.cause = err; + expect(isUniqueViolationError(err)).toBe(false); + }); +}); + +describe('isUniqueViolationError — an unrecognised error is never a conflict', () => { + /** + * The default has to be "not a conflict". A false positive answers 409 — + * which an SDK will not retry — and points the user at a value that is + * fine; a false negative costs only the generic envelope that was the + * behaviour before this predicate existed. + */ + it.each([ + ['a business rule from a hook', '删除被阻断:该客户下仍有未结订单'], + ['a validation message', 'email is required'], + ['a missing table', 'no such table: sys_user'], + ['a syntax error', 'near "FROM": syntax error'], + ['a connection failure', 'ECONNREFUSED 127.0.0.1:5432'], + // Shares the words "constraint" and "failed" with SQLite's unique text. + ['a sibling constraint failure', 'CHECK constraint failed: sys_user_age_check'], + ])('is not a conflict: %s', (_label, message) => { + expect(isUniqueViolationError(new Error(message))).toBe(false); + }); +}); diff --git a/packages/types/src/unique-violation.ts b/packages/types/src/unique-violation.ts new file mode 100644 index 0000000000..2092218b81 --- /dev/null +++ b/packages/types/src/unique-violation.ts @@ -0,0 +1,174 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * The one named predicate for "is this driver error a unique-constraint + * violation?" (#6250). + * + * ## The defect this retires + * + * Before this module the repo carried **four** hand-written, mutually different + * answers to that single question — no two covering the same dialects: + * + * | where | judged by | covered | + * |:---|:---|:---| + * | `service-messaging`'s `isUniqueViolation()` | 3 codes + 3 message substrings | all three | + * | `@objectstack/rest`'s `mapDataError` | `unique constraint` / `unique violation` only | **no MySQL** | + * | `@objectstack/rest`'s `sanitizeRowError` | three column-extracting regexes | all three | + * | `driver-sql`'s inline regex | `unique constraint failed\|duplicate entry\|duplicate key value` | all three | + * + * The REST row is the one a user could feel. Its verdict decides whether a + * conflict comes back as the API contract's `409 UNIQUE_VIOLATION` (a + * registered code in `packages/spec/src/api/error-code-ledger.zod.ts`) or as a + * generic `500 INTERNAL_ERROR`, and MySQL's phrasing — + * `ER_DUP_ENTRY: Duplicate entry 'acme@example.com' for key 'idx_email_unique'` + * — matches neither substring. Measured on `origin/main` before this change, + * through the real `mapDataError`: + * + * ``` + * mysql, bare message => 500 INTERNAL_ERROR ← the reported defect + * mysql, knex-prefixed SQL => 500 DATABASE_ERROR ← second spelling, same hole + * postgres, SQLSTATE only => 500 INTERNAL_ERROR ← the code channel was unread + * sqlite, message => 409 UNIQUE_VIOLATION + * postgres, message => 409 UNIQUE_VIOLATION + * ``` + * + * So the hole was never MySQL-only: it was "the mapping reads one channel + * (message substrings) of the two that drivers actually use". SQLite and + * Postgres were invisible survivors because their prose happens to contain the + * words the substring test looks for. + * + * ## Why a predicate rather than a wider heuristic + * + * `looksLikeInternalErrorLeak` (one file over) answers a **different** + * question — "would echoing this text leak server internals?" — and the 409 + * mapping used to be nested *inside* its true-branch, so a message had to look + * like a leak before it could be recognised as a conflict. Those two questions + * have no reason to agree, and MySQL is the case where they don't. Widening the + * leak heuristic to reach the conflict branch would have coupled them harder + * and quietly reclassified unrelated driver text as safe-to-expose; naming the + * conflict question separately unpicks them instead. Same move as #5841's + * `isMissingTableError`, and the same reason. + * + * ## Home + * + * `@objectstack/types` because every consumer of the question already depends + * on it, so adopting the predicate never adds an edge. This module deliberately + * imports nothing. + * + * ## What this predicate does NOT do + * + * It does not name the **conflicting column**. `sanitizeRowError` extracts one + * for the import path, and #5495 wants one to decide whether an autonumber + * collision is retryable — but a structured conflict-column export is a new + * contract surface, so it is deliberately not decided here (#6250's ruling). + * This answers the yes/no question only. + */ + +/** + * One dialect vocabulary, in the three channels drivers actually use. + * + * Same shape as `@objectstack/metadata`'s `DriverErrorSignature` — deliberately, + * because it is the shape the drivers force: Postgres puts SQLSTATE on `code`, + * mysql2 puts a symbolic name on `code` *and* a number on `errno`, and the + * SQLite family often gives nothing but prose. + */ +interface UniqueViolationSignature { + /** `error.code` — Postgres SQLSTATE, mysql2's symbolic name, SQLite's extended result code. */ + readonly codes: ReadonlySet; + /** `error.errno` — MySQL/MariaDB's numeric equivalent of the same condition. */ + readonly errnos: ReadonlySet; + /** `error.message` — the only channel a knex-wrapped or SQLite-family error reliably carries. */ + readonly message: RegExp; +} + +/** + * The union of every unique-violation signal the four pre-existing + * implementations encoded, plus the `errno` channel their `code`-only reads + * missed. + * + * **Seeded from what real drivers emit, not invented here.** Every entry traces + * to one of the four inventoried implementations; nothing was added on a guess: + * + * - `23505` — PostgreSQL SQLSTATE `unique_violation` (from `service-messaging`). + * - `ER_DUP_ENTRY` — mysql2's symbolic name for 1062 (from `service-messaging`). + * - `SQLITE_CONSTRAINT_UNIQUE` — better-sqlite3 / libsql extended result code + * (from `service-messaging`). + * - `1062` — the same MySQL condition on the channel mysql2 *also* sets. The + * one addition, and not a new dialect: `@objectstack/metadata`'s + * `schema-sync-errors.ts` already reads `errno` alongside `code` for exactly + * these drivers, so a code-only read is a known gap rather than a decision. + * + * The message limbs are a **superset of what `mapDataError` already treated as + * 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 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 ...` + * (from `service-messaging` and `driver-sql`). + * - `duplicate entry` — MySQL's `Duplicate entry 'x' for key 'i'` + * (from `service-messaging` and `driver-sql`). **This is the limb whose + * absence made every MySQL conflict a 500.** + * + * Deliberately NOT here: bare `constraint failed`, which SQLite emits for + * NOT NULL and FOREIGN KEY too. A predicate that says "unique" too often is a + * 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. + */ +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, +}; + +/** How far to follow an `error.cause` chain — drivers wrap, but not deeply. */ +const MAX_CAUSE_DEPTH = 4; + +/** + * Whether a thrown driver error is a unique/primary-key constraint violation. + * + * Reads all three channels in turn — `code`, `errno`, `message` — then one step + * down the `cause` chain, because pool and query-builder layers re-throw with + * the original attached. A plain string is judged on the message channel, so a + * caller that has already unwrapped `err.message` can pass it straight in. + * + * **Unrecognised is always `false`.** The default has to be "not a conflict": + * a false positive relabels an unrelated failure as the client's fault (a 409 + * an SDK will not retry, pointing at a value that is fine), while a false + * negative costs only the generic envelope that was the status quo. + * + * @param error - the thrown value, of any shape. + * + * @example + * ```ts + * catch (error) { + * if (isUniqueViolationError(error)) return conflict(); // 409 UNIQUE_VIOLATION + * throw error; + * } + * ``` + */ +export function isUniqueViolationError(error: unknown): boolean { + return matchesUniqueViolation(error, 0); +} + +function matchesUniqueViolation(error: unknown, depth: number): boolean { + if (error === null || error === undefined || depth > MAX_CAUSE_DEPTH) return false; + + if (typeof error === 'string') return UNIQUE_VIOLATION.message.test(error); + if (typeof error !== 'object') return false; + + const err = error as { code?: unknown; errno?: unknown; message?: unknown; cause?: unknown }; + + if (typeof err.code === 'string' && UNIQUE_VIOLATION.codes.has(err.code)) return true; + // Postgres drivers hand SQLSTATE back as a string; a numeric `code` is + // MySQL's errno wearing the other field's name, so it is judged as one. + if (typeof err.code === 'number' && UNIQUE_VIOLATION.errnos.has(err.code)) return true; + if (typeof err.errno === 'number' && UNIQUE_VIOLATION.errnos.has(err.errno)) return true; + if (typeof err.message === 'string' && UNIQUE_VIOLATION.message.test(err.message)) return true; + + return matchesUniqueViolation(err.cause, depth + 1); +}