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 pascal chambon)

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 pascal chambon, 2 years ago

Description: modified (diff)

comment:2 by Mariusz Felisiak, 2 years ago

Cc: Simon Charette added
Component: Uncategorizedcontrib.contenttypes
Summary: model.refresh_from_db() doesn't seem to clear cached generic foreign keysmodel.refresh_from_db() doesn't clear cached generic foreign keys
Triage Stage: UnreviewedAccepted
Type: UncategorizedBug

Thanks for the report.

comment:3 by Simon Charette, 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):  
    737737            if field.is_cached(self):
    738738                field.delete_cached_value(self)
    739739
     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
    740745        self._state.db = db_instance._state.db
    741746
    742747    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?

comment:4 by Bhuvnesh, 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 :)

in reply to:  4 comment:5 by pascal chambon, 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 Bhuvnesh, 2 years ago

Owner: changed from nobody to Bhuvnesh
Status: newassigned

comment:7 by Bhuvnesh, 2 years ago

Has patch: set

comment:8 by Simon Charette, 2 years ago

Triage Stage: AcceptedReady for checkin

Patch is looking great, thanks for the tests Bhuvnesh.

Pascal, did you confirm the proposed patch solves your issue?

comment:9 by pascal chambon, 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 !

Last edited 2 years ago by pascal chambon (previous) (diff)

comment:10 by GitHub <noreply@…>, 2 years ago

Resolution: fixed
Status: assignedclosed

In 123b1d3:

Fixed #34137 -- Made Model.refresh_from_db() clear cached generic relations.

Thanks Simon Charette for the implementation idea.

Note: See TracTickets for help on using tickets.
Back to Top