Opened 7 years ago

Closed 7 years ago

#29076 closed New feature (fixed)

Make Model.refresh_from_db() clear cached foreign key and one to one fields, even if the related ID hasn't changed

Reported by: Jon Dufresne Owned by: nobody
Component: Database layer (models, ORM) Version: dev
Severity: Normal Keywords:
Cc: Anssi Kääriäinen, Carlton Gibson 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

Currently, Model.refresh_from_db() doesn't clear cached foreign key or one to one fields if the related ID has not changed. I find this behavior counter intuitive and almost always undesired.

Model.refresh_from_db() is frequently used in testing. For the model to be in an incompletely reloaded state can cause silent test passes or unexpected test failures.

In my experience, this has behavior has also been a frequent pain point for developers just learning the framework. Intuitively, most developers think refreshing a model will refresh all attributes.

True, this behavior is documented. But I think it can be improved to more often do the expected thing and help new Django developers.

A common use case:

A model instance with many fields relationships is altered by a view. A test reloads the model to verify the instance changed as expected, including the relationships.

Change History (5)

comment:1 by Jon Dufresne, 7 years ago

Has patch: set

comment:2 by Tim Graham, 7 years ago

Cc: Anssi Kääriäinen added
Summary: Model.refresh_from_db() should clear cached foreign key and one to one fieldsMake Model.refresh_from_db() clear cached foreign key and one to one fields, even if the related ID hasn't changed
Triage Stage: UnreviewedAccepted

I think this is fine. Did you check the original discussion for anything about this? I guess performance was the main concern in the initial implementation.

It seems like the change would also be consistent with the unconditional cached relation clearing added in #27846.

Last edited 7 years ago by Tim Graham (previous) (diff)

comment:3 by Jon Dufresne, 7 years ago

I read through the ticket, PR, and mailing list discussion. I didn't see anything that explicitly decided against this behavior, but please let me know if I missed it. I agree that it was probably done this way as an attempt at a performance optimization. That said, I think my proposed change will still lead to less confusion when using .refresh_from_db() as well as more consistent handling of different field types.

comment:4 by Carlton Gibson, 7 years ago

Cc: Carlton Gibson added
Triage Stage: AcceptedReady for checkin

The patch looks fine.

FWIW my view is that not being surprising trumps performance for refresh_from_db() — we already know we're going to take a hit calling it. It should refresh from DB.

comment:5 by Tim Graham <timograham@…>, 7 years ago

Resolution: fixed
Status: newclosed

In 136bf5c2:

Fixed #29076 -- Made Model.refresh_from_db() clear cached relationships even if the related id doesn't change.

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