From 06ff963c97e3f2bc5cbf4304b92d1da4629b1d2e Mon Sep 17 00:00:00 2001
From: Michael Fridman <mf192@icloud.com>
Date: Fri, 7 Jul 2023 14:58:39 -0400
Subject: [PATCH] fix: unterminated up statement is ignored (#558)

---
 CHANGELOG.md                                 |  4 +++-
 internal/sqlparser/parser.go                 | 16 +++++++++++++++-
 internal/sqlparser/parser_test.go            | 16 ++++++++++++++++
 internal/sqlparser/testdata/invalid/up/a.sql |  5 +++++
 internal/sqlparser/testdata/invalid/up/b.sql |  4 ++++
 internal/sqlparser/testdata/invalid/up/c.sql |  3 +++
 internal/sqlparser/testdata/invalid/up/d.sql |  2 ++
 7 files changed, 48 insertions(+), 2 deletions(-)
 create mode 100644 internal/sqlparser/testdata/invalid/up/a.sql
 create mode 100644 internal/sqlparser/testdata/invalid/up/b.sql
 create mode 100644 internal/sqlparser/testdata/invalid/up/c.sql
 create mode 100644 internal/sqlparser/testdata/invalid/up/d.sql

diff --git a/CHANGELOG.md b/CHANGELOG.md
index 9387181..76e3ef7 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -7,7 +7,9 @@ adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html).
 
 ## [Unreleased]
 
-- Fix pre-built binary versioning and make small improvements to the build process.
+- Fix pre-built binary versioning and make small improvements to GoReleaser config.
+- Fix an edge case in the `sqlparser` where the last up statement may be ignored if it's
+  unterminated and followed by a `-- +goose Down` annotation.
 
 ## [v3.13.1] - 2023-07-03
 
diff --git a/internal/sqlparser/parser.go b/internal/sqlparser/parser.go
index 369dd8d..5de637c 100644
--- a/internal/sqlparser/parser.go
+++ b/internal/sqlparser/parser.go
@@ -127,6 +127,12 @@ func ParseSQLMigration(r io.Reader, direction Direction, debug bool) (stmts []st
 			case "+goose Down":
 				switch stateMachine.get() {
 				case gooseUp, gooseStatementEndUp:
+					// If we hit a down annotation, but the buffer is not empty, we have an unfinished SQL query from a
+					// previous up annotation. This is an error, because we expect the SQL query to be terminated by a semicolon
+					// and the buffer to have been reset.
+					if bufferRemaining := strings.TrimSpace(buf.String()); len(bufferRemaining) > 0 {
+						return nil, false, missingSemicolonError(stateMachine.state, direction, bufferRemaining)
+					}
 					stateMachine.set(gooseDown)
 				default:
 					return nil, false, fmt.Errorf("must start with '-- +goose Up' annotation, stateMachine=%d, see https://github.com/pressly/goose#sql-migrations", stateMachine.state)
@@ -238,12 +244,20 @@ func ParseSQLMigration(r io.Reader, direction Direction, debug bool) (stmts []st
 	}
 
 	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.state, direction, bufferRemaining)
+		return nil, false, missingSemicolonError(stateMachine.state, direction, bufferRemaining)
 	}
 
 	return stmts, useTx, nil
 }
 
+func missingSemicolonError(state parserState, direction Direction, s string) error {
+	return fmt.Errorf("failed to parse migration: state %d, direction: %v: unexpected unfinished SQL query: %q: missing semicolon?",
+		state,
+		direction,
+		s,
+	)
+}
+
 // cleanupStatement attempts to find the last semicolon and trims
 // the remaining chars from the input string. This is useful for cleaning
 // up a statement containing trailing comments or empty lines.
diff --git a/internal/sqlparser/parser_test.go b/internal/sqlparser/parser_test.go
index 6b08115..0a7b85a 100644
--- a/internal/sqlparser/parser_test.go
+++ b/internal/sqlparser/parser_test.go
@@ -86,6 +86,22 @@ func TestSplitStatements(t *testing.T) {
 	}
 }
 
+func TestInvalidUp(t *testing.T) {
+	t.Parallel()
+
+	testdataDir := filepath.Join("testdata", "invalid", "up")
+	entries, err := os.ReadDir(testdataDir)
+	check.NoError(t, err)
+	check.NumberNotZero(t, len(entries))
+
+	for _, entry := range entries {
+		by, err := os.ReadFile(filepath.Join(testdataDir, entry.Name()))
+		check.NoError(t, err)
+		_, _, err = ParseSQLMigration(strings.NewReader(string(by)), DirectionUp, false)
+		check.HasError(t, err)
+	}
+}
+
 func TestUseTransactions(t *testing.T) {
 	t.Parallel()
 
diff --git a/internal/sqlparser/testdata/invalid/up/a.sql b/internal/sqlparser/testdata/invalid/up/a.sql
new file mode 100644
index 0000000..414866f
--- /dev/null
+++ b/internal/sqlparser/testdata/invalid/up/a.sql
@@ -0,0 +1,5 @@
+-- +goose Up
+SELECT * FROM foo;
+SELECT * FROM bar
+-- +goose Down
+SELECT * FROM baz;
diff --git a/internal/sqlparser/testdata/invalid/up/b.sql b/internal/sqlparser/testdata/invalid/up/b.sql
new file mode 100644
index 0000000..07ce74c
--- /dev/null
+++ b/internal/sqlparser/testdata/invalid/up/b.sql
@@ -0,0 +1,4 @@
+-- +goose Up
+SELECT * FROM bar
+-- +goose Down
+SELECT * FROM baz;
diff --git a/internal/sqlparser/testdata/invalid/up/c.sql b/internal/sqlparser/testdata/invalid/up/c.sql
new file mode 100644
index 0000000..5507e50
--- /dev/null
+++ b/internal/sqlparser/testdata/invalid/up/c.sql
@@ -0,0 +1,3 @@
+-- +goose Up
+SELECT * FROM bar
+-- +goose Down
diff --git a/internal/sqlparser/testdata/invalid/up/d.sql b/internal/sqlparser/testdata/invalid/up/d.sql
new file mode 100644
index 0000000..d9d02fe
--- /dev/null
+++ b/internal/sqlparser/testdata/invalid/up/d.sql
@@ -0,0 +1,2 @@
+-- +goose Up
+SELECT * FROM bar