Repository: amacneil/dbmate
Pull request #397 (2023-02-26 23:52:53)

Stricter migration parsing
- Always require down directive
- Require up migration before down migration

Closes https://github.com/amacneil/dbmate/issues/108


Patch:
diff --git a/README.md b/README.md
index ec83f9b..e24420f 100644
--- a/README.md
+++ b/README.md
@@ -478,7 +478,9 @@ create table users (
 drop table if exists users;
 ```
 
-Both up and down migrations are stored in the same file, for ease of editing. When you apply a migration dbmate only stores the version number, not the contents, so you should always rollback a migration before modifying its contents. For this reason, you can safely rename a migration file without affecting its applied status, as long as you keep the version number intact.
+Both up and down migrations are stored in the same file, for ease of editing. Both up and down directives are required, even if you choose not to implement the down migration.
+
+When you apply a migration dbmate only stores the version number, not the contents, so you should always rollback a migration before modifying its contents. For this reason, you can safely rename a migration file without affecting its applied status, as long as you keep the version number intact.
 
 ### Schema file
 
diff --git a/pkg/dbmate/migration.go b/pkg/dbmate/migration.go
index c2df7ea..57aed43 100644
--- a/pkg/dbmate/migration.go
+++ b/pkg/dbmate/migration.go
@@ -60,7 +60,9 @@ var (
 // Error codes
 var (
 	ErrParseMissingUp      = errors.New("dbmate requires each migration to define an up block with '-- migrate:up'")
-	ErrParseUnexpectedStmt = errors.New("dbmate does not support statements defined outside of the '-- migrate:up' or '-- migrate:down' blocks")
+	ErrParseMissingDown    = errors.New("dbmate requires each migration to define a down block with '-- migrate:down'")
+	ErrParseWrongOrder     = errors.New("dbmate requires '-- migrate:up' to appear before '-- migrate:down'")
+	ErrParseUnexpectedStmt = errors.New("dbmate does not support statements preceding the '-- migrate:up' block")
 )
 
 // parseMigrationContents parses the string contents of a migration.
@@ -69,34 +71,30 @@ var (
 // requires that at least an up block was defined and will otherwise
 // return an error.
 func parseMigrationContents(contents string) (*ParsedMigration, error) {
-	upDirectiveStart, upDirectiveEnd, hasDefinedUpBlock := getMatchPositions(contents, upRegExp)
-	downDirectiveStart, downDirectiveEnd, hasDefinedDownBlock := getMatchPositions(contents, downRegExp)
+	upDirectiveStart, hasDefinedUpBlock := getMatchPosition(contents, upRegExp)
+	downDirectiveStart, hasDefinedDownBlock := getMatchPosition(contents, downRegExp)
 
 	if !hasDefinedUpBlock {
 		return nil, ErrParseMissingUp
-	} else if statementsPrecedeMigrateBlocks(contents, upDirectiveStart, downDirectiveStart) {
-		return nil, ErrParseUnexpectedStmt
 	}
-
-	upEnd := len(contents)
-	downEnd := len(contents)
-
-	if hasDefinedDownBlock && upDirectiveStart < downDirectiveStart {
-		upEnd = downDirectiveStart
-	} else if hasDefinedDownBlock && upDirectiveStart > downDirectiveStart {
-		downEnd = upDirectiveStart
-	} else {
-		downEnd = -1
+	if !hasDefinedDownBlock {
+		return nil, ErrParseMissingDown
+	}
+	if upDirectiveStart > downDirectiveStart {
+		return nil, ErrParseWrongOrder
+	}
+	if statementsPrecedeMigrateBlocks(contents, upDirectiveStart) {
+		return nil, ErrParseUnexpectedStmt
 	}
 
-	upDirective := substring(contents, upDirectiveStart, upDirectiveEnd)
-	downDirective := substring(contents, downDirectiveStart, downDirectiveEnd)
+	upBlock := substring(contents, upDirectiveStart, downDirectiveStart)
+	downBlock := substring(contents, downDirectiveStart, len(contents))
 
 	parsed := ParsedMigration{
-		Up:          substring(contents, upDirectiveStart, upEnd),
-		UpOptions:   parseMigrationOptions(upDirective),
-		Down:        substring(contents, downDirectiveStart, downEnd),
-		DownOptions: parseMigrationOptions(downDirective),
+		Up:          upBlock,
+		UpOptions:   parseMigrationOptions(upBlock),
+		Down:        downBlock,
+		DownOptions: parseMigrationOptions(downBlock),
 	}
 	return &parsed, nil
 }
@@ -111,6 +109,9 @@ func parseMigrationContents(contents string) (*ParsedMigration, error) {
 func parseMigrationOptions(contents string) ParsedMigrationOptions {
 	options := make(migrationOptions)
 
+	// remove everything after first newline
+	contents = strings.SplitN(contents, "\n", 2)[0]
+
 	// strip away the -- migrate:[up|down] part
 	contents = blockDirectiveRegExp.ReplaceAllString(contents, "")
 
@@ -156,14 +157,8 @@ func parseMigrationOptions(contents string) ParsedMigrationOptions {
 // -- migrate:up
 // create table users (id serial, status status_type);
 // `, 54, -1)
-func statementsPrecedeMigrateBlocks(contents string, upDirectiveStart, downDirectiveStart int) bool {
-	until := upDirectiveStart
-
-	if downDirectiveStart > -1 {
-		until = min(upDirectiveStart, downDirectiveStart)
-	}
-
-	lines := strings.Split(contents[0:until], "\n")
+func statementsPrecedeMigrateBlocks(contents string, upDirectiveStart int) bool {
+	lines := strings.Split(contents[0:upDirectiveStart], "\n")
 
 	for _, line := range lines {
 		if isEmptyLine(line) || isCommentLine(line) {
@@ -186,12 +181,12 @@ func isCommentLine(s string) bool {
 	return commentLineRegExp.MatchString(s)
 }
 
-func getMatchPositions(s string, re *regexp.Regexp) (int, int, bool) {
+func getMatchPosition(s string, re *regexp.Regexp) (int, bool) {
 	match := re.FindStringIndex(s)
 	if match == nil {
-		return -1, -1, false
+		return -1, false
 	}
-	return match[0], match[1], true
+	return match[0], true
 }
 
 func substring(s string, begin, end int) string {
@@ -200,10 +195,3 @@ func substring(s string, begin, end int) string {
 	}
 	return s[begin:end]
 }
-
-func min(a, b int) int {
-	if a < b {
-		return a
-	}
-	return b
-}


Test patch:
diff --git a/pkg/dbmate/migration_test.go b/pkg/dbmate/migration_test.go
index 8e9d90d..adde73d 100644
--- a/pkg/dbmate/migration_test.go
+++ b/pkg/dbmate/migration_test.go
@@ -35,23 +35,24 @@ drop table users;
 }
 
 func TestParseMigrationContents(t *testing.T) {
-	// It supports the typical use case.
-	migration := `-- migrate:up
+	t.Run("support the typical use case", func(t *testing.T) {
+		migration := `-- migrate:up
 create table users (id serial, name text);
 -- migrate:down
 drop table users;`
 
-	parsed, err := parseMigrationContents(migration)
-	require.Nil(t, err)
+		parsed, err := parseMigrationContents(migration)
+		require.Nil(t, err)
 
-	require.Equal(t, "-- migrate:up\ncreate table users (id serial, name text);\n", parsed.Up)
-	require.Equal(t, true, parsed.UpOptions.Transaction())
+		require.Equal(t, "-- migrate:up\ncreate table users (id serial, name text);\n", parsed.Up)
+		require.Equal(t, true, parsed.UpOptions.Transaction())
 
-	require.Equal(t, "-- migrate:down\ndrop table users;", parsed.Down)
-	require.Equal(t, true, parsed.DownOptions.Transaction())
+		require.Equal(t, "-- migrate:down\ndrop table users;", parsed.Down)
+		require.Equal(t, true, parsed.DownOptions.Transaction())
+	})
 
-	// It does not require space between the '--' and 'migrate'
-	migration = `
+	t.Run("do not require space between '--' and 'migrate'", func(t *testing.T) {
+		migration := `
 --migrate:up
 create table users (id serial, name text);
 
@@ -59,58 +60,76 @@ create table users (id serial, name text);
 drop table users;
 `
 
-	parsed, err = parseMigrationContents(migration)
-	require.Nil(t, err)
+		parsed, err := parseMigrationContents(migration)
+		require.Nil(t, err)
 
-	require.Equal(t, "--migrate:up\ncreate table users (id serial, name text);\n\n", parsed.Up)
-	require.Equal(t, true, parsed.UpOptions.Transaction())
+		require.Equal(t, "--migrate:up\ncreate table users (id serial, name text);\n\n", parsed.Up)
+		require.Equal(t, true, parsed.UpOptions.Transaction())
 
-	require.Equal(t, "--migrate:down\ndrop table users;\n", parsed.Down)
-	require.Equal(t, true, parsed.DownOptions.Transaction())
+		require.Equal(t, "--migrate:down\ndrop table users;\n", parsed.Down)
+		require.Equal(t, true, parsed.DownOptions.Transaction())
+	})
 
-	// It is acceptable for down to be defined before up
-	migration = `-- migrate:down
+	t.Run("require up before down", func(t *testing.T) {
+		migration := `-- migrate:down
 drop table users;
 -- migrate:up
 create table users (id serial, name text);
 `
 
-	parsed, err = parseMigrationContents(migration)
-	require.Nil(t, err)
+		_, err := parseMigrationContents(migration)
+		require.Error(t, err, "dbmate requires '-- migrate:up' to appear before '-- migrate:down'")
+	})
 
-	require.Equal(t, "-- migrate:up\ncreate table users (id serial, name text);\n", parsed.Up)
-	require.Equal(t, true, parsed.UpOptions.Transaction())
-
-	require.Equal(t, "-- migrate:down\ndrop table users;\n", parsed.Down)
-	require.Equal(t, true, parsed.DownOptions.Transaction())
-
-	// It supports turning transactions off for a given migration block,
-	// e.g., the below would not work in Postgres inside a transaction.
-	// It also supports omitting the down block.
-	migration = `-- migrate:up transaction:false
+	t.Run("support disabling transactions", func(t *testing.T) {
+		// e.g., the below would not work in Postgres inside a transaction.
+		// It also supports omitting the down block.
+		migration := `-- migrate:up transaction:false
+ALTER TYPE colors ADD VALUE 'orange' AFTER 'red';
+-- migrate:down transaction:false
 ALTER TYPE colors ADD VALUE 'orange' AFTER 'red';
 `
 
-	parsed, err = parseMigrationContents(migration)
-	require.Nil(t, err)
+		parsed, err := parseMigrationContents(migration)
+		require.Nil(t, err)
 
-	require.Equal(t, "-- migrate:up transaction:false\nALTER TYPE colors ADD VALUE 'orange' AFTER 'red';\n", parsed.Up)
-	require.Equal(t, false, parsed.UpOptions.Transaction())
+		require.Equal(t, "-- migrate:up transaction:false\nALTER TYPE colors ADD VALUE 'orange' AFTER 'red';\n", parsed.Up)
+		require.Equal(t, false, parsed.UpOptions.Transaction())
+
+		require.Equal(t, "-- migrate:down transaction:false\nALTER TYPE colors ADD VALUE 'orange' AFTER 'red';\n", parsed.Down)
+		require.Equal(t, false, parsed.DownOptions.Transaction())
+	})
+
+	t.Run("require migrate blocks", func(t *testing.T) {
+		migration := `
+ALTER TABLE users
+ADD COLUMN status status_type DEFAULT 'active';
+`
 
-	require.Equal(t, "", parsed.Down)
-	require.Equal(t, true, parsed.DownOptions.Transaction())
+		_, err := parseMigrationContents(migration)
+		require.Error(t, err, "dbmate requires each migration to define an up block with '-- migrate:up'")
+	})
 
-	// It does *not* support omitting the up block.
-	migration = `-- migrate:down
+	t.Run("require an up block", func(t *testing.T) {
+		migration := `-- migrate:down
 drop table users;
 `
 
-	_, err = parseMigrationContents(migration)
-	require.NotNil(t, err)
-	require.Equal(t, "dbmate requires each migration to define an up block with '-- migrate:up'", err.Error())
+		_, err := parseMigrationContents(migration)
+		require.Error(t, err, "dbmate requires each migration to define an up block with '-- migrate:up'")
+	})
 
-	// It allows leading comments and whitespace preceding the migrate blocks
-	migration = `
+	t.Run("require a down block", func(t *testing.T) {
+		migration := `-- migrate:up
+create table users (id serial, name text);
+`
+
+		_, err := parseMigrationContents(migration)
+		require.Error(t, err, "dbmate requires each migration to define a down block with '-- migrate:down'")
+	})
+
+	t.Run("allow leading comments and whitespace preceding the migrate blocks", func(t *testing.T) {
+		migration := `
 -- This migration creates the users table.
 -- It'll drop it in the event of a rollback.
 
@@ -121,17 +140,18 @@ create table users (id serial, name text);
 drop table users;
 `
 
-	parsed, err = parseMigrationContents(migration)
-	require.Nil(t, err)
+		parsed, err := parseMigrationContents(migration)
+		require.Nil(t, err)
 
-	require.Equal(t, "-- migrate:up\ncreate table users (id serial, name text);\n\n", parsed.Up)
-	require.Equal(t, true, parsed.UpOptions.Transaction())
+		require.Equal(t, "-- migrate:up\ncreate table users (id serial, name text);\n\n", parsed.Up)
+		require.Equal(t, true, parsed.UpOptions.Transaction())
 
-	require.Equal(t, "-- migrate:down\ndrop table users;\n", parsed.Down)
-	require.Equal(t, true, parsed.DownOptions.Transaction())
+		require.Equal(t, "-- migrate:down\ndrop table users;\n", parsed.Down)
+		require.Equal(t, true, parsed.DownOptions.Transaction())
+	})
 
-	// It does *not* allow arbitrary statements preceding the migrate blocks
-	migration = `
+	t.Run("do not allow arbitrary statements preceding the migrate blocks", func(t *testing.T) {
+		migration := `
 -- create status_type
 CREATE TYPE status_type AS ENUM ('active', 'inactive');
 
@@ -144,17 +164,7 @@ ALTER TABLE users
 DROP COLUMN status;
 `
 
-	_, err = parseMigrationContents(migration)
-	require.NotNil(t, err)
-	require.Equal(t, "dbmate does not support statements defined outside of the '-- migrate:up' or '-- migrate:down' blocks", err.Error())
-
-	// It requires an at least an up block
-	migration = `
-ALTER TABLE users
-ADD COLUMN status status_type DEFAULT 'active';
-`
-
-	_, err = parseMigrationContents(migration)
-	require.NotNil(t, err)
-	require.Equal(t, "dbmate requires each migration to define an up block with '-- migrate:up'", err.Error())
+		_, err := parseMigrationContents(migration)
+		require.Error(t, err, "dbmate does not support statements preceding the '-- migrate:up' block")
+	})
 }