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
31 stars 5 forks source link

Can't filter Function objects by schema in alembic #52

Closed nilsso closed 5 months ago

nilsso commented 5 months ago

In my current Postgres database probject I have a default "public" schema that I'm managing with SQA and Alembic, but also a "gis" schema with all the postgis junk including functions. So far I'm able to ignore the schema via include_name.

# alembic/env.py
INCLUDE_SCHEMAS = [None]

def include_name(name: str, type_: str, _):
    return type_ != "schema" or name in INCLUDE_NAMES

def do_run_migrations(connection: Connection) -> None:
    context.configure(
        connection=connection,
        target_metadata=target_metadata,
        include_schemas=True,
        include_name=include_name,
    )

And this works for ignoring all the standard objects for the "gis" schema.

But this isn't working for, at the very least, Function objects, and alembic autogenerate wants to drop all the functions from the "gis" schema.

Just wondering if there's something I'm missing or if there's more of an overhaul that needs to be done to get Functions to be schema filterable. (I'm pretty green to the whole alembic ecosystem, but I've just started experimenting in my own fork.)

nilsso commented 5 months ago

Actually I've got it working with a couple changes.

# src/sqlalchemy_declarative_extensions/dialects/postgresql/query.py
def get_functions_postgresql(
    connection: Connection,
    schemas: set[str | None],  # NEW
):
    functions = []
    for f in connection.execute(functions_query).fetchall():
        schema = f.schema if f.schema is not None else "public"  # NEW
        if schema in schemas:
            function = Function(
                name=f.name,
                definition=f.source,
                returns=f.return_type,
                language=f.language,
                schema=f.schema,
            )
            functions.append(function)
    return functions
# src/sqlalchemy_declarative_extensions/function/compare.py
def compare_functions(
    connection: Connection,
    schemas: set[str | None],
    functions: Functions,
    metadata: MetaData,
) -> list[Operation]:
    # ...
    existing_functions = get_functions(
        connection,
        schemas,  # NEW
    )
    # ...
# src/sqlalchemy_declarative_extensions/alembic/function.py
@comparators.dispatch_for("schema")
def _compare_functions(
    autogen_context: AutogenContext,
    upgrade_ops: UpgradeOps,
    schemas: Set[Optional[str]],
):
    # ...
    result = compare_functions(
        autogen_context.connection,
        schemas,  # NEW
        functions,
        metadata,
    )
    # ...
DanCardin commented 5 months ago

I guess the question is what the best way to expose that exclusion to users. I somewhat suspect something like Functions(exclude=['gis.*']) would be a combination of the easiest way and the most generally applicable to add to other object types.

I was initially thinking hooking it up to only those schemas that one declares through the library, but for all i know you declare a gis schema too so it doesn't attempt to drop it :p

EDIT: I guess the main drawback of this (rather than something specific to schemas is that it'll be more challenging to filter them out in the query itself, although tbh i dont think that'll be a particularly relevant optimization

nilsso commented 5 months ago

No I didn't either really think the goal would be to filter them out in the query, but IIRC the standard objects do to seem to be filtered by schema at the "fetch object names" stage in the dispatcher.

Alas I was also looking to manage functions with parameters, but, since you say in the docs that support is minimally targeting trigger functions, for now I'm going to put my enhancement request on hold (there's a lot of other things I've got to get done for work 😀).

But still thanks for putting these extensions together. It's been a useful little dive into Alembic internals for me.

DanCardin commented 5 months ago

Well I'll say that I think the above exclude=["some glob string"] is a very viable option for most object types, and i'm perfectly happy to add it (took like 10 mins, i just need to write tests), especially if someone's pointed out a scenario that'd block use of the plugin otherwise.

As far as getting more complete function support goes, that's probably a lot more involved. I mostly just haven't had a personal reason to go about implementing them. If you happen to have any specific subsets of function syntax (and examples) you'd need supported, I could look into it (although given this issue title, feel free to submit another issue and i'll scope this one to exclusion)

EDIT: exclude -> ignore. more in line with the other naming, and slightly more clear what it will do