Skip to content

Commit 4bbe2a9

Browse files
kriscolemanclaude
andcommitted
fix(persistence): return errors instead of silently skipping bad specs
Addressing review feedback on schema.go. driverTableSchema returned nil (and UpdateDBSchema then skipped the table) when a spec had no schema, was missing the driver's schema block, or the driver was unrecognized. Those are all malformed inputs that should fail loudly rather than be silently skipped. - driverTableSchema now returns an error for a nil schema, a missing per-driver schema block, and an unsupported driver. - UpdateDBSchema propagates that error, errors on a table spec with no name, and guards against a nil connection. - Add unit tests covering the error paths. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
1 parent 6382c53 commit 4bbe2a9

2 files changed

Lines changed: 97 additions & 10 deletions

File tree

pkg/persistence/schema.go

Lines changed: 17 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,9 @@ func UpdateDBSchema(driver string, uri string, schemaDir string) error {
2222
if err != nil {
2323
return errors.Wrapf(err, "failed to connect to %s", driver)
2424
}
25+
if conn == nil {
26+
return errors.Errorf("no database connection returned for driver %q", driver)
27+
}
2528
defer conn.Close()
2629

2730
statements := []string{}
@@ -49,9 +52,13 @@ func UpdateDBSchema(driver string, uri string, schemaDir string) error {
4952
return errors.Wrapf(err, "failed to unmarshal %s", path)
5053
}
5154

52-
schema := driverTableSchema(driver, table.Spec.Schema)
53-
if schema == nil {
54-
return nil
55+
if table.Spec.Name == "" {
56+
return errors.Errorf("table spec %s has no name", path)
57+
}
58+
59+
schema, err := driverTableSchema(driver, table.Spec.Schema)
60+
if err != nil {
61+
return errors.Wrapf(err, "failed to resolve %s schema for %s", driver, path)
5562
}
5663

5764
stmnts, err := conn.PlanTableSchema(table.Spec.Name, schema, nil)
@@ -84,23 +91,23 @@ func connectSchemahero(driver string, uri string) (interfaces.SchemaHeroDatabase
8491
}
8592
}
8693

87-
func driverTableSchema(driver string, schema *schemasv1alpha4.TableSchema) any {
94+
func driverTableSchema(driver string, schema *schemasv1alpha4.TableSchema) (any, error) {
8895
if schema == nil {
89-
return nil
96+
return nil, errors.New("table spec has no schema")
9097
}
9198

9299
switch driver {
93100
case "postgres":
94101
if schema.Postgres == nil {
95-
return nil
102+
return nil, errors.New("table spec has no postgres schema")
96103
}
97-
return schema.Postgres
104+
return schema.Postgres, nil
98105
case "rqlite":
99106
if schema.RQLite == nil {
100-
return nil
107+
return nil, errors.New("table spec has no rqlite schema")
101108
}
102-
return schema.RQLite
109+
return schema.RQLite, nil
103110
default:
104-
return nil
111+
return nil, errors.Errorf("unsupported schemahero driver %q", driver)
105112
}
106113
}

pkg/persistence/schema_test.go

Lines changed: 80 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,80 @@
1+
package persistence
2+
3+
import (
4+
"testing"
5+
6+
schemasv1alpha4 "github.com/schemahero/schemahero/pkg/apis/schemas/v1alpha4"
7+
"github.com/stretchr/testify/assert"
8+
"github.com/stretchr/testify/require"
9+
)
10+
11+
func Test_driverTableSchema(t *testing.T) {
12+
pg := &schemasv1alpha4.PostgresqlTableSchema{}
13+
rq := &schemasv1alpha4.RqliteTableSchema{}
14+
15+
tests := []struct {
16+
name string
17+
driver string
18+
schema *schemasv1alpha4.TableSchema
19+
want any
20+
wantErr string
21+
}{
22+
{
23+
name: "postgres schema present",
24+
driver: "postgres",
25+
schema: &schemasv1alpha4.TableSchema{Postgres: pg},
26+
want: pg,
27+
},
28+
{
29+
name: "rqlite schema present",
30+
driver: "rqlite",
31+
schema: &schemasv1alpha4.TableSchema{RQLite: rq},
32+
want: rq,
33+
},
34+
{
35+
name: "nil schema is an error",
36+
driver: "rqlite",
37+
schema: nil,
38+
wantErr: "no schema",
39+
},
40+
{
41+
name: "postgres driver without a postgres schema is an error",
42+
driver: "postgres",
43+
schema: &schemasv1alpha4.TableSchema{RQLite: rq},
44+
wantErr: "no postgres schema",
45+
},
46+
{
47+
name: "rqlite driver without an rqlite schema is an error",
48+
driver: "rqlite",
49+
schema: &schemasv1alpha4.TableSchema{Postgres: pg},
50+
wantErr: "no rqlite schema",
51+
},
52+
{
53+
name: "unsupported driver is an error",
54+
driver: "mysql",
55+
schema: &schemasv1alpha4.TableSchema{Postgres: pg},
56+
wantErr: "unsupported schemahero driver",
57+
},
58+
}
59+
60+
for _, tt := range tests {
61+
t.Run(tt.name, func(t *testing.T) {
62+
got, err := driverTableSchema(tt.driver, tt.schema)
63+
if tt.wantErr != "" {
64+
require.Error(t, err)
65+
assert.Contains(t, err.Error(), tt.wantErr)
66+
assert.Nil(t, got)
67+
return
68+
}
69+
require.NoError(t, err)
70+
assert.Equal(t, tt.want, got)
71+
})
72+
}
73+
}
74+
75+
func Test_connectSchemahero_unsupportedDriver(t *testing.T) {
76+
conn, err := connectSchemahero("mysql", "")
77+
require.Error(t, err)
78+
assert.Nil(t, conn)
79+
assert.Contains(t, err.Error(), "unsupported schemahero driver")
80+
}

0 commit comments

Comments
 (0)