diff --git a/internal/store/collections.go b/internal/store/collections.go index 7efdf2d7..22fcd689 100644 --- a/internal/store/collections.go +++ b/internal/store/collections.go @@ -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) diff --git a/internal/store/collections_test.go b/internal/store/collections_test.go index 90854a79..db6356a0 100644 --- a/internal/store/collections_test.go +++ b/internal/store/collections_test.go @@ -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 diff --git a/internal/store/export.go b/internal/store/export.go index b6c1fd59..e112b6bc 100644 --- a/internal/store/export.go +++ b/internal/store/export.go @@ -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)