Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #331 +/- ##
===========================================
- Coverage 84.31% 58.02% -26.30%
===========================================
Files 16 16
Lines 1556 1589 +33
===========================================
- Hits 1312 922 -390
- Misses 141 608 +467
+ Partials 103 59 -44
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
Adds execution-time support for MySQL/SingleStore DELIMITER directives and introduces an opt-in migration option to preserve SQL comments so comment-only statements can be executed (primarily for test coverage).
Changes:
- Add
libschema.PreserveComments()migration option and plumb it into SQL execution. - Update Postgres tests to preserve comments for comment-only migrations.
- Add MySQL and SingleStore integration tests covering delimiter-based stored procedure migrations.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
api.go |
Adds preserveComments flag + PreserveComments() option/accessor on migrations. |
internal/mhelp/run_sql.go |
Executes statements from token lists; now conditionally strips comments and strips leading/trailing delimiter directives. |
lspostgres/non_tx_test.go |
Updates comment-only migration test to use PreserveComments(). |
lspostgres/bad_test.go |
Updates RepeatUntilNoOp/comment-only migration case to use PreserveComments(). |
lsmysql/mysql_test.go |
Adds delimiter migration test + shared helper for delimiter-based migrations. |
lsmysql/singlestore_test.go |
Adds SingleStore wrapper test to reuse the MySQL delimiter helper. |
| testMigrationWithDelimiter(t, dsn, "ENGINE = InnoDB", mysqlNew) | ||
| } | ||
|
|
||
| func testMigrationWithDelimiter(t *testing.T, dsn string, createPostfix string, driverNew driverNew) { |
There was a problem hiding this comment.
testMigrationWithDelimiter takes a createPostfix argument that is never used. This makes the helper harder to understand and maintain; either remove the parameter (and update callers) or use it in the migration SQL so the helper matches the other MySQL test helpers’ pattern.
| testMigrationWithDelimiter(t, dsn, "ENGINE = InnoDB", mysqlNew) | |
| } | |
| func testMigrationWithDelimiter(t *testing.T, dsn string, createPostfix string, driverNew driverNew) { | |
| testMigrationWithDelimiter(t, dsn, mysqlNew) | |
| } | |
| func testMigrationWithDelimiter(t *testing.T, dsn string, driverNew driverNew) { |
| t.Log("Doing migrations in database/schema", options.SchemaOverride) | ||
|
|
||
| t.Log("We will error on unknown migrations") | ||
|
|
There was a problem hiding this comment.
This helper logs "We will error on unknown migrations" but it never sets options.ErrorOnUnknownMigrations=true (unlike testMysqlHappyPath). Either set the option here or remove/update the log message so it matches actual behavior.
| options.ErrorOnUnknownMigrations = true |
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
| if !m.Base().PreserveComments() { | ||
| tokens = tokens.Strip() | ||
| } | ||
| if len(tokens) == 0 { | ||
| continue | ||
| } | ||
| if tokens[0].Type == sqltoken.DelimiterStatement { | ||
| log.Debug("Stripping leading delimiter statement from migration", map[string]any{ | ||
| "name": m.Base().Name.Name, | ||
| "library": m.Base().Name.Library, | ||
| }) | ||
| tokens = tokens[1:] | ||
| if len(tokens) == 0 { | ||
| continue | ||
| } | ||
| } | ||
| if tokens[len(tokens)-1].Type == sqltoken.DelimiterStatement { | ||
| log.Debug("Stripping trailing delimiter statement from migration", map[string]any{ | ||
| "name": m.Base().Name.Name, | ||
| "library": m.Base().Name.Library, | ||
| }) | ||
| tokens = tokens[:len(tokens)-1] | ||
| if len(tokens) == 0 { | ||
| continue | ||
| } | ||
| } |
There was a problem hiding this comment.
Delimiter statements are detected only at tokens[0]/tokens[len-1] after optional tokens.Strip(). When PreserveComments() is set, tokens are not stripped, so leading/trailing whitespace/comments can prevent detecting sqltoken.DelimiterStatement (and can even result in executing whitespace-only or delimiter-only fragments). Consider detecting delimiter statements using a stripped/trimmed view of the tokens (skip leading/trailing whitespace/comments) regardless of PreserveComments, while still executing the original tokens when comments are preserved.
| if !m.Base().PreserveComments() { | |
| tokens = tokens.Strip() | |
| } | |
| if len(tokens) == 0 { | |
| continue | |
| } | |
| if tokens[0].Type == sqltoken.DelimiterStatement { | |
| log.Debug("Stripping leading delimiter statement from migration", map[string]any{ | |
| "name": m.Base().Name.Name, | |
| "library": m.Base().Name.Library, | |
| }) | |
| tokens = tokens[1:] | |
| if len(tokens) == 0 { | |
| continue | |
| } | |
| } | |
| if tokens[len(tokens)-1].Type == sqltoken.DelimiterStatement { | |
| log.Debug("Stripping trailing delimiter statement from migration", map[string]any{ | |
| "name": m.Base().Name.Name, | |
| "library": m.Base().Name.Library, | |
| }) | |
| tokens = tokens[:len(tokens)-1] | |
| if len(tokens) == 0 { | |
| continue | |
| } | |
| } | |
| stripped := tokens.Strip() | |
| if len(stripped) == 0 { | |
| continue | |
| } | |
| if !m.Base().PreserveComments() { | |
| tokens = stripped | |
| if tokens[0].Type == sqltoken.DelimiterStatement { | |
| log.Debug("Stripping leading delimiter statement from migration", map[string]any{ | |
| "name": m.Base().Name.Name, | |
| "library": m.Base().Name.Library, | |
| }) | |
| tokens = tokens[1:] | |
| if len(tokens) == 0 { | |
| continue | |
| } | |
| } | |
| if tokens[len(tokens)-1].Type == sqltoken.DelimiterStatement { | |
| log.Debug("Stripping trailing delimiter statement from migration", map[string]any{ | |
| "name": m.Base().Name.Name, | |
| "library": m.Base().Name.Library, | |
| }) | |
| tokens = tokens[:len(tokens)-1] | |
| if len(tokens) == 0 { | |
| continue | |
| } | |
| } | |
| } else { | |
| if stripped[0].Type == sqltoken.DelimiterStatement { | |
| log.Debug("Stripping leading delimiter statement from migration", map[string]any{ | |
| "name": m.Base().Name.Name, | |
| "library": m.Base().Name.Library, | |
| }) | |
| for i, t := range tokens { | |
| if t.Type == sqltoken.DelimiterStatement { | |
| tokens = append(tokens[:i], tokens[i+1:]...) | |
| break | |
| } | |
| } | |
| if len(tokens) == 0 { | |
| continue | |
| } | |
| stripped = tokens.Strip() | |
| if len(stripped) == 0 { | |
| continue | |
| } | |
| } | |
| if stripped[len(stripped)-1].Type == sqltoken.DelimiterStatement { | |
| log.Debug("Stripping trailing delimiter statement from migration", map[string]any{ | |
| "name": m.Base().Name.Name, | |
| "library": m.Base().Name.Library, | |
| }) | |
| for i := len(tokens) - 1; i >= 0; i-- { | |
| if tokens[i].Type == sqltoken.DelimiterStatement { | |
| tokens = append(tokens[:i], tokens[i+1:]...) | |
| break | |
| } | |
| } | |
| if len(tokens) == 0 { | |
| continue | |
| } | |
| } | |
| } |
Turns out that supporting delimiter goes beyond the change in sqltoken.