refactor(store): enforce NOT NULL on nullable text columns

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;

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;

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
-}

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")
 }