Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork7k
Re-prefetch related objects after updating#8043
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
dr-ftvkun commentedJun 3, 2022 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
TLDR; this PR seems to be a good compromise between re-invoking So, about the bug. Let we have a viewset with the following get_queryset(): defget_queryset(self):returnEntity.objects.prefetch_related(Prefetch("related_objects",queryset=RelatedObject.objects.filter(is_removed=False).order_by("order") ) ) Expected:
Actual:
So in general I think just re-prefetching them would do the right job. |
lovelydinosaur commentedJun 6, 2022
Okay. I can't say that I know those bits of private implementation, but I'm okay with this. Marking it as a priority, so I can verify to myself that the change does fix the behaviour as described above. Once that's confirmed I think we should be okay to merge this. |
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
fjsj commentedAug 26, 2022
As this is buggy in current main/master branch, as explained by@dr-ftvkun above, I also think this should be merged. |
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
yuekui commentedNov 23, 2022
Can we re-open it? |
dr-ftvkun commentedNov 23, 2022
@tomchristie could you please re-open it? Hoping one day someone (mb me?) will have enough energy to push it to become a part of DRF or be rejected by a good reason (not just because it got stale). |
auvipy commentedNov 23, 2022
Hey good people, I'm a new maintainer here. you can ping me anytime for reviewing and insights for direction. if we are unsure about anything we then ping him for final call. I have already reviewed and merged some old pr. so in coming weeks this project will have more active maintainers available. |
dr-ftvkun commentedNov 23, 2022
Hey@auvipy! Great news! I think this PR at least needs tests for the cases i providedabove. But still, could you also look at it to check the general idea? |
auvipy commentedNov 23, 2022
I will review this thoroughly |
yuekui commentedNov 24, 2022
Thanks@dr-ftvkun , I'll add tests for that. |
auvipy left a comment
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.
I like the general idea, but I want to validate myself more looking into the django pvt stuffs. that may be first of next week
yuekui commentedDec 2, 2022
Hi@auvipy please let me know if there's any improvement we could do for this PR. |
auvipy commentedDec 5, 2022
we need to verify the usage of django internals here. so need little bit more time then expected. please allow us some time to get back to you for this. |
auvipy commentedMar 20, 2024
we got a regression report, also a possible fix, can you check?#9314 |
yuekui commentedMar 20, 2024
Yeah that should fix the case that if only get_object() was defined, it looks good to me. |
Instead of re-loading
get_object()or not using any prefetch after updating, we could keep the instance and re-prefetch related objects for the updated instance.refs:
#4553
#4668
#4661