-
-
Notifications
You must be signed in to change notification settings - Fork 6.9k
Invalidate any existing prefetch cache on PUT requests. #4668
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
The only drawback I can see to this approach is, if I remember well, in the absence of a prefetch cache, every use of the related objects from that point forward will trigger a query. (but double-check that, I did not follow the latest Django updates very closely). I was wondering whether this could be a possible solution too, digging just a single bit deeper into the queryset internals: cache = getattr(instance, '_prefetched_objects_cache', None)
if cache:
for key, queryset in cache.items():
cache[key] = queryset.all() Or, invalidating the cache in place instead: cache = getattr(instance, '_prefetched_objects_cache', None)
if cache:
for queryset in cache.values():
queryset._result_cache = None The idea is using related objects will still use the prefetch cache. First use will re-load data from the database, subsequent loads will not. (I for one would love to see a class-level flag allowing to disable that code section entirely, but if I am too isolated a user, I will just use my own altered |
Thanks for all that! Given that we use the instance once-only after removing the cache it's not clear that'd benefit us, tho would be perfectly happy to consider any pull requests that came along with assertNumQueries to demonstrate any improvements that we could still make here. |
I don't either, let's just say that if someone does he'll make his own mixin too. Well, thanks for taking the time to come up with an improved solution. |
I'm not sure this patch is the most diplomatic approach... People spend a lot of time tweaking The primary differences are:
def update(self, instance, validated_data):
"""
Surgically removes potentially stale, prefetched, cached, related objects
in order to ensure non-stale related data in the serialized object after updating it.
"""
# There are several reports of issues around this, e.g.:
# https://github.com/tomchristie/django-rest-framework/pull/1754
# https://github.com/tomchristie/django-rest-framework/pull/4668
if hasattr(instance, "_prefetched_objects_cache"):
# noinspection PyProtectedMember
prefetch_objects_cache = instance._prefetched_objects_cache
# It's common practice to support nested writes by popping it from `validated_data`
# before going through the standard `update` from DRF
initial_data_keys = self.initial_data.keys() if self.initial_data else []
validated_data_keys = validated_data.keys()
all_possible = set(initial_data_keys + validated_data_keys)
prefetch_keys_to_clear = set(prefetch_objects_cache.keys()).intersection(all_possible)
for key in prefetch_keys_to_clear:
del prefetch_objects_cache[key]
# This would essentially go on to DRF's `update`
return super(BaseModelSerializer, self).update(instance, validated_data) Summing Up...
I see there's already been a ton of discussion around this - sorry I'm late to the party. Thanks for the wonderful framework - I hope that helps |
@fredpalmer > current release re-invokes For now, the best workaround is to have your own At least with this patch here, the impact will be much more limited that with current release and it will avoid some pitfalls. |
@tomchristie Is the behaviour here settled? (It's currently unreleased right?) Lots of people run into this when first using But equally, we're all adults, and I keep coming back to the upstream decision not to handle this, i.e. to leave it to application developers.
This seems (to me) an overly heavy burden to put on everybody just in order to add what are essentially training wheels for using If we do want this, do we not also want — although "want" isn't quite what I mean to say — a setting and/or an |
Yes, it's settled. (Tho the improvement is currently unreleased, yes.) We've got two options:
There's no question which of those two we should do. It may be that there's still further improvements we could make, and I'm very happy to consider productive discussion there, but that's a different issue. |
Invalidate the prefetch cache instead of reloading the object from the database when a prefetch_related queryset is used with a PUT request.
Would be nice to also have a test case demonstrating why this approach has a different behavioral effect.
Closes #4661.
Refs #4553.