Opened 8 years ago

Closed 8 years ago

Last modified 18 months ago

#26706 closed Bug (fixed)

Reverse many to one and forward many to many managers's prefetch object cache should be cleared on add/remove/clear().

Reported by: Sharat M R Owned by: Yoong Kang Lim
Component: Database layer (models, ORM) Version: dev
Severity: Normal Keywords: manytomany m2m clear
Cc: mattdentremont@… Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description (last modified by Sharat M R)

class Media(models.Model):
    name = models.CharField(max_length=100)


class Tag(models.Model):
    tag = models.CharField(max_length=100)
    medias = models.ManyToManyField(Media)

    def __unicode__(self):
        return self.tag

Assume tag object below already has three items of Media model in medias which are saved in db. [m1, m2, m3].

tag = Tag.objects.get(id=1)
tag.medias.clear()
print(tag.medias.all())

The above code prints all the three [m1, m2, m3]. Fetching the tag again using get and printing givens an empty list "[ ]"

Failing Testcase

class Test(TestCase):
    def test(self):
        tag = Tag.objects.create(tag='Tag')
        tag.medias.create(name='A')
        tag.medias.create(name='B')
        tag.medias.create(name='C')
        tag = Tag.objects.prefetch_related('medias').get(id=1)
        tag.medias.clear()
        print(tag.medias.all())

I guess its related to prefetch_related function.

Change History (15)

comment:1 by Tim Graham, 8 years ago

Resolution: worksforme
Status: newclosed

I can't reproduce that. Here's my complete test case:

from django.test import TestCase

from .models import Tag


class Test(TestCase):
    def test(self):
        tag = Tag.objects.create(tag='Tag')
        tag.medias.create(name='A')
        tag.medias.create(name='B')
        tag.medias.create(name='C')
        tag = Tag.objects.get(id=1)
        tag.medias.clear()
        print(tag.medias.all())

Also, Django's test suite tests the behavior.

Please reopen if you can provide a failing test case.

comment:2 by Sharat M R, 8 years ago

Description: modified (diff)
Resolution: worksforme
Status: closednew
class Test(TestCase):
    def test(self):
        tag = Tag.objects.create(tag='Tag')
        tag.medias.create(name='A')
        tag.medias.create(name='B')
        tag.medias.create(name='C')
        tag = Tag.objects.prefetch_related('medias').get(id=1)
        tag.medias.clear()
        print(tag.medias.all())

This test print all the three medias for me

Last edited 8 years ago by Sharat M R (previous) (diff)

comment:3 by Sharat M R, 8 years ago

Summary: ManyToMany clear doesn't clear the items from the object but deletes from db"ManyToMany with prefetch_related" clear function doesn't clear the items from the object but deletes from db

comment:4 by Simon Charette, 8 years ago

Summary: "ManyToMany with prefetch_related" clear function doesn't clear the items from the object but deletes from dbReverse many to one and forward many to many managers's prefetch object cache should be cleared on clear().
Triage Stage: UnreviewedAccepted

From a quick look at the code this looks like a legitimate issue.

The manager classes returned by create_forward_many_to_many_manager and create_reverse_many_to_one_manager factories are both affected.

comment:5 by Yoong Kang Lim, 8 years ago

Owner: changed from nobody to Yoong Kang Lim
Status: newassigned

comment:6 by Simon Charette, 8 years ago

Has patch: set
Keywords: manytomany, m2m, clear → manytomany m2m clear
Triage Stage: AcceptedReady for checkin
Version: 1.8master

PR

LGTM pending some cosmetic changes.

comment:7 by Tim Graham, 8 years ago

I agree the current behavior might be unexpected, however, I'm not sure if it's a good idea to add this behavior on some manager methods but not others (see #25344)? At least the behavior should be documented, probably.

comment:8 by Yoong Kang Lim, 8 years ago

I just read #25344, thanks, didn't know there were other methods that were affected.

I think the behaviour should be added in both cases. Is it not reasonable, in general, to expect cache to be cleared on change of data? If the historical cached list is needed for any reason, it could always be stored in a variable.

I do agree that in either case it should be documented. Either that your prefetched values could be out of date, or that any changes via the add(), clear() and remove() methods will clear the cache.

I have updated my patch to cater for add(), remove() in addition to clear(). Happy to amend and add documentation either way we go, but I'll wait for consensus before adding documentation.

Version 1, edited 8 years ago by Yoong Kang Lim (previous) (next) (diff)

comment:9 by Tim Graham, 8 years ago

Could you make a post on the DevelopersMailingList about this? I want to make sure that there's a consensus that the possible backwards-incompatibilities of clearing existing caches are acceptable.

comment:11 by Tim Graham, 8 years ago

Needs documentation: set
Triage Stage: Ready for checkinAccepted

comment:12 by Matt d'Entremont, 8 years ago

Cc: mattdentremont@… added

comment:13 by Tim Graham, 8 years ago

Needs documentation: unset
Summary: Reverse many to one and forward many to many managers's prefetch object cache should be cleared on clear().Reverse many to one and forward many to many managers's prefetch object cache should be cleared on add/remove/clear().

comment:14 by Tim Graham <timograham@…>, 8 years ago

Resolution: fixed
Status: assignedclosed

In d30febb4:

Fixed #26706 -- Made RelatedManager modification methods clear prefetch_related() cache.

comment:15 by GitHub <noreply@…>, 18 months ago

In 40a2c811:

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

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