[Django] #30325: Inconsistent count() behaviour on reverse relations in ManyToManyFields with custom model manager
Django
with custom model manager
-------------------------------------+-------------------------------------
Reporter: Tobias | Owner: nobody
Kunze |
Type: Bug | Status: new
Component: Database | Version: 2.2
layer (models, ORM) |
Severity: Normal | Keywords:
Triage Stage: | Has patch: 0
Unreviewed |
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 0
UI/UX: 0 |
-------------------------------------+-------------------------------------
When looking up a reverse relation on a ManyToManyField that uses a custom
model manager, not using `all()` before calling `count()` will not use the
custom model manager. This results in `obj.m2m_lookup.count()` yielding a
different result than `obj.m2m_lookup.all().count`, which is new behaviour
in 2.2, and unexpected (at least to me).
I've written up a [https://github.com/rixx/django-22-issue minimal test
case] to showcase the issue. Inlining here:
{{{
#!python
from django.db import models
class Author(models.Model):
name = models.CharField(max_length=100)
class BookManager(models.Manager):
def get_queryset(self):
return super().get_queryset().exclude(state='deleted')
class Book(models.Model):
title = models.CharField(max_length=100)
authors = models.ManyToManyField(to=Author, related_name='books')
state = models.CharField(choices=(('regular', 'regular'), ('deleted',
'deleted')), default='regular', max_length=100)
objects = BookManager()
a = Author.objects.create(name='Bill')
b1 = Book.objects.create(title='Sonnets', state='deleted')
b2 = Book.objects.create(title='Hamlet')
b1.authors.add(a)
b2.authors.add(a)
a.books.count() # Returns 2
a.books.all().count() # Returns 1
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/30325>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
with custom model manager
-------------------------------------+-------------------------------------
Type: Bug | Status: closed
Component: Database layer | Version: 2.2
(models, ORM) |
Severity: Normal | Resolution: invalid
Keywords: | Triage Stage:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* status: new => closed
* resolution: => invalid
Comment:
a.books.all() is managed by the BookManager so it exclude the 'deleted'
book b1
--
Ticket URL: <https://code.djangoproject.com/ticket/30325#comment:1>
Django
with custom model manager
-------------------------------------+-------------------------------------
Type: Bug | Status: new
Component: Database layer | Version: 2.2
(models, ORM) |
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* status: closed => new
* resolution: invalid =>
* severity: Normal => Release blocker
* stage: Unreviewed => Accepted
Comment:
Reproduced at 1ffddfc233e2d5139cc6ec31a4ec6ef70b10f87f and works in Django
2.1.
Thanks for the report. Can you bisect to check which commit change this
behavior?
--
Ticket URL: <https://code.djangoproject.com/ticket/30325#comment:2>
Django
with custom model manager
-------------------------------------+-------------------------------------
Component: Database layer | Version: 2.2
(models, ORM) |
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* status: new => closed
* resolution: => invalid
Comment:
This is not a bug!
--
Ticket URL: <https://code.djangoproject.com/ticket/30325#comment:3>
Django
with custom model manager
-------------------------------------+-------------------------------------
Type: Bug | Status: new
Component: Database layer | Version: 2.2
(models, ORM) |
Severity: Release blocker | Resolution:
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* status: closed => new
* resolution: invalid =>
Comment:
PizzolatoDavide Please do not shout.
`a.books.count() != a.books.all().count()` for me it is a bug, moreover
this behavior has changed in Django 2.2.
--
Ticket URL: <https://code.djangoproject.com/ticket/30325#comment:4>
Django
with custom model manager
-------------------------------------+-------------------------------------
Type: Bug | Status: new
Component: Database layer | Version: 2.2
(models, ORM) |
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Tobias Kunze):
> This is not a bug!
Could you elaborate on this? As it is changed behaviour from Django 2.1
and previous versions, it's either a bug/regression, or a documentation
bug by not mentioning this changed behaviour in the release notes, in my
opinion. Up until now I've assumed that `something.related.count()` and
`something.related.all().count()` will yield the same result, and if this
is not true, it should be documented. (Or is it documented? Could you
point me at the right place to look?)
--
Ticket URL: <https://code.djangoproject.com/ticket/30325#comment:5>
Django
with custom model manager
-------------------------------------+-------------------------------------
Type: Bug | Status: new
Component: Database layer | Version: 2.2
(models, ORM) |
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Marten Kenbeek):
Regression caused by [changeset:"1299421cadc4fcf63585f2f88337078e43e660e0"
1299421] (#29725). `.count()` and `.exists()` on a many-to-many relation
now use the through model's base manager instead of the related model's
default manager.
--
Ticket URL: <https://code.djangoproject.com/ticket/30325#comment:6>
Django
with custom model manager
-------------------------------------+-------------------------------------
Type: Bug | Status: new
Component: Database layer | Version: 2.2
(models, ORM) |
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Tobias Kunze):
As per discussion at DjangoCon Europe, the patch in question will probably
be reverted, closing this issue (and re-opening #29725, potentially). I've
submitted regression tests: https://github.com/django/django/pull/11205
--
Ticket URL: <https://code.djangoproject.com/ticket/30325#comment:7>
Django
with custom model manager
-------------------------------------+-------------------------------------
| Kunze
Type: Bug | Status: assigned
Component: Database layer | Version: 2.2
(models, ORM) |
Severity: Release blocker | Resolution:
| checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* owner: nobody => Tobias Kunze
* status: new => assigned
* has_patch: 0 => 1
* stage: Accepted => Ready for checkin
Comment:
[https://github.com/django/django/pull/11205 PR]
--
Ticket URL: <https://code.djangoproject.com/ticket/30325#comment:8>
Django
with custom model manager
-------------------------------------+-------------------------------------
| Kunze
Component: Database layer | Version: 2.2
(models, ORM) |
Keywords: | Triage Stage: Ready for
| checkin
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* status: assigned => closed
* resolution: => fixed
Comment:
In [changeset:"5f7991c42cff73b6278106d499d719b726f85ead" 5f7991c4]:
{{{
#!CommitTicketReference repository=""
revision="5f7991c42cff73b6278106d499d719b726f85ead"
Fixed #30325 -- Reverted "Fixed #29725 -- Removed unnecessary join in
QuerySet.count() and exists() on a many-to-many relation."
This reverts commit 1299421cadc4fcf63585f2f88337078e43e660e0 due to
a regression with custom managers.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/30325#comment:9>
Django
with custom model manager
-------------------------------------+-------------------------------------
| Kunze
Type: Bug | Status: closed
Component: Database layer | Version: 2.2
(models, ORM) |
Severity: Release blocker | Resolution: fixed
Keywords: | Triage Stage: Ready for
| checkin
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Mariusz Felisiak <felisiak.mariusz@…>):
In [changeset:"9ac8520fcde29840a1345be19d80dbda53aa6d03" 9ac8520]:
{{{
#!CommitTicketReference repository=""
revision="9ac8520fcde29840a1345be19d80dbda53aa6d03"
Refs #30325 -- Added tests for using count()/exists() with custom managers
and reverse M2M relations.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/30325#comment:10>
Django
with custom model manager
-------------------------------------+-------------------------------------
| Kunze
Type: Bug | Status: closed
Component: Database layer | Version: 2.2
(models, ORM) |
Severity: Release blocker | Resolution: fixed
Keywords: | Triage Stage: Ready for
| checkin
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Mariusz Felisiak <felisiak.mariusz@…>):
In [changeset:"e8de1cc94c9a251ba7c569468f1e94faf9a05671" e8de1cc9]:
{{{
#!CommitTicketReference repository=""
revision="e8de1cc94c9a251ba7c569468f1e94faf9a05671"
[2.2.x] Fixed #30325 -- Reverted "Fixed #29725 -- Removed unnecessary join
in QuerySet.count() and exists() on a many-to-many relation."
This reverts commit 1299421cadc4fcf63585f2f88337078e43e660e0 due to
a regression with custom managers.
Backport of 5f7991c42cff73b6278106d499d719b726f85ead from master
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/30325#comment:11>
Django
with custom model manager
-------------------------------------+-------------------------------------
| Kunze
Type: Bug | Status: closed
Component: Database layer | Version: 2.2
(models, ORM) |
Severity: Release blocker | Resolution: fixed
Keywords: | Triage Stage: Ready for
| checkin
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Mariusz Felisiak <felisiak.mariusz@…>):
In [changeset:"5289d4faec5ddc7592c719825914fe430003f724" 5289d4f]:
{{{
#!CommitTicketReference repository=""
revision="5289d4faec5ddc7592c719825914fe430003f724"
[2.2.x] Refs #30325 -- Added tests for using count()/exists() with custom
managers and reverse M2M relations.
Backport of 9ac8520fcde29840a1345be19d80dbda53aa6d03 from master
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/30325#comment:12>