sqlalchemy / alembic

A database migrations tool for SQLAlchemy.
MIT License
2.75k stars 240 forks source link

implement type detection change for MySQL ENUM #779

Open zzzeek opened 3 years ago

zzzeek commented 3 years ago

this refers to native enum. per https://github.com/sqlalchemy/alembic/issues/329#issuecomment-761611901 this is not being detected right now and as of #605 we do generally compare arguments.

grubbins commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

that's essentially what your workaround is doing.

hughhan1 commented 3 years ago

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 commented 2 years ago

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 commented 10 months ago

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. )

caniko commented 1 month ago

@kevinschumacher is correct, I just hit the same issue.