diff --git a/README.md b/README.md index 192a350..bc1d450 100644 --- a/README.md +++ b/README.md @@ -176,6 +176,39 @@ 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. + +### Recording a decision: `cdclint:ignore` + +Some findings are decisions, not mistakes: a column holding PII is left off +the include list on purpose. Say so in a comment on the line the finding +points at, with the reason: + +```sql +ALTER TABLE users + ADD COLUMN ssn_hash TEXT, -- cdclint:ignore schema-before-connector: PII, never streamed + ADD COLUMN nickname TEXT; +``` + +The finding no longer fails the run and is listed as acknowledged, with the +reason; `nickname`, on the next line, is still raised. A marker after code +covers its own line only; a marker on a line of its own covers the line +below. It works in migrations and in sink DDL, with `--` or MySQL's `#`, and +names one or more rules separated by commas. The reason after the colon is +required, because it is the record of the decision. A marker with no reason, +with a rule name that does not exist, or that covers no finding is itself a +warning (`ignore-marker`). A `schema-before-connector` marker is expected to +go quiet once its pull request merges, since that rule judges the change; +to keep the column out of the inventory afterwards too, name both rules: +`-- cdclint:ignore schema-before-connector,source-column-not-captured: PII`. + 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..e2dcd7d 100644 --- a/cmd/cdclint/corpus_test.go +++ b/cmd/cdclint/corpus_test.go @@ -82,7 +82,18 @@ 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)) + // The same path as the command line: markers in the SQL + // directories, no --disable. + findings, hidden, acked, err := lint(in, markerDirs(filepath.Join(name, "migrations"), sinks), nil) + if err != nil { + t.Fatal(err) + } + 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 := TextReport(findings, hidden, acked) 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..054459c 100644 --- a/cmd/cdclint/main.go +++ b/cmd/cdclint/main.go @@ -14,12 +14,14 @@ import ( "encoding/json" "flag" "fmt" + "io" "os" "strings" "github.com/avison9/cdclint/internal/capture/debezium" "github.com/avison9/cdclint/internal/engine" "github.com/avison9/cdclint/internal/gitread" + "github.com/avison9/cdclint/internal/ignore" "github.com/avison9/cdclint/internal/model" "github.com/avison9/cdclint/internal/sink/clickhouse" "github.com/avison9/cdclint/internal/sink/connect" @@ -36,16 +38,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 +57,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 +67,51 @@ 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, acked, err := lint(in, markerDirs(*migrations, sinks), disabled) + if err != nil { + fmt.Fprintln(stderr, "cdclint:", err) + return 2 + } switch *format { case "json": - enc := json.NewEncoder(os.Stdout) + enc := json.NewEncoder(stdout) enc.SetIndent("", " ") type out struct { Rule string `json:"rule"` @@ -102,8 +129,15 @@ func run(args []string) int { rows = []out{} } _ = enc.Encode(rows) + // The array's shape is what consumers parse, so the notes go + // to stderr rather than into it. + for _, line := range strings.SplitAfter(ackNote(acked)+hiddenNote(hidden), "\n") { + if line != "" { + fmt.Fprint(stderr, "cdclint: "+line) + } + } default: - os.Stdout.WriteString(Render(findings)) + fmt.Fprint(stdout, TextReport(findings, hidden, acked)) } threshold := model.Error switch *minSev { @@ -120,6 +154,114 @@ func run(args []string) int { return 0 } +// lint runs the rules, lets cdclint:ignore markers in the SQL directories +// acknowledge findings, and leaves out disabled rules. Markers are applied +// first, so a marker for a disabled rule is not reported as covering +// nothing; acknowledged findings of a disabled rule are left out too. +func lint(in *engine.Input, dirs []string, disabled map[string]bool) ([]model.Finding, map[string]int, []ignore.Ack, error) { + markers, err := ignore.Scan(dirs) + if err != nil { + return nil, nil, nil, err + } + ran := func(rule string) bool { + if disabled[rule] { + return false + } + return rule != "schema-before-connector" || in.Base != nil + } + // schema-before-connector judges the change, so its marker only ever + // covers something in the pull request that adds the column. + diffRules := map[string]bool{"schema-before-connector": true} + kept, acked := ignore.Apply(engine.Run(in), markers, engine.KnownRule, ran, diffRules) + kept, hidden := without(kept, disabled) + var shown []ignore.Ack + for _, a := range acked { + if !disabled[a.Finding.Rule] { + shown = append(shown, a) + } + } + return kept, hidden, shown, nil +} + +// markerDirs are the directories whose SQL files may carry cdclint:ignore +// markers: the migrations and every sink directory, with their dialect +// prefixes taken off. +func markerDirs(migrations string, sinks []string) []string { + strip := func(s string) string { + if i := strings.Index(s, ":"); i > 0 && !strings.Contains(s[:i], "/") { + return s[i+1:] + } + return s + } + dirs := []string{strip(migrations)} + for _, s := range sinks { + dirs = append(dirs, strip(s)) + } + return dirs +} + +// TextReport is the text output: the findings that stand and a summary, +// then what cdclint:ignore acknowledged and what --disable left out. A run +// whose findings were all acknowledged or disabled does not claim that the +// three files agree, which would be more than was checked. +func TextReport(findings []model.Finding, hidden map[string]int, acked []ignore.Ack) string { + var b strings.Builder + switch { + case len(findings) > 0 || len(hidden) == 0 && len(acked) == 0: + b.WriteString(Render(findings)) + case len(acked) == 0: + b.WriteString("ok: nothing to report outside the disabled rules\n") + case len(hidden) == 0: + b.WriteString("ok: nothing to report beyond what cdclint:ignore acknowledges\n") + default: + b.WriteString("ok: nothing to report outside the disabled rules and what cdclint:ignore acknowledges\n") + } + b.WriteString(ackNote(acked)) + b.WriteString(hiddenNote(hidden)) + return b.String() +} + +// ackNote lists each acknowledged finding with the reason its marker gives. +func ackNote(acked []ignore.Ack) string { + var b strings.Builder + for _, a := range acked { + fmt.Fprintf(&b, "acknowledged (cdclint:ignore): %s %s: %s\n", a.Finding.Rule, a.Finding.Pos, a.Reason) + } + return b.String() +} + +// 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/corpus/README.md b/corpus/README.md index 06a890a..98ecd1d 100644 --- a/corpus/README.md +++ b/corpus/README.md @@ -26,6 +26,8 @@ The rest exercise one path each: | `clickhouse-kafka-connect` | a ClickHouse MergeTree table fed by the Kafka Connect sink rather than a Kafka-engine table | | `kafka-topic-typo` | a Kafka-engine table naming a topic nothing produces | | `include-list-typo` | an include-list pattern that matches no column, and the read it silently breaks | +| `ignore-marker-pii` | the diff rule with a decision recorded: of two columns added and left off the include list, the one whose line says `cdclint:ignore schema-before-connector: PII` is acknowledged and the forgotten one is raised; a trailing marker does not reach the next line | +| `ignore-marker-mistakes` | markers used wrong: one with no reason, one naming a rule that does not exist, one covering nothing, each an `ignore-marker` warning; and one on the line above a finding, which acknowledges it | | `golang-migrate-down-files` | Mattermost v10.11.0's `000092_add_createat_to_teammembers.down.sql` drops a column from the wrong table; read in name order it ran just before its up file and deleted `reactions.createat`. Down files are skipped | | `goose-down-section` | a goose file's down section follows its up section; applied whole, every table was created and dropped again. Down sections are skipped | | `include-table-no-schema` | Stack Overflow 74103659: `table.include.list` names `ipaddrs` without its schema, the connector runs and no topic appears | diff --git a/corpus/diff-adds-column-connector-untouched/expected.txt b/corpus/diff-adds-column-connector-untouched/expected.txt index a1fe7df..32e5bf5 100644 --- a/corpus/diff-adds-column-connector-untouched/expected.txt +++ b/corpus/diff-adds-column-connector-untouched/expected.txt @@ -4,5 +4,5 @@ info source-column-not-captured diff-adds-column-connector-untouched/migrations/ warning schema-before-connector diff-adds-column-connector-untouched/migrations/0035_validation_response_distance.sql:5 this change adds public.report_validations.response_distance_m to a captured table without adding it to column.include.list in diff-adds-column-connector-untouched/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.report_validations.response_distance_m 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) + fix: add public.report_validations.response_distance_m to column.include.list in the same change, or, if it is left off on purpose, say so on the line that adds it: -- cdclint:ignore schema-before-connector: 0 error(s), 1 warning(s), 1 info diff --git a/corpus/diff-connector-captures-one-of-two/expected.txt b/corpus/diff-connector-captures-one-of-two/expected.txt index 746b794..3c7fe20 100644 --- a/corpus/diff-connector-captures-one-of-two/expected.txt +++ b/corpus/diff-connector-captures-one-of-two/expected.txt @@ -1,5 +1,5 @@ -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) + fix: add public.reports.movement_cleared_by to column.include.list in the same change, or, if it is left off on purpose, say so on the line that adds it: -- cdclint:ignore schema-before-connector: 0 error(s), 1 warning(s), 0 info diff --git a/corpus/diff-connector-touched-other-table/expected.txt b/corpus/diff-connector-touched-other-table/expected.txt index 3fe2566..b06df4f 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 + fix: add public.reports.movement_cleared_at to column.include.list in the same change, or, if it is left off on purpose, say so on the line that adds it: -- cdclint:ignore schema-before-connector: +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) + fix: add public.reports.movement_cleared_by to column.include.list in the same change, or, if it is left off on purpose, say so on the line that adds it: -- cdclint:ignore schema-before-connector: +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, if it is left off on purpose, say so on the line that adds it: -- cdclint:ignore schema-before-connector: 0 error(s), 3 warning(s), 0 info diff --git a/corpus/diff-include-list-narrowed/expected.txt b/corpus/diff-include-list-narrowed/expected.txt index 603d13e..31d7801 100644 --- a/corpus/diff-include-list-narrowed/expected.txt +++ b/corpus/diff-include-list-narrowed/expected.txt @@ -4,5 +4,5 @@ info source-column-not-captured diff-include-list-narrowed/migrations/0033_repor warning schema-before-connector diff-include-list-narrowed/migrations/0035_validation_response_distance.sql:5 this change adds public.report_validations.response_distance_m to a captured table without adding it to column.include.list in diff-include-list-narrowed/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.report_validations.response_distance_m 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) + fix: add public.report_validations.response_distance_m to column.include.list in the same change, or, if it is left off on purpose, say so on the line that adds it: -- cdclint:ignore schema-before-connector: 0 error(s), 1 warning(s), 1 info diff --git a/corpus/ignore-marker-mistakes/connector.json b/corpus/ignore-marker-mistakes/connector.json new file mode 100644 index 0000000..7642ab1 --- /dev/null +++ b/corpus/ignore-marker-mistakes/connector.json @@ -0,0 +1,12 @@ +{ + "name": "shop-source", + "config": { + "connector.class": "io.debezium.connector.postgresql.PostgresConnector", + "plugin.name": "pgoutput", + "topic.prefix": "shop", + "table.include.list": "public.orders", + "column.include.list": "public\\.orders\\.(id|total)", + "transforms": "unwrap", + "transforms.unwrap.type": "io.debezium.transforms.ExtractNewRecordState" + } +} diff --git a/corpus/ignore-marker-mistakes/expected.txt b/corpus/ignore-marker-mistakes/expected.txt new file mode 100644 index 0000000..210d9d7 --- /dev/null +++ b/corpus/ignore-marker-mistakes/expected.txt @@ -0,0 +1,15 @@ +warning ignore-marker ignore-marker-mistakes/sink/0001_orders.sql:3 + cdclint:ignore topic-table-mapping covers no finding on the line below it + fix: put it after the code on the line the finding points at, or alone on the line above it, or remove it if the finding is gone +warning ignore-marker ignore-marker-mistakes/sink/0001_orders.sql:8 + cdclint:ignore sink-column-not-captured gives no reason, so it acknowledges nothing; the reason is the record of the decision + fix: say why after a colon: -- cdclint:ignore sink-column-not-captured: +error sink-column-not-captured ignore-marker-mistakes/sink/0001_orders.sql:9 + public.orders.currency is read by kafka_orders (reads topic shop.public.orders) but is not matched by column.include.list in ignore-marker-mistakes/connector.json + every row will carry the column's default, with no error anywhere + fix: add public.orders.currency to column.include.list, deploy the connector, then apply the sink schema; rows already written need a snapshot +warning ignore-marker ignore-marker-mistakes/sink/0001_orders.sql:11 + cdclint:ignore names sink-colum-unknown, which is not a rule, so it acknowledges nothing + fix: use one of the rule names cdclint --help lists +1 error(s), 3 warning(s), 0 info +acknowledged (cdclint:ignore): sink-column-not-captured ignore-marker-mistakes/sink/0001_orders.sql:11: filled by a backfill job, not by the stream diff --git a/corpus/ignore-marker-mistakes/migrations/0001_orders.sql b/corpus/ignore-marker-mistakes/migrations/0001_orders.sql new file mode 100644 index 0000000..42d01c4 --- /dev/null +++ b/corpus/ignore-marker-mistakes/migrations/0001_orders.sql @@ -0,0 +1,6 @@ +CREATE TABLE orders ( + id BIGSERIAL PRIMARY KEY, + total NUMERIC(12, 2) NOT NULL, + currency CHAR(3) NOT NULL, + note TEXT +); diff --git a/corpus/ignore-marker-mistakes/sink/0001_orders.sql b/corpus/ignore-marker-mistakes/sink/0001_orders.sql new file mode 100644 index 0000000..f86b766 --- /dev/null +++ b/corpus/ignore-marker-mistakes/sink/0001_orders.sql @@ -0,0 +1,18 @@ +-- Each marker here is a mistake cdclint must point out, except the one +-- above note, which acknowledges the finding on the line below it. +-- cdclint:ignore topic-table-mapping: the topic was renamed long ago +CREATE TABLE kafka_orders +( + `id` Int64, + `total` String, + -- cdclint:ignore sink-column-not-captured + `currency` String, + -- cdclint:ignore sink-column-not-captured: filled by a backfill job, not by the stream + `note` Nullable(String) -- cdclint:ignore sink-colum-unknown: a typo in the rule name +) +ENGINE = Kafka +SETTINGS + kafka_broker_list = '${KAFKA_BROKERS}', + kafka_topic_list = 'shop.public.orders', + kafka_group_name = 'clickhouse-orders', + kafka_format = 'JSONEachRow'; diff --git a/corpus/ignore-marker-pii/base/connector.json b/corpus/ignore-marker-pii/base/connector.json new file mode 100644 index 0000000..bd44d0c --- /dev/null +++ b/corpus/ignore-marker-pii/base/connector.json @@ -0,0 +1,12 @@ +{ + "name": "app-source", + "config": { + "connector.class": "io.debezium.connector.postgresql.PostgresConnector", + "plugin.name": "pgoutput", + "topic.prefix": "app", + "table.include.list": "public.users", + "column.include.list": "public\\.users\\.(id|email)", + "transforms": "unwrap", + "transforms.unwrap.type": "io.debezium.transforms.ExtractNewRecordState" + } +} diff --git a/corpus/ignore-marker-pii/base/migrations/0001_users.sql b/corpus/ignore-marker-pii/base/migrations/0001_users.sql new file mode 100644 index 0000000..088f383 --- /dev/null +++ b/corpus/ignore-marker-pii/base/migrations/0001_users.sql @@ -0,0 +1,4 @@ +CREATE TABLE users ( + id UUID PRIMARY KEY, + email TEXT NOT NULL +); diff --git a/corpus/ignore-marker-pii/connector.json b/corpus/ignore-marker-pii/connector.json new file mode 100644 index 0000000..bd44d0c --- /dev/null +++ b/corpus/ignore-marker-pii/connector.json @@ -0,0 +1,12 @@ +{ + "name": "app-source", + "config": { + "connector.class": "io.debezium.connector.postgresql.PostgresConnector", + "plugin.name": "pgoutput", + "topic.prefix": "app", + "table.include.list": "public.users", + "column.include.list": "public\\.users\\.(id|email)", + "transforms": "unwrap", + "transforms.unwrap.type": "io.debezium.transforms.ExtractNewRecordState" + } +} diff --git a/corpus/ignore-marker-pii/expected.txt b/corpus/ignore-marker-pii/expected.txt new file mode 100644 index 0000000..87e1cb6 --- /dev/null +++ b/corpus/ignore-marker-pii/expected.txt @@ -0,0 +1,6 @@ +warning schema-before-connector ignore-marker-pii/migrations/0002_users_profile.sql:6 + this change adds public.users.nickname to a captured table without adding it to column.include.list in ignore-marker-pii/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.users.nickname to column.include.list in the same change, or, if it is left off on purpose, say so on the line that adds it: -- cdclint:ignore schema-before-connector: +0 error(s), 1 warning(s), 0 info +acknowledged (cdclint:ignore): schema-before-connector ignore-marker-pii/migrations/0002_users_profile.sql:5: PII, never streamed diff --git a/corpus/ignore-marker-pii/migrations/0001_users.sql b/corpus/ignore-marker-pii/migrations/0001_users.sql new file mode 100644 index 0000000..088f383 --- /dev/null +++ b/corpus/ignore-marker-pii/migrations/0001_users.sql @@ -0,0 +1,4 @@ +CREATE TABLE users ( + id UUID PRIMARY KEY, + email TEXT NOT NULL +); diff --git a/corpus/ignore-marker-pii/migrations/0002_users_profile.sql b/corpus/ignore-marker-pii/migrations/0002_users_profile.sql new file mode 100644 index 0000000..847ab13 --- /dev/null +++ b/corpus/ignore-marker-pii/migrations/0002_users_profile.sql @@ -0,0 +1,6 @@ +-- The change under review adds two columns and leaves both off the +-- connector's include list. One is left off on purpose, and the line that +-- adds it says so; the other was forgotten, and is raised. +ALTER TABLE users + ADD COLUMN ssn_hash TEXT, -- cdclint:ignore schema-before-connector: PII, never streamed + ADD COLUMN nickname TEXT; diff --git a/corpus/ignore-marker-pii/sink/0001_users.sql b/corpus/ignore-marker-pii/sink/0001_users.sql new file mode 100644 index 0000000..8ba3d3a --- /dev/null +++ b/corpus/ignore-marker-pii/sink/0001_users.sql @@ -0,0 +1,11 @@ +CREATE TABLE kafka_users +( + `id` String, + `email` String +) +ENGINE = Kafka +SETTINGS + kafka_broker_list = '${KAFKA_BROKERS}', + kafka_topic_list = 'app.public.users', + kafka_group_name = 'clickhouse-users', + kafka_format = 'JSONEachRow'; diff --git a/corpus/mysql-diff-adds-column/expected.txt b/corpus/mysql-diff-adds-column/expected.txt index 2be666f..3ed92d3 100644 --- a/corpus/mysql-diff-adds-column/expected.txt +++ b/corpus/mysql-diff-adds-column/expected.txt @@ -1,5 +1,5 @@ warning schema-before-connector mysql-diff-adds-column/migrations/V2__orders_coupon.sql:3 this change adds shop.orders.coupon_code to a captured table without adding it to column.include.list in mysql-diff-adds-column/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 shop.orders.coupon_code 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) + fix: add shop.orders.coupon_code to column.include.list in the same change, or, if it is left off on purpose, say so on the line that adds it: -- cdclint:ignore schema-before-connector: 0 error(s), 1 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/engine/diff.go b/internal/engine/diff.go index 154bb3a..876daf2 100644 --- a/internal/engine/diff.go +++ b/internal/engine/diff.go @@ -111,7 +111,7 @@ func schemaBeforeConnector(in *Input, reads []Read) ([]model.Finding, map[string fs = append(fs, model.Finding{ Rule: "schema-before-connector", Severity: model.Warning, Pos: c.Pos, Message: fmt.Sprintf("this change adds %s to a captured table without adding it to %s in %s (compared with %s)\nthe column will not be in the stream; if a sink is later given it, every row will be the default until a snapshot", q, in.Contract.ColumnListSetting(), in.Contract.Pos().File, short(in.Base.Ref)), - Fix: fmt.Sprintf("%s 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)", edit(in.Contract.ColumnListSetting(), q)), + Fix: fmt.Sprintf("%s in the same change, or, if it is left off on purpose, say so on the line that adds it: -- cdclint:ignore schema-before-connector: ", edit(in.Contract.ColumnListSetting(), q)), }) } } diff --git a/internal/engine/rules.go b/internal/engine/rules.go new file mode 100644 index 0000000..06ed784 --- /dev/null +++ b/internal/engine/rules.go @@ -0,0 +1,29 @@ +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", + "ignore-marker", +} + +// KnownRule reports whether name is one of Rules. +func KnownRule(name string) bool { + for _, r := range Rules { + if r == name { + return true + } + } + return false +} diff --git a/internal/ignore/ignore.go b/internal/ignore/ignore.go new file mode 100644 index 0000000..88d2f40 --- /dev/null +++ b/internal/ignore/ignore.go @@ -0,0 +1,192 @@ +// Package ignore reads cdclint:ignore markers: a comment in a migration or a +// sink file that records a decision about one finding, so the finding stops +// failing the run while the decision stays in the file, next to the column, +// where the pull request that made it shows it. +// +// ALTER TABLE users +// ADD COLUMN ssn_hash TEXT; -- cdclint:ignore schema-before-connector: PII, never streamed +// +// A marker names one or more rules and gives a reason after a colon. After +// code on the same line it covers that line only; on a line of its own it +// covers the line below. A trailing marker must not reach the next line: +// on a two-column ALTER it would silently acknowledge the column that was +// forgotten. The reason is required: it is the record of the decision. +package ignore + +import ( + "bufio" + "fmt" + "os" + "path/filepath" + "regexp" + "sort" + "strings" + + "github.com/avison9/cdclint/internal/migrate" + "github.com/avison9/cdclint/internal/model" +) + +// Rule is the name of the findings this package raises about markers. +const Rule = "ignore-marker" + +// Ack is a finding a marker acknowledged, with the marker's reason. +type Ack struct { + Finding model.Finding + Reason string +} + +// Marker is one cdclint:ignore comment. +type Marker struct { + Pos model.Pos + Rules []string + Reason string + // Alone is true for a marker on a line of its own, which covers the + // line below; a marker after code covers its own line. + Alone bool + used bool +} + +// Covers reports whether the marker applies to a finding at line. +func (m *Marker) Covers(line int) bool { + if m.Alone { + return line == m.Pos.Line+1 + } + return line == m.Pos.Line +} + +var marker = regexp.MustCompile(`(?:--|#)\s*cdclint:ignore\b(.*)$`) + +// Scan reads the *.sql files directly in each dir (the readers' own +// selection, down migrations left out) for markers. +func Scan(dirs []string) ([]*Marker, error) { + var out []*Marker + for _, dir := range dirs { + entries, err := os.ReadDir(dir) + if err != nil { + return nil, err + } + var names []string + for _, e := range entries { + if !e.IsDir() && strings.HasSuffix(strings.ToLower(e.Name()), ".sql") && !migrate.Down(e.Name()) { + names = append(names, e.Name()) + } + } + sort.Strings(names) + for _, n := range names { + path := filepath.Join(dir, n) + f, err := os.Open(path) + if err != nil { + return nil, err + } + sc := bufio.NewScanner(f) + sc.Buffer(make([]byte, 64*1024), 4*1024*1024) + for line := 1; sc.Scan(); line++ { + text := sc.Text() + if loc := marker.FindStringSubmatchIndex(text); loc != nil { + mk := parse(model.Pos{File: path, Line: line}, text[loc[2]:loc[3]]) + mk.Alone = strings.TrimSpace(text[:loc[0]]) == "" + out = append(out, mk) + } + } + err = sc.Err() + f.Close() + if err != nil { + return nil, fmt.Errorf("%s: %w", path, err) + } + } + } + return out, nil +} + +func parse(pos model.Pos, rest string) *Marker { + m := &Marker{Pos: pos} + rules := rest + if i := strings.Index(rest, ":"); i >= 0 { + rules, m.Reason = rest[:i], strings.TrimSpace(rest[i+1:]) + } + for _, r := range strings.FieldsFunc(rules, func(c rune) bool { return c == ',' || c == ' ' || c == '\t' }) { + m.Rules = append(m.Rules, r) + } + return m +} + +// Apply splits findings into the ones that stand and the ones a marker +// acknowledges, and adds a warning for each marker that is malformed or, +// for a rule that ran and judges the current state, covers nothing. +// ran reports whether a rule ran this time; diffRules are the rules that +// judge the change rather than the state, whose markers go quiet once the +// change that added them has merged and are therefore never unused. +func Apply(findings []model.Finding, markers []*Marker, known, ran func(string) bool, diffRules map[string]bool) (kept []model.Finding, acknowledged []Ack) { + valid := func(m *Marker) bool { return m.Reason != "" && len(m.Rules) > 0 && allKnown(m.Rules, known) } + for _, f := range findings { + var by *Marker + for _, m := range markers { + if valid(m) && m.Pos.File == f.Pos.File && m.Covers(f.Pos.Line) && contains(m.Rules, f.Rule) { + by = m + break + } + } + if by == nil { + kept = append(kept, f) + continue + } + by.used = true + acknowledged = append(acknowledged, Ack{Finding: f, Reason: by.Reason}) + } + for _, m := range markers { + switch { + case len(m.Rules) == 0: + kept = append(kept, problem(m, "a cdclint:ignore marker names no rule, so it acknowledges nothing", + "name the rule and the reason: -- cdclint:ignore : ")) + case !allKnown(m.Rules, known): + kept = append(kept, problem(m, fmt.Sprintf("cdclint:ignore names %s, which is not a rule, so it acknowledges nothing", strings.Join(unknown(m.Rules, known), ", ")), + "use one of the rule names cdclint --help lists")) + case m.Reason == "": + kept = append(kept, problem(m, fmt.Sprintf("cdclint:ignore %s gives no reason, so it acknowledges nothing; the reason is the record of the decision", strings.Join(m.Rules, ",")), + fmt.Sprintf("say why after a colon: -- cdclint:ignore %s: ", strings.Join(m.Rules, ",")))) + case !m.used && judgedNow(m.Rules, ran, diffRules): + kept = append(kept, problem(m, fmt.Sprintf("cdclint:ignore %s covers no finding %s", strings.Join(m.Rules, ","), map[bool]string{true: "on the line below it", false: "on its line"}[m.Alone]), + "put it after the code on the line the finding points at, or alone on the line above it, or remove it if the finding is gone")) + } + } + model.Sort(kept) + return kept, acknowledged +} + +// judgedNow is true when every rule the marker names ran this time and +// judges the current state, so a marker that covers nothing is stale. +func judgedNow(rules []string, ran func(string) bool, diffRules map[string]bool) bool { + for _, r := range rules { + if diffRules[r] || !ran(r) { + return false + } + } + return true +} + +func problem(m *Marker, message, fix string) model.Finding { + return model.Finding{Rule: Rule, Severity: model.Warning, Pos: m.Pos, Message: message, Fix: fix} +} + +func allKnown(rules []string, known func(string) bool) bool { + return len(unknown(rules, known)) == 0 +} + +func unknown(rules []string, known func(string) bool) []string { + var out []string + for _, r := range rules { + if !known(r) { + out = append(out, r) + } + } + return out +} + +func contains(list []string, s string) bool { + for _, x := range list { + if x == s { + return true + } + } + return false +} diff --git a/internal/ignore/ignore_test.go b/internal/ignore/ignore_test.go new file mode 100644 index 0000000..2c9d2c0 --- /dev/null +++ b/internal/ignore/ignore_test.go @@ -0,0 +1,78 @@ +package ignore + +import ( + "os" + "path/filepath" + "reflect" + "testing" + + "github.com/avison9/cdclint/internal/model" +) + +func scanText(t *testing.T, name, text string) []*Marker { + t.Helper() + dir := t.TempDir() + if err := os.WriteFile(filepath.Join(dir, name), []byte(text), 0o644); err != nil { + t.Fatal(err) + } + ms, err := Scan([]string{dir}) + if err != nil { + t.Fatal(err) + } + return ms +} + +func TestMarkersAreParsedInBothCommentStyles(t *testing.T) { + ms := scanText(t, "V2.sql", "ALTER TABLE t ADD c INT; -- cdclint:ignore schema-before-connector, source-column-not-captured: PII\n"+ + "# cdclint:ignore sink-column-not-captured: MySQL comment style\n"+ + "-- cdclint:ignore schema-before-connector\n"+ + "-- a comment that mentions cdclint but is not a marker\n") + if len(ms) != 3 { + t.Fatalf("got %d markers, want 3", len(ms)) + } + if !reflect.DeepEqual(ms[0].Rules, []string{"schema-before-connector", "source-column-not-captured"}) || ms[0].Reason != "PII" || ms[0].Alone { + t.Errorf("trailing marker = %+v", ms[0]) + } + if ms[1].Reason != "MySQL comment style" || !ms[1].Alone || ms[1].Pos.Line != 2 { + t.Errorf("# marker = %+v", ms[1]) + } + if ms[2].Reason != "" { + t.Errorf("a marker with no colon has no reason: %+v", ms[2]) + } +} + +func TestDownMigrationsAreNotScanned(t *testing.T) { + if ms := scanText(t, "0001_x.down.sql", "-- cdclint:ignore schema-before-connector: x\n"); len(ms) != 0 { + t.Errorf("markers in a down file: %d", len(ms)) + } +} + +func finding(rule string, line int) model.Finding { + return model.Finding{Rule: rule, Severity: model.Warning, Pos: model.Pos{File: "f.sql", Line: line}} +} + +func known(string) bool { return true } + +func TestATrailingMarkerCoversItsLineAndAStandaloneOneTheNext(t *testing.T) { + trailing := &Marker{Pos: model.Pos{File: "f.sql", Line: 5}, Rules: []string{"r"}, Reason: "why"} + alone := &Marker{Pos: model.Pos{File: "f.sql", Line: 7}, Rules: []string{"r"}, Reason: "why", Alone: true} + kept, acked := Apply([]model.Finding{finding("r", 5), finding("r", 6), finding("r", 8)}, + []*Marker{trailing, alone}, known, known, nil) + if len(acked) != 2 || acked[0].Finding.Pos.Line != 5 || acked[1].Finding.Pos.Line != 8 { + t.Errorf("acknowledged %+v", acked) + } + if len(kept) != 1 || kept[0].Pos.Line != 6 { + t.Errorf("kept %+v: the line after a trailing marker must stand", kept) + } +} + +func TestUnusedMarkersAreReportedOnlyForRulesThatJudgedTheState(t *testing.T) { + diff := &Marker{Pos: model.Pos{File: "f.sql", Line: 1}, Rules: []string{"schema-before-connector"}, Reason: "merged long ago"} + notRun := &Marker{Pos: model.Pos{File: "f.sql", Line: 2}, Rules: []string{"disabled-rule"}, Reason: "x"} + stale := &Marker{Pos: model.Pos{File: "f.sql", Line: 3}, Rules: []string{"state-rule"}, Reason: "x"} + ran := func(r string) bool { return r != "disabled-rule" } + kept, _ := Apply(nil, []*Marker{diff, notRun, stale}, known, ran, map[string]bool{"schema-before-connector": true}) + if len(kept) != 1 || kept[0].Rule != Rule || kept[0].Pos.Line != 3 { + t.Errorf("kept %+v, want one ignore-marker warning for the stale state-rule marker", kept) + } +} 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) + } + } +}