feat(store): enforce NOT NULL on collections.settings (IDEA-1484) (#562)

* feat(store): enforce NOT NULL on collections.settings (IDEA-1484)

Adds migration 055 (SQLite) and pg 034 (Postgres) to backfill any NULL
collections.settings rows to '{}' and then enforce NOT NULL DEFAULT '{}'
at the column level. Eliminates the bug class that BUG-1482 / PR #561
plugged defensively in the four reader sites.

SQLite uses the standard table-rebuild recipe (PRAGMA foreign_keys=OFF,
copy via COALESCE, RENAME, recreate the single dependent index from
032_permission_indexes.sql). Same PK values are preserved so FKs in
items, views, collection_access, and grants remain valid.

Postgres uses the simple in-place ALTER TABLE; SET DEFAULT is a no-op
belt-and-braces since 001_initial.sql:115 already had DEFAULT '{}'.

The defensive sql.NullString scans in collections.go / export.go and
the import-side ""→"{}" coercion in export.go remain in place — they
revert in a separate follow-up PR after this migration ships
everywhere.

Removes the four BUG-1482 NULL-only regression tests from
collections_test.go (their `UPDATE collections SET settings = NULL`
setup is now a hard write error against the new constraint and the
NULL-scan branch they guarded is no longer reachable). Reworks
TestExportImportRoundTripWithNullSettings into
TestExportImportRoundTripWithEmptyStringSettings — it now mutates
the exported bundle in-memory to carry the "" sentinel rather than
forcing a NULL row, still exercising the import-side coercion path
that survives this PR.

* test(store): cover collections.settings NOT NULL outcome (IDEA-1484)

Addresses Codex R1 P2: the migration test surface lacked direct
constraint-check coverage. Adds two focused outcome tests against the
post-migration schema:

- TestCollectionsSettingsNotNullEnforced — raw INSERT with settings=NULL
  must fail. Error shape differs across SQLite (NOT NULL constraint
  failed) and Postgres (SQLSTATE 23502); we only assert err != nil.
- TestCollectionsSettingsDefaultsToEmptyObject — raw INSERT omitting the
  settings column entirely must materialize the column DEFAULT as the
  Go string "{}" when read back via GetCollection. Same assertion on
  both drivers; the defensive sql.NullString scan + Postgres JSONB
  normalization both surface "{}".

Both tests reuse createTestWorkspace + the testStore harness, so they
run automatically on whichever driver the test invocation selects.
R1 P1 (migration runner atomicity) is out of scope per established
codebase precedent (022, 025 use the same pattern); will be filed as
a follow-up IDEA.

internal/store/collections_test.go

@@ -8,45 +8,65 @@ import (
 	"github.com/PerpetualSoftware/pad/internal/models"
 )
 
-// TestListCollectionsMinimalHandlesNullSettings is the regression guard for
+// IDEA-1484: the BUG-1482 NULL-settings regression tests
-// BUG-1482: ListCollectionsMinimal previously coalesced settings against an
+// (TestListCollectionsMinimalHandlesNullSettings,
-// empty SQL string literal, which fails at planner time on Postgres because
+// TestGetCollectionHandlesNullSettings,
-// collections.settings is JSONB and an empty string is not valid JSON
+// TestListCollectionsHandlesNullSettings,
-// (SQLSTATE 22P02). The query failed regardless of row contents. SQLite's
+// TestExportWorkspaceHandlesNullSettings) were removed when migration
-// loose typing hid the issue.
+// 055 / pg 034 made collections.settings NOT NULL DEFAULT '{}'. With the
-//
+// constraint in place, the `UPDATE collections SET settings = NULL` setup
-// This test exercises both drivers and explicitly stores a NULL `settings`
+// those tests relied on is now a hard write error, and the scenario they
-// to cover the column-nullability branch — neither the SQLite migration
+// guarded (production data legally holding NULL) is no longer reachable.
-// (`settings TEXT DEFAULT '{}'`) nor the Postgres one (`settings JSONB
+// The defensive `sql.NullString` scans in collections.go / export.go
-// DEFAULT '{}'`) marks the column NOT NULL, so production data can legally
+// remain in place and will be reverted in a separate follow-up PR.
-// hold NULL. The contract is that NULL surfaces as `""` so existing
+
-// `if c.Settings != ""` guards in downstream consumers continue to work.
+// TestCollectionsSettingsNotNullEnforced is the IDEA-1484 outcome guard:
-func TestListCollectionsMinimalHandlesNullSettings(t *testing.T) {
+// after migration 055 / pg 034, attempting to write a literal SQL NULL
+// into collections.settings must fail at the driver level. The error
+// shape differs across SQLite ("NOT NULL constraint failed:
+// collections.settings") and Postgres ("null value in column ... violates
+// not-null constraint", SQLSTATE 23502), so we only assert that an error
+// surfaces — not its content.
+func TestCollectionsSettingsNotNullEnforced(t *testing.T) {
 	s := testStore(t)
-	ws := createTestWorkspace(t, s, "ListCollectionsMinimal NULL Settings")
+	ws := createTestWorkspace(t, s, "NOT NULL Enforcement")
 
-	if err := s.SeedDefaultCollections(ws.ID); err != nil {
+	_, err := s.db.Exec(s.q(`
-		t.Fatalf("SeedDefaultCollections error: %v", err)
+		INSERT INTO collections (id, workspace_id, name, slug, settings, created_at, updated_at)
+		VALUES (?, ?, ?, ?, NULL, ?, ?)
+	`), "test-col-not-null", ws.ID, "Things", "things-not-null", "2024-01-01T00:00:00Z", "2024-01-01T00:00:00Z")
+	if err == nil {
+		t.Fatalf("expected NOT NULL constraint violation when inserting NULL settings, got nil error")
+	}
+}
+
+// TestCollectionsSettingsDefaultsToEmptyObject is the companion guard:
+// when an INSERT omits the settings column entirely, the column DEFAULT
+// must materialize as the empty JSON object `{}`. SQLite stores it as
+// TEXT and Postgres stores it as JSONB (which normalizes to `{}` on
+// readback); both surface through GetCollection's defensive scan as the
+// Go string `{}`.
+func TestCollectionsSettingsDefaultsToEmptyObject(t *testing.T) {
+	s := testStore(t)
+	ws := createTestWorkspace(t, s, "Settings Default")
+
+	const id = "test-col-default-settings"
+	if _, err := s.db.Exec(s.q(`
+		INSERT INTO collections (id, workspace_id, name, slug, created_at, updated_at)
+		VALUES (?, ?, ?, ?, ?, ?)
+	`), id, ws.ID, "Things", "things-default", "2024-01-01T00:00:00Z", "2024-01-01T00:00:00Z"); err != nil {
+		t.Fatalf("INSERT omitting settings failed: %v", err)
 	}
 
-	// Force one collection's settings to NULL via direct SQL to simulate
+	got, err := s.GetCollection(id)
-	// legacy / partially-initialized rows. CreateCollection's normal path
-	// coerces empty settings to "{}", so we have to bypass it.
-	if _, err := s.db.Exec(s.q(`UPDATE collections SET settings = NULL WHERE workspace_id = ?`), ws.ID); err != nil {
-		t.Fatalf("force NULL settings: %v", err)
-	}
-
-	colls, err := s.ListCollectionsMinimal(ws.ID)
 	if err != nil {
-		t.Fatalf("ListCollectionsMinimal error (BUG-1482 regression): %v", err)
+		t.Fatalf("GetCollection error: %v", err)
 	}
-	if len(colls) == 0 {
+	if got == nil {
-		t.Fatalf("ListCollectionsMinimal returned 0 collections; expected the seeded defaults")
+		t.Fatalf("GetCollection returned nil for %q", id)
 	}
-	for _, c := range colls {
+	if got.Settings != "{}" {
-		if c.Settings != "" {
+		t.Errorf("expected default settings to materialize as %q, got %q", "{}", got.Settings)
-			t.Errorf("collection %q: expected NULL settings to surface as empty string sentinel, got %q", c.ID, c.Settings)
-		}
 	}
 }
 
@@ -101,122 +121,38 @@ func TestListCollectionsMinimalReturnsSettingsJSON(t *testing.T) {
 	}
 }
 
-// TestGetCollectionHandlesNullSettings is the sibling regression guard for
+// TestExportImportRoundTripWithEmptyStringSettings guards the paired
-// BUG-1482 round-2: `GetCollection` previously scanned `settings` directly
+// import-side `""→"{}"` coercion at export.go:~210. After IDEA-1484's
-// into a Go string, which fails on Postgres for any row holding NULL with
+// migration, the source column can no longer hold NULL, but exports of
-// "Scan error: converting NULL to string is unsupported". Latent today
+// legacy bundles or pre-migration backups may still carry an empty-string
-// because `CreateCollection` coerces empty→`{}`, but the column is nullable
+// settings value (the BUG-1482 sentinel for the previously-nullable
-// on both drivers and legacy/manually-poisoned rows would 500 every handler
+// column). ImportWorkspace must coerce that back to a valid JSON object
-// that goes through GetCollection. Sentinel contract: NULL → "".
+// before INSERT, otherwise the import would fail on Postgres because `""`
-func TestGetCollectionHandlesNullSettings(t *testing.T) {
+// is not valid JSONB and downstream consumers gated on
-	s := testStore(t)
+// `c.Settings != ""` would misinterpret it.
-	ws := createTestWorkspace(t, s, "GetCollection NULL Settings")
+func TestExportImportRoundTripWithEmptyStringSettings(t *testing.T) {
-
-	created, err := s.CreateCollection(ws.ID, models.CollectionCreate{
-		Name: "Things",
-		Slug: "things",
-	})
-	if err != nil {
-		t.Fatalf("CreateCollection error: %v", err)
-	}
-	if _, err := s.db.Exec(s.q(`UPDATE collections SET settings = NULL WHERE id = ?`), created.ID); err != nil {
-		t.Fatalf("force NULL settings: %v", err)
-	}
-
-	got, err := s.GetCollection(created.ID)
-	if err != nil {
-		t.Fatalf("GetCollection error (BUG-1482 regression): %v", err)
-	}
-	if got == nil {
-		t.Fatalf("GetCollection returned nil for existing collection %q", created.ID)
-	}
-	if got.Settings != "" {
-		t.Errorf("expected NULL settings to surface as empty string sentinel, got %q", got.Settings)
-	}
-}
-
-// TestListCollectionsHandlesNullSettings guards the non-Minimal sibling.
-// `ListCollections` powers the dashboard + sidebar; a single NULL-settings
-// row would crash the entire list scan on Postgres.
-func TestListCollectionsHandlesNullSettings(t *testing.T) {
-	s := testStore(t)
-	ws := createTestWorkspace(t, s, "ListCollections NULL Settings")
-
-	if err := s.SeedDefaultCollections(ws.ID); err != nil {
-		t.Fatalf("SeedDefaultCollections error: %v", err)
-	}
-	if _, err := s.db.Exec(s.q(`UPDATE collections SET settings = NULL WHERE workspace_id = ?`), ws.ID); err != nil {
-		t.Fatalf("force NULL settings: %v", err)
-	}
-
-	colls, err := s.ListCollections(ws.ID)
-	if err != nil {
-		t.Fatalf("ListCollections error (BUG-1482 regression): %v", err)
-	}
-	if len(colls) == 0 {
-		t.Fatalf("ListCollections returned 0 collections; expected the seeded defaults")
-	}
-	for _, c := range colls {
-		if c.Settings != "" {
-			t.Errorf("collection %q: expected NULL settings to surface as empty string sentinel, got %q", c.ID, c.Settings)
-		}
-	}
-}
-
-// TestExportWorkspaceHandlesNullSettings guards the third sibling reader.
-// A NULL-settings row would crash the export pipeline on Postgres before
-// any data was emitted. Sentinel contract: NULL → "".
-func TestExportWorkspaceHandlesNullSettings(t *testing.T) {
-	s := testStore(t)
-	ws := createTestWorkspace(t, s, "ExportWorkspace NULL Settings")
-
-	if err := s.SeedDefaultCollections(ws.ID); err != nil {
-		t.Fatalf("SeedDefaultCollections error: %v", err)
-	}
-	if _, err := s.db.Exec(s.q(`UPDATE collections SET settings = NULL WHERE workspace_id = ?`), ws.ID); err != nil {
-		t.Fatalf("force NULL settings: %v", err)
-	}
-
-	exp, err := s.ExportWorkspace(ws.Slug)
-	if err != nil {
-		t.Fatalf("ExportWorkspace error (BUG-1482 regression): %v", err)
-	}
-	if len(exp.Collections) == 0 {
-		t.Fatalf("ExportWorkspace returned 0 collections; expected the seeded defaults")
-	}
-	for _, c := range exp.Collections {
-		if c.Settings != "" {
-			t.Errorf("exported collection %q: expected NULL settings to surface as empty string sentinel, got %q", c.ID, c.Settings)
-		}
-	}
-}
-
-// TestExportImportRoundTripWithNullSettings guards the paired contract created
-// by the BUG-1482 round-2 fix: now that ExportWorkspace successfully reads
-// NULL-settings rows (surfacing them as `""`), ImportWorkspace must be able
-// to re-insert those bundles. Without the import-side `""→"{}"` coercion at
-// export.go:~210, this round-trip would fail at INSERT time on Postgres
-// because `""` is not valid JSONB. SQLite is type-loose and accepts `""`,
-// but downstream consumers (gated on `c.Settings != ""`) would interpret an
-// empty-string-settings collection as "no settings" — silently different
-// from the original NULL row's semantic intent.
-func TestExportImportRoundTripWithNullSettings(t *testing.T) {
 	s := testStore(t)
 	owner := createTestUser(t, s, "round-trip-owner@test.com", "Round Trip Owner", "password123")
-	src := createTestWorkspace(t, s, "Export-Import Round Trip NULL Settings")
+	src := createTestWorkspace(t, s, "Export-Import Round Trip Empty Settings")
 
 	if err := s.SeedDefaultCollections(src.ID); err != nil {
 		t.Fatalf("SeedDefaultCollections error: %v", err)
 	}
-	if _, err := s.db.Exec(s.q(`UPDATE collections SET settings = NULL WHERE workspace_id = ?`), src.ID); err != nil {
-		t.Fatalf("force NULL settings: %v", err)
-	}
 
 	exp, err := s.ExportWorkspace(src.Slug)
 	if err != nil {
 		t.Fatalf("ExportWorkspace error: %v", err)
 	}
 
+	// Simulate a legacy export bundle whose collections carry the
+	// empty-string sentinel (originally surfaced by BUG-1482's defensive
+	// `sql.NullString` scan for NULL-settings rows). The IDEA-1484
+	// migration eliminates NULL at the column level, but ImportWorkspace
+	// must still tolerate `""` from older bundles in flight.
+	for i := range exp.Collections {
+		exp.Collections[i].Settings = ""
+	}
+
 	imported, err := s.ImportWorkspace(exp, "round-trip-import-target", owner.ID)
 	if err != nil {
 		t.Fatalf("ImportWorkspace error (BUG-1482 import-side regression): %v", err)

internal/store/migrations/055_collections_settings_not_null.sql

@@ -0,0 +1,57 @@
+-- IDEA-1484: harden collections.settings to NOT NULL DEFAULT '{}'.
+-- See also BUG-1482 / PR #561 (squash 714da48): the defensive `sql.NullString`
+-- scans in collections.go and export.go remain in place and will be reverted
+-- in a separate follow-up PR after this migration has rolled out everywhere.
+--
+-- SQLite does not support ALTER COLUMN ... SET NOT NULL, so the table is
+-- rebuilt via the standard SQLite recipe. Existing FKs from items, views,
+-- collection_access, and grants point at collections(id); since we preserve
+-- the same primary-key values during the copy, those references remain valid.
+-- foreign_keys is toggled OFF for the duration to avoid the constraint
+-- checker tripping on the transient DROP TABLE.
+
+PRAGMA foreign_keys = OFF;
+
+-- Backfill any NULL settings before applying the constraint.
+UPDATE collections SET settings = '{}' WHERE settings IS NULL;
+
+DROP TABLE IF EXISTS collections_new;
+
+CREATE TABLE collections_new (
+    id           TEXT PRIMARY KEY,
+    workspace_id TEXT NOT NULL REFERENCES workspaces(id),
+    name         TEXT NOT NULL,
+    slug         TEXT NOT NULL,
+    icon         TEXT DEFAULT '',
+    description  TEXT DEFAULT '',
+    schema       TEXT NOT NULL DEFAULT '{"fields":[]}',
+    settings     TEXT NOT NULL DEFAULT '{}',
+    sort_order   INTEGER DEFAULT 0,
+    is_default   INTEGER DEFAULT 0,
+    created_at   TEXT NOT NULL,
+    updated_at   TEXT NOT NULL,
+    deleted_at   TEXT,
+    prefix       TEXT NOT NULL DEFAULT '',
+    is_system    INTEGER NOT NULL DEFAULT 0,
+    UNIQUE(workspace_id, slug)
+);
+
+INSERT INTO collections_new (
+    id, workspace_id, name, slug, icon, description, schema, settings,
+    sort_order, is_default, created_at, updated_at, deleted_at,
+    prefix, is_system
+)
+SELECT
+    id, workspace_id, name, slug, icon, description, schema,
+    COALESCE(settings, '{}'),
+    sort_order, is_default, created_at, updated_at, deleted_at,
+    prefix, is_system
+FROM collections;
+
+DROP TABLE collections;
+ALTER TABLE collections_new RENAME TO collections;
+
+-- Recreate indexes (originally from 032_permission_indexes.sql).
+CREATE INDEX IF NOT EXISTS idx_collections_system ON collections(workspace_id, is_system) WHERE is_system = 1 AND deleted_at IS NULL;
+
+PRAGMA foreign_keys = ON;

internal/store/pgmigrations/034_collections_settings_not_null.sql

@@ -0,0 +1,12 @@
+-- IDEA-1484: harden collections.settings to NOT NULL DEFAULT '{}'::jsonb.
+-- The DEFAULT clause was already '{}'::jsonb in 001_initial.sql, so SET
+-- DEFAULT here is a no-op idempotency belt; SET NOT NULL is the load-bearing
+-- change. See also BUG-1482 / PR #561.
+--
+-- The defensive `sql.NullString` scans in collections.go / export.go remain
+-- in place and will be reverted in a separate follow-up PR after this
+-- migration has rolled out everywhere.
+
+UPDATE collections SET settings = '{}'::jsonb WHERE settings IS NULL;
+ALTER TABLE collections ALTER COLUMN settings SET NOT NULL;
+ALTER TABLE collections ALTER COLUMN settings SET DEFAULT '{}'::jsonb;