Opened 6 years ago

Closed 4 years ago

Last modified 12 months 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


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 Tim Graham, 6 years ago

Cc: Nicolas Delaby added

Nicolas, do you remember the reason for the restriction?

comment:2 by Nicolas Delaby, 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 Simon Charette, 6 years ago

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 by Noumbissi Valere Gille Geovan, 5 years ago

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

comment:5 by saber solooki, 5 years ago

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

comment:6 by Simon Charette, 5 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 Reupen Shah, 5 years ago

Cc: Reupen Shah added

comment:8 by Alex Scott, 5 years ago

Cc: Alex Scott added

comment:9 by Alex Scott, 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:

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 Matt Ferrante, 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 Mariusz Felisiak, 5 years ago

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

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

comment:12 by Matt Ferrante, 5 years ago

Thanks for the heads-up felixxm. Opened one against master:

I'll work through the test failures.

comment:13 by Matt Ferrante, 5 years ago

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

comment:14 by Mariusz Felisiak, 5 years ago

Has patch: set

comment:15 by Mariusz Felisiak, 4 years ago

Patch needs improvement: set

comment:16 by Matt Ferrante, 4 years ago

felixxm, I've updated the PR, can you take another look? Thanks!

comment:17 by Mariusz Felisiak, 4 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 Matt Ferrante, 4 years ago

Patch needs improvement: unset

comment:19 by Matt Ferrante, 4 years ago

Patch available here, rebased and it's good to go!

comment:20 by Matt Ferrante, 4 years ago

Owner: Noumbissi Valere Gille Geovan removed
Status: assignednew

comment:21 by Matt Ferrante, 4 years ago

Owner: set to Matt Ferrante
Status: newassigned

comment:22 by Matt Ferrante, 4 years ago

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

comment:23 by Mariusz Felisiak, 4 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 Matt Ferrante, 4 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 Simon Charette, 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 Mariusz Felisiak, 4 years ago

Triage Stage: AcceptedReady for checkin

comment:28 by Mariusz Felisiak <felisiak.mariusz@…>, 4 years ago

Resolution: fixed
Status: assignedclosed

In 7d6916e:

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

comment:29 by Mariusz Felisiak <felisiak.mariusz@…>, 12 months ago

In e4a5527d:

Refs #29789 -- Added more tests for FilteredRelation with condition outside of relation name.

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