Skip to content

fix(types,rest): one named unique-violation predicate — MySQL conflicts return 409 UNIQUE_VIOLATION, not 500 (#6250) - #6541

Merged
os-project-manager merged 2 commits into
mainfrom
claude/issue-6250-unique-violation-predicate
Aug 8, 2026
Merged

fix(types,rest): one named unique-violation predicate — MySQL conflicts return 409 UNIQUE_VIOLATION, not 500 (#6250)#6541
os-project-manager merged 2 commits into
mainfrom
claude/issue-6250-unique-violation-predicate

Conversation

@os-project-manager

Copy link
Copy Markdown
Collaborator

Fixes #6250

Premise: verified, and the hole is wider than reported

The issue's core claim holds. Measured against origin/main before any edit, through the real mapDataError:

sample before
MySQL, bare ER_DUP_ENTRY: Duplicate entry 'acme@example.com' for key 'idx_email_unique' 500 INTERNAL_ERROR
MySQL, knex-wrapped (insert into ... - ER_DUP_ENTRY: ...) 500 DATABASE_ERROR
Postgres, SQLSTATE 23505 on code with plain prose 500 INTERNAL_ERROR
SQLite, UNIQUE constraint failed: sys_user.email 409 UNIQUE_VIOLATION
Postgres, duplicate key value violates unique constraint "..." 409 UNIQUE_VIOLATION

Two findings beyond the issue body:

  1. A second MySQL spelling. The knex-wrapped form does trip looksLikeInternalErrorLeak (via the leading insert into ), so it reached the if — and the nested substring test still did not recognise MySQL, so it came back 500 DATABASE_ERROR. Same hole, different envelope.
  2. The defect was never MySQL-only. The mapping read one of the two channels drivers use. A Postgres error carrying SQLSTATE 23505 on code was a 500 as well. The real shape is "the message channel was read, the code channel was not".

One correction to the issue's inventory, for the record: the row claiming import-runner.ts is "仅 Postgres" has drifted. Its sanitizeRowError has covered all three dialects since #3572 (git log -S confirms) — the line the issue cites is the third limb of a three-way chain, not the whole of it. This does not change the premise; the four implementations are still four, and still disagree.

What ships

isUniqueViolationError(error), new in @objectstack/types (packages/types/src/unique-violation.ts) — the home the ruling names, and the one all consumers already depend on, so adopting it adds no dependency edge. Modelled on #5841's isMissingTableError and shaped like @objectstack/metadata's schema-sync-errors.ts: a single signature read across every channel a driver uses — code, errno, message, then 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 issue inventories, so routing REST through it cannot narrow a verdict clients rely on today. The two message limbs unique constraint / unique violation are inherited verbatim from the REST limb being replaced; duplicate key / duplicate entry and the three codes come from service-messaging and driver-sql. The one addition is MySQL's errno 1062 — not a new dialect, just the second field mysql2 sets for the same condition, and schema-sync-errors.ts already reads errno alongside code for exactly these drivers.

mapDataError routes through it, hoisted above the leak classifier. The 409 branch used to live inside the looksLikeInternalErrorLeak(raw) true-branch, so a conflict had to look like a server-internals leak before it could be recognised as a conflict. Those are two unrelated questions and MySQL is where they disagree. The predicate now runs first and unconditionally; what is left in the leak branch is its original job.

Both security constraints, addressed and pinned

1. The leak classifier was NOT widened — it is byte-identical. packages/types/src/error-leak.ts is untouched in this PR (see the diff). The fix hoists the conflict question out of it rather than teaching it MySQL, so nothing else it guards can be reclassified as safe-to-expose as a side effect. Teaching it MySQL would also have coupled an information-disclosure rule to a conflict vocabulary, so every future dialect would have to be taught to both. Pinned: a test asserts the MySQL message is still not classified as a leak, and that this no longer decides whether the conflict is seen.

2. The 409 body echoes nothing the driver said. MySQL interpolates the offending user data into its message (Duplicate entry 'acme@example.com' ...) and Postgres its index name, plus the column and value in its DETAIL: line. The body is fixed text plus the object name the route supplied. Pinned per conflict sample: the serialised body contains neither acme@example.com, nor idx_email_unique, nor the driver message, nor insert into, nor sys_user.email — and every dialect produces the identical sentence. The full driver text still reaches the operator's log unchanged.

Tests — one table, two faces

packages/rest/src/rest-unique-violation-dialects.test.ts holds a single DIALECT_SAMPLES table driven through both faces in the same run: isUniqueViolationError and mapDataError. A dialect that regresses on either goes red, and adding one to only one face cannot go green — which is the point, since the defect being fixed is two copies disagreeing. Samples are seeded from the four inventoried implementations plus the two spellings the re-measurement turned up.

Every case asserts code AND status, never "it stopped being a 500": status alone would stay green if a conflict came back as, say, 409 DELETE_RESTRICTED.

Negatives are pinned per dialect, because a predicate that says "unique" too often is a worse bug than the one being fixed — 409 tells an SDK not to retry and points the user at a value that is fine. Each dialect's NOT NULL and FOREIGN KEY failures keep their current envelopes (400 VALIDATION_FAILED / 500 DATABASE_ERROR). SQLite's negatives are the sharp ones: NOT NULL constraint failed and FOREIGN KEY constraint failed share the words constraint failed with the positive.

packages/types/src/unique-violation.test.ts is a complement, not a second table — it deliberately restates no dialect vocabulary (that would rebuild the fork) and covers only what a table of realistic driver errors cannot express: non-object throws, null-safety, cause depth including the bound and a self-referential chain. The table cannot live in that package instead, because @objectstack/types cannot import @objectstack/rest.

Reverse verification — direction predicted before running

Prediction: reverting only rest-server.ts should turn the MySQL rows and both code-channel rows red, and leave the SQLite/Postgres message rows green — because those two spellings already worked and the fix must not narrow them. Measured, exactly that: 17 failed / 31 passed, and the 17 are precisely the five MySQL positives, postgres/code, sqlite/code, and the derived assertions over the conflict set. First failure reads AssertionError: expected 500 to be 409.

Face 1 stayed entirely green under that revert, which is the shared table earning its keep: the two faces are independently red-able.

Convergence is partial by design, not forgotten

Three consumers still carry their own hand-written unique-violation checks and are deliberately untouched here:

On that last point, for whoever picks it up: this PR makes the column-extraction export cheap but not free. The signature struct already separates the channels, so a uniqueViolationColumn(error) sibling would slot in beside the predicate without restructuring — but it is a new contract surface (which spelling wins when dialects name an index rather than a column?), so it stays its own card and never a rider, per the ruling.

Verification

  • pnpm --filter @objectstack/types --filter @objectstack/rest test — types 165/165, rest 932/932 (65 files), re-run after merging main.
  • pnpm lint — clean. Full turbo run typecheck over packages/*, packages/*/*, apps/*120/120.
  • Every check:* step enumerated from .github/workflows/lint.yml (both jobs, 36 root gates + the spec/examples/downstream set) run one by one — all PASS, re-run after the merge. check:error-code-casing, check:route-envelope and check:engine-double-contract included; no new fake engine was introduced.
  • check:type-check-debt@objectstack/rest TEST_DEBT measures 144 against a recorded 163, with zero errors in the new test file; @objectstack/rest src DEBT measures 2, both pre-existing in package-routes.ts. Full ratchet: "34 ledger entries re-measured, none above its recorded number".
  • node scripts/check-nul-bytes.mjs — OK, plus a control-byte self-scan over every touched file.

Generated by Claude Code

claude added 2 commits August 8, 2026 02:31
…ts are 409, not 500 (#6250)

`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 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_017uFVNMmTxLpmfQYiuKM1Yx
@vercel

vercel Bot commented Aug 8, 2026

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

1 Skipped Deployment
Project Deployment Actions Updated (UTC)
objectstack Ignored Ignored Aug 8, 2026 3:43am

Request Review

@github-actions

github-actions Bot commented Aug 8, 2026

Copy link
Copy Markdown
Contributor

📓 Docs Drift Check

This PR changes 2 package(s): @objectstack/rest, @objectstack/types.

11 hand-written doc(s) reference the affected code and may need an implementation-accuracy re-verification:

  • content/docs/ai/connect-mcp.mdx (via @objectstack/rest)
  • content/docs/api/error-handling-server.mdx (via @objectstack/rest)
  • content/docs/api/index.mdx (via @objectstack/rest)
  • content/docs/permissions/authentication.mdx (via @objectstack/rest)
  • content/docs/plugins/index.mdx (via @objectstack/rest)
  • content/docs/plugins/packages.mdx (via @objectstack/rest, @objectstack/types)
  • content/docs/protocol/kernel/http-protocol.mdx (via @objectstack/rest)
  • content/docs/protocol/kernel/i18n-standard.mdx (via packages/rest)
  • content/docs/releases/implementation-status.mdx (via @objectstack/rest)
  • content/docs/releases/v12.mdx (via @objectstack/rest)
  • content/docs/releases/v17.mdx (via @objectstack/rest)

Advisory only. To re-verify, run the docs-accuracy-audit workflow scoped to these files:
node scripts/docs-audit/affected-docs.mjs origin/main → pass the list as args.docs.

@github-actions github-actions Bot added size/l documentation Improvements or additions to documentation tests tooling labels Aug 8, 2026
@os-project-manager
os-project-manager marked this pull request as ready for review August 8, 2026 04:39
@os-project-manager
os-project-manager added this pull request to the merge queue Aug 8, 2026
Merged via the queue into main with commit 88f9d94 Aug 8, 2026
25 checks passed
@os-project-manager
os-project-manager deleted the claude/issue-6250-unique-violation-predicate branch August 8, 2026 04:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

documentation Improvements or additions to documentation size/l tests tooling

Projects

None yet

2 participants