Opened 12 years ago
Last modified 4 years ago
#20024 new Bug
QuerySet.exclude() does not work with lists containing a 'None' element.
Reported by: | Owned by: | ||
---|---|---|---|
Component: | Database layer (models, ORM) | Version: | dev |
Severity: | Normal | Keywords: | |
Cc: | Carsten Fuchs, Simon Charette | Triage Stage: | Accepted |
Has patch: | no | Needs documentation: | no |
Needs tests: | no | Patch needs improvement: | no |
Easy pickings: | no | UI/UX: | no |
Description (last modified by )
For example:
Entry.objects.exclude(foo__in=[None, 1])
It is supposed to return all items whose foo field is not None or 1, but it actually returns an empty query set.
Change History (14)
comment:1 by , 12 years ago
Triage Stage: | Unreviewed → Accepted |
---|---|
Type: | Uncategorized → Bug |
comment:2 by , 12 years ago
Let's not be too smart... NULL in SQL is nowhere near as consistent as None in Python because of SQL's tri-valued boolean logic; we cannot hide this.
comment:3 by , 12 years ago
Description: | modified (diff) |
---|
comment:5 by , 9 years ago
Summary: | 'exclude' does not work with lists containing a 'None' element. → QuerySet.exclude() does not work with lists containing a 'None' element. |
---|
comment:6 by , 6 years ago
Cc: | added |
---|
comment:7 by , 4 years ago
I think this should be fixed, this a seriously surprising flaw in the ORM. I've proposed several possible fixes for this issue in #31883, I'll try to summarize them here.
The simplest would be to just ignore None
s in the list. This would be consistent with how filter
currently works. When dealing with querysets (__in=qs.values('col')
), it should be enough to add WHERE col IS NOT NULL
in the SELECT
subquery.
A better solution, but a more breaking change, would be to add a proper IS [NOT] NULL
check, in both exclude
and filter
cases, if None
is in the list. This would allow writing filter(col__in=[something, None])
instead of filter(Q(col=something) | Q(col=None))
. Some additional checks for inner querysets would also be required, so that the behaviour is consistent, or the queryset could be just converted to a list beforehand.
comment:8 by , 4 years ago
Owner: | changed from | to
---|---|
Status: | new → assigned |
Version: | 1.4 → master |
I wanted to fix this, and I've found that it has already been partially fixed in #31667. However, there are no tests for exclude
, and the behaviour is now inconsistent, since exclude(col__in=queryset.values('col2'))
still gives no results when col2
contains NULL
. I'll try to provide a patch to clean this up.
comment:9 by , 4 years ago
Cc: | added |
---|
Thanks for surfacing this inconsistency Adam.
It looks like adding an else
clause to deal with a query right hand side value that has explicitly selected fields that can be null (primary keys can't)
The RelatedIn
lookup will also need to be adjusted
comment:10 by , 4 years ago
Has patch: | set |
---|
I've created a patch. No PR yet, though, because one test fails: WindowFunctionTests.test_window_expression_within_subquery
. SQL doesn't support filtering by window functions, so I guess the code should be corrected to use another subquery (or maybe an EXISTS
query). Unfortunately I don't know how to write this without using QuerySet utilities.
As for RelatedIn
, the things are weird here. It seems this case is handled differently and already works correctly, and worked even before #31667 (test_ticket20024_related_in
). It was probably fixed in #13815 long ago. There is IS NOT NULL
in the subquery, actually there are two next to each other. One is coming from split_exclude()
(called in build_filter()
), but the other one -- I couldn't find. Also, what looks like another thing to take a closer look at -- there is NOT IN (None)
instead of NOT IN (NULL)
in the SQL for the first query in that test, when the rhs.discard(None)
line is removed.
comment:11 by , 4 years ago
Owner: | removed |
---|---|
Status: | assigned → new |
It would be best to make the behaviour of In
lookup consistent with RelatedIn
. Sadly, I wasn't able to understand how the latter works. So my patch currently crashes with window functions, and it also crashes with OuterRef
due to #31714. As much as I'd like to finish this, I don't have enough time and knowledge to look into it further. If anyone comes up with a solution, feel free to claim this issue.
comment:12 by , 4 years ago
Has patch: | unset |
---|
comment:13 by , 4 years ago
Owner: | set to |
---|---|
Status: | new → assigned |
comment:14 by , 4 years ago
Owner: | removed |
---|---|
Status: | assigned → new |
I fear I won't be able to work on this issue in the near future so I'm deassigning myself.
Now that we filter out literal None
values from __in=[None]
lookup (#31667) we do have a mismatch with __in=nullable_expression
but at least the behaviour is consistent with the nullable_field=F('nullable_field')
and nullable_field=None
asymmetry (#32043).
This is a known failure (tested in queries/tests.py, test_col_not_in_list_containing_null()).
This is somewhat hard to fix as SQL's "somecol NOT IN lst_containing_null" is weird - it will always return False (well, technically UNKNOWN but this doesn't matter here), and this is what causes the bug.
It would be possible to check for presence of None in the given list. If present, remove it and add in a proper IS NULL check instead. But then a query like
.exclude(somecol__in=qs_containig_null)
would work differently from.exclude(somecol__in=list(qs_containig_null))
. I guess that could be classified as known abstraction leak.