← back to Cli Printing Press
fix(cli): harden store migrations for generated identifiers (#308)
f33d8e55fbd219ce51ff90a9439dc72324695310 · 2026-04-26 11:04:03 -0700 · Trevin Chow
Backfill all generated store columns before migration indexes run, and quote invalid SQLite identifiers for tables, indexes, FTS tables, and triggers. This prevents generated CLIs from failing upgrades when specs produce indexed non-parent columns or numeric resource names.
Files touched
M internal/generator/generator.goM internal/generator/generator_test.goM internal/generator/schema_builder.goM internal/generator/schema_builder_test.goM internal/generator/templates/store.go.tmplM internal/generator/templates/store_schema_version_test.go.tmpl
Diff
commit f33d8e55fbd219ce51ff90a9439dc72324695310
Author: Trevin Chow <trevin@trevinchow.com>
Date: Sun Apr 26 11:04:03 2026 -0700
fix(cli): harden store migrations for generated identifiers (#308)
Backfill all generated store columns before migration indexes run, and quote invalid SQLite identifiers for tables, indexes, FTS tables, and triggers. This prevents generated CLIs from failing upgrades when specs produce indexed non-parent columns or numeric resource names.
---
internal/generator/generator.go | 33 ++++++-
internal/generator/generator_test.go | 103 +++++++++++++++++++++
internal/generator/schema_builder.go | 32 ++++++-
internal/generator/schema_builder_test.go | 21 +++++
internal/generator/templates/store.go.tmpl | 30 +++---
.../templates/store_schema_version_test.go.tmpl | 45 ++++-----
6 files changed, 223 insertions(+), 41 deletions(-)
diff --git a/internal/generator/generator.go b/internal/generator/generator.go
index f0ee5d94..63d64567 100644
--- a/internal/generator/generator.go
+++ b/internal/generator/generator.go
@@ -254,6 +254,13 @@ func New(s *spec.APISpec, outputDir string) *Generator {
"jsonEnumSuggestion": jsonEnumSuggestion,
"envName": naming.EnvPrefix,
"safeName": safeSQLName,
+ "isBackfillColumn": isStoreBackfillColumn,
+ "hasBackfillColumns": hasStoreBackfillColumns,
+ "backfillDecl": storeBackfillDecl,
+ "safeNameSuffix": func(name, suffix string) string {
+ return safeSQLName(name + suffix)
+ },
+ "sqlString": sqlStringLiteral,
"hasDomainUpsert": func(name string) bool {
return domainUpsertMethodName(name) != "UpsertBatch"
},
@@ -1714,7 +1721,11 @@ func toPascal(s string) string {
lower := strings.ToLower(part)
parts[i] = strings.ToUpper(lower[:1]) + lower[1:]
}
- return strings.Join(parts, "")
+ result := strings.Join(parts, "")
+ if len(result) > 0 && !unicode.IsLetter(rune(result[0])) {
+ result = "V" + result
+ }
+ return result
}
func domainUpsertMethodName(tableName string) string {
@@ -1820,6 +1831,26 @@ func updateSet(cols []ColumnDef) string {
return strings.Join(updates, ", ")
}
+func isStoreBackfillColumn(col ColumnDef) bool {
+ switch col.Name {
+ case "id", "data", "synced_at":
+ return false
+ default:
+ return !col.PrimaryKey
+ }
+}
+
+func hasStoreBackfillColumns(table TableDef) bool {
+ return slices.ContainsFunc(table.Columns, isStoreBackfillColumn)
+}
+
+func storeBackfillDecl(col ColumnDef) string {
+ if strings.TrimSpace(col.Type) == "" {
+ return "TEXT"
+ }
+ return col.Type
+}
+
func cobraFlagFunc(t string) string {
switch t {
case "string":
diff --git a/internal/generator/generator_test.go b/internal/generator/generator_test.go
index d4f257eb..9ca14b60 100644
--- a/internal/generator/generator_test.go
+++ b/internal/generator/generator_test.go
@@ -1049,6 +1049,109 @@ func TestGenerateStoreUpsertBatchDispatchesToTypedTable(t *testing.T) {
runGoCommand(t, outputDir, "test", "./internal/store")
}
+func TestGenerateStoreBackfillsIndexedColumnsOnUpgrade(t *testing.T) {
+ t.Parallel()
+
+ apiSpec := &spec.APISpec{
+ Name: "indexedupgrade",
+ Version: "0.1.0",
+ BaseURL: "https://api.example.com",
+ Auth: spec.AuthConfig{Type: "none"},
+ Config: spec.ConfigSpec{
+ Format: "toml",
+ Path: "~/.config/indexedupgrade-pp-cli/config.toml",
+ },
+ Resources: map[string]spec.Resource{
+ "emails": {
+ Description: "Manage emails",
+ Endpoints: map[string]spec.Endpoint{
+ "list": {
+ Method: "GET",
+ Path: "/emails",
+ Description: "List emails",
+ Response: spec.ResponseDef{Type: "array"},
+ Params: []spec.Param{
+ {Name: "id", Type: "string"},
+ {Name: "email_id", Type: "string"},
+ {Name: "name", Type: "string"},
+ {Name: "description", Type: "string"},
+ {Name: "created_at", Type: "string", Format: "date-time"},
+ },
+ },
+ },
+ },
+ },
+ }
+
+ outputDir := filepath.Join(t.TempDir(), naming.CLI(apiSpec.Name))
+ gen := New(apiSpec, outputDir)
+ gen.VisionSet = VisionTemplateSet{Store: true}
+ require.NoError(t, gen.Generate())
+
+ storeSrc, err := os.ReadFile(filepath.Join(outputDir, "internal", "store", "store.go"))
+ require.NoError(t, err)
+ src := string(storeSrc)
+ assert.Contains(t, src, `{table: "emails", column: "email_id", decl: "TEXT"}`)
+ assert.Contains(t, src, `{table: "emails", column: "created_at", decl: "DATETIME"}`)
+
+ runGoCommand(t, outputDir, "mod", "tidy")
+ runGoCommand(t, outputDir, "test", "./internal/store")
+}
+
+func TestGenerateStoreQuotesNumericTableAndDerivedIdentifiers(t *testing.T) {
+ t.Parallel()
+
+ apiSpec := &spec.APISpec{
+ Name: "numericstore",
+ Version: "0.1.0",
+ BaseURL: "https://api.example.com",
+ Auth: spec.AuthConfig{Type: "none"},
+ Config: spec.ConfigSpec{
+ Format: "toml",
+ Path: "~/.config/numericstore-pp-cli/config.toml",
+ },
+ Resources: map[string]spec.Resource{
+ "0": {
+ Description: "Manage numeric resources",
+ Endpoints: map[string]spec.Endpoint{
+ "list": {
+ Method: "GET",
+ Path: "/0",
+ Description: "List numeric resources",
+ Response: spec.ResponseDef{Type: "array"},
+ Params: []spec.Param{
+ {Name: "id", Type: "string"},
+ {Name: "replay_id", Type: "string"},
+ {Name: "name", Type: "string"},
+ {Name: "description", Type: "string"},
+ {Name: "message", Type: "string"},
+ {Name: "created_at", Type: "string", Format: "date-time"},
+ },
+ },
+ },
+ },
+ },
+ }
+
+ outputDir := filepath.Join(t.TempDir(), naming.CLI(apiSpec.Name))
+ gen := New(apiSpec, outputDir)
+ gen.VisionSet = VisionTemplateSet{Store: true}
+ require.NoError(t, gen.Generate())
+
+ storeSrc, err := os.ReadFile(filepath.Join(outputDir, "internal", "store", "store.go"))
+ require.NoError(t, err)
+ src := string(storeSrc)
+ assert.Contains(t, src, `CREATE TABLE IF NOT EXISTS "0"`)
+ assert.Contains(t, src, `CREATE VIRTUAL TABLE IF NOT EXISTS "0_fts"`)
+ assert.Contains(t, src, `CREATE TRIGGER IF NOT EXISTS "0_ai"`)
+ assert.Contains(t, src, `content='0'`)
+ assert.NotContains(t, src, "func (s *Store) upsert0Tx(")
+ assert.Contains(t, src, "func (s *Store) upsertV0Tx(")
+
+ runGoCommand(t, outputDir, "mod", "tidy")
+ runGoCommand(t, outputDir, "test", "./internal/store")
+}
+
// --- Unit 7: Feature Verification Tests ---
func generatePetstore(t *testing.T) string {
diff --git a/internal/generator/schema_builder.go b/internal/generator/schema_builder.go
index ed38c8fc..9dddaf3c 100644
--- a/internal/generator/schema_builder.go
+++ b/internal/generator/schema_builder.go
@@ -335,15 +335,39 @@ var sqlReservedWords = map[string]bool{
"trigger": true, "view": true, "replace": true, "match": true,
}
-// safeSQLName returns the identifier double-quoted if it's a SQL reserved word.
-// Safe to use as a template function for table and column names in SQL DDL.
+// safeSQLName returns an identifier that is safe to use in SQLite DDL.
+// SQLite accepts many names when double-quoted, so quote reserved words and
+// anything that is not a valid bare identifier (for example, names beginning
+// with a digit).
func safeSQLName(name string) string {
- if sqlReservedWords[strings.ToLower(name)] {
- return `"` + name + `"`
+ if sqlReservedWords[strings.ToLower(name)] || !isBareSQLiteIdentifier(name) {
+ return `"` + strings.ReplaceAll(name, `"`, `""`) + `"`
}
return name
}
+func isBareSQLiteIdentifier(name string) bool {
+ if name == "" {
+ return false
+ }
+ for i, r := range name {
+ if i == 0 {
+ if r != '_' && (r < 'A' || r > 'Z') && (r < 'a' || r > 'z') {
+ return false
+ }
+ continue
+ }
+ if r != '_' && (r < 'A' || r > 'Z') && (r < 'a' || r > 'z') && (r < '0' || r > '9') {
+ return false
+ }
+ }
+ return true
+}
+
+func sqlStringLiteral(s string) string {
+ return `'` + strings.ReplaceAll(s, `'`, `''`) + `'`
+}
+
// toSnakeCase converts camelCase, PascalCase, or kebab-case to snake_case.
func toSnakeCase(s string) string {
s = strings.ReplaceAll(s, ".", "_")
diff --git a/internal/generator/schema_builder_test.go b/internal/generator/schema_builder_test.go
index 7fd379db..82f43114 100644
--- a/internal/generator/schema_builder_test.go
+++ b/internal/generator/schema_builder_test.go
@@ -37,6 +37,27 @@ func TestToSnakeCase(t *testing.T) {
}
}
+func TestSafeSQLNameQuotesUnsafeIdentifiers(t *testing.T) {
+ tests := []struct {
+ name string
+ in string
+ want string
+ }{
+ {name: "bare identifier", in: "messages", want: "messages"},
+ {name: "reserved word", in: "references", want: `"references"`},
+ {name: "starts with digit", in: "0", want: `"0"`},
+ {name: "derived starts with digit", in: "0_fts", want: `"0_fts"`},
+ {name: "contains punctuation", in: "foo/bar", want: `"foo/bar"`},
+ {name: "escapes quote", in: `foo"bar`, want: `"foo""bar"`},
+ }
+
+ for _, tt := range tests {
+ t.Run(tt.name, func(t *testing.T) {
+ assert.Equal(t, tt.want, safeSQLName(tt.in))
+ })
+ }
+}
+
func TestCollectTextFieldNames(t *testing.T) {
// Fields like tag/label/category/metadata should be picked up for FTS5
// alongside the core text fields. Motivated by the ESPN retro where
diff --git a/internal/generator/templates/store.go.tmpl b/internal/generator/templates/store.go.tmpl
index a01ea7f7..4d739bf1 100644
--- a/internal/generator/templates/store.go.tmpl
+++ b/internal/generator/templates/store.go.tmpl
@@ -176,8 +176,8 @@ 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"},
+{{- if isBackfillColumn $col}}
+ {table: "{{$table.Name}}", column: "{{$col.Name}}", decl: "{{backfillDecl $col}}"},
{{- end}}
{{- end}}
{{- end}}
@@ -230,34 +230,34 @@ func (s *Store) migrate() error {
{{- end}}
)`,
{{- range .Indexes}}
- `CREATE {{if .Unique}}UNIQUE {{end}}INDEX IF NOT EXISTS {{.Name}} ON {{safeName .TableName}}({{safeName .Columns}})`,
+ `CREATE {{if .Unique}}UNIQUE {{end}}INDEX IF NOT EXISTS {{safeName .Name}} ON {{safeName .TableName}}({{safeName .Columns}})`,
{{- end}}
{{- if .FTS5}}
{{- if .FTS5Triggers}}
- `CREATE VIRTUAL TABLE IF NOT EXISTS {{.Name}}_fts USING fts5(
+ `CREATE VIRTUAL TABLE IF NOT EXISTS {{safeNameSuffix .Name "_fts"}} USING fts5(
{{- range $i, $field := .FTS5Fields}}
{{- if $i}},{{end}}
{{safeName $field}}
{{- end}},
- content='{{safeName .Name}}',
+ content={{sqlString .Name}},
content_rowid='rowid'
)`,
- `CREATE TRIGGER IF NOT EXISTS {{.Name}}_ai AFTER INSERT ON {{safeName .Name}} BEGIN
- INSERT INTO {{.Name}}_fts(rowid, {{safeJoin .FTS5Fields ", "}})
+ `CREATE TRIGGER IF NOT EXISTS {{safeNameSuffix .Name "_ai"}} AFTER INSERT ON {{safeName .Name}} BEGIN
+ INSERT INTO {{safeNameSuffix .Name "_fts"}}(rowid, {{safeJoin .FTS5Fields ", "}})
VALUES (new.rowid, {{- range $i, $field := .FTS5Fields}}{{if $i}}, {{end}}new.{{safeName $field}}{{- end}});
END`,
- `CREATE TRIGGER IF NOT EXISTS {{.Name}}_ad AFTER DELETE ON {{safeName .Name}} BEGIN
- INSERT INTO {{.Name}}_fts({{.Name}}_fts, rowid, {{safeJoin .FTS5Fields ", "}})
+ `CREATE TRIGGER IF NOT EXISTS {{safeNameSuffix .Name "_ad"}} AFTER DELETE ON {{safeName .Name}} BEGIN
+ INSERT INTO {{safeNameSuffix .Name "_fts"}}({{safeNameSuffix .Name "_fts"}}, rowid, {{safeJoin .FTS5Fields ", "}})
VALUES ('delete', old.rowid, {{- range $i, $field := .FTS5Fields}}{{if $i}}, {{end}}old.{{safeName $field}}{{- end}});
END`,
- `CREATE TRIGGER IF NOT EXISTS {{.Name}}_au AFTER UPDATE ON {{safeName .Name}} BEGIN
- INSERT INTO {{.Name}}_fts({{.Name}}_fts, rowid, {{safeJoin .FTS5Fields ", "}})
+ `CREATE TRIGGER IF NOT EXISTS {{safeNameSuffix .Name "_au"}} AFTER UPDATE ON {{safeName .Name}} BEGIN
+ INSERT INTO {{safeNameSuffix .Name "_fts"}}({{safeNameSuffix .Name "_fts"}}, rowid, {{safeJoin .FTS5Fields ", "}})
VALUES ('delete', old.rowid, {{- range $i, $field := .FTS5Fields}}{{if $i}}, {{end}}old.{{safeName $field}}{{- end}});
- INSERT INTO {{.Name}}_fts(rowid, {{safeJoin .FTS5Fields ", "}})
+ INSERT INTO {{safeNameSuffix .Name "_fts"}}(rowid, {{safeJoin .FTS5Fields ", "}})
VALUES (new.rowid, {{- range $i, $field := .FTS5Fields}}{{if $i}}, {{end}}new.{{safeName $field}}{{- end}});
END`,
{{- else}}
- `CREATE VIRTUAL TABLE IF NOT EXISTS {{.Name}}_fts USING fts5(
+ `CREATE VIRTUAL TABLE IF NOT EXISTS {{safeNameSuffix .Name "_fts"}} USING fts5(
id,
{{- range $i, $field := .FTS5Fields}}
{{safeName $field}},
@@ -462,9 +462,9 @@ func (s *Store) upsert{{pascal .Name}}Tx(tx *sql.Tx, id string, obj map[string]a
// Standalone FTS: manually sync since content-sync triggers aren't used.
// Use explicit rowid for FTS5 compatibility with modernc.org/sqlite.
ftsRowid := ftsRowID(id)
- _, _ = tx.Exec(`DELETE FROM {{.Name}}_fts WHERE rowid = ?`, ftsRowid)
+ _, _ = tx.Exec(`DELETE FROM {{safeNameSuffix .Name "_fts"}} WHERE rowid = ?`, ftsRowid)
_, _ = tx.Exec(
- `INSERT INTO {{.Name}}_fts (rowid, id, {{safeJoin .FTS5Fields ", "}}) VALUES (?{{- range .FTS5Fields}}, ?{{- end}}, ?)`,
+ `INSERT INTO {{safeNameSuffix .Name "_fts"}} (rowid, id, {{safeJoin .FTS5Fields ", "}}) VALUES (?{{- range .FTS5Fields}}, ?{{- end}}, ?)`,
ftsRowid,
id,
{{- range .FTS5Fields}}
diff --git a/internal/generator/templates/store_schema_version_test.go.tmpl b/internal/generator/templates/store_schema_version_test.go.tmpl
index fed45228..9f44969d 100644
--- a/internal/generator/templates/store_schema_version_test.go.tmpl
+++ b/internal/generator/templates/store_schema_version_test.go.tmpl
@@ -115,21 +115,17 @@ func TestSchemaVersion_ReopenIsIdempotent(t *testing.T) {
}
}
{{- 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) {
+{{- if hasBackfillColumns $table}}
+
+// TestMigrate_AddsColumnsOnUpgrade_{{pascal $table.Name}} verifies that opening a
+// database created by an older binary succeeds and adds newly generated
+// columns before CREATE INDEX runs against the pre-existing table. Regression
+// coverage for parent_id upgrades and indexed generated columns.
+func TestMigrate_AddsColumnsOnUpgrade_{{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).
+ // none of the newer generated columns. user_version stays 0 (pre-gate).
raw, err := sql.Open("sqlite", dbPath)
if err != nil {
t.Fatalf("open raw: %v", err)
@@ -144,22 +140,22 @@ func TestMigrate_AddsParentIDOnUpgrade_{{pascal $table.Name}}(t *testing.T) {
}
raw.Close()
- // Opening with the new binary must run the CREATE INDEX on parent_id
- // without erroring on the missing column.
+ // Opening with the new binary must run CREATE INDEX statements without
+ // erroring on missing generated columns.
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.
+ // The migration must have added every generated 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
+ hasColumn := make(map[string]bool)
for rows.Next() {
var cid int
var name, typ string
@@ -168,15 +164,22 @@ func TestMigrate_AddsParentIDOnUpgrade_{{pascal $table.Name}}(t *testing.T) {
if err := rows.Scan(&cid, &name, &typ, ¬null, &dflt, &pk); err != nil {
t.Fatalf("scan: %v", err)
}
- if name == "parent_id" {
- hasParentID = true
- }
+ hasColumn[name] = 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")
+
+ for _, want := range []string{
+{{- range $col := $table.Columns}}
+{{- if isBackfillColumn $col}}
+ "{{$col.Name}}",
+{{- end}}
+{{- end}}
+ } {
+ if !hasColumn[want] {
+ t.Fatalf("%s column missing from {{$table.Name}} after migrate", want)
+ }
}
}
{{- end}}
← 42721ebc chore(main): release 2.3.8 (#304)
·
back to Cli Printing Press
·
test(cli): add golden output harness (#310) 0a1e80d9 →