#30355 closed Cleanup/optimization (fixed)
Specifying custom manager doesn't work with prefetch
Reported by: | Kyle Mulka | Owned by: | Akash Kumar Sen |
---|---|---|---|
Component: | Documentation | Version: | dev |
Severity: | Normal | Keywords: | |
Cc: | Kourt Bailey, Akash Kumar Sen | Triage Stage: | Ready for checkin |
Has patch: | yes | Needs documentation: | no |
Needs tests: | no | Patch needs improvement: | no |
Easy pickings: | no | UI/UX: | no |
Description
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
Change History (17)
comment:2 by , 6 years ago
Triage Stage: | Unreviewed → Accepted |
---|
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.
comment:3 by , 6 years ago
Version: | 1.11 → master |
---|
comment:4 by , 6 years ago
Cc: | added |
---|
comment:5 by , 6 years ago
Owner: | changed from | to
---|---|
Status: | new → assigned |
comment:6 by , 6 years ago
Owner: | removed |
---|---|
Status: | assigned → new |
comment:7 by , 6 years ago
Component: | Database layer (models, ORM) → Documentation |
---|---|
Needs documentation: | set |
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.
comment:8 by , 4 years ago
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?
comment:9 by , 19 months ago
Cc: | added |
---|---|
Has patch: | set |
Owner: | set to |
Status: | new → assigned |
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
comment:10 by , 19 months ago
Needs documentation: | unset |
---|---|
Patch needs improvement: | set |
comment:11 by , 19 months ago
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.
comment:12 by , 19 months ago
Patch needs improvement: | unset |
---|
comment:13 by , 19 months ago
Patch needs improvement: | set |
---|
comment:14 by , 19 months ago
Patch needs improvement: | unset |
---|
comment:15 by , 19 months ago
Triage Stage: | Accepted → Ready for checkin |
---|---|
Type: | Bug → Cleanup/optimization |
I've reproduced this behavior with the master branch (the original report was for 1.11), 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: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