#29789 closed New feature (fixed)
Support nested relations in FilteredRelation's condition
Reported by: | Thomas Riccardi | Owned by: | Matt Ferrante |
---|---|---|---|
Component: | Database layer (models, ORM) | Version: | dev |
Severity: | Normal | Keywords: | FilteredRelation nested relations |
Cc: | Nicolas Delaby, Reupen Shah, Alex Scott | Triage Stage: | Ready for checkin |
Has patch: | yes | Needs documentation: | no |
Needs tests: | no | Patch needs improvement: | no |
Easy pickings: | no | UI/UX: | no |
Description
As the documentation explains, FilteredRelation's condition do not support nested relations:
>>> Restaurant.objects.annotate( ... pizzas_with_toppings_startswith_n=FilteredRelation( ... 'pizzas__toppings', ... condition=Q(pizzas__toppings__name__startswith='n'), ... ), ... ) Traceback (most recent call last): ... ValueError: FilteredRelation's condition doesn't support nested relations (got 'pizzas__toppings__name__startswith').
It would be great to support nested relations in FilteredRelation's condition (I encountered this limitation multiple times recently).
Change History (29)
comment:1 by , 6 years ago
Cc: | added |
---|
comment:2 by , 6 years ago
I simply couldn't find a way to make it work, I don't remember exactly why and I didn't have real incitement to do it at that time.
The explicit ValueError is more a gate keeper preventing users to follow a path we know will fail cryptically.
My suggestion would be start writing some tests, removing the ValueError and see how it goes.
My priority at that time was to land the feature even with partial support. Now that a big chunk is there, we can more easily improve the support so nested relations is supported.
Unfortunately I'm at the moment away from django stack, so I Invite you, Thomas , to give it a try, if it is important for you. You might go further than I could.
comment:3 by , 6 years ago
Triage Stage: | Unreviewed → Accepted |
---|---|
Version: | 2.0 → master |
If I remember correctly there was some weirdness when dealing with exclusion of such nested filtered relations.
comment:4 by , 6 years ago
Owner: | changed from | to
---|---|
Status: | new → assigned |
comment:6 by , 6 years ago
#30349 addressed an issue when using exclude
with FilteredRelation
that might explain the ValueError
raised when trying to add support for nested relations.
comment:7 by , 5 years ago
Cc: | added |
---|
comment:8 by , 5 years ago
Cc: | added |
---|
comment:9 by , 5 years ago
Noumbissi, just wondering if you're working on this? And if so, how it's progressing?
This problem (of needing to join to a subquery or add additional conditions to a join that is nested more than once) seems to come up quite a bit. I've posted my own question on StackOverflow:
https://stackoverflow.com/questions/58895627/django-queryset-with-aggregation-and-filtered-left-join-multiple-levels-deep
In the absence of this feature, it'd be nice to have someone intimately familiar with the ORM propose how they would tackle this as I haven't seen a "this feature is not yet available, but as a workaround, try this instead..". Would be very helpful.
comment:10 by , 5 years ago
Noumbissi, Alex Scott, Nicolas Delaby
I've opened a PR for supporting Nested FilteredRelations in 2.2
I can also support this for 3.0, but I made the PR for 2.2 because I'm using 2.2. Happy to open the PR for 3.0 as well.
Feedback is appreciated, I included comments in other places I had examined for making the change.
comment:11 by , 5 years ago
Matt, we don't accept new features to the Django 2.2 and 3.0. Please create PR against master branch.
comment:12 by , 5 years ago
Thanks for the heads-up felixxm. Opened one against master:
https://github.com/django/django/pull/12293
I'll work through the test failures.
comment:13 by , 5 years ago
Got the tests passing in https://github.com/django/django/pull/12293
comment:15 by , 5 years ago
Patch needs improvement: | set |
---|
comment:16 by , 5 years ago
felixxm, I've updated the PR, can you take another look? Thanks!
https://github.com/django/django/pull/12333
comment:17 by , 5 years ago
Matt, Thanks, you don't need to ping me, if PR is ready for another review just uncheck "Patch needs improvement".
comment:18 by , 5 years ago
Patch needs improvement: | unset |
---|
comment:19 by , 5 years ago
Patch available here, rebased and it's good to go!
https://github.com/django/django/pull/12333
comment:20 by , 5 years ago
Owner: | removed |
---|---|
Status: | assigned → new |
comment:21 by , 5 years ago
Owner: | set to |
---|---|
Status: | new → assigned |
comment:23 by , 5 years ago
Matt, you can ask any friend to review it. I'm sorry but you must be patient we have many PRs in a review queue.
comment:24 by , 5 years ago
I've been using a branch with this patch for nested FilteredRelations in production for 4 months without issues. It works really well and added a lot of flexibility and allowed for performance optimizations which helped with our analytical query speed. Hoping this gets approved and changes to "Ready for checkin" soon!
comment:25 by , 4 years ago
I think the proposed patch is ready to be merged. Looks like current implementation of FilteredRelation(condition=~Q)
might do weird things under some circumstances but that seems like an artifact of how exclusions against multi-valued relationship is currently implemented.
comment:26 by , 4 years ago
Triage Stage: | Accepted → Ready for checkin |
---|
Nicolas, do you remember the reason for the restriction?