amadan

rastrillo / idear Public

idear: error classes, an atomic Claim, and a corrected coverage table (fix round 1)

The reviewer's mutation battery found two FALSE entries in my report's
"caught" list. Both reproduce: they survive 10/10.

  Transfer's target-is-active check, dropped alone, is green — the
  conditional promote's `AND deactivated_at IS NULL` still holds it, and
  vice versa. Only removing both goes red.

  Accept's CAS replaced by a lookup + update INSIDE the same transaction
  is green, because db.Open caps the writer pool at one connection, so a
  write transaction is fully serialised and the two forms are equivalent
  ON THIS POOL. Only moving the lookup outside the transaction goes red,
  and that is the mutation I actually ran while labelling it "no CAS".

Both moved to the survived list. The CAS stays: the equivalence is an
accident of another package's pool configuration, not a property of this
code, and a second process on the same file — a rolling deploy, a cron
worker — breaks it. No single-process harness can express that, which is
exactly why the code must not depend on it. Claim had the identical
hidden dependency and is now one statement:

  INSERT INTO idear_members (...) SELECT ?,... WHERE NOT EXISTS (SELECT 1 FROM idear_members)

with rows-affected checked, so the emptiness test and the insert cannot
be separated by another transaction OR another process. Across two
processes the old read-then-insert degraded to SQLITE_BUSY_SNAPSHOT — no
double Owner, but an opaque driver error where the caller is owed
ErrOwnerExists.

TestConcurrentClaimYieldsOneOwner was only an ~80% detector (21/25) of
the single most important regression in the file. Now 20 independent
rounds of six racers on fresh instances: 10/10 red, deterministic.

Every sentinel now belongs to a class a handler can switch on, because
one that belongs to none falls through to the default arm and renders as
a 500. ErrLastOwner — this package's most security-relevant refusal —
was a bare errors.New and would have rendered as a server error; it now
unwraps to ErrForbidden while staying distinguishable from it. The new
ErrInvalid class (400) covers ErrInvalidRole plus two new siblings,
ErrInvalidEmail and ErrInvalidSubject, replacing the three bare
errors.New argument errors. errors.go now states the taxonomy and the
rule that adding a sentinel means choosing its class — the defect was
not any one sentinel, it was that nothing said the choice had to be made.

ExpiresAt is written with an explicit .UTC(). Expiry is a TEXT
comparison over time.Time.String(), which renders the zone: a value
built in Europe/Dublin stores as "+0100 IST" and sorts against a UTC row
by its offset characters. Worse than the monotonic trap already pinned,
because it is silent and seasonal.

TestSchema_RoundTripsAModel added by name. Reverting the migration to
TEXT was caught only as collateral damage in thirty unrelated tests;
schema_test.go itself still only proved the DDL parses, which is the
exact shape that let the bug through in the first place.

Nit: the transfer/deactivate race no longer folds "both refused" into
"deactivate won", which could have let the both-outcomes-occurred check
pass on a run where neither operation ever succeeded.

Re-derived battery: 18 mutations, race-sensitive ones 10x, others 3x.
Every result matched its expectation; verbatim output is in the report.
gofmt clean, go vet clean, go test -count=1 green x5, -race green x3.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Paul Campbell pushed by paul@keymail.dev 5707e39fa49119abce4716da272baa453bac2c76 parent 05d3c1c
5 files changed, +391 −83
  • errors.go +74 −13
  • race_test.go +70 −46
  • roster.go +48 −15
  • roster_test.go +121 −9
  • schema_test.go +78 −0
diff --git a/errors.go b/errors.go
index c4feca6..c016a07 100644
--- a/errors.go
+++ b/errors.go
@@ -2,10 +2,27 @@ package idear
import "errors"
-// The store's sentinels. Every Roster method that refuses returns one
-// of these (or an ErrForbidden from MayActOn — see policy.go), wrapped
-// with context where a reason helps a log line. Callers test with
-// errors.Is, never by comparing strings.
+// The store's sentinels, and the two CLASSES a handler switches on.
+//
+// Every Roster method that refuses returns one of these, wrapped with
+// context where a reason helps a log line. Callers test with errors.Is,
+// never by comparing strings.
+//
+// The classes exist because a handler needs a status code, not a
+// diagnosis, and because a sentinel that belongs to no class falls
+// through to the default arm of that switch — which is a 500. That is
+// how a refusal ends up rendered as "the server is broken". Every
+// sentinel here is therefore reachable by errors.Is from exactly one
+// of:
+//
+// ErrInvalid the caller sent something malformed → 400
+// ErrForbidden the caller may not do this → 403
+// ErrNotFound no such member → 404
+// ErrNoInvitation, ErrOwnerExists flow-specific, rendered by the
+// flow that produces them
+//
+// Adding a sentinel to this file means deciding which class it is in.
+// A bare errors.New here is a 500 waiting to happen.
var (
// ErrOwnerExists is Claim's refusal: this instance already has a
// roster, so the claim is closed. It is what the losers of a
@@ -27,6 +44,13 @@ var (
// message that distinguishes it from a permission refusal.
ErrNotFound = errors.New("idear: no such member")
+ // ErrInvalid is the CLASS of every malformed-argument refusal: a
+ // role that is not a role, a blank address, a blank subject. A
+ // handler renders the whole class 400. Match the class when you
+ // want a status code and the specific sentinel when you want to
+ // name the field that was wrong.
+ ErrInvalid = errors.New("idear: invalid argument")
+
// ErrLastOwner refuses any change that would leave the instance
// without exactly one active Owner: deactivating the Owner, or
// changing the Owner's role by any route other than Transfer.
@@ -34,16 +58,53 @@ var (
// It is checked BEFORE the full MayActOn matrix, and deliberately:
// "the owner cannot be removed" is true for every actor at every
// rank, so returning ErrForbidden instead would tell an Admin that
- // some higher-ranked actor could do this — which is false. It is
- // still a refusal: handlers render it at 403 alongside
- // ErrForbidden, not as a bad request.
- ErrLastOwner = errors.New("idear: the owner cannot be removed")
+ // some higher-ranked actor could do this — which is false.
+ //
+ // It nevertheless UNWRAPS to ErrForbidden, because it is a refusal
+ // and a handler must render it 403. Left as a bare errors.New it
+ // belonged to no class, and the obvious handler taxonomy
+ // (ErrInvalid 400, ErrForbidden 403, ErrNotFound 404, default 500)
+ // would have rendered this package's most security-relevant
+ // refusal as a server error. Code that wants the SPECIFIC reason
+ // still gets it: errors.Is(err, ErrLastOwner) stays true, and
+ // stays FALSE for an ordinary ErrForbidden.
+ ErrLastOwner error = classErr{"idear: the owner cannot be removed", ErrForbidden}
// ErrInvalidRole is a role argument that is not one of the three
// known roles. It is a malformed argument, not a refusal of
- // authority — a handler renders it 400 where it renders
- // ErrForbidden 403. Refusing RoleOwner is NOT this error: owner is
- // a perfectly valid role, and Invite and SetRole refuse it with
- // ErrForbidden because ownership moves only by Transfer.
- ErrInvalidRole = errors.New("idear: not a role")
+ // authority — 400, not 403. Refusing RoleOwner is NOT this error:
+ // owner is a perfectly valid role, and Invite and SetRole refuse
+ // it with ErrForbidden because ownership moves only by Transfer.
+ ErrInvalidRole error = classErr{"idear: not a role", ErrInvalid}
+
+ // ErrInvalidEmail is a blank or whitespace-only address where one
+ // is required. It is the sibling of ErrInvalidRole on the same
+ // form: the role field and the address field of one invitation
+ // must classify the same way, or a mistyped address renders 500
+ // while a mistyped role renders 400.
+ ErrInvalidEmail error = classErr{"idear: not an email address", ErrInvalid}
+
+ // ErrInvalidSubject is a blank session Subject where one is
+ // required. Subject is the join to the app's own identity, so an
+ // empty one is a caller bug — usually idear's middleware mounted
+ // OUTSIDE the app's session guard, which is the misconfiguration
+ // the design spec §5 warns about.
+ ErrInvalidSubject error = classErr{"idear: not a subject", ErrInvalid}
)
+
+// classErr is a sentinel that also belongs to a class: errors.Is finds
+// it by identity (the struct is comparable, and no two sentinels here
+// share a message), and unwraps past it to the class.
+//
+// One mechanism rather than four hand-written types, because four
+// sentinels need exactly this shape and a fifth will too — and because
+// the mistake it exists to prevent is systematic, not local. Declared
+// as `error` rather than as the concrete type so no caller can grow a
+// dependency on classErr itself.
+type classErr struct {
+ msg string
+ class error
+}
+
+func (e classErr) Error() string { return e.msg }
+func (e classErr) Unwrap() error { return e.class }
diff --git a/race_test.go b/race_test.go
index 2e55a96..4b7ca4e 100644
--- a/race_test.go
+++ b/race_test.go
@@ -86,49 +86,62 @@ func pair(swap bool, first, second func()) {
// two people who can each demote the other, on a roster that is
// supposed to have exactly one root of authority.
func TestConcurrentClaimYieldsOneOwner(t *testing.T) {
- const n = 6
- h := ideartest.New(t)
- ctx := h.Ctx()
+ // Six racers, on a FRESH instance, repeated. A single round of six
+ // was measured at only ~84% detection (21 failures in 25 runs)
+ // against the mutation that moves the count out of the
+ // transaction — so a one-shot `go test` missed the single most
+ // important regression in this file about one run in five. Rounds
+ // are independent trials: twenty of them put a miss past 1 in
+ // 10^17, which is the difference between a test and a coin.
+ const (
+ n = 6
+ rounds = 20
+ )
- gate, wait := release()
- errs := make([]error, n)
- var wg sync.WaitGroup
- for i := range n {
- wg.Add(1)
- go func() {
- defer wg.Done()
- wait()
- _, errs[i] = h.Roster.Claim(ctx,
- fmt.Sprintf("claimant-%d", i),
- fmt.Sprintf("claimant-%d@example.test", i),
- fmt.Sprintf("Claimant %d", i))
- }()
- }
- close(gate)
- wg.Wait()
+ for round := range rounds {
+ h := ideartest.New(t)
+ ctx := h.Ctx()
- won, refused := 0, 0
- for i, err := range errs {
- switch {
- case err == nil:
- won++
- case errors.Is(err, idear.ErrOwnerExists):
- refused++
- default:
- t.Errorf("claimant %d failed with an unexpected error: %v", i, err)
+ gate, wait := release()
+ errs := make([]error, n)
+ var wg sync.WaitGroup
+ for i := range n {
+ wg.Add(1)
+ go func() {
+ defer wg.Done()
+ wait()
+ _, errs[i] = h.Roster.Claim(ctx,
+ fmt.Sprintf("claimant-%d", i),
+ fmt.Sprintf("claimant-%d@example.test", i),
+ fmt.Sprintf("Claimant %d", i))
+ }()
+ }
+ close(gate)
+ wg.Wait()
+
+ won, refused := 0, 0
+ for i, err := range errs {
+ switch {
+ case err == nil:
+ won++
+ case errors.Is(err, idear.ErrOwnerExists):
+ refused++
+ default:
+ t.Fatalf("round %d: claimant %d failed with an unexpected error: %v", round, i, err)
+ }
+ }
+ if won != 1 {
+ t.Fatalf("round %d: %d of %d concurrent claims succeeded, want exactly 1", round, won, n)
+ }
+ if refused != n-1 {
+ t.Fatalf("round %d: %d claims were refused with ErrOwnerExists, want %d", round, refused, n-1)
+ }
+ if got := h.CountMembers(); got != 1 {
+ t.Fatalf("round %d: the roster holds %d rows, want exactly 1", round, got)
+ }
+ if owner := h.TheOwner(); !owner.Active() {
+ t.Fatalf("round %d: the owner is not active", round)
}
- }
- if won != 1 {
- t.Errorf("%d of %d concurrent claims succeeded, want exactly 1", won, n)
- }
- if refused != n-1 {
- t.Errorf("%d claims were refused with ErrOwnerExists, want %d", refused, n-1)
- }
- if got := h.CountMembers(); got != 1 {
- t.Errorf("the roster holds %d rows, want exactly 1", got)
- }
- if owner := h.TheOwner(); !owner.Active() {
- t.Error("the owner is not active")
}
}
@@ -312,7 +325,7 @@ func TestRevokeRacingAcceptNeverAdmits(t *testing.T) {
// runs, and must be refused.
func TestTransferRacingDeactivateNeverStrandsOwner(t *testing.T) {
const iterations = 40
- transferWins, deactivateWins := 0, 0
+ transferWins, deactivateWins, bothFailed := 0, 0, 0
for i := range iterations {
h := ideartest.New(t)
@@ -325,10 +338,16 @@ func TestTransferRacingDeactivateNeverStrandsOwner(t *testing.T) {
func() { transferErr = h.Roster.Transfer(ctx, owner, target) },
func() { removeErr = h.Roster.Deactivate(ctx, owner, target) },
)
- if transferErr == nil {
+ switch {
+ case transferErr == nil:
transferWins++
- } else {
+ case removeErr == nil:
deactivateWins++
+ default:
+ // Both refused. Folding this into "deactivate won" would
+ // let the both-outcomes-occurred check below pass on a
+ // run where neither operation ever succeeded.
+ bothFailed++
}
if transferErr != nil && !errors.Is(transferErr, idear.ErrForbidden) {
@@ -365,10 +384,15 @@ func TestTransferRacingDeactivateNeverStrandsOwner(t *testing.T) {
}
}
- t.Logf("%d iterations: transfer won %d, deactivate won %d", iterations, transferWins, deactivateWins)
+ t.Logf("%d iterations: transfer won %d, deactivate won %d, both refused %d",
+ iterations, transferWins, deactivateWins, bothFailed)
if transferWins == 0 || deactivateWins == 0 {
- t.Errorf("only one order ever occurred (transfer %d, deactivate %d); "+
+ t.Errorf("only one order ever occurred (transfer %d, deactivate %d, both refused %d); "+
"the two calls are not actually racing and this test proves nothing",
- transferWins, deactivateWins)
+ transferWins, deactivateWins, bothFailed)
+ }
+ if bothFailed != 0 {
+ t.Errorf("%d iterations refused BOTH operations; one of them must always be able to win",
+ bothFailed)
}
}
diff --git a/roster.go b/roster.go
index 97cb710..fe75d5a 100644
--- a/roster.go
+++ b/roster.go
@@ -223,16 +223,17 @@ func (rs *Roster) IsEmpty(ctx context.Context) (bool, error) {
// Claim makes the first arrival the Owner of an unclaimed instance.
//
-// It succeeds only when idear_members holds zero rows, counted inside
-// the same transaction as the insert. Two concurrent first signups
-// therefore produce exactly one Owner; the loser gets ErrOwnerExists
-// and is an orphan — a user row in the app with no membership — which
-// the signed-in reconciliation route exists to heal (design spec §5).
-// That is a designed path, not an accident.
+// It succeeds only when idear_members holds ZERO ROWS — not zero
+// ACTIVE rows — and the emptiness test and the insert are ONE
+// statement, so nothing can come between them. Two concurrent first
+// signups therefore produce exactly one Owner; the loser gets
+// ErrOwnerExists and is an orphan — a user row in the app with no
+// membership — which the signed-in reconciliation route exists to heal
+// (design spec §5). That is a designed path, not an accident.
func (rs *Roster) Claim(ctx context.Context, subject, email, name string) (*Member, error) {
subject = strings.TrimSpace(subject)
if subject == "" {
- return nil, errors.New("idear: Claim needs a non-empty subject")
+ return nil, fmt.Errorf("%w: Claim needs a non-empty subject", ErrInvalidSubject)
}
m := &Member{
Subject: subject,
@@ -240,15 +241,37 @@ func (rs *Roster) Claim(ctx context.Context, subject, email, name string) (*Memb
Name: strings.TrimSpace(name),
Role: RoleOwner,
}
+ now := rs.now()
err := rs.tx(ctx, func(tx *gorm.DB) error {
- var n int64
- if err := tx.Model(&Member{}).Count(&n).Error; err != nil {
- return err
+ // One statement, so the emptiness test and the insert cannot
+ // be separated by anything at all — not by another
+ // transaction, and not by another PROCESS. A count followed
+ // by an insert is also correct here, but only because
+ // rastrillo/db caps the writer pool at one connection; a
+ // second process on the same file (a rolling deploy, a cron
+ // worker) breaks that and the pair degrades to
+ // SQLITE_BUSY_SNAPSHOT — no double Owner, but an opaque
+ // driver error where the caller is owed ErrOwnerExists.
+ // Correctness that depends on an unrelated setting in another
+ // package is what breaks two releases later.
+ res := tx.Exec(`INSERT INTO idear_members
+ (subject, email, name, role, deactivated_at, created_at, updated_at)
+ SELECT ?, ?, ?, ?, NULL, ?, ?
+ WHERE NOT EXISTS (SELECT 1 FROM idear_members)`,
+ m.Subject, m.Email, m.Name, m.Role, now, now)
+ if res.Error != nil {
+ return res.Error
}
- if n != 0 {
+ // Zero rows means the NOT EXISTS failed: the roster has rows.
+ // ALL rows, not just active ones — a fully deactivated roster
+ // is not empty and must not reopen the claim.
+ if res.RowsAffected != 1 {
return ErrOwnerExists
}
- return tx.Create(m).Error
+ // Read the row back for its id and timestamps. Safe inside
+ // this transaction for the same reason Accept's read-back is:
+ // the insert above already claimed it.
+ return tx.Where("subject = ?", m.Subject).Take(m).Error
})
if err != nil {
return nil, err
@@ -271,7 +294,7 @@ func (rs *Roster) Invite(ctx context.Context, actor *Member, email string, role
}
email = normalizeEmail(email)
if email == "" {
- return nil, "", errors.New("idear: Invite needs a non-empty email")
+ return nil, "", fmt.Errorf("%w: Invite needs a non-empty email", ErrInvalidEmail)
}
token, err := newToken()
if err != nil {
@@ -283,7 +306,17 @@ func (rs *Roster) Invite(ctx context.Context, actor *Member, email string, role
Email: email,
Role: role,
TokenHash: hashToken(token),
- ExpiresAt: now.Add(rs.cfg.InviteTTL),
+ // The .UTC() is redundant today — rs.now() already returns
+ // UTC and Add preserves the location — and it stays anyway.
+ // Expiry is decided by a TEXT comparison over
+ // time.Time.String(), and String() renders the zone: a value
+ // in Europe/Dublin becomes "... +0100 IST", which sorts
+ // against "... +0000 UTC" by the offset characters and gives
+ // an answer unrelated to which instant is later. That is a
+ // worse failure than the monotonic-reading trap in rs.now(),
+ // because it is silent and seasonal. One call pins it at the
+ // only site whose value is ever compared in SQL.
+ ExpiresAt: now.Add(rs.cfg.InviteTTL).UTC(),
}
err = rs.tx(ctx, func(tx *gorm.DB) error {
cur, err := loadMember(tx, actor.ID)
@@ -368,7 +401,7 @@ func (rs *Roster) Revoke(ctx context.Context, actor *Member, id int64) error {
func (rs *Roster) Accept(ctx context.Context, token, subject, name string) (*Member, error) {
subject = strings.TrimSpace(subject)
if subject == "" {
- return nil, errors.New("idear: Accept needs a non-empty subject")
+ return nil, fmt.Errorf("%w: Accept needs a non-empty subject", ErrInvalidSubject)
}
if token == "" {
return nil, ErrNoInvitation
diff --git a/roster_test.go b/roster_test.go
index 9b7ca24..80a6ed1 100644
--- a/roster_test.go
+++ b/roster_test.go
@@ -2,6 +2,7 @@ package idear_test
import (
"errors"
+ "fmt"
"strings"
"testing"
"time"
@@ -28,6 +29,74 @@ func TestNew_Defaults(t *testing.T) {
}
}
+// TestErrorClasses pins what a handler switches on.
+//
+// A sentinel that belongs to no class falls through to the default arm
+// of the obvious taxonomy — ErrInvalid 400, ErrForbidden 403,
+// ErrNotFound 404, default 500 — and is rendered as a server error. Two
+// of this package's sentinels were exactly that before this test
+// existed, including ErrLastOwner, which is its most
+// security-relevant refusal.
+func TestErrorClasses(t *testing.T) {
+ cases := []struct {
+ name string
+ err error
+ is []error // classes and identities errors.Is MUST find
+ isNot []error // and ones it must NOT
+ }{
+ {
+ name: "ErrLastOwner is a refusal, rendered 403",
+ err: idear.ErrLastOwner,
+ is: []error{idear.ErrLastOwner, idear.ErrForbidden},
+ isNot: []error{idear.ErrInvalid, idear.ErrNotFound},
+ },
+ {
+ // The specific reason must stay distinguishable, or the
+ // ordering TestSetRole_AuthorityFloorComesFirst asserts
+ // becomes untestable.
+ name: "an ordinary ErrForbidden is not ErrLastOwner",
+ err: idear.ErrForbidden,
+ is: []error{idear.ErrForbidden},
+ isNot: []error{idear.ErrLastOwner, idear.ErrInvalid},
+ },
+ {
+ name: "ErrInvalidRole is malformed input, rendered 400",
+ err: idear.ErrInvalidRole,
+ is: []error{idear.ErrInvalidRole, idear.ErrInvalid},
+ isNot: []error{idear.ErrForbidden, idear.ErrInvalidEmail, idear.ErrInvalidSubject},
+ },
+ {
+ name: "ErrInvalidEmail is malformed input, rendered 400",
+ err: idear.ErrInvalidEmail,
+ is: []error{idear.ErrInvalidEmail, idear.ErrInvalid},
+ isNot: []error{idear.ErrForbidden, idear.ErrInvalidRole, idear.ErrInvalidSubject},
+ },
+ {
+ name: "ErrInvalidSubject is malformed input, rendered 400",
+ err: idear.ErrInvalidSubject,
+ is: []error{idear.ErrInvalidSubject, idear.ErrInvalid},
+ isNot: []error{idear.ErrForbidden, idear.ErrInvalidRole, idear.ErrInvalidEmail},
+ },
+ }
+ for _, tc := range cases {
+ t.Run(tc.name, func(t *testing.T) {
+ // Wrapped the way the store wraps them, not bare: that is
+ // the form a handler actually receives.
+ wrapped := fmt.Errorf("%w: some context", tc.err)
+ for _, target := range tc.is {
+ if !errors.Is(wrapped, target) {
+ t.Errorf("errors.Is(%v, %v) = false, want true", wrapped, target)
+ }
+ }
+ for _, target := range tc.isNot {
+ if errors.Is(wrapped, target) {
+ t.Errorf("errors.Is(%v, %v) = true, want false", wrapped, target)
+ }
+ }
+ })
+ }
+}
+
// -------------------------------------------------------- IsEmpty/Claim
func TestClaim_FirstArrivalBecomesOwner(t *testing.T) {
@@ -107,8 +176,9 @@ func TestClaim_DeactivatedRosterDoesNotReopenTheClaim(t *testing.T) {
func TestClaim_RequiresSubject(t *testing.T) {
h := ideartest.New(t)
- if _, err := h.Roster.Claim(h.Ctx(), " ", "nobody@example.test", ""); err == nil {
- t.Fatal("Claim with a blank subject succeeded; the subject is the join to the app's own identity")
+ _, err := h.Roster.Claim(h.Ctx(), " ", "nobody@example.test", "")
+ if !errors.Is(err, idear.ErrInvalidSubject) || !errors.Is(err, idear.ErrInvalid) {
+ t.Fatalf("Claim with a blank subject = %v, want ErrInvalidSubject in the ErrInvalid class", err)
}
if n := h.CountMembers(); n != 0 {
t.Errorf("roster holds %d members after a refused claim, want 0", n)
@@ -177,6 +247,26 @@ func TestInvite_RankRules(t *testing.T) {
}
}
+// TestInvite_RejectsBlankEmail is the sibling of
+// TestInvite_RejectsUnknownRole: the role field and the address field
+// of one invitation form must classify the same way, or a mistyped
+// address renders 500 while a mistyped role renders 400.
+func TestInvite_RejectsBlankEmail(t *testing.T) {
+ h := ideartest.New(t)
+ owner := h.Owner()
+ for _, email := range []string{"", " ", "\t\n"} {
+ _, _, err := h.Roster.Invite(h.Ctx(), owner, email, idear.RoleMember)
+ if !errors.Is(err, idear.ErrInvalidEmail) || !errors.Is(err, idear.ErrInvalid) {
+ t.Errorf("Invite(%q) = %v, want ErrInvalidEmail in the ErrInvalid class", email, err)
+ }
+ }
+ if invs, err := h.Roster.PendingInvitations(h.Ctx()); err != nil {
+ t.Fatalf("PendingInvitations: %v", err)
+ } else if len(invs) != 0 {
+ t.Errorf("%d invitations were minted from a blank address", len(invs))
+ }
+}
+
func TestInvite_RejectsUnknownRole(t *testing.T) {
h := ideartest.New(t)
owner := h.Owner()
@@ -365,8 +455,9 @@ func TestAccept_Refusals(t *testing.T) {
if err != nil {
t.Fatalf("Invite: %v", err)
}
- if _, err := h.Roster.Accept(ctx, token, " ", "Blank"); err == nil {
- t.Fatal("Accept with a blank subject succeeded")
+ _, err = h.Roster.Accept(ctx, token, " ", "Blank")
+ if !errors.Is(err, idear.ErrInvalidSubject) || !errors.Is(err, idear.ErrInvalid) {
+ t.Fatalf("Accept with a blank subject = %v, want ErrInvalidSubject in the ErrInvalid class", err)
}
if h.Invitation(inv.ID).AcceptedAt != nil {
t.Error("the invitation was consumed by the refused Accept")
@@ -774,10 +865,19 @@ func TestPendingInvitations_HidesSpentAndExpired(t *testing.T) {
// the instants sort — including across the boundary where the
// fractional part is trimmed away entirely.
//
-// It would stop answering correctly the moment a stored value carried
-// a monotonic clock reading, because String() appends " m=+0.0001" to
-// those. That is why the store's clock is time.Now().UTC() and never
-// time.Now(): .UTC() strips the monotonic reading, and Add does not.
+// Two things break it, and both are pinned below.
+//
+// A monotonic clock reading: String() appends " m=+0.0001" to any time
+// that still carries one, so the stored text no longer has the shape
+// the comparison assumes. .UTC() strips it; time.Now() alone does not,
+// and neither does Add on top of it.
+//
+// A non-UTC location, which is the worse of the two because it is
+// silent and seasonal: String() renders the zone, so a value built in
+// Europe/Dublin stores as "... +0100 IST" and sorts against a UTC row
+// by the offset characters — an answer with no relationship to which
+// instant is later. An invitation minted in summer could outlive one
+// minted in winter regardless of its actual expiry.
func TestExpiryComparisonMatchesGo(t *testing.T) {
h := ideartest.New(t)
ctx := h.Ctx()
@@ -791,8 +891,20 @@ func TestExpiryComparisonMatchesGo(t *testing.T) {
t.Fatalf("ExpiresAt carries a monotonic reading (%s); stored, it would break every expiry comparison",
inv.ExpiresAt.String())
}
+ if inv.ExpiresAt.Location() != time.UTC {
+ t.Fatalf("ExpiresAt is in %v, not UTC (%s); String() renders the zone, so a non-UTC row "+
+ "sorts against a UTC one by its offset characters and the expiry comparison is meaningless",
+ inv.ExpiresAt.Location(), inv.ExpiresAt.String())
+ }
- base := h.Invitation(inv.ID).ExpiresAt
+ stored := h.Invitation(inv.ID)
+ if stored.ExpiresAt.Location() != time.UTC {
+ t.Fatalf("ExpiresAt came back in %v, not UTC (%s)", stored.ExpiresAt.Location(), stored.ExpiresAt.String())
+ }
+ if !stored.ExpiresAt.Equal(inv.ExpiresAt) {
+ t.Errorf("ExpiresAt round-tripped as %v, want %v", stored.ExpiresAt, inv.ExpiresAt)
+ }
+ base := stored.ExpiresAt
for _, delta := range []time.Duration{
-time.Hour, -time.Second, -time.Millisecond, -time.Nanosecond,
0, time.Nanosecond, time.Millisecond, time.Second, time.Hour,
diff --git a/schema_test.go b/schema_test.go
index 8317b0d..a8705a7 100644
--- a/schema_test.go
+++ b/schema_test.go
@@ -5,6 +5,7 @@ import (
"database/sql"
"path/filepath"
"testing"
+ "time"
"github.com/carlosframework/rastrillo/db"
"github.com/carlosframework/rastrillo/migrate"
@@ -134,3 +135,80 @@ func TestSchema_FrozenChecksum(t *testing.T) {
}
}
}
+
+// TestSchema_RoundTripsAModel writes a Member through GORM and reads
+// it back.
+//
+// It exists because TestSchema_Apply does not, and the difference is
+// the whole of the defect this migration was corrected for. Apply
+// proves the CREATE TABLE parses. It cannot prove the DECLARED COLUMN
+// TYPES are usable, and they were not: with the timestamp columns
+// declared TEXT, modernc.org/sqlite handed every stored timestamp back
+// as a raw string, because it converts one to a time.Time only when
+// the column's declared type is DATE, DATETIME or TIMESTAMP. Every
+// read of a Member or an Invitation failed with
+//
+// sql: Scan error on column index 5, name "created_at":
+// unsupported Scan, storing driver.Value type string into type *time.Time
+//
+// and the tables could be written and never read. A schema test that
+// only parses the DDL is exactly the shape that let that through, so
+// this one exercises a value instead: the assertion is that CreatedAt
+// comes back as a real instant, which is only possible if the column
+// type is right.
+//
+// Reverting migrations/0001_init.sql to TEXT must fail HERE, by name,
+// and not only as collateral damage in thirty unrelated tests.
+func TestSchema_RoundTripsAModel(t *testing.T) {
+ d := openDB(t)
+ if _, err := migrate.Apply(context.Background(), d,
+ migrate.Merge(sessions.Schema, idear.Schema)); err != nil {
+ t.Fatalf("migrate.Apply: %v", err)
+ }
+
+ m := &idear.Member{Subject: "s1", Email: "a@example.test", Name: "A", Role: idear.RoleOwner}
+ if err := d.G.Create(m).Error; err != nil {
+ t.Fatalf("writing a Member: %v", err)
+ }
+ if m.ID == 0 {
+ t.Fatal("Create did not assign an id")
+ }
+
+ var got idear.Member
+ if err := d.G.Where("id = ?", m.ID).Take(&got).Error; err != nil {
+ t.Fatalf("reading the Member back: %v\n"+
+ "An \"unsupported Scan ... into type *time.Time\" here means a timestamp column "+
+ "is declared TEXT again; it must be DATETIME.", err)
+ }
+ if got.CreatedAt.IsZero() {
+ t.Error("CreatedAt came back as the zero time; the column is not carrying an instant")
+ }
+ if got.UpdatedAt.IsZero() {
+ t.Error("UpdatedAt came back as the zero time")
+ }
+ if got.DeactivatedAt != nil {
+ t.Errorf("DeactivatedAt = %v, want nil — a NULL timestamp must scan as a nil *time.Time", got.DeactivatedAt)
+ }
+ if got.Subject != m.Subject || got.Role != idear.RoleOwner {
+ t.Errorf("read back %+v, want subject %q at role owner", got, m.Subject)
+ }
+
+ // The nullable timestamp must round-trip when it is SET, too — a
+ // deactivated member is read on every request the middleware
+ // gates.
+ when := time.Now().UTC().Truncate(time.Millisecond)
+ if err := d.G.Model(&idear.Member{}).Where("id = ?", m.ID).
+ Update("deactivated_at", when).Error; err != nil {
+ t.Fatalf("deactivating: %v", err)
+ }
+ var off idear.Member
+ if err := d.G.Where("id = ?", m.ID).Take(&off).Error; err != nil {
+ t.Fatalf("reading the deactivated Member back: %v", err)
+ }
+ if off.DeactivatedAt == nil || !off.DeactivatedAt.Equal(when) {
+ t.Errorf("DeactivatedAt = %v, want %v", off.DeactivatedAt, when)
+ }
+ if off.Active() {
+ t.Error("the member reads as active after being deactivated")
+ }
+}