[Django] #32549: Make `Q(Q(), ...)` equivalent to `Q(...)`
Django
-------------------------------------+-------------------------------------
Reporter: jonathan- | Owner: nobody
golorry |
Type: | Status: new
Cleanup/optimization |
Component: Database | Version: dev
layer (models, ORM) | Keywords: Q objects, nested,
Severity: Normal | empty
Triage Stage: | Has patch: 0
Unreviewed |
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 0
UI/UX: 0 |
-------------------------------------+-------------------------------------
Empty Q objects (`Q()` or `~Q()`, etc) evaluate to `False`, but `Q(Q())`
evaluates to `True`. This interferes with the shortcut on logical
operations involving empty Q objects and can lead to bugs when trying to
detect empty operations.
{{{
def q_any(iterable):
q = Q()
for element in iterable:
q |= element
if q:
return q
return Q(pk__in=[])
Item.objects.filter(q_any()) # no items
Item.objects.filter(q_any([Q()])) # no items
Item.objects.filter(q_any([Q(Q())])) # all items
}}}
This patch removes empty Q objects from `args` during Q object
initialization.
This requires https://code.djangoproject.com/ticket/32548 in order to
prevent a regression in logical operations between query expressions and
empty Q objects.
--
Ticket URL: <https://code.djangoproject.com/ticket/32549>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
-------------------------------------+-------------------------------------
Type: | Status: new
Cleanup/optimization |
Component: Database layer | Version: dev
(models, ORM) |
Severity: Normal | Resolution:
Keywords: Q objects, nested, | Triage Stage:
empty | Unreviewed
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* has_patch: 0 => 1
Old description:
> Empty Q objects (`Q()` or `~Q()`, etc) evaluate to `False`, but `Q(Q())`
> evaluates to `True`. This interferes with the shortcut on logical
> operations involving empty Q objects and can lead to bugs when trying to
> detect empty operations.
> {{{
> def q_any(iterable):
> q = Q()
> for element in iterable:
> q |= element
> if q:
> return q
> return Q(pk__in=[])
> Item.objects.filter(q_any()) # no items
> Item.objects.filter(q_any([Q()])) # no items
> Item.objects.filter(q_any([Q(Q())])) # all items
> }}}
>
> This patch removes empty Q objects from `args` during Q object
> initialization.
>
> This requires https://code.djangoproject.com/ticket/32548 in order to
> prevent a regression in logical operations between query expressions and
> empty Q objects.
New description:
Empty Q objects (`Q()` or `~Q()`, etc) evaluate to `False`, but `Q(Q())`
evaluates to `True`. This interferes with the shortcut on logical
operations involving empty Q objects and can lead to bugs when trying to
detect empty operations.
{{{
def q_any(iterable):
q = Q()
for element in iterable:
q |= element
if q:
return q
return Q(pk__in=[])
Item.objects.filter(q_any()) # no items
Item.objects.filter(q_any([Q()])) # no items
Item.objects.filter(q_any([Q(Q())])) # all items
}}}
Patch https://github.com/django/django/pull/14127 removes empty Q objects
from `args` during Q object initialization.
This requires https://code.djangoproject.com/ticket/32548 in order to
prevent a regression in logical operations between query expressions and
empty Q objects.
--
--
Ticket URL: <https://code.djangoproject.com/ticket/32549#comment:1>
Django
-------------------------------------+-------------------------------------
Type: | Status: closed
Cleanup/optimization |
Component: Database layer | Version: dev
(models, ORM) |
Keywords: Q objects, nested, | Triage Stage:
empty | Unreviewed
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* status: new => closed
* resolution: => wontfix
Comment:
Thanks for this proposition, however I don't think we should implicitly
remove anything from `args`, even an empty `Q()` instances. This would be
backward incompatible, inconsistent with the `Node` behavior, and can be
handled in your code.
--
Ticket URL: <https://code.djangoproject.com/ticket/32549#comment:2>
Django
-------------------------------------+-------------------------------------
Reporter: jonathan-golorry | Owner: nobody
Cleanup/optimization |
Component: Database layer | Version: dev
(models, ORM) |
Severity: Normal | Resolution:
empty | Unreviewed
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* status: closed => new
* resolution: wontfix =>
Old description:
> Empty Q objects (`Q()` or `~Q()`, etc) evaluate to `False`, but `Q(Q())`
> evaluates to `True`. This interferes with the shortcut on logical
> operations involving empty Q objects and can lead to bugs when trying to
> detect empty operations.
> {{{
> def q_any(iterable):
> q = Q()
> for element in iterable:
> q |= element
> if q:
> return q
> return Q(pk__in=[])
> Item.objects.filter(q_any()) # no items
> Item.objects.filter(q_any([Q()])) # no items
> Item.objects.filter(q_any([Q(Q())])) # all items
> }}}
>
> Patch https://github.com/django/django/pull/14127 removes empty Q objects
> from `args` during Q object initialization.
>
> This requires https://code.djangoproject.com/ticket/32548 in order to
> prevent a regression in logical operations between query expressions and
> empty Q objects.
New description:
Empty Q objects (`Q()` or `~Q()`, etc) evaluate to `False`, but `Q(Q())`
evaluates to `True`. This interferes with the shortcut on logical
operations involving empty Q objects and can lead to bugs when trying to
detect empty operations.
{{{
def q_any(iterable):
q = Q()
for element in iterable:
q |= element
if q:
return q
return Q(pk__in=[])
Item.objects.filter(q_any()) # no items
Item.objects.filter(q_any([Q()])) # no items
Item.objects.filter(q_any([Q(Q())])) # all items
}}}
~~Patch https://github.com/django/django/pull/14127 removes empty Q
objects from `args` during Q object initialization.~~
Patch https://github.com/django/django/pull/14130 adds `.empty()` for
detecting nested empty Q objects and uses that instead of boolean
evaluation for shortcutting logical operations between empty Q objects. So
`Q(x=1) | Q(Q())` will now shortcut to `Q(x=1)` the same way `Q(x=1) |
Q()` does. No simplifying is done during Q object initialization.
This requires https://code.djangoproject.com/ticket/32548 in order to
prevent a regression in logical operations between query expressions and
empty Q objects.
--
Comment:
Here is an alternate approach that adds a way to check for nested empty Q
objects instead of making them impossible to create.
--
Ticket URL: <https://code.djangoproject.com/ticket/32549#comment:3>
Django
-------------------------------------+-------------------------------------
Reporter: jonathan-golorry | Owner: nobody
Cleanup/optimization |
Component: Database layer | Version: dev
(models, ORM) |
Keywords: Q objects, nested, | Triage Stage:
empty | Unreviewed
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* status: new => closed
* resolution: => wontfix
Comment:
Replying to [comment:3 jonathan-golorry]:
> Here is an alternate approach that adds a way to check for nested empty
Q objects instead of making them impossible to create.
It looks that you need `Q.empty()` as a hook for #32554 (to which I'm also
not convinced, but we can discuss this in #32554). I don't think that we
need this as a standalone feature. Moreover it's still backward
incompatible, because it changes the current behavior when combining a
nested `Q()` objects. It's also inconsistent with the current API and
quite misleading, e.g.
{{{
>>> from django.db.models import Q
>>> q = Q(Q())
>>> q.empty()
True
>>> len(q)
1
>>> bool(q)
True
}}}
I understand that you need this for building generic nested queries but
it's not needed for most users. You can always use your own subclass of
`Q` and even release it as a third-party package.
[https://docs.djangoproject.com/en/stable/internals/contributing/triaging-
tickets/#closing-tickets Please follow triaging guidelines with regards to
wontfix tickets.]
--
Ticket URL: <https://code.djangoproject.com/ticket/32549#comment:4>
Django
-------------------------------------+-------------------------------------
Reporter: jonathan-golorry | Owner: nobody
Type: | Status: closed
Cleanup/optimization |
Component: Database layer | Version: dev
(models, ORM) |
Severity: Normal | Resolution: wontfix
Keywords: Q objects, nested, | Triage Stage:
empty | Unreviewed
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by jonathan-golorry):
We already shortcut logical operations between empty Q objects, so I view
this more as an extension of that shortcutting (for legibility, not
performance). I don't think it's fair to say it's backwards incompatible,
since the simplified Q objects will be logically equivalent to the old
ones. The expected outcome of Q object combinations is logical
consistency, not a specific tree structure (as evidenced by `len((Q(x=1,
y=2) | Q(x=1, y=2)).children == 1)`.
My original example shows why it's desirable to have `q.empty()` behave
differently from `not bool(q)`. Empty Q objects are a potential security
risk, but the standard way to check for them doesn't catch nested empty Q
objects. #32554 doesn't actually build nested queries, it just uses this
to avoid bugs with nested query inputs.
My apologies for reopening without a mailing list consensus, though I feel
"If there is a way they can improve the ticket to reopen it, let them
know" indicates that I may reopen if I address the only concern you
expressed in your closing comment. If the reason for closing is that
legibility of Q objects isn't it's own concern, I can roll `empty` into
#32554 without the logic shortcut.
--
Ticket URL: <https://code.djangoproject.com/ticket/32549#comment:5>
Django
-------------------------------------+-------------------------------------
Reporter: jonathan-golorry | Owner: nobody
Type: | Status: closed
Cleanup/optimization |
Component: Database layer | Version: dev
(models, ORM) |
Severity: Normal | Resolution: wontfix
Keywords: Q objects, nested, | Triage Stage:
empty | Unreviewed
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Ian Foote):
I'm persuaded that this is a problem. Accidentally leaking data because we
treat {{{Q(Q())}}} differently to {{{Q()}}} feels like a bug to me, so
fixing it shouldn't be blocked by backwards compatibility arguments.
--
Ticket URL: <https://code.djangoproject.com/ticket/32549#comment:6>
Django
-------------------------------------+-------------------------------------
Reporter: jonathan-golorry | Owner: nobody
Type: | Status: closed
Cleanup/optimization |
Component: Database layer | Version: dev
(models, ORM) |
Severity: Normal | Resolution: wontfix
Keywords: Q objects, nested, | Triage Stage:
empty | Unreviewed
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by jonathan-golorry):
Replying to [comment:6 Ian Foote]:
> I'm persuaded that this is a problem. Accidentally leaking data because
we treat {{{Q(Q())}}} differently to {{{Q()}}} feels like a bug to me, so
fixing it shouldn't be blocked by backwards compatibility arguments.
#32554 does a lot more to prevent accidentally leaking data. If that's the
only concern, I think it's better to roll `Q.empty()` into that patch.
This ticket is primarily about whether or not to shortcut logical
operations between nested empty Q objects.
--
Ticket URL: <https://code.djangoproject.com/ticket/32549#comment:7>