[Django] #25787: Existence checks for related database sets
Django
-------------------------------------+-------------------------------------
Reporter: JustusAdam | Owner: nobody
Type: New feature | Status: new
Component: Database layer | Version: 1.8
(models, ORM) | Keywords: ManyToMany, contains,
Severity: Normal | in
Triage Stage: Unreviewed | Has patch: 0
Easy pickings: 1 | UI/UX: 0
-------------------------------------+-------------------------------------
I have recently had to check for presence of a particular object in a set
of related objects (either ManyToMany or ForeignKey) and I found it
unnecessary cumbersome not to be able to use the
{{{in}}} operator.
I propose to add an implementation of the {{{__contains__}}} method to the
{{{ManyRelatedManager}}} object which executes something like the
following query:
{{{
def __contains__(self, obj):
return self.filter(id=obj.id).exists()
}}}
which would enable the following sample behaviour:
{{{
my_student = request.user.student
my_course = Course.objects.get(id=course_id)
is_teacher = my_student in my_course.teachers
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/25787>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
-------------------------------------+-------------------------------------
Reporter: JustusAdam | Owner:
Type: New feature | Status: assigned
Component: Database layer | Version: 1.8
(models, ORM) |
Keywords: ManyToMany, | Triage Stage:
contains, in | Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
-------------------------------------+-------------------------------------
* owner: nobody => andersonresende
* needs_better_patch: => 0
* status: new => assigned
* needs_tests: => 0
* needs_docs: => 0
--
Ticket URL: <https://code.djangoproject.com/ticket/25787#comment:1>
Django
-------------------------------------+-------------------------------------
Reporter: JustusAdam | Owner:
| andersonresende
Component: Database layer | Version: 1.8
(models, ORM) |
Keywords: ManyToMany, | Triage Stage:
contains, in | Unreviewed
Has patch: 0 | Needs documentation: 0
Easy pickings: 1 | UI/UX: 0
-------------------------------------+-------------------------------------
* status: assigned => closed
* resolution: => wontfix
Comment:
Doesn't `my_student in my_course.teachers.all()` work? I don't think a
`__contains__` shortcut belongs on the manager -- otherwise users might
expect the rest of QuerySet comparisons like `__eq__` to work as well.
--
Ticket URL: <https://code.djangoproject.com/ticket/25787#comment:2>
Django
-------------------------------------+-------------------------------------
Reporter: JustusAdam | Owner:
| andersonresende
Type: New feature | Status: closed
(models, ORM) |
Keywords: ManyToMany, | Triage Stage:
contains, in | Unreviewed
Has patch: 0 | Needs documentation: 0
Easy pickings: 1 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by JustusAdam):
If that executes lazily then I am ok with this, thank you.
--
Ticket URL: <https://code.djangoproject.com/ticket/25787#comment:3>
Django
-------------------------------------+-------------------------------------
Reporter: JustusAdam | Owner:
| andersonresende
Type: New feature | Status: closed
(models, ORM) |
Keywords: ManyToMany, | Triage Stage:
contains, in | Unreviewed
Has patch: 0 | Needs documentation: 0
Easy pickings: 1 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by timgraham):
True, hopefully the documentation added in #17156 addresses that.
--
Ticket URL: <https://code.djangoproject.com/ticket/25787#comment:4>
Django
-------------------------------------+-------------------------------------
Reporter: JustusAdam | Owner:
| andersonresende
Type: New feature | Status: closed
(models, ORM) |
Keywords: ManyToMany, | Triage Stage:
contains, in | Unreviewed
Has patch: 0 | Needs documentation: 0
Easy pickings: 1 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by JustusAdam):
Why can't the QuerySet itself make the decision whether to use
{{{__iter__}}} or {{{self.filter(..).exists()}}}? It should have the
necessary knowledge. If it is itself unevaluated using {{{.exists()}}} is
the more efficient way in any situation and if it is evaluated then the
choice should depend on the size of the data.
--
Ticket URL: <https://code.djangoproject.com/ticket/25787#comment:5>
Django
-------------------------------------+-------------------------------------
Reporter: JustusAdam | Owner:
| andersonresende
Type: New feature | Status: closed
(models, ORM) |
Keywords: ManyToMany, | Triage Stage:
contains, in | Unreviewed
Has patch: 0 | Needs documentation: 0
Easy pickings: 1 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by timgraham):
One example I can think of is that the QuerySet doesn't know if it's used
later where it would need to be evaluated anyway. It seems simpler to me
if a developer can rely on a particular operation always working in a
certain way.
--
Ticket URL: <https://code.djangoproject.com/ticket/25787#comment:6>
Django
-------------------------------------+-------------------------------------
Reporter: JustusAdam | Owner:
| andersonresende
Type: New feature | Status: closed
(models, ORM) |
Keywords: ManyToMany, | Triage Stage:
contains, in | Unreviewed
Has patch: 0 | Needs documentation: 0
Easy pickings: 1 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by JustusAdam):
I understand but I think this should be explained in the documentation
rather than using the inefficient way straight away.
Any (normal) user, that does not dig too deep into the workings of
QuerySets will, use {{{in}}} instead of {{{filter().exists()}}} because
that's the python way of doing it. I don't think it is a good idea to
condemn them to potentially very inefficient queries for an operation that
could be much cheaper.
The actual behaviour of {{{__contains__}}} could be explained in the
documentation which someone very worried about efficiency can read and
then use {{{list()}}} to force evaluation if they need a reproducible
query.
{{{filter().exists()}}} poses very little overhead because it queries on
an indexed field that I don't think we'd have to worry about the overhead.
It is not like the current implementation doesn't have its pitfalls. Using
{{{list()}}} on an evaluated QuerySet won't execute queries unlike on en
unevaluated QuerySet. What I am trying to say is that operations on
QuerySet's ''already'' do not work the same way every time, that's simply
their nature. As I understand the mission of django it is to abstract away
complexity and deliver efficiency, which I believe this change would
bring.
Theoretically even using {{{in}}} on an evaluated query set is potentially
very inefficient if the set is large, since it is stored in a list rather
than a {{{set}}}.
--
Ticket URL: <https://code.djangoproject.com/ticket/25787#comment:7>
Django
-------------------------------------+-------------------------------------
Reporter: JustusAdam | Owner:
| andersonresende
Type: New feature | Status: closed
(models, ORM) |
Keywords: ManyToMany, | Triage Stage:
contains, in | Unreviewed
Has patch: 0 | Needs documentation: 0
Easy pickings: 1 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by timgraham):
The idea has been [https://github.com/django/django/pull/3906 proposed
before] (actually [https://github.com/django/django/pull/480 more than
once]). You can raise your proposal on the DevelopersMailingList if you
want to get another opinion.
The documentation added in the related ticket seems okay to me, but feel
free to submit a patch with any clarifications you'd like to see.
--
Ticket URL: <https://code.djangoproject.com/ticket/25787#comment:8>
Django
-------------------------------------+-------------------------------------
Reporter: JustusAdam | Owner:
| andersonresende
Type: New feature | Status: closed
(models, ORM) |
Keywords: ManyToMany, | Triage Stage:
contains, in | Unreviewed
Has patch: 0 | Needs documentation: 0
Easy pickings: 1 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by JustusAdam):
Nah, that's fine. If there has been a deliberate decision made against it
I will refrain from pushing this further. After all this is an opinionated
change and if the django development team feels differently about this
than I do that's fine.
--
Ticket URL: <https://code.djangoproject.com/ticket/25787#comment:9>