[Django] #26706: ManyToMany clear doesn't clear the items from the object but deletes from db
Django
db
----------------------------------------------+----------------------------
Reporter: cosmosgenius | Owner: nobody
Type: Bug | Status: new
Component: Database layer (models, ORM) | Version: 1.8
Severity: Normal | Keywords: manytomany,
| m2m, clear
Triage Stage: Unreviewed | Has patch: 0
Easy pickings: 0 | UI/UX: 0
----------------------------------------------+----------------------------
{{{
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 "[ ]"
--
Ticket URL: <https://code.djangoproject.com/ticket/26706>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
db
Reporter: cosmosgenius | Owner: nobody
Type: Bug | Status: closed
Component: Database layer | Version: 1.8
(models, ORM) | Resolution:
Severity: Normal | worksforme
Keywords: manytomany, m2m, | Triage Stage:
clear | Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by timgraham):
* status: new => closed
* needs_better_patch: => 0
* resolution: => worksforme
* needs_tests: => 0
* needs_docs: => 0
Comment:
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
[https://github.com/django/django/blob/9c53facc45908bc0593de194a60bc75e5d34a48e/tests/many_to_many/tests.py#L507-L509
tests the behavior].
Please reopen if you can provide a failing test case.
--
Ticket URL: <https://code.djangoproject.com/ticket/26706#comment:1>
Django
db
Reporter: cosmosgenius | Owner: nobody
Type: Bug | Status: new
(models, ORM) |
Severity: Normal | Resolution:
Keywords: manytomany, m2m, | Triage Stage:
clear | Unreviewed
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by cosmosgenius):
* status: closed => new
* resolution: worksforme =>
Old description:
> {{{
>
> 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 "[ ]"
New description:
{{{
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.
--
Comment:
{{{
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())
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/26706#comment:2>
Django
from the object but deletes from db
Reporter: cosmosgenius | Owner: nobody
Type: Bug | Status: new
(models, ORM) |
Severity: Normal | Resolution:
Keywords: manytomany, m2m, | Triage Stage:
clear | Unreviewed
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
--
Ticket URL: <https://code.djangoproject.com/ticket/26706#comment:3>
Django
cache should be cleared on clear().
-------------------------------------+-------------------------------------
Reporter: cosmosgenius | Owner: nobody
Type: Bug | Status: new
(models, ORM) |
Severity: Normal | Resolution:
clear |
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by charettes):
* stage: Unreviewed => Accepted
Comment:
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/26706#comment:4>
Django
cache should be cleared on clear().
-------------------------------------+-------------------------------------
Type: Bug | Status: assigned
Component: Database layer | Version: 1.8
(models, ORM) |
Severity: Normal | Resolution:
Keywords: manytomany, m2m, | Triage Stage: Accepted
clear |
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by yoongkang):
* owner: nobody => yoongkang
* status: new => assigned
--
Ticket URL: <https://code.djangoproject.com/ticket/26706#comment:5>
Django
cache should be cleared on clear().
-------------------------------------+-------------------------------------
Reporter: cosmosgenius | Owner: yoongkang
Type: Bug | Status: assigned
(models, ORM) |
Severity: Normal | Resolution:
clear | checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by charettes):
* keywords: manytomany, m2m, clear => manytomany m2m clear
* has_patch: 0 => 1
* version: 1.8 => master
* stage: Accepted => Ready for checkin
Comment:
[https://github.com/django/django/pull/6725/files PR]
LGTM pending some cosmetic changes.
--
Ticket URL: <https://code.djangoproject.com/ticket/26706#comment:6>
Django
cache should be cleared on clear().
-------------------------------------+-------------------------------------
Reporter: cosmosgenius | Owner: yoongkang
Type: Bug | Status: assigned
Component: Database layer | Version: master
(models, ORM) |
Severity: Normal | Resolution:
Keywords: manytomany m2m | Triage Stage: Ready for
clear | checkin
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by timgraham):
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/26706#comment:7>
Django
cache should be cleared on clear().
-------------------------------------+-------------------------------------
Reporter: cosmosgenius | Owner: yoongkang
Type: Bug | Status: assigned
Component: Database layer | Version: master
(models, ORM) |
Severity: Normal | Resolution:
Keywords: manytomany m2m | Triage Stage: Ready for
clear | checkin
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by yoongkang):
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 the cache to be cleared when you mutate the items?
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/26706#comment:8>
Django
cache should be cleared on clear().
-------------------------------------+-------------------------------------
Reporter: cosmosgenius | Owner: yoongkang
Type: Bug | Status: assigned
Component: Database layer | Version: master
(models, ORM) |
Severity: Normal | Resolution:
Keywords: manytomany m2m | Triage Stage: Ready for
clear | checkin
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by timgraham):
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/26706#comment:9>
Django
cache should be cleared on clear().
-------------------------------------+-------------------------------------
Reporter: cosmosgenius | Owner: yoongkang
Type: Bug | Status: assigned
Component: Database layer | Version: master
(models, ORM) |
Severity: Normal | Resolution:
Keywords: manytomany m2m | Triage Stage: Ready for
clear | checkin
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by timgraham):
[https://groups.google.com/d/topic/django-
developers/ejOpJ_r7tv8/discussion django-developers thread]
--
Ticket URL: <https://code.djangoproject.com/ticket/26706#comment:10>
Django
cache should be cleared on clear().
-------------------------------------+-------------------------------------
Reporter: cosmosgenius | Owner: yoongkang
Type: Bug | Status: assigned
Component: Database layer | Version: master
(models, ORM) |
Severity: Normal | Resolution:
clear |
Has patch: 1 | Needs documentation: 1
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by timgraham):
* needs_docs: 0 => 1
* stage: Ready for checkin => Accepted
--
Ticket URL: <https://code.djangoproject.com/ticket/26706#comment:11>
Django
cache should be cleared on clear().
-------------------------------------+-------------------------------------
Reporter: cosmosgenius | Owner: yoongkang
Type: Bug | Status: assigned
Component: Database layer | Version: master
(models, ORM) |
Severity: Normal | Resolution:
Keywords: manytomany m2m | Triage Stage: Accepted
clear |
Has patch: 1 | Needs documentation: 1
Easy pickings: 0 | UI/UX: 0
Changes (by mdentremont):
* cc: mattdentremont@… (added)
--
Ticket URL: <https://code.djangoproject.com/ticket/26706#comment:12>
Django
-------------------------------------+-------------------------------------
Reporter: cosmosgenius | Owner: yoongkang
Type: Bug | Status: assigned
Component: Database layer | Version: master
(models, ORM) |
Severity: Normal | Resolution:
Keywords: manytomany m2m | Triage Stage: Accepted
clear |
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by timgraham):
* needs_docs: 1 => 0
--
Ticket URL: <https://code.djangoproject.com/ticket/26706#comment:13>
Django
cache should be cleared on add/remove/clear().
-------------------------------------+-------------------------------------
Reporter: cosmosgenius | Owner: yoongkang
Component: Database layer | Version: master
(models, ORM) |
Keywords: manytomany m2m | Triage Stage: Accepted
clear |
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Tim Graham <timograham@…>):
* status: assigned => closed
* resolution: => fixed
Comment:
In [changeset:"d30febb4e59b659e0d279c77f61f936c199a05b2" d30febb4]:
{{{
#!CommitTicketReference repository=""
revision="d30febb4e59b659e0d279c77f61f936c199a05b2"
Fixed #26706 -- Made RelatedManager modification methods clear
prefetch_related() cache.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/26706#comment:14>
Django
cache should be cleared on add/remove/clear().
-------------------------------------+-------------------------------------
| Kang Lim
Type: Bug | Status: closed
Component: Database layer | Version: dev
(models, ORM) |
Severity: Normal | Resolution: fixed
Keywords: manytomany m2m | Triage Stage: Accepted
clear |
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by GitHub <noreply@…>):
In [changeset:"40a2c811e3ed7fdb26ab4443e39e113c2fcf2aa9" 40a2c811]:
{{{
#!CommitTicketReference repository=""
revision="40a2c811e3ed7fdb26ab4443e39e113c2fcf2aa9"
Refs #26706, Refs #34633 -- Added test for prefetch_related() cache
invalidation in ManyRelatedManager.create().
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/26706#comment:15>