-
-
Notifications
You must be signed in to change notification settings - Fork 301
Fix MySQL native ENUM autogenerate detection #1746
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Open
furkankoykiran
wants to merge
3
commits into
sqlalchemy:main
Choose a base branch
from
furkankoykiran:fix/mysql-native-enum-autogenerate
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -11,6 +11,7 @@ | |
|
|
||
| from sqlalchemy import schema | ||
| from sqlalchemy import types as sqltypes | ||
| from sqlalchemy.dialects.mysql import ENUM as MySQL_ENUM | ||
| from sqlalchemy.sql import elements | ||
| from sqlalchemy.sql import functions | ||
| from sqlalchemy.sql import operators | ||
|
|
@@ -348,6 +349,56 @@ def correct_for_autogen_foreignkeys(self, conn_fks, metadata_fks): | |
| ): | ||
| cnfk.onupdate = "RESTRICT" | ||
|
|
||
| def compare_type( | ||
| self, | ||
| inspector_column: schema.Column[Any], | ||
| metadata_column: schema.Column, | ||
| ) -> bool: | ||
| """Override compare_type to properly detect MySQL native ENUM changes. | ||
|
|
||
| This addresses the issue where autogenerate fails to detect when new | ||
| values are added to or removed from MySQL native ENUM columns. | ||
| """ | ||
| # Check if both columns are MySQL native ENUMs | ||
| metadata_type = metadata_column.type | ||
| inspector_type = inspector_column.type | ||
|
|
||
| if isinstance( | ||
| metadata_type, (sqltypes.Enum, MySQL_ENUM) | ||
zzzeek marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| ) and isinstance(inspector_type, (sqltypes.Enum, MySQL_ENUM)): | ||
| # For native ENUMs, compare the actual enum values | ||
| metadata_enums = None | ||
| inspector_enums = None | ||
|
|
||
| # Extract enum values from metadata column | ||
| if isinstance(metadata_type, sqltypes.Enum): | ||
|
||
| if hasattr(metadata_type, "enums"): | ||
zzzeek marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| metadata_enums = metadata_type.enums | ||
| elif isinstance(metadata_type, MySQL_ENUM): | ||
zzzeek marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| if hasattr(metadata_type, "enums"): | ||
| metadata_enums = metadata_type.enums | ||
|
|
||
| # Extract enum values from inspector column | ||
| if isinstance(inspector_type, sqltypes.Enum): | ||
zzzeek marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| if hasattr(inspector_type, "enums"): | ||
|
||
| inspector_enums = inspector_type.enums | ||
| elif isinstance(inspector_type, MySQL_ENUM): | ||
zzzeek marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| if hasattr(inspector_type, "enums"): | ||
| inspector_enums = inspector_type.enums | ||
|
|
||
| # Compare enum values if both are available | ||
| if metadata_enums is not None and inspector_enums is not None: | ||
zzzeek marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| # Convert to tuples to preserve order | ||
| # (important for MySQL ENUMs) | ||
| metadata_values = tuple(metadata_enums) | ||
zzzeek marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| inspector_values = tuple(inspector_enums) | ||
|
|
||
| if metadata_values != inspector_values: | ||
| return True | ||
|
|
||
| # Fall back to default comparison for non-ENUM types | ||
| return super().compare_type(inspector_column, metadata_column) | ||
|
|
||
|
|
||
| class MariaDBImpl(MySQLImpl): | ||
| __dialect__ = "mariadb" | ||
|
|
||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,225 @@ | ||
| # mypy: allow-untyped-defs, allow-incomplete-defs, allow-untyped-calls | ||
| # mypy: no-warn-return-any, allow-any-generics | ||
zzzeek marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
|
|
||
| """Tests for MySQL native ENUM autogenerate detection. | ||
| This addresses the bug where Alembic's autogenerate fails to detect | ||
| when new values are added to or removed from MySQL native ENUM columns. | ||
| """ | ||
|
|
||
| from sqlalchemy import Column | ||
| from sqlalchemy import Enum | ||
| from sqlalchemy import Integer | ||
| from sqlalchemy import MetaData | ||
| from sqlalchemy import Table | ||
| from sqlalchemy.dialects.mysql import ENUM as MySQL_ENUM | ||
|
|
||
| from alembic import autogenerate | ||
| from alembic.migration import MigrationContext | ||
| from alembic.testing import combinations | ||
| from alembic.testing import config | ||
| from alembic.testing.fixtures import TestBase | ||
|
|
||
|
|
||
| class MySQLEnumTest(TestBase): | ||
zzzeek marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| """Test MySQL native ENUM comparison in autogenerate.""" | ||
|
|
||
| __only_on__ = "mysql", "mariadb" | ||
| __backend__ = True | ||
|
|
||
| def setUp(self): | ||
zzzeek marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| self.bind = config.db | ||
| self.metadata = MetaData() | ||
|
|
||
| def tearDown(self): | ||
| with config.db.begin() as conn: | ||
| self.metadata.drop_all(conn) | ||
|
|
||
| def _get_autogen_context(self, bind, metadata): | ||
| """Helper to create an autogenerate context.""" | ||
| migration_ctx = MigrationContext.configure( | ||
| connection=bind, | ||
| opts={"target_metadata": metadata, "compare_type": True}, | ||
| ) | ||
| return autogenerate.api.AutogenContext(migration_ctx, metadata) | ||
|
|
||
| @combinations(("backend",)) | ||
| def test_enum_value_added(self): | ||
| """Test that adding a value to ENUM is detected.""" | ||
| # Create initial table with ENUM | ||
| Table( | ||
| "test_enum_table", | ||
| self.metadata, | ||
| Column("id", Integer, primary_key=True), | ||
| Column("status", Enum("A", "B", "C", native_enum=True)), | ||
| ) | ||
|
|
||
| with self.bind.begin() as conn: | ||
| self.metadata.create_all(conn) | ||
|
|
||
| # Create modified metadata with additional ENUM value | ||
| m2 = MetaData() | ||
| Table( | ||
| "test_enum_table", | ||
| m2, | ||
| Column("id", Integer, primary_key=True), | ||
| Column( | ||
| "status", Enum("A", "B", "C", "D", native_enum=True) | ||
| ), # Added 'D' | ||
| ) | ||
|
|
||
| with self.bind.begin() as conn: | ||
| autogen_context = self._get_autogen_context(conn, m2) | ||
| diffs = [] | ||
| autogenerate.compare._produce_net_changes(autogen_context, diffs) | ||
|
|
||
| # There should be differences detected | ||
| if hasattr(diffs, "__iter__") and not isinstance(diffs, str): | ||
| # Check if any operation was generated | ||
| assert ( | ||
| len(diffs) > 0 | ||
| ), "No differences detected for ENUM value addition!" | ||
|
|
||
| @combinations(("backend",)) | ||
| def test_enum_value_removed(self): | ||
| """Test that removing a value from ENUM is detected.""" | ||
| # Create initial table with ENUM | ||
| Table( | ||
| "test_enum_table2", | ||
| self.metadata, | ||
| Column("id", Integer, primary_key=True), | ||
| Column("status", Enum("A", "B", "C", "D", native_enum=True)), | ||
| ) | ||
|
|
||
| with self.bind.begin() as conn: | ||
| self.metadata.create_all(conn) | ||
|
|
||
| # Create modified metadata with removed ENUM value | ||
| m2 = MetaData() | ||
| Table( | ||
| "test_enum_table2", | ||
| m2, | ||
| Column("id", Integer, primary_key=True), | ||
| Column( | ||
| "status", Enum("A", "B", "C", native_enum=True) | ||
| ), # Removed 'D' | ||
| ) | ||
|
|
||
| with self.bind.begin() as conn: | ||
| autogen_context = self._get_autogen_context(conn, m2) | ||
| diffs = [] | ||
| autogenerate.compare._produce_net_changes(autogen_context, diffs) | ||
|
|
||
| # There should be differences detected | ||
| if hasattr(diffs, "__iter__") and not isinstance(diffs, str): | ||
| assert ( | ||
| len(diffs) > 0 | ||
| ), "No differences detected for ENUM value removal!" | ||
|
|
||
| @combinations(("backend",)) | ||
| def test_enum_value_reordered(self): | ||
| """Test that reordering ENUM values is detected. | ||
| In MySQL, ENUM order matters for sorting and comparison. | ||
| """ | ||
| # Create initial table with ENUM | ||
| Table( | ||
| "test_enum_table3", | ||
| self.metadata, | ||
| Column("id", Integer, primary_key=True), | ||
| Column("status", Enum("A", "B", "C", native_enum=True)), | ||
| ) | ||
|
|
||
| with self.bind.begin() as conn: | ||
| self.metadata.create_all(conn) | ||
|
|
||
| # Create modified metadata with reordered ENUM values | ||
| m2 = MetaData() | ||
| Table( | ||
| "test_enum_table3", | ||
| m2, | ||
| Column("id", Integer, primary_key=True), | ||
| Column( | ||
| "status", Enum("C", "B", "A", native_enum=True) | ||
| ), # Reordered | ||
| ) | ||
|
|
||
| with self.bind.begin() as conn: | ||
| autogen_context = self._get_autogen_context(conn, m2) | ||
| diffs = [] | ||
| autogenerate.compare._produce_net_changes(autogen_context, diffs) | ||
|
|
||
| # There should be differences detected | ||
| if hasattr(diffs, "__iter__") and not isinstance(diffs, str): | ||
| assert ( | ||
| len(diffs) > 0 | ||
| ), "No differences detected for ENUM value reordering!" | ||
|
|
||
| @combinations(("backend",)) | ||
| def test_enum_no_change(self): | ||
| """Test that identical ENUMs are not flagged as different.""" | ||
| # Create initial table with ENUM | ||
| Table( | ||
| "test_enum_table4", | ||
| self.metadata, | ||
| Column("id", Integer, primary_key=True), | ||
| Column("status", Enum("A", "B", "C", native_enum=True)), | ||
| ) | ||
|
|
||
| with self.bind.begin() as conn: | ||
| self.metadata.create_all(conn) | ||
|
|
||
| # Create identical metadata | ||
| m2 = MetaData() | ||
| Table( | ||
| "test_enum_table4", | ||
| m2, | ||
| Column("id", Integer, primary_key=True), | ||
| Column("status", Enum("A", "B", "C", native_enum=True)), | ||
| ) | ||
|
|
||
| with self.bind.begin() as conn: | ||
| autogen_context = self._get_autogen_context(conn, m2) | ||
| diffs = [] | ||
| autogenerate.compare._produce_net_changes(autogen_context, diffs) | ||
|
|
||
| # There should be NO differences for identical ENUMs | ||
| # We just check it doesn't crash and completes successfully | ||
| pass | ||
|
|
||
| @combinations(("backend",)) | ||
| def test_mysql_enum_dialect_type(self): | ||
| """Test using MySQL-specific ENUM type directly.""" | ||
| # Create initial table with MySQL ENUM | ||
| Table( | ||
| "test_mysql_enum", | ||
| self.metadata, | ||
| Column("id", Integer, primary_key=True), | ||
| Column("status", MySQL_ENUM("pending", "active", "closed")), | ||
| ) | ||
|
|
||
| with self.bind.begin() as conn: | ||
| self.metadata.create_all(conn) | ||
|
|
||
| # Create modified metadata with additional ENUM value | ||
| m2 = MetaData() | ||
| Table( | ||
| "test_mysql_enum", | ||
| m2, | ||
| Column("id", Integer, primary_key=True), | ||
| Column( | ||
| "status", | ||
| MySQL_ENUM("pending", "active", "closed", "archived"), | ||
| ), # Added 'archived' | ||
| ) | ||
|
|
||
| with self.bind.begin() as conn: | ||
| autogen_context = self._get_autogen_context(conn, m2) | ||
| diffs = [] | ||
| autogenerate.compare._produce_net_changes(autogen_context, diffs) | ||
|
|
||
| # There should be differences detected | ||
| if hasattr(diffs, "__iter__") and not isinstance(diffs, str): | ||
| assert ( | ||
| len(diffs) > 0 | ||
| ), "No differences detected for MySQL ENUM value addition!" | ||
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The docstring should document the return value and parameters for clarity. Consider expanding it: