diff --git a/README.md b/README.md index 20d9f32..6ed9833 100644 --- a/README.md +++ b/README.md @@ -157,6 +157,12 @@ cdclint --migrations db/migrations \ --sink-connector cdc/snowflake-sink.json ``` +The migrations are applied in filename order, the way every migration runner +does, and the down half of a migration is left out, since a forward migrate +never runs it: golang-migrate's `*.down.sql` files, and the down section of a +goose (`-- +goose Down`), sql-migrate (`-- +migrate Down`) or dbmate +(`-- migrate:down`) file. + Add `--base origin/main` (any git ref) and the diff-aware rule judges the change itself: a column added to a captured table and left off the include list is raised while the author is still there. Each new column is judged on diff --git a/corpus/README.md b/corpus/README.md index cf1d37e..c2dddf5 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 | +| `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 | | `diff-adds-column-connector-untouched` | the diff rule: `base/` holds the migrations and connector before the change; the change adds a column and leaves the connector alone | | `diff-connector-captures-one-of-two` | #963 with a hurried fix: two of three new columns go on the include list in the same change; the third is raised, since adding two says nothing about it | | `diff-connector-touched-other-table` | RefuseRadar #962 and #963 in one range: the connector gains report_validations columns, the migration adds reports columns; the reports ones are raised | diff --git a/corpus/golang-migrate-down-files/connector.json b/corpus/golang-migrate-down-files/connector.json new file mode 100644 index 0000000..10fb6a3 --- /dev/null +++ b/corpus/golang-migrate-down-files/connector.json @@ -0,0 +1,11 @@ +{ + "name": "mm-source", + "config": { + "connector.class": "io.debezium.connector.postgresql.PostgresConnector", + "plugin.name": "pgoutput", + "topic.prefix": "mm", + "table.include.list": "public.reactions,public.teammembers", + "transforms": "unwrap", + "transforms.unwrap.type": "io.debezium.transforms.ExtractNewRecordState" + } +} diff --git a/corpus/golang-migrate-down-files/expected.txt b/corpus/golang-migrate-down-files/expected.txt new file mode 100644 index 0000000..cd1f70b --- /dev/null +++ b/corpus/golang-migrate-down-files/expected.txt @@ -0,0 +1 @@ +ok: source, connector and sink agree diff --git a/corpus/golang-migrate-down-files/migrations/0001_create_tables.down.sql b/corpus/golang-migrate-down-files/migrations/0001_create_tables.down.sql new file mode 100644 index 0000000..4ca37d3 --- /dev/null +++ b/corpus/golang-migrate-down-files/migrations/0001_create_tables.down.sql @@ -0,0 +1,2 @@ +DROP TABLE IF EXISTS teammembers; +DROP TABLE IF EXISTS reactions; diff --git a/corpus/golang-migrate-down-files/migrations/0001_create_tables.up.sql b/corpus/golang-migrate-down-files/migrations/0001_create_tables.up.sql new file mode 100644 index 0000000..99d27dc --- /dev/null +++ b/corpus/golang-migrate-down-files/migrations/0001_create_tables.up.sql @@ -0,0 +1,10 @@ +CREATE TABLE reactions ( + userid VARCHAR(26) NOT NULL, + postid VARCHAR(26) NOT NULL, + createat BIGINT +); + +CREATE TABLE teammembers ( + teamid VARCHAR(26) NOT NULL, + userid VARCHAR(26) NOT NULL +); diff --git a/corpus/golang-migrate-down-files/migrations/0002_add_createat_to_teammembers.down.sql b/corpus/golang-migrate-down-files/migrations/0002_add_createat_to_teammembers.down.sql new file mode 100644 index 0000000..0106755 --- /dev/null +++ b/corpus/golang-migrate-down-files/migrations/0002_add_createat_to_teammembers.down.sql @@ -0,0 +1,6 @@ +-- Reduced from Mattermost v10.11.0, 000092_add_createat_to_teammembers.down.sql: +-- the down migration drops the column from the wrong table (Reactions, not +-- TeamMembers). A forward migrate never runs it, but read in name order it +-- ran just before its own up file and deleted reactions.createat, which the +-- sink reads. +ALTER TABLE reactions DROP COLUMN IF EXISTS createat; diff --git a/corpus/golang-migrate-down-files/migrations/0002_add_createat_to_teammembers.up.sql b/corpus/golang-migrate-down-files/migrations/0002_add_createat_to_teammembers.up.sql new file mode 100644 index 0000000..d2394b8 --- /dev/null +++ b/corpus/golang-migrate-down-files/migrations/0002_add_createat_to_teammembers.up.sql @@ -0,0 +1 @@ +ALTER TABLE teammembers ADD COLUMN IF NOT EXISTS createat BIGINT DEFAULT 0; diff --git a/corpus/golang-migrate-down-files/sink/0001_reactions.sql b/corpus/golang-migrate-down-files/sink/0001_reactions.sql new file mode 100644 index 0000000..55090df --- /dev/null +++ b/corpus/golang-migrate-down-files/sink/0001_reactions.sql @@ -0,0 +1,12 @@ +CREATE TABLE kafka_reactions +( + `userid` String, + `postid` String, + `createat` Int64 +) +ENGINE = Kafka +SETTINGS + kafka_broker_list = '${KAFKA_BROKERS}', + kafka_topic_list = 'mm.public.reactions', + kafka_group_name = 'clickhouse-reactions', + kafka_format = 'JSONEachRow'; diff --git a/corpus/goose-down-section/connector.json b/corpus/goose-down-section/connector.json new file mode 100644 index 0000000..ede6452 --- /dev/null +++ b/corpus/goose-down-section/connector.json @@ -0,0 +1,11 @@ +{ + "name": "shop-source", + "config": { + "connector.class": "io.debezium.connector.postgresql.PostgresConnector", + "plugin.name": "pgoutput", + "topic.prefix": "shop", + "table.include.list": "public.orders", + "transforms": "unwrap", + "transforms.unwrap.type": "io.debezium.transforms.ExtractNewRecordState" + } +} diff --git a/corpus/goose-down-section/expected.txt b/corpus/goose-down-section/expected.txt new file mode 100644 index 0000000..cd1f70b --- /dev/null +++ b/corpus/goose-down-section/expected.txt @@ -0,0 +1 @@ +ok: source, connector and sink agree diff --git a/corpus/goose-down-section/migrations/00001_create_orders.sql b/corpus/goose-down-section/migrations/00001_create_orders.sql new file mode 100644 index 0000000..0879eb9 --- /dev/null +++ b/corpus/goose-down-section/migrations/00001_create_orders.sql @@ -0,0 +1,11 @@ +-- goose keeps both halves of a migration in one file. The down section +-- comes after the up one, so a reader that applies the whole file creates +-- the table and drops it again. +-- +goose Up +CREATE TABLE orders ( + id BIGSERIAL PRIMARY KEY, + total NUMERIC(12, 2) NOT NULL +); + +-- +goose Down +DROP TABLE orders; diff --git a/corpus/goose-down-section/migrations/00002_add_currency.sql b/corpus/goose-down-section/migrations/00002_add_currency.sql new file mode 100644 index 0000000..8933f1a --- /dev/null +++ b/corpus/goose-down-section/migrations/00002_add_currency.sql @@ -0,0 +1,7 @@ +-- +goose Up +-- +goose StatementBegin +ALTER TABLE orders ADD COLUMN currency CHAR(3) NOT NULL DEFAULT 'EUR'; +-- +goose StatementEnd + +-- +goose Down +ALTER TABLE orders DROP COLUMN currency; diff --git a/corpus/goose-down-section/sink/0001_orders.sql b/corpus/goose-down-section/sink/0001_orders.sql new file mode 100644 index 0000000..eea4d71 --- /dev/null +++ b/corpus/goose-down-section/sink/0001_orders.sql @@ -0,0 +1,12 @@ +CREATE TABLE kafka_orders +( + `id` Int64, + `total` String, + `currency` String +) +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/internal/migrate/migrate.go b/internal/migrate/migrate.go new file mode 100644 index 0000000..138fbfe --- /dev/null +++ b/internal/migrate/migrate.go @@ -0,0 +1,65 @@ +// Package migrate knows how the common migration tools mark the "down" +// half of a migration, which a forward migrate never runs and a reader of +// the schema must not apply either. +// +// - golang-migrate keeps each half in its own file: +// {version}_{title}.up.sql and {version}_{title}.down.sql. +// - goose (-- +goose Up / -- +goose Down), sql-migrate (-- +migrate Up / +// -- +migrate Down) and dbmate (-- migrate:up / -- migrate:down) keep +// both halves in one file, the down section after the up one, each +// marker optionally followed by options (notransaction, +// transaction:false). +// +// Applied in name order, a separate down file runs just before its own up +// file and is usually a no-op; Mattermost's 000092 down file drops a +// column of another table (Reactions.CreateAt) and deleted it from the +// schema cdclint read. A down section in the same file runs right after +// its up section and undoes it every time: every goose table vanished. +package migrate + +import ( + "path" + "regexp" + "strings" +) + +// Down reports whether path names a golang-migrate down migration. +func Down(p string) bool { + return strings.HasSuffix(strings.ToLower(path.Base(strings.ReplaceAll(p, "\\", "/"))), ".down.sql") +} + +var marker = regexp.MustCompile(`(?i)^\s*--\s*(?:\+goose\s+(up|down)|\+migrate\s+(up|down)|migrate:(up|down))\b`) + +// Up returns text with every down section blanked out, line for line, so +// the statements that remain keep their line numbers. Text with no markers +// is returned unchanged. +func Up(text string) string { + lines := strings.SplitAfter(text, "\n") + down, changed := false, false + for i, l := range lines { + m := marker.FindStringSubmatch(l) + if m != nil { + down = strings.EqualFold(m[1]+m[2]+m[3], "down") + continue + } + if down { + lines[i] = blank(l) + changed = true + } + } + if !changed { + return text + } + return strings.Join(lines, "") +} + +// blank keeps only the line's ending, so the line still counts. +func blank(l string) string { + switch { + case strings.HasSuffix(l, "\r\n"): + return "\r\n" + case strings.HasSuffix(l, "\n"): + return "\n" + } + return "" +} diff --git a/internal/migrate/migrate_test.go b/internal/migrate/migrate_test.go new file mode 100644 index 0000000..1a9e755 --- /dev/null +++ b/internal/migrate/migrate_test.go @@ -0,0 +1,51 @@ +package migrate + +import "testing" + +func TestDownFilesAreGolangMigrates(t *testing.T) { + for p, want := range map[string]bool{ + "db/000092_add_createat.down.sql": true, + "db/000092_add_createat.up.sql": false, + "db/1_init.DOWN.SQL": true, + `db\2_x.down.sql`: true, + "db/0003_countdown.sql": false, + "db/V1__down.sql": false, + } { + if got := Down(p); got != want { + t.Errorf("Down(%q) = %v, want %v", p, got, want) + } + } +} + +func TestUpBlanksDownSectionsLineForLine(t *testing.T) { + for name, tc := range map[string]struct{ in, want string }{ + "goose": { + "-- +goose Up\nCREATE TABLE t (id INT);\n-- +goose Down\nDROP TABLE t;\n", + "-- +goose Up\nCREATE TABLE t (id INT);\n-- +goose Down\n\n", + }, + "sql-migrate with options": { + "-- +migrate Up notransaction\nALTER TABLE t ADD c INT;\n\n-- +migrate Down\nALTER TABLE t DROP c;\n", + "-- +migrate Up notransaction\nALTER TABLE t ADD c INT;\n\n-- +migrate Down\n\n", + }, + "dbmate, down then up again": { + "-- migrate:up transaction:false\nCREATE TABLE a (x INT);\n-- migrate:down\nDROP TABLE a;\n-- migrate:up\nCREATE TABLE b (y INT);\n", + "-- migrate:up transaction:false\nCREATE TABLE a (x INT);\n-- migrate:down\n\n-- migrate:up\nCREATE TABLE b (y INT);\n", + }, + "CRLF line endings": { + "-- +goose Up\r\nCREATE TABLE t (id INT);\r\n-- +goose Down\r\nDROP TABLE t;\r\n", + "-- +goose Up\r\nCREATE TABLE t (id INT);\r\n-- +goose Down\r\n\r\n", + }, + "no markers is unchanged": { + "CREATE TABLE t (id INT);\n-- a comment about down migrations\n", + "CREATE TABLE t (id INT);\n-- a comment about down migrations\n", + }, + "a column named down is not a marker": { + "CREATE TABLE t (\n down BOOLEAN\n);\n", + "CREATE TABLE t (\n down BOOLEAN\n);\n", + }, + } { + if got := Up(tc.in); got != tc.want { + t.Errorf("%s:\n got %q\nwant %q", name, got, tc.want) + } + } +} diff --git a/internal/source/postgres/postgres.go b/internal/source/postgres/postgres.go index b59354c..a5ad307 100644 --- a/internal/source/postgres/postgres.go +++ b/internal/source/postgres/postgres.go @@ -16,6 +16,7 @@ import ( "strings" "github.com/avison9/cdclint/internal/ddl" + "github.com/avison9/cdclint/internal/migrate" "github.com/avison9/cdclint/internal/model" "github.com/avison9/cdclint/internal/sqlsplit" ) @@ -56,9 +57,13 @@ type NamedFile struct { } // ReadFiles applies migrations already in memory, in the order given. +// Down migrations are skipped: a forward migrate never runs them. func ReadFiles(files []NamedFile) (*model.Source, error) { src := &model.Source{} for _, f := range files { + if migrate.Down(f.Path) { + continue + } if err := Apply(src, f.Path, f.Text); err != nil { return nil, fmt.Errorf("%s: %w", f.Path, err) } @@ -66,9 +71,10 @@ func ReadFiles(files []NamedFile) (*model.Source, error) { return src, nil } -// Apply runs one file's statements against src. +// Apply runs one file's statements against src, leaving out a down section +// (goose, sql-migrate, dbmate) the way a forward migrate does. func Apply(src *model.Source, file, text string) error { - for _, st := range sqlsplit.Split(text) { + for _, st := range sqlsplit.Split(migrate.Up(text)) { pos := model.Pos{File: file, Line: st.Line} w := ddl.Words(st.Text) switch {