Selecting multiple admin list_filters across relations return results that don't match both filters

[Yeago]

Problems arise when filtering across relations in the admin. The current method relies on FilterSpecs chaining filter() clauses. While these create explicit ANDs when filtering on fields on a single table (expected), they imply OR when a filter crosses a relation because each filter() clause creates a new join.

Consider a case where someone would want to filter across all People who have a Contact between dates X and Y that is on_phone (as opposed to in_person). In the current state of the admin, you can't arrive at this list because of the above subtlety with how chained filters() change behavior when spanning relations (which I would argue, this hypothetical user doesn't care about).

​https://github.com/yeago/django-1/commit/bccabbfcf443f0fac511599ccfdf166bb1697b6b

I believe allowing FilterSpec objects to return Q-likes is a happy middle-ground that doesn't break current assumptions.

[Yeago]

One note is that this method won't play well with the current "naked" approach to adding relation items to list_display. Perhaps the implicit filters (such as django.contrib.admin.filters.RelatedFieldListFilter) could return a Q instead of a chained filter().

[Tim Graham]

Could you give a sample project that demonstrates this issue?

[steve yeago]

​https://github.com/yeago/qfilter-problem

[steve yeago]

Its the same as opening a shell and doing

   AnyModel.objects.filter(any__join_field2='X').filter(any__join_field2='Y')

vs

    AnyModel.objects.filter(field2='X').filter(field2='Y')

If you look at the sample project and attempt to filter Info objects of person X within a date Y, you can't.

[Tim Graham]

Sorry, I'm still not understanding. After selecting "This month" and "person 2" in the admin filters for the info page, I see the query:

SELECT "app_info"."id", "app_info"."name"
FROM "app_info"
INNER JOIN "app_contact" ON ("app_info"."id" = "app_contact"."info_id")
INNER JOIN "app_contact" T3 ON ("app_info"."id" = T3."info_id")
WHERE ("app_contact"."date" < 2016-11-01 00:00:00 AND "app_contact"."date" >= 2016-10-01 00:00:00 AND T3."person_id" = 2)

It seems to give the expected results of infos that have contacts with person=2 AND date=this month. Please let me know my mistake.

[steve yeago]

Well the case I am attempting to satisfy here is when you want a list of infos by that person and this month (the person 2 contact in my project was last year).

[Tim Graham]

It sounds like you want list filter's to work using OR then? I guess that could be a duplicate of #18736. It seems problematic to implement your proposal without a corresponding UI change as a user wouldn't have any way to know whether AND or OR are in use. What do you think?

[steve yeago]

I think that its likely we still are not understanding each other. In my case above, its not OR, its AND. The only difference is that the filters are applied to the same join versus creating additional joins for each filtered field. I think its probably much more likely that ordinary users would expect things to behave as I am suggesting rather than how they are. I don't think anyone would want to filter on a list of hypothetical Info objects, and then filter them by user, and then filter them by date, and still be confronted by objects that do not reflect querying of user AND date.

as you saw in your example, you ended up with one object. This is due to the difference between

.filter(contact__date=X).filter(contact__user=Y)

and

.filter(contact__date=X, contact__user=Y)

[Tim Graham]

I'm trying to use AND and OR as they work in SQL:

The AND operator displays a record if both the first condition AND the second condition are true.
The OR operator displays a record if either the first condition OR the second condition is true.

It seems like your usage of AND is the SQL OR. Rather than the second list filter criteria narrowing down the results of the first list filter criteria, you want to see objects that match all of the list filters that are selected. Is that correct?

[steve yeago]

I'm not really sure how you arrive at my usage being OR. In the case I am trying to describe, I still want all conditions to be true. But I want the application of those conditions to happen on one single join instead of separate joins. That is still SQL AND.

The proof is in the pudding, here is a link to the line number in my sample patch where I am applying a SQL AND ​https://github.com/yeago/django-1/commit/bccabbfcf443f0fac511599ccfdf166bb1697b6b#diff-ea7d8c684e252f3dad6aa458df7d3070R329

if the through linke doesn't work, in github its line 329

Hopefully with this we can set aside discussion of OR vs AND.

[steve yeago]

(Although while we are on that subject I could see many ways forward for features related to OR filtering if FilterSpecs used Q objects instead of chained filters)

[Tim Graham]

I didn't see the fixtures in your sample project until a few comments ago. Using that data, could you give sample selected filters and the expected vs. actual results?

[Josh Smeaton]

I think I understand the point you're trying to make Steve. Do the docs here represent the different behaviours you're trying to put across? ​https://docs.djangoproject.com/en/1.10/topics/db/queries/#spanning-multi-valued-relationships

Those docs primarily refer to multi-value relationships which I'm not certain is the use case you're trying to resolve. But to summarise:

.filter(multi__value__lookup=1, multi__value__other=2) is equivalent to (MVL=1 AND MVO=2) because the WHERE clause targets a single join.

SELECT * FROM model 
JOIN multi ON model.multi_id = multi.id 
JOIN value on multi.value_id = value.id 
WHERE value.MVL=1 AND value.MVO = 2

Where .filter(multi__value__lookup=1).filter(multi__value__other=2) is logically equivalent to (MVL=1 OR MVO=2) because MVL and MVO have separate joins, so they can both match independently since it's a multivalue relationship.

SELECT * FROM model 
JOIN multi ON model.multi_id = multi.id 
JOIN value on multi.value_id = value.id 
JOIN multi m2 ON model.multi_id = m2.id 
JOIN value v2 on multi.value_id = v2.id 
WHERE value.MVL=1 AND v2.MVO = 2

Does this correctly summarise the behaviour?

[steve yeago]

Josh: Yes.

Tim: there's only 3 records and I would argue that filtering for date of last year and staff B would more intuitively return no results, however, you can't ever get there.

[Josh Smeaton]

In that case, I tentatively agree that targeting a single join is *usually* what you want. But having some way to make this decision is even better. I admit I don't really follow the patch that well, but some test cases might make your change more clear. Are you able to provide test cases that allow users to target a single join or chain to multiple? By users, I really mean admin developers.

Or does your change implement targeting a single join, only, allowing no customisation? If so, even though it's arguably a better design, it's a backwards incompatibility that someone is certainly relying on.

[steve yeago]

I kept my patch intentionally simple and didn't try to solve any of those other cases, but if there is some current wishlist or related tickets I can consider them in a patch and move forward. But I think a greater design decision should be made about this issue, though, because regardless of the past I don't think that leaving the logical OR is consistent or even intentionally used or realized by most implementors. The filter().filter() issue is something that trips up even more experienced Djangonauts.

Also, where would these tests go? I do see a tests module in the admin but I don't see anything related to these sorts of things in it.

[Tim Graham]

Sorry, I had some trouble understanding the issue -- I guess the sample I was looking at didn't demonstrate the issue at first.

I'm not sure about the best way to resolve it. The ​proposed ModelAdmin.filter_q_behavior attribute seems to expose internals of Django's ORM too much. Maybe trying to frame the solution as allowing a way for the admin user or the ModelAdmin author to choose between AND and OR in the filter criteria as proposed in #18736 would be simpler for everyone to understand.

Tests might be in tests/admin_filters/ or some of the other admin_* test packages.

[steve yeago]

The problem with an approach like that is that this is only superficially like an OR, but it's still an AND.

Can you explain what you mean a little more by exposing internals?

[Tim Graham]

I think all the documented ModelAdmin attributes are relatively straight forward. Q is a Django ORM thing and if I didn't follow this ticket, I don't think it would be at all obvious or intuitive what filter_q_behavior might mean or what effect it might have (maybe the idea is fine and only a better name is needed). Maybe you'd like to try the DevelopersMailingList to get some other input.

[steve yeago]

Oh well yes you had mentioned you wanted this to be backwards compatible or choosable. If people agreed that the proposed behavior is preferable then that option could go away after a deprecation schedule. The issue gets a few elaborate notices in the docs so it stands to reason an API option on this subject would at least have some reference material to back it up.

As for that other ticket, this patch would provide a few hooks for people to apply their Qs in complex ways. I think that simply attaching an AND or OR to each would quickly become too simplistic for most authors.

[Tim Graham]

I'm not sure if I agree with Josh's feeling that someone is relying on this. I'd lean toward calling the current behavior a bug and just fixing it, if possible. I guess using list filter's across multiple relations to the same model might not be very common which is why this didn't surface until now.

[Simon Charette]

I wonder if we could introduce a new filterspec API that relies on ​the ''sticky filter'' behavior of the ORM for this purpose instead:

list_filters = [
    ('multi__value', StickyFilter([
        'lookup',
        ('other', SomeFilterSpect()),
    ]))
]

The StickyFilter (or whatever name we find for this object) would delegate filtering and rendering to its underlying filters but make sure to mark the queryset object passed to it as having sticky filters (aliases are reused when a queryset is sticky) before calling each of its filters queryset(request, sticky_queryset) methods.

[steve yeago]

That's a pretty terrifying API.

[Connor Osborn]

I filed a ticket #27388 for this same issue because Dango Rest Framework depended on the api of chaining filters as an AND. As Josh Smeaton pointed out chained filters are logically OR'd statements. I think this is so counter to intuition that it is reasonable to break backwards compatibility in an upcoming release. I think the current api is likely to cause more pain in the future than the pain that would be caused by breaking existing code. If a function gets passed a queryset it cannot assume that it can call filter and gather the results, because of the nature of OR, it will include more than desired if another filter had been previously applied. Perhaps most importantly, applying multiple joins for the purpose of ORing can lead to exponentially slow queries, because successive joins can have multiplicative factors on the number of rows. This was the original reason I filed the ticket, the successive joins resulted in a search of 1million rows for a table with originally several thousand.

[steve yeago]

Its pretty disappointing that all discussion on a ticket with a patch stops simply because a long-time contributor with some vague alternative assertions about how to approach the problem can't handle feedback.

[Tim Graham]

I'm not sure what you're hoping to accomplish with that comment. Its meaning is vague and doesn't seem constructive. I haven't seen any responses to my suggestion in comment:21 to call the current behavior a bug and fix it. Perhaps it's a way to move forward.

[steve yeago]

Who are you waiting for a response from? I called it a bug long ago.

[Tim Graham]

Actually it looks like Connor seconded treating the current behavior as a bug, so I think the next step would be to provide a patch for evaluating that approach.

[steve yeago]

The ticket was opened with a patch that evaluated that approach.

[Tim Graham]

I see ​PR 7345, did I miss something else? My understanding is that patch adds a new feature rather than corrects the existing behavior.

[steve yeago]

Yeah, with more robust approaches you get both bugs solved and new features. Perhaps you could elaborate more about what kind of patch you are hoping to see? One where filterspec objects filter upon objects and return them, but that somehow doesn't chain ORs? That is fundamentally not how chained querying works in django. If you're arguing that some kind of hackery on the filtered queryset to merge the joins is more preferable, then please say so. I don't really think you understand the parameters of this problem. If you could help escalate this issue to someone with some knowledge of the plumbing at play here I would very much appreciate it. Maybe they could chime in with something useful and we could push this very crucial and old problem (which I have been contending with for 9 years through maintained forks) forward.

[Tim Graham]

Maybe the bugfix is to change the search behavior to use the filter_q_behavior approach. Does it break any existing tests? Can you add a new test to show that it corrects the query for spanning relations?

You can try bumping ​the thread on the mailing list if you need more guidance. I'm employed to review patches, but besides that, Django is a volunteer-driven project, so there's no escalation process. Some of the people who worked on the ORM are no longer active contributors.