Skip to content

Support sequence of metadata objects in target_metadata #91

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

Closed
martlaf opened this issue Sep 27, 2024 · 7 comments · Fixed by #92 or #94
Closed

Support sequence of metadata objects in target_metadata #91

martlaf opened this issue Sep 27, 2024 · 7 comments · Fixed by #92 or #94

Comments

@martlaf
Copy link

martlaf commented Sep 27, 2024

Since alembic rev 0.9.0, we can pass a sequence of objects in target_metadata.

###  Database definition
from sqlalchemy import MetaData, Integer
from sqlalchemy.orm import declarative_base, mapped_column

Base1 = declarative_base(metadata=MetaData(schema="first_schema"))
Base2 = declarative_base(metadata=MetaData(schema="second_schema"))

class Table1(Base1):
    __tablename__ = "table_in_first_schema"
    id = mapped_column(Integer, primary_key=True)

class Table2(Base2):
    __tablename__ = "table_in_second_schema"
    id = mapped_column(Integer, primary_key=True)


###  env.py
from alembic import context

context.configure(
    connection=connection,
    target_metadata=[Base1.metadata, Base2.metadata],
    ...
)
with context.begin_transaction():
    context.run_migrations()
    

When trying to declare my database (declare_database(Base.metadata, schemas=Schemas().are(Base.metadata.schema))) using sqlalchemy-declarative-extensions and autogenerating a migration, I will get an AttibuteError: 'list' object has no attribute 'info' in all the comparators that will try to fetch the info declare_database has stored against the metadata.

I believe one way to fix this would be to, like they did in their tests setup, coerce target_metadata into a list before interating through all metadata objects.

@DanCardin
Copy link
Owner

Interesting! so i assume we're talking about this: https://github.com/DanCardin/sqlalchemy-declarative-extensions/blob/main/src/sqlalchemy_declarative_extensions/alembic/schema.py#L23 as the exception point.

The main problem I see is that of combining multiple instances of each object together. I suppose I need some Schemas.merge(autogen_context.metadata) that recombines them into a single instance. And this would likely need to assert all instances of Schemas have the same other settings, otherwise it's ambiguous what end-result you want. not tooooo too bad, so long as that's true.

The only remaining problem is that Views actually make use of the metadata downstream, in order to provide a naming-scheme in the case of materialized view indices. I think.....similarly this could be made to assert they're the same before passing it through, because again once you're at the point of comparing existing index names vs declared ones, the literal metadata instance would be ambiguous.

tl;dr should be doable

@DanCardin
Copy link
Owner

I suppose it might be worth submitting a bug to alembic, their type annotation implies it's Optional[MetaData] I was completely unaware that it was allowed to be a list

@DanCardin
Copy link
Owner

just kidding, i just submitted a PR to update the typing

@DanCardin
Copy link
Owner

mmm Rows also uses metadata in a way that's probably the least easy to work around. I could probably release the fix much more quickly for Schemas if that's the only one affecting you? and then deal with the rest after the fact

@martlaf
Copy link
Author

martlaf commented Sep 27, 2024

Actually the ones I'd be interested in would be grants and ideally roles

@DanCardin
Copy link
Owner

ah good. both are on the straightforward side relative to View/Row

@DanCardin
Copy link
Owner

I suppose I'll reopen this though because of Views/Rows

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants