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

Fix UpdateModelMixin to work when no queryset is defined is defined on the view #9314

Conversation

browniebroke
Copy link
Member

Description

It seems that the fix from #8043 isn't working in all cases, especially when the users aren't setting the queryset attribute, or overriding the get_queryset() method, and instead opt to override the get_object method.

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

Comment on lines -73 to +72
if queryset._prefetch_related_lookups:
if hasattr(instance, '_prefetched_objects_cache'):
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When only get_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 a get_queryset method.

@browniebroke browniebroke force-pushed the fix-updatemixin-queryset-error branch from 80e09d0 to 89c79d6 Compare March 19, 2024 21:57
@auvipy auvipy self-requested a review March 20, 2024 08:11
Copy link
Contributor

@yuekui yuekui left a 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())
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is still expecting get_queryset or queryset 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?

Copy link
Contributor

@yuekui yuekui Mar 21, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How is it possible that self has '_prefetched_objects_cache' defined without defining queryset or get_queryset?

Copy link
Member Author

@browniebroke browniebroke Mar 21, 2024

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:

class UserRetrieveWithoutQuerySet(generics.RetrieveUpdateAPIView):
    serializer_class = UserSerializer

    def get_object(self):
        return User.objects.prefetch_related('groups').get(pk=self.kwargs['pk'])

In such case, I would argue that it's reasonable to require users to override the get_queryset method... The error message says exactly that. Would probably need to be documented better, though.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably just an extra if statement or try catch would solve all these cases.

@auvipy
Copy link
Member

auvipy commented Mar 21, 2024

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

@browniebroke
Copy link
Member Author

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...

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