Opened 2 years ago
Closed 2 years ago
#34137 closed Bug (fixed)
model.refresh_from_db() doesn't clear cached generic foreign keys
Reported by: | pascal chambon | Owned by: | Bhuvnesh |
---|---|---|---|
Component: | contrib.contenttypes | Version: | 3.2 |
Severity: | Normal | Keywords: | |
Cc: | Simon Charette | Triage Stage: | Ready for checkin |
Has patch: | yes | Needs documentation: | no |
Needs tests: | no | Patch needs improvement: | no |
Easy pickings: | no | UI/UX: | no |
Description (last modified by )
In my code, Users have a generic foreign key like so:
controlled_entity_content_type = models.ForeignKey(ContentType, blank=True, null=True, on_delete=models.CASCADE) controlled_entity_object_id = models.PositiveIntegerField(blank=True, null=True) controlled_entity = GenericForeignKey("controlled_entity_content_type", "controlled_entity_object_id")
However, in unit-tests, when I refresh a user instance, the controlled_entity relation isn't cleared from cache, as can be seen here with IDs:
old_controlled_entity = authenticated_user.controlled_entity authenticated_user.refresh_from_db() new_controlled_entity = authenticated_user.controlled_entity assert id(old_controlled_entity) != id(new_controlled_entity) # FAILS
And this leads to subtle bugs like non-transitive equalities in tests :
assert authenticated_user.controlled_entity == fixtures.client1_project2_organization3 assert fixtures.client1_project2_organization3.get_pricing_plan() == pricing_plan assert authenticated_user.controlled_entity.get_pricing_plan() == pricing_plan # FAILS
Calling "authenticated_user.controlled_entity.refresh_from_db()" solved this particular bug, but "authenticated_user.refresh_from_db()" isn't enough.
Tested under Django3.2.13 but the code of refresh_from_db() hasn't changed since then in Git's main branch (except few cosmetic adjustments on code format).
I'm a bit lost in the code of refresh_from_db(), but I've just seen that the generic relation appears once in this loop, but is not considered as "cached" in the if() branch.
for field in self._meta.related_objects: #print("%% CLEARING RELATED FIELD", field) if field.is_cached(self): #print("%% DONE") # not called field.delete_cached_value(self)
Change History (10)
comment:1 by , 2 years ago
Description: | modified (diff) |
---|
comment:2 by , 2 years ago
Cc: | added |
---|---|
Component: | Uncategorized → contrib.contenttypes |
Summary: | model.refresh_from_db() doesn't seem to clear cached generic foreign keys → model.refresh_from_db() doesn't clear cached generic foreign keys |
Triage Stage: | Unreviewed → Accepted |
Type: | Uncategorized → Bug |
comment:3 by , 2 years ago
Bonjour Pascal,
It seems to be an oversight in Model.refresh_from_db
as it should also consider _meta.private_fields
which is where GenericForeignKey
and GenericRel
end up as opposed to related_objects
. Something along these lines should address the issue
-
django/db/models/base.py
diff --git a/django/db/models/base.py b/django/db/models/base.py index 2eb7ba7e9b..0f5f8d0881 100644
a b def refresh_from_db(self, using=None, fields=None): 737 737 if field.is_cached(self): 738 738 field.delete_cached_value(self) 739 739 740 # Clear cached private relations. 741 for field in self._meta.private_fields: 742 if field.is_relation and field.is_cached(self): 743 field.delete_cached_value(self) 744 740 745 self._state.db = db_instance._state.db 741 746 742 747 async def arefresh_from_db(self, using=None, fields=None):
Would you be interested in submitting a patch whit these changes and adding a regression test to the suite?
follow-up: 5 comment:4 by , 2 years ago
Hi,
I was just working around to write a regression test for the issue, if not pascal I would like to submit a patch with a test and changes proposed by simon. Thanks :)
comment:5 by , 2 years ago
Please proceed yes
I'll be happy to backport/test the PR against my own project to validate it in real conditions
comment:6 by , 2 years ago
Owner: | changed from | to
---|---|
Status: | new → assigned |
comment:8 by , 2 years ago
Triage Stage: | Accepted → Ready for checkin |
---|
Patch is looking great, thanks for the tests Bhuvnesh.
Pascal, did you confirm the proposed patch solves your issue?
comment:9 by , 2 years ago
Juste tested the patch against my codebase, now the code works without forcing a "authenticated_user.controlled_entity.refresh_from_db()" :)
So looks quite good to me ! Thanks for the swift patch !
Thanks for the report.