gsadmin/pad
1
0
Fork 0
mirror of https://github.com/PerpetualSoftware/pad.git synced 2026-09-22 10:33:27 +00:00
Code Issues Packages Projects Releases Wiki Activity

refactor(store): drop defensive sql.NullString scans on collections.settings (IDEA-1484 follow-up)

Browse Source 
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.
This commit is contained in:
Dave
2026-05-15 23:04:54 +00:00
parent 0766d7ecf1
commit ed6cfc42fd
5 changed files with 16 additions and 131 deletions
Download Patch File Download Diff File Expand all files Collapse all files
internal/store/collections.go
+3 -35
View File
@@ -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)
internal/store/collections_test.go
+5 -63
View File
@@ -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
internal/store/export.go
+2 -25
View File
@@ -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)
internal/store/item_stars.go
+3 -4
View File
@@ -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
}
internal/store/items.go
+3 -4
View File
@@ -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{