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.
* status: new => assigned
* owner: nobody => André Ericson
--
Ticket URL: <https://code.djangoproject.com/ticket/30841#comment:1>
Comment (by André Ericson):
PR here: https://github.com/django/django/pull/11873
--
Ticket URL: <https://code.djangoproject.com/ticket/30841#comment:2>
* has_patch: 0 => 1
--
Ticket URL: <https://code.djangoproject.com/ticket/30841#comment:3>
* stage: Unreviewed => Accepted
--
Ticket URL: <https://code.djangoproject.com/ticket/30841#comment:4>
* 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>
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>
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>
* 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>
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>
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>
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>
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>
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>
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>
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>
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>
* has_patch: 0 => 1
--
Ticket URL: <https://code.djangoproject.com/ticket/30841#comment:17>
* needs_better_patch: 0 => 1
* needs_tests: 0 => 1
--
Ticket URL: <https://code.djangoproject.com/ticket/30841#comment:18>
* needs_better_patch: 1 => 0
--
Ticket URL: <https://code.djangoproject.com/ticket/30841#comment:19>
* needs_tests: 1 => 0
* stage: Accepted => Ready for checkin
--
Ticket URL: <https://code.djangoproject.com/ticket/30841#comment:20>
* 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>
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>