Skip to content

Commit e0f6664

Browse files
authored
Add check for schema-agnostic sql (kagent-dev#2067)
## Description Adds a static check that migration SQL never hard-codes a schema, and flips that gate in the migrations guide from *Target* to enforced. Static-only — runs in the existing `go test` CI, no database required. ## Changes `go/core/pkg/migrations/cross_track_test.go`: - **`TestSchemaAgnosticSQL`** — rejects `CREATE`/`DROP SCHEMA`, schema-qualified DDL (`CREATE TABLE foo.bar`), `SET search_path`, and `ALTER ... SET SCHEMA` in every migration file. The schema a migration lands in must come from the connection, not the file. - **`TestSchemaViolations`** — table-driven unit test with positive and negative cases. Detection is factored into a `schemaViolations` helper; comment stripping via `stripSQLComments`. --------- Signed-off-by: Jeremy Alvis <jeremy.alvis@solo.io>
1 parent 91c26d0 commit e0f6664

2 files changed

Lines changed: 131 additions & 10 deletions

File tree

.claude/skills/kagent-dev/references/database-migrations.md

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -105,9 +105,9 @@ CREATE TABLE myschema.eval_set (...);
105105
CREATE TABLE IF NOT EXISTS eval_set (...);
106106
```
107107

108-
Schema is a deploy-time choice, fixed by the connection rather than the migration file. A hard-coded schema breaks any deployment that runs the track in a different schema (e.g. a connection that sets `?search_path=<schema>`). The core and vector migrations comply today (verified by inspection until the lint lands).
108+
Schema is a deploy-time choice, fixed by the connection rather than the migration file. A hard-coded schema breaks any deployment that runs the track in a different schema (e.g. a connection that sets `?search_path=<schema>`). The core and vector migrations comply, enforced by `TestSchemaAgnosticSQL`.
109109

110-
> **Enforcement.** A static lint test rejecting the forbidden patterns across all migration files (*Target — not yet enforced*; see [Static Analysis Enforcement](#static-analysis-enforcement)).
110+
> **Enforcement.** `TestSchemaAgnosticSQL` rejects the forbidden patterns across all migration files (see [Static Analysis Enforcement](#static-analysis-enforcement)).
111111
112112
### Idempotency and cross-track safety
113113

@@ -197,7 +197,7 @@ The policies above are enforced by static analysis tests in `go/core/pkg/migrati
197197
| `TestNoCrossTrackDDL` | No track may `ALTER TABLE` or `CREATE INDEX ON` a table owned by another track |
198198
| `TestMigrationGuards` | Up migrations must use `IF NOT EXISTS` on all `CREATE`/`ADD COLUMN`; down migrations must use `IF EXISTS` on all `DROP` statements |
199199
| Contraction guard *(target)* | Blocks undeclared destructive DDL — `DROP`/`RENAME` of shipped objects, type narrowing, new constraints on shipped tables (see [Backward compatibility and contraction](#backward-compatibility-and-contraction)) |
200-
| Schema-agnostic lint *(target)* | Rejects `CREATE SCHEMA`, schema-qualified DDL, `SET search_path`, and `ALTER ... SET SCHEMA` (see [Schema-agnostic SQL](#schema-agnostic-sql)) |
200+
| `TestSchemaAgnosticSQL` | Rejects `CREATE SCHEMA`, schema-qualified DDL, `SET search_path`, and `ALTER ... SET SCHEMA` (see [Schema-agnostic SQL](#schema-agnostic-sql)) |
201201

202202
**Adding a new track**: add the track directory name to the `tracks` slice in each test so the new track is covered by the same checks.
203203

go/core/pkg/migrations/cross_track_test.go

Lines changed: 128 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -85,24 +85,24 @@ func crossTrackViolations(fsys fs.FS, foreignTables map[string]string) ([]violat
8585
return violations, err
8686
}
8787

88-
// guardCheck describes a DDL statement that requires an idempotency guard.
89-
// re captures the first significant word after the keyword; if that word is not
90-
// "if" (case-insensitive) the guard is absent.
91-
type guardCheck struct {
88+
// sqlCheck pairs a name with a regex used by the static migration checks below.
89+
// How re is interpreted depends on the check: the guard checks capture the first
90+
// token after a keyword and require it to be "if"; other checks match on presence.
91+
type sqlCheck struct {
9292
name string
9393
re *regexp.Regexp
9494
}
9595

9696
// upGuardChecks are statements in up migrations that must use IF NOT EXISTS.
97-
var upGuardChecks = []guardCheck{
97+
var upGuardChecks = []sqlCheck{
9898
{"CREATE TABLE", regexp.MustCompile(`(?i)\bCREATE\s+TABLE\s+(\w+)`)},
9999
{"CREATE INDEX", regexp.MustCompile(`(?i)\bCREATE\s+(?:UNIQUE\s+)?INDEX\s+(?:CONCURRENTLY\s+)?(\w+)`)},
100100
{"CREATE EXTENSION", regexp.MustCompile(`(?i)\bCREATE\s+EXTENSION\s+(\w+)`)},
101101
{"ADD COLUMN", regexp.MustCompile(`(?i)\bADD\s+COLUMN\s+(\w+)`)},
102102
}
103103

104104
// downGuardChecks are statements in down migrations that must use IF EXISTS.
105-
var downGuardChecks = []guardCheck{
105+
var downGuardChecks = []sqlCheck{
106106
{"DROP TABLE", regexp.MustCompile(`(?i)\bDROP\s+TABLE\s+(\w+)`)},
107107
{"DROP INDEX", regexp.MustCompile(`(?i)\bDROP\s+INDEX\s+(\w+)`)},
108108
{"DROP EXTENSION", regexp.MustCompile(`(?i)\bDROP\s+EXTENSION\s+(\w+)`)},
@@ -129,7 +129,7 @@ func TestMigrationGuards(t *testing.T) {
129129
return err
130130
}
131131

132-
var checks []guardCheck
132+
var checks []sqlCheck
133133
switch {
134134
case strings.HasSuffix(path, ".up.sql"):
135135
checks = upGuardChecks
@@ -212,3 +212,124 @@ func TestNoCrossTrackDDL(t *testing.T) {
212212
}
213213
}
214214
}
215+
216+
// stripSQLComments removes `--` line comments so the static checks below match
217+
// real statements, not commented-out or explanatory SQL.
218+
func stripSQLComments(s string) string {
219+
var b strings.Builder
220+
for line := range strings.SplitSeq(s, "\n") {
221+
if i := strings.Index(line, "--"); i >= 0 {
222+
line = line[:i]
223+
}
224+
b.WriteString(line)
225+
b.WriteByte('\n')
226+
}
227+
return b.String()
228+
}
229+
230+
// --- Schema-agnostic SQL ---
231+
//
232+
// Migration SQL must not name a schema: the schema a migration lands in is
233+
// chosen by the connection (search_path), not the file, so the same files apply
234+
// into whatever schema the connection selects. See database-migrations.md,
235+
// "Schema-agnostic SQL". Static check over every migration file — no database.
236+
237+
var schemaQualifiedChecks = []sqlCheck{
238+
{"CREATE SCHEMA", regexp.MustCompile(`(?i)\bCREATE\s+SCHEMA\b`)},
239+
{"DROP SCHEMA", regexp.MustCompile(`(?i)\bDROP\s+SCHEMA\b`)},
240+
{"search_path", regexp.MustCompile(`(?i)\bsearch_path\b`)},
241+
{"SET SCHEMA", regexp.MustCompile(`(?i)\bSET\s+SCHEMA\b`)},
242+
{"schema-qualified table", regexp.MustCompile(`(?i)\b(?:CREATE|ALTER|DROP)\s+TABLE\s+(?:IF\s+(?:NOT\s+)?EXISTS\s+)?\w+\.\w+`)},
243+
{"schema-qualified index target", regexp.MustCompile(`(?i)\bON\s+\w+\.\w+`)},
244+
{"schema-qualified reference", regexp.MustCompile(`(?i)\bREFERENCES\s+\w+\.\w+`)},
245+
}
246+
247+
// schemaViolations returns the names of schema-qualified patterns found in SQL.
248+
func schemaViolations(sql string) []string {
249+
content := stripSQLComments(sql)
250+
var found []string
251+
for _, c := range schemaQualifiedChecks {
252+
if c.re.MatchString(content) {
253+
found = append(found, c.name)
254+
}
255+
}
256+
return found
257+
}
258+
259+
// TestSchemaAgnosticSQL rejects any schema name in migration SQL. The connection
260+
// selects the schema; a hard-coded one breaks any deployment that runs the track
261+
// in a different schema.
262+
func TestSchemaAgnosticSQL(t *testing.T) {
263+
tracks := []string{"core", "vector"}
264+
for _, track := range tracks {
265+
sub, err := fs.Sub(migrations.FS, track)
266+
if err != nil {
267+
t.Fatalf("fs.Sub(%q): %v", track, err)
268+
}
269+
err = fs.WalkDir(sub, ".", func(path string, d fs.DirEntry, err error) error {
270+
if err != nil || d.IsDir() || !strings.HasSuffix(path, ".sql") {
271+
return err
272+
}
273+
data, err := fs.ReadFile(sub, path)
274+
if err != nil {
275+
return err
276+
}
277+
for _, v := range schemaViolations(string(data)) {
278+
t.Errorf(
279+
"schema reference in %s/%s: %s; migrations must be schema-agnostic (the connection selects the schema)",
280+
track, path, v,
281+
)
282+
}
283+
return nil
284+
})
285+
if err != nil {
286+
t.Fatalf("WalkDir(%q): %v", track, err)
287+
}
288+
}
289+
}
290+
291+
func TestSchemaViolations(t *testing.T) {
292+
tests := []struct {
293+
name string
294+
sql string
295+
want []string
296+
}{
297+
{"unqualified table", `CREATE TABLE IF NOT EXISTS eval_set (id TEXT);`, nil},
298+
{"unqualified index", `CREATE INDEX IF NOT EXISTS i ON eval_set(id);`, nil},
299+
{"schema in comment", `-- create table myschema.foo here` + "\n" + `CREATE TABLE IF NOT EXISTS foo (id TEXT);`, nil},
300+
{"qualified table", `CREATE TABLE myschema.eval_set (id TEXT);`, []string{"schema-qualified table"}},
301+
{"create schema", `CREATE SCHEMA IF NOT EXISTS myschema;`, []string{"CREATE SCHEMA"}},
302+
{"set search_path", `SET search_path TO myschema;`, []string{"search_path"}},
303+
{"set schema", `ALTER TABLE foo SET SCHEMA myschema;`, []string{"SET SCHEMA"}},
304+
{"qualified index target", `CREATE INDEX i ON myschema.foo(id);`, []string{"schema-qualified index target"}},
305+
{"qualified reference", `ALTER TABLE foo ADD CONSTRAINT fk FOREIGN KEY (b) REFERENCES myschema.bar(id);`, []string{"schema-qualified reference"}},
306+
}
307+
for _, tt := range tests {
308+
t.Run(tt.name, func(t *testing.T) {
309+
got := schemaViolations(tt.sql)
310+
if !equalStringSets(got, tt.want) {
311+
t.Errorf("schemaViolations() = %v, want %v", got, tt.want)
312+
}
313+
})
314+
}
315+
}
316+
317+
// equalStringSets compares two string slices ignoring order and duplicates.
318+
func equalStringSets(a, b []string) bool {
319+
sa, sb := make(map[string]bool), make(map[string]bool)
320+
for _, s := range a {
321+
sa[s] = true
322+
}
323+
for _, s := range b {
324+
sb[s] = true
325+
}
326+
if len(sa) != len(sb) {
327+
return false
328+
}
329+
for s := range sa {
330+
if !sb[s] {
331+
return false
332+
}
333+
}
334+
return true
335+
}

0 commit comments

Comments
 (0)