[Django] #32554: Add Q.TRUE, Q.FALSE, Q.any() and Q.all()
Django
-------------------------------------+-------------------------------------
Reporter: jonathan- | Owner: nobody
golorry |
Type: New | Status: new
feature |
Component: Database | Version: dev
layer (models, ORM) |
Severity: Normal | Keywords: Q objects, any, all
Triage Stage: | Has patch: 0
Unreviewed |
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 0
UI/UX: 0 |
-------------------------------------+-------------------------------------
`Q.FALSE` and `Q.TRUE` are classattribute aliases for `Q(pk__in=[])` and
`~Q(pk__in=[])`.
`Q.any()` and `Q.all()` mimic python's builtin `any` and `all`, defaulting
to `Q.FALSE` and `Q.TRUE` for empty iterators (or iterators containing
only empty Q objects).
See this forum thread discussing why these are an improvement over many
ad-hoc implementations people are using:
https://forum.djangoproject.com/t/improving-q-objects-with-true-false-and-
none/851
My patch relies on https://code.djangoproject.com/ticket/32548 and
https://code.djangoproject.com/ticket/32549. I kept those as separate
tickets because they each had side effects that warranted their own
discussion.
--
Ticket URL: <https://code.djangoproject.com/ticket/32554>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
-------------------------------------+-------------------------------------
| golorry
Type: New feature | Status: assigned
Component: Database layer | Version: dev
(models, ORM) |
Severity: Normal | Resolution:
Keywords: Q objects, any, all | Triage Stage:
Has patch: 1 | Needs documentation: 1
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* owner: nobody => jonathan-golorry
* needs_docs: 0 => 1
* has_patch: 0 => 1
* status: new => assigned
Old description:
> `Q.FALSE` and `Q.TRUE` are classattribute aliases for `Q(pk__in=[])` and
> `~Q(pk__in=[])`.
>
> `Q.any()` and `Q.all()` mimic python's builtin `any` and `all`,
> defaulting to `Q.FALSE` and `Q.TRUE` for empty iterators (or iterators
> containing only empty Q objects).
>
> See this forum thread discussing why these are an improvement over many
> ad-hoc implementations people are using:
> https://forum.djangoproject.com/t/improving-q-objects-with-true-false-
> and-none/851
>
> My patch relies on https://code.djangoproject.com/ticket/32548 and
> https://code.djangoproject.com/ticket/32549. I kept those as separate
> tickets because they each had side effects that warranted their own
> discussion.
New description:
`Q.FALSE` and `Q.TRUE` are classattribute aliases for `Q(pk__in=[])` and
`~Q(pk__in=[])`.
`Q.any()` and `Q.all()` mimic python's builtin `any` and `all`, defaulting
to `Q.FALSE` and `Q.TRUE` for empty iterators (or iterators containing
only empty Q objects).
Patch: https://github.com/django/django/pull/14131
See this forum thread discussing why these are an improvement over many
ad-hoc implementations people are using:
https://forum.djangoproject.com/t/improving-q-objects-with-true-false-and-
none/851
My patch relies on https://code.djangoproject.com/ticket/32548 and
https://code.djangoproject.com/ticket/32549. I kept those as separate
tickets because they each had side effects that warranted their own
discussion.
--
--
Ticket URL: <https://code.djangoproject.com/ticket/32554#comment:1>
Django
-------------------------------------+-------------------------------------
| golorry
Type: New feature | Status: assigned
Component: Database layer | Version: dev
(models, ORM) |
Severity: Normal | Resolution:
Has patch: 1 | Needs documentation: 1
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Old description:
> `Q.FALSE` and `Q.TRUE` are classattribute aliases for `Q(pk__in=[])` and
> `~Q(pk__in=[])`.
>
> `Q.any()` and `Q.all()` mimic python's builtin `any` and `all`,
> defaulting to `Q.FALSE` and `Q.TRUE` for empty iterators (or iterators
> containing only empty Q objects).
>
> Patch: https://github.com/django/django/pull/14131
>
> See this forum thread discussing why these are an improvement over many
> ad-hoc implementations people are using:
> https://forum.djangoproject.com/t/improving-q-objects-with-true-false-
> and-none/851
>
> My patch relies on https://code.djangoproject.com/ticket/32548 and
> https://code.djangoproject.com/ticket/32549. I kept those as separate
> tickets because they each had side effects that warranted their own
> discussion.
New description:
`Q.empty()` checks for nested empty Q objects such as `Q(Q())`. See #32549
for rationale.
`Q.FALSE` and `Q.TRUE` are classattribute aliases for `Q(pk__in=[])` and
`~Q(pk__in=[])`.
`Q.any()` and `Q.all()` mimic python's builtin `any` and `all`, defaulting
to `Q.FALSE` and `Q.TRUE` for empty iterators (or iterators containing
only empty Q objects).
Patch: https://github.com/django/django/pull/14159
See this forum thread discussing why these are an improvement over many
ad-hoc implementations people are using:
https://forum.djangoproject.com/t/improving-q-objects-with-true-false-and-
none/851
--
--
Ticket URL: <https://code.djangoproject.com/ticket/32554#comment:2>
Django
-------------------------------------+-------------------------------------
Reporter: jonathan-golorry | Owner: jonathan-
| golorry
Type: New feature | Status: assigned
Component: Database layer | Version: dev
(models, ORM) |
Severity: Normal | Resolution:
Has patch: 1 | Needs documentation: 1
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
--
Ticket URL: <https://code.djangoproject.com/ticket/32554#comment:3>
Django
-------------------------------------+-------------------------------------
Reporter: jonathan-golorry | Owner: jonathan-
| golorry
Component: Database layer | Version: dev
(models, ORM) |
Keywords: Q objects, any, all | Triage Stage:
Has patch: 1 | Needs documentation: 1
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* status: assigned => closed
* resolution: => wontfix
Comment:
Thanks for this ticket, however I have mixed-feelings. Django API is
already massive. IMO new hooks will make `Q` expressions more confusing
and are not necessary per se. `Q.any()` and `Q.all()` can be replaced with
`reduce()` (that's how we do this in Django itself, see
[https://github.com/django/django/blob/7c08f26bf0439c1ed593b51b51ad847f7e262bc1/django/contrib/admin/options.py#L1032
an example]). I don't see why the new API should be preferred over an
example with `reduce()` in
[https://docs.djangoproject.com/en/dev/topics/db/queries/#complex-lookups-
with-q-objects docs]. It has already been
[https://github.com/django/django/pull/13798#issuecomment-767492096
pointed out] that we need a good example of building `Q()` dynamically.
About `Q.TRUE` and `Q.FALSE`, these constants are really tricky and niche.
I don't believe that folks will not how to use them properly (I don't see
immediate and wide use). Moreover, I'm not sure why Django should
recommend the `pk__in=[]` lookup (and not e.g. `pk=-1`), everything
depends on the context. It is so niche that folks should choose on their
own.
I strongly believe that we need a good example of building `Q()`
dynamically with the existing API, rather than adding yet another method
to do the same.
--
Ticket URL: <https://code.djangoproject.com/ticket/32554#comment:4>
Django
-------------------------------------+-------------------------------------
Reporter: jonathan-golorry | Owner: jonathan-
| golorry
Type: New feature | Status: closed
Component: Database layer | Version: dev
(models, ORM) |
Severity: Normal | Resolution: wontfix
Has patch: 1 | Needs documentation: 1
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by jonathan-golorry):
Replying to [comment:4 Mariusz Felisiak]:
> Thanks for this ticket, however I have mixed-feelings. Django API is
already massive. IMO new hooks will make `Q` expressions more confusing
and are not necessary per se. `Q.any()` and `Q.all()` can be replaced with
`reduce()` (that's how we do this in Django itself, see
[https://github.com/django/django/blob/7c08f26bf0439c1ed593b51b51ad847f7e262bc1/django/contrib/admin/options.py#L1032
an example]). I don't see why the new API should be preferred over an
example with `reduce()` in
[https://docs.djangoproject.com/en/dev/topics/db/queries/#complex-lookups-
with-q-objects docs]. It has already been
[https://github.com/django/django/pull/13798#issuecomment-767492096
pointed out] that we need a good example of building `Q()` dynamically.
`reduce` isn't great. A common mistake I see in accepted answers on stack
overflow is that people forget to address empty iterators (potentially
leaking data due to empty `Q()` objects).
https://stackoverflow.com/questions/13076822/django-dynamically-filtering-
with-q-objects
https://stackoverflow.com/questions/852414/how-to-dynamically-compose-an-
or-query-filter-in-django
`reduce` turns that into a TypeError, which is better than leaking data,
but handling the case properly is still a pain. It's hard to explain why
some initializers work as defaults and others don't.
{{{
reduce(operator.and_, iterator, initializer=~Q(pk__in=[]) # "all",
defaulting to everything
reduce(operator.or_, iterator, initializer=Q(pk__in=[]) # "any",
defaulting to nothing
reduce(operator.and_, iterator, initializer=Q(pk__in=[]) # always
returns nothing
reduce(operator.or_, iterator, initializer=~Q(pk__in=[]) # always
returns everything
}}}
That's ignoring the absolute foot-gun that is `initializer=Q()`. You need
to add logically meaningless expressions to make the queries safe.
{{{
reduce(operator.and_, iterator, initializer=Q()) | Q(pk__in=[]) # "all",
defaulting to nothing
reduce(operator.or_, iterator, initializer=Q()) & ~Q(pk__in=[]) # "or",
defaulting to everything
}}}
Using manual checks to avoid needing initializer isn't great because of
edge cases around `bool(Q(Q()))` and `iterator=[Q()]`.
> About `Q.TRUE` and `Q.FALSE`, these constants are really tricky and
niche. I don't believe that folks will not how to use them properly (I
don't see immediate and wide use). Moreover, I'm not sure why Django
should recommend the `pk__in=[]` lookup (and not e.g. `pk=-1`), everything
depends on the context. It is so niche that folks should choose on their
own.
The main argument for having `Q.TRUE` and `Q.FALSE` is to make the above
code examples more readable. The logic of ANDing with TRUE is already
weird enough that it's easy to miss a `~`.
The query optimizer can handle `pk__in=[]`, not the others.
{{{
Article.objects.filter(Q(pk__in=[])) # doesn't hit DB
Article.objects.filter(Q(pk=None)) # hits DB and returns nothing
Article.objects.none() # doesn't hit DB
Article.objects.filter(Q()) # returns everything
}}}
> I strongly believe that we need a good example of building `Q()`
dynamically with the existing API, rather than adding yet another method
to do the same.
I think empty `Q()` objects are a significant security risk. The code
required to handle them properly is unintuitive and difficult to read. If
we just documented a good example, it would need to sufficiently explain
all these risks.
--
Ticket URL: <https://code.djangoproject.com/ticket/32554#comment:5>