Opened 2 years ago
Closed 2 years ago
#34633 closed Bug (fixed)
Add prefetch_related() cache invalidation for create() in reverse many-to-one managers.
| Reported by: | Rob Percival | Owned by: | Aman Pandey |
|---|---|---|---|
| Component: | Database layer (models, ORM) | Version: | 4.2 |
| Severity: | Normal | Keywords: | |
| Cc: | Triage Stage: | Ready for checkin | |
| Has patch: | yes | Needs documentation: | no |
| Needs tests: | no | Patch needs improvement: | no |
| Easy pickings: | yes | UI/UX: | no |
Description
The documentation says:
if you call the database-altering methods add(), remove(), clear() or set(), on related managers, any prefetched cache for the relation will be cleared.
This is accurate. However, there are other database-altering methods, such as create(), for which the cache ought to be cleared but isn't. This results in this confusing state of affairs:
x = MyModel.objects.prefetch_related("related_objects").get()
assert len(x.related_objects.all()) == 0
x.related_objects.create()
assert len(x.related_objects.all()) == 1 # Assertion fails because the prefetch cache isn't cleared by create()
Using add() rather than create() would cause the above code to work as expected. If there's a good reason for create() not clearing the cache, could that be documented please? Otherwise, could it clear the cache? From a search of Stack Overflow, this is definitely a source of confusion for some.
Change History (13)
comment:1 by , 2 years ago
comment:2 by , 2 years ago
| Summary: | RelatedManager._remove_prefetched_objects() not called in RelatedManager.create() → Add prefetch_related() cache invalidation for create() in reverse many-to-one managers. |
|---|---|
| Triage Stage: | Unreviewed → Accepted |
Thanks for the report, it seems that it's only missing for create() in reverse many-to-one managers. It works for many-to-many managers (we could add an extra test method for this, see PR) and GenericRelation (see #29612). See a regression test:
-
tests/many_to_one/tests.py
diff --git a/tests/many_to_one/tests.py b/tests/many_to_one/tests.py index 7a6d112a09..d360cd9164 100644
a b class ManyToOneTests(TestCase): 799 799 # refs #21563 800 800 self.assertFalse(hasattr(Article(), "reporter")) 801 801 802 def test_create_after_prefetch(self): 803 c = City.objects.create(name="Musical City") 804 d1 = District.objects.create(name="Ladida", city=c) 805 city = City.objects.prefetch_related("districts").get(id=c.id) 806 self.assertSequenceEqual(city.districts.all(), [d1]) 807 d2 = city.districts.create(name="Goa") 808 self.assertSequenceEqual(city.districts.all(), [d1, d2]) 809 802 810 def test_clear_after_prefetch(self): 803 811 c = City.objects.create(name="Musical City") 804 812 d = District.objects.create(name="Ladida", city=c)
Rob, would you like to prepare a patch?
comment:3 by , 2 years ago
| Easy pickings: | set |
|---|
comment:5 by , 2 years ago
| Owner: | changed from to |
|---|---|
| Status: | new → assigned |
Sure, I can look at creating a patch for this.
comment:6 by , 2 years ago
| Owner: | changed from to |
|---|
comment:7 by , 2 years ago
| Resolution: | → fixed |
|---|---|
| Status: | assigned → closed |
comment:8 by , 2 years ago
| Resolution: | fixed |
|---|---|
| Status: | closed → new |
Please do not close a ticket before a patch is merged into the main branch.
comment:9 by , 2 years ago
| Owner: | changed from to |
|---|---|
| Status: | new → assigned |
comment:10 by , 2 years ago
| Owner: | changed from to |
|---|
comment:12 by , 2 years ago
| Has patch: | set |
|---|---|
| Triage Stage: | Accepted → Ready for checkin |
The original ticket #26706, ML thread & PR didn't seem to make any mention of
create()I'd agree this seems inconsistent but keen to see what felix & charettes (charettes reviewed the PR) think 🤔