Document that GenericRelation.remove()/clear() delete objects.

[chgad]

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.

[macieyn]

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

[Asjad Ahmed Khan]

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

[chgad]

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?

[Jacob Walls]

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

[Mariusz Felisiak]

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?

[chgad]

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.

[Jacob Walls]

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.

[Mariusz Felisiak]

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.

[chgad]

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.

[Sumagna Das]

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

[Jacob Walls]

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.

[Craig Smith]

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

[Craig Smith]

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.

[Mariusz Felisiak]

Craig, thanks please submit PR via GitHub.

[Mariusz Felisiak]

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.

[Craig Smith]

​PR

[Craig Smith]

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?

[Mariusz Felisiak]

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

[Mariusz Felisiak <felisiak.mariusz@…>]

In 6fa3d02f:

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

[Mariusz Felisiak <felisiak.mariusz@…>]

In 354c1524:

Fixed #32045 -- Doc'd GenericRelatedObjectManager methods.

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

[Mariusz Felisiak <felisiak.mariusz@…>]

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