Combining Q() objects with boolean expressions crashes.

[jonathan-golorry]

Currently Q objects with 1 child are treated differently during deconstruct.

>>> from django.db.models import Q
>>> Q(x=1).deconstruct()
('django.db.models.Q', (), {'x': 1})
>>> Q(x=1, y=2).deconstruct()
('django.db.models.Q', (('x', 1), ('y', 2)), {})

This causes issues when deconstructing Q objects with a non-subscriptable child.

>>> from django.contrib.auth import get_user_model
>>> from django.db.models import Exists
>>> Q(Exists(get_user_model().objects.filter(username='jim'))).deconstruct()
Traceback (most recent call last):
  File "<console>", line 1, in <module>
  File "...", line 90, in deconstruct
    kwargs = {child[0]: child[1]}
TypeError: 'Exists' object is not subscriptable

Patch ​https://github.com/django/django/pull/14126 removes the special case, meaning single-child Q objects deconstruct into args instead of kwargs. A more backward-compatible approach would be to keep the special case and explicitly check that the child is a length-2 tuple, but it's unlikely that anyone is relying on this undocumented behavior.

[Mariusz Felisiak]

Conditional expressions can be combined together, so it's not necessary to encapsulate Exists() with Q(). Moreover it's an undocumented and untested to pass conditional expressions to Q(). Nevertheless I think it makes sense to support this.

There is no need to change the current format of deconstruct(), it should be enough to handle conditional expressions, e.g.

diff --git a/django/db/models/query_utils.py b/django/db/models/query_utils.py
index ae0f886107..5dc71ad619 100644
--- a/django/db/models/query_utils.py
+++ b/django/db/models/query_utils.py
@@ -85,7 +85,7 @@ class Q(tree.Node):
         if path.startswith('django.db.models.query_utils'):
             path = path.replace('django.db.models.query_utils', 'django.db.models')
         args, kwargs = (), {}
-        if len(self.children) == 1 and not isinstance(self.children[0], Q):
+        if len(self.children) == 1 and not isinstance(self.children[0], Q) and getattr(self.children[0], 'conditional', False) is False:
             child = self.children[0]
             kwargs = {child[0]: child[1]}
         else:

[Nick Pope]

As Q() has .conditional set to True, we can amend Mariusz' example above to the following:

django/db/models/query_utils.py

@@ -85 +85 @@
         if path.startswith('django.db.models.query_utils'):
             path = path.replace('django.db.models.query_utils', 'django.db.models')
         args, kwargs = (), {}
-        if len(self.children) == 1 and not isinstance(self.children[0], Q):
+        if len(self.children) == 1 and getattr(self.children[0], 'conditional', False) is False:
             child = self.children[0]
             kwargs = {child[0]: child[1]}
         else:

[jonathan-golorry]

Django already passes conditional expressions to Q objects internally. ​https://github.com/django/django/blob/main/django/db/models/expressions.py#L113

Tested here ​https://github.com/django/django/blob/main/tests/expressions/tests.py#L827

That test is only succeeding because Q(...) | Q(Q()) is treated differently from Q(...) | Q().

As for the form of .deconstruct(), is there any reason for keeping the special case? It's:

1. Inconsistent: Q(x=1).deconstruct() vs Q(x=1, y=2).deconstruct()
2. Fragile: Unsupported inputs like Q(False) sometimes (but not always!) lead to "not subscriptable" errors.
3. Incorrect: Q(("x", 1)).deconstruct() incorrectly puts the condition in kwargs instead of args.

[Mariusz Felisiak]

Django already passes conditional expressions to Q objects internally. ​https://github.com/django/django/blob/main/django/db/models/expressions.py#L113

Tested here ​https://github.com/django/django/blob/main/tests/expressions/tests.py#L827

These are example of combining conditional expressions. Again, initializing Q objects with conditional expressions is undocumented and untested.

That test is only succeeding because Q(...) | Q(Q()) is treated differently from Q(...) | Q().

I'm not sure how it's related with the ticket 🤔

As for the form of .deconstruct(), is there any reason for keeping the special case? It's:

1. Inconsistent: Q(x=1).deconstruct() vs Q(x=1, y=2).deconstruct()

First is a single condition without a connector.

2. Fragile: Unsupported inputs like Q(False) sometimes (but not always!) lead to "not subscriptable" errors.
3. Incorrect: Q(("x", 1)).deconstruct() incorrectly puts the condition in kwargs instead of args.

I wouldn't say that is incorrect Q(('x', 1)) is equivalent to the Q(x=1) so I don't see anything wrong with this behavior.

[jonathan-golorry]

I suppose it's a semantics argument whether Q(Exists...) is untested if there's a test that runs that exact expression, but isn't solely checking that functionality.

My point is that Q(("x", 1)) and Q(x=1) are equivalent, so it's impossible for the deconstruct to correctly recreate the original args and kwargs in all cases. Therefore, unless there's an argument for keeping the special case, it's better to consistently use args for both Q(x=1).deconstruct() and Q(x=1, y=2).deconstruct().

I point out Q(Exists...) | Q(Q()) to show that the fragility of the special case is problematic and hard to catch. An internal optimization for nested empty Q objects can cause conditional expression combination to fail. That's why I'd like this patch to be focused on removing the special case and making Q objects more robust for all inputs, rather than only adding support for expressions. Both would make my future work on Q objects possible, but the current patch would put django in a better position for future development.

Edit: To clarify on my future work, I intend to add .empty() to detect nested empty Q objects -- such as Q(Q()) -- and remove them from logical operations. This would currently cause a regression in test_boolean_expression_combined_with_empty_Q. Ultimately the goal is to add robust implementations of Q.any() and Q.all() that can't return empty Q objects that accidentally leak the entire table ​https://forum.djangoproject.com/t/improving-q-objects-with-true-false-and-none/851/9

[Mariusz Felisiak]

Ian, can I ask for your opinion? We need another pair of eyes, I really don't see why the current format of deconstruct() is problematic 🤷.

[Ian Foote]

I like the consistency and simplicity of the reworked deconstruct method. I think removing weird edge-cases in Q is a good thing.

I think I would personally prefer a deconstruct api that always uses kwargs where possible, rather than args:

('django.db.models.Q', (), {'x': 1, 'y': 2}) looks nicer than ('django.db.models.Q', (('x', 1), ('y', 2)), {}) to me.

I don't know how much harder this would be to implement though, and it's a machine facing interface, so human readability isn't the highest priority.

[jonathan-golorry]

Unfortunately we can't use kwargs for Q objects with multiple children.

(Q(x=1) & Q(x=2)).deconstruct() would lose an argument.

[Ian Foote]

Replying to jonathan-golorry:

Unfortunately we can't use kwargs for Q objects with multiple children.

(Q(x=1) & Q(x=2)).deconstruct() would lose an argument.

Excellent point!

[Mariusz Felisiak]

OK, let's treat this as a bugfix.

[Mariusz Felisiak <felisiak.mariusz@…>]

In 54f60bc:

Refs #32548 -- Added tests for passing conditional expressions to Q().

[Mariusz Felisiak <felisiak.mariusz@…>]

In 00b0786:

Fixed #32548 -- Fixed crash when combining Q() objects with boolean expressions.

[Mariusz Felisiak]

It's a regression in 466920f6d726eee90d5566e0a9948e92b33a122e (backported in 0e2979e95d0f68f28c92c84c14655e7510d4e789), thanks to Jaap Roes for noticing this.

I've prepared a backport, ​PR.

[Mariusz Felisiak <felisiak.mariusz@…>]

In d0267690:

[3.2.x] Fixed #32548 -- Fixed crash when combining Q() objects with boolean expressions.

Backport of 00b0786de533dbb3f6208d8d5eaddbf765b4e5b8 from main.

Regression in 466920f6d726eee90d5566e0a9948e92b33a122e.

[Mariusz Felisiak <felisiak.mariusz@…>]

In 338eb65b:

Refs #32548 -- Forwardported 3.2.1 release note.