fix(store): handle nullable collections.settings end-to-end (BUG-1482) (#561)

* fix(store): make ListCollectionsMinimal Postgres-safe (BUG-1482) `COALESCE(settings, '')` failed at planner time on Postgres because `collections.settings` is JSONB and `''` is not valid JSON (SQLSTATE 22P02). The query failed regardless of row contents; SQLite is type-loose and accepted it, leaving the bug latent in the two production callers (`handlers_dashboard.go`, `handlers_items.go`). Switch the query to a plain `SELECT ... settings ...` and scan into `sql.NullString`, materializing NULL as the empty-string sentinel. This preserves the existing contract that downstream consumers (`buildDoneContextMap`, `ListCollections`'s own scan loop) gate on via `if c.Settings != ""`, so no caller-side changes are needed. Adds two regression tests in `collections_test.go` that exercise the NULL-settings case (the planner-time failure mode) and the happy-path JSON round-trip. Both run against SQLite and Postgres via the existing PAD_TEST_POSTGRES_URL switch in `testStore`. * test(store): tighten ListCollectionsMinimal happy-path assertion (BUG-1482) Codex review round 1 flagged TestListCollectionsMinimalReturnsSettingsJSON as too permissive: `Settings != ""` would pass for `{}` or any wrong JSON payload. Postgres JSONB also normalizes formatting/key order, so a string compare against the input literal would be brittle across drivers. Switch to a semantic compare: unmarshal both sides into map[string]any and reflect.DeepEqual. This actually verifies the JSON round-trips through the (now fixed) ListCollectionsMinimal path on both drivers. * fix(store): NULL-safe settings scan in GetCollection / ListCollections / ExportWorkspace (BUG-1482) Round-2 extension of the same fix shape. Direct `Scan(... &c.Settings ...)` into a Go string fails on Postgres for any row holding a real NULL with "Scan error: converting NULL to string is unsupported". The column is nullable on both drivers (TEXT DEFAULT '{}' / JSONB DEFAULT '{}'), so legacy or manually-poisoned rows can 500 every handler that goes through these readers — `GetCollection` is the hot reader on every item handler, `ListCollections` powers dashboard + sidebar, `ExportWorkspace` crashes the export pipeline before any data is emitted. Same fix as ListCollectionsMinimal: scan into sql.NullString, materialize NULL as "" to preserve the existing sentinel contract that downstream consumers gate on via `if c.Settings != ""` (handlers_dashboard.go:247, handlers_items.go:1626, collections.go:196 in ListCollections's own post-scan loop). Audited; no caller depends on a non-empty default. Adds TestGetCollectionHandlesNullSettings, TestListCollectionsHandlesNullSettings, and TestExportWorkspaceHandlesNullSettings — each forces a NULL via direct UPDATE (bypassing CreateCollection's empty→`{}` coercion) and asserts the function returns without error and surfaces "" downstream. All pass on SQLite and Postgres. * fix(store): coerce empty-string settings to {} on workspace import (BUG-1482) The earlier commits in this PR made ExportWorkspace, GetCollection, and ListCollections all return `""` for a NULL `collections.settings` row, preserving the in-process sentinel contract that downstream consumers (buildDoneContextMap and friends) already gate on via `c.Settings != ""`. That fix surfaced a paired contract gap: ImportWorkspace previously inserted `c.Settings` verbatim into the collections table. After the reader fixes, an exported NULL-settings row materializes as `""` in the bundle, which Postgres's JSONB column rejects at INSERT time. Without this commit, exporting a workspace with any NULL-settings row and re-importing it would have crashed on Postgres — turning one half of a symmetric contract green while leaving the other half broken. Mirror the same coercion CreateCollection applies on the normal create path: when the bundle's settings field is the empty-string sentinel, write `"{}"` instead. Add a round-trip regression test (TestExportImportRoundTripWithNullSettings) that NULL-poisons a workspace's settings, exports, re-imports, and asserts the re-imported collections hold valid JSON. Verified on both drivers. * style(store): rewrite doc comment to avoid gofmt apostrophe-pair rewrite Go 1.19+ gofmt's doc-comment formatter collapses `''` (two ASCII apostrophes) inside backtick code spans into a single `”` (U+201D right double quotation mark) — a typographic-pair heuristic that doesn't quite fit when the literal pair is the load-bearing thing being described (here: SQL's empty-string literal in COALESCE). CI's golangci-lint flagged the file as gofmt-dirty for this reason. Rewrite the prose to describe the bug without using `''` literally: "coalesced settings against an empty SQL string literal" reads more clearly than `COALESCE(settings, '')` becoming `COALESCE(settings, ”)` after gofmt normalization. Functionally identical comment; lint-clean.
2026-09-22 10:33:27 +00:00 · 2026-05-15 17:22:57 -04:00
parent 7c663a3d3f
commit 714da48442
3 changed files with 301 additions and 6 deletions
@@ -65,6 +65,13 @@ func (s *Store) GetCollection(id string) (*models.Collection, error) {
 	var createdAt, updatedAt string
 	var deletedAt *string
 	var isDefault bool
+	// `collections.settings` is nullable on both drivers (TEXT DEFAULT '{}' on
+	// SQLite, JSONB DEFAULT '{}' on Postgres). A NULL row would fail to scan
+	// directly into a Go string on Postgres with "converting NULL to string
+	// is unsupported". Materialize NULL as "" to preserve the sentinel that
+	// downstream consumers gate on via `if c.Settings != ""`. Same fix shape
+	// as ListCollectionsMinimal — see BUG-1482.
+	var settings sql.NullString

 	err := s.db.QueryRow(s.q(`
 		SELECT id, workspace_id, name, slug, prefix, icon, description, schema, settings, sort_order, is_default, is_system, created_at, updated_at, deleted_at
@@ -72,7 +79,7 @@ func (s *Store) GetCollection(id string) (*models.Collection, error) {
 		WHERE id = ? AND deleted_at IS NULL
 	`), id).Scan(
 		&c.ID, &c.WorkspaceID, &c.Name, &c.Slug, &c.Prefix, &c.Icon, &c.Description,
-		&c.Schema, &c.Settings, &c.SortOrder, &isDefault, &c.IsSystem,
+		&c.Schema, &settings, &c.SortOrder, &isDefault, &c.IsSystem,
 		&createdAt, &updatedAt, &deletedAt,
 	)
 	if err == sql.ErrNoRows {
@@ -82,6 +89,9 @@ func (s *Store) GetCollection(id string) (*models.Collection, error) {
 		return nil, fmt.Errorf("get collection: %w", err)
 	}

+	if settings.Valid {
+		c.Settings = settings.String
+	}
 	c.IsDefault = isDefault
 	c.CreatedAt = parseTime(createdAt)
 	c.UpdatedAt = parseTime(updatedAt)
@@ -111,8 +121,18 @@ func (s *Store) GetCollectionBySlug(workspaceID, slug string) (*models.Collectio
 // handlers that build a ctxMap for isItemDone). Includes soft-deleted
 // collections so items still attached to them can be evaluated.
 func (s *Store) ListCollectionsMinimal(workspaceID string) ([]models.Collection, error) {
+	// Note: we deliberately avoid `COALESCE(settings, '')` here. On Postgres,
+	// `collections.settings` is JSONB and `COALESCE(jsonb_col, '')` forces a
+	// planner-time cast of `''` to JSON which fails with SQLSTATE 22P02
+	// ("invalid input syntax for type json"), regardless of row contents.
+	// SQLite is type-loose and accepts it, which is why the bug was latent
+	// (BUG-1482). Scanning into sql.NullString is dialect-agnostic and
+	// preserves the historical sentinel: a NULL settings value materializes
+	// as `""`, matching the contract that downstream consumers
+	// (buildDoneContextMap, ListCollections's own scan loop) already gate on
+	// via `if c.Settings != ""`.
 	rows, err := s.db.Query(
-		s.q(`SELECT id, schema, COALESCE(settings, '') FROM collections WHERE workspace_id = ?`),
+		s.q(`SELECT id, schema, settings FROM collections WHERE workspace_id = ?`),
 		workspaceID,
 	)
 	if err != nil {
@@ -122,9 +142,13 @@ func (s *Store) ListCollectionsMinimal(workspaceID string) ([]models.Collection,
 	var result []models.Collection
 	for rows.Next() {
 		var c models.Collection
-		if err := rows.Scan(&c.ID, &c.Schema, &c.Settings); err != nil {
+		var settings sql.NullString
+		if err := rows.Scan(&c.ID, &c.Schema, &settings); err != nil {
 			return nil, fmt.Errorf("scan collection minimal: %w", err)
 		}
+		if settings.Valid {
+			c.Settings = settings.String
+		}
 		result = append(result, c)
 	}
 	return result, rows.Err()
@@ -151,13 +175,21 @@ func (s *Store) ListCollections(workspaceID string) ([]models.Collection, error)
 		var c models.Collection
 		var createdAt, updatedAt string
 		var isDefault bool
+		// Nullable JSONB on Postgres; scan via sql.NullString and materialize
+		// NULL as "" so the downstream `if c.Settings != ""` guard below
+		// (and in handlers_dashboard.buildDoneContextMap) keeps working.
+		// Same fix shape as ListCollectionsMinimal — see BUG-1482.
+		var settings sql.NullString
 		if err := rows.Scan(
 			&c.ID, &c.WorkspaceID, &c.Name, &c.Slug, &c.Prefix, &c.Icon, &c.Description,
-			&c.Schema, &c.Settings, &c.SortOrder, &isDefault, &c.IsSystem,
+			&c.Schema, &settings, &c.SortOrder, &isDefault, &c.IsSystem,
 			&createdAt, &updatedAt, &c.ItemCount,
 		); err != nil {
 			return nil, err
 		}
+		if settings.Valid {
+			c.Settings = settings.String
+		}
 		c.IsDefault = isDefault
 		c.CreatedAt = parseTime(createdAt)
 		c.UpdatedAt = parseTime(updatedAt)
@@ -1,11 +1,251 @@
 package store

 import (
+	"encoding/json"
+	"reflect"
 	"testing"

 	"github.com/PerpetualSoftware/pad/internal/models"
 )

+// TestListCollectionsMinimalHandlesNullSettings is the regression guard for
+// BUG-1482: ListCollectionsMinimal previously coalesced settings against an
+// empty SQL string literal, which fails at planner time on Postgres because
+// collections.settings is JSONB and an empty string is not valid JSON
+// (SQLSTATE 22P02). The query failed regardless of row contents. SQLite's
+// loose typing hid the issue.
+//
+// This test exercises both drivers and explicitly stores a NULL `settings`
+// to cover the column-nullability branch — neither the SQLite migration
+// (`settings TEXT DEFAULT '{}'`) nor the Postgres one (`settings JSONB
+// DEFAULT '{}'`) marks the column NOT NULL, so production data can legally
+// hold NULL. The contract is that NULL surfaces as `""` so existing
+// `if c.Settings != ""` guards in downstream consumers continue to work.
+func TestListCollectionsMinimalHandlesNullSettings(t *testing.T) {
+	s := testStore(t)
+	ws := createTestWorkspace(t, s, "ListCollectionsMinimal NULL Settings")
+
+	if err := s.SeedDefaultCollections(ws.ID); err != nil {
+		t.Fatalf("SeedDefaultCollections error: %v", err)
+	}
+
+	// Force one collection's settings to NULL via direct SQL to simulate
+	// 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)
+	}
+	if len(colls) == 0 {
+		t.Fatalf("ListCollectionsMinimal 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)
+		}
+	}
+}
+
+// TestListCollectionsMinimalReturnsSettingsJSON verifies the happy path:
+// a collection with non-NULL JSON settings round-trips through the minimal
+// query intact on both drivers.
+func TestListCollectionsMinimalReturnsSettingsJSON(t *testing.T) {
+	s := testStore(t)
+	ws := createTestWorkspace(t, s, "ListCollectionsMinimal JSON Settings")
+
+	created, err := s.CreateCollection(ws.ID, models.CollectionCreate{
+		Name:     "Things",
+		Slug:     "things",
+		Settings: `{"done_field":"status","done_values":["closed"]}`,
+	})
+	if err != nil {
+		t.Fatalf("CreateCollection error: %v", err)
+	}
+
+	colls, err := s.ListCollectionsMinimal(ws.ID)
+	if err != nil {
+		t.Fatalf("ListCollectionsMinimal error: %v", err)
+	}
+	// Compare settings semantically. Postgres JSONB normalizes formatting and
+	// key order, so a byte-for-byte string compare against the input literal
+	// would be brittle across drivers. Unmarshal both sides and assert the
+	// decoded values are equal — this verifies the JSON actually round-trips
+	// rather than just that *some* non-empty string came back.
+	want := map[string]any{
+		"done_field":  "status",
+		"done_values": []any{"closed"},
+	}
+	var found bool
+	for _, c := range colls {
+		if c.ID != created.ID {
+			continue
+		}
+		found = true
+		if c.Settings == "" {
+			t.Fatalf("expected non-empty settings JSON, got empty string")
+		}
+		var got map[string]any
+		if err := json.Unmarshal([]byte(c.Settings), &got); err != nil {
+			t.Fatalf("settings is not valid JSON: %v (raw=%q)", err, c.Settings)
+		}
+		if !reflect.DeepEqual(got, want) {
+			t.Errorf("settings round-trip mismatch:\n  got:  %#v\n  want: %#v", got, want)
+		}
+	}
+	if !found {
+		t.Fatalf("created collection %q not returned by ListCollectionsMinimal", created.ID)
+	}
+}
+
+// TestGetCollectionHandlesNullSettings is the sibling regression guard for
+// BUG-1482 round-2: `GetCollection` previously scanned `settings` directly
+// into a Go string, which fails on Postgres for any row holding NULL with
+// "Scan error: converting NULL to string is unsupported". Latent today
+// because `CreateCollection` coerces empty→`{}`, but the column is nullable
+// on both drivers and legacy/manually-poisoned rows would 500 every handler
+// that goes through GetCollection. Sentinel contract: NULL → "".
+func TestGetCollectionHandlesNullSettings(t *testing.T) {
+	s := testStore(t)
+	ws := createTestWorkspace(t, s, "GetCollection NULL Settings")
+
+	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")
+
+	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)
+	}
+
+	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)
+	}
+	if imported == nil {
+		t.Fatalf("ImportWorkspace returned nil workspace")
+	}
+
+	// Re-read the imported collections and assert they hold valid JSON
+	// (the import-side coercion materialized `""` back to `"{}"`).
+	colls, err := s.ListCollections(imported.ID)
+	if err != nil {
+		t.Fatalf("ListCollections on imported workspace: %v", err)
+	}
+	if len(colls) == 0 {
+		t.Fatalf("imported workspace has 0 collections; expected the round-tripped defaults")
+	}
+	for _, c := range colls {
+		if c.Settings == "" {
+			t.Errorf("imported collection %q: settings should have been coerced from \"\" to a valid JSON object, got empty string", c.ID)
+			continue
+		}
+		var got map[string]any
+		if err := json.Unmarshal([]byte(c.Settings), &got); err != nil {
+			t.Errorf("imported collection %q: settings is not valid JSON: %v (raw=%q)", c.ID, err, c.Settings)
+		}
+	}
+}
+
 // TestSeedFromBlankTemplate verifies that bootstrapping a workspace from the
 // blank template (IDEA-1479) produces exactly two collections (Conventions,
 // Playbooks) and zero items. Drift here means the template silently grew
@@ -1,6 +1,7 @@
 package store

 import (
+	"database/sql"
 	"fmt"
 	"strings"
 	"time"
@@ -41,9 +42,19 @@ func (s *Store) ExportWorkspace(slug string) (*models.WorkspaceExport, error) {
 	for rows.Next() {
 		var c models.CollectionExport
 		var isDefault, isSystem bool
-		if err := rows.Scan(&c.ID, &c.Name, &c.Slug, &c.Icon, &c.Description, &c.Schema, &c.Settings, &c.Prefix, &c.SortOrder, &isDefault, &isSystem, &c.CreatedAt, &c.UpdatedAt); err != nil {
+		// `collections.settings` is nullable on both drivers; a NULL row
+		// would error out a direct scan into Go string on Postgres
+		// ("converting NULL to string is unsupported"). Materialize NULL
+		// as "" to keep the export's existing sentinel — same fix shape
+		// as ListCollectionsMinimal / GetCollection / ListCollections.
+		// See BUG-1482.
+		var settings sql.NullString
+		if err := rows.Scan(&c.ID, &c.Name, &c.Slug, &c.Icon, &c.Description, &c.Schema, &settings, &c.Prefix, &c.SortOrder, &isDefault, &isSystem, &c.CreatedAt, &c.UpdatedAt); err != nil {
 			return nil, fmt.Errorf("scan collection: %w", err)
 		}
+		if settings.Valid {
+			c.Settings = settings.String
+		}
 		c.IsDefault = isDefault
 		c.IsSystem = isSystem
 		export.Collections = append(export.Collections, c)
@@ -193,10 +204,22 @@ func (s *Store) ImportWorkspace(data *models.WorkspaceExport, newName string, ow
 		newCollID := newID()
 		collMap[c.ID] = newCollID

+		// Coerce the empty-string sentinel back to a valid JSON object before
+		// insert. Exports of NULL-settings rows surface as `""` per the
+		// store-layer sentinel contract (BUG-1482 round-2); Postgres's JSONB
+		// column rejects `""` at write time, and SQLite would accept it but
+		// then downstream JSON parsers would choke. CreateCollection does the
+		// same coercion on the normal create path; this loop bypasses that
+		// helper for transactional/verbatim import, so we mirror it here.
+		settings := c.Settings
+		if settings == "" {
+			settings = "{}"
+		}
+
 		_, err := tx.Exec(s.q(`
 			INSERT INTO collections (id, workspace_id, name, slug, icon, description, schema, settings, prefix, sort_order, is_default, is_system, created_at, updated_at)
 			VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?)`),
-			newCollID, ws.ID, c.Name, c.Slug, c.Icon, c.Description, c.Schema, c.Settings, c.Prefix, c.SortOrder, s.dialect.BoolToInt(c.IsDefault), s.dialect.BoolToInt(c.IsSystem),
+			newCollID, ws.ID, c.Name, c.Slug, c.Icon, c.Description, c.Schema, settings, c.Prefix, c.SortOrder, s.dialect.BoolToInt(c.IsDefault), s.dialect.BoolToInt(c.IsSystem),
 			c.CreatedAt, c.UpdatedAt)
 		if err != nil {
 			return nil, fmt.Errorf("import collection %s: %w", c.Name, err)