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

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

Open
chriswhite199 opened this issue Jun 19, 2024 · 8 comments
Open

Comments

@chriswhite199
Copy link

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.

  • OS: Linux
  • Python: 3.12.2
  • Alembic: 1.13.1
  • SQLAlchemy: 2.0.31
  • Database: Postgres 14
  • DBAPI: ?
@chriswhite199 chriswhite199 added the requires triage New issue that requires categorization label Jun 19, 2024
@zzzeek zzzeek added autogenerate - rendering autogenerate - detection and removed requires triage New issue that requires categorization labels Jun 19, 2024
@zzzeek
Copy link
Member

zzzeek commented Jun 19, 2024

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
Copy link
Author

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
Copy link
Member

zzzeek commented Jun 19, 2024

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
Copy link
Author

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
Copy link
Member

zzzeek commented Jun 20, 2024

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
Copy link
Member

CaselIT commented Jun 21, 2024

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
Copy link
Member

zzzeek commented Jun 21, 2024

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
Copy link
Member

CaselIT commented Jun 23, 2024

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

@CaselIT CaselIT changed the title autogeneration with conditional indices conditional ddl support; add autogeneration support for ddf_if() Jun 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants