gorilla-co / odata-query

An OData v4 query parser and transpiler for Python
MIT License
71 stars 16 forks source link

Filter not working for uuid column #25

Open schwingkopf opened 2 years ago

schwingkopf commented 2 years ago

Hello,

thank you for this phantastic library, it helps me a lot!

However, I experienced that filtering by uuid columns is not working. Here a minimal example:

import uuid

import sqlalchemy as sa
from sqlalchemy.orm import declarative_base
from sqlalchemy.orm.session import Session
from sqlalchemy_utils import UUIDType
from odata_query.sqlalchemy import apply_odata_query

Base = declarative_base()

class MyTable(Base):
    __tablename__ = "mytable"
    id = sa.Column(UUIDType(binary=False), primary_key=True, default=uuid.uuid4, nullable=False)

DATABASE_URL = "sqlite:///test.db"
engine = sa.create_engine(DATABASE_URL)

Base.metadata.drop_all(bind=engine)
Base.metadata.create_all(bind=engine)

with Session(engine) as db:
    t = MyTable()
    db.add(t)
    db.commit()

    q = db.query(MyTable)
    q = apply_odata_query(q, f"id eq {t.id}")
    print(q.all())

Output: [] The UUIDType(binary=False) generates CHAR(32).

I found out that the following change to ./odata-query/sqlalchemy/sqlalchemy_clause.py makes it work (is the missing 'py_' a typo?):

def visit_GUID(self, node: ast.GUID) -> BindParameter:
        ":meta private:"
        # return literal(node.val)    # Old version
        return literal(node.py_val)  # New version

Output: [<__main__.MyTable object at 0x000001F8295C3DC0>]

Also it seems like the tests in ./odata-query/tests/integration/sqlalchemy/test_querying.py do imply filtering with uuids but only against non-matching:

(Author, "id eq a7af27e6-f5a0-11e9-9649-0a252986adba", 0),
(Author, "id in (a7af27e6-f5a0-11e9-9649-0a252986adba, 800c56e4-354d-11eb-be38-3af9d323e83c)", 0),

Would be nice if this could be fixed.

*Edit: Just found out that my proposed change leads to exceptions when using the in clause, that are otherwise not present:

...
q = apply_odata_query(q, f"id in ({t.id}, 800c56e4-354d-11eb-be38-3af9d323e83c)")
...

Outputs:

sqlalchemy.exc.InterfaceError: (sqlite3.InterfaceError) Error binding parameter 0 - probably unsupported type.
[SQL: SELECT mytable.id AS mytable_id
FROM mytable
WHERE mytable.id IN (?, ?)]
[parameters: (UUID('0fc434f4-d10d-4725-8d4f-4e00972dea15'), UUID('800c56e4-354d-11eb-be38-3af9d323e83c'))]
(Background on this error at: https://sqlalche.me/e/14/rvf5)
OliverHofkens commented 2 years ago

Hi, thanks for the kind words, for reporting this issue, and for including a minimal reproducing example!

UUID fields are indeed a pain. Their implementation differs a lot between databases, and on top of that we have SQLAlchemy which can treat it as either a binary UUID or a simple string. As you mention in your workaround and the edit afterwards, SQLAlchemy queries sometimes expect a UUID object (which is what py_val returns) and a plain str at other times (which is why we use val in the code currently).

There's probably a universal solution to be found somewhere in SQLAlchemy, but I haven't found it yet. I'll have to do some further digging!

OliverHofkens commented 2 years ago

Since a generic UUID type is being introduced in SQLAlchemy 2.0, I'm gonna wait until after that release before putting too much effort into this. I believe this bug relates to SQLAlchemy's bind_processor logic, so it might simply be fixed in a new, generic UUID type.

schwingkopf commented 1 year ago

SQLAlchemy 2.0.0 and 2.0.1 have been released recently. Would be nice if you could have a look at the issue again.

tstadel commented 1 year ago

psycopg (3) is also affected by this. psycopg2 was not as it didn't cast string params to VARCHAR. It would be great if we could find a solution for this.

tstadel commented 1 year ago

@OliverHofkens @schwingkopf I see two options here: 1) Add a flag (e.g. via environment variable ODATA_QUERY_KEEP_UUID_TYPE) to give users complete control over the behavior 2) Introduce ODATAv2 syntax (e.g. my_id eq guid'01234567-89ab-cdef-0123-456789abcdef') that always enforces the UUID type

Both are non-breaking. 1) would be consistent with ODATAv4 and should be easy to implement. 2) is a bit ugly as we mix syntax of multiple ODATA versions and most likely it's a bit more expensive to implement.

WDYT?

OliverHofkens commented 1 year ago

Thanks for staying on top of this! I haven't been keeping up with all the new releases like SQLAlchemy 2, Psycopg3 and Python 3.12... I should have some time soon to revisit this one 🤞

AHMETCEMRE commented 1 month ago

I've encountered a similar issue with UUID handling, particularly when filtering UUID columns in queries, which caused type mismatch errors. I've made some changes to properly handle UUIDs and created a pull request https://github.com/gorilla-co/odata-query/pull/60 that resolves this. I upgraded to SQLAlchemy ^2.0 and tested the solution with both PostgreSQL and SQLite databases, successfully filtering by UUID.