Opened 11 months ago

Closed 5 months 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 David Sanders, 11 months ago

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 🤔

comment:2 by Mariusz Felisiak, 11 months 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: UnreviewedAccepted

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):  
    799799        # refs #21563
    800800        self.assertFalse(hasattr(Article(), "reporter"))
    801801
     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
    802810    def test_clear_after_prefetch(self):
    803811        c = City.objects.create(name="Musical City")
    804812        d = District.objects.create(name="Ladida", city=c)

Rob, would you like to prepare a patch?

Last edited 11 months ago by Mariusz Felisiak (previous) (diff)

comment:3 by Mariusz Felisiak, 11 months ago

Easy pickings: set

comment:4 by GitHub <noreply@…>, 11 months ago

In 40a2c811:

Refs #26706, Refs #34633 -- Added test for prefetch_related() cache invalidation in ManyRelatedManager.create().

comment:5 by Rob Percival, 11 months ago

Owner: changed from nobody to Rob Percival
Status: newassigned

Sure, I can look at creating a patch for this.

Hey are you working on this patch bcz i am interested to work on it

Last edited 9 months ago by Faishal Manzar (previous) (diff)

comment:6 by Muhammad Zulkifal, 9 months ago

Owner: changed from Rob Percival to Muhammad Zulkifal

comment:7 by Muhammad Zulkifal, 9 months ago

Resolution: fixed
Status: assignedclosed

comment:8 by Claude Paroz, 9 months ago

Resolution: fixed
Status: closednew

Please do not close a ticket before a patch is merged into the main branch.

comment:9 by Claude Paroz, 9 months ago

Owner: changed from Muhammad Zulkifal to Rob Percival
Status: newassigned

comment:10 by Aman Pandey, 6 months ago

Owner: changed from Rob Percival to Aman Pandey

comment:12 by Mariusz Felisiak, 5 months ago

Has patch: set
Triage Stage: AcceptedReady for checkin

comment:13 by Mariusz Felisiak <felisiak.mariusz@…>, 5 months ago

Resolution: fixed
Status: assignedclosed

In 0fcd72b:

Fixed #34633 -- Made create() method of reverse many-to-one managers clear prefetch_related() cache.

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