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.

Sign up for GitHub

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

Jump to bottom

Managing more than one entity in AstToSqlAlchemyClauseVisitor #24

Open
AndiZeta opened this issue Sep 7, 2022 · 3 comments · May be fixed by #30
Open

Managing more than one entity in AstToSqlAlchemyClauseVisitor #24

AndiZeta opened this issue Sep 7, 2022 · 3 comments · May be fixed by #30
Assignees
@OliverHofkens

Comments

@AndiZeta
Copy link

AndiZeta commented Sep 7, 2022

Hi,
I faced a problem that very likely has a simple solution that I do not see. I am using release v0.5.2 to modify sqlalchemy queries.

I tried to use the module to filter a query that selects more than one ORM entity. I faced the problem that AstToSqlAlchemyClauseVisitor expects a single type on entity, but the query gets and filter fields from ORM entities of different types. I guess there is a way of doing this and I do not realize which is it.

As an example, using the models from models.py used in testing, I did not find a proper way to freely filter the following query:

stmt = select(Author, Comment).join(Comment)

Using a similar approach as in shorthand.apply_odata_query I can apply filters on fields of any of the two entities (using AliasRewriter if needed), but since AstToSqlAlchemyClauseVisitor accepts a single entity I do not see a way to filter on both.

Please let me know if there is a proper way to manage this kind of queries. Meanwhile, I made an ugly hack to circumvent this, that it probably breaks many things.
@OliverHofkens
Copy link
Member

Hmm, I haven't really used queries that select multiple entities before.

Our usual approach is to select a single entity and let OData handle any joining and filtering through attributes, e.g.:
For comments/author/name eq 'Tom', the visitor would join "comments" and "author" automatically.
We then usually load any relations through the options method of SQLAlchemy queries, e.g.:
q = apply_odata_query(...).options(joinedload(Comment.author)).

But your case definitely sounds like something we should support as well! I'll have to think about how this should behave and how we can make it work.

@OliverHofkens OliverHofkens self-assigned this Sep 7, 2022
@AndiZeta
Copy link
Author

AndiZeta commented Sep 7, 2022

But your case definitely sounds like something we should support as well! I'll have to think about how this should behave and how we can make it work.

In some way, I made it work... The problem is that I do not know if it behaves has it should.

I made AstToSqlAlchemyClauseVisitor check many entities looking for an Identifier. I do not think its behaviour is good enough (I am not sure if it is even well defined) to make a PR, but it could be a good starting point. See here. Notice that it is not a branched from master but from the commit closer to the one tagged as v0.5.2 that I found. If you think it is a good idea to make a PR and start a discussion there, just let me know.

@OliverHofkens
Copy link
Member

Hey @AndiZeta,
finally found some time to look at your branch.
I think the implementation looks good, and using the OData namespace to select the model is very clever! 👍

One minor thing I would add is to allow passing the model "namespaces" as a dict, instead of always relying on model.__name__. So maybe accepting the following for the root_model parameter:

  • A single entity: Current behavior, unchanged.
  • A collection of entities: The behavior in your branch.
  • A dictionary of {namespace: entity}: Behavior in your branch, but using this dict instead of creating it ourselves.

One other thing I'll have to keep in mind is whether this behavior can somehow map onto Django as well, as I'd like to keep the implementations as close as possible.

I'd happily accept a PR, or otherwise I can cook something up myself based on your code when I find some more time.
Thanks again!

AndiZeta added a commit to AndiZeta/odata-query that referenced this issue Oct 25, 2022
@AndiZeta
Chage AstToSqlAlchemyClauseVisitor to accept {namespace: entity} di…
43d13be 
…cts as requested in issue gorilla-co#24
@AndiZeta AndiZeta linked a pull request Oct 25, 2022 that will close this issue
Open
@OliverHofkens OliverHofkens linked a pull request Oct 28, 2022 that will close this issue
Allow multiple models in sqlalchemy #30
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@OliverHofkens OliverHofkens

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Allow multiple models in sqlalchemy AndiZeta/odata-query
2 participants
@OliverHofkens @AndiZeta