Fail statements precede up/down migration blocks (#77)

pkg/dbmate/migrations.go

@@ -41,8 +41,13 @@ func parseMigration(path string) (Migration, Migration, error) {
 	return up, down, err
 }
 
-var upRegExp = regexp.MustCompile(`(?m)^--\s*migrate:up\s+(.+)*$`)
+var upRegExp = regexp.MustCompile(`(?m)^--\s*migrate:up(\s*$|\s+\S+)`)
-var downRegExp = regexp.MustCompile(`(?m)^--\s*migrate:down\s+(.+)*$`)
+var downRegExp = regexp.MustCompile(`(?m)^--\s*migrate:down(\s*$|\s+\S+)$`)
+var emptyLineRegExp = regexp.MustCompile(`^\s*$`)
+var commentLineRegExp = regexp.MustCompile(`^\s*--`)
+var whitespaceRegExp = regexp.MustCompile(`\s+`)
+var optionSeparatorRegExp = regexp.MustCompile(`:`)
+var blockDirectiveRegExp = regexp.MustCompile(`^--\s*migrate:[up|down]]`)
 
 // parseMigrationContents parses the string contents of a migration.
 // It will return two Migration objects, the first representing the "up"
@@ -53,68 +58,143 @@ func parseMigrationContents(contents string) (Migration, Migration, error) {
 	up := NewMigration()
 	down := NewMigration()
 
-	upMatch := upRegExp.FindStringSubmatchIndex(contents)
+	upDirectiveStart, upDirectiveEnd, hasDefinedUpBlock := getMatchPositions(contents, upRegExp)
-	downMatch := downRegExp.FindStringSubmatchIndex(contents)
+	downDirectiveStart, downDirectiveEnd, hasDefinedDownBlock := getMatchPositions(contents, downRegExp)
 
-	didNotDefineUpBlock := len(upMatch) == 0
+	if !hasDefinedUpBlock {
-	onlyDefinedUpBlock := len(upMatch) != 0 && len(downMatch) == 0
-
-	if didNotDefineUpBlock {
 		return up, down, fmt.Errorf("dbmate requires each migration to define an up bock with '-- migrate:up'")
-	} else if onlyDefinedUpBlock {
+	} else if statementsPrecedeMigrateBlocks(contents, upDirectiveStart, downDirectiveStart) {
-		up.Contents = strings.TrimSpace(contents)
+		return up, down, fmt.Errorf("dbmate does not support statements defined outside of the '-- migrate:up' or '-- migrate:down' blocks")
-		up.Options = parseMigrationOptions(contents, upMatch[2], upMatch[3])
+	}
-	} else {
-		upStart := upMatch[0]
-		downStart := downMatch[0]
 
-		upEnd := downMatch[0]
+	upEnd := len(contents)
 	downEnd := len(contents)
 
-		// If migrate:down was defined above migrate:up, correct the end indices
+	if hasDefinedDownBlock && upDirectiveStart < downDirectiveStart {
-		if upMatch[0] > downMatch[0] {
+		upEnd = downDirectiveStart
-			upEnd = downEnd
+	} else if hasDefinedDownBlock && upDirectiveStart > downDirectiveStart {
-			downEnd = upMatch[0]
+		downEnd = upDirectiveStart
+	} else {
+		downEnd = -1
 	}
 
-		up.Contents = strings.TrimSpace(contents[upStart:upEnd])
+	upDirective := substring(contents, upDirectiveStart, upDirectiveEnd)
-		up.Options = parseMigrationOptions(contents, upMatch[2], upMatch[3])
+	downDirective := substring(contents, downDirectiveStart, downDirectiveEnd)
 
-		down.Contents = strings.TrimSpace(contents[downStart:downEnd])
+	up.Options = parseMigrationOptions(upDirective)
-		down.Options = parseMigrationOptions(contents, downMatch[2], downMatch[3])
+	up.Contents = substring(contents, upDirectiveStart, upEnd)
-	}
+
+	down.Options = parseMigrationOptions(downDirective)
+	down.Contents = substring(contents, downDirectiveStart, downEnd)
 
 	return up, down, nil
 }
 
-var whitespaceRegExp = regexp.MustCompile(`\s+`)
+// parseMigrationOptions parses the migration options out of a block
-var optionSeparatorRegExp = regexp.MustCompile(`:`)
+// directive into an object that implements the MigrationOptions interface.
-
-// parseMigrationOptions parses the options portion of a migration
-// block into an object that satisfies the MigrationOptions interface,
-// i.e., the 'transaction:false' piece of the following:
 //
-//     -- migrate:up transaction:false
+// For example:
-//     create table users (id serial, name string);
-//     -- migrate:down
-//     drop table users;
 //
-func parseMigrationOptions(contents string, begin, end int) MigrationOptions {
+//     fmt.Printf("%#v", parseMigrationOptions("-- migrate:up transaction:false"))
-	mOpts := make(migrationOptions)
+//     // migrationOptions{"transaction": "false"}
+//
+func parseMigrationOptions(contents string) MigrationOptions {
+	options := make(migrationOptions)
 
-	if begin == -1 || end == -1 {
+	// strip away the -- migrate:[up|down] part
-		return mOpts
+	contents = blockDirectiveRegExp.ReplaceAllString(contents, "")
+
+	// remove leading and trailing whitespace
+	contents = strings.TrimSpace(contents)
+
+	// return empty options if nothing is left to parse
+	if contents == "" {
+		return options
 	}
 
-	optionsString := strings.TrimSpace(contents[begin:end])
+	// split the options string into pairs, e.g. "transaction:false foo:bar" -> []string{"transaction:false", "foo:bar"}
+	stringPairs := whitespaceRegExp.Split(contents, -1)
 
-	optionGroups := whitespaceRegExp.Split(optionsString, -1)
+	for _, stringPair := range stringPairs {
-	for _, group := range optionGroups {
+		// split stringified pair into key and value pairs, e.g. "transaction:false" -> []string{"transaction", "false"}
-		pair := optionSeparatorRegExp.Split(group, -1)
+		pair := optionSeparatorRegExp.Split(stringPair, -1)
+
+		// if the syntax is well-formed, then store the key and value pair in options
 		if len(pair) == 2 {
-			mOpts[pair[0]] = pair[1]
+			options[pair[0]] = pair[1]
 		}
 	}
 
-	return mOpts
+	return options
+}
+
+// statementsPrecedeMigrateBlocks inspects the contents between the first character
+// of a string and the index of the first block directive to see if there are any statements
+// defined outside of the block directive. It'll return true if it finds any such statements.
+//
+// For example:
+//
+// This will return false:
+//
+// statementsPrecedeMigrateBlocks(`-- migrate:up
+// create table users (id serial);
+// `, 0, -1)
+//
+// This will return true:
+//
+// statementsPrecedeMigrateBlocks(`create type status_type as enum('active', 'inactive');
+// -- 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")
+
+	for _, line := range lines {
+		if isEmptyLine(line) || isCommentLine(line) {
+			continue
+		}
+		return true
+	}
+
+	return false
+}
+
+// isEmptyLine will return true if the line has no
+// characters or if all the characters are whitespace characters
+func isEmptyLine(s string) bool {
+	return emptyLineRegExp.MatchString(s)
+}
+
+// isCommentLine will return true if the line is a SQL comment
+func isCommentLine(s string) bool {
+	return commentLineRegExp.MatchString(s)
+}
+
+func getMatchPositions(s string, re *regexp.Regexp) (int, int, bool) {
+	match := re.FindStringIndex(s)
+	if match == nil {
+		return -1, -1, false
+	}
+	return match[0], match[1], true
+}
+
+func substring(s string, begin, end int) string {
+	if begin == -1 || end == -1 {
+		return ""
+	}
+	return s[begin:end]
+}
+
+func min(a, b int) int {
+	if a < b {
+		return a
+	}
+	return b
 }

pkg/dbmate/migrations_test.go

@@ -16,7 +16,7 @@ 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, "-- migrate:up\ncreate table users (id serial, name text);\n", up.Contents)
 	require.Equal(t, true, up.Options.Transaction())
 
 	require.Equal(t, "-- migrate:down\ndrop table users;", down.Contents)
@@ -34,10 +34,10 @@ 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, "--migrate:up\ncreate table users (id serial, name text);\n\n", up.Contents)
 	require.Equal(t, true, up.Options.Transaction())
 
-	require.Equal(t, "--migrate:down\ndrop table users;", down.Contents)
+	require.Equal(t, "--migrate:down\ndrop table users;\n", down.Contents)
 	require.Equal(t, true, down.Options.Transaction())
 
 	// It is acceptable for down to be defined before up
@@ -50,10 +50,10 @@ create table users (id serial, name text);
 	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, "-- migrate:up\ncreate table users (id serial, name text);\n", up.Contents)
 	require.Equal(t, true, up.Options.Transaction())
 
-	require.Equal(t, "-- migrate:down\ndrop table users;", down.Contents)
+	require.Equal(t, "-- migrate:down\ndrop table users;\n", down.Contents)
 	require.Equal(t, true, down.Options.Transaction())
 
 	// It supports turning transactions off for a given migration block,
@@ -66,17 +66,64 @@ ALTER TYPE colors ADD VALUE 'orange' AFTER 'red';
 	up, down, err = parseMigrationContents(migration)
 	require.Nil(t, err)
 
-	require.Equal(t, "-- migrate:up transaction:false\nALTER TYPE colors ADD VALUE 'orange' AFTER 'red';", up.Contents)
+	require.Equal(t, "-- migrate:up transaction:false\nALTER TYPE colors ADD VALUE 'orange' AFTER 'red';\n", 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
+	migration = `-- migrate:down
-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())
+
+	// It allows leading comments and whitespace preceding the migrate blocks
+	migration = `
+-- This migration creates the users table.
+-- It'll drop it in the event of a rollback.
+
+-- 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);\n\n", up.Contents)
+	require.Equal(t, true, up.Options.Transaction())
+
+	require.Equal(t, "-- migrate:down\ndrop table users;\n", down.Contents)
+	require.Equal(t, true, down.Options.Transaction())
+
+	// It does *not* allow arbitrary statements preceding the migrate blocks
+	migration = `
+-- create status_type
+CREATE TYPE status_type AS ENUM ('active', 'inactive');
+
+-- migrate:up
+ALTER TABLE users
+ADD COLUMN status status_type DEFAULT 'active';
+
+-- migrate:down
+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)