Skip to content
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

Filter not working for uuid column #25

Open
schwingkopf opened this issue Sep 16, 2022 · 7 comments
Open

Filter not working for uuid column #25

schwingkopf opened this issue Sep 16, 2022 · 7 comments
Assignees
Labels
bug Something isn't working

Comments

@schwingkopf
Copy link

schwingkopf commented Sep 16, 2022

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
Copy link
Member

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 OliverHofkens self-assigned this Sep 22, 2022
@OliverHofkens OliverHofkens added the bug Something isn't working label Sep 22, 2022
@OliverHofkens
Copy link
Member

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
Copy link
Author

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
Copy link
Contributor

tstadel commented Oct 23, 2023

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
Copy link
Contributor

tstadel commented Oct 24, 2023

@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
Copy link
Member

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
Copy link

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 #60 that resolves this. I upgraded to SQLAlchemy ^2.0 and tested the solution with both PostgreSQL and SQLite databases, successfully filtering by UUID.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

4 participants