#35154: QuerySet implements `contains` but not `__contains__`
-------------------------------------+-------------------------------------
Reporter: fidoriel | Owner: nobody
Type: New feature | Status: closed
Component: Database layer | Version: 5.0
(models, ORM) |
Severity: Normal | Resolution: wontfix
Keywords: queryset contains | Triage Stage:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Richard Ebeling):
(Collaborator of @fidoriel here)
=== Background: Type Annotation
One intention of this issue is to improve the usability of type-
annotations and static type checking with django code. Due to the missing
`__contains__` method, `QuerySet`s do not fulfil the typing requirements
to be a `Container`, so passing them to a method that expects a
`Container` will fail static type checking. In consequence, they also do
not meet the requirements of `Collection` and `Sequence`. This makes type-
annotating methods that take, e.g., a list or a queryset, verbose und
unnecessarily specific, since you always have to type as "takes a
Container[x] OR a QuerySet[x]".
One could argue that this is a problem with Python's abstract base classes
and the fact that most ABCs inherit from `Container`, when most users
don't really care if `__contains__` is present or not. If it isn't, an
`element in our_container_like_object` test still is valid using the
fallback mechanism from above. However, a QuerySet semantically fulfills
all the requirements of `Container`, so I don't think this is a reason to
not make it one.
=== Past Discussion
The discussion in #31561 revolves around `Set` not being a suitable base
class because `Set`s do not have an order, but `QuerySet`s do. From what I
see, it does not give any points against making `QuerySet` a
`collections.abc.Collection` (by adding `__contains__`) in general.
In #24141, Carl Meyer said
> I think the original PR (to implement `QuerySet.__contains__()` as
`.filter(pk=obj.pk).exists()` under the hood) is a non-starter
which I understand as criticism towards this specific implementation
(`__contains__` will be `.filter(pk=obj.pk).exists()` under the hood,
which always hits the database, even if objects are already cached -- this
is unexpected in comparison to other helper methods that use the object
cache). I don't see any arguments against having `__contains__`
implemented here.
The discussion in https://groups.google.com/g/django-
developers/c/NZaMq9BALrs/m/OCNTh6QyCAAJ rejects the idea of implementing
`.__contains__()` identically to the current implementation of
`.contains()`, i.e., not evaluating the queryset. I don't see an argument
against having _some_ implementation of `__contains__`.
The discussion in https://github.com/django/django/pull/3906 concludes
that `x in qs` should always evaluate an unevaluated queryset. I don't see
an argument to not have `__contains()__`, just a requirement towards its
behavior.
I didn't find arguments against implementing `__contains__` in the
discussion of https://github.com/django/django/pull/13038.
Unless I'm missing something here, there's not really a point against
having `__contains__` in general. There are the arguments that
* a `__contains__` implementation shouldn't always hit the database if
objects are already cached (agree)
* `len(qs)`, `bool(qs)`, `x in qs` should be consistent in whether they
evaluate a queryset or not (agree)
* `x in qs` should evaluate the queryset (I guess debatable depending on
perspective, but not our goal in this ticket)
=== Proposal
In general, it is pythonic to test membership by using `in`. Currently,
the django documentation does not say what happens in this case, you have
to be aware of the elementwise-iteration-and-comparison fallback in python
and conclude that this iteration implies that the queryset will be
evaluated.
If we implemented `__contains__` as `evalute if not evaluted, then return
contains()`, we wouldn't have any changes in behavior, but `QuerySet`
would be a `Container`. Additionally, the documentation could include
this, e.g. at https://docs.djangoproject.com/en/5.0/ref/models/querysets
/#when-querysets-are-evaluated. I think both are desirable.
Thoughts / Did I miss something?
--
Ticket URL: <https://code.djangoproject.com/ticket/35154#comment:3>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
--
You received this message because you are subscribed to the Google Groups
"Django updates" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to [email protected].
To view this discussion on the web visit
https://groups.google.com/d/msgid/django-updates/0107018d79a8258d-9d1fc677-2a1c-4c34-ae79-15b96031548c-000000%40eu-central-1.amazonses.com.
