From 6f22f94a864a348fc3c42e9187bd79fa2083b872 Mon Sep 17 00:00:00 2001 From: Dave Date: Fri, 15 May 2026 23:17:23 +0000 Subject: [PATCH] fix(store): restore import-side settings coercion (codex R1 P1) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Codex R1 caught that the prior commit reverted the import-side `""→"{}"` coercion incorrectly. The NOT NULL DEFAULT '{}' schema constraint added by PR #562 only fires when the INSERT omits the settings column — but ImportWorkspace explicitly supplies the value. A legacy bundle or external JSON workspace import whose `collections[].settings` is "" would therefore bypass the default: Postgres rejects "" at JSONB type-validation; SQLite silently stores invalid JSON. The coercion was doing two jobs (BUG-1482 had folded them together): 1. Defending against NULL-materialized-as-"" on read — obsolete now that the column cannot hold NULL. 2. Defending against legacy/external "" settings on the import boundary — still required because schema constraints don't validate JSON. Job #1's defense (the sql.NullString scans) stays reverted; the column cannot hold NULL. Job #2's defense (the import-side coercion) is restored and renamed in the comment to reflect that it's a boundary normalizer for external data, not a transitional NULL-handler. `TestExportImportRoundTripWithEmptyStringSettings` is restored with an updated comment that makes the boundary-normalization framing explicit. The constraint-outcome tests from PR #562 remain unchanged. Tests pass on both drivers (SQLite full ./..., Postgres internal/store + internal/server). --- internal/store/collections_test.go | 75 ++++++++++++++++++++++++++++-- internal/store/export.go | 17 ++++++- 2 files changed, 86 insertions(+), 6 deletions(-) diff --git a/internal/store/collections_test.go b/internal/store/collections_test.go index 3bc25900..c67a85c6 100644 --- a/internal/store/collections_test.go +++ b/internal/store/collections_test.go @@ -17,11 +17,18 @@ 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 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. +// +// The defensive `sql.NullString` scans in collections.go / export.go were +// reverted in the IDEA-1484 follow-up — direct string scans are safe now +// that the column cannot hold NULL. +// +// The import-side `""→"{}"` coercion in ImportWorkspace is INTENTIONALLY +// RETAINED. The NOT NULL DEFAULT '{}' constraint only fires when the +// INSERT omits the column; ImportWorkspace explicitly supplies the value, +// so a `""` from a legacy bundle or external JSON import would bypass the +// default and either fail on Postgres (invalid JSONB) or silently store +// invalid JSON on SQLite. `TestExportImportRoundTripWithEmptyStringSettings` +// guards that boundary normalization. // TestCollectionsSettingsNotNullEnforced is the IDEA-1484 outcome guard: // after migration 055 / pg 034, attempting to write a literal SQL NULL @@ -124,6 +131,64 @@ func TestListCollectionsMinimalReturnsSettingsJSON(t *testing.T) { } } +// TestExportImportRoundTripWithEmptyStringSettings guards the import-side +// `""→"{}"` coercion in ImportWorkspace. IDEA-1484 (PR #562) hardened +// collections.settings to NOT NULL DEFAULT '{}', but the DEFAULT clause +// only fires when the INSERT omits the column — ImportWorkspace +// explicitly supplies the value. Without the coercion, a legacy bundle +// or external JSON payload whose settings is "" would fail at Postgres's +// JSONB type-validation (and silently store invalid JSON on SQLite). +// This test simulates the bundle by mutating the in-memory export bundle +// to inject "" settings, then asserts ImportWorkspace materializes them +// back to valid JSON on the destination side. +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/external bundle whose collections carry "" settings. + 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 (IDEA-1484 import-side coercion 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..51610c09 100644 --- a/internal/store/export.go +++ b/internal/store/export.go @@ -193,10 +193,25 @@ func (s *Store) ImportWorkspace(data *models.WorkspaceExport, newName string, ow newCollID := newID() collMap[c.ID] = newCollID + // Coerce empty-string settings to a valid JSON object before insert. + // IDEA-1484 (PR #562) hardened collections.settings to NOT NULL + // DEFAULT '{}', but this INSERT explicitly supplies the settings + // column — so the DEFAULT clause does NOT fire when c.Settings is + // "". Without this coercion, Postgres rejects `""` at JSONB + // type-validation and SQLite silently stores invalid JSON. Legacy + // bundles and plain-JSON workspace imports (handlers_workspaces.go, + // handlers_import_bundle.go, cmd/pad/main.go's migrate command) can + // still carry "" settings, so normalization belongs at the import + // boundary rather than at the schema level. + 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)