#32045 closed Cleanup/optimization (fixed)
Document that GenericRelation.remove()/clear() delete objects.
Reported by: | chgad | Owned by: | Craig Smith |
---|---|---|---|
Component: | Documentation | Version: | 3.1 |
Severity: | Normal | Keywords: | |
Cc: | Asjad Ahmed Khan, Sumagna Das | 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
Hello I'm currently working on some custom logic in the admin interface when i encountered this behavior.
Suppose we have two simple models
class Photograph(DeepZoom): content_type = models.ForeignKey(ContentType, on_delete=models.CASCADE, null=True) object_id = models.PositiveIntegerField(default=0) profile = GenericForeignKey("content_type", "object_id") img = models.FileField(...)
and
class MyProfile(models.Model): name = models.CharField(max_length=200, default="") photos = GenericRelation(Photograph, null=True)
I can confirm that the creation of a "Photograph" without a relation to a "MyProfile" instance works fine. Also, adding "Photographs" to a "MyProfile" via
some_profile_instance.photos.add(some_photograph_instance)
works just fine.
But as soon as i want to remove a "Photograph" from the Relation to a "MyProfile" instance the "Photograph" instance is not only removed from the relation but also deleted. In the shell it looks like this (pre-created Photographs an MyPofile instance/s):
In [1]: Photograph.objects.all() Out[1]: <QuerySet [<Photograph: photograph/file_one.png>, <Photograph: photograph/file_two.png>]> In [2]: MyProfile.objects.first().photos.all() Out[2]: <QuerySet [<Photograph: photograph/file_one.png>]> In [3]: photo_instance = MyProfile.objects.first().photos.first() In [4]: MyProfile.objects.first().new_photo.remove(photo_instance) In [5]: MyProfile.objects.first().photos.all() Out[5]: <QuerySet []> # As expected In [6]: Photograph.objects.all() Out[6]: <QuerySet [<Photograph: photograph/file_two.png>]> # ... but the Photograph got entirely deleted
As far as i understand the docs https://docs.djangoproject.com/en/3.1/ref/models/relations/#django.db.models.fields.related.RelatedManager.remove
this is not the expected behavior. If the behavior of "remove" is different for GenericRelations i couldn't find any documentation on it.
P.S.: The same issue holds for the "set" method.
Attachments (1)
Change History (26)
comment:1 by , 4 years ago
Component: | Uncategorized → contrib.contenttypes |
---|
comment:3 by , 4 years ago
Cc: | added |
---|---|
Easy pickings: | set |
Needs tests: | set |
Owner: | changed from | to
Status: | new → assigned |
I have put this ticket under 'Needs Tests' category.
comment:4 by , 4 years ago
Yeah it's clear to me that the provided link points to the general RelatedManager doc but since I couldn't find anything concerning GenericRelations it was the only resource mentioning what set
, remove
and add
are intended to do.
Do you agree that the RelatedManager for GenericRelations should behave the same as it does for other relations?
follow-up: 7 comment:6 by , 4 years ago
Easy pickings: | unset |
---|---|
Needs tests: | unset |
Resolution: | → needsinfo |
Status: | assigned → closed |
Summary: | Removing item from GenericRelation deletes the item. → Removing item from GenericRelation deletes all items. |
Thanks for this ticket, however everything works for me with provided models:
class MyTests(TestCase): def test_1(self): p1 = MyProfile.objects.create(name='profile1') p2 = MyProfile.objects.create(name='profile2') photo1 = Photograph.objects.create(profile=p1) photo2 = Photograph.objects.create(profile=p1) photo3 = Photograph.objects.create(profile=p2) self.assertEqual(Photograph.objects.count(), 3) self.assertCountEqual(p1.photos.all(), [photo1, photo2]) self.assertCountEqual(p2.photos.all(), [photo3]) p1.photos.remove(photo1) self.assertCountEqual(p1.photos.all(), [photo2]) self.assertCountEqual(p2.photos.all(), [photo3]) self.assertEqual(Photograph.objects.count(), 2)
Can you provide a small sample project that reproduces this issue?
comment:7 by , 4 years ago
Replying to felixxm:
Thanks for this ticket, however everything works for me with provided models:
class MyTests(TestCase): def test_1(self): p1 = MyProfile.objects.create(name='profile1') p2 = MyProfile.objects.create(name='profile2') photo1 = Photograph.objects.create(profile=p1) photo2 = Photograph.objects.create(profile=p1) photo3 = Photograph.objects.create(profile=p2) self.assertEqual(Photograph.objects.count(), 3) self.assertCountEqual(p1.photos.all(), [photo1, photo2]) self.assertCountEqual(p2.photos.all(), [photo3]) p1.photos.remove(photo1) self.assertCountEqual(p1.photos.all(), [photo2]) self.assertCountEqual(p2.photos.all(), [photo3]) self.assertEqual(Photograph.objects.count(), 2)Can you provide a small sample project that reproduces this issue?
That is what I'm describing. But remove
should not delete the photo1
entry. So with your example Test I expect the last assertion fail!
As the docs. describe remove
is expected to remove an entry from the Relation p1.photos.all()
but keep it in the Database, unlinked to a profile.
comment:8 by , 4 years ago
From the documentation on clear()
directly below the docs linked in the issue description:
"Note this doesn’t delete the related objects – it just disassociates them."
It sounds like that's what you want, which is why I asked if you wanted clear()
instead of remove()
. If clear()
gives you the behavior you want, I suggest this be accepted as a documentation ticket to clarify that remove()
also removes from the database.
follow-up: 10 comment:9 by , 4 years ago
Component: | contrib.contenttypes → Documentation |
---|---|
Easy pickings: | set |
Resolution: | needsinfo |
Status: | closed → new |
Summary: | Removing item from GenericRelation deletes all items. → Document that GenericRelation.remove()/clear() delete objects. |
Triage Stage: | Unreviewed → Accepted |
Type: | Bug → Cleanup/optimization |
Version: | 3.0 → 3.1 |
clear()
and remove()
for GenericRelation
s delete objects, this is how it works since its introduction in bca5327b21eb2e3ee18292cbe532d6d0071201d8 (it's mentioned in some comments 21174#comment:4, 21174#comment:6 and Django 1.7 release notes).
I think we can add a note about this behavior of clear()
and remove()
to the Reverse generic relations docs.
comment:10 by , 4 years ago
Replying to felixxm:
clear()
andremove()
forGenericRelation
s delete objects, this is how it works since its introduction in bca5327b21eb2e3ee18292cbe532d6d0071201d8 (it's mentioned in some comments 21174#comment:4, 21174#comment:6 and Django 1.7 release notes).
Thanks for pointing this out.
I think we can add a note about this behavior of
clear()
andremove()
to the Reverse generic relations docs.
Yes! This would have saved me a lot of time.
But nonetheless, wouldn't it be disreable to be able to disassociate Objects from the GenericRelation Queryset ? Other than by doing what macieyn suggested earlier.
comment:11 by , 4 years ago
Cc: | added |
---|
Is this a good ticket for a first time contributor?
if yes then i would like to try on it and would need some help also
comment:12 by , 4 years ago
Owner: | removed |
---|---|
Status: | new → assigned |
Yes, this is a good first ticket. This ticket was claimed before it was accepted in its current form, so I'm unassigning for now, but that individual or anyone else is free to claim it.
comment:13 by , 4 years ago
Status: | assigned → new |
---|
by , 4 years ago
Attachment: | 32045.diff added |
---|
Patch to update documentation about remove and clear methods on GenericRelation manager
comment:14 by , 4 years ago
Owner: | set to |
---|---|
Status: | new → assigned |
Hi, I hope that the above patch can resolve this one. It only adds descriptions of the remove and clear methods, and not too much detail. But it addresses the important point that those two methods on GenericRelation managers delete the related objects. Please let me know if it needs more detail or breadth. Also, should the tests be updated to confirm the behaviour of those methods. Happy to add some tests for these methods too if it helps.
comment:15 by , 4 years ago
Has patch: | set |
---|
comment:17 by , 4 years ago
Also, should the tests be updated to confirm the behaviour of those methods. Happy to add some tests for these methods too if it helps.
Extra tests are always welcome.
comment:19 by , 4 years ago
Hi Mariusz, I have linked a PR above. It has a couple extra tests. Please let me know if there are any further changes that are necessary. For instance, should the mention about the difference in remove()
and clear()
on the RelatedManager
be made into an admonition
?
comment:21 by , 4 years ago
Patch needs improvement: | set |
---|
comment:22 by , 4 years ago
Patch needs improvement: | unset |
---|---|
Triage Stage: | Accepted → Ready for checkin |
As a workaround I suggest this solution:
I agree that the way it work currently is not what you would expect. BTW the link to the docs that you provided is for Related Manager in general, but you're using Contenttype and its docs does not provide info about adding relationships through
add
orset
orremove
method so it's behavoiur might be unexpected. Here is link to Generic Relations in Contenttypes https://docs.djangoproject.com/en/3.1/ref/contrib/contenttypes/#generic-relations