Opened 6 years ago
Closed 6 years ago
#29625 closed Cleanup/optimization (fixed)
Make Model.refresh_from_db() clear prefetch related caches
Reported by: | Ming Qin | Owned by: | Ming Qin |
---|---|---|---|
Component: | Database layer (models, ORM) | Version: | dev |
Severity: | Normal | Keywords: | prefetch_related, _prefetched_objects_cache |
Cc: | Triage Stage: | Accepted | |
Has patch: | yes | Needs documentation: | no |
Needs tests: | no | Patch needs improvement: | no |
Easy pickings: | no | UI/UX: | no |
Description
The method currently starts with underscore, meaning it's not supposed to be accessed from the outside. But I find that sometimes I have to use this method to clear the cache, otherwise the outdated data will be returned. The following test shows a scenario where this method is useful.
def setUpTestData(cls): cls.book1 = Book.objects.create(title='Les confessions Volume I') cls.book2 = Book.objects.create(title='Candide') cls.author1 = AuthorWithAge.objects.create(name='Rousseau', first_book=cls.book1, age=70) cls.author2 = AuthorWithAge.objects.create(name='Voltaire', first_book=cls.book2, age=65) cls.book1.authors.add(cls.author1) cls.book2.authors.add(cls.author2) FavoriteAuthors.objects.create(author=cls.author1, likes_author=cls.author2) def test_remove_prefetched_objects(self): """ Test RelatedManager._remove_prefetched_objects method """ books = Book.objects.prefetch_related('first_time_authors') book1 = books.get(title='Les confessions Volume I') book2 = books.get(title='Candide') # Update the related object - author1. So book1 has no first-time author now, but still has one in cache author1 = book1.authors.first() author1.first_book = book2 author1.save() self.assertQuerysetEqual( book1.first_time_authors.all(), ['<Author: Rousseau>'], msg="Expect the outdated cached data") # Now clear the prefetched objects book1.first_time_authors._remove_prefetched_objects() self.assertQuerysetEqual( book1.first_time_authors.all(), [], msg="Expect the fresh data from database")
I'm working on opening a PR to add the above test case, and create an alias of that method with no underscore. Let me know what you think.
Change History (12)
comment:2 by , 6 years ago
Triage Stage: | Unreviewed → Accepted |
---|
I don't really like an additional method either but refresh_from_db()
only reloads model fields -- it doesn't clear related manager caches, so I guess this might be okay. Prefetch related cache clearing was added in #26706.
comment:3 by , 6 years ago
Yeah as Tim said, refresh_from_db()
doesn't help in this case. I did an experiment just to confirm:
>>> book1.refresh_from_db() >>> book1.refresh_from_db(fields=["authors"]) >>> list(book1.authors.all()) # still hits the prefetched cache
follow-up: 5 comment:4 by , 6 years ago
Could you confirm refresh_from_db(fields=['authors'])
works on Django 2.1? If not this might be an omission in #29076.
follow-up: 6 comment:5 by , 6 years ago
Replying to Simon Charette:
Could you confirm
refresh_from_db(fields=['authors'])
works on Django 2.1? If not this might be an omission in #29076.
No, it doesn't work. Still hits the cache. Tested on master
, 2.1rc1
, and 2.1
.
I'll be happy to fix refresh_from_db
, instead of opening this new API.
Does the following behavior sound good?
- If
refresh_from_db()
is called without argument, it won't clear prefetched cache. - If it's called specifically with an related field, and that field is many-to-many or many-to-one field, the prefetched cache will be cleared.
comment:6 by , 6 years ago
Replying to Ming Qin:
No, it doesn't work. Still hits the cache. Tested on
master
,2.1rc1
, and2.1
.
I'll be happy to fixrefresh_from_db
, instead of opening this new API.
Does the following behavior sound good?
- If
refresh_from_db()
is called without argument, it won't clear prefetched cache.- If it's called specifically with an related field, and that field is many-to-many or many-to-one field, the prefetched cache will be cleared.
- No, it should clear all fields prefetched cache like it does right now with non-prefetched related manager cache.
- I haven't investigated but it looks like
ManyToManyField.is_cached
andManyToMany.delete_cached_value
might need to be redefined fromField
in order to takeinstance._prefetched_objects_cache
into consideration as well. It'd be interesting to test how prefetched foreign keys and o2o behave here as well.
comment:7 by , 6 years ago
Easy pickings: | unset |
---|---|
Has patch: | unset |
Summary: | Expose RelatedManager._remove_prefetched_objects as public method → Make Model.refresh_from_db() clear prefetch related caches |
comment:8 by , 6 years ago
Owner: | changed from | to
---|---|
Status: | new → assigned |
comment:10 by , 6 years ago
Patch needs improvement: | set |
---|
comment:11 by , 6 years ago
Patch needs improvement: | unset |
---|
Not sure about the value of adding this. We already have `refresh_from_db()` available.
A public
remove_prefetched_objects()
would be more fine-grained, yes, but would it be worth the extra API surface area, documentation and potential for user-confusion?("Just use
refresh_from_db()
" is nice and clear.)