[Django] #30841: Using isnull to a non-boolean value does not promote join
Django
-------------------------------------+-------------------------------------
Reporter: André | Owner: nobody
Ericson |
Type: Bug | Status: new
Component: Database | Version: master
layer (models, ORM) |
Severity: Normal | Keywords:
Triage Stage: | Has patch: 0
Unreviewed |
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 0
UI/UX: 0 |
-------------------------------------+-------------------------------------
Using `isnull` filter with a non-boolean value will not promote `INNER
JOIN` to an `OUTER JOIN` leading to inconsistent behaviour.
I'm submitting a PR together with this ticket and will link it here.
--
Ticket URL: <https://code.djangoproject.com/ticket/30841>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
-------------------------------------+-------------------------------------
| Ericson
Type: Bug | Status: assigned
Component: Database layer | Version: master
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* status: new => assigned
* owner: nobody => André Ericson
--
Ticket URL: <https://code.djangoproject.com/ticket/30841#comment:1>
Django
-------------------------------------+-------------------------------------
| Ericson
Type: Bug | Status: assigned
Component: Database layer | Version: master
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage:
| Unreviewed
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by André Ericson):
PR here: https://github.com/django/django/pull/11873
--
Ticket URL: <https://code.djangoproject.com/ticket/30841#comment:2>
Django
-------------------------------------+-------------------------------------
| Ericson
Type: Bug | Status: assigned
Component: Database layer | Version: master
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage:
| Unreviewed
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* has_patch: 0 => 1
--
Ticket URL: <https://code.djangoproject.com/ticket/30841#comment:3>
Django
-------------------------------------+-------------------------------------
Reporter: André Ericson | Owner: André
| Ericson
Type: Bug | Status: assigned
Component: Database layer | Version: master
(models, ORM) |
Severity: Normal | Resolution:
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* stage: Unreviewed => Accepted
--
Ticket URL: <https://code.djangoproject.com/ticket/30841#comment:4>
Django
-------------------------------------+-------------------------------------
Reporter: André Ericson | Owner: André
| Ericson
Component: Database layer | Version: master
(models, ORM) |
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* status: assigned => closed
* type: Bug => New feature
* resolution: => wontfix
Comment:
After the reconsideration I don't think that we should change this
[https://docs.djangoproject.com/en/2.2/ref/models/querysets/#isnull
documented] behavior (that is in Django from the very beginning).
`__isnull` lookup expects boolean values in many places and IMO it would
be confusing if we'll allow for truthy/falsy values, e.g. take a look at
these examples `field__isnull='false'` or `field__isnull='true'` (both
would return the same result).
Sorry for my previous acceptation (I shouldn't triage tickets in the
weekend).
--
Ticket URL: <https://code.djangoproject.com/ticket/30841#comment:5>
Django
-------------------------------------+-------------------------------------
Reporter: André Ericson | Owner: André
| Ericson
Type: New feature | Status: closed
Component: Database layer | Version: master
(models, ORM) |
Severity: Normal | Resolution: wontfix
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by André Ericson):
Replying to [comment:5 felixxm]:
> After the reconsideration I don't think that we should change this
[https://docs.djangoproject.com/en/2.2/ref/models/querysets/#isnull
documented] behavior (that is in Django from the very beginning).
`__isnull` lookup expects boolean values in many places and IMO it would
be confusing if we'll allow for truthy/falsy values, e.g. take a look at
these examples `field__isnull='false'` or `field__isnull='true'` (both
would return the same result). You can always call `bool()` on a right
hand side.
>
> Sorry for my previous acceptation (I shouldn't triage tickets in the
weekend).
I understand your point. But is there anything we can do to avoid people
falling for the same pitfall I did? The problem, in my opinion, is that it
works fine for simple queries but as soon as you add a join that needs
promotion it will break, silently. Maybe we should make it raise an
exception when a non-boolean is passed?
One valid example is to have a class that implements `__bool__`.
You can see here
https://github.com/django/django/blob/d9881a025c15d87b2a7883ee50771117450ea90d/django/db/models/lookups.py#L465-L470
that non-bool value is converted to IS NULL and IS NOT NULL already using
the truthy/falsy values.
> IMO it would be confusing if we'll allow for truthy/falsy values, e.g.
take a look at these examples field__isnull='false' or
field__isnull='true' (both would return the same result).
This is already the case. It just is inconsistent, in `lookups.py`
`field__isnull='false' ` will be a positive condition but on the
`query.py` it will be the negative condition.
--
Ticket URL: <https://code.djangoproject.com/ticket/30841#comment:6>
Django
-------------------------------------+-------------------------------------
Reporter: André Ericson | Owner: André
| Ericson
Type: New feature | Status: closed
Component: Database layer | Version: master
(models, ORM) |
Severity: Normal | Resolution: wontfix
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by André Ericson):
Maybe adding a note on the documentation? something like: "Although it
might seem like it will work with non-bool fields, this is not supported
and can lead to inconsistent behaviours"
--
Ticket URL: <https://code.djangoproject.com/ticket/30841#comment:7>
Django
-------------------------------------+-------------------------------------
Reporter: André Ericson | Owner: André
Cleanup/optimization | Status: new
Component: Database layer | Version: master
(models, ORM) |
Severity: Normal | Resolution:
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* status: closed => new
* type: New feature => Cleanup/optimization
* has_patch: 1 => 0
* resolution: wontfix =>
Old description:
> Using `isnull` filter with a non-boolean value will not promote `INNER
> JOIN` to an `OUTER JOIN` leading to inconsistent behaviour.
>
> I'm submitting a PR together with this ticket and will link it here.
New description:
`__isnull` should not allow for non-boolean values. Using truthy/falsey
doesn't promote `INNER JOIN` to an `OUTER JOIN` but works fine for a
simple queries. Using non-boolean values is
[https://docs.djangoproject.com/en/2.2/ref/models/querysets/#isnull
undocumented] and untested. IMO we should raise an error for non-boolean
values to avoid confusion and for consistency.
--
Comment:
Agreed, we should raise an error for non-boolean values, e.g.
{{{
diff --git a/django/db/models/lookups.py b/django/db/models/lookups.py
index 9344979c56..fc4a38c4fe 100644
--- a/django/db/models/lookups.py
+++ b/django/db/models/lookups.py
@@ -463,6 +463,11 @@ class IsNull(BuiltinLookup):
prepare_rhs = False
def as_sql(self, compiler, connection):
+ if not isinstance(self.rhs, bool):
+ raise ValueError(
+ 'The QuerySet value for an isnull lookup must be True or
'
+ 'False.'
+ )
sql, params = compiler.compile(self.lhs)
if self.rhs:
return "%s IS NULL" % sql, params
}}}
I changed the ticket description.
--
Ticket URL: <https://code.djangoproject.com/ticket/30841#comment:8>
Django
-------------------------------------+-------------------------------------
Reporter: André Ericson | Owner: André
Type: | Ericson
Cleanup/optimization | 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
-------------------------------------+-------------------------------------
Comment (by André Ericson):
Thanks, I'll work on it! Wouldn't that possibly break backward
compatibility? I'm not familiar with how Django moves in that regard.
--
Ticket URL: <https://code.djangoproject.com/ticket/30841#comment:9>
Django
-------------------------------------+-------------------------------------
Reporter: André Ericson | Owner: André
Type: | Ericson
Cleanup/optimization | 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
-------------------------------------+-------------------------------------
Comment (by felixxm):
We can add a release note in "Backwards incompatible changes" or deprecate
this and remove in Django 4.0. I have to thing about it, please give me a
day, maybe I will change my mind :)
--
Ticket URL: <https://code.djangoproject.com/ticket/30841#comment:10>
Django
-------------------------------------+-------------------------------------
Reporter: André Ericson | Owner: André
Type: | Ericson
Cleanup/optimization | 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
-------------------------------------+-------------------------------------
Comment (by André Ericson):
No problem. Thanks for taking the time to look into this!
--
Ticket URL: <https://code.djangoproject.com/ticket/30841#comment:11>
Django
-------------------------------------+-------------------------------------
Reporter: André Ericson | Owner: André
Type: | Ericson
Cleanup/optimization | 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
-------------------------------------+-------------------------------------
Comment (by André Ericson):
Another interesting example related to this:
> As an anecdote, I've also got bitten by this possibility.
> An attempt to write WHERE (field IS NULL) = boolean_field as
`.filter(field__isnull=F('boolean_field'))` didn't go as I expected.
> Alexandr Aktsipetrov -- https://groups.google.com/forum/#!msg/django-
developers/AhY2b3rxkfA/0sz3hNanCgAJ
This example will generate the `WHERE .... IS NULL`. I guess we also would
want an exception thrown here.
--
Ticket URL: <https://code.djangoproject.com/ticket/30841#comment:12>
Django
-------------------------------------+-------------------------------------
Reporter: André Ericson | Owner: André
Type: | Ericson
Cleanup/optimization | 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
-------------------------------------+-------------------------------------
Comment (by felixxm):
André, IMO we should deprecate using non-boolean values in Django 3.1
(`RemovedInDjango40Warning`) and remove in Django 4.0 (even if it is
untested and undocumented). I can imagine that a lot of people use e.g.
`1` and `0` instead of booleans.
Attached diff fixes also issue with passing a `F()` expression.
--
Ticket URL: <https://code.djangoproject.com/ticket/30841#comment:13>
Django
-------------------------------------+-------------------------------------
Reporter: André Ericson | Owner: André
Type: | Ericson
Cleanup/optimization | 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
-------------------------------------+-------------------------------------
Comment (by André Ericson):
Replying to [comment:13 felixxm]:
> André, IMO we should deprecate using non-boolean values in Django 3.1
(`RemovedInDjango40Warning`) and remove in Django 4.0 (even if it is
untested and undocumented). I can imagine that a lot of people use e.g.
`1` and `0` instead of booleans.
>
> Attached diff fixes also issue with passing a `F()` expression.
>
> {{{
> def as_sql(self, compiler, connection):
> if not isinstance(self.rhs, bool):
> raise RemovedInDjango40Warning(...)
> ....
> }}}
Sound like a good plan. Not super familiar with the branch structure of
Django. So, I guess the path to follow is to make a PR to master adding
the deprecation warning and eventually when master is 4.x we create the PR
raising the ValueError. Is that right?
Thanks!
--
Ticket URL: <https://code.djangoproject.com/ticket/30841#comment:14>
Django
-------------------------------------+-------------------------------------
Reporter: André Ericson | Owner: André
Type: | Ericson
Cleanup/optimization | 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
-------------------------------------+-------------------------------------
Comment (by Simon Charette):
André, yes mostly. You can find more details about that
[https://docs.djangoproject.com/en/2.2/internals/contributing/writing-code
/submitting-patches/#deprecating-a-feature from the documentation].
--
Ticket URL: <https://code.djangoproject.com/ticket/30841#comment:15>
Django
-------------------------------------+-------------------------------------
Reporter: André Ericson | Owner: André
Type: | Ericson
Cleanup/optimization | 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
-------------------------------------+-------------------------------------
Comment (by André Ericson):
Replying to [comment:15 Simon Charette]:
> André, yes mostly. You can find more details about that
[https://docs.djangoproject.com/en/2.2/internals/contributing/writing-code
/submitting-patches/#deprecating-a-feature from the documentation].
Thanks! I had missed the bit about deprecation. Super helpful!
--
Ticket URL: <https://code.djangoproject.com/ticket/30841#comment:16>
Django
-------------------------------------+-------------------------------------
Reporter: André Ericson | Owner: André
Type: | Ericson
Cleanup/optimization | 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
-------------------------------------+-------------------------------------
* has_patch: 0 => 1
--
Ticket URL: <https://code.djangoproject.com/ticket/30841#comment:17>
Django
-------------------------------------+-------------------------------------
Reporter: André Ericson | Owner: André
Type: | Ericson
Cleanup/optimization | Status: new
Component: Database layer | Version: master
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 1 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* needs_better_patch: 0 => 1
* needs_tests: 0 => 1
--
Ticket URL: <https://code.djangoproject.com/ticket/30841#comment:18>
Django
-------------------------------------+-------------------------------------
Reporter: André Ericson | Owner: André
Type: | Ericson
Cleanup/optimization | Status: new
Component: Database layer | Version: master
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 1 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* needs_better_patch: 1 => 0
--
Ticket URL: <https://code.djangoproject.com/ticket/30841#comment:19>
Django
-------------------------------------+-------------------------------------
Reporter: André Ericson | Owner: André
Type: | Ericson
Cleanup/optimization | Status: new
Component: Database layer | Version: master
(models, ORM) |
Severity: Normal | Resolution:
| checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* needs_tests: 1 => 0
* stage: Accepted => Ready for checkin
--
Ticket URL: <https://code.djangoproject.com/ticket/30841#comment:20>
Django
-------------------------------------+-------------------------------------
Reporter: André Ericson | Owner: André
Type: | Ericson
Component: Database layer | Version: master
(models, ORM) |
Keywords: | Triage Stage: Ready for
| checkin
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* status: new => closed
* resolution: => fixed
Comment:
In [changeset:"31174031f1ded30d96c77908b965755e0be94c94" 3117403]:
{{{
#!CommitTicketReference repository=""
revision="31174031f1ded30d96c77908b965755e0be94c94"
Fixed #30841 -- Deprecated using non-boolean values for isnull lookup.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/30841#comment:21>
Django
-------------------------------------+-------------------------------------
Reporter: André Ericson | Owner: André
Type: | Ericson
Cleanup/optimization | Status: closed
Component: Database layer | Version: master
(models, ORM) |
Severity: Normal | Resolution: fixed
Keywords: | Triage Stage: Ready for
| checkin
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Mariusz Felisiak <felisiak.mariusz@…>):
In [changeset:"396da8b94c62cf955ab401eb40a952b7edba0376" 396da8b9]:
{{{
#!CommitTicketReference repository=""
revision="396da8b94c62cf955ab401eb40a952b7edba0376"
Refs #30841 -- Made isnull lookup raise ValueError for non-boolean values.
Per deprecation timeline.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/30841#comment:22>