← back to Cli Printing Press

fix(cli): backfill parent_id on store upgrade (#272) (#276)

892de38b779dc8d86174127185f3af549f1a8479 · 2026-04-24 23:55:05 -0700 · Trevin Chow

* fix(cli): backfill parent_id column on store upgrade (#272)

CREATE TABLE IF NOT EXISTS is a no-op when the table already exists, so
the parent_id column added by the dependent-resources work never landed
on databases created by older binaries — and the follow-on CREATE INDEX
ON <table>(parent_id) then errored with "no such column: parent_id",
locking users out of their own data.

Add ensureColumn / backfillColumns helpers to store.go.tmpl and call
them from migrate() before the migrations slice runs. ensureColumn is
idempotent: skips if the table doesn't yet exist (fresh install — the
CREATE TABLE creates it with the column declared) or if the column is
already present, otherwise runs ALTER TABLE ADD COLUMN. The template
emits one (table, "parent_id", "TEXT") tuple per dependent table.

Adds TestMigrate_AddsParentIDOnUpgrade_<Table> to
store_schema_version_test.go.tmpl, emitted per parent_id-bearing table.
Pre-creates a raw DB with the older shape, calls Open(), and asserts
no error plus the column now exists. Runs in CI under
TestGenerateDependentSyncCompiles.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* fix(cli): drop safeName from Go-string-literal contexts in store templates

safeSQLName wraps SQL reserved words (~50 entries: order, view, key,
index, column, references, match, trigger, replace, set, default,
values, etc.) in literal double-quote characters. Five sites in the
store templates were interpolating safeName output into Go double-quoted
string literals, where the embedded quotes break compilation for any
spec whose dependent-resource snake_cased table name hits the reserved
list. The very upgrade-path fix from the previous commit did not compile
for those CLIs.

Affected sites:
- store.go.tmpl:165 — backfillColumns struct slice (this PR's regression)
- store_schema_version_test.go.tmpl:179 — t.Fatalf format (this PR's
  regression)
- store_upsert_batch_test.go.tmpl:59,92 — fmt.Sprintf SQL builders
  (latent since PR #268)

Pass the bare table name to ensureColumn / fmt.Sprintf and let the SQL
format string own the identifier quoting (`FROM "%s"`, `PRAGMA
table_info("%s")`). ensureColumn already does this for its own DDL and
parameter-binds the sqlite_master probe.

Also harden ensureColumn against a concurrent-Open race: SQLite returns
SQLITE_ERROR ("duplicate column name") for ALTER TABLE ADD COLUMN when
the column was added between our PRAGMA check and the ALTER —
busy_timeout doesn't retry SQLITE_ERROR — so the losing process used to
surface a fatal error even though the DB was correctly migrated. Treat
duplicate-column as success.

Adds TestGenerateDependentSyncReservedWordCompiles which generates a
CLI with a `references` child resource (snake_cases to a reserved word),
builds the project, and runs go test ./internal/store. Locks in the
regression for the safeName class of bug at the generator-unit-test
layer rather than waiting for dogfood time.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

Files touched

• M internal/generator/generator_test.go
• M internal/generator/templates/store.go.tmpl
• M internal/generator/templates/store_schema_version_test.go.tmpl
• M internal/generator/templates/store_upsert_batch_test.go.tmpl

Diff

commit 892de38b779dc8d86174127185f3af549f1a8479
Author: Trevin Chow <trevin@trevinchow.com>
Date:   Fri Apr 24 23:55:05 2026 -0700

    fix(cli): backfill parent_id on store upgrade (#272) (#276)
    
    * fix(cli): backfill parent_id column on store upgrade (#272)
    
    CREATE TABLE IF NOT EXISTS is a no-op when the table already exists, so
    the parent_id column added by the dependent-resources work never landed
    on databases created by older binaries — and the follow-on CREATE INDEX
    ON <table>(parent_id) then errored with "no such column: parent_id",
    locking users out of their own data.
    
    Add ensureColumn / backfillColumns helpers to store.go.tmpl and call
    them from migrate() before the migrations slice runs. ensureColumn is
    idempotent: skips if the table doesn't yet exist (fresh install — the
    CREATE TABLE creates it with the column declared) or if the column is
    already present, otherwise runs ALTER TABLE ADD COLUMN. The template
    emits one (table, "parent_id", "TEXT") tuple per dependent table.
    
    Adds TestMigrate_AddsParentIDOnUpgrade_<Table> to
    store_schema_version_test.go.tmpl, emitted per parent_id-bearing table.
    Pre-creates a raw DB with the older shape, calls Open(), and asserts
    no error plus the column now exists. Runs in CI under
    TestGenerateDependentSyncCompiles.
    
    Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
    
    * fix(cli): drop safeName from Go-string-literal contexts in store templates
    
    safeSQLName wraps SQL reserved words (~50 entries: order, view, key,
    index, column, references, match, trigger, replace, set, default,
    values, etc.) in literal double-quote characters. Five sites in the
    store templates were interpolating safeName output into Go double-quoted
    string literals, where the embedded quotes break compilation for any
    spec whose dependent-resource snake_cased table name hits the reserved
    list. The very upgrade-path fix from the previous commit did not compile
    for those CLIs.
    
    Affected sites:
    - store.go.tmpl:165 — backfillColumns struct slice (this PR's regression)
    - store_schema_version_test.go.tmpl:179 — t.Fatalf format (this PR's
      regression)
    - store_upsert_batch_test.go.tmpl:59,92 — fmt.Sprintf SQL builders
      (latent since PR #268)
    
    Pass the bare table name to ensureColumn / fmt.Sprintf and let the SQL
    format string own the identifier quoting (`FROM "%s"`, `PRAGMA
    table_info("%s")`). ensureColumn already does this for its own DDL and
    parameter-binds the sqlite_master probe.
    
    Also harden ensureColumn against a concurrent-Open race: SQLite returns
    SQLITE_ERROR ("duplicate column name") for ALTER TABLE ADD COLUMN when
    the column was added between our PRAGMA check and the ALTER —
    busy_timeout doesn't retry SQLITE_ERROR — so the losing process used to
    surface a fatal error even though the DB was correctly migrated. Treat
    duplicate-column as success.
    
    Adds TestGenerateDependentSyncReservedWordCompiles which generates a
    CLI with a `references` child resource (snake_cases to a reserved word),
    builds the project, and runs go test ./internal/store. Locks in the
    regression for the safeName class of bug at the generator-unit-test
    layer rather than waiting for dogfood time.
    
    Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
    
    ---------
    
    Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
---
 internal/generator/generator_test.go               | 66 ++++++++++++++++
 internal/generator/templates/store.go.tmpl         | 91 ++++++++++++++++++++++
 .../templates/store_schema_version_test.go.tmpl    | 67 ++++++++++++++++
 .../templates/store_upsert_batch_test.go.tmpl      |  4 +-
 4 files changed, 226 insertions(+), 2 deletions(-)

diff --git a/internal/generator/generator_test.go b/internal/generator/generator_test.go
index 3c09ee8c..6e4f21c7 100644
--- a/internal/generator/generator_test.go
+++ b/internal/generator/generator_test.go
@@ -2957,6 +2957,72 @@ func TestGenerateDependentSyncCompiles(t *testing.T) {
 	runGoCommand(t, outputDir, "test", "./internal/store")
 }
 
+func TestGenerateDependentSyncReservedWordCompiles(t *testing.T) {
+	t.Parallel()
+
+	// A spec whose dependent-resource table name is a SQL reserved word
+	// (e.g. references, trigger, view, order) must still produce a CLI
+	// that builds and whose store tests pass. The store template's
+	// backfillColumns slice and the upgrade-path test's t.Fatalf format
+	// string both interpolate the table name into Go double-quoted
+	// strings; safeSQLName quote-wraps reserved words for SQL contexts,
+	// so applying it in a Go-string context would emit invalid Go for
+	// any reserved-word resource. Regression for issue #272 follow-up.
+	apiSpec := &spec.APISpec{
+		Name:    "docstore",
+		Version: "0.1.0",
+		BaseURL: "https://api.example.com",
+		Auth: spec.AuthConfig{
+			Type:    "api_key",
+			Header:  "Authorization",
+			Format:  "Bearer {token}",
+			EnvVars: []string{"DOCSTORE_API_KEY"},
+		},
+		Config: spec.ConfigSpec{
+			Format: "toml",
+			Path:   "~/.config/docstore-pp-cli/config.toml",
+		},
+		Resources: map[string]spec.Resource{
+			"documents": {
+				Description: "Manage documents",
+				Endpoints: map[string]spec.Endpoint{
+					"list": {
+						Method:      "GET",
+						Path:        "/documents",
+						Description: "List documents",
+						Response:    spec.ResponseDef{Type: "array"},
+						Pagination:  &spec.Pagination{CursorParam: "after", LimitParam: "limit"},
+					},
+				},
+			},
+			// "references" snake_cases to "references" — a SQL reserved
+			// word per internal/generator/schema_builder.go:322.
+			"references": {
+				Description: "Manage references attached to a document",
+				Endpoints: map[string]spec.Endpoint{
+					"list": {
+						Method:      "GET",
+						Path:        "/documents/{documentId}/references",
+						Description: "List references for a document",
+						Response:    spec.ResponseDef{Type: "array"},
+						Pagination:  &spec.Pagination{CursorParam: "after", LimitParam: "limit"},
+					},
+				},
+			},
+		},
+	}
+
+	outputDir := filepath.Join(t.TempDir(), naming.CLI(apiSpec.Name))
+	gen := New(apiSpec, outputDir)
+	require.NoError(t, gen.Generate())
+
+	// The generated store.go must compile (no embedded "" from safeName
+	// quote-wrapping) and the per-table upgrade test must run cleanly.
+	runGoCommand(t, outputDir, "mod", "tidy")
+	runGoCommand(t, outputDir, "build", "./...")
+	runGoCommand(t, outputDir, "test", "./internal/store")
+}
+
 func TestGenerateGraphQLCompiles(t *testing.T) {
 	t.Parallel()
 
diff --git a/internal/generator/templates/store.go.tmpl b/internal/generator/templates/store.go.tmpl
index 0511b1c9..a01ea7f7 100644
--- a/internal/generator/templates/store.go.tmpl
+++ b/internal/generator/templates/store.go.tmpl
@@ -102,6 +102,93 @@ func (s *Store) setSchemaVersion(version int) error {
 	return nil
 }
 
+// ensureColumn adds a column to an existing table if it isn't already
+// present. It is the upgrade-path safety valve for schema additions:
+// CREATE TABLE IF NOT EXISTS is a no-op when the table already exists, so
+// columns added by newer binaries (e.g. parent_id from the dependent-
+// resources work) never land on databases created by older binaries —
+// which then trip "no such column" when a follow-on CREATE INDEX runs.
+//
+// Skips silently if the table doesn't yet exist (fresh install — the
+// CREATE TABLE migration will create it with the column already declared)
+// or if the column already exists.
+func (s *Store) ensureColumn(table, column, decl string) error {
+	var name string
+	err := s.db.QueryRow(
+		`SELECT name FROM sqlite_master WHERE type='table' AND name=?`, table,
+	).Scan(&name)
+	if err == sql.ErrNoRows {
+		return nil
+	}
+	if err != nil {
+		return fmt.Errorf("checking table %s: %w", table, err)
+	}
+
+	rows, err := s.db.Query(fmt.Sprintf(`PRAGMA table_info("%s")`, table))
+	if err != nil {
+		return fmt.Errorf("table_info %s: %w", table, err)
+	}
+	defer rows.Close()
+	for rows.Next() {
+		var cid int
+		var n, typ string
+		var notnull, pk int
+		var dflt sql.NullString
+		if err := rows.Scan(&cid, &n, &typ, &notnull, &dflt, &pk); err != nil {
+			return fmt.Errorf("scan table_info %s: %w", table, err)
+		}
+		if n == column {
+			return nil
+		}
+	}
+	if err := rows.Err(); err != nil {
+		return fmt.Errorf("iterating table_info %s: %w", table, err)
+	}
+
+	if _, err := s.db.Exec(fmt.Sprintf(`ALTER TABLE "%s" ADD COLUMN "%s" %s`, table, column, decl)); err != nil {
+		// A concurrent Open() may have added the column between our
+		// PRAGMA check and this ALTER. SQLite returns SQLITE_ERROR with
+		// "duplicate column name", which busy_timeout does not retry.
+		// The DB is now in the desired state regardless of who won.
+		if strings.Contains(err.Error(), "duplicate column name") {
+			return nil
+		}
+		return fmt.Errorf("add column %s.%s: %w", table, column, err)
+	}
+	return nil
+}
+
+// backfillColumns adds columns that newer binaries declare but that
+// pre-existing databases (created before those columns were added) lack.
+// Must run before the migrations slice so that subsequent CREATE INDEX
+// statements referencing the column can succeed against the upgraded
+// table. Idempotent: safe to call on fresh DBs (table-not-found short-
+// circuit) and on already-current DBs (column-exists short-circuit).
+//
+// Table names are emitted bare (no safeName) — ensureColumn double-quotes
+// them at SQL emit time and uses parameter binding for the sqlite_master
+// lookup, so the values flow as Go string literals first and SQL
+// identifiers second. Wrapping with safeName here would embed literal
+// double-quote characters into the Go string and break compilation for
+// any spec whose dependent-resource snake_cased name is a SQL reserved
+// word.
+func (s *Store) backfillColumns() error {
+	for _, c := range []struct{ table, column, decl string }{
+{{- range $table := .Tables}}
+{{- range $col := $table.Columns}}
+{{- if eq $col.Name "parent_id"}}
+		{table: "{{$table.Name}}", column: "parent_id", decl: "TEXT"},
+{{- end}}
+{{- end}}
+{{- end}}
+	} {
+		if err := s.ensureColumn(c.table, c.column, c.decl); err != nil {
+			return err
+		}
+	}
+	return nil
+}
+
 func (s *Store) migrate() error {
 	current, err := s.SchemaVersion()
 	if err != nil {
@@ -111,6 +198,10 @@ func (s *Store) migrate() error {
 		return fmt.Errorf("database schema version %d is newer than supported version %d; upgrade the CLI binary or open an older database", current, StoreSchemaVersion)
 	}
 
+	if err := s.backfillColumns(); err != nil {
+		return fmt.Errorf("backfilling columns: %w", err)
+	}
+
 	migrations := []string{
 		`CREATE TABLE IF NOT EXISTS resources (
 			id TEXT PRIMARY KEY,
diff --git a/internal/generator/templates/store_schema_version_test.go.tmpl b/internal/generator/templates/store_schema_version_test.go.tmpl
index 7ddd5e04..fed45228 100644
--- a/internal/generator/templates/store_schema_version_test.go.tmpl
+++ b/internal/generator/templates/store_schema_version_test.go.tmpl
@@ -114,3 +114,70 @@ func TestSchemaVersion_ReopenIsIdempotent(t *testing.T) {
 		t.Fatalf("reopened version = %d, want %d", v, StoreSchemaVersion)
 	}
 }
+{{- range $table := .Tables}}
+{{- $hasParentID := false}}
+{{- range $col := $table.Columns}}{{if eq $col.Name "parent_id"}}{{$hasParentID = true}}{{end}}{{end}}
+{{- if $hasParentID}}
+
+// TestMigrate_AddsParentIDOnUpgrade_{{pascal $table.Name}} verifies that opening a
+// database created by an older binary — before the dependent-resources work
+// added the parent_id column — succeeds and adds the column rather than
+// failing with "no such column: parent_id" when CREATE INDEX runs against a
+// pre-existing table. Regression for issue #272.
+func TestMigrate_AddsParentIDOnUpgrade_{{pascal $table.Name}}(t *testing.T) {
+	dbPath := filepath.Join(t.TempDir(), "data.db")
+
+	// Pre-create the DB with the older table shape: id, data, synced_at and
+	// no parent_id column. This mirrors what a pre-dependent-resources binary
+	// would have written. user_version stays 0 (pre-gate).
+	raw, err := sql.Open("sqlite", dbPath)
+	if err != nil {
+		t.Fatalf("open raw: %v", err)
+	}
+	if _, err := raw.Exec(`CREATE TABLE {{safeName $table.Name}} (
+		id TEXT PRIMARY KEY,
+		data JSON NOT NULL,
+		synced_at DATETIME DEFAULT CURRENT_TIMESTAMP
+	)`); err != nil {
+		raw.Close()
+		t.Fatalf("create old table: %v", err)
+	}
+	raw.Close()
+
+	// Opening with the new binary must run the CREATE INDEX on parent_id
+	// without erroring on the missing column.
+	s, err := Open(dbPath)
+	if err != nil {
+		t.Fatalf("open upgraded db: %v", err)
+	}
+	defer s.Close()
+
+	// The migration must have added the parent_id column.
+	rows, err := s.DB().Query(`PRAGMA table_info({{safeName $table.Name}})`)
+	if err != nil {
+		t.Fatalf("table_info: %v", err)
+	}
+	defer rows.Close()
+
+	hasParentID := false
+	for rows.Next() {
+		var cid int
+		var name, typ string
+		var notnull, pk int
+		var dflt sql.NullString
+		if err := rows.Scan(&cid, &name, &typ, &notnull, &dflt, &pk); err != nil {
+			t.Fatalf("scan: %v", err)
+		}
+		if name == "parent_id" {
+			hasParentID = true
+		}
+	}
+	if err := rows.Err(); err != nil {
+		t.Fatalf("rows: %v", err)
+	}
+	if !hasParentID {
+		t.Fatalf("parent_id column missing from {{$table.Name}} after migrate")
+	}
+}
+{{- end}}
+{{- end}}
diff --git a/internal/generator/templates/store_upsert_batch_test.go.tmpl b/internal/generator/templates/store_upsert_batch_test.go.tmpl
index a25feaf0..5de1787f 100644
--- a/internal/generator/templates/store_upsert_batch_test.go.tmpl
+++ b/internal/generator/templates/store_upsert_batch_test.go.tmpl
@@ -56,7 +56,7 @@ func TestUpsertBatch_Populates{{pascal .Name}}Table(t *testing.T) {
 	}
 
 	var typed int
-	typedQuery := fmt.Sprintf(`SELECT COUNT(*) FROM %s`, "{{safeName .Name}}")
+	typedQuery := fmt.Sprintf(`SELECT COUNT(*) FROM "%s"`, "{{.Name}}")
 	if err := db.QueryRow(typedQuery).Scan(&typed); err != nil {
 		t.Fatalf("count {{.Name}}: %v", err)
 	}
@@ -89,7 +89,7 @@ func TestUpsertBatch_Sets{{pascal .Name}}ParentID(t *testing.T) {
 	db := s.DB()
 
 	var matchedA int
-	parentQuery := fmt.Sprintf(`SELECT COUNT(*) FROM %s WHERE parent_id = ?`, "{{safeName .Name}}")
+	parentQuery := fmt.Sprintf(`SELECT COUNT(*) FROM "%s" WHERE parent_id = ?`, "{{.Name}}")
 	if err := db.QueryRow(parentQuery, "parent-A").Scan(&matchedA); err != nil {
 		t.Fatalf("count by parent_id: %v", err)
 	}

← 7c9cce48 chore(main): release 2.3.2 (#269)  ·  back to Cli Printing Press  ·  fix(ci): build from local checkout instead of proxying priva 1a95d78b →