The down half of a migration is not applied: golang-migrate down files and goose, sql-migrate and dbmate down sections

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

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 |

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"
+  }
+}

corpus/golang-migrate-down-files/expected.txt

@@ -0,0 +1 @@
+ok: source, connector and sink agree

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;

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
+);

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;

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;

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';

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"
+  }
+}

corpus/goose-down-section/expected.txt

@@ -0,0 +1 @@
+ok: source, connector and sink agree

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;

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;

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';

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 ""
+}

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)
+		}
+	}
+}

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,19 +57,24 @@ 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)
 		}
 	}
 	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 {