Opened 3 years ago

Closed 3 years ago

Last modified 3 years ago

#32548 closed Bug (fixed)

Combining Q() objects with boolean expressions crashes.

Reported by: jonathan-golorry Owned by: jonathan-golorry
Component: Database layer (models, ORM) Version: dev
Severity: Normal Keywords: Q objects, deconstruct
Cc: Ian Foote, Matthew Schinckel Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description (last modified by 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.

Change History (17)

comment:1 by jonathan-golorry, 3 years ago

Description: modified (diff)
Has patch: set

comment:2 by Mariusz Felisiak, 3 years ago

Cc: Ian Foote Matthew Schinckel added
Needs tests: set
Owner: changed from nobody to jonathan-golorry
Patch needs improvement: set
Status: newassigned
Summary: Remove special kwarg deconstruct case for Q objects with 1 childSupport passing conditional expressions to Q().
Triage Stage: UnreviewedAccepted

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:

comment:3 by Nick Pope, 3 years ago

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

  • django/db/models/query_utils.py

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

comment:4 by jonathan-golorry, 3 years ago

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.

in reply to:  4 comment:5 by Mariusz Felisiak, 3 years ago

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.

  1. Fragile: Unsupported inputs like Q(False) sometimes (but not always!) lead to "not subscriptable" errors.
  2. 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.

comment:6 by jonathan-golorry, 3 years ago

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

Version 3, edited 3 years ago by jonathan-golorry (previous) (next) (diff)

in reply to:  6 comment:7 by Mariusz Felisiak, 3 years ago

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 🤷.

comment:8 by Ian Foote, 3 years ago

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.

in reply to:  8 ; comment:9 by jonathan-golorry, 3 years ago

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

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

in reply to:  9 comment:10 by Ian Foote, 3 years ago

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!

comment:11 by Mariusz Felisiak, 3 years ago

Summary: Support passing conditional expressions to Q().Combining Q() objects with boolean expressions crashes.
Type: Cleanup/optimizationBug

OK, let's treat this as a bugfix.

comment:12 by Mariusz Felisiak, 3 years ago

Needs tests: unset
Patch needs improvement: unset
Triage Stage: AcceptedReady for checkin

comment:13 by Mariusz Felisiak <felisiak.mariusz@…>, 3 years ago

In 54f60bc:

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

comment:14 by Mariusz Felisiak <felisiak.mariusz@…>, 3 years ago

Resolution: fixed
Status: assignedclosed

In 00b0786:

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

comment:15 by Mariusz Felisiak, 3 years ago

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

I've prepared a backport, PR.

comment:16 by Mariusz Felisiak <felisiak.mariusz@…>, 3 years ago

In d0267690:

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

Backport of 00b0786de533dbb3f6208d8d5eaddbf765b4e5b8 from main.

Regression in 466920f6d726eee90d5566e0a9948e92b33a122e.

comment:17 by Mariusz Felisiak <felisiak.mariusz@…>, 3 years ago

In 338eb65b:

Refs #32548 -- Forwardported 3.2.1 release note.

Note: See TracTickets for help on using tickets.
Back to Top