mirror of
https://github.com/PerpetualSoftware/pad.git
synced 2026-09-22 10:33:27 +00:00
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:
@@ -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
|
||||
|
||||
@@ -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)
|
||||
|
||||
Reference in New Issue
Block a user