diff --git a/CHANGELOG.md b/CHANGELOG.md index b4c8bb2..7e602e2 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -24,6 +24,25 @@ exact tag (`ghcr.io/calnode/calnode:0.1.0`) if you need stability between upgrad copied price is how a paid meeting quietly starts selling for nothing. Bookings are not copied. +### Fixed +- **Constraint violations are recognised by SQLite's error code rather than by its + English message.** Thirteen call sites asked `strings.Contains(err.Error(), "UNIQUE + constraint failed")`, and SQLite reports a PRIMARY KEY collision + (`SQLITE_CONSTRAINT_PRIMARYKEY`, 1555) with that exact message while giving it a + different code from an ordinary unique violation (`SQLITE_CONSTRAINT_UNIQUE`, 2067). + The text could not tell the two apart, so nothing that needed to distinguish them + could. + + `db.IsUniqueViolation`, `db.IsCheckViolation` and `db.IsForeignKeyViolation` answer + from the driver's code, falling back to the message only for an error that arrives + without its driver type still attached. A driver error whose code does not match is a + definite no rather than a fall-through, so an error cannot be classified by whether + its text happened to contain an English phrase. + + Each class is provoked against the real schema in a test rather than constructed by + hand, including the primary-key case, which is the one a code match written from the + message alone would get wrong. + ## [0.8.0] - 2026-09-03 ### Added diff --git a/internal/booking/service.go b/internal/booking/service.go index da604f9..665dc1f 100644 --- a/internal/booking/service.go +++ b/internal/booking/service.go @@ -13,6 +13,7 @@ import ( "strings" "time" + "github.com/calnode/calnode/internal/db" "github.com/calnode/calnode/internal/uid" ) @@ -141,7 +142,7 @@ func (s *Service) Create(ctx context.Context, p CreateParams) (*Booking, error) VALUES (?, ?, ?, ?, ?, 'confirmed', ?, ?, ?)`, bookingID, p.EventTypeID, chosenHost, startStr, endStr, p.LocationValue, now, now) if err != nil { - if isUniqueViolation(err) { + if db.IsUniqueViolation(err) { return nil, ErrDoubleBooked } return nil, fmt.Errorf("booking: insert: %w", err) @@ -483,7 +484,7 @@ func (s *Service) Reschedule(ctx context.Context, bookingID string, newStart, ne if _, err := tx.ExecContext(ctx, ` UPDATE bookings SET start_at = ?, end_at = ?, updated_at = ? WHERE id = ?`, startStr, endStr, now, bookingID); err != nil { - if isUniqueViolation(err) { + if db.IsUniqueViolation(err) { return nil, ErrDoubleBooked } return nil, fmt.Errorf("booking: reschedule update: %w", err) @@ -540,7 +541,7 @@ func (s *Service) ReassignHost(ctx context.Context, bookingID, newHostID string) if _, err := tx.ExecContext(ctx, ` UPDATE bookings SET host_id = ?, updated_at = ? WHERE id = ?`, newHostID, now, bookingID); err != nil { - if isUniqueViolation(err) { + if db.IsUniqueViolation(err) { return nil, ErrDoubleBooked } return nil, fmt.Errorf("booking: reassign update: %w", err) @@ -651,8 +652,3 @@ func scanBooking(s scanner) (*Booking, error) { } return &b, nil } - -// isUniqueViolation reports whether err is a SQLite UNIQUE constraint failure. -func isUniqueViolation(err error) bool { - return strings.Contains(err.Error(), "UNIQUE constraint failed") -} diff --git a/internal/db/constraint.go b/internal/db/constraint.go new file mode 100644 index 0000000..33b2909 --- /dev/null +++ b/internal/db/constraint.go @@ -0,0 +1,85 @@ +package db + +import ( + "errors" + "slices" + "strings" + + "modernc.org/sqlite" +) + +// Constraint violations are the one class of database error Calnode routinely acts +// on rather than just reporting: a duplicate slug is a 409, an out-of-range value is +// a 400, a dangling reference is a 404. Deciding which is which was a substring match +// on SQLite's English message, which is invisible to every gate and, as the codes +// below show, cannot actually distinguish the two cases it is asked to. +// +// SQLite's extended result codes, as reported by (*sqlite.Error).Code(). +// +// ⛔ SQLITE_CONSTRAINT_PRIMARYKEY is a SEPARATE code from SQLITE_CONSTRAINT_UNIQUE +// even though both carry the message "UNIQUE constraint failed". A text match cannot +// tell them apart at all, and matching only 2067 would silently stop recognising +// primary-key collisions. Calnode has one that matters: +// idempotency_keys.idempotency_key is a bare PRIMARY KEY, so every idempotent replay +// arrives as 1555. Both belong to IsUniqueViolation. +const ( + sqliteConstraintCheck = 275 // SQLITE_CONSTRAINT_CHECK + sqliteConstraintForeignKey = 787 // SQLITE_CONSTRAINT_FOREIGNKEY + sqliteConstraintPrimaryKey = 1555 // SQLITE_CONSTRAINT_PRIMARYKEY + sqliteConstraintUnique = 2067 // SQLITE_CONSTRAINT_UNIQUE +) + +// SQLite's message fragments, used only as a fallback — see violates. +const ( + sqliteUniqueText = "UNIQUE constraint failed" + sqliteCheckText = "CHECK constraint failed" + sqliteForeignKeyText = "FOREIGN KEY constraint failed" +) + +// IsUniqueViolation reports whether err is a unique-constraint violation — a +// duplicate slug, a replayed idempotency key, a second booking at one host's exact +// start time. A primary-key collision counts. +func IsUniqueViolation(err error) bool { + return violates(err, sqliteUniqueText, sqliteConstraintUnique, sqliteConstraintPrimaryKey) +} + +// IsCheckViolation reports whether err is a CHECK-constraint violation, i.e. a value +// outside the set the column allows. Callers turn this into a 400, since the only way +// to reach it is a request carrying a value the handler did not validate. +func IsCheckViolation(err error) bool { + return violates(err, sqliteCheckText, sqliteConstraintCheck) +} + +// IsForeignKeyViolation reports whether err is a foreign-key violation — a reference +// to a row that does not exist, or a delete that would orphan one. +func IsForeignKeyViolation(err error) bool { + return violates(err, sqliteForeignKeyText, sqliteConstraintForeignKey) +} + +// violates classifies err: the driver's own error code when one is available, the +// message only when it is not. +// +// A driver error is a DEFINITE answer in both directions. A *sqlite.Error whose code +// does not match returns false and does not fall 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. It would also reintroduce +// the primary-key trap in reverse: a 1555 error excluded by code would be readmitted +// by its "UNIQUE constraint failed" message. +// +// The text fallback is deliberate rather than vestigial. It covers an error that +// reaches here without the concrete driver type still attached — a driver release +// that changes its error type, a layer that reformats an error into a plain one +// instead of wrapping it. In that case the message is the only signal left, and +// answering from it beats answering "not a constraint violation" and returning a 500. +func violates(err error, sqliteText string, sqliteCodes ...int) bool { + if err == nil { + return false + } + + var sqliteErr *sqlite.Error + if errors.As(err, &sqliteErr) { + return slices.Contains(sqliteCodes, sqliteErr.Code()) + } + + return strings.Contains(err.Error(), sqliteText) +} diff --git a/internal/db/constraint_test.go b/internal/db/constraint_test.go new file mode 100644 index 0000000..174063d --- /dev/null +++ b/internal/db/constraint_test.go @@ -0,0 +1,186 @@ +package db_test + +import ( + "database/sql" + "errors" + "testing" + + "github.com/calnode/calnode/internal/db" +) + +// openMigrated returns an in-memory database with the schema applied, so the +// constraints these tests provoke are the real ones. +func openMigrated(t *testing.T) *sql.DB { + t.Helper() + database, err := db.Open("sqlite://:memory:") + if err != nil { + t.Fatalf("db.Open: %v", err) + } + t.Cleanup(func() { database.Close() }) + if err := db.Migrate(database); err != nil { + t.Fatalf("db.Migrate: %v", err) + } + return database +} + +// TestConstraintPredicates provokes a real violation of each class against the real +// schema and asserts the predicate recognises it. +// +// Provoked rather than constructed: a hand-built error would only prove the +// predicate agrees with whatever the test author believed the driver returns, and +// the belief this replaces (that the message distinguishes the cases) was wrong. +func TestConstraintPredicates(t *testing.T) { + database := openMigrated(t) + + // A user and an event type to hang the booking constraints off. + const userID = "u-constraint" + if _, err := database.Exec( + `INSERT INTO users (id, email, name, iana_timezone) VALUES (?, ?, 'Host', 'UTC')`, + userID, userID+"@example.com"); err != nil { + t.Fatalf("seed user: %v", err) + } + const etID = "et-constraint" + if _, err := database.Exec( + `INSERT INTO event_types (id, user_id, slug, name, duration_minutes) + VALUES (?, ?, ?, 'Call', 30)`, etID, userID, etID); err != nil { + t.Fatalf("seed event type: %v", err) + } + + t.Run("unique", func(t *testing.T) { + // users.email is UNIQUE. + _, err := database.Exec( + `INSERT INTO users (id, email, name, iana_timezone) VALUES (?, ?, 'Clash', 'UTC')`, + "u-clash", userID+"@example.com") + assertOnly(t, err, "unique", db.IsUniqueViolation) + }) + + // ⛔ The case that distinguishes a correct implementation from a plausible one, + // and the reason the text match could not be kept. + // + // SQLite reports a PRIMARY KEY collision as SQLITE_CONSTRAINT_PRIMARYKEY (1555), + // NOT as SQLITE_CONSTRAINT_UNIQUE (2067) — while still saying "UNIQUE constraint + // failed" in the message. So a predicate matching only 2067 passes the subtest + // above and fails here, and the message cannot tell the two apart at all. + // + // This is not a theoretical shape: idempotency_keys.idempotency_key is a bare + // PRIMARY KEY, so claimIdempotencyKey's entire replay path depends on 1555 being + // classified as a unique violation. + t.Run("unique via primary key", func(t *testing.T) { + const key = "idem-key-constraint" + if _, err := database.Exec( + `INSERT INTO idempotency_keys (idempotency_key, request_hash, created_at) VALUES (?, 'h', ?)`, + key, "2026-06-01T00:00:00Z"); err != nil { + t.Fatalf("seed idempotency key: %v", err) + } + _, err := database.Exec( + `INSERT INTO idempotency_keys (idempotency_key, request_hash, created_at) VALUES (?, 'h', ?)`, + key, "2026-06-01T00:00:00Z") + assertOnly(t, err, "primary key", db.IsUniqueViolation) + }) + + t.Run("check", func(t *testing.T) { + // bookings.status has CHECK (status IN ('confirmed','cancelled')). + _, err := database.Exec(` + INSERT INTO bookings (id, event_type_id, host_id, start_at, end_at, status, created_at, updated_at) + VALUES (?, ?, ?, '2026-06-15T09:00:00Z', '2026-06-15T09:30:00Z', 'not-a-status', ?, ?)`, + "b-check", etID, userID, "2026-06-01T00:00:00Z", "2026-06-01T00:00:00Z") + assertOnly(t, err, "check", db.IsCheckViolation) + }) + + t.Run("foreign key", func(t *testing.T) { + // event_type_id references event_types(id). This needs foreign_keys=ON, + // which db.Open sets. + _, err := database.Exec(` + INSERT INTO bookings (id, event_type_id, host_id, start_at, end_at, status, created_at, updated_at) + VALUES (?, 'no-such-event-type', ?, '2026-06-15T10:00:00Z', '2026-06-15T10:30:00Z', 'confirmed', ?, ?)`, + "b-fk", userID, "2026-06-01T00:00:00Z", "2026-06-01T00:00:00Z") + assertOnly(t, err, "foreign key", db.IsForeignKeyViolation) + }) + + t.Run("unrelated error", func(t *testing.T) { + // A predicate that answered true for everything would satisfy every caller + // above and be badly wrong, so the negative cases carry as much weight. + _, err := database.Exec(`SELECT * FROM a_table_that_does_not_exist`) + if err == nil { + t.Fatal("expected an error from a missing table") + } + assertNone(t, err, "missing table") + assertNone(t, errors.New("some unrelated failure"), "plain error") + assertNone(t, nil, "nil") + }) + + t.Run("unhandled constraint class", func(t *testing.T) { + // A NOT NULL violation is a constraint violation of a class nothing here + // classifies (SQLITE_CONSTRAINT_NOTNULL, 1299). It must match none of the + // three, so a caller cannot turn it into a 409 by accident. + _, err := database.Exec( + `INSERT INTO users (id, email, name, iana_timezone) VALUES (?, ?, NULL, 'UTC')`, + "u-null", "u-null@example.com") + if err == nil { + t.Fatal("expected a NOT NULL violation") + } + assertNone(t, err, "not-null violation") + }) +} + +// TestConstraintTextFallback covers the branch the live driver never reaches: an +// error that arrives without its driver type still attached. +// +// The branch exists for a driver release that changes its error type, or a layer +// that reformats an error into a plain one rather than wrapping it — in which case +// the message is the only signal left, and answering from it beats returning a 500. +// Without this test the fallback would be unexecuted code that reads like an +// accident. +func TestConstraintTextFallback(t *testing.T) { + cases := []struct { + text string + want func(error) bool + name string + }{ + {"boom: UNIQUE constraint failed: t.a", db.IsUniqueViolation, "unique"}, + {"boom: CHECK constraint failed: n > 0", db.IsCheckViolation, "check"}, + {"boom: FOREIGN KEY constraint failed", db.IsForeignKeyViolation, "foreign key"}, + } + for _, c := range cases { + err := errors.New(c.text) + if !c.want(err) { + t.Errorf("%s: fallback did not recognise %q", c.name, c.text) + } + } + // And the fallback is still discriminating, not a catch-all. + assertNone(t, errors.New("NOT NULL constraint failed: t.a"), "not-null text") +} + +// assertOnly checks that want recognises err and the other two predicates do not: +// the classes have to be distinguishable, or a CHECK violation becomes a 409. +func assertOnly(t *testing.T, err error, class string, want func(error) bool) { + t.Helper() + if err == nil { + t.Fatalf("%s: expected a constraint violation, got nil", class) + } + if !want(err) { + t.Errorf("%s: predicate did not recognise %v", class, err) + } + matches := 0 + for _, p := range []func(error) bool{db.IsUniqueViolation, db.IsCheckViolation, db.IsForeignKeyViolation} { + if p(err) { + matches++ + } + } + if matches != 1 { + t.Errorf("%s: %d of 3 predicates matched %v; want exactly 1", class, matches, err) + } +} + +func assertNone(t *testing.T, err error, what string) { + t.Helper() + for name, p := range map[string]func(error) bool{ + "IsUniqueViolation": db.IsUniqueViolation, + "IsCheckViolation": db.IsCheckViolation, + "IsForeignKeyViolation": db.IsForeignKeyViolation, + } { + if p(err) { + t.Errorf("%s returned true for %s (%v)", name, what, err) + } + } +} diff --git a/internal/handler/availability.go b/internal/handler/availability.go index 6577260..3eb766e 100644 --- a/internal/handler/availability.go +++ b/internal/handler/availability.go @@ -4,8 +4,8 @@ import ( "database/sql" "encoding/json" "net/http" - "strings" + "github.com/calnode/calnode/internal/db" "github.com/calnode/calnode/internal/uid" ) @@ -47,7 +47,7 @@ func (h *Handler) CreateAvailabilityRule(w http.ResponseWriter, r *http.Request) VALUES (?, ?, ?, ?, ?, ?)`, id, user.ID, req.EventTypeID, req.DayOfWeek, req.StartTime, req.EndTime) if err != nil { - if strings.Contains(err.Error(), "UNIQUE constraint failed") { + if db.IsUniqueViolation(err) { h.writeError(w, http.StatusConflict, "a rule for this day and time already exists") return } @@ -177,7 +177,7 @@ func (h *Handler) UpdateAvailabilityRule(w http.ResponseWriter, r *http.Request) `UPDATE availability_rules SET day_of_week=?, start_time=?, end_time=? WHERE id=? AND user_id=?`, current.DayOfWeek, current.StartTime, current.EndTime, id, user.ID) if err != nil { - if strings.Contains(err.Error(), "UNIQUE constraint failed") { + if db.IsUniqueViolation(err) { h.writeError(w, http.StatusConflict, "a rule for this day and time already exists") return } diff --git a/internal/handler/booking_handler.go b/internal/handler/booking_handler.go index 7f0c5d9..53f7a8e 100644 --- a/internal/handler/booking_handler.go +++ b/internal/handler/booking_handler.go @@ -15,6 +15,7 @@ import ( "github.com/calnode/calnode/internal/booking" "github.com/calnode/calnode/internal/calendar" + "github.com/calnode/calnode/internal/db" "github.com/calnode/calnode/internal/i18n" "github.com/calnode/calnode/internal/mailer" "github.com/calnode/calnode/internal/slots" @@ -807,7 +808,7 @@ func (h *Handler) CreateBooking(w http.ResponseWriter, r *http.Request) { } // A question was deleted between validateAnswers and the INSERT — return a // clean 422 rather than leaking a generic 500 for an FK constraint failure. - if isForeignKeyViolation(err) { + if db.IsForeignKeyViolation(err) { h.writeError(w, http.StatusUnprocessableEntity, "one or more questions are no longer available") return } @@ -1843,11 +1844,6 @@ func (h *Handler) loadHostPrefs(ctx context.Context, hostID string) (hostPrefs, return p, nil } -// isForeignKeyViolation reports whether err is a SQLite FOREIGN KEY constraint failure. -func isForeignKeyViolation(err error) bool { - return strings.Contains(err.Error(), "FOREIGN KEY constraint failed") -} - // enqueueReminder inserts a reminder.send job scheduled hoursBefore hours before startAt. // If the computed run_at has already passed, the job fires on the next poll cycle. func (h *Handler) enqueueReminder(ctx context.Context, bookingID string, startAt time.Time, hoursBefore int) error { diff --git a/internal/handler/event_type.go b/internal/handler/event_type.go index 40a9e98..d2da9c8 100644 --- a/internal/handler/event_type.go +++ b/internal/handler/event_type.go @@ -7,6 +7,7 @@ import ( "net/http" "strings" + "github.com/calnode/calnode/internal/db" "github.com/calnode/calnode/internal/uid" ) @@ -315,11 +316,11 @@ func (h *Handler) CreateEventType(w http.ResponseWriter, r *http.Request) { routingMode, bufBefore, bufAfter, minNotice, maxFuture, maxActive, showTaken, defaultMsgConfirmation, defaultMsgCancellation, defaultMsgReschedule, defaultMsgReminder) if err != nil { - if strings.Contains(err.Error(), "UNIQUE constraint failed") { + if db.IsUniqueViolation(err) { h.writeError(w, http.StatusConflict, "slug already in use") return } - if strings.Contains(err.Error(), "CHECK constraint failed") { + if db.IsCheckViolation(err) { h.writeError(w, http.StatusBadRequest, "invalid location_type or routing_mode value") return } @@ -701,7 +702,7 @@ func (h *Handler) PatchEventType(w http.ResponseWriter, r *http.Request) { "UPDATE event_types SET "+strings.Join(setClauses, ", ")+" WHERE slug = ? AND user_id = ?", // #nosec G202 -- setClauses is built by set()/the literal col list above; every column name is a hardcoded string, every value is bound via args... args...) if err != nil { - if strings.Contains(err.Error(), "CHECK constraint failed") { + if db.IsCheckViolation(err) { h.writeError(w, http.StatusBadRequest, "invalid location_type or routing_mode value") return } @@ -780,7 +781,7 @@ func (h *Handler) DeleteEventType(w http.ResponseWriter, r *http.Request) { res, err := h.db.ExecContext(r.Context(), `DELETE FROM event_types WHERE slug = ? AND user_id = ?`, slug, user.ID) if err != nil { - if strings.Contains(err.Error(), "FOREIGN KEY constraint failed") { + if db.IsForeignKeyViolation(err) { h.writeError(w, http.StatusConflict, "this event type has bookings in its history (including cancelled ones) and can't be deleted — deactivate it instead") return } diff --git a/internal/handler/idempotency.go b/internal/handler/idempotency.go index 91717f9..8991666 100644 --- a/internal/handler/idempotency.go +++ b/internal/handler/idempotency.go @@ -5,8 +5,9 @@ import ( "crypto/sha256" "database/sql" "encoding/hex" - "strings" "time" + + "github.com/calnode/calnode/internal/db" ) // idempotencyRecord is a previously-seen Idempotency-Key's stored outcome. @@ -38,7 +39,7 @@ func (h *Handler) claimIdempotencyKey(ctx context.Context, key, reqHash string) if err == nil { return nil, false, nil } - if !strings.Contains(err.Error(), "UNIQUE constraint failed") { + if !db.IsUniqueViolation(err) { return nil, false, err } diff --git a/internal/handler/override.go b/internal/handler/override.go index aaa2274..d34db3a 100644 --- a/internal/handler/override.go +++ b/internal/handler/override.go @@ -4,9 +4,9 @@ import ( "database/sql" "encoding/json" "net/http" - "strings" "time" + "github.com/calnode/calnode/internal/db" "github.com/calnode/calnode/internal/uid" ) @@ -155,7 +155,7 @@ func (h *Handler) CreateAvailabilityOverride(w http.ResponseWriter, r *http.Requ INSERT INTO availability_overrides (id, user_id, date, is_available, reason, start_time, end_time) VALUES (?, ?, ?, ?, ?, ?, ?)`, id, user.ID, req.Date, isAvailInt, req.Reason, req.StartTime, req.EndTime); err != nil { - if strings.Contains(err.Error(), "UNIQUE constraint failed") { + if db.IsUniqueViolation(err) { h.writeError(w, http.StatusConflict, "an override already exists for this date; delete it first") return } diff --git a/internal/handler/teams.go b/internal/handler/teams.go index ff32173..c096625 100644 --- a/internal/handler/teams.go +++ b/internal/handler/teams.go @@ -8,6 +8,7 @@ import ( "strings" "time" + "github.com/calnode/calnode/internal/db" "github.com/calnode/calnode/internal/uid" ) @@ -86,7 +87,7 @@ func (h *Handler) CreateTeam(w http.ResponseWriter, r *http.Request) { if _, err := h.db.ExecContext(r.Context(), `INSERT INTO teams (id, name, slug, created_at) VALUES (?, ?, ?, ?)`, id, req.Name, slug, now); err != nil { - if strings.Contains(err.Error(), "UNIQUE constraint failed") { + if db.IsUniqueViolation(err) { h.writeError(w, http.StatusConflict, "a team with that slug already exists") return } @@ -222,7 +223,7 @@ func (h *Handler) PatchTeam(w http.ResponseWriter, r *http.Request) { res, err := h.db.ExecContext(r.Context(), "UPDATE teams SET "+strings.Join(sets, ", ")+" WHERE id = ?", args...) // #nosec G202 -- sets is built above from hardcoded "col = ?" literals only; every value is bound via args... if err != nil { - if strings.Contains(err.Error(), "UNIQUE constraint failed") { + if db.IsUniqueViolation(err) { h.writeError(w, http.StatusConflict, "a team with that slug already exists") return } @@ -306,7 +307,7 @@ func (h *Handler) AddTeamMember(w http.ResponseWriter, r *http.Request) { INSERT INTO team_members (id, team_id, user_id, role, routing_priority) VALUES (?, ?, ?, 'member', ?)`, uid.New(), teamID, req.UserID, req.RoutingPriority); err != nil { - if strings.Contains(err.Error(), "UNIQUE constraint failed") { + if db.IsUniqueViolation(err) { h.writeError(w, http.StatusConflict, "that user is already in this team") return }