Skip to content

Classify constraint violations by SQLite's error codes, not its message text - #37

Merged
shockalotti merged 1 commit into
Calnode:mainfrom
distronode-com:fix/constraint-error-codes
Sep 9, 2026
Merged

Classify constraint violations by SQLite's error codes, not its message text#37
shockalotti merged 1 commit into
Calnode:mainfrom
distronode-com:fix/constraint-error-codes

Conversation

@distronode-com

Copy link
Copy Markdown
Contributor

This is the piece of #29 you said you would take, on its own against main, with the four pg* constants and the pgconn import dropped. No dialect layer, no second migration set, no new dependency: modernc.org/sqlite is already a direct dependency, and nothing else is added.

The bug it fixes

Thirteen call sites decided what a database error meant by matching English text:

strings.Contains(err.Error(), "UNIQUE constraint failed")

SQLite reports a PRIMARY KEY collision as SQLITE_CONSTRAINT_PRIMARYKEY (1555) and an ordinary unique violation as SQLITE_CONSTRAINT_UNIQUE (2067), and it uses the same message for both. So the text cannot distinguish them, and nothing built on it can either.

The trap is that the obvious fix is also wrong. A code match written from the message alone matches 2067 and silently stops recognising primary-key collisions, which matters here: idempotency_keys.idempotency_key is a bare PRIMARY KEY, so claimIdempotencyKey's entire replay path arrives as 1555. Both codes belong to IsUniqueViolation.

What is in it

internal/db/constraint.go adds IsUniqueViolation, IsCheckViolation and IsForeignKeyViolation, and the thirteen call sites use them. Two local helpers with the same job (booking.isUniqueViolation, handler.isForeignKeyViolation) are removed in favour of the shared ones.

The classification is code-first, with the message only as a fallback. A *sqlite.Error whose code does not match returns false rather than falling through to the text comparison. Falling through would classify an error by whether its message happened to contain an English phrase, which is the fragility being removed, and it would reintroduce the primary-key trap in reverse: a 1555 excluded by code would be readmitted by its "UNIQUE constraint failed" text.

The fallback itself is deliberate rather than vestigial, and the reasoning is in the violates doc comment you asked to keep.

Evidence

Every class is provoked against the real migrated schema rather than constructed by hand, because a hand-built error only proves the predicate agrees with what its author believed the driver returns, and the belief being replaced here was exactly that kind of belief.

assertOnly additionally requires that exactly one of the three predicates matches, so the classes stay distinguishable and a CHECK violation cannot quietly become a 409. The negative cases carry the same weight: a missing table, a plain error, nil, and a NOT NULL violation (1299) must match none of the three.

The primary-key case is the one worth pointing at. Deleting sqliteConstraintPrimaryKey from IsUniqueViolation fails it, and the failure prints the driver's own words:

--- FAIL: TestConstraintPredicates/unique_via_primary_key
    primary key: predicate did not recognise constraint failed:
    UNIQUE constraint failed: idempotency_keys.idempotency_key (1555)

That is the message and the code side by side, which is the whole argument for this change in one line.

gofmt, go vet and go test ./... are clean on main + this branch.

🤖 Generated with Claude Code

Thirteen call sites decided what a database error meant by matching English:
strings.Contains(err.Error(), "UNIQUE constraint failed"). That is invisible to
every gate, and it cannot answer the question it is being asked.

SQLite reports a PRIMARY KEY collision as SQLITE_CONSTRAINT_PRIMARYKEY (1555)
and an ordinary unique violation as SQLITE_CONSTRAINT_UNIQUE (2067), and it uses
the SAME message for both. So the text match cannot distinguish them, and a code
match written from the message alone would match 2067 and silently stop
recognising every primary-key collision. idempotency_keys.idempotency_key is a
bare PRIMARY KEY, so the replay path depends on 1555 being read as a unique
violation.

IsUniqueViolation, IsCheckViolation and IsForeignKeyViolation answer from the
driver's code and fall back to the message only when the concrete driver type is
no longer attached. A driver error whose code does not match returns false rather
than falling through: falling through would classify by whether the message
happened to contain an English phrase, and would readmit 1555 by its text after
excluding it by code.

Each class is provoked against the real schema rather than constructed, so the
test asserts what the driver does rather than what its author believed. Removing
1555 from IsUniqueViolation fails the primary-key case, which is what makes that
case worth having.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ No new issues found.

Reviewed changes Shared SQLite constraint classifiers keyed on extended result codes, with provoked tests and all prior message-substring call sites moved over.

  • db.IsUniqueViolation / IsCheckViolation / IsForeignKeyViolation — code-first via *sqlite.Error.Code(), text only when the driver type is gone; non-matching codes do not fall through.
  • PK 1555 counted as unique — required for idempotency_keys bare PRIMARY KEY / claimIdempotencyKey replay.
  • Call-site migration — booking service, availability, override, teams, event types, idempotency, booking answers FK; local helpers removed.
  • Provoked tests — real schema UNIQUE/PK/CHECK/FK plus exclusivity and negative cases; text-fallback branch covered separately.

Pullfrog  | View workflow run | Using Grok𝕏

@shockalotti
shockalotti merged commit 38e7324 into Calnode:main Sep 9, 2026
5 checks passed
@github-actions github-actions Bot locked and limited conversation to collaborators Sep 9, 2026
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants