-
-
Notifications
You must be signed in to change notification settings - Fork 33.1k
Fixed #34791 -- Fixed incorrect Prefetch()'s cache for singly related objects. #17207
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hello! Thank you for your contribution 💪
As it's your first contribution be sure to check out the patch review checklist.
If you're fixing a ticket from Trac make sure to set the "Has patch" flag and include a link to this PR in the ticket!
If you have any design or process questions then you can ask in the Django forum.
Welcome aboard ⛵️!
8fc8b19
to
ab26977
Compare
ab26977
to
6681b9b
Compare
@charettes There seems to be a test failing, but I don't think it has anything to do with the changes I made? Could it be a flaky (I don't know if you have many of those on django)? |
@MaxDude132 I confirm that it is flaky and not related to your changes. |
@charettes I believe it is ready to merge |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking great Maxime.
Left a few comments for tweaks it should eventually be reviewed. It might take a few days as we're currently approaching the feature freeze for 5.0 and fellows are busy merging changes before the window closes.
6681b9b
to
698fdd6
Compare
@charettes All changes were addressed. Thanks a lot for your help on this, it is much appreciated as this was my first real deep-dive into the Django code |
@MaxDude132 glad I could help, you did a good job at identifying the root cause and providing a patch. |
… objects. Changed the cache name used for singly related objects to be the to_attr parameter passed to a Prefetch object. This fixes issues with checking if values have already been fetched in cases where the Field already has some prefetched value, but not for the same model attr.
@MaxDude132 Thanks 👍 Welcome aboard ⛵ |
698fdd6
to
254df3a
Compare
Changed the cache name used for singly related objects to be the to_attr parameter passed to a Prefetch object. This fixes issues with checking if values have already been fetched in cases where the Field already has some prefetched value, but not for the same model attr.