Opened 4 years ago

Closed 4 years ago

Last modified 4 years ago

#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)

32045.diff (1.3 KB ) - added by Craig Smith 4 years ago.
Patch to update documentation about remove and clear methods on GenericRelation manager

Download all attachments as: .zip

Change History (26)

comment:1 by macieyn, 4 years ago

Component: Uncategorizedcontrib.contenttypes

comment:2 by macieyn, 4 years ago

As a workaround I suggest this solution:

In [1]: from app.models import Photo, Profile

In [2]: Profile.objects.all()
Out[2]: <QuerySet [<Profile: Profile object (1)>]>

In [3]: Photo.objects.all()
Out[3]: <QuerySet [<Photo: Photo object (2)>, <Photo: Photo object (3)>]>

In [4]: profile = Profile.objects.first()

In [5]: profile.photos.all()
Out[5]: <QuerySet [<Photo: Photo object (2)>]>

In [6]: photo = Photo.objects.last()

In [7]: photo
Out[7]: <Photo: Photo object (3)>

In [8]: profile.photos.add(photo)

In [9]: profile.photos.all()
Out[9]: <QuerySet [<Photo: Photo object (2)>, <Photo: Photo object (3)>]>

In [10]: photo.profile
Out[10]: <Profile: Profile object (1)>

In [11]: photo.profile = None

In [12]: photo.save()

In [13]: profile.photos.all()
Out[13]: <QuerySet [<Photo: Photo object (2)>]>

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 or set or remove 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

Last edited 4 years ago by macieyn (previous) (diff)

comment:3 by Asjad Ahmed Khan, 4 years ago

Cc: Asjad Ahmed Khan added
Easy pickings: set
Needs tests: set
Owner: changed from nobody to Asjad Ahmed Khan
Status: newassigned

I have put this ticket under 'Needs Tests' category.

comment:4 by chgad, 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, removeand add are intended to do.

Do you agree that the RelatedManager for GenericRelations should behave the same as it does for other relations?

comment:5 by Jacob Walls, 4 years ago

What happens if you use clear() or set(clear=True)?

comment:6 by Mariusz Felisiak, 4 years ago

Easy pickings: unset
Needs tests: unset
Resolution: needsinfo
Status: assignedclosed
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?

in reply to:  6 comment:7 by chgad, 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 to 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.

Last edited 4 years ago by chgad (previous) (diff)

comment:8 by Jacob Walls, 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.

comment:9 by Mariusz Felisiak, 4 years ago

Component: contrib.contenttypesDocumentation
Easy pickings: set
Resolution: needsinfo
Status: closednew
Summary: Removing item from GenericRelation deletes all items.Document that GenericRelation.remove()/clear() delete objects.
Triage Stage: UnreviewedAccepted
Type: BugCleanup/optimization
Version: 3.03.1

clear() and remove() for GenericRelations 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.

in reply to:  9 comment:10 by chgad, 4 years ago

Replying to felixxm:

clear() and remove() for GenericRelations 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() and remove() 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 Sumagna Das, 4 years ago

Cc: Sumagna Das 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 Jacob Walls, 4 years ago

Owner: Asjad Ahmed Khan removed
Status: newassigned

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 Jacob Walls, 4 years ago

Status: assignednew

by Craig Smith, 4 years ago

Attachment: 32045.diff added

Patch to update documentation about remove and clear methods on GenericRelation manager

comment:14 by Craig Smith, 4 years ago

Owner: set to Craig Smith
Status: newassigned

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 Craig Smith, 4 years ago

Has patch: set

comment:16 by Mariusz Felisiak, 4 years ago

Craig, thanks please submit PR via GitHub.

comment:17 by Mariusz Felisiak, 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:18 by Craig Smith, 4 years ago

comment:19 by Craig Smith, 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:20 by Mariusz Felisiak, 4 years ago

Thanks, reviewers will comment on PR, it can take few days.

comment:21 by Mariusz Felisiak, 4 years ago

Patch needs improvement: set

comment:22 by Mariusz Felisiak, 4 years ago

Patch needs improvement: unset
Triage Stage: AcceptedReady for checkin

comment:23 by Mariusz Felisiak <felisiak.mariusz@…>, 4 years ago

In 6fa3d02f:

Refs #32045 -- Added tests for GenericRelatedObjectManager.clear()/remove().

comment:24 by Mariusz Felisiak <felisiak.mariusz@…>, 4 years ago

Resolution: fixed
Status: assignedclosed

In 354c1524:

Fixed #32045 -- Doc'd GenericRelatedObjectManager methods.

This also documents that .remove() and clear() methods delete related
objects.

comment:25 by Mariusz Felisiak <felisiak.mariusz@…>, 4 years ago

In d67f965:

[3.1.x] Fixed #32045 -- Doc'd GenericRelatedObjectManager methods.

This also documents that .remove() and clear() methods delete related
objects.

Backport of 354c1524b38c9b9f052c1d78dcbfa6ed5559aeb3 from master

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