sqlalchemy / alembic

A database migrations tool for SQLAlchemy.
MIT License
2.89k stars 249 forks source link

The Enum's `name` property is lost within an autogenerated migration #1551

Open mberdyshev opened 1 month ago

mberdyshev commented 1 month ago

Describe the bug A migration doesn't provide the name property of Enum if sqlalchemy.Enum is augmented with sqlalchemy.types.Decorator. Even if the name property is explicitly given.

Expected behavior The property has to be provided in Enum's definition inside a migration.

To Reproduce sql_types.py

from typing import Any
from sqlalchemy import Enum, TypeDecorator

class Enum1(Enum):
    def __init__(self, *enums: object, **kw: Any):
        validate_strings = kw.pop("validate_strings", True)
        super().__init__(*enums, **kw, validate_strings=validate_strings)

class Enum2(TypeDecorator):
    impl = Enum
    cache_ok = True

    def __init__(self, *enums: object, **kw: Any):
        validate_strings = kw.pop("validate_strings", True)
        super().__init__(*enums, **kw, validate_strings=validate_strings)

tables.py

import enum
from sqlalchemy import Column, MetaData, Table
from sql_types import Enum1, Enum2

class TestEnum(str, enum.Enum):
    FIRST = enum.auto()
    SECOND = enum.auto()

metadata = MetaData()

test_table = Table(
    "test",
    metadata,
    Column("col1", Enum1(TestEnum), nullable=False),
    Column("col2", Enum2(TestEnum, name="enum2"), nullable=False),
)

alembic revision --autogenerate -m "Enums" provides (shortened):

def upgrade() -> None:
    # ### commands auto generated by Alembic - please adjust! ###
    op.create_table('test',
    sa.Column('col1', db.sql_types.Enum1('FIRST', 'SECOND', name='testenum'), nullable=False),
    sa.Column('col2', db.sql_types.Enum2('FIRST', 'SECOND'), nullable=False)
    )
    # ### end Alembic commands ###

def downgrade() -> None:
    # ### commands auto generated by Alembic - please adjust! ###
    op.drop_table('test')
    # ### end Alembic commands ###

Error As you can see from the migration enums are defined differently - the second one misses its name property. If I try to run it on PostgreSQL I get the error:

Stacktrace ```cmd INFO [alembic.runtime.migration] Context impl PostgresqlImpl. INFO [alembic.runtime.migration] Generating static SQL INFO [alembic.runtime.migration] Will assume transactional DDL. BEGIN; CREATE TABLE alembic_version ( version_num VARCHAR(32) NOT NULL, CONSTRAINT alembic_version_pkc PRIMARY KEY (version_num) ); INFO [alembic.runtime.migration] Running upgrade -> 264b6f57ae76, Enums -- Running upgrade -> 264b6f57ae76 CREATE TYPE testenum AS ENUM ('FIRST', 'SECOND'); Traceback (most recent call last): File "C:\Program Files\WindowsApps\PythonSoftwareFoundation.Python.3.10_3.10.3056.0_x64__qbz5n2kfra8p0\lib\runpy.py", line 196, in _run_module_as_main return _run_code(code, main_globals, None, File "C:\Program Files\WindowsApps\PythonSoftwareFoundation.Python.3.10_3.10.3056.0_x64__qbz5n2kfra8p0\lib\runpy.py", line 86, in _run_code exec(code, run_globals) File "C:\Users\Михаил\AppData\Local\Packages\PythonSoftwareFoundation.Python.3.10_qbz5n2kfra8p0\LocalCache\local-packages\Python310\site-packages\alembic\__main__.py", line 4, in main(prog="alembic") File "C:\Users\Михаил\AppData\Local\Packages\PythonSoftwareFoundation.Python.3.10_qbz5n2kfra8p0\LocalCache\local-packages\Python310\site-packages\alembic\config.py", line 636, in main CommandLine(prog=prog).main(argv=argv) File "C:\Users\Михаил\AppData\Local\Packages\PythonSoftwareFoundation.Python.3.10_qbz5n2kfra8p0\LocalCache\local-packages\Python310\site-packages\alembic\config.py", line 626, in main self.run_cmd(cfg, options) File "C:\Users\Михаил\AppData\Local\Packages\PythonSoftwareFoundation.Python.3.10_qbz5n2kfra8p0\LocalCache\local-packages\Python310\site-packages\alembic\config.py", line 603, in run_cmd fn( File "C:\Users\Михаил\AppData\Local\Packages\PythonSoftwareFoundation.Python.3.10_qbz5n2kfra8p0\LocalCache\local-packages\Python310\site-packages\alembic\command.py", line 406, in upgrade script.run_env() File "C:\Users\Михаил\AppData\Local\Packages\PythonSoftwareFoundation.Python.3.10_qbz5n2kfra8p0\LocalCache\local-packages\Python310\site-packages\alembic\script\base.py", line 586, in run_env util.load_python_file(self.dir, "env.py") File "C:\Users\Михаил\AppData\Local\Packages\PythonSoftwareFoundation.Python.3.10_qbz5n2kfra8p0\LocalCache\local-packages\Python310\site-packages\alembic\util\pyfiles.py", line 95, in load_python_file module = load_module_py(module_id, path) File "C:\Users\Михаил\AppData\Local\Packages\PythonSoftwareFoundation.Python.3.10_qbz5n2kfra8p0\LocalCache\local-packages\Python310\site-packages\alembic\util\pyfiles.py", line 113, in load_module_py spec.loader.exec_module(module) # type: ignore File "", line 883, in exec_module File "", line 241, in _call_with_frames_removed File "c:\Users\Михаил\PycharmProjects\Example Project\db\alembic\env.py", line 78, in run_migrations_offline() File "c:\Users\Михаил\PycharmProjects\Example Project\db\alembic\env.py", line 52, in run_migrations_offline context.run_migrations() File "", line 8, in run_migrations File "C:\Users\Михаил\AppData\Local\Packages\PythonSoftwareFoundation.Python.3.10_qbz5n2kfra8p0\LocalCache\local-packages\Python310\site-packages\alembic\runtime\environment.py", line 946, in run_migrations self.get_context().run_migrations(**kw) File "C:\Users\Михаил\AppData\Local\Packages\PythonSoftwareFoundation.Python.3.10_qbz5n2kfra8p0\LocalCache\local-packages\Python310\site-packages\alembic\runtime\migration.py", line 628, in run_migrations step.migration_fn(**kw) File "C:\Users\Михаил\PycharmProjects\Example Project\db\alembic\versions\264b6f57ae76_enums.py", line 24, in upgrade op.create_table('test', File "", line 8, in create_table File "", line 3, in create_table File "C:\Users\Михаил\AppData\Local\Packages\PythonSoftwareFoundation.Python.3.10_qbz5n2kfra8p0\LocalCache\local-packages\Python310\site-packages\alembic\operations\ops.py", line 1318, in create_table return operations.invoke(op) File "C:\Users\Михаил\AppData\Local\Packages\PythonSoftwareFoundation.Python.3.10_qbz5n2kfra8p0\LocalCache\local-packages\Python310\site-packages\alembic\operations\base.py", line 442, in invoke return fn(self, operation) File "C:\Users\Михаил\AppData\Local\Packages\PythonSoftwareFoundation.Python.3.10_qbz5n2kfra8p0\LocalCache\local-packages\Python310\site-packages\alembic\operations\toimpl.py", line 143, in create_table operations.impl.create_table(table, **kw) File "C:\Users\Михаил\AppData\Local\Packages\PythonSoftwareFoundation.Python.3.10_qbz5n2kfra8p0\LocalCache\local-packages\Python310\site-packages\alembic\ddl\impl.py", line 366, in create_table table.dispatch.before_create( File "C:\Users\Михаил\AppData\Local\Packages\PythonSoftwareFoundation.Python.3.10_qbz5n2kfra8p0\LocalCache\local-packages\Python310\site-packages\sqlalchemy\event\attr.py", line 497, in __call__ fn(*args, **kw) File "C:\Users\Михаил\AppData\Local\Packages\PythonSoftwareFoundation.Python.3.10_qbz5n2kfra8p0\LocalCache\local-packages\Python310\site-packages\sqlalchemy\util\langhelpers.py", line 852, in __call__ return getattr(self.target, self.name)(*arg, **kw) File "C:\Users\Михаил\AppData\Local\Packages\PythonSoftwareFoundation.Python.3.10_qbz5n2kfra8p0\LocalCache\local-packages\Python310\site-packages\sqlalchemy\sql\sqltypes.py", line 1130, in _on_table_create t._on_table_create(target, bind, **kw) File "C:\Users\Михаил\AppData\Local\Packages\PythonSoftwareFoundation.Python.3.10_qbz5n2kfra8p0\LocalCache\local-packages\Python310\site-packages\sqlalchemy\dialects\postgresql\named_types.py", line 98, in _on_table_create self.create(bind=bind, checkfirst=checkfirst) File "C:\Users\Михаил\AppData\Local\Packages\PythonSoftwareFoundation.Python.3.10_qbz5n2kfra8p0\LocalCache\local-packages\Python310\site-packages\sqlalchemy\dialects\postgresql\named_types.py", line 338, in create super().create(bind, checkfirst=checkfirst) File "C:\Users\Михаил\AppData\Local\Packages\PythonSoftwareFoundation.Python.3.10_qbz5n2kfra8p0\LocalCache\local-packages\Python310\site-packages\sqlalchemy\dialects\postgresql\named_types.py", line 51, in create bind._run_ddl_visitor(self.DDLGenerator, self, checkfirst=checkfirst) File "C:\Users\Михаил\AppData\Local\Packages\PythonSoftwareFoundation.Python.3.10_qbz5n2kfra8p0\LocalCache\local-packages\Python310\site-packages\sqlalchemy\engine\mock.py", line 61, in _run_ddl_visitor visitorcallable(self.dialect, self, **kwargs).traverse_single(element) File "C:\Users\Михаил\AppData\Local\Packages\PythonSoftwareFoundation.Python.3.10_qbz5n2kfra8p0\LocalCache\local-packages\Python310\site-packages\sqlalchemy\sql\visitors.py", line 664, in traverse_single return meth(obj, **kw) File "C:\Users\Михаил\AppData\Local\Packages\PythonSoftwareFoundation.Python.3.10_qbz5n2kfra8p0\LocalCache\local-packages\Python310\site-packages\sqlalchemy\dialects\postgresql\named_types.py", line 153, in visit_enum self.connection.execute(CreateEnumType(enum)) File "C:\Users\Михаил\AppData\Local\Packages\PythonSoftwareFoundation.Python.3.10_qbz5n2kfra8p0\LocalCache\local-packages\Python310\site-packages\sqlalchemy\engine\mock.py", line 69, in execute return self._execute_impl(obj, parameters) File "C:\Users\Михаил\AppData\Local\Packages\PythonSoftwareFoundation.Python.3.10_qbz5n2kfra8p0\LocalCache\local-packages\Python310\site-packages\alembic\runtime\migration.py", line 675, in dump self.impl._exec(construct) File "C:\Users\Михаил\AppData\Local\Packages\PythonSoftwareFoundation.Python.3.10_qbz5n2kfra8p0\LocalCache\local-packages\Python310\site-packages\alembic\ddl\impl.py", line 190, in _exec compiled = construct.compile(dialect=self.dialect, **compile_kw) File "C:\Users\Михаил\AppData\Local\Packages\PythonSoftwareFoundation.Python.3.10_qbz5n2kfra8p0\LocalCache\local-packages\Python310\site-packages\sqlalchemy\sql\elements.py", line 308, in compile return self._compiler(dialect, **kw) File "C:\Users\Михаил\AppData\Local\Packages\PythonSoftwareFoundation.Python.3.10_qbz5n2kfra8p0\LocalCache\local-packages\Python310\site-packages\sqlalchemy\sql\ddl.py", line 69, in _compiler return dialect.ddl_compiler(dialect, self, **kw) File "C:\Users\Михаил\AppData\Local\Packages\PythonSoftwareFoundation.Python.3.10_qbz5n2kfra8p0\LocalCache\local-packages\Python310\site-packages\sqlalchemy\sql\compiler.py", line 870, in __init__ self.string = self.process(self.statement, **compile_kwargs) File "C:\Users\Михаил\AppData\Local\Packages\PythonSoftwareFoundation.Python.3.10_qbz5n2kfra8p0\LocalCache\local-packages\Python310\site-packages\sqlalchemy\sql\compiler.py", line 915, in process return obj._compiler_dispatch(self, **kwargs) File "C:\Users\Михаил\AppData\Local\Packages\PythonSoftwareFoundation.Python.3.10_qbz5n2kfra8p0\LocalCache\local-packages\Python310\site-packages\sqlalchemy\sql\visitors.py", line 141, in _compiler_dispatch return meth(self, **kw) # type: ignore # noqa: E501 File "C:\Users\Михаил\AppData\Local\Packages\PythonSoftwareFoundation.Python.3.10_qbz5n2kfra8p0\LocalCache\local-packages\Python310\site-packages\sqlalchemy\dialects\postgresql\base.py", line 2236, in visit_create_enum_type self.preparer.format_type(type_), File "C:\Users\Михаил\AppData\Local\Packages\PythonSoftwareFoundation.Python.3.10_qbz5n2kfra8p0\LocalCache\local-packages\Python310\site-packages\sqlalchemy\dialects\postgresql\base.py", line 2728, in format_type raise exc.CompileError( sqlalchemy.exc.CompileError: PostgreSQL ENUM type requires a name. ```

Versions.

Additional context The docs state that subclassing from TypeDecorator should be preferred:

This method is preferred to direct subclassing of SQLAlchemy’s built-in types as it ensures that all required functionality of the underlying type is kept in place.

Have a nice day!

CaselIT commented 1 month ago

Hi,

Managing all attributes of enum with a type decorator is not easy, so it's likely missing something here.

If your use case I would more simply do something like

def Enum1(*args,**kwargs):
  kwargs.set_default('validate_strings', True)
  return Enum(*args, **kwargs)

In any case it's a valid bug

mberdyshev commented 1 month ago

I've looked up the source code and found that type rendering depends on __repr__(). So, inside SQLAlchemy I managed to find that Enum and TypeDecorator representations are different. To sum it up, the difference is the following:

enum = Enum(TestEnum)
print(util.generic_repr(enum))  # TypeDecorator
# Enum('FIRST', 'SECOND')
print(util.generic_repr(enum, to_inspect=[Enum, SchemaType]))  # Enum
# Enum('FIRST', 'SECOND', name='testenum')

It seems now that this bug is more related to SQLAlchemy. Probably TypeDecorator's __repr__() should respect the contained type's __repr__().

CaselIT commented 1 month ago

nice finding. Seems like that may be the issue. I'm not sure why there is a reason for that repr in type decorator (there usually is).

@zzzeek maybe a type should have a generic method to format itself that type decorator could hook into?

zzzeek commented 1 month ago

But the TypeDecorator does not repr() as the inner type does, it has its ownr repr() that does something different:

from sqlalchemy import *

print(repr(Enum("a", "b", "c", name="e1")))

class MyType(TypeDecorator):
    impl = Enum

print(repr(MyType("a", "b", "c", name="e2")))

prints:

Enum('a', 'b', 'c', name='e1')
MyType('a', 'b', 'c')

note it uses MyType. so that's not just calling Enum.__repr__(). also no I'm not going to take the string from Enum.__repr__() and manipulate it, that's too much guessing.

so there's no quick fix here. choices include:

  1. add a render() rule to Alembic for typedecorator + enum
  2. fix util.generic_repr() to not need all those special instructions inside of Enum.repr() so that it can be used more generically
  3. add a new series of methods to TypeEngine that break out the internals of __repr__() into commands that are used to build out a composed repr
zzzeek commented 1 month ago

this would be the most expedient fix for the moment, but it wouldn't get the extra arguments used by enum beyond those used for schema type:

diff --git a/lib/sqlalchemy/sql/sqltypes.py b/lib/sqlalchemy/sql/sqltypes.py
index bc2d898ab..6676a61e0 100644
--- a/lib/sqlalchemy/sql/sqltypes.py
+++ b/lib/sqlalchemy/sql/sqltypes.py
@@ -3826,3 +3826,4 @@ type_api.MATCHTYPE = MATCHTYPE
 type_api.INDEXABLE = INDEXABLE = Indexable
 type_api.TABLEVALUE = TABLEVALUE
 type_api._resolve_value_to_type = _resolve_value_to_type
+type_api.SchemaType = SchemaType
diff --git a/lib/sqlalchemy/sql/type_api.py b/lib/sqlalchemy/sql/type_api.py
index 9f40905fa..f484e26db 100644
--- a/lib/sqlalchemy/sql/type_api.py
+++ b/lib/sqlalchemy/sql/type_api.py
@@ -58,6 +58,7 @@ if typing.TYPE_CHECKING:
     from .sqltypes import NUMERICTYPE as NUMERICTYPE  # noqa: F401
     from .sqltypes import STRINGTYPE as STRINGTYPE  # noqa: F401
     from .sqltypes import TABLEVALUE as TABLEVALUE  # noqa: F401
+    from .sqltypes import SchemaType as SchemaType  # noqa: F401
     from ..engine.interfaces import Dialect
     from ..util.typing import GenericProtocol

@@ -2276,7 +2277,13 @@ class TypeDecorator(SchemaEventTarget, ExternalType, TypeEngine[_T]):
         return self.impl_instance.sort_key_function

     def __repr__(self) -> str:
-        return util.generic_repr(self, to_inspect=self.impl_instance)
+        inst = self.impl_instance
+        if isinstance(inst, SchemaType):
+            to_inspect=[inst, SchemaType]
+        else:
+            to_inspect = [inst]
+
+        return util.generic_repr(self, to_inspect=to_inspect)

 class Variant(TypeDecorator[_T]):
CaselIT commented 1 month ago

But the TypeDecorator does not repr() as the inner type does, it has its ownr repr() that does something different:

I know, that's why I suggested to have new methods for this. regarding the fix you proposed it's likely better to have a proper method for it. the effort should be similar

abhiaagarwal commented 2 weeks ago

For what it's worth, I ran into a similar issue. I have a PydanticModelType generic

class PydanticModelType(TypeDecorator, Generic[T]):
    """Generic class representing a pydantic model that can be serialized to Python."""

    cache_ok = True
    impl = JSON()

    def __init__(self, pydantic_type: type[T]) -> None:
        """Initializes class with the type of the pydantic model."""
        self.pydantic_type = pydantic_type
        super().__init__()

    @override
    def load_dialect_impl(self, dialect: Dialect) -> TypeEngine:
        # Use JSONB for PostgreSQL and JSON for other databases.
        if dialect.name == "postgresql":
            return dialect.type_descriptor(JSONB())

        return dialect.type_descriptor(JSON())

    @override
    def process_bind_param(self, value: T | None, _dialect: Dialect) -> str | None:
        if value is not None:
            value = self.pydantic_type.model_dump_json(value)
        return value

    @override
    def process_result_value(
        self, value: str | None, _dialect: Dialect
    ) -> BaseModel | None:
        if value is not None:
            value = self.pydantic_type.model_validate_json(value)
        return value

And when I declare it in my class

configuration: Mapped[Config] = mapped_column(
        PydanticModelType(Config), nullable=False
    )

Alembic generates this:

sa.Column('configuration', app.database.types.PydanticModelType(), nullable=False),

Not sure if it's the same bug. I'll try overriding repr later.