fix(store): restore import-side settings coercion (codex R1 P1)

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).
This commit is contained in:
Dave
2026-05-15 23:17:23 +00:00
parent ed6cfc42fd
commit 6f22f94a86
2 changed files with 86 additions and 6 deletions
+70 -5
View File
@@ -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
+16 -1
View File
@@ -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)