Opened 5 years ago

Closed 11 months ago

Last modified 11 months ago

#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:1 by Daniel Hepper, 5 years ago

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:

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

Last edited 5 years ago by Daniel Hepper (previous) (diff)

comment:2 by Daniel Hepper, 5 years ago

Triage Stage: UnreviewedAccepted

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 Daniel Hepper, 5 years ago

Version: 1.11master

comment:4 by Kourt Bailey, 5 years ago

Cc: Kourt Bailey added

comment:5 by robinh00d, 5 years ago

Owner: changed from nobody to robinh00d
Status: newassigned

comment:6 by robinh00d, 5 years ago

Owner: robinh00d removed
Status: assignednew

comment:7 by Simon Charette, 5 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 saeldion, 3 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 Akash Kumar Sen, 11 months ago

Cc: Akash Kumar Sen added
Has patch: set
Owner: set to Akash Kumar Sen
Status: newassigned

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 Mariusz Felisiak, 11 months ago

Needs documentation: unset
Patch needs improvement: set

comment:11 by Akash Kumar Sen, 11 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 Akash Kumar Sen, 11 months ago

Patch needs improvement: unset

comment:13 by Mariusz Felisiak, 11 months ago

Patch needs improvement: set

comment:14 by Akash Kumar Sen, 11 months ago

Patch needs improvement: unset

comment:15 by Mariusz Felisiak, 11 months ago

Triage Stage: AcceptedReady for checkin
Type: BugCleanup/optimization

comment:16 by Mariusz Felisiak <felisiak.mariusz@…>, 11 months ago

Resolution: fixed
Status: assignedclosed

In eb84c06:

[4.2.x] Fixed #30355 -- Doc'd interaction between custom managers and prefetch_related().

Backport of 5f2308710b5a3d9f5f135b7ade08214f5c154ec4 from main

comment:17 by Mariusz Felisiak <felisiak.mariusz@…>, 11 months ago

In 5f230871:

Fixed #30355 -- Doc'd interaction between custom managers and prefetch_related().

Note: See TracTickets for help on using tickets.
Back to Top