alembic icon indicating copy to clipboard operation
alembic copied to clipboard
verified publisher icon sqlalchemy

ENUM with metadata is not translated to sql CREATE TYPE in autogenerate

Open bluefish6 opened this issue 4 years ago • 1 comments

Hi,

I know that ENUM autogeneration is not entirely complete and polished, but I think I might have encountered not something not working, but working incorrectly :)

Describe the bug

  1. Autogenerate for postgres ENUM with metadata=... generates a migration with Metadata(bind=None) without importing Metadata, resulting in NameError when running migration.
  2. The enum type is not created in sql if specified with metadata=..., even though it seems like a well-documented use-case in sqlalchemy docs.

Expected behavior

  • from sqlalchemy import MetaData
  • Create the enum type in the database

To Reproduce

from enum import Enum

from sqlalchemy import BigInteger, Column, MetaData, Table
from sqlalchemy.dialects.postgresql import ENUM

metadata = MetaData(schema="myschema")

class SomeEnum(str, Enum):
    OPEN = "OPEN"
    CLOSED = "CLOSED"

some_enum = ENUM(
    SomeEnum,
    metadata=metadata,
    schema=metadata.schema,
)

some_table = Table(
    "some_table",
    metadata,
    Column("id", BigInteger, primary_key=True),
    Column("some_enum_value", some_enum),
)

Resulting migration file:

"""initial

Revision ID: 653c623403f4
Revises: 
Create Date: 2021-06-24 18:15:52.750889

"""
from alembic import op
import sqlalchemy as sa
from sqlalchemy.dialects import postgresql

# revision identifiers, used by Alembic.
revision = '653c623403f4'
down_revision = None
branch_labels = None
depends_on = None


def upgrade():
    # ### commands auto generated by Alembic - please adjust! ###
    op.create_table('some_table',
    sa.Column('id', sa.BigInteger(), nullable=False),
    sa.Column('some_enum_value', postgresql.ENUM('OPEN', 'CLOSED', name='someenum', schema='myschema', metadata=MetaData(bind=None)), nullable=True),
    sa.PrimaryKeyConstraint('id'),
    schema='myschema'
    )
    # ### end Alembic commands ###


def downgrade():
    # ### commands auto generated by Alembic - please adjust! ###
    op.drop_table('some_table', schema='myschema')
    # ### end Alembic commands ###

Error

18:15 $ alembic upgrade head --sql
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 myschema.alembic_version (
    version_num VARCHAR(32) NOT NULL, 
    CONSTRAINT alembic_version_pkc PRIMARY KEY (version_num)
);

INFO  [alembic.runtime.migration] Running upgrade  -> 653c623403f4, initial
-- Running upgrade  -> 653c623403f4

Traceback (most recent call last):
  File "/home/.../bin/alembic", line 8, in <module>
    sys.exit(main())
  File "/home/.../lib/python3.8/site-packages/alembic/config.py", line 559, in main
    CommandLine(prog=prog).main(argv=argv)
  File "/home/.../lib/python3.8/site-packages/alembic/config.py", line 553, in main
    self.run_cmd(cfg, options)
  File "/home/.../lib/python3.8/site-packages/alembic/config.py", line 530, in run_cmd
    fn(
  File "/home/.../lib/python3.8/site-packages/alembic/command.py", line 293, in upgrade
    script.run_env()
  File "/home/.../lib/python3.8/site-packages/alembic/script/base.py", line 490, in run_env
    util.load_python_file(self.dir, "env.py")
  File "/home/.../lib/python3.8/site-packages/alembic/util/pyfiles.py", line 97, in load_python_file
    module = load_module_py(module_id, path)
  File "/home/.../lib/python3.8/site-packages/alembic/util/compat.py", line 184, in load_module_py
    spec.loader.exec_module(module)
  File "<frozen importlib._bootstrap_external>", line 783, in exec_module
  File "<frozen importlib._bootstrap>", line 219, in _call_with_frames_removed
  File "migrations/env.py", line 90, in <module>
    run_migrations_offline()
  File "migrations/env.py", line 60, in run_migrations_offline
    context.run_migrations()
  File "<string>", line 8, in run_migrations
  File "/home/.../lib/python3.8/site-packages/alembic/runtime/environment.py", line 813, in run_migrations
    self.get_context().run_migrations(**kw)
  File "/home/.../lib/python3.8/site-packages/alembic/runtime/migration.py", line 561, in run_migrations
    step.migration_fn(**kw)
  File "/home/.../migrations/versions/2021_06_24_653c623403f4_initial.py", line 23, in upgrade
    sa.Column('some_enum_value', postgresql.ENUM('OPEN', 'CLOSED', name='someenum', schema='myschema', metadata=MetaData(bind=None)), nullable=True),
NameError: name 'MetaData' is not defined

Note that adding the missing import (from sqlalchemy import MetaData) results in a proper sql, but without the CREATE TYPE:

alembic upgrade head --sql
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 myschema.alembic_version (
    version_num VARCHAR(32) NOT NULL, 
    CONSTRAINT alembic_version_pkc PRIMARY KEY (version_num)
);

INFO  [alembic.runtime.migration] Running upgrade  -> 653c623403f4, initial
-- Running upgrade  -> 653c623403f4

CREATE TABLE myschema.some_table (
    id BIGSERIAL NOT NULL, 
    some_enum_value myschema.someenum, 
    PRIMARY KEY (id)
);

INSERT INTO myschema.alembic_version (version_num) VALUES ('653c623403f4');

COMMIT;

Versions.

  • OS: Ubuntu 20.04.2
  • Python: 3.8.5
  • Alembic: 1.6.5
  • SQLAlchemy: 1.3.24 (I can't upgrade to 1.4 yet)
  • Database: PostgreSQL 12.7
  • DBAPI: psycopg2-binary 2.8.6

Additional context

Note that removing the metadata argument makes the issue disappear:

some_enum = ENUM(
    SomeEnum,
    # metadata=metadata,
    schema=metadata.schema,
)

gives no metadata=Metadata(bind=None), so no NameError is given as a result:

sa.Column('some_enum_value', postgresql.ENUM('OPEN', 'CLOSED', name='someenum', schema='myschema'), nullable=True),

Also, the resulting SQL is as expected:

-- Running upgrade  -> c2d662ec840a

CREATE TYPE myschema.someenum AS ENUM ('OPEN', 'CLOSED');

CREATE TABLE myschema.some_table (
    id BIGSERIAL NOT NULL, 
    some_enum_value myschema.someenum, 
    PRIMARY KEY (id)
);

According to the sqlalchemy docs:

To use a common enumerated type between multiple tables, the best practice is to declare the Enum or ENUM independently, and associate it with the MetaData object itself: ... If we specify checkfirst=True, the individual table-level create operation will check for the ENUM and create if not exists:

So I guess in scenario where I provided the metadata argument, it is expected that the enum would not be created by sqlalchemy table create unless asked to with checkfirst=True. However I think that alembic should either produce the same SQL statements in both cases (metadata argument to enum specified or not), or at least inform in the docs how to enforce the checkfirst-like behaviour.

bluefish6 avatar Jun 24 '21 19:06 bluefish6

hey there -

if you look at the https://github.com/sqlalchemy/alembic/issues?q=is%3Aissue+is%3Aopen+label%3A%22autogenerate+for+enums%22 tag, you'll see support for PG ENUM with autogenerate is 100% zero in alembic right now, so at least the second part of your issue, that is certainly already logged here. The ENUM issue including PGs as well as the constraint we create for non-native enum is fully opened in alembic because it's not clear how this would all work. For example if someone adds a column with an ENUM named "foo", do we emit CREATE TYPE for that enum? What if that enum was already created? how do we know? this works in straight SQLAlchemy because we run a "checkfirst" query against the database. But Alembic does not have this option, as it must generate migrations that work unconditionally as SQL scripts (CREATE TYPE IF NOT EXISTS? maybe, but what if the new Enum has new/different elements than what's there?? also PG won't let you change the elements in an ENUM without turning on autocommit! it's super bad). Plus people will want to add new PG ENUM types individudally, and we dont have an op.create_postgresql_enum() operation; should we? If we have that, then should "add_column()" not create the enum? should autogenerate render the "create_enum()" operation explicitly? how will that work for a cross-platform migration? these are all the unsolved problems here and I simply cannot just whip out a few lines of code to make part of this use case work without potentially creating broken systems that are extremely diffcult to revise once they are public.

for the first part of your issue, it makes no sense to create Enum(metadata=MetaData()) that isn't assigned to some variable already, and autogenerate can't know how to do that, so again I don't know what autogenerate would be expected to do there except perhaps raise an error.

Propose marking this as dupe of #89

just to sum up, the whole set of issues under the "Autogenerate for enums" label describes individual problems but there is no proposed solution that can accommodate all use cases right now. Partial fixes and solutions will make the problem worse. The issue overall needs a champion of some kind who can fully specify how autogen in this area will work for all databases, all options, top to bottom, plus be backwards compatible for existing migrations. That's what we need.

zzzeek avatar Jun 24 '21 20:06 zzzeek