Skip to content

Commit

Permalink
fix test race condition and remove verbose global in parser (#457)
Browse files Browse the repository at this point in the history
  • Loading branch information
@mfridman
mfridman authored Jan 28, 2023
1 parent c2f9bcb commit b4af752
Show file tree
Hide file tree
Showing 7 changed files with 96 additions and 72 deletions.
131 changes: 75 additions & 56 deletions internal/sqlparser/parser.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,20 @@ import (
"sync"
)

type Direction string

const (
DirectionUp Direction = "up"
DirectionDown Direction = "down"
)

func FromBool(b bool) Direction {
if b {
return DirectionUp
}
return DirectionDown
}

type parserState int

const (
Expand All @@ -23,15 +37,37 @@ const (
gooseStatementEndDown // 6
)

type stateMachine parserState
type stateMachine struct {
state parserState
verbose bool
}

func (s *stateMachine) Get() parserState {
return parserState(*s)
func newStateMachine(begin parserState, verbose bool) *stateMachine {
return &stateMachine{
state: begin,
verbose: verbose,
}
}

func (s *stateMachine) get() parserState {
return s.state
}

func (s *stateMachine) Set(new parserState) {
verboseInfo("StateMachine: %v => %v", *s, new)
*s = stateMachine(new)
func (s *stateMachine) set(new parserState) {
s.print("set %d => %d", s.state, new)
s.state = new
}

const (
grayColor = "\033[90m"
resetColor = "\033[00m"
)

func (s *stateMachine) print(msg string, args ...interface{}) {
msg = "StateMachine: " + msg
if s.verbose {
log.Printf(grayColor+msg+resetColor, args...)
}
}

const scanBufSize = 4 * 1024 * 1024
Expand All @@ -53,15 +89,15 @@ var bufferPool = sync.Pool{
// within a statement. For these cases, we provide the explicit annotations
// 'StatementBegin' and 'StatementEnd' to allow the script to
// tell us to ignore semicolons.
func ParseSQLMigration(r io.Reader, direction bool) (stmts []string, useTx bool, err error) {
func ParseSQLMigration(r io.Reader, direction Direction, verbose bool) (stmts []string, useTx bool, err error) {
scanBufPtr := bufferPool.Get().(*[]byte)
scanBuf := *scanBufPtr
defer bufferPool.Put(scanBufPtr)

scanner := bufio.NewScanner(r)
scanner.Buffer(scanBuf, scanBufSize)

stateMachine := stateMachine(start)
stateMachine := newStateMachine(start, verbose)
useTx = true

var buf bytes.Buffer
Expand All @@ -70,7 +106,7 @@ func ParseSQLMigration(r io.Reader, direction bool) (stmts []string, useTx bool,
if verbose {
log.Println(line)
}
if stateMachine.Get() == start && strings.TrimSpace(line) == "" {
if stateMachine.get() == start && strings.TrimSpace(line) == "" {
continue
}
// TODO(mf): validate annotations to avoid common user errors:
Expand All @@ -80,40 +116,40 @@ func ParseSQLMigration(r io.Reader, direction bool) (stmts []string, useTx bool,

switch cmd {
case "+goose Up":
switch stateMachine.Get() {
switch stateMachine.get() {
case start:
stateMachine.Set(gooseUp)
stateMachine.set(gooseUp)
default:
return nil, false, fmt.Errorf("duplicate '-- +goose Up' annotations; stateMachine=%v, see https://github.com/pressly/goose#sql-migrations", stateMachine)
return nil, false, fmt.Errorf("duplicate '-- +goose Up' annotations; stateMachine=%d, see https://github.com/pressly/goose#sql-migrations", stateMachine.state)
}
continue

case "+goose Down":
switch stateMachine.Get() {
switch stateMachine.get() {
case gooseUp, gooseStatementEndUp:
stateMachine.Set(gooseDown)
stateMachine.set(gooseDown)
default:
return nil, false, fmt.Errorf("must start with '-- +goose Up' annotation, stateMachine=%v, see https://github.com/pressly/goose#sql-migrations", stateMachine)
return nil, false, fmt.Errorf("must start with '-- +goose Up' annotation, stateMachine=%d, see https://github.com/pressly/goose#sql-migrations", stateMachine.state)
}
continue

case "+goose StatementBegin":
switch stateMachine.Get() {
switch stateMachine.get() {
case gooseUp, gooseStatementEndUp:
stateMachine.Set(gooseStatementBeginUp)
stateMachine.set(gooseStatementBeginUp)
case gooseDown, gooseStatementEndDown:
stateMachine.Set(gooseStatementBeginDown)
stateMachine.set(gooseStatementBeginDown)
default:
return nil, false, fmt.Errorf("'-- +goose StatementBegin' must be defined after '-- +goose Up' or '-- +goose Down' annotation, stateMachine=%v, see https://github.com/pressly/goose#sql-migrations", stateMachine)
return nil, false, fmt.Errorf("'-- +goose StatementBegin' must be defined after '-- +goose Up' or '-- +goose Down' annotation, stateMachine=%d, see https://github.com/pressly/goose#sql-migrations", stateMachine.state)
}
continue

case "+goose StatementEnd":
switch stateMachine.Get() {
switch stateMachine.get() {
case gooseStatementBeginUp:
stateMachine.Set(gooseStatementEndUp)
stateMachine.set(gooseStatementEndUp)
case gooseStatementBeginDown:
stateMachine.Set(gooseStatementEndDown)
stateMachine.set(gooseStatementEndDown)
default:
return nil, false, errors.New("'-- +goose StatementEnd' must be defined after '-- +goose StatementBegin', see https://github.com/pressly/goose#sql-migrations")
}
Expand All @@ -129,11 +165,11 @@ func ParseSQLMigration(r io.Reader, direction bool) (stmts []string, useTx bool,
if buf.Len() == 0 {
// This check ensures leading comments and empty lines prior to a statement are ignored.
if strings.HasPrefix(strings.TrimSpace(line), "--") || line == "" {
verboseInfo("StateMachine: ignore comment")
stateMachine.print("ignore comment")
continue
}
}
switch stateMachine.Get() {
switch stateMachine.get() {
case gooseStatementEndDown, gooseStatementEndUp:
// Do not include the "+goose StatementEnd" annotation in the final statement.
default:
Expand All @@ -147,62 +183,62 @@ func ParseSQLMigration(r io.Reader, direction bool) (stmts []string, useTx bool,
// 1) basic query with semicolon; 2) psql statement
//
// Export statement once we hit end of statement.
switch stateMachine.Get() {
switch stateMachine.get() {
case gooseUp, gooseStatementBeginUp, gooseStatementEndUp:
if !direction /*down*/ {
if direction == DirectionDown {
buf.Reset()
verboseInfo("StateMachine: ignore down")
stateMachine.print("ignore down")
continue
}
case gooseDown, gooseStatementBeginDown, gooseStatementEndDown:
if direction /*up*/ {
if direction == DirectionUp {
buf.Reset()
verboseInfo("StateMachine: ignore up")
stateMachine.print("ignore up")
continue
}
default:
return nil, false, fmt.Errorf("failed to parse migration: unexpected state %d on line %q, see https://github.com/pressly/goose#sql-migrations", stateMachine, line)
return nil, false, fmt.Errorf("failed to parse migration: unexpected state %d on line %q, see https://github.com/pressly/goose#sql-migrations", stateMachine.state, line)
}

switch stateMachine.Get() {
switch stateMachine.get() {
case gooseUp:
if endsWithSemicolon(line) {
stmts = append(stmts, cleanupStatement(buf.String()))
buf.Reset()
verboseInfo("StateMachine: store simple Up query")
stateMachine.print("store simple Up query")
}
case gooseDown:
if endsWithSemicolon(line) {
stmts = append(stmts, cleanupStatement(buf.String()))
buf.Reset()
verboseInfo("StateMachine: store simple Down query")
stateMachine.print("store simple Down query")
}
case gooseStatementEndUp:
stmts = append(stmts, cleanupStatement(buf.String()))
buf.Reset()
verboseInfo("StateMachine: store Up statement")
stateMachine.Set(gooseUp)
stateMachine.print("store Up statement")
stateMachine.set(gooseUp)
case gooseStatementEndDown:
stmts = append(stmts, cleanupStatement(buf.String()))
buf.Reset()
verboseInfo("StateMachine: store Down statement")
stateMachine.Set(gooseDown)
stateMachine.print("store Down statement")
stateMachine.set(gooseDown)
}
}
if err := scanner.Err(); err != nil {
return nil, false, fmt.Errorf("failed to scan migration: %w", err)
}
// EOF

switch stateMachine.Get() {
switch stateMachine.get() {
case start:
return nil, false, errors.New("failed to parse migration: must start with '-- +goose Up' annotation, see https://github.com/pressly/goose#sql-migrations")
case gooseStatementBeginUp, gooseStatementBeginDown:
return nil, false, errors.New("failed to parse migration: missing '-- +goose StatementEnd' annotation")
}

if bufferRemaining := strings.TrimSpace(buf.String()); len(bufferRemaining) > 0 {
return nil, false, fmt.Errorf("failed to parse migration: state %d, direction: %v: unexpected unfinished SQL query: %q: missing semicolon?", stateMachine, direction, bufferRemaining)
return nil, false, fmt.Errorf("failed to parse migration: state %d, direction: %v: unexpected unfinished SQL query: %q: missing semicolon?", stateMachine.state, direction, bufferRemaining)
}

return stmts, useTx, nil
Expand Down Expand Up @@ -240,20 +276,3 @@ func endsWithSemicolon(line string) bool {

return strings.HasSuffix(prev, ";")
}

var verbose bool

func SetVersbose(b bool) {
verbose = b
}

const (
grayColor = "\033[90m"
resetColor = "\033[00m"
)

func verboseInfo(s string, args ...interface{}) {
if verbose {
log.Printf(grayColor+s+resetColor, args...)
}
}
20 changes: 14 additions & 6 deletions internal/sqlparser/parser_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,15 @@ import (
"github.com/pressly/goose/v3/internal/check"
)

var (
debug = false
)

func TestMain(m *testing.M) {
debug, _ = strconv.ParseBool(os.Getenv("DEBUG_TEST"))
os.Exit(m.Run())
}

func TestSemicolons(t *testing.T) {
t.Parallel()

Expand Down Expand Up @@ -38,7 +47,6 @@ func TestSemicolons(t *testing.T) {

func TestSplitStatements(t *testing.T) {
t.Parallel()
// SetVerbose(true)

type testData struct {
sql string
Expand All @@ -59,7 +67,7 @@ func TestSplitStatements(t *testing.T) {

for i, test := range tt {
// up
stmts, _, err := ParseSQLMigration(strings.NewReader(test.sql), true)
stmts, _, err := ParseSQLMigration(strings.NewReader(test.sql), DirectionUp, debug)
if err != nil {
t.Error(fmt.Errorf("tt[%v] unexpected error: %w", i, err))
}
Expand All @@ -68,7 +76,7 @@ func TestSplitStatements(t *testing.T) {
}

// down
stmts, _, err = ParseSQLMigration(strings.NewReader(test.sql), false)
stmts, _, err = ParseSQLMigration(strings.NewReader(test.sql), DirectionDown, debug)
if err != nil {
t.Error(fmt.Errorf("tt[%v] unexpected error: %w", i, err))
}
Expand Down Expand Up @@ -97,7 +105,7 @@ func TestUseTransactions(t *testing.T) {
if err != nil {
t.Error(err)
}
_, useTx, err := ParseSQLMigration(f, true)
_, useTx, err := ParseSQLMigration(f, DirectionUp, debug)
if err != nil {
t.Error(err)
}
Expand All @@ -117,7 +125,7 @@ func TestParsingErrors(t *testing.T) {
downFirst,
}
for i, sql := range tt {
_, _, err := ParseSQLMigration(strings.NewReader(sql), true)
_, _, err := ParseSQLMigration(strings.NewReader(sql), DirectionUp, debug)
if err == nil {
t.Errorf("expected error on tt[%v] %q", i, sql)
}
Expand Down Expand Up @@ -386,7 +394,7 @@ func testValidUp(t *testing.T, dir string, count int) {
f, err := os.Open(filepath.Join(dir, "input.sql"))
check.NoError(t, err)
t.Cleanup(func() { f.Close() })
statements, _, err := ParseSQLMigration(f, true)
statements, _, err := ParseSQLMigration(f, DirectionUp, debug)
check.NoError(t, err)
check.Number(t, len(statements), count)
compareStatements(t, dir, statements)
Expand Down
3 changes: 1 addition & 2 deletions migration.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -61,8 +61,7 @@ func (m *Migration) run(db *sql.DB, direction bool) error {
}
defer f.Close()

sqlparser.SetVersbose(verbose)
statements, useTx, err := sqlparser.ParseSQLMigration(f, direction)
statements, useTx, err := sqlparser.ParseSQLMigration(f, sqlparser.FromBool(direction), verbose)
if err != nil {
return fmt.Errorf("ERROR %v: failed to parse SQL migration file: %w", filepath.Base(m.Source), err)
}
Expand Down
2 changes: 0 additions & 2 deletions tests/e2e/allow_missing_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -321,8 +321,6 @@ func setupTestDB(t *testing.T, version int64) *sql.DB {
db, err := newDockerDB(t)
check.NoError(t, err)

check.NoError(t, goose.SetDialect(*dialect))

// Create goose table.
current, err := goose.EnsureDBVersion(db)
check.NoError(t, err)
Expand Down
6 changes: 6 additions & 0 deletions tests/e2e/main_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ import (
"syscall"
"testing"

"github.com/pressly/goose/v3"
"github.com/pressly/goose/v3/internal/check"
"github.com/pressly/goose/v3/internal/testdb"
)
Expand Down Expand Up @@ -73,6 +74,11 @@ func TestMain(m *testing.M) {
migrationsDir = filepath.Join("testdata", *dialect, "migrations")
seedDir = filepath.Join("testdata", *dialect, "seed")

if err := goose.SetDialect(*dialect); err != nil {
log.Printf("failed to set dialect %q: %v\n", *dialect, err)
os.Exit(1)
}

exitCode := m.Run()
// Useful for debugging test services.
if *debug {
Expand Down
Loading

0 comments on commit b4af752

Please sign in to comment.