Prevent using __isnull lookup with non-boolean value.

[André Ericson]

__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 ​undocumented and untested. IMO we should raise an error for non-boolean values to avoid confusion and for consistency.

[André Ericson]

PR here: ​https://github.com/django/django/pull/11873

[Mariusz Felisiak]

After the reconsideration I don't think that we should change this ​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).

[André Ericson]

Replying to felixxm:

After the reconsideration I don't think that we should change this ​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 fieldisnull='false' or fieldisnull='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.

[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"

[Mariusz Felisiak]

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.