Opened 5 years ago

Closed 3 years ago

#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 (28)

comment:1 Changed 5 years ago by Tim Graham

Cc: Nicolas Delaby added

Nicolas, do you remember the reason for the restriction?

comment:2 Changed 5 years ago by Nicolas Delaby

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 Changed 5 years ago by Simon Charette

Triage Stage: UnreviewedAccepted
Version: 2.0master

If I remember correctly there was some weirdness when dealing with exclusion of such nested filtered relations.

comment:4 Changed 4 years ago by Noumbissi Valere Gille Geovan

Owner: changed from nobody to Noumbissi Valere Gille Geovan
Status: newassigned

comment:5 Changed 4 years ago by saber solooki

Are you support this feature in next version or not? thank's in advance

comment:6 Changed 4 years ago by Simon Charette

#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 Changed 4 years ago by Reupen Shah

Cc: Reupen Shah added

comment:8 Changed 4 years ago by Alex Scott

Cc: Alex Scott added

comment:9 Changed 4 years ago by Alex Scott

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 Changed 3 years ago by Matt Ferrante

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.

https://github.com/django/django/pull/12288

comment:11 Changed 3 years ago by Mariusz Felisiak

Matt, we don't accept new features to the Django 2.2 and 3.0. Please create PR against master branch.

Last edited 3 years ago by Mariusz Felisiak (previous) (diff)

comment:12 Changed 3 years ago by Matt Ferrante

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 Changed 3 years ago by Matt Ferrante

Last edited 3 years ago by Matt Ferrante (previous) (diff)

comment:14 Changed 3 years ago by Mariusz Felisiak

Has patch: set

comment:15 Changed 3 years ago by Mariusz Felisiak

Patch needs improvement: set

comment:16 Changed 3 years ago by Matt Ferrante

felixxm, I've updated the PR, can you take another look? Thanks!
https://github.com/django/django/pull/12333

comment:17 Changed 3 years ago by Mariusz Felisiak

Matt, Thanks, you don't need to ping me, if PR is ready for another review just uncheck "Patch needs improvement".

comment:18 Changed 3 years ago by Matt Ferrante

Patch needs improvement: unset

comment:19 Changed 3 years ago by Matt Ferrante

Patch available here, rebased and it's good to go!
https://github.com/django/django/pull/12333

comment:20 Changed 3 years ago by Matt Ferrante

Owner: Noumbissi Valere Gille Geovan deleted
Status: assignednew

comment:21 Changed 3 years ago by Matt Ferrante

Owner: set to Matt Ferrante
Status: newassigned

comment:22 Changed 3 years ago by Matt Ferrante

felixxm, should I be doing something else to get this another review?

comment:23 Changed 3 years ago by Mariusz Felisiak

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 Changed 3 years ago by Matt Ferrante

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 Changed 3 years ago by Simon Charette

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 Changed 3 years ago by Mariusz Felisiak

Triage Stage: AcceptedReady for checkin

comment:27 Changed 3 years ago by Mariusz Felisiak

comment:28 Changed 3 years ago by Mariusz Felisiak <felisiak.mariusz@…>

Resolution: fixed
Status: assignedclosed

In 7d6916e:

Fixed #29789 -- Added support for nested relations to FilteredRelation.

Note: See TracTickets for help on using tickets.
Back to Top