DanCardin / sqlalchemy-declarative-extensions

Library to declare additional kinds of objects not natively supported by SqlAlchemy/Alembic.
https://sqlalchemy-declarative-extensions.readthedocs.io/en/latest/
Apache License 2.0
36 stars 7 forks source link

Support sequence of metadata objects in target_metadata #91

Closed martlaf closed 1 month ago

martlaf commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

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

DanCardin commented 1 month ago

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 commented 1 month ago

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

DanCardin commented 1 month ago

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

DanCardin commented 1 month ago

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