Repository: amacneil/dbmate
Pull request #72 (2019-05-20 19:28:12)

Require presence of up block; optional space after --
This fixes panics that occur under two conditions when parsing a migration file:

* There is no space between `--` and `migrate:[up|down]`
* Neither an up or down migration block was defined

Both the previous implementation (before options were introduced in #68) and existing code would not run migrations under those two circumstances. However, the difference is that previously dbmate silently did nothing and now the code panics. This PR fixes the panics in the following ways:

* Allow 0 or more spaces between `--` and `migrate:[up|down]`
* Explicitly require at least an up block to be defined and fail with a sensical error message when the up block is omitted

To be clear, the fixes in this PR introduce new behavior: 1) allowing something that previously would silently no-op to succeed and 2) explicitly failing with an error message when no up block was defined. This behavior is desired, but I wanted to at least call out the behavior changes as it potentially has consequences on existing migrations (experienced this in my team's application).


Patch:
diff --git a/README.md b/README.md
index 947012f..c3adcc4 100644
--- a/README.md
+++ b/README.md
@@ -218,10 +218,9 @@ dbmate supports options passed to a migration block in the form of `key:value` p
 ```sql
 -- migrate:up transaction:false
 ALTER TYPE colors ADD VALUE 'orange' AFTER 'red';
-ALTER TYPE colors ADD VALUE 'yellow' AFTER 'orange';
 ```
 
-`transaction` will default to `true` for if your database supports it.
+`transaction` will default to `true` if your database supports it.
 
 ### Schema File
 
diff --git a/pkg/dbmate/migrations.go b/pkg/dbmate/migrations.go
index 526280d..fb96d64 100644
--- a/pkg/dbmate/migrations.go
+++ b/pkg/dbmate/migrations.go
@@ -1,6 +1,7 @@
 package dbmate
 
 import (
+	"fmt"
 	"io/ioutil"
 	"regexp"
 	"strings"
@@ -36,37 +37,33 @@ func parseMigration(path string) (Migration, Migration, error) {
 	if err != nil {
 		return NewMigration(), NewMigration(), err
 	}
-	up, down := parseMigrationContents(string(data))
-	return up, down, nil
+	up, down, err := parseMigrationContents(string(data))
+	return up, down, err
 }
 
-var upRegExp = regexp.MustCompile(`(?m)^-- migrate:up\s+(.+)*$`)
-var downRegExp = regexp.MustCompile(`(?m)^-- migrate:down\s+(.+)*$`)
+var upRegExp = regexp.MustCompile(`(?m)^--\s*migrate:up\s+(.+)*$`)
+var downRegExp = regexp.MustCompile(`(?m)^--\s*migrate:down\s+(.+)*$`)
 
 // parseMigrationContents parses the string contents of a migration.
 // It will return two Migration objects, the first representing the "up"
-// block and the second representing the "down" block.
-//
-// Note that with the way this is currently defined, it is possible to
-// correctly parse a migration that does not define an "up" block or a
-// "down" block, or one that defines neither. This behavior is, in part,
-// to preserve backwards compatibility.
-func parseMigrationContents(contents string) (Migration, Migration) {
+// block and the second representing the "down" block. This function
+// requires that at least an up block was defined and will otherwise
+// return an error.
+func parseMigrationContents(contents string) (Migration, Migration, error) {
 	up := NewMigration()
 	down := NewMigration()
 
 	upMatch := upRegExp.FindStringSubmatchIndex(contents)
 	downMatch := downRegExp.FindStringSubmatchIndex(contents)
 
+	didNotDefineUpBlock := len(upMatch) == 0
 	onlyDefinedUpBlock := len(upMatch) != 0 && len(downMatch) == 0
-	onlyDefinedDownBlock := len(upMatch) == 0 && len(downMatch) != 0
 
-	if onlyDefinedUpBlock {
+	if didNotDefineUpBlock {
+		return up, down, fmt.Errorf("dbmate requires each migration to define an up bock with '-- migrate:up'")
+	} else if onlyDefinedUpBlock {
 		up.Contents = strings.TrimSpace(contents)
 		up.Options = parseMigrationOptions(contents, upMatch[2], upMatch[3])
-	} else if onlyDefinedDownBlock {
-		down.Contents = strings.TrimSpace(contents)
-		down.Options = parseMigrationOptions(contents, downMatch[2], downMatch[3])
 	} else {
 		upStart := upMatch[0]
 		downStart := downMatch[0]
@@ -87,7 +84,7 @@ func parseMigrationContents(contents string) (Migration, Migration) {
 		down.Options = parseMigrationOptions(contents, downMatch[2], downMatch[3])
 	}
 
-	return up, down
+	return up, down, nil
 }
 
 var whitespaceRegExp = regexp.MustCompile(`\s+`)


Test patch:
diff --git a/pkg/dbmate/migrations_test.go b/pkg/dbmate/migrations_test.go
index bbb46c6..967846f 100644
--- a/pkg/dbmate/migrations_test.go
+++ b/pkg/dbmate/migrations_test.go
@@ -7,12 +7,14 @@ import (
 )
 
 func TestParseMigrationContents(t *testing.T) {
+	// It supports the typical use case.
 	migration := `-- migrate:up
 create table users (id serial, name text);
 -- migrate:down
 drop table users;`
 
-	up, down := parseMigrationContents(migration)
+	up, down, err := parseMigrationContents(migration)
+	require.Nil(t, err)
 
 	require.Equal(t, "-- migrate:up\ncreate table users (id serial, name text);", up.Contents)
 	require.Equal(t, true, up.Options.Transaction())
@@ -20,13 +22,33 @@ drop table users;`
 	require.Equal(t, "-- migrate:down\ndrop table users;", down.Contents)
 	require.Equal(t, true, down.Options.Transaction())
 
+	// It does not require space between the '--' and 'migrate'
+	migration = `
+--migrate:up
+create table users (id serial, name text);
+
+--migrate:down
+drop table users;
+`
+
+	up, down, err = parseMigrationContents(migration)
+	require.Nil(t, err)
+
+	require.Equal(t, "--migrate:up\ncreate table users (id serial, name text);", up.Contents)
+	require.Equal(t, true, up.Options.Transaction())
+
+	require.Equal(t, "--migrate:down\ndrop table users;", down.Contents)
+	require.Equal(t, true, down.Options.Transaction())
+
+	// It is acceptable for down to be defined before up
 	migration = `-- migrate:down
 drop table users;
 -- migrate:up
 create table users (id serial, name text);
 `
 
-	up, down = parseMigrationContents(migration)
+	up, down, err = parseMigrationContents(migration)
+	require.Nil(t, err)
 
 	require.Equal(t, "-- migrate:up\ncreate table users (id serial, name text);", up.Contents)
 	require.Equal(t, true, up.Options.Transaction())
@@ -34,19 +56,30 @@ create table users (id serial, name text);
 	require.Equal(t, "-- migrate:down\ndrop table users;", down.Contents)
 	require.Equal(t, true, down.Options.Transaction())
 
-	// This migration would not work in Postgres if it were to
-	// run in a transaction, so we would want to disable transactions.
+	// 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
 ALTER TYPE colors ADD VALUE 'orange' AFTER 'red';
-ALTER TYPE colors ADD VALUE 'yellow' AFTER 'orange';
 `
 
-	up, down = parseMigrationContents(migration)
+	up, down, err = parseMigrationContents(migration)
+	require.Nil(t, err)
 
-	require.Equal(t, "-- migrate:up transaction:false\nALTER TYPE colors ADD VALUE 'orange' AFTER 'red';\nALTER TYPE colors ADD VALUE 'yellow' AFTER 'orange';", up.Contents)
+	require.Equal(t, "-- migrate:up transaction:false\nALTER TYPE colors ADD VALUE 'orange' AFTER 'red';", up.Contents)
 	require.Equal(t, false, up.Options.Transaction())
 
 	require.Equal(t, "", down.Contents)
 	require.Equal(t, true, down.Options.Transaction())
 
+	// It does *not* support omitting the up block.
+	migration = `-- drop users table
+begin;
+drop table users;
+commit;
+`
+
+	_, _, err = parseMigrationContents(migration)
+	require.NotNil(t, err)
+	require.Equal(t, "dbmate requires each migration to define an up bock with '-- migrate:up'", err.Error())
 }