Skip to content
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

implement type detection change for MySQL ENUM #779

Open
zzzeek opened this issue Jan 16, 2021 · 6 comments
Open

implement type detection change for MySQL ENUM #779

zzzeek opened this issue Jan 16, 2021 · 6 comments
Labels
autogenerate for enums a long term subject, tagging issues related to this mysql use case not quite a feature and not quite a bug, something we just didn't think of

Comments

@zzzeek
Copy link
Member

zzzeek commented Jan 16, 2021

this refers to native enum. per #329 (comment) this is not being detected right now and as of #605 we do generally compare arguments.

@zzzeek zzzeek added mysql use case not quite a feature and not quite a bug, something we just didn't think of requires triage New issue that requires categorization and removed requires triage New issue that requires categorization labels Jan 16, 2021
@zzzeek zzzeek added the autogenerate for enums a long term subject, tagging issues related to this label Jan 16, 2021
@grubbins
Copy link

Looking at this further (for mysql), I realised that if the number of enum values is the same, then changes in the list of possible enum values are detected - but, if you add an enum item, they are not. That's because in alembic.ddl.impl in DefaultImpl._column_args_match:

        if (
            len(meta_params.args) == len(inspected_params.args)
            and meta_params.args != inspected_params.args
        ):
            return False

i.e. if the length of the args list is different, then we ignore the args values and detect no change.

Detecting a change whenever the length of the args lists are different would make sense, but I'm sure it produces tons of false positives - I already saw that myself with spurious INTEGER(display_width=11) to Integer() conversions.

As a workaround I tried adding this:

        if (
                meta_params.token0 == 'enum'
                and len(meta_params.args) != len(inspected_params.args)
        ):
            return False

This does actually work for my experiment - I added an item to my enum, and the correct migration script was produced. I don't imagine this is the right answer in general though. Maybe we can always detect a change in num args as a change, but filter out false positives like the display_width case.

@zzzeek
Copy link
Member Author

zzzeek commented Jan 16, 2021

enum is a special case, so I would not rely upon the logic added as part of #605 for this datatype. as things worked previously, a new compare_type() method should be restored to https://github.com/sqlalchemy/alembic/blob/master/alembic/ddl/mysql.py#L141 which accommodates for ENUM explicitly before calling the superclass method.

@zzzeek
Copy link
Member Author

zzzeek commented Jan 16, 2021

that's essentially what your workaround is doing.

@hughhan1
Copy link

I am still having this problem as well in MySQL, using flask_sqlalchemy's db.Column to define the columns on my model. Regardless of whether I use db.Enum or sqlalchemy.dialects.mysql.enumerated.ENUM to define the type of the column, Alembic is not detecting any changes when I add new values to the enum.

But in contrast, if I define a new column using the db.Enum type, Alembic successfully generates the migrations necessary to create the enum column with the proper values.

I also confirmed that when reflecting, the column's type is sqlalchemy.dialects.mysql.enumerated.ENUM.

@zliebersbach
Copy link

zliebersbach commented Sep 16, 2022

Here is how I solved it:

def my_compare_type(context, inspected_column, metadata_column, inspected_type, metadata_type):
    # return False if the metadata_type is the same as the inspected_type
    # or None to allow the default implementation to compare these
    # types. a return value of True means the two types do not
    # match and should result in a type change operation.

    if isinstance(inspected_type, sqlalchemy.Enum) and isinstance(metadata_type, sqlalchemy.Enum):
        inspected_enum_values = set(inspected_type.enums)
        metadata_enum_values = set(metadata_type.enums)
        return inspected_enum_values != metadata_enum_values

    return None

...and then in context configuration:

context.configure(
    ...
    compare_type=my_compare_type,
)

Just edit in env.py!

@kevinschumacher
Copy link

This issue also applies to postgres. But the custom compare_type proposed by @zliebersbach at least results in some migration auto-generated, even if the operations don't necessarily work 100%

(In the case of native enum, it never actually emits a statement to update the type definition; in non-native varchar+CHECK enums, it correctly updates the length of the field and emits a create constraint statement that is correct, but because of other limitations I guess doesn't notice you have to first drop the existing constraint. )

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
autogenerate for enums a long term subject, tagging issues related to this mysql use case not quite a feature and not quite a bug, something we just didn't think of
Projects
None yet
Development

No branches or pull requests

5 participants