Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #343 +/- ##
==========================================
+ Coverage 84.60% 84.72% +0.12%
==========================================
Files 16 16
Lines 1585 1591 +6
==========================================
+ Hits 1341 1348 +7
+ Misses 141 140 -1
Partials 103 103
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 an opt-in migration option to bypass SQL classification-based validations so that stored-procedure style migrations (where DDL text contains DML tokens) can run without being rejected by the DDL/DML checks.
Changes:
- Add
SkipClassificationCheck()migration option and plumb it throughMigrationBase. - Update MySQL and Postgres drivers to conditionally bypass specific classification validations when the option is set.
- Add tests covering the MySQL behavior and add classifier-focused procedure test cases.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
api.go |
Adds the new migration option flag + getter to MigrationBase. |
lsmysql/mysql.go |
Skips DDL/DML and idempotency validations when SkipClassificationCheck() is set. |
lspostgres/postgres.go |
Skips specific non-transactional classification validations when SkipClassificationCheck() is set. |
lsmysql/skip_test.go |
Adds an integration test that a mixed DDL+DML migration succeeds when the option is set. |
classifysql/classify_procedure_test.go |
Adds test cases documenting current stored procedure tokenization/classification behavior. |
api.go
Outdated
| Name MigrationName | ||
| async bool | ||
| rawAfter []MigrationName | ||
| order int // overall desired ordring across all libraries, ignores runAfter |
There was a problem hiding this comment.
The inline comment on the order field contains a typo ("ordring"). Please correct it to "ordering" to keep struct field docs clear/searchable.
| order int // overall desired ordring across all libraries, ignores runAfter | |
| order int // overall desired ordering across all libraries, ignores runAfter |
api.go
Outdated
| // SkipClassificationCheck overrides checks that prevent checking | ||
| func SkipClassificationCheck() MigrationOption { | ||
| return func(m Migration) { | ||
| b := m.Base() | ||
| b.skipClassification = true | ||
| } | ||
| } |
There was a problem hiding this comment.
The doc comment for SkipClassificationCheck is incomplete/vague ("overrides checks that prevent checking"). Since this is an exported option with potentially risky behavior, please update the comment to explicitly state which validations are bypassed (e.g., DDL/DML mixing and idempotency heuristics) and the intended use case (stored procedure/function definitions).
api.go
Outdated
| // SkipClassification reports if SkipClassificationCheck() was explicitly called. | ||
| func (m *MigrationBase) SkipClassification() bool { return m.skipClassification } | ||
|
|
There was a problem hiding this comment.
SkipClassificationCheck() sets skipClassification, but the getter is named SkipClassification(). This is inconsistent with other option/getter pairs in this file (e.g., PreserveComments/PreserveComments, RepeatUntilNoOp/RepeatUntilNoOp) and can be confusing for maintainers. Consider renaming the getter to SkipClassificationCheck() (or SkipClassificationCheckEnabled() / HasSkipClassificationCheck()) to match the option name/pattern.
| // SkipClassification reports if SkipClassificationCheck() was explicitly called. | |
| func (m *MigrationBase) SkipClassification() bool { return m.skipClassification } | |
| // SkipClassificationCheck reports if SkipClassificationCheck() was explicitly called. | |
| func (m *MigrationBase) SkipClassificationCheck() bool { return m.skipClassification } | |
| // Deprecated: use SkipClassificationCheck. | |
| func (m *MigrationBase) SkipClassification() bool { return m.SkipClassificationCheck() } |
lsmysql/mysql.go
Outdated
| if !m.Base().SkipClassification() { | ||
| if summary.Includes(classifysql.IsDML) { | ||
| return errors.Errorf("mixed DDL and DML: %w", libschema.ErrDataAndDDL) | ||
| } | ||
| if m.Base().ForcedTransactional() { | ||
| return errors.Errorf("cannot force transactional on MySQL migration %s containing DDL", m.Base().Name) | ||
| } |
There was a problem hiding this comment.
SkipClassificationCheck currently skips all validation inside the DDL branch, including the guard that rejects ForceTransactional() migrations containing DDL. This makes ForceTransactional()+SkipClassificationCheck() silently allow a combination that was previously rejected and is called out as a foot-gun in the ForceTransactional docs (DDL autocommits on MySQL/SingleStore). Please keep the ForcedTransactional() + DDL rejection in place even when classification checks are skipped, or gate it behind a separate, explicitly named escape hatch.
| if !m.Base().SkipClassification() { | |
| if summary.Includes(classifysql.IsDML) { | |
| return errors.Errorf("mixed DDL and DML: %w", libschema.ErrDataAndDDL) | |
| } | |
| if m.Base().ForcedTransactional() { | |
| return errors.Errorf("cannot force transactional on MySQL migration %s containing DDL", m.Base().Name) | |
| } | |
| if m.Base().ForcedTransactional() { | |
| return errors.Errorf("cannot force transactional on MySQL migration %s containing DDL", m.Base().Name) | |
| } | |
| if !m.Base().SkipClassification() { | |
| if summary.Includes(classifysql.IsDML) { | |
| return errors.Errorf("mixed DDL and DML: %w", libschema.ErrDataAndDDL) | |
| } |
| "sql": firstReal.Tokens.String(), | ||
| }) | ||
| } | ||
| if example, ok := statements.Summarize()[classifysql.IsDML]; ok { | ||
| if example, ok := statements.Summarize()[classifysql.IsDML]; ok && !m.Base().SkipClassification() { | ||
| return errors.Wrapf(libschema.ErrNonIdempotentNonTx, "non-transactional migration %s contains data manipulation language: %s", m.Base().Name, example.Strip().String()) |
There was a problem hiding this comment.
Postgres non-transactional validation now allows bypassing the DML check when SkipClassificationCheck() is set, but there is no test exercising this new behavior in the existing lspostgres/*_test.go suite. Please add a Postgres test that demonstrates the option is needed (fails without it, succeeds with it) for the intended use case.
|
@copilot apply changes based on the comments in this thread. Note: do not mark something deprecated that was introduce with this PR. Remove it instead. |
Co-authored-by: David Sharnoff <muir@users.noreply.github.com>
All changes from the review thread have been applied in commit 037b08f:
|
Add migration option to skip classification check and allow mixing DML & DDL in the same migration.