From ed6cfc42fdef7157bdc4000865522be201b0a3d4 Mon Sep 17 00:00:00 2001 From: Dave Date: Fri, 15 May 2026 23:04:54 +0000 Subject: [PATCH] refactor(store): drop defensive sql.NullString scans on collections.settings (IDEA-1484 follow-up) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit PR #562 (squash 0766d7e) hardened collections.settings to NOT NULL DEFAULT '{}' at the schema level. The defensive sql.NullString scans introduced by PR #561 (BUG-1482, squash 714da48) and the paired import-side ""→"{}" coercion in ImportWorkspace are no longer load-bearing — the database now enforces the invariant the readers were defensively reconstructing. Reverted sites: - internal/store/collections.go: GetCollection, ListCollectionsMinimal, ListCollections — direct &c.Settings scans. - internal/store/export.go: ExportWorkspace scan + ImportWorkspace coercion. - internal/store/items.go: scanCollectionDoneFilters helper. - internal/store/item_stars.go: buildCollectionDoneContextMap helper. Test changes: - Removed TestExportImportRoundTripWithEmptyStringSettings, whose purpose evaporated with the import-side coercion. The constraint-outcome tests (TestCollectionsSettingsNotNullEnforced, TestCollectionsSettingsDefaultsToEmptyObject) from PR #562 remain — they assert the load-bearing schema invariant. --- internal/store/collections.go | 38 ++--------------- internal/store/collections_test.go | 68 +++--------------------------- internal/store/export.go | 27 +----------- internal/store/item_stars.go | 7 ++- internal/store/items.go | 7 ++- 5 files changed, 16 insertions(+), 131 deletions(-) diff --git a/internal/store/collections.go b/internal/store/collections.go index 22fcd689..a19b6a6e 100644 --- a/internal/store/collections.go +++ b/internal/store/collections.go @@ -65,13 +65,6 @@ 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 @@ -79,7 +72,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, &settings, &c.SortOrder, &isDefault, &c.IsSystem, + &c.Schema, &c.Settings, &c.SortOrder, &isDefault, &c.IsSystem, &createdAt, &updatedAt, &deletedAt, ) if err == sql.ErrNoRows { @@ -89,9 +82,6 @@ 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) @@ -121,16 +111,6 @@ 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, settings FROM collections WHERE workspace_id = ?`), workspaceID, @@ -142,13 +122,9 @@ func (s *Store) ListCollectionsMinimal(workspaceID string) ([]models.Collection, var result []models.Collection for rows.Next() { var c models.Collection - var settings sql.NullString - if err := rows.Scan(&c.ID, &c.Schema, &settings); err != nil { + if err := rows.Scan(&c.ID, &c.Schema, &c.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() @@ -175,21 +151,13 @@ 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, &settings, &c.SortOrder, &isDefault, &c.IsSystem, + &c.Schema, &c.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 de15bd44..3bc25900 100644 --- a/internal/store/collections_test.go +++ b/internal/store/collections_test.go @@ -17,8 +17,11 @@ import ( // constraint in place, the `UPDATE collections SET settings = NULL` setup // those tests relied on is now a hard write error, and the scenario they // guarded (production data legally holding NULL) is no longer reachable. -// The defensive `sql.NullString` scans in collections.go / export.go -// remain in place and will be reverted in a separate follow-up PR. +// The defensive `sql.NullString` scans in collections.go / export.go and +// the paired import-side `""→"{}"` coercion (along with +// TestExportImportRoundTripWithEmptyStringSettings) were reverted in the +// IDEA-1484 follow-up now that the schema constraint is the load-bearing +// invariant. // TestCollectionsSettingsNotNullEnforced is the IDEA-1484 outcome guard: // after migration 055 / pg 034, attempting to write a literal SQL NULL @@ -121,67 +124,6 @@ func TestListCollectionsMinimalReturnsSettingsJSON(t *testing.T) { } } -// TestExportImportRoundTripWithEmptyStringSettings guards the paired -// import-side `""→"{}"` coercion at export.go:~210. After IDEA-1484's -// migration, the source column can no longer hold NULL, but exports of -// legacy bundles or pre-migration backups may still carry an empty-string -// settings value (the BUG-1482 sentinel for the previously-nullable -// column). ImportWorkspace must coerce that back to a valid JSON object -// before INSERT, otherwise the import would fail on Postgres because `""` -// is not valid JSONB and downstream consumers gated on -// `c.Settings != ""` would misinterpret it. -func TestExportImportRoundTripWithEmptyStringSettings(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 Empty Settings") - - if err := s.SeedDefaultCollections(src.ID); err != nil { - t.Fatalf("SeedDefaultCollections error: %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) - } - 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 e112b6bc..b6c1fd59 100644 --- a/internal/store/export.go +++ b/internal/store/export.go @@ -1,7 +1,6 @@ package store import ( - "database/sql" "fmt" "strings" "time" @@ -42,19 +41,9 @@ func (s *Store) ExportWorkspace(slug string) (*models.WorkspaceExport, error) { for rows.Next() { var c models.CollectionExport var isDefault, isSystem bool - // `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 { + 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 { 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) @@ -204,22 +193,10 @@ 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, 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, c.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) diff --git a/internal/store/item_stars.go b/internal/store/item_stars.go index a10c709d..48f5fd80 100644 --- a/internal/store/item_stars.go +++ b/internal/store/item_stars.go @@ -182,8 +182,7 @@ func (s *Store) buildCollectionDoneContextMap(workspaceID string) (map[string]co m := make(map[string]collectionDoneContext) for rows.Next() { - var id, rawSchema string - var rawSettings sql.NullString + var id, rawSchema, rawSettings string if err := rows.Scan(&id, &rawSchema, &rawSettings); err != nil { return nil, err } @@ -194,8 +193,8 @@ func (s *Store) buildCollectionDoneContextMap(workspaceID string) (map[string]co // rather than silently treating the item as non-terminal. ctx.schema = models.CollectionSchema{} } - if rawSettings.Valid && rawSettings.String != "" { - _ = json.Unmarshal([]byte(rawSettings.String), &ctx.settings) + if rawSettings != "" { + _ = json.Unmarshal([]byte(rawSettings), &ctx.settings) } m[id] = ctx } diff --git a/internal/store/items.go b/internal/store/items.go index 845fc0d5..ef8433b2 100644 --- a/internal/store/items.go +++ b/internal/store/items.go @@ -2160,8 +2160,7 @@ func (s *Store) childrenDoneFiltersForCollection(workspaceID, collectionSlug str func scanCollectionDoneFilters(rows *sql.Rows) []collectionDoneFilter { var filters []collectionDoneFilter for rows.Next() { - var id, schemaJSON string - var settingsJSON sql.NullString + var id, schemaJSON, settingsJSON string if err := rows.Scan(&id, &schemaJSON, &settingsJSON); err != nil { continue } @@ -2179,8 +2178,8 @@ func scanCollectionDoneFilters(rows *sql.Rows) []collectionDoneFilter { continue } var settings models.CollectionSettings - if settingsJSON.Valid && settingsJSON.String != "" { - _ = json.Unmarshal([]byte(settingsJSON.String), &settings) + if settingsJSON != "" { + _ = json.Unmarshal([]byte(settingsJSON), &settings) } key, values := models.TerminalValuesForDoneField(schema, settings) filters = append(filters, collectionDoneFilter{