Skip to content

Commit

Permalink
Stricter migration parsing
Browse files Browse the repository at this point in the history 
Require down directive and up before down.
  • Loading branch information
@amacneil
amacneil committed Feb 26, 2023
1 parent 6d5099c commit cf20cdd
Show file tree
Hide file tree
Showing 2 changed files with 100 additions and 102 deletions.
64 changes: 26 additions & 38 deletions pkg/dbmate/migration.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,8 @@ var (
// Error codes
var (
ErrParseMissingUp = errors.New("dbmate requires each migration to define an up block with '-- migrate:up'")
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 defined outside of the '-- migrate:up' or '-- migrate:down' blocks")
)

Expand All @@ -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
}
Expand All @@ -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, "")

Expand Down Expand Up @@ -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) {
Expand All @@ -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 {
Expand All @@ -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
}
138 changes: 74 additions & 64 deletions pkg/dbmate/migration_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -35,82 +35,101 @@ 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);
--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\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.
Expand All @@ -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');
Expand All @@ -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 defined outside of the '-- migrate:up' or '-- migrate:down' blocks")
})
}

0 comments on commit cf20cdd

Please sign in to comment.