From 32cce7ced377e01c70a47ba67f44fb4ae8899d9d Mon Sep 17 00:00:00 2001 From: avison9 Date: Sat, 26 Sep 2026 21:13:19 +0100 Subject: [PATCH 1/2] --disable leaves rules out of the output and the exit code, and says what it left out A repository that has read its inventory of uncaptured columns does not want it on every run: RefuseRadar prints 161 source-column-not-captured lines and set 2026-10-21 to decide whether they stay. --disable RULE (comma-separated or repeated) drops a rule's findings from the output and from the exit code. A filter must not make a run look cleaner than it was, so the text output ends with what was left out ("not shown (--disable): source-column-not-captured 161"), a run whose every finding was hidden says "ok: nothing to report outside the disabled rules" rather than that the files agree, and --format json keeps its array shape and puts the same note on stderr. An unknown rule name is an error listing the rules, not a filter that silently hides nothing; engine.Rules is that list, and the corpus test fails if any finding's rule is missing from it. Disabling schema-before-connector skips the base altogether, as if --base were not given. Filtering its findings afterwards would also lose the columns it raised, because source-column-not-captured leaves raised columns out of its inventory, so they would appear nowhere. The action gains a disable input. run() now takes its output streams, so the tests drive the real command line: hiding a rule, a disabled error no longer failing the run, commas and repeats, an unknown name, the base skipped, and JSON's stderr note. --- README.md | 9 ++++ action.yml | 11 +++++ cmd/cdclint/corpus_test.go | 8 +++- cmd/cdclint/main.go | 83 +++++++++++++++++++++++++++++++---- cmd/cdclint/main_test.go | 90 ++++++++++++++++++++++++++++++++++++++ internal/engine/rules.go | 28 ++++++++++++ 6 files changed, 219 insertions(+), 10 deletions(-) create mode 100644 cmd/cdclint/main_test.go create mode 100644 internal/engine/rules.go diff --git a/README.md b/README.md index 192a350..7296bf6 100644 --- a/README.md +++ b/README.md @@ -176,6 +176,15 @@ on every pull request by default, against the base branch's tip. Paths on the command line are relative to the current directory, in the working tree and at the base alike, so it runs from a subdirectory of a monorepo. +`--disable RULE[,RULE]` leaves rules out: their findings are not shown and do +not fail the run, and the output ends with what was left out, for example +`not shown (--disable): source-column-not-captured 161`, so a filtered run +never reads as a clean one. `--disable source-column-not-captured` is the usual +one, for a repository that has read its inventory of uncaptured columns and +does not want it on every run. Disabling `schema-before-connector` is the same +as leaving out `--base`. An unknown rule name is an error, not a filter that +hides nothing. + Files in, findings out, non-zero exit. No database, no daemon, no credentials. Under a second on a laptop. `--fail-on warning` or `info` raises the bar; `--format json` is for anything that wants to post findings somewhere. A diff --git a/action.yml b/action.yml index 0e9b284..f7cd0cf 100644 --- a/action.yml +++ b/action.yml @@ -35,6 +35,13 @@ inputs: description: "Exit non-zero at this severity or above: error, warning or info." required: false default: error + disable: + description: >- + Rules to leave out, comma-separated, such as source-column-not-captured. + Their findings are not shown and do not fail the step; the log says how + many were left out. An unknown rule name fails the step. + required: false + default: "" version: description: cdclint release to run, as a tag such as v0.1.0; latest by default. required: false @@ -121,11 +128,15 @@ runs: SINKS: ${{ inputs.sink }} SINK_CONNECTORS: ${{ inputs.sink-connector }} FAIL_ON: ${{ inputs.fail-on }} + DISABLE: ${{ inputs.disable }} BASE: ${{ inputs.base }} PR_BASE_SHA: ${{ github.event.pull_request.base.sha }} run: | set -uo pipefail args=(--migrations "$MIGRATIONS" --connector "$CONNECTOR" --fail-on "$FAIL_ON") + if [ -n "$DISABLE" ]; then + args+=(--disable "$DISABLE") + fi # The diff rule needs the base commit's objects, and # actions/checkout fetches one commit by default. Whatever form the # base was given in (main, origin/main, a tag, a commit id), it is diff --git a/cmd/cdclint/corpus_test.go b/cmd/cdclint/corpus_test.go index c2e76d9..174aa63 100644 --- a/cmd/cdclint/corpus_test.go +++ b/cmd/cdclint/corpus_test.go @@ -82,7 +82,13 @@ func TestCorpus(t *testing.T) { if _, err := os.Stat(filepath.Join(name, "base")); err == nil { in.Base = baseFromDir(t, filepath.Join(name, "base"), filepath.Join(name, "connector.json")) } - got := Render(engine.Run(in)) + findings := engine.Run(in) + for _, f := range findings { + if !engine.KnownRule(f.Rule) { + t.Errorf("finding from rule %q, which engine.Rules does not list; --disable could not name it", f.Rule) + } + } + got := Render(findings) expectedPath := filepath.Join(name, "expected.txt") if *update { if err := os.WriteFile(expectedPath, []byte(got), 0o644); err != nil { diff --git a/cmd/cdclint/main.go b/cmd/cdclint/main.go index b2ee952..7dba74a 100644 --- a/cmd/cdclint/main.go +++ b/cmd/cdclint/main.go @@ -14,6 +14,7 @@ import ( "encoding/json" "flag" "fmt" + "io" "os" "strings" @@ -36,16 +37,18 @@ func (m *multi) String() string { return strings.Join(*m, ",") } func (m *multi) Set(v string) error { *m = append(*m, v); return nil } func main() { - os.Exit(run(os.Args[1:])) + os.Exit(run(os.Args[1:], os.Stdout, os.Stderr)) } -func run(args []string) int { +func run(args []string, stdout, stderr io.Writer) int { fs := flag.NewFlagSet("cdclint", flag.ContinueOnError) + fs.SetOutput(stderr) var ( migrations = fs.String("migrations", "", "directory of source migrations, applied in name order; Postgres or MySQL, from the connector class, or say it with postgres:DIR or mysql:DIR") connector = fs.String("connector", "", "Debezium source connector JSON") sinks multi sinkConns multi + disable multi format = fs.String("format", "text", "output format: text or json") minSev = fs.String("fail-on", "error", "exit non-zero at this severity or above: error, warning, info") base = fs.String("base", "", "git ref of the change's base (a branch, a commit, origin/main); enables schema-before-connector, which judges the diff") @@ -53,6 +56,8 @@ func run(args []string) int { ) fs.Var(&sinks, "sink", "sink DDL directory as [dialect:]DIR; dialect is clickhouse (default), bigquery, snowflake or iceberg; repeatable") fs.Var(&sinkConns, "sink-connector", "Kafka Connect sink connector JSON; repeatable") + fs.Var(&disable, "disable", "rule names to leave out, comma-separated or repeated: "+strings.Join(engine.Rules, ", ")+ + "; their findings are not shown and do not fail the run, and the output says how many were left out") fs.Usage = func() { fmt.Fprintln(fs.Output(), "usage: cdclint --migrations DIR --connector FILE --sink [dialect:]DIR [--sink-connector FILE]...") fs.PrintDefaults() @@ -61,30 +66,47 @@ func run(args []string) int { return 2 } if *showVer || (fs.NArg() > 0 && fs.Arg(0) == "version") { - fmt.Println("cdclint", version) + fmt.Fprintln(stdout, "cdclint", version) return 0 } + disabled := map[string]bool{} + for _, v := range disable { + for _, name := range strings.Split(v, ",") { + if name = strings.TrimSpace(name); name == "" { + continue + } + if !engine.KnownRule(name) { + fmt.Fprintf(stderr, "cdclint: --disable: unknown rule %q; the rules are %s\n", name, strings.Join(engine.Rules, ", ")) + return 2 + } + disabled[name] = true + } + } if *migrations == "" || *connector == "" || len(sinks) == 0 { fs.Usage() return 2 } in, err := load(*migrations, *connector, sinks, sinkConns) if err != nil { - fmt.Fprintln(os.Stderr, "cdclint:", err) + fmt.Fprintln(stderr, "cdclint:", err) return 2 } - if *base != "" { + // Disabling the diff rule is the same as not giving --base: filtering + // its findings afterwards would also drop the columns it raised, which + // source-column-not-captured then leaves out of its list, so they + // would appear nowhere. + if *base != "" && !disabled["schema-before-connector"] { b, err := LoadBase(*base, *migrations, *connector) if err != nil { - fmt.Fprintln(os.Stderr, "cdclint:", err) + fmt.Fprintln(stderr, "cdclint:", err) return 2 } in.Base = b } - findings := engine.Run(in) + findings, hidden := without(engine.Run(in), disabled) switch *format { case "json": - enc := json.NewEncoder(os.Stdout) + enc := json.NewEncoder(stdout) enc.SetIndent("", " ") type out struct { Rule string `json:"rule"` @@ -102,8 +124,19 @@ func run(args []string) int { rows = []out{} } _ = enc.Encode(rows) + // The array's shape is what consumers parse, so the note goes + // to stderr rather than into it. + if note := hiddenNote(hidden); note != "" { + fmt.Fprint(stderr, "cdclint: "+note) + } default: - os.Stdout.WriteString(Render(findings)) + if len(findings) == 0 && len(hidden) > 0 { + // "Agree" would claim more than was checked. + fmt.Fprintln(stdout, "ok: nothing to report outside the disabled rules") + } else { + fmt.Fprint(stdout, Render(findings)) + } + fmt.Fprint(stdout, hiddenNote(hidden)) } threshold := model.Error switch *minSev { @@ -120,6 +153,38 @@ func run(args []string) int { return 0 } +// without removes the findings of disabled rules and counts them by rule. +func without(findings []model.Finding, disabled map[string]bool) ([]model.Finding, map[string]int) { + if len(disabled) == 0 { + return findings, nil + } + var kept []model.Finding + hidden := map[string]int{} + for _, f := range findings { + if disabled[f.Rule] { + hidden[f.Rule]++ + continue + } + kept = append(kept, f) + } + return kept, hidden +} + +// hiddenNote says what --disable left out, so a filtered run never reads as +// a clean one. It is empty when nothing was left out. +func hiddenNote(hidden map[string]int) string { + if len(hidden) == 0 { + return "" + } + var parts []string + for _, r := range engine.Rules { + if n := hidden[r]; n > 0 { + parts = append(parts, fmt.Sprintf("%s %d", r, n)) + } + } + return "not shown (--disable): " + strings.Join(parts, ", ") + "\n" +} + // Render is the text output: findings, then a one-line summary. func Render(findings []model.Finding) string { var b strings.Builder diff --git a/cmd/cdclint/main_test.go b/cmd/cdclint/main_test.go new file mode 100644 index 0000000..d7055b3 --- /dev/null +++ b/cmd/cdclint/main_test.go @@ -0,0 +1,90 @@ +package main + +import ( + "bytes" + "strings" + "testing" +) + +func cli(t *testing.T, args ...string) (int, string, string) { + t.Helper() + var out, errOut bytes.Buffer + code := run(args, &out, &errOut) + return code, out.String(), errOut.String() +} + +func entry(name string) []string { + d := "../../corpus/" + name + return []string{"--migrations", d + "/migrations", "--connector", d + "/connector.json", "--sink", d + "/sink"} +} + +func TestDisableHidesARuleAndSaysSo(t *testing.T) { + // column-never-captured reports two source-column-not-captured infos + // and nothing else. + code, out, _ := cli(t, append(entry("column-never-captured"), "--disable", "source-column-not-captured")...) + want := "ok: nothing to report outside the disabled rules\n" + + "not shown (--disable): source-column-not-captured 2\n" + if code != 0 || out != want { + t.Fatalf("exit %d, output:\n%s\nwant:\n%s", code, out, want) + } +} + +func TestADisabledRuleDoesNotFailTheRun(t *testing.T) { + // include-list-typo: one sink-column-not-captured error and one + // captured-column-missing warning; exit 1 at the default --fail-on error. + if code, _, _ := cli(t, entry("include-list-typo")...); code != 1 { + t.Fatalf("without --disable: exit %d, want 1", code) + } + code, out, _ := cli(t, append(entry("include-list-typo"), "--disable", "sink-column-not-captured")...) + if code != 0 { + t.Errorf("exit %d, want 0: the only error was disabled", code) + } + if !strings.Contains(out, "warning captured-column-missing") || strings.Contains(out, "sink-column-not-captured include-list") { + t.Errorf("output:\n%s", out) + } + if !strings.HasSuffix(out, "0 error(s), 1 warning(s), 0 info\nnot shown (--disable): sink-column-not-captured 1\n") { + t.Errorf("summary:\n%s", out) + } +} + +func TestDisableTakesCommasAndRepeats(t *testing.T) { + a, outA, _ := cli(t, append(entry("include-list-typo"), "--disable", "sink-column-not-captured,captured-column-missing")...) + b, outB, _ := cli(t, append(entry("include-list-typo"), "--disable", "sink-column-not-captured", "--disable", " captured-column-missing ")...) + want := "ok: nothing to report outside the disabled rules\n" + + "not shown (--disable): sink-column-not-captured 1, captured-column-missing 1\n" + if a != 0 || b != 0 || outA != want || outB != want { + t.Fatalf("commas: exit %d\n%s\nrepeats: exit %d\n%s\nwant:\n%s", a, outA, b, outB, want) + } +} + +func TestAnUnknownRuleIsAnErrorNotASilentFilter(t *testing.T) { + code, out, errOut := cli(t, append(entry("include-list-typo"), "--disable", "source-column-not-capturd")...) + if code != 2 || out != "" { + t.Fatalf("exit %d, stdout %q", code, out) + } + if !strings.Contains(errOut, `unknown rule "source-column-not-capturd"`) || !strings.Contains(errOut, "source-column-not-captured") { + t.Errorf("stderr: %s", errOut) + } +} + +func TestDisablingTheDiffRuleSkipsTheBase(t *testing.T) { + // A ref that does not exist fails the base load; with the diff rule + // disabled the base is never read, exactly as without --base. + if code, _, _ := cli(t, append(entry("clean"), "--base", "no-such-ref-cdclint")...); code != 2 { + t.Fatalf("the base should have been read and failed: exit %d", code) + } + code, out, errOut := cli(t, append(entry("clean"), "--base", "no-such-ref-cdclint", "--disable", "schema-before-connector")...) + if code != 0 || out != "ok: source, connector and sink agree\n" { + t.Fatalf("exit %d, stdout %q, stderr %q", code, out, errOut) + } +} + +func TestJSONKeepsItsShapeAndNotesOnStderr(t *testing.T) { + code, out, errOut := cli(t, append(entry("include-list-typo"), "--format", "json", "--disable", "sink-column-not-captured")...) + if code != 0 || !strings.HasPrefix(strings.TrimSpace(out), "[") || strings.Contains(out, "sink-column-not-captured") { + t.Fatalf("exit %d, stdout:\n%s", code, out) + } + if errOut != "cdclint: not shown (--disable): sink-column-not-captured 1\n" { + t.Errorf("stderr %q", errOut) + } +} diff --git a/internal/engine/rules.go b/internal/engine/rules.go new file mode 100644 index 0000000..bbc5829 --- /dev/null +++ b/internal/engine/rules.go @@ -0,0 +1,28 @@ +package engine + +// Rules names every rule Run can report, in the README's order. --disable +// checks names against it, so a typo is an error rather than a filter that +// silently hides nothing, and the corpus test checks that every finding's +// rule is here, so a new rule cannot be missed. +var Rules = []string{ + "sink-column-not-captured", + "sink-table-not-captured", + "sink-column-unknown", + "source-column-not-captured", + "captured-column-missing", + "captured-table-missing", + "topic-table-mapping", + "sink-column-flattened", + "mv-column-match", + "schema-before-connector", +} + +// KnownRule reports whether name is one of Rules. +func KnownRule(name string) bool { + for _, r := range Rules { + if r == name { + return true + } + } + return false +} From 35e19f5e0d3c004467a32310bc1977c099699b0f Mon Sep 17 00:00:00 2001 From: avison9 Date: Sat, 26 Sep 2026 21:21:50 +0100 Subject: [PATCH 2/2] A column an ALTER TABLE adds is placed on its own line, not on the ALTER's In a multi-line ALTER TABLE every added column was positioned at the statement's first line, so three columns added by one migration were reported at the same line (RefuseRadar #963's three reports columns all at :6). The readers parse actions with their whitespace folded, so an action cannot be found in the file verbatim; each added column's name is now searched for in the statement text, as a whole word, after the table's name and after the previous action, and the column takes that line. When the name cannot be found the statement's line stays, as before. This is also what lets a marker on the line that adds a column refer to that column's finding. Two corpus entries change, line numbers only: diff-connector-captures- one-of-two (:5 to :7, movement_cleared_by's own line) and diff-connector-touched-other-table (three findings at :6 become :7, :8, :9, now in file order instead of message order). Every message is unchanged. --- .../expected.txt | 2 +- .../expected.txt | 12 +++---- internal/ddl/ddl.go | 31 +++++++++++++++++++ internal/source/mysql/mysql.go | 21 ++++++++++--- internal/source/mysql/mysql_test.go | 20 ++++++++++++ internal/source/postgres/postgres.go | 14 +++++++-- internal/source/postgres/postgres_test.go | 20 ++++++++++++ 7 files changed, 106 insertions(+), 14 deletions(-) diff --git a/corpus/diff-connector-captures-one-of-two/expected.txt b/corpus/diff-connector-captures-one-of-two/expected.txt index 746b794..8701d54 100644 --- a/corpus/diff-connector-captures-one-of-two/expected.txt +++ b/corpus/diff-connector-captures-one-of-two/expected.txt @@ -1,4 +1,4 @@ -warning schema-before-connector diff-connector-captures-one-of-two/migrations/0141_movement_doubt_cleared.sql:5 +warning schema-before-connector diff-connector-captures-one-of-two/migrations/0141_movement_doubt_cleared.sql:7 this change adds public.reports.movement_cleared_by to a captured table without adding it to column.include.list in diff-connector-captures-one-of-two/connector.json (compared with base) the column will not be in the stream; if a sink is later given it, every row will be the default until a snapshot fix: add public.reports.movement_cleared_by to column.include.list in the same change, or leave it off on purpose and let this warning stand as the record of that (it blocks only under --fail-on warning) diff --git a/corpus/diff-connector-touched-other-table/expected.txt b/corpus/diff-connector-touched-other-table/expected.txt index 3fe2566..3b060ee 100644 --- a/corpus/diff-connector-touched-other-table/expected.txt +++ b/corpus/diff-connector-touched-other-table/expected.txt @@ -1,13 +1,13 @@ -warning schema-before-connector diff-connector-touched-other-table/migrations/0141_movement_doubt_cleared.sql:6 - this change adds public.reports.movement_clear_reason to a captured table without adding it to column.include.list in diff-connector-touched-other-table/connector.json (compared with base) - the column will not be in the stream; if a sink is later given it, every row will be the default until a snapshot - fix: add public.reports.movement_clear_reason to column.include.list in the same change, or leave it off on purpose and let this warning stand as the record of that (it blocks only under --fail-on warning) -warning schema-before-connector diff-connector-touched-other-table/migrations/0141_movement_doubt_cleared.sql:6 +warning schema-before-connector diff-connector-touched-other-table/migrations/0141_movement_doubt_cleared.sql:7 this change adds public.reports.movement_cleared_at to a captured table without adding it to column.include.list in diff-connector-touched-other-table/connector.json (compared with base) the column will not be in the stream; if a sink is later given it, every row will be the default until a snapshot fix: add public.reports.movement_cleared_at to column.include.list in the same change, or leave it off on purpose and let this warning stand as the record of that (it blocks only under --fail-on warning) -warning schema-before-connector diff-connector-touched-other-table/migrations/0141_movement_doubt_cleared.sql:6 +warning schema-before-connector diff-connector-touched-other-table/migrations/0141_movement_doubt_cleared.sql:8 this change adds public.reports.movement_cleared_by to a captured table without adding it to column.include.list in diff-connector-touched-other-table/connector.json (compared with base) the column will not be in the stream; if a sink is later given it, every row will be the default until a snapshot fix: add public.reports.movement_cleared_by to column.include.list in the same change, or leave it off on purpose and let this warning stand as the record of that (it blocks only under --fail-on warning) +warning schema-before-connector diff-connector-touched-other-table/migrations/0141_movement_doubt_cleared.sql:9 + this change adds public.reports.movement_clear_reason to a captured table without adding it to column.include.list in diff-connector-touched-other-table/connector.json (compared with base) + the column will not be in the stream; if a sink is later given it, every row will be the default until a snapshot + fix: add public.reports.movement_clear_reason to column.include.list in the same change, or leave it off on purpose and let this warning stand as the record of that (it blocks only under --fail-on warning) 0 error(s), 3 warning(s), 0 info diff --git a/internal/ddl/ddl.go b/internal/ddl/ddl.go index a183639..c679193 100644 --- a/internal/ddl/ddl.go +++ b/internal/ddl/ddl.go @@ -200,3 +200,34 @@ func ItemLine(text, item string, cursor *int) int { *cursor = at + len(item) return 1 + strings.Count(text[:at], "\n") } + +// NameLine returns the 1-based line, counted from the start of text, of the +// first occurrence of the identifier name at or after *cursor, quoted or not, +// as a whole word; cursor is advanced past it so the next search starts +// there. It returns 0, leaving cursor alone, when name is not found. The +// readers use it to place each column an ALTER TABLE adds on its own line, +// since the actions they parse have had their whitespace folded. +func NameLine(text, name string, cursor *int) int { + lower := strings.ToLower(text) + target := strings.ToLower(name) + for from := *cursor; from < len(lower); { + at := strings.Index(lower[from:], target) + if at < 0 { + return 0 + } + at += from + end := at + len(target) + before := at == 0 || !identChar(lower[at-1]) + after := end >= len(lower) || !identChar(lower[end]) + if before && after { + *cursor = end + return 1 + strings.Count(text[:at], "\n") + } + from = at + 1 + } + return 0 +} + +func identChar(c byte) bool { + return c == '_' || c == '$' || (c >= 'a' && c <= 'z') || (c >= '0' && c <= '9') +} diff --git a/internal/source/mysql/mysql.go b/internal/source/mysql/mysql.go index afdcd2a..fea9bd2 100644 --- a/internal/source/mysql/mysql.go +++ b/internal/source/mysql/mysql.go @@ -98,7 +98,7 @@ func (r *reader) statement(file string, st sqlsplit.Statement) error { case ddl.HasPrefixFold(w, "CREATE", "TABLE"): err = r.createTable(body, w, pos) case ddl.HasPrefixFold(w, "ALTER") && tableKeyword(w) > 0: - err = r.alterTable(w, tableKeyword(w), pos) + err = r.alterTable(body, w, tableKeyword(w), pos) case ddl.HasPrefixFold(w, "RENAME", "TABLE"): err = r.renameTables(w[2:]) case ddl.HasPrefixFold(w, "DROP", "TABLE"), ddl.HasPrefixFold(w, "DROP", "TEMPORARY", "TABLE"): @@ -330,7 +330,7 @@ func typeWords(w []string) []string { return w } -func (r *reader) alterTable(w []string, at int, pos model.Pos) error { +func (r *reader) alterTable(text string, w []string, at int, pos model.Pos) error { if at+1 >= len(w) { return nil } @@ -342,6 +342,10 @@ func (r *reader) alterTable(w []string, at int, pos model.Pos) error { if t == nil { return nil } + // Each added column is placed on its own line: search the statement for + // its name, after the table's name and after the previous action. + cursor := 0 + ddl.NameLine(text, table, &cursor) // Actions are comma-separated after the name; each starts with a verb. for _, a := range ddl.SplitTop(strings.Join(w[at+2:], " ")) { aw := ddl.Words(a) @@ -372,12 +376,12 @@ func (r *reader) alterTable(w []string, at int, pos model.Pos) error { inner, _, _ := ddl.Body(aw[j]) for _, def := range ddl.SplitTop(inner) { if dw := ddl.Words(def); len(dw) > 0 && !isKey(dw) { - addColumn(t, dw, pos) + addColumn(t, dw, linePos(text, dw[0], pos, &cursor)) } } continue } - addColumn(t, aw[j:], pos) + addColumn(t, aw[j:], linePos(text, aw[j], pos, &cursor)) case "DROP": if j == 1 && (isKey(aw[1:]) || strings.EqualFold(aw[1], "PARTITION") || strings.EqualFold(aw[1], "DEFAULT")) { continue @@ -425,6 +429,15 @@ func (r *reader) alterTable(w []string, at int, pos model.Pos) error { return nil } +// linePos is pos moved to the line in text where the column name next +// appears, or pos itself when it cannot be found. +func linePos(text, name string, pos model.Pos, cursor *int) model.Pos { + if line := ddl.NameLine(text, ddl.Unquote(name), cursor); line > 0 { + pos.Line += line - 1 + } + return pos +} + // addColumn adds one column definition, honouring FIRST and AFTER. func addColumn(t *model.Table, dw []string, pos model.Pos) { name := ddl.Unquote(dw[0]) diff --git a/internal/source/mysql/mysql_test.go b/internal/source/mysql/mysql_test.go index 90b44f7..6de74c2 100644 --- a/internal/source/mysql/mysql_test.go +++ b/internal/source/mysql/mysql_test.go @@ -116,6 +116,26 @@ func TestDownMigrationsAreLeftOut(t *testing.T) { } } +func TestColumnsAnAlterAddsAreOnTheirOwnLines(t *testing.T) { + text := "CREATE TABLE orders (id INT);\n" + + "ALTER TABLE `orders`\n" + + " ADD COLUMN `currency` CHAR(3) AFTER id,\n" + + " ADD (\n" + + " paid_at DATETIME,\n" + + " orders_total DECIMAL(12,2)\n" + + " );\n" + src, err := ReadFiles([]source.NamedFile{{Path: "V2.sql", Text: text}}, "shop") + if err != nil { + t.Fatal(err) + } + o := src.Table("shop", "orders") + for name, want := range map[string]int{"currency": 3, "paid_at": 5, "orders_total": 6} { + if c := o.Column(name); c == nil || c.Pos.Line != want { + t.Errorf("%s at %+v, want line %d", name, c, want) + } + } +} + func TestAnUnqualifiedTableWithNoDatabaseIsAnError(t *testing.T) { _, err := ReadFiles([]source.NamedFile{{Path: "V1.sql", Text: "CREATE TABLE t (id INT);"}}, "") if err == nil || !strings.Contains(err.Error(), "database.include.list") { diff --git a/internal/source/postgres/postgres.go b/internal/source/postgres/postgres.go index 683e357..a21e83e 100644 --- a/internal/source/postgres/postgres.go +++ b/internal/source/postgres/postgres.go @@ -61,7 +61,7 @@ func Apply(src *model.Source, file, text string) error { case ddl.HasPrefixFold(w, "CREATE", "TABLE"), ddl.HasPrefixFold(w, "CREATE", "UNLOGGED", "TABLE"): createTable(src, st.Text, pos) case ddl.HasPrefixFold(w, "ALTER", "TABLE"): - alterTable(src, w, pos) + alterTable(src, st.Text, w, pos) case ddl.HasPrefixFold(w, "DROP", "TABLE"): dropTable(src, w) } @@ -174,7 +174,7 @@ func containsFold(w []string, kw string) bool { return false } -func alterTable(src *model.Source, w []string, pos model.Pos) { +func alterTable(src *model.Source, text string, w []string, pos model.Pos) { i := 2 for i < len(w) && (strings.EqualFold(w[i], "ONLY") || strings.EqualFold(w[i], "IF") || strings.EqualFold(w[i], "EXISTS")) { i++ @@ -189,6 +189,10 @@ func alterTable(src *model.Source, w []string, pos model.Pos) { } // Actions are comma-separated after the name; each starts with a verb. actions := ddl.SplitTop(strings.Join(w[i+1:], " ")) + // Each added column is placed on its own line: search the statement for + // its name, after the table's name and after the previous action. + cursor := 0 + ddl.NameLine(text, table, &cursor) for _, a := range actions { aw := ddl.Words(a) if len(aw) == 0 { @@ -208,7 +212,11 @@ func alterTable(src *model.Source, w []string, pos model.Pos) { } name := ddl.Unquote(aw[j]) if t.Column(name) == nil { - t.Columns = append(t.Columns, model.Column{Name: name, Type: strings.Join(typeWords(aw[j+1:]), " "), Pos: pos}) + at := pos + if line := ddl.NameLine(text, name, &cursor); line > 0 { + at.Line = pos.Line + line - 1 + } + t.Columns = append(t.Columns, model.Column{Name: name, Type: strings.Join(typeWords(aw[j+1:]), " "), Pos: at}) } case ddl.HasPrefixFold(aw, "DROP", "COLUMN"), ddl.HasPrefixFold(aw, "DROP") && len(aw) >= 2 && !strings.EqualFold(aw[1], "CONSTRAINT"): j := 1 diff --git a/internal/source/postgres/postgres_test.go b/internal/source/postgres/postgres_test.go index 731fb7c..dd2994b 100644 --- a/internal/source/postgres/postgres_test.go +++ b/internal/source/postgres/postgres_test.go @@ -83,3 +83,23 @@ func TestANameGluedToAMultiLineColumnListDoesNotPanic(t *testing.T) { t.Errorf("postid line = %d, want 3", r.Columns[1].Pos.Line) } } + +func TestColumnsAnAlterAddsAreOnTheirOwnLines(t *testing.T) { + src := &model.Source{} + text := "CREATE TABLE reports (id UUID);\n" + + "ALTER TABLE reports\n" + + " ADD COLUMN IF NOT EXISTS cleared_at TIMESTAMPTZ,\n" + + " -- a comment between actions\n" + + " ADD COLUMN cleared_by UUID REFERENCES users(id),\n" + + " ADD COLUMN reports_note TEXT;\n" + + "ALTER TABLE reports ADD COLUMN one_line INT;\n" + if err := Apply(src, "0141.sql", text); err != nil { + t.Fatal(err) + } + r := src.Table("public", "reports") + for name, want := range map[string]int{"cleared_at": 3, "cleared_by": 5, "reports_note": 6, "one_line": 7} { + if c := r.Column(name); c == nil || c.Pos.Line != want { + t.Errorf("%s at %+v, want line %d", name, c, want) + } + } +}