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

Programming error while comparing grants #20

Closed guillaume-alliander closed 1 year ago

guillaume-alliander commented 1 year ago

Hi,

I'm trying to use this library to manage grants. It works nicely for schemas and roles, but I get an error for grants.

I am basically following the example, where I eventually have in env.py (this is for use with Alembic):

from sqlalchemy.orm import as_declarative
from sqlalchemy_declarative_extensions import declarative_database, Grants, register_alembic_events
from sqlalchemy_declarative_extensions.dialects.postgresql import DefaultGrant

@declarative_database
@as_declarative()
class Base:
    grants = Grants().are(
        DefaultGrant.on_tables_in_schema("public", "example").grant("select", to="read")
    )

register_alembic_events(schemas=False, roles=False, grants=True, rows=False, views=False)

When I try to autogenerate migrations, this sql query runs (with parameter substitution done for ease of trying it out):

SELECT 
pg_namespace.nspname AS schema,
 pg_class.relname AS name,
 pg_class.relkind AS relkind,
 pg_roles.rolname AS owner,
 CAST(pg_class.relacl AS VARCHAR[]) AS acl
FROM pg_class 
JOIN pg_namespace ON pg_class.relnamespace = pg_namespace.oid
JOIN pg_roles ON pg_class.relowner = pg_roles.oid
WHERE pg_class.relkind IN (
    'r'::VARCHAR,
    'S'::VARCHAR,
    'f'::VARCHAR,
    'n'::VARCHAR,
    'T'::VARCHAR,
    'v'::VARCHAR
) 
AND pg_class.relname NOT LIKE 'pg_%'::VARCHAR
AND pg_namespace.nspname != 'information_schema'::VARCHAR
AND pg_namespace.nspname NOT LIKE 'pg_%'::VARCHAR

UNION

SELECT null AS schema,
 pg_namespace.nspname AS name,
 'n'::VARCHAR AS relkind, -- <-- if I remove the casting here, all is fine
 pg_roles.rolname AS owner,
 CAST(pg_namespace.nspacl AS VARCHAR[]) AS nspacl
FROM pg_namespace
JOIN pg_roles ON pg_namespace.nspowner = pg_roles.oid
WHERE pg_namespace.nspname != 'information_schema'::VARCHAR
AND pg_namespace.nspname NOT LIKE 'pg_%'::VARCHAR
AND pg_namespace.nspname != 'public'::VARCHAR;

which fails with:

sqlalchemy.exc.ProgrammingError: (psycopg.errors.CannotCoerce) UNION could not convert type character varying to "char" LINE 3: ...LECT $10 AS schema, pg_namespace.nspname AS name, $11::VARCH...

The place of the error (and workaround) is indicated in the query. The same query run directly in PG gives the same error.

The traceback is:

Traceback (most recent call last): File "/home/gui/.conda/envs/pow/lib/python3.9/site-packages/sqlalchemy/engine/base.py", line 1964, in _exec_single_context self.dialect.do_execute( File "/home/gui/.conda/envs/pow/lib/python3.9/site-packages/sqlalchemy/engine/default.py", line 747, in do_execute cursor.execute(statement, parameters) File "/home/gui/.conda/envs/pow/lib/python3.9/site-packages/psycopg/cursor.py", line 723, in execute raise ex.with_traceback(None) psycopg.errors.CannotCoerce: UNION could not convert type character varying to "char" LINE 3: ...LECT $10 AS schema, pg_namespace.nspname AS name, $11::VARCH... ^

This happens at least with postgres 13 (and postgis 3, apline) from docker and on postgres 15 (postgis 3, normal version): https://registry.hub.docker.com/r/postgis/postgis/tags?page=1&name=15-3.3 I am using sqlachemy 2.

I tried to look deeper, but that is not something it looks like I could fix.

DanCardin commented 1 year ago

What i find weird is that I can't reproduce this error running the library's test suite, which should definitely cover default grant comparison, and i can confirm that the pasted query fails in even postgres 11, which is what i'm testing with.

Your specific change, i think doesn't work because SQLAlchemy then complains about something else. I suspect literal("n").cast(CHAR).label("relkind"), should work, but i'd really like to see it fail in the test-suite

DanCardin commented 1 year ago

Aha. So the buried lede here was that you're using psycopg (i.e. psycopg3) rather than psycopg2 or asyncpg. This seems to emit a bunch of additional casts which aren't apparent when using the other options.

The fix should be relatively simple, I'm mostly trying to figure out CI to ensure general compatibility

guillaume-alliander commented 1 year ago

Indeed, I am using psycopg! And your next PR looks much cleaner than mine, thanks!

psycopg has another unfortunate (but workable around) side effect. I have roles with an infinity validuntil (default RDS grants, I cannot change them). With psycopg2, infinity is cast to datetime.max. With psycopg this raises an error when analysing the current roles: https://github.com/DanCardin/sqlalchemy-declarative-extensions/blob/main/src/sqlalchemy_declarative_extensions/dialects/postgresql/schema.py#L64 I wondered about submitting a PR which would reduce infinity to a PG date which would fit into python datetime (this should have no side effect because this query is only used internally by sqlalchemy-declarative-extensions), but as there is a recipe in the psycopg doc to overload the dateloader, it might not be needed, although I am now doing a global override, which might be overkill. Anyway, this is not relevant to this PR :)

DanCardin commented 1 year ago

This comment ^, actually seems to have revealed a latent "bug" where we were not removing valid until if set. Seems like "the way" to do this is by setting it to infinity. Setting it to (or leaving it at) None in python implies infinity/unsetness.

So our fix is going to be to coerce infinity to None, and treat them as the same except for unsetting.

Should be released in v0.4.4 in a few mins

guillaume-alliander commented 1 year ago

Awesome, thanks for all your help!

DanCardin commented 1 year ago

feel free to create a new issue, if this didn't solve all the your problems, closing for now.