From 5174a3b8251c49b32582babb247f3f597b94fd38 Mon Sep 17 00:00:00 2001 From: ogunf Date: Fri, 14 Aug 2026 01:16:12 +0000 Subject: [PATCH] refactor(store): enforce NOT NULL on nullable text columns Make challenges.client_domain, sessions.memo, and sessions.client_domain NOT NULL DEFAULT '', storing the absence of a value as an empty string rather than NULL. Adds a golang-migrate migration that backfills existing NULLs before tightening the columns, and updates the store to insert and read strings directly instead of through a nullable helper. Closes #3 --- .../000002_not_null_text_columns.down.sql | 8 ++++++ .../000002_not_null_text_columns.up.sql | 11 ++++++++ internal/store/postgres.go | 25 ++++--------------- internal/store/store_test.go | 7 ++++++ 4 files changed, 31 insertions(+), 20 deletions(-) create mode 100644 internal/store/migrations/000002_not_null_text_columns.down.sql create mode 100644 internal/store/migrations/000002_not_null_text_columns.up.sql diff --git a/internal/store/migrations/000002_not_null_text_columns.down.sql b/internal/store/migrations/000002_not_null_text_columns.down.sql new file mode 100644 index 0000000..46da75b --- /dev/null +++ b/internal/store/migrations/000002_not_null_text_columns.down.sql @@ -0,0 +1,8 @@ +ALTER TABLE challenges ALTER COLUMN client_domain DROP NOT NULL; +ALTER TABLE challenges ALTER COLUMN client_domain DROP DEFAULT; + +ALTER TABLE sessions ALTER COLUMN memo DROP NOT NULL; +ALTER TABLE sessions ALTER COLUMN memo DROP DEFAULT; + +ALTER TABLE sessions ALTER COLUMN client_domain DROP NOT NULL; +ALTER TABLE sessions ALTER COLUMN client_domain DROP DEFAULT; diff --git a/internal/store/migrations/000002_not_null_text_columns.up.sql b/internal/store/migrations/000002_not_null_text_columns.up.sql new file mode 100644 index 0000000..a32c971 --- /dev/null +++ b/internal/store/migrations/000002_not_null_text_columns.up.sql @@ -0,0 +1,11 @@ +UPDATE challenges SET client_domain = '' WHERE client_domain IS NULL; +ALTER TABLE challenges ALTER COLUMN client_domain SET DEFAULT ''; +ALTER TABLE challenges ALTER COLUMN client_domain SET NOT NULL; + +UPDATE sessions SET memo = '' WHERE memo IS NULL; +ALTER TABLE sessions ALTER COLUMN memo SET DEFAULT ''; +ALTER TABLE sessions ALTER COLUMN memo SET NOT NULL; + +UPDATE sessions SET client_domain = '' WHERE client_domain IS NULL; +ALTER TABLE sessions ALTER COLUMN client_domain SET DEFAULT ''; +ALTER TABLE sessions ALTER COLUMN client_domain SET NOT NULL; diff --git a/internal/store/postgres.go b/internal/store/postgres.go index 43e5177..9e62e8c 100644 --- a/internal/store/postgres.go +++ b/internal/store/postgres.go @@ -82,7 +82,7 @@ func (p *Postgres) RecordChallenge(ctx context.Context, rec ChallengeRecord) err _, err := p.pool.Exec(ctx, query, rec.Nonce, rec.Account, rec.HomeDomain, - nullable(rec.ClientDomain), rec.IssuedAt, rec.ExpiresAt) + rec.ClientDomain, rec.IssuedAt, rec.ExpiresAt) if err != nil { return fmt.Errorf("recording challenge: %w", err) } @@ -104,16 +104,10 @@ func (p *Postgres) ConsumeChallenge(ctx context.Context, nonce string, now time. WHERE nonce = $1 AND consumed_at IS NULL AND expires_at >= $2 RETURNING account, home_domain, client_domain` - var ( - out ConsumedChallenge - clientDomain *string - ) + var out ConsumedChallenge err := p.pool.QueryRow(ctx, consume, nonce, now). - Scan(&out.Account, &out.HomeDomain, &clientDomain) + Scan(&out.Account, &out.HomeDomain, &out.ClientDomain) if err == nil { - if clientDomain != nil { - out.ClientDomain = *clientDomain - } return &out, nil } if !errors.Is(err, pgx.ErrNoRows) { @@ -160,8 +154,8 @@ func (p *Postgres) RecordSession(ctx context.Context, rec SessionRecord) error { VALUES ($1, $2, $3, $4, $5, $6, $7)` _, err := p.pool.Exec(ctx, query, - rec.JTI, rec.Account, nullable(rec.Memo), rec.HomeDomain, - nullable(rec.ClientDomain), rec.IssuedAt, rec.ExpiresAt) + rec.JTI, rec.Account, rec.Memo, rec.HomeDomain, + rec.ClientDomain, rec.IssuedAt, rec.ExpiresAt) if err != nil { return fmt.Errorf("recording session: %w", err) } @@ -209,12 +203,3 @@ func (p *Postgres) CleanupExpiredChallenges(ctx context.Context, interval time.D } } } - -// nullable maps an empty string to SQL NULL, so an absent client domain is -// stored as NULL rather than as an empty string. -func nullable(s string) *string { - if s == "" { - return nil - } - return &s -} diff --git a/internal/store/store_test.go b/internal/store/store_test.go index f50cccc..fec0390 100644 --- a/internal/store/store_test.go +++ b/internal/store/store_test.go @@ -65,10 +65,17 @@ func TestMigrationsAreEmbedded(t *testing.T) { require.ElementsMatch(t, []string{ "migrations/000001_init.up.sql", "migrations/000001_init.down.sql", + "migrations/000002_not_null_text_columns.up.sql", + "migrations/000002_not_null_text_columns.down.sql", }, entries) up, err := fs.ReadFile(migrationsFS, "migrations/000001_init.up.sql") require.NoError(t, err) require.Contains(t, string(up), "CREATE TABLE challenges") require.Contains(t, string(up), "CREATE TABLE sessions") + + notNull, err := fs.ReadFile(migrationsFS, "migrations/000002_not_null_text_columns.up.sql") + require.NoError(t, err) + require.Contains(t, string(notNull), "client_domain SET NOT NULL") + require.Contains(t, string(notNull), "memo SET NOT NULL") }