sqlalchemy / alembic

A database migrations tool for SQLAlchemy.
MIT License
2.76k stars 241 forks source link

conditional ddl support; add autogeneration support for `ddf_if()` #1491

Open chriswhite199 opened 3 months ago

chriswhite199 commented 3 months ago

Describe the bug

Using conditional Indices with ddl_if, alembic ignores the target dialect and creates all the indices, not just the index that matches the conditional constraint.

from sqlalchemy import Index, String
from sqlalchemy.orm import DeclarativeBase, Mapped, mapped_column

class Base(DeclarativeBase):
    pass

class User(Base):
    __tablename__ = "user"

    __table_args__ = (
        Index("my_pg_index_psql", "name").ddl_if(dialect="postgresql"),
        Index("my_pg_index_sqlite", "name").ddl_if(dialect="sqlite"),
    )

    id: Mapped[int] = mapped_column(primary_key=True)
    name: Mapped[str] = mapped_column(String(30))
./bin/alembic revision --autogenerate -m 'DB init'
INFO  [alembic.runtime.migration] Context impl PostgresqlImpl.
INFO  [alembic.runtime.migration] Will assume transactional DDL.
INFO  [alembic.autogenerate.compare] Detected added table 'user_account'
INFO  [alembic.autogenerate.compare] Detected added index ''my_pg_index_psql'' on '('name',)'
INFO  [alembic.autogenerate.compare] Detected added index ''my_pg_index_sqlite'' on '('name',)'

Generate python file contains two create_index statements, should be a single stmt:

def upgrade() -> None:
    # ### commands auto generated by Alembic - please adjust! ###
    op.create_table('user',
    sa.Column('id', sa.Integer(), nullable=False),
    sa.Column('name', sa.String(length=30), nullable=False),
    sa.PrimaryKeyConstraint('id')
    )
    op.create_index('my_pg_index_psql', 'user_account', ['name'], unique=False)
    op.create_index('my_pg_index_sqlite', 'user_account', ['name'], unique=False)
    # ### end Alembic commands ###

Expected behavior Only the index for the target dialect should be created, not both. Note if you use the same index name for both Index defs, you will get a single op.create_index but the configuration will be random.

To Reproduce As above, using postgres as the db in alembic.ini:

sqlalchemy.url = postgresql://postgres:password@localhost/postgres

Versions.

zzzeek commented 3 months ago

hi -

this is very special use case so you would need to manually adjust the autogenerated file for now. logic would need to be added to autogenerate to look for these ddl_ifs but it would likely then not be able to correctly generate each dialect-specific index, since autogen is only against one database.

chriswhite199 commented 3 months ago

Fair enough, just checking whether this was a known issue, bug, not supported etc.

For the case of autogen, it would logically follow that it should be easier because it's a single dialect, so just ignore those not for the current dialect?

I'll see if i can understand the autogen code and maybe submit a PR for this.

Might consider updating https://alembic.sqlalchemy.org/en/latest/autogenerate.html#what-does-autogenerate-detect-and-what-does-it-not-detect to denote this edge case

zzzeek commented 3 months ago

well it would render the Index() for just one of the databases, and not the other, meaning your migration file will only work on that one database. that is, it wouldnt generate a cross-compatible migration file even though your model is cross compatible.

chriswhite199 commented 3 months ago

So for my specific use case, the indexing options are different for PSQL vs SQLite, and SQLite is only used for quick integration tests. I only ever autogenerate for PSQL.

Would you also not be able to follow this guidance and just generate multiple migration file sets for each dialect type:

https://alembic.sqlalchemy.org/en/latest/cookbook.html#run-multiple-alembic-environments-from-one-ini-file

zzzeek commented 3 months ago

if you really wanted multiple separate environments, sure, usually people want one migration set for a certain kind of schema and they use cross-compatibility features for things like special indexes.

anyway, PRs to improve the behavior are welcome as long as you can make some tests for them

CaselIT commented 3 months ago

how could a direct support api look like? pass a kw arg with a dict of kw similar to https://docs.sqlalchemy.org/en/20/core/constraints.html#sqlalchemy.schema.HasConditionalDDL.ddl_if ?

op.create_index('my_pg_index_psql', 'user_account', ['name'], unique=False, ddl_if={'dialect': 'postgresql'})
op.create_index('my_pg_index_sqlite', 'user_account', ['name'], unique=False ddl_if={'dialect': 'sqlite'})

something else like a conditional?

if op.ddl_if(dialect='postgresql'):
  op.create_index('my_pg_index_psql', 'user_account', ['name'], unique=False)
if op.ddl_if(dialect='sqlite'):
  op.create_index('my_pg_index_sqlite', 'user_account', ['name'], unique=False)
zzzeek commented 3 months ago

I like the second form better, but it might be more challenging from a python rendering perspective (or it likely doesnt matter much). I'm pretty sure we talked about, or I had in my head at some point, that we should have some formal "if " construct in alembic (annnd....I'm pretty sure we dont yet, but not 100% sure)

CaselIT commented 3 months ago

it's for sure more general, since it could be used by user to make conditional sections. It's likely easy to add as an api, but supporting it in the autogenerate it may be a bit more complex