Skip to content

Commit bbbfcfc

Browse files
qq9340100claude
andauthored
fix(types): require a violation phrasing in isUniqueViolationError's message limb (#8590) (#8730)
* 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 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NaS1PAHJcPfAA2acnV53Tn * docs(types): correct unbacked-conflict-target's module head after #8590 (#8590) 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 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NaS1PAHJcPfAA2acnV53Tn --------- Co-authored-by: Claude <noreply@anthropic.com>
1 parent f047810 commit bbbfcfc

5 files changed

Lines changed: 528 additions & 51 deletions
Lines changed: 67 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,67 @@
1+
---
2+
"@objectstack/types": patch
3+
---
4+
5+
fix(types): `isUniqueViolationError` stops claiming the sentences that say a unique constraint is ABSENT (#8590)
6+
7+
The shared predicate's message limb was a bare `unique constraint`, and a word
8+
pair is not a condition. Every dialect that can say "this row violated a unique
9+
constraint" can also say "there is no unique constraint here", and the same two
10+
words sit adjacent in both — so the predicate answered **true** for errors
11+
meaning the exact opposite of what it detects. `rest-server.ts` maps that
12+
verdict to `409 UNIQUE_VIOLATION`, which tells a client to change a value when
13+
nothing was ever compared, on a status an SDK will not retry.
14+
15+
**Measured on live servers for this fix, all three supported dialect families**
16+
— SQLite via better-sqlite3, PostgreSQL 16.13 via `pg` 8.22.0, MariaDB 10.11.14
17+
via `mysql2` 3.23.1, all through knex 3.3.0 — driving each dialect through both
18+
conditions plus the NOT NULL / FOREIGN KEY near misses:
19+
20+
```
21+
sqlite ON CONFLICT clause does not match any PRIMARY KEY or UNIQUE constraint
22+
-> was true, WRONG (the reported defect, #8590)
23+
postgres there is no unique constraint matching given keys for referenced table "t"
24+
-> was true, WRONG (42830 — found by this fix's dialect sweep)
25+
postgres there is no unique or exclusion constraint matching the ON CONFLICT specification
26+
-> false (the pair is not adjacent here)
27+
mysql the condition cannot arise: knex compiles to ON DUPLICATE KEY UPDATE,
28+
which carries no conflict target (confirmed against a live server)
29+
```
30+
31+
**Postgres was not clean either, and that chose the fix.** #8590 was filed
32+
reading the collision as SQLite-only, with Postgres escaping "by luck of word
33+
order". The sweep raised **42830** — a `FOREIGN KEY` referencing a non-unique
34+
column — where Postgres puts `unique constraint` adjacent in its own absence
35+
sentence. The card offered two candidate fixes; only one survives 42830. A
36+
negative lookahead on SQLite's missing-index sentence is a blocklist that can
37+
only enumerate absence sentences somebody already tripped over, and it answers
38+
`true` on 42830. So the limb now requires a **violation phrasing**
39+
`unique constraint failed` (SQLite) or `violates unique constraint` (Postgres) —
40+
which restores the module's own stated default, *unrecognised is `false`*, to
41+
the message channel.
42+
43+
**Both spellings the retired limb covered are preserved exactly**, which was the
44+
constraint on the fix: the limb was inherited verbatim from the REST branch
45+
#6250 replaced and covered SQLite's `UNIQUE constraint failed: t.c` *and*
46+
Postgres' `... violates unique constraint "..."`. The `unique violation`,
47+
`duplicate key` and `duplicate entry` limbs are untouched, as are the `code` and
48+
`errno` channels — MySQL's `Duplicate entry` path never went through the
49+
narrowed limb at all.
50+
51+
**No user-visible behaviour changes today; this closes a latent inversion.** The
52+
one site compiling a caller-supplied conflict target (`SqlDriver.upsert`)
53+
recognises the unbacked target *first* in its catch and throws a refusal
54+
declaring `status: 400`, and `mapDataError` reads `declaredHttpStatus` before it
55+
reaches the unique-violation branch — so the 409 was gated off the wire by
56+
ordering, not by the verdict. That ordering was the only thing standing between
57+
this and a wrong status, which is why the verdict is now pinned rather than left
58+
to it. A repo-wide scan of every string literal whose verdict moves found no
59+
consumer relying on the old answer: all of them are prose, a different
60+
predicate's vocabulary (`looksLikeInternalErrorLeak` keeps its own list), or
61+
fixtures asserted through the status-passthrough path.
62+
63+
`unbacked-conflict-target.test.ts`'s pin — written by #8567 to point at itself
64+
rather than go quietly green — is **inverted, not deleted**, and
65+
`unique-violation-absence-sentences.test.ts` pins the absence sentences per
66+
dialect in both directions, including the code channel, so re-reading `code`
67+
cannot undo the message-side fix from the other side.

packages/types/src/unbacked-conflict-target.test.ts

Lines changed: 41 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -30,13 +30,16 @@
3030
* vocabulary growing an `ON CONFLICT` one, because that is the file people
3131
* extend.
3232
*
33-
* ⚠️ Running it that way is what found **#8590**: on SQLite the separation is
34-
* ALREADY broken in the pre-existing direction — `isUniqueViolationError`
35-
* claims the unbacked-target error, because SQLite's missing-index sentence
36-
* ends `…PRIMARY KEY or UNIQUE constraint` and that vocabulary matches the word
37-
* pair `unique constraint` wherever it appears. Not fixed here (it moves
38-
* verdicts in six packages); pinned as measured, per dialect, so the fix
39-
* announces itself. See the suite below.
33+
* ⚠️ Running it that way is what found **#8590**: on SQLite the separation was
34+
* broken in the pre-existing direction — `isUniqueViolationError` claimed the
35+
* unbacked-target error, because SQLite's missing-index sentence ends
36+
* `…PRIMARY KEY or UNIQUE constraint` and that vocabulary matched the word pair
37+
* `unique constraint` wherever it appeared. #8567 pinned it as measured rather
38+
* than fixing it (the fix moves verdicts in six consuming packages); **#8590
39+
* has since closed it** by requiring a violation phrasing in that limb, and the
40+
* pin below was inverted rather than deleted — which is what a pin written to
41+
* point at itself is for. The separation is now clean on both dialects, in both
42+
* directions, and the suite below is what keeps it that way.
4043
*/
4144

4245
import { describe, expect, it } from 'vitest';
@@ -157,29 +160,38 @@ describe('[#8567] the `code` channel is deliberately unread — measured over-ma
157160
describe('[#8567] ⚠️ separation from isUniqueViolationError — the inverse condition', () => {
158161
/**
159162
* ⚠️ This suite was written expecting clean disjointness in both
160-
* directions. It went RED on the first run, and the measurement won: on
161-
* SQLite, `isUniqueViolationError` ALREADY claims the unbacked-target
162-
* error. Filed as **#8590**, deliberately not fixed here — narrowing that
163-
* predicate moves verdicts in six consuming packages and needs its own
164-
* measured pass.
163+
* directions. It went RED on the first run and the measurement won: on
164+
* SQLite, `isUniqueViolationError` claimed the unbacked-target error.
165+
* #8567 filed that as **#8590** and pinned the wrong verdict as measured
166+
* rather than fixing it, because narrowing that predicate moves verdicts in
167+
* six consuming packages and needed its own measured pass.
165168
*
166-
* The cause is a superstring collision, not a judgement call. Its message
167-
* limb is `/unique constraint|…/i`, and SQLite's sentence for the MISSING
168-
* index ends `…any PRIMARY KEY or UNIQUE constraint` — the two words sit
169-
* adjacent inside a sentence that says the constraint is absent. Postgres
170-
* escapes only on word order (`unique or exclusion constraint` is not
171-
* adjacent), which is the tell that a word pair is being matched rather
172-
* than a condition.
169+
* **#8590 has since landed, and this pin was INVERTED — that is the pin
170+
* working, not an obstacle to route around.** The cause was a superstring
171+
* collision, not a judgement call: the limb was a bare `unique constraint`,
172+
* and SQLite's sentence for the MISSING index ends `…any PRIMARY KEY or
173+
* UNIQUE constraint`, so the two words sit adjacent inside a sentence that
174+
* says the constraint is ABSENT. The limb now requires a violation
175+
* phrasing (`unique constraint failed` / `violates unique constraint`), so
176+
* mentioning a unique constraint is no longer enough to be claimed as one.
173177
*
174-
* So the pins below record the state as MEASURED, per dialect, rather than
175-
* as hoped. When #8590 lands, the SQLite row goes red and points straight
176-
* at itself — which is the entire reason to pin a known defect instead of
177-
* leaving the direction untested.
178+
* ⚠️ Postgres was believed to escape "by luck of word order" — its
179+
* `unique or exclusion constraint` is not adjacent. That reading was too
180+
* kind: #8590's own dialect sweep raised PG 42830,
181+
* `there is no unique constraint matching given keys for referenced table`,
182+
* where Postgres puts the pair adjacent in its own ABSENCE sentence. Both
183+
* dialects had the collision; only SQLite's instance was on the path this
184+
* file measures. The absence sentences are pinned per dialect in
185+
* `unique-violation-absence-sentences.test.ts`.
186+
*
187+
* Both rows are therefore `false` now, and the map is kept per dialect
188+
* rather than collapsed to a constant so a regression names the dialect it
189+
* came back on.
178190
*/
179191
const UNIQUE_VIOLATION_VERDICT_ON_UNBACKED: Record<string, boolean> = {
180-
// ⚠️ THE DEFECT (#8590). Correct value is `false`; flip it when #8590 lands.
181-
sqlite: true,
182-
// Correct today, and only by luck of word order — see above.
192+
// [#8590] Was `true` — the defect. Inverted when the fix landed.
193+
sqlite: false,
194+
// Correct before #8590 on this sentence, and now correct by rule.
183195
postgres: false,
184196
};
185197

@@ -188,8 +200,9 @@ describe('[#8567] ⚠️ separation from isUniqueViolationError — the inverse
188200
expect(isUnbackedConflictTargetError(new Error(dialect.knexPrefixed))).toBe(true);
189201
expect(
190202
isUniqueViolationError(new Error(dialect.knexPrefixed)),
191-
'if this changed, #8590 either landed (SQLite → false: delete the exception) or ' +
192-
'regressed (Postgres → true: a new limb is matching the missing-index sentence)',
203+
'both dialects are `false` since #8590. A `true` here means the unique-violation ' +
204+
'vocabulary has regrown a limb that matches a sentence saying the constraint is ' +
205+
'ABSENT — the superstring collision #8590 closed, back on this dialect',
193206
).toBe(UNIQUE_VIOLATION_VERDICT_ON_UNBACKED[key]);
194207
});
195208
}

packages/types/src/unbacked-conflict-target.ts

Lines changed: 30 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -20,17 +20,38 @@
2020
* warning is repeated at both call sites and in `unique-violation.ts` because
2121
* it is the most expensive mistake available anywhere near this question.
2222
*
23-
* ⚠️ The separation is **not clean today, in the pre-existing direction**, and
24-
* pinning it is what found that: `isUniqueViolationError` claims SQLite's
23+
* ⛔ **Nothing below may take a limb from that vocabulary, or give one to it.**
24+
* Unconditional, and permanent: the two predicates answer inverse questions, so
25+
* a limb that travels between them produces a confident inverted answer. This
26+
* prohibition was once written as holding "while #8590 is open", which was
27+
* wrong twice over — it reads as expiring, and #8590 has since closed.
28+
*
29+
* ⚠️ The separation **was** broken in the pre-existing direction, and pinning
30+
* it is what found that: `isUniqueViolationError` claimed SQLite's
2531
* unbacked-target error, because that sentence ends `…PRIMARY KEY or UNIQUE
26-
* constraint` and its vocabulary matches the word pair `unique constraint`
27-
* wherever it appears — including inside a sentence saying the constraint is
28-
* ABSENT. Filed as #8590; not fixed by #8567, which would have moved verdicts
29-
* in six consuming packages on a card that measured a different question.
32+
* constraint` and its vocabulary matched the word pair `unique constraint`
33+
* wherever it appeared — including inside a sentence saying the constraint is
34+
* ABSENT. #8567 filed that as #8590 and pinned it rather than fixing it, which
35+
* would have moved verdicts in six consuming packages on a card that measured a
36+
* different question. **#8590 has since closed it**: that predicate's message
37+
* limb now requires a VIOLATION phrasing — `unique constraint failed` (SQLite)
38+
* or `violates unique constraint` (Postgres) — so merely mentioning a unique
39+
* constraint no longer answers yes.
40+
*
41+
* ⚠️ Postgres was believed to escape that collision "by luck of word order",
42+
* its `unique or exclusion constraint` not being adjacent. #8590's dialect
43+
* sweep disproved it: PG **42830**, `there is no unique constraint matching
44+
* given keys for referenced table "t"` — a FOREIGN KEY referencing a non-unique
45+
* column — puts the pair adjacent in Postgres' own ABSENCE sentence. Both
46+
* dialects had the collision; only SQLite's instance sat on the path this file
47+
* measures. That is why the fix is an allowlist of violation phrasings and not
48+
* a negative lookahead on SQLite's sentence, which would still answer `true`
49+
* there.
50+
*
3051
* `unbacked-conflict-target.test.ts` records both predicates' verdicts on every
31-
* measured text, per dialect, so neither the fix nor a fresh drift can land
32-
* silently. Nothing below may take a limb from that vocabulary, or give one to
33-
* it, while #8590 is open.
52+
* measured text, per dialect, and `unique-violation-absence-sentences.test.ts`
53+
* pins the absence sentences on both sides — so neither a fix nor a fresh drift
54+
* can land silently in either direction.
3455
*
3556
* ## What each dialect actually says — measured, never transcribed
3657
*

0 commit comments

Comments
 (0)