Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork7k
Fix UpdateModelMixin to work when no queryset is defined is defined on the view#9314
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
Fix UpdateModelMixin to work when no queryset is defined is defined on the view#9314
Uh oh!
There was an error while loading.Please reload this page.
Conversation
| if queryset._prefetch_related_lookups: | ||
| if hasattr(instance, '_prefetched_objects_cache'): |
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.
When onlyget_object is defined, I assume that folks are unlikely to have advanced prefetches in their method (although it's very much possible).
If some things were prefetched, we assume that there is aget_queryset method.
80e09d0 to89c79d6CompareThere 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.
Yeah this should fix the case that if only get_object() was defined, looks good to me
| instance._prefetched_objects_cache = {} | ||
| prefetch_related_objects([instance], *queryset._prefetch_related_lookups) | ||
| queryset = self.filter_queryset(self.get_queryset()) |
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.
This is still expectingget_queryset orqueryset to be defined and will raise an AssertError if it is not, which means it will still not be backwards compatabile with 3.14 correct?
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.
How is it possible thatself has '_prefetched_objects_cache' defined without definingqueryset orget_queryset?
browniebrokeMar 21, 2024 • 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.
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.
That's what I meant by my comment earlier, one might do:
classUserRetrieveWithoutQuerySet(generics.RetrieveUpdateAPIView):serializer_class=UserSerializerdefget_object(self):returnUser.objects.prefetch_related('groups').get(pk=self.kwargs['pk'])
In such case, I would argue that it's reasonable to require users to override theget_queryset method... The error message says exactly that. Would probably need to be documented better, though.
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.
Probably just an extraif statement ortry catch would solve all these cases.
auvipy commentedMar 21, 2024 • 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.
should we revert the actual implementation as breaking change? and then come with a better fix? or in this case we can just go with this?#9327 |
Going to close this as it becoming clear that we should revert. I was only trying to help get a stable 3.15.x out, which a revert achieves... |
Description
It seems that the fix from#8043 isn't working in all cases, especially when the users aren't setting the
querysetattribute, or overriding theget_queryset()method, and instead opt to override theget_objectmethod.Attempt to fix this edge case by delaying the call of
get_queryset, to be only if the instance that was just updated had some prefetched relations.Fix#9306