Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 1 commit into from Jun 7, 2023

Conversation

Akash-Kumar-Sen
Copy link
Contributor

@Akash-Kumar-Sen Akash-Kumar-Sen commented May 26, 2023

@Akash-Kumar-Sen Akash-Kumar-Sen changed the title Fixed : #30355 Specifying custom manager doesn't work with prefetch in documentation Fixed : #30355 Specified custom manager doesn't work with prefetch in documentation May 29, 2023
@nessita nessita self-requested a review May 30, 2023 18:16
@nessita
Copy link
Contributor

nessita commented May 30, 2023

Thank you @Akash-Kumar-Sen for this work. I have squashed commits, tweaked the text a little bit and changed the display to be an admonition:

image

I'll leave this PR here for a few days to see if other reviewers prefer other wording, otherwise I'll land before the week ends.

@nessita nessita changed the title Fixed : #30355 Specified custom manager doesn't work with prefetch in documentation Fixed #30355 -- Doc'd interaction between custom managers and prefetch_related. May 30, 2023
@smithdc1
Copy link
Member

smithdc1 commented Jun 1, 2023

Thank you @Akash-Kumar-Sen for picking this up and pushing it forward. 👍

I'm slightly nervous this sounds like we're just documenting a bug / limitation of Django, but I'm fairly sure this isn't the case. I'm left with more questions than answers.

Could we add a little more to explain either why this is the correct behaviour or what the other means are to work around this are? (comments on ticket indicate there are, but it's not clear to me what that could be).

Is it possible to write something like?

Using prefetch_related uses the default manager because ... therefore use ... instead.

@felixxm
Copy link
Member

felixxm commented Jun 2, 2023

I'm slightly nervous this sounds like we're just documenting a bug / limitation of Django, but I'm fairly sure this isn't the case. I'm left with more questions than answers.

I have the same feelings. The current behavior is what it is. I wouldn't document it as unexpected, maybe:

``QuerySet.prefetch_related()`` on a reverse relation always uses the default manager not a custom one.  ...

@Akash-Kumar-Sen
Copy link
Contributor Author

@smithdc1 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 import Prefetch.

Attaching the testcase for better understanding: https://github.com/Akash-Kumar-Sen/django/blob/ticket_30355_3/tests/dummy_tests/tests.py

docs/topics/db/queries.txt Outdated Show resolved Hide resolved
@felixxm felixxm changed the title Fixed #30355 -- Doc'd interaction between custom managers and prefetch_related. Fixed #30355 -- Doc'd interaction between custom managers and prefetch_related(). Jun 7, 2023
@felixxm
Copy link
Member

felixxm commented Jun 7, 2023

@Akash-Kumar-Sen Thanks 👍 I pushed small formatting edits.

@felixxm felixxm merged commit 5f23087 into django:main Jun 7, 2023
21 checks passed
@felixxm felixxm temporarily deployed to schedules June 8, 2023 02:57 — with GitHub Actions Inactive
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants