Unintuitive behavior of ~ on Q() actually useful?

Using Django Using the ORM
1

I just ran into #34603 (~Q() incorrectly interpreted as full rather than empty) – Django again today. The issue is closed as “wontfix” and I have a hard time understanding the benefits of the current behavior. It leads to subtle bugs in the (~100k LOC) application I’m working on from time to time. There are workarounds, but these need to be remembered by members of the team to be applied in every case.

The behavior I’m talking about, in short, is this. Given:

q1 = Q(field=1)
q2 = Q()
  • qs.filter(q1) will return the inverse of qs.filter(~q1).
  • qs.filter(q2) will not return the inverse of qs.filter(~q2). Instead, it will return the same set of rows.

There are a lot of places where we construct Q objects incrementally, often passing them as arguments to functions. We often start with an “empty” Q() object (for example from a default argument of a wrapper function), adding and removing subsets of data using q |= Q(...) and q &= ~Q(...). When looking at the code, it’s not always clear which Q object might be the empty Q object in some cases and which can’t. The issue is that code using a Q object might only work correctly when that Q object is never the empty Q object.

I never came across a situation where the current behavior lead to the expected result, only situations where the it resulted in a bug (e.g. returning all rows instead of no rows after removing all permissions from an access token).

Examples that I have seen of code that behaves unexpectedly:

# Start with no selected rows. 
q = ~Q()

if condition_a:
    q |= Q(...)

if condition_b:
    q |= Q(...)

# condition_a and condition_b should dictate which rows to include.
# Instead, when all conditions are False, all rows are included.
qs.filter(q)

qs.filter(q1 & ~q2)

# Refactoring the above code to this will subtly break it:
qs.filter(q1).exclude(~q2)

# Each condition selects a set of rows. We want to return the union of all those rows.
conditions = [Q(...) for i in ...]
# When the list of conditions is empty, all rows are returned instead.
qs.filter(reduce(operator.or_, conditions, Q())

Has anyone of you ever found the current behavior useful or expected?

Background

Q() objects states:

A Q() object represents an SQL condition that can be used in database-related operations.

Conditions in SQL follow boolean logic, so I’d expect Q objects to do the same. I mean, imagine a = not (foo and bar) working correctly but a = not True resulting in True… :sweat_smile:

2

Expected? Yes.

This link then refers you to Complex lookups with q objects, which states:

A Q object (django.db.models.Q ) is an object used to encapsulate a collection of keyword arguments. These keyword arguments are specified as in “Field lookups” above.

<opinion>
As far as I’m concerned, a Q() object that fails to define “a collection of keyword arguments” is a NULL object, neither True nor False, but simply something to be ignored. From that perspective, all the behavior identified above makes sense. (In fact, I probably wouldn’t mind if that expression were defined as being illegal.)

I have no problem with accepting the premise that there is no difference between having Q() (alone or in combination with any operators) in an expression and not having Q() in the expression at all. I believe it is a fallacy to assign any logical value to what is a NULL expression.
</opinion>

3

I have never run into this inconsistency myself as I’ve always gated filter calls with if condition and called .empty() manually if wanted to negate the results but looking at how WhereNode.as_sql and its associated tests are structured I get a sense that this was a simple omission that should likely be resolved.

The following patch passes the full test suite without issues

diff --git a/django/db/models/sql/where.py b/django/db/models/sql/where.py
index 0fded5cce3..450515ba7a 100644
--- a/django/db/models/sql/where.py
+++ b/django/db/models/sql/where.py
@@ -177,7 +177,10 @@ def as_sql(self, compiler, connection):
         conn = " %s " % self.connector
         sql_string = conn.join(result)
         if not sql_string:
-            raise FullResultSet
+            if self.negated:
+                raise EmptyResultSet
+            else:
+                raise FullResultSet
         if self.negated:
             # Some backends (Oracle at least) need parentheses around the inner
             # SQL in the negated case, even if the inner SQL contains just a
diff --git a/tests/queries/tests.py b/tests/queries/tests.py
index 7ac8a65d42..9bd0a84fc1 100644
--- a/tests/queries/tests.py
+++ b/tests/queries/tests.py
@@ -3650,6 +3650,9 @@ def test_empty_nodes(self):
         w = WhereNode(children=[empty_w, NothingNode()], connector=AND)
         with self.assertRaises(EmptyResultSet):
             w.as_sql(compiler, connection)
+        empty_w.negate()
+        with self.assertRaises(EmptyResultSet):
+            empty_w.as_sql(compiler, connection)


 class QuerySetExceptionTests(SimpleTestCase):

Judging how self.negated is handled in all cases except for the not sql_string bit in WhereNode.as_sql it does appear to be an anomaly.

I tend to agree that not () maps better to not match all rows than match all rows

1 Like
4

What are you trying to tell me with this quote? I know how to construct Q objects and how they work internally. That section misses to mention that other Q objects can be passed as positional arguments to Q.__init__(), which I think is also an important part of the interface of Q.

Where is that explicitly stated in the documentation? AFAICT, this special treatment of instances of Q with an empty set of children isn’t mentioned anywhere.

Adding this special “null” value to the set of boolean expressions breaks De Morgan’s law and changes the base case of and and or over sequences (what all() and any() do in Python). IMHO that does not make it easier to understand. I’ve seen a few bugs caused by this special value.

I have yet to come across a use case that where this null value serves a purpose. And, if these use cases are as rare as I suspect, IMHO it should be more explicit, having to write something like Q.NULL to get such a value, while Q() would behave analogously to Python’s all().

5

My workaround is to use these helpers:

# Q object that is false for all rows.
# Can be used as an initial value when iteratively constructing a Q object.
# The Django ORM is smart enough to remove this condition
# and/or not perform a DB query when this would exclude all rows.
Q_EMPTY = Q(pk__in=[])

# Q object that is true for all rows.
# Can be used instead of Q(), which exhibits strange behavior in some cases.
# See https://code.djangoproject.com/ticket/34603
Q_ALL = ~Q_EMPTY

These behave like the constant expressions false and true in boolean algebra (satisfying DeMorgan and monotonicity and non-monotonicity) and until now I haven’t seen any strange behavior.