[Django] #32548: Remove special kwarg deconstruct case for Q objects with 1 child

[Django]

#32548: Remove special kwarg deconstruct case for Q objects with 1 child
-------------------------------------+-------------------------------------
Reporter: jonathan- | Owner: nobody
golorry |
Type: | Status: new
Cleanup/optimization |
Component: Database | Version: dev
layer (models, ORM) | Keywords: Q objects,
Severity: Normal | deconstruct
Triage Stage: | Has patch: 0
Unreviewed |
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 0
UI/UX: 0 |
-------------------------------------+-------------------------------------
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
}}}

I'll add a patch that 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.

--
Ticket URL: <https://code.djangoproject.com/ticket/32548>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.

[Django]

#32548: Remove special kwarg deconstruct case for Q objects with 1 child
-------------------------------------+-------------------------------------

Reporter: jonathan-golorry | Owner: nobody
Type: | Status: new
Cleanup/optimization |
Component: Database layer | Version: dev
(models, ORM) |
Severity: Normal | Resolution:
Keywords: Q objects, | Triage Stage:
deconstruct | Unreviewed
Has patch: 1 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Changes (by jonathan-golorry):

* has_patch: 0 => 1

Old description:

> 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
> }}}
>
> I'll add a patch that 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.

New description:

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.

--

--
Ticket URL: <https://code.djangoproject.com/ticket/32548#comment:1>

[Django]

#32548: Support passing conditional expressions to Q().
-------------------------------------+-------------------------------------
Reporter: jonathan-golorry | Owner: jonathan-
Type: | golorry
Cleanup/optimization | Status: assigned

Component: Database layer | Version: dev
(models, ORM) |
Severity: Normal | Resolution:

Keywords: Q objects, | Triage Stage: Accepted
deconstruct |
Has patch: 1 | Needs documentation: 0
Needs tests: 1 | Patch needs improvement: 1

Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Changes (by Mariusz Felisiak):

* status: new => assigned
* cc: Ian Foote, Matthew Schinckel (added)
* needs_better_patch: 0 => 1
* needs_tests: 0 => 1
* owner: nobody => jonathan-golorry
* stage: Unreviewed => Accepted

Comment:

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:
}}}

--
Ticket URL: <https://code.djangoproject.com/ticket/32548#comment:2>

[Django]

#32548: Support passing conditional expressions to Q().
-------------------------------------+-------------------------------------
Reporter: jonathan-golorry | Owner: jonathan-
Type: | golorry
Cleanup/optimization | Status: assigned
Component: Database layer | Version: dev
(models, ORM) |
Severity: Normal | Resolution:
Keywords: Q objects, | Triage Stage: Accepted
deconstruct |
Has patch: 1 | Needs documentation: 0
Needs tests: 1 | Patch needs improvement: 1

Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by Nick Pope):

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

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 getattr(self.children[0],

'conditional', False) is False:
child = self.children[0]

kwargs = {child[0]: child[1]}

else:
}}}

--
Ticket URL: <https://code.djangoproject.com/ticket/32548#comment:3>

[Django]

#32548: Support passing conditional expressions to Q().
-------------------------------------+-------------------------------------
Reporter: jonathan-golorry | Owner: jonathan-
Type: | golorry
Cleanup/optimization | Status: assigned
Component: Database layer | Version: dev
(models, ORM) |
Severity: Normal | Resolution:
Keywords: Q objects, | Triage Stage: Accepted
deconstruct |
Has patch: 1 | Needs documentation: 0
Needs tests: 1 | Patch needs improvement: 1

Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by 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.

--
Ticket URL: <https://code.djangoproject.com/ticket/32548#comment:4>

[Django]

#32548: Support passing conditional expressions to Q().
-------------------------------------+-------------------------------------
Reporter: jonathan-golorry | Owner: jonathan-
Type: | golorry
Cleanup/optimization | Status: assigned
Component: Database layer | Version: dev
(models, ORM) |
Severity: Normal | Resolution:
Keywords: Q objects, | Triage Stage: Accepted
deconstruct |
Has patch: 1 | Needs documentation: 0
Needs tests: 1 | Patch needs improvement: 1

Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by 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.

--
Ticket URL: <https://code.djangoproject.com/ticket/32548#comment:5>

[Django]

#32548: Support passing conditional expressions to Q().
-------------------------------------+-------------------------------------
Reporter: jonathan-golorry | Owner: jonathan-
Type: | golorry
Cleanup/optimization | Status: assigned
Component: Database layer | Version: dev
(models, ORM) |
Severity: Normal | Resolution:
Keywords: Q objects, | Triage Stage: Accepted
deconstruct |
Has patch: 1 | Needs documentation: 0
Needs tests: 1 | Patch needs improvement: 1

Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by 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)` and
`Q(x=1, y=2)`.

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.

--
Ticket URL: <https://code.djangoproject.com/ticket/32548#comment:6>

[Django]

#32548: Support passing conditional expressions to Q().
-------------------------------------+-------------------------------------
Reporter: jonathan-golorry | Owner: jonathan-
Type: | golorry
Cleanup/optimization | Status: assigned
Component: Database layer | Version: dev
(models, ORM) |
Severity: Normal | Resolution:
Keywords: Q objects, | Triage Stage: Accepted
deconstruct |
Has patch: 1 | Needs documentation: 0
Needs tests: 1 | Patch needs improvement: 1

Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by 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 🤷.

--
Ticket URL: <https://code.djangoproject.com/ticket/32548#comment:7>

[Django]

#32548: Support passing conditional expressions to Q().
-------------------------------------+-------------------------------------
Reporter: jonathan-golorry | Owner: jonathan-
Type: | golorry
Cleanup/optimization | Status: assigned
Component: Database layer | Version: dev
(models, ORM) |
Severity: Normal | Resolution:
Keywords: Q objects, | Triage Stage: Accepted
deconstruct |
Has patch: 1 | Needs documentation: 0
Needs tests: 1 | Patch needs improvement: 1

Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by 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.

--
Ticket URL: <https://code.djangoproject.com/ticket/32548#comment:8>

[Django]

#32548: Support passing conditional expressions to Q().
-------------------------------------+-------------------------------------
Reporter: jonathan-golorry | Owner: jonathan-
Type: | golorry
Cleanup/optimization | Status: assigned
Component: Database layer | Version: dev
(models, ORM) |
Severity: Normal | Resolution:
Keywords: Q objects, | Triage Stage: Accepted
deconstruct |
Has patch: 1 | Needs documentation: 0
Needs tests: 1 | Patch needs improvement: 1

Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by 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.

--
Ticket URL: <https://code.djangoproject.com/ticket/32548#comment:9>

[Django]

#32548: Support passing conditional expressions to Q().
-------------------------------------+-------------------------------------
Reporter: jonathan-golorry | Owner: jonathan-
Type: | golorry
Cleanup/optimization | Status: assigned
Component: Database layer | Version: dev
(models, ORM) |
Severity: Normal | Resolution:
Keywords: Q objects, | Triage Stage: Accepted
deconstruct |
Has patch: 1 | Needs documentation: 0
Needs tests: 1 | Patch needs improvement: 1

Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by Ian Foote):

Replying to [comment:9 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!

--
Ticket URL: <https://code.djangoproject.com/ticket/32548#comment:10>

[Django]

#32548: Combining Q() objects with boolean expressions crashes.

-------------------------------------+-------------------------------------
Reporter: jonathan-golorry | Owner: jonathan-

| golorry
Type: Bug | Status: assigned

Component: Database layer | Version: dev
(models, ORM) |
Severity: Normal | Resolution:
Keywords: Q objects, | Triage Stage: Accepted
deconstruct |
Has patch: 1 | Needs documentation: 0
Needs tests: 1 | Patch needs improvement: 1

Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Changes (by Mariusz Felisiak):

* type: Cleanup/optimization => Bug

Comment:

OK, let's treat this as a bugfix.

--
Ticket URL: <https://code.djangoproject.com/ticket/32548#comment:11>

[Django]

#32548: Combining Q() objects with boolean expressions crashes.
-------------------------------------+-------------------------------------
Reporter: jonathan-golorry | Owner: jonathan-
| golorry
Type: Bug | Status: assigned
Component: Database layer | Version: dev
(models, ORM) |
Severity: Normal | Resolution:

Keywords: Q objects, | Triage Stage: Ready for
deconstruct | checkin
Has patch: 1 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Changes (by Mariusz Felisiak):

* needs_better_patch: 1 => 0
* needs_tests: 1 => 0
* stage: Accepted => Ready for checkin

--
Ticket URL: <https://code.djangoproject.com/ticket/32548#comment:12>

[Django]

#32548: Combining Q() objects with boolean expressions crashes.
-------------------------------------+-------------------------------------
Reporter: jonathan-golorry | Owner: jonathan-
| golorry
Type: Bug | Status: assigned
Component: Database layer | Version: dev
(models, ORM) |
Severity: Normal | Resolution:
Keywords: Q objects, | Triage Stage: Ready for
deconstruct | checkin

Has patch: 1 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by Mariusz Felisiak <felisiak.mariusz@…>):

In [changeset:"54f60bc85df7fe38ba6ef6779996d8544d340d3e" 54f60bc]:
{{{
#!CommitTicketReference repository=""
revision="54f60bc85df7fe38ba6ef6779996d8544d340d3e"
Refs #32548 -- Added tests for passing conditional expressions to Q().
}}}

--
Ticket URL: <https://code.djangoproject.com/ticket/32548#comment:13>

[Django]

#32548: Combining Q() objects with boolean expressions crashes.
-------------------------------------+-------------------------------------
Reporter: jonathan-golorry | Owner: jonathan-
| golorry

Type: Bug | Status: closed

Component: Database layer | Version: dev
(models, ORM) |

Severity: Normal | Resolution: fixed

Keywords: Q objects, | Triage Stage: Ready for
deconstruct | checkin

Has patch: 1 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Changes (by Mariusz Felisiak <felisiak.mariusz@…>):

* status: assigned => closed
* resolution: => fixed

Comment:

In [changeset:"00b0786de533dbb3f6208d8d5eaddbf765b4e5b8" 00b0786]:
{{{
#!CommitTicketReference repository=""
revision="00b0786de533dbb3f6208d8d5eaddbf765b4e5b8"
Fixed #32548 -- Fixed crash when combining Q() objects with boolean
expressions.
}}}

--
Ticket URL: <https://code.djangoproject.com/ticket/32548#comment:14>

[Django]

#32548: Combining Q() objects with boolean expressions crashes.
-------------------------------------+-------------------------------------
Reporter: jonathan-golorry | Owner: jonathan-
| golorry
Type: Bug | Status: closed
Component: Database layer | Version: dev
(models, ORM) |
Severity: Normal | Resolution: fixed
Keywords: Q objects, | Triage Stage: Ready for
deconstruct | checkin

Has patch: 1 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by Mariusz Felisiak):

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

I've prepared a backport, [https://github.com/django/django/pull/14267
PR].

--
Ticket URL: <https://code.djangoproject.com/ticket/32548#comment:15>

[Django]

#32548: Combining Q() objects with boolean expressions crashes.
-------------------------------------+-------------------------------------
Reporter: jonathan-golorry | Owner: jonathan-
| golorry
Type: Bug | Status: closed
Component: Database layer | Version: dev
(models, ORM) |
Severity: Normal | Resolution: fixed
Keywords: Q objects, | Triage Stage: Ready for
deconstruct | checkin

Has patch: 1 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by Mariusz Felisiak <felisiak.mariusz@…>):

In [changeset:"d0267690f8a8e83065459d13a5a6f29e75640f78" d0267690]:
{{{
#!CommitTicketReference repository=""
revision="d0267690f8a8e83065459d13a5a6f29e75640f78"
[3.2.x] Fixed #32548 -- Fixed crash when combining Q() objects with
boolean expressions.

Backport of 00b0786de533dbb3f6208d8d5eaddbf765b4e5b8 from main.

Regression in 466920f6d726eee90d5566e0a9948e92b33a122e.
}}}

--
Ticket URL: <https://code.djangoproject.com/ticket/32548#comment:16>

[Django]

#32548: Combining Q() objects with boolean expressions crashes.
-------------------------------------+-------------------------------------
Reporter: jonathan-golorry | Owner: jonathan-
| golorry
Type: Bug | Status: closed
Component: Database layer | Version: dev
(models, ORM) |
Severity: Normal | Resolution: fixed
Keywords: Q objects, | Triage Stage: Ready for
deconstruct | checkin

Has patch: 1 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by Mariusz Felisiak <felisiak.mariusz@…>):

In [changeset:"338eb65b6ecca9c7334aae94ce0b44a0faca4b1d" 338eb65b]:
{{{
#!CommitTicketReference repository=""
revision="338eb65b6ecca9c7334aae94ce0b44a0faca4b1d"
Refs #32548 -- Forwardported 3.2.1 release note.
}}}

--
Ticket URL: <https://code.djangoproject.com/ticket/32548#comment:17>