[Django] #20024: 'exclude' does not work with lists containing a 'None' element.
Django
----------------------------------------------+--------------------
Reporter: stillwater.ke@… | Owner: nobody
Type: Uncategorized | Status: new
Component: Database layer (models, ORM) | Version: 1.4
Severity: Normal | Keywords:
Triage Stage: Unreviewed | Has patch: 0
Easy pickings: 0 | UI/UX: 0
----------------------------------------------+--------------------
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.
(Note there is no space between underlines. The layout looks weird when
two underline characters are put together.)
--
Ticket URL: <https://code.djangoproject.com/ticket/20024>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
Reporter: stillwater.ke@… | Owner: nobody
Type: Bug | Status: new
Component: Database layer | Version: 1.4
(models, ORM) | Resolution:
Severity: Normal | Triage Stage: Accepted
Keywords: | Needs documentation: 0
Has patch: 0 | Patch needs improvement: 0
Needs tests: 0 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Changes (by akaariai):
* needs_better_patch: => 0
* stage: Unreviewed => Accepted
* type: Uncategorized => Bug
* needs_tests: => 0
* needs_docs: => 0
Comment:
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/20024#comment:1>
Django
Reporter: stillwater.ke@… | Owner: nobody
Type: Bug | Status: new
Component: Database layer | Version: 1.4
(models, ORM) | Resolution:
Severity: Normal | Triage Stage: Accepted
Keywords: | Needs documentation: 0
Has patch: 0 | Patch needs improvement: 0
Needs tests: 0 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Comment (by aaugustin):
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/20024#comment:2>
Django
Reporter: stillwater.ke@… | Owner: nobody
Type: Bug | Status: new
Component: Database layer | Version: 1.4
(models, ORM) | Resolution:
Severity: Normal | Triage Stage: Accepted
Keywords: | Needs documentation: 0
Has patch: 0 | Patch needs improvement: 0
Needs tests: 0 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Old description:
> 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.
> (Note there is no space between underlines. The layout looks weird when
> two underline characters are put together.)
New description:
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.
--
--
Ticket URL: <https://code.djangoproject.com/ticket/20024#comment:3>
Django
Reporter: stillwater.ke@… | Owner: nobody
Type: Bug | Status: new
Component: Database layer | Version: 1.4
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by timgraham):
#13768 is a duplicate.
--
Ticket URL: <https://code.djangoproject.com/ticket/20024#comment:4>
Django
-------------------------------------+-------------------------------------
Reporter: stillwater.ke@… | Owner: nobody
Type: Bug | Status: new
Component: Database layer | Version: 1.4
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
--
Ticket URL: <https://code.djangoproject.com/ticket/20024#comment:5>
Django
-------------------------------------+-------------------------------------
Reporter: stillwater.ke@… | Owner: nobody
Type: Bug | Status: new
Component: Database layer | Version: 1.4
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Carsten Fuchs):
* cc: Carsten Fuchs (added)
--
Ticket URL: <https://code.djangoproject.com/ticket/20024#comment:6>
Django
-------------------------------------+-------------------------------------
Reporter: stillwater.ke@… | Owner: nobody
Type: Bug | Status: new
Component: Database layer | Version: 1.4
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Adam Sołtysik):
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/20024#comment:7>
Django
-------------------------------------+-------------------------------------
Reporter: stillwater.ke@… | Owner: Adam
| Sołtysik
Type: Bug | Status: assigned
Component: Database layer | Version: master
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Adam Sołtysik):
* owner: nobody => Adam Sołtysik
* status: new => assigned
* version: 1.4 => master
Comment:
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/20024#comment:8>
Django
-------------------------------------+-------------------------------------
Reporter: stillwater.ke@… | Owner: Adam
| Sołtysik
Type: Bug | Status: assigned
Component: Database layer | Version: master
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Simon Charette):
* cc: Simon Charette (added)
Comment:
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
--
Ticket URL: <https://code.djangoproject.com/ticket/20024#comment:9>
Django
-------------------------------------+-------------------------------------
Reporter: stillwater.ke@… | Owner: Adam
| Sołtysik
Type: Bug | Status: assigned
Component: Database layer | Version: master
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Adam Sołtysik):
* has_patch: 0 => 1
Comment:
I've created a
[https://github.com/adamsol/django/commit/f50e315ce315ead117bfe4335a9fda758e88ae6f
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/20024#comment:10>
Django
-------------------------------------+-------------------------------------
Reporter: stillwater.ke@… | Owner: (none)
Type: Bug | Status: new
Component: Database layer | Version: master
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Adam Sołtysik):
* owner: Adam Sołtysik => (none)
* status: assigned => new
Comment:
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/20024#comment:11>
Django
-------------------------------------+-------------------------------------
Reporter: stillwater.ke@… | Owner: (none)
Type: Bug | Status: new
Component: Database layer | Version: master
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by felixxm):
* has_patch: 1 => 0
--
Ticket URL: <https://code.djangoproject.com/ticket/20024#comment:12>
Django
-------------------------------------+-------------------------------------
Reporter: stillwater.ke@… | Owner: Simon
| Charette
Type: Bug | Status: assigned
Component: Database layer | Version: master
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Simon Charette):
* owner: (none) => Simon Charette
* status: new => assigned
--
Ticket URL: <https://code.djangoproject.com/ticket/20024#comment:13>
Django
-------------------------------------+-------------------------------------
Reporter: stillwater.ke@… | Owner: (none)
Component: Database layer | Version: master
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Simon Charette):
* owner: Simon Charette => (none)
* status: assigned => new
Comment:
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).
--
Ticket URL: <https://code.djangoproject.com/ticket/20024#comment:14>
Django
-------------------------------------+-------------------------------------
Reporter: stillwater.ke@… | Owner: Jason
| Hall
Type: Bug | Status: assigned
Component: Database layer | Version: dev
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Changes (by Jason Hall):
* owner: (none) => Jason Hall
> 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.
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.
Comment:
I’ve been reading up on SQL’s tri-valued logic (TRUE / FALSE / UNKNOWN),
which explains why SQL needs IS NULL and IS NOT NULL for comparisons
involving NULL. I knew that when writing queries involving blank values
one is supposed to use IS NULL, now I understand why. Neat.
As of ticket #31667, filter(field__in=[None]) now silently ignores None,
which means some of the earlier concerns about fixing this no longer
apply.
I believe this can be resolved cleanly by checking if None is present in
the rhs, and if so, splitting the clause into an IN condition and an IS
NULL condition joined by OR. That should align exclude(field__in=[None,
X]) with user expectations and prevent SQL NULL logic from leaking into
the ORM unnecessarily.
Assigning to myself and will begin a patch.
--
Ticket URL: <https://code.djangoproject.com/ticket/20024#comment:15>
Django
-------------------------------------+-------------------------------------
Reporter: stillwater.ke@… | Owner: Jason
| Hall
Type: Bug | Status: assigned
Component: Database layer | Version: dev
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Changes (by Jason Hall):
--
Ticket URL: <https://code.djangoproject.com/ticket/20024#comment:16>
Django
-------------------------------------+-------------------------------------
Reporter: stillwater.ke@… | Owner: Jason
| Hall
Type: Bug | Status: assigned
Component: Database layer | Version: dev
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Changes (by Jason Hall):
--
Ticket URL: <https://code.djangoproject.com/ticket/20024#comment:17>
Django
-------------------------------------+-------------------------------------
Reporter: stillwater.ke@… | Owner: Jason
| Hall
Type: Bug | Status: assigned
Component: Database layer | Version: dev
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Changes (by Jason Hall):
--
Ticket URL: <https://code.djangoproject.com/ticket/20024#comment:18>
Django
-------------------------------------+-------------------------------------
Reporter: stillwater.ke@… | Owner: Jason
| Hall
Type: Bug | Status: assigned
Component: Database layer | Version: dev
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Changes (by Simon Charette):
* needs_tests: 0 => 1
Comment:
Composite primary key tests still need adjustments.
--
Ticket URL: <https://code.djangoproject.com/ticket/20024#comment:19>
Django
-------------------------------------+-------------------------------------
Reporter: stillwater.ke@… | Owner: Jason
| Hall
Type: Bug | Status: assigned
Component: Database layer | Version: dev
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Changes (by Jason Hall):
* needs_tests: 1 => 0
--
Ticket URL: <https://code.djangoproject.com/ticket/20024#comment:20>
Django
Reporter: stillwater.ke@… | Owner: (none)
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Changes (by Jason Hall):
* owner: Jason Hall => (none)
Ticket URL: <https://code.djangoproject.com/ticket/20024#comment:21>
Django
-------------------------------------+-------------------------------------
Reporter: stillwater.ke@… | Owner: (none)
Type: Bug | Status: new
Component: Database layer | Version: dev
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Changes (by Simon Charette):
--
Ticket URL: <https://code.djangoproject.com/ticket/20024#comment:22>
Django
-------------------------------------+-------------------------------------
Reporter: stillwater.ke@… | Owner: Jason
| Hall
Type: Bug | Status: assigned
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Changes (by Sarah Boyce):
* owner: (none) => Jason Hall
* status: new => assigned
Hello Jason
I see you have closed a few of your PRs but left this one open, so I will
keep this assigned to you in case this update was a mistake
If you do not wish to work on the ticket, you can unassign yourself and
close the PR and I will mark "Patch needs improvement" as Yes to indicate
the ticket needs a new owner before being put back in the review queue
I appreciate that you might have gotten tired of waiting for a review and
that might be why you've closed some PRs. It's been a busy period with the
6.0 feature freeze, various security reports and conferences - this means
it takes a really long time for us to get round to all PRs and I'm sorry
if that's been your experience.
--
Ticket URL: <https://code.djangoproject.com/ticket/20024#comment:23>
Django
-------------------------------------+-------------------------------------
Reporter: stillwater.ke@… | Owner: (none)
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Changes (by Jason Hall):
* has_patch: 1 => 0
* owner: Jason Hall => (none)
* status: assigned => new
Ticket URL: <https://code.djangoproject.com/ticket/20024#comment:24>