[Django] #30355: Specifying custom manager doesn't work with prefetch
Django
-------------------------------------+-------------------------------------
Reporter: Kyle | Owner: nobody
Mulka |
Type: Bug | Status: new
Component: Database | Version: 1.11
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 using prefetch and specifying a custom manager to use for a reverse
relation, Django doesn't filter correctly. Here's an example:
{{{
class Business(models.Model):
name = models.CharField(max_length=255)
def approved_reviews(self):
return self.review_set(manager='approved_reviews').all()
class ApprovedReviewsManager(models.Manager):
def get_queryset(self):
return super().get_queryset().filter(status=Review.APPROVED)
class Review(models.Model):
NEW = 1
APPROVED = 2
STATUS_CHOICES = (
(NEW, 'New'),
(APPROVED, 'Approved'),
)
business = models.ForeignKey(Business)
text = models.CharField(max_length=255)
status = models.IntegerField(choices=STATUS_CHOICES, default=NEW)
objects = models.Manager()
approved_reviews = ApprovedReviewsManager()
class ApprovedReviewsTest(TestCase):
def test_with_prefetch(self):
business = Business()
business.save()
review = Review()
review.business = business
review.save()
businesses = Business.objects.prefetch_related('review_set').all()
business = businesses[0]
approved_reviews =
business.review_set(manager='approved_reviews').all()
self.assertEqual(len(approved_reviews), 0)
}}}
The full test project is available here:
https://github.com/mulka/django_prefetch_manager_bug/blob/master/review_site/tests.py
--
Ticket URL: <https://code.djangoproject.com/ticket/30355>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
-------------------------------------+-------------------------------------
Type: Bug | Status: new
Component: Database layer | Version: 1.11
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Daniel Hepper):
I've reproduced this behavior with the master branch, but I don't know if
this is actually a bug or just a case of a somewhat unintuitive API. This
should at least be documented in the section on using a custom reverse
manager: https://docs.djangoproject.com/en/2.2/topics/db/queries/#using-a
-custom-reverse-manager
A possible workaround would be to use a `django.db.models.Prefetch`
object:
{{{
class ApprovedReviewsTest(TestCase):
def test_with_prefetch(self):
business = Business()
business.save()
review = Review()
review.business = business
review.save()
prefetch_approved_reviews = Prefetch('review_set',
queryset=Review.approved_reviews.all())
businesses =
Business.objects.prefetch_related(prefetch_approved_reviews).all()
business = businesses[0]
approved_reviews =
business.review_set(manager='approved_reviews').all()
self.assertEqual(len(approved_reviews), 0)
}}}
In this example, you don't even have to specify the custom manager,
`business.review_set.all()` would give you the same result.
The underlying issue is that there is only a single prefetched object
cache per field, see
https://github.com/django/django/blob/de7f6b51b21747e19e90d9e3e04e0cdbf84e8a75/django/db/models/fields/related_descriptors.py#L607
--
Ticket URL: <https://code.djangoproject.com/ticket/30355#comment:1>
Django
-------------------------------------+-------------------------------------
Type: Bug | Status: new
Component: Database layer | Version: 1.11
(models, ORM) |
Severity: Normal | Resolution:
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* stage: Unreviewed => Accepted
Comment:
I was about to write a patch for the docs on "Using a custom reverse
manager" to highlight the implication of using prefetch with custom
managers.
But thinking about it further made me realize that the prefetch might have
happened in a completely unrelated part of the code. As a conclusion, I'd
consider this a bug.
--
Ticket URL: <https://code.djangoproject.com/ticket/30355#comment:2>
Django
-------------------------------------+-------------------------------------
Type: Bug | Status: new
Component: Database layer | Version: master
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* version: 1.11 => master
--
Ticket URL: <https://code.djangoproject.com/ticket/30355#comment:3>
Django
-------------------------------------+-------------------------------------
Type: Bug | Status: assigned
Component: Database layer | Version: master
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* owner: nobody => robinh00d
* status: new => assigned
--
Ticket URL: <https://code.djangoproject.com/ticket/30355#comment:4>
Django
-------------------------------------+-------------------------------------
Type: Bug | Status: new
Component: Database layer | Version: master
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* status: assigned => new
* owner: robinh00d => (none)
--
Ticket URL: <https://code.djangoproject.com/ticket/30355#comment:5>
Django
-------------------------------------+-------------------------------------
Type: Bug | Status: new
Component: Database layer | Version: master
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* cc: Kourt Bailey (added)
--
Ticket URL: <https://code.djangoproject.com/ticket/30355#comment:4>
Django
-------------------------------------+-------------------------------------
Type: Bug | Status: assigned
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* owner: nobody => robinh00d
* status: new => assigned
--
Ticket URL: <https://code.djangoproject.com/ticket/30355#comment:5>
Django
-------------------------------------+-------------------------------------
Type: Bug | Status: new
Component: Database layer | Version: master
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* owner: robinh00d => (none)
* status: assigned => new
--
Ticket URL: <https://code.djangoproject.com/ticket/30355#comment:6>
Django
Reporter: Kyle Mulka | Owner: (none)
Component: Documentation | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Simon Charette):
* needs_docs: 0 => 1
* component: Database layer (models, ORM) => Documentation
Comment:
I agree that it's more of a documentation issue as `prefetch_related` will
use the default manager just like accessing a related manager does.
In other words `Business.objects.prefetch_related('review_set')` will use
the default manager just like `business.review_set.all()` does.
--
Ticket URL: <https://code.djangoproject.com/ticket/30355#comment:7>
Django
Reporter: Kyle Mulka | Owner: (none)
Component: Documentation | Version: dev
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 1
Easy pickings: 0 | UI/UX: 0
Comment (by saeldion):
I'd also weigh in on the bug side. Since the prefetch can happen elsewhere
its easy to miss. It feels weird that specifying a manager would have no
effect at all if it's already prefetched. Maybe the prefetch cache could
be ignored/invalidated if you pass an explicit manager?
--
Ticket URL: <https://code.djangoproject.com/ticket/30355#comment:8>
Django
Reporter: Kyle Mulka | Owner: Ankur Roy
Type: Bug | Status: assigned
Component: Documentation | Version: dev
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 1
Easy pickings: 0 | UI/UX: 0
Changes (by Ankur Roy):
* owner: (none) => Ankur Roy
* status: new => assigned
Comment:
I will review the documentation and add the text as Simon Charette
described if it hasn't been added already.
--
Ticket URL: <https://code.djangoproject.com/ticket/30355#comment:9>
Django
Reporter: Kyle Mulka | Owner: (none)
Component: Documentation | Version: dev
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 1
Easy pickings: 0 | UI/UX: 0
Changes (by Ankur Roy):
* owner: Ankur Roy => (none)
* status: assigned => new
--
Ticket URL: <https://code.djangoproject.com/ticket/30355#comment:10>
Django
Reporter: Kyle Mulka | Owner: Akshat verma
Type: Bug | Status: assigned
Component: Documentation | Version: dev
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 1
Easy pickings: 0 | UI/UX: 0
Changes (by Akshat verma):
* owner: (none) => Akshat verma
* status: new => assigned
Old description:
New description:
self.assertEqual(len(approved_reviews), 0)
}}}
--
--
Ticket URL: <https://code.djangoproject.com/ticket/30355#comment:9>
Django
Reporter: Kyle Mulka | Owner: Akshat verma
Type: Bug | Status: assigned
Component: Documentation | Version: dev
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 1 | Patch needs improvement: 1
Easy pickings: 1 | UI/UX: 1
-------------------------------+----------------------------------------
Changes (by Akshat verma):
* cc: Akshat verma (added)
* needs_better_patch: 0 => 1
* needs_tests: 0 => 1
* easy: 0 => 1
* has_patch: 0 => 1
* ui_ux: 0 => 1
Old description:
New description:
self.assertEqual(len(approved_reviews), 0)
}}}
#This implementation: by Akshat verma
class ToppingManager(models.Manager):
def filter_vegetarian(self):
return self.filter(is_vegetarian=True)
#Looks non-standard. docs look like they do a safer method of modifying
the super-class method for this sort of lazy-eval stuff. #If I rewrite
your method in that style, it would look like:
class ToppingManager(models.Manager):
def filter_vegetarian(self):
return super(ToppingManager,
self).get_queryset().filter(is_vegetarian=True)
#You wouldn't strictly need the super() here, but safer to use it because
you should know that you want to start with the #models.Manager
get_queryset method.
--
Comment:
#This implementation: by Akshat verma
class ToppingManager(models.Manager):
def filter_vegetarian(self):
return self.filter(is_vegetarian=True)
#Looks non-standard. docs look like they do a safer method of modifying
the super-class method for this sort of lazy-eval stuff. #If I rewrite
your method in that style, it would look like:
class ToppingManager(models.Manager):
def filter_vegetarian(self):
return super(ToppingManager,
self).get_queryset().filter(is_vegetarian=True)
#You wouldn't strictly need the super() here, but safer to use it because
you should know that you want to start with the #models.Manager
get_queryset method.
--
Ticket URL: <https://code.djangoproject.com/ticket/30355#comment:10>
Django
Reporter: Kyle Mulka | Owner: Akshat verma
Type: Bug | Status: assigned
Component: Documentation | Version: dev
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Akshat verma):
* owner: (none) => Akshat verma
* status: new => assigned
--
Ticket URL: <https://code.djangoproject.com/ticket/30355#comment:9>
Django
Reporter: Kyle Mulka | Owner: Akash Kumar Sen
Type: Bug | Status: assigned
Component: Documentation | Version: dev
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Akash Kumar Sen):
* cc: Akash Kumar Sen (added)
* owner: (none) => Akash Kumar Sen
* has_patch: 0 => 1
* status: new => assigned
Comment:
For me it also seems a documentation issue, as the same results can be
achieved by obtaining other means. Created a documentation patch
https://github.com/django/django/pull/16900
--
Ticket URL: <https://code.djangoproject.com/ticket/30355#comment:9>
Django
Reporter: Kyle Mulka | Owner: Akash Kumar Sen
Type: Bug | Status: assigned
Component: Documentation | Version: dev
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
Changes (by Mariusz Felisiak):
* needs_better_patch: 0 => 1
* needs_docs: 1 => 0
--
Ticket URL: <https://code.djangoproject.com/ticket/30355#comment:10>
Django
Reporter: Kyle Mulka | Owner: Akash Kumar Sen
Type: Bug | Status: assigned
Component: Documentation | Version: dev
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
Comment (by Akash Kumar Sen):
The possible explanation here is that {{{queryset}}} generated with
{{{prefetch}}} does not modify the queryset or make a DB query when we are
fetching the reviews, this is the sole purpose of prefetch, so if we try
to use a {{{custom manager}}} with {{{prefetch}}} that will violate the
purpose of using prefetch, hitting 1 extra query every time we try to
fetch the reviews.
I will update the docs with this explanation for better understanding
along with the alternative way using from {{{django.db.models.Prefetch}}}.
Attaching the testcase for better understanding: https://github.com/Akash-
Kumar-Sen/django/blob/ticket_30355_3/tests/dummy_tests/tests.py.
Some feedbacks willbe helpful as I still have a little confusion.
--
Ticket URL: <https://code.djangoproject.com/ticket/30355#comment:11>
Django
Reporter: Kyle Mulka | Owner: Akash Kumar Sen
Type: Bug | Status: assigned
Component: Documentation | Version: dev
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Akash Kumar Sen):
* needs_better_patch: 1 => 0
--
Ticket URL: <https://code.djangoproject.com/ticket/30355#comment:12>
Django
Reporter: Kyle Mulka | Owner: Akash Kumar Sen
Type: Bug | Status: assigned
Component: Documentation | Version: dev
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
Changes (by Mariusz Felisiak):
* needs_better_patch: 0 => 1
--
Ticket URL: <https://code.djangoproject.com/ticket/30355#comment:13>
Django
Reporter: Kyle Mulka | Owner: Akash Kumar Sen
Type: Bug | Status: assigned
Component: Documentation | Version: dev
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Akash Kumar Sen):
* needs_better_patch: 1 => 0
--
Ticket URL: <https://code.djangoproject.com/ticket/30355#comment:14>
Django
-------------------------------------+-------------------------------------
Type: | Kumar Sen
Cleanup/optimization | Status: assigned
Component: Documentation | Version: dev
Severity: Normal | Resolution:
| checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* type: Bug => Cleanup/optimization
* stage: Accepted => Ready for checkin
--
Ticket URL: <https://code.djangoproject.com/ticket/30355#comment:15>
Django
-------------------------------------+-------------------------------------
Type: | Kumar Sen
Component: Documentation | Version: dev
Severity: Normal | Resolution: fixed
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:"eb84c068ed7c2982389b8d331418e99dd9709ff7" eb84c06]:
{{{
#!CommitTicketReference repository=""
revision="eb84c068ed7c2982389b8d331418e99dd9709ff7"
[4.2.x] Fixed #30355 -- Doc'd interaction between custom managers and
prefetch_related().
Backport of 5f2308710b5a3d9f5f135b7ade08214f5c154ec4 from main
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/30355#comment:16>
Django
-------------------------------------+-------------------------------------
Type: | Kumar Sen
Cleanup/optimization | Status: closed
Component: Documentation | Version: dev
Severity: Normal | 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:"5f2308710b5a3d9f5f135b7ade08214f5c154ec4" 5f230871]:
{{{
#!CommitTicketReference repository=""
revision="5f2308710b5a3d9f5f135b7ade08214f5c154ec4"
Fixed #30355 -- Doc'd interaction between custom managers and
prefetch_related().
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/30355#comment:17>