Improve SET/ENUM validation#660
Conversation
There was a problem hiding this comment.
Pull request overview
This PR strengthens Spirit’s preflight validation for MySQL ENUM/SET changes to better handle reorder scenarios and to prevent unsafe type conversions in buffered migrations (per issue #618).
Changes:
- Update
enumReorderCheckandsetReorderCheckto treat non-ENUM/SETexisting column types as type conversions (not reorders), avoiding false positives. - Add a new preflight check (
enumSetRemoval) to blockENUM/SET→ non-ENUM/SETconversions in buffered mode to prevent ordinal-based corruption. - Add/extend tests covering
VARCHAR→ENUM/SETand buffered/unbuffered behavior, plus utility tests forisEnumOrSetType.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/migration/check/setreorder.go | Skip reorder validation when existing type isn’t ENUM/SET (treat as conversion). |
| pkg/migration/check/setreorder_test.go | Add coverage for VARCHAR→SET conversions not being flagged as reorders. |
| pkg/migration/check/enumreorder.go | Skip reorder validation when existing type isn’t ENUM/SET (treat as conversion). |
| pkg/migration/check/enumreorder_test.go | Add coverage for VARCHAR→ENUM conversions not being flagged as reorders. |
| pkg/migration/check/enumsetutil.go | Introduce isEnumOrSetType helper. |
| pkg/migration/check/enumsetutil_test.go | Add tests for isEnumOrSetType. |
| pkg/migration/check/enumsetremoval.go | New preflight check to block ENUM/SET→non-ENUM/SET in buffered mode. |
| pkg/migration/check/enumsetremoval_test.go | Add tests ensuring buffered conversions fail and unbuffered conversions pass. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
This PR improves Spirit’s preflight validation around ENUM/SET schema changes to prevent unsafe buffered-mode migrations and avoid unavoidable checksum failures from SET member reordering (Issue #618).
Changes:
- Refines
ENUM/SETreorder detection by extracting modified columns more generally and skipping reorder validation when the existing column wasn’tENUM/SET(e.g.,VARCHAR → ENUM/SET). - Adds a new preflight check to block
ENUM/SET → non-ENUM/SETtype conversions in buffered mode (to prevent ordinal/bitmask replay corruption). - Adds/extends test coverage for the new behaviors and documents test DB usage patterns in
AGENTS.md.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/migration/check/setreorder.go | Updates SET reorder check to identify SET via TiDB type and skip non-ENUM/SET existing-type conversions. |
| pkg/migration/check/setreorder_test.go | Adds coverage for VARCHAR → SET cases (should not be treated as reorders). |
| pkg/migration/check/enumreorder.go | Updates ENUM reorder check to identify SET via TiDB type and skip non-ENUM/SET existing-type conversions. |
| pkg/migration/check/enumreorder_test.go | Adds coverage for VARCHAR → ENUM cases (should not be treated as reorders). |
| pkg/migration/check/enumsetutil.go | Introduces isEnumOrSetType/isSetType and refactors altered-column extraction helpers. |
| pkg/migration/check/enumsetutil_test.go | Adds unit tests for isEnumOrSetType. |
| pkg/migration/check/enumsetremoval.go | New buffered-mode preflight check blocking ENUM/SET → non-ENUM/SET conversions. |
| pkg/migration/check/enumsetremoval_test.go | New integration tests for buffered vs unbuffered behavior on ENUM/SET → non-ENUM/SET conversions. |
| AGENTS.md | Updates guidance on when to use the default test DB vs unique test databases. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Improves preflight validation around MySQL SET/ENUM schema changes to better handle reorders and to prevent unsafe buffered-mode type conversions that can corrupt data or guarantee checksum failures.
Changes:
- Make
enumReorderCheck/setReorderCheckapply only when the existing column type isENUM/SETrespectively (avoids false failures on VARCHAR→ENUM/SET, etc.). - Add
enumSetRemovalCheckto block buffered-mode conversions fromENUM/SETto non-ENUM/SET, andENUM↔SETcross-conversions. - Add/extend integration tests for the new/adjusted behaviors and add small enum/set type-string helpers.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| pkg/migration/check/setreorder.go | Skip SET reorder validation when the existing column isn’t a SET (avoids false positives on cross-type conversions). |
| pkg/migration/check/setreorder_test.go | Adds coverage for VARCHAR→SET scenarios that should not be treated as reorders. |
| pkg/migration/check/enumreorder.go | Skip ENUM reorder validation when the existing column isn’t an ENUM (avoids false positives on cross-type conversions). |
| pkg/migration/check/enumreorder_test.go | Adds coverage for VARCHAR→ENUM scenarios that should not be treated as reorders. |
| pkg/migration/check/enumsetutil.go | Adds helpers to identify enum/set type strings; refactors ALTER parsing to expose all modified columns. |
| pkg/migration/check/enumsetutil_test.go | Adds tests for the new enum/set type-string helpers. |
| pkg/migration/check/enumsetremoval.go | New buffered-mode safety check preventing unsafe ENUM/SET removals and ENUM↔SET conversions. |
| pkg/migration/check/enumsetremoval_test.go | New tests covering buffered vs unbuffered behavior for unsafe conversions. |
| AGENTS.md | Updates documented testing patterns to reflect common usage in this repo. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
A Pull Request should be associated with an Issue.
Fixes #618 with a more complete fix.