Opened 6 years ago
Closed 6 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 , 6 years ago
| Has patch: | set |
|---|
comment:2 by , 6 years ago
| Cc: | added |
|---|---|
| Summary: | Model.refresh_from_db() should clear cached foreign key and one to one fields → Make Model.refresh_from_db() clear cached foreign key and one to one fields, even if the related ID hasn't changed |
| Triage Stage: | Unreviewed → Accepted |
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.
comment:3 by , 6 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 , 6 years ago
| Cc: | added |
|---|---|
| Triage Stage: | Accepted → Ready 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.
PR