Opened 4 years ago

Last modified 6 months ago

#19149 new Bug

Generic Relation not cascading with Multi table inheritance.

Reported by: thomaspurchas Owned by:
Component: Database layer (models, ORM) Version: master
Severity: Normal Keywords: multitable, inheritance, genericforeignkey
Cc: thomaspurchas, michael@…, michael@…, sky.kok@… Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: yes
Easy pickings: no UI/UX: no

Description

Generic Relations don't cascade on delete if you are using the generic relation of a superclass in a subclass, then deleting the subclass does not result in the normal cascade behaviour.

For example using the following models:

class TaggedItem(models.Model):
    tag = models.SlugField()
    content_type = models.ForeignKey(ContentType)
    object_id = models.PositiveIntegerField()
    content_object = generic.GenericForeignKey('content_type', 'object_id')

    def __unicode__(self):
        return self.tag

class Post(models.Model):
    title = models.TextField(blank=True)

    relation = generic.GenericRelation(TaggedItem)

    def __unicode__(self):
        return self.title

class ParentPost(Post):

    description = models.TextField(blank=True)

    def __unicode__(self):
        return self.description  

If you create a ParentPost and a TaggedItem related using the GenericForeignKey:

>>> p = ParentPost()
>>> p.title = "This is a title"
>>> p.description = "This is a description"
>>> p.save()

>>> t = TaggedItem(content_object=p, tag="This is a tag")
>>> t.save()
>>> TaggedItem.objects.all()
[<TaggedItem: Ypo>, <TaggedItem: This is a tag>]

and then delete the ParentPost, the TaggedItem is not also deleted, rather it just points to a None object.

>>> p.delete()
>>> TaggedItem.objects.all()
[<TaggedItem: Ypo>, <TaggedItem: This is a tag>]
>>> print t.content_object
None

If you repeat the same procedure with the Post model you get the expected behaviour where the TaggedItem is deleted.

Change History (15)

comment:1 Changed 4 years ago by Nicolas Lara

Owner: changed from nobody to Nicolas Lara
Status: newassigned

This should be a relatively easy fix. At the moment the delete query is adding a WHERE clause with the id of the parent (in this case Post). We need to change that to use the id of the actual object being deleted.

comment:2 Changed 4 years ago by Nicolas Lara

Has patch: set

comment:3 Changed 4 years ago by Nicolas Lara

Triage Stage: UnreviewedReady for checkin

comment:4 Changed 4 years ago by Simon Charette

Triage Stage: Ready for checkinAccepted
Version: 1.4master

Don't mark your own patch as RFC.

comment:5 Changed 4 years ago by Jannis Leidel

Triage Stage: AcceptedReady for checkin

comment:6 Changed 4 years ago by Anssi Kääriäinen

Patch needs improvement: set
Triage Stage: Ready for checkinAccepted

The patch isn't correct - doing if not objs: return [] doesn't return a queryset.

comment:7 Changed 4 years ago by michael@…

Cc: michael@… added

comment:8 Changed 4 years ago by miracle2k

This fixes the merge conflict and returns an empty queryset: https://github.com/miracle2k/django/compare/19149-generic-rel

comment:9 Changed 4 years ago by Anssi Kääriäinen

The patch doesn't work correctly for parent model associations. I modified the test case to this:

    def test_inherited_models_delete(self):
        """
        Test that when deleting a class that inherits a GenericRelation,
        the correct related object is deleted on cascade.
        """
        p = Post.objects.create(title="This is a title",
            description="This is a description")
        ppost = ParentPost.objects.get(pk=p.pk)
        t1 = TaggedItem.objects.create(content_object=p, tag="This is a tag")
        t2 = TaggedItem.objects.create(content_object=ppost,
                                       tag="This is anoter tag")
        ppost_ct = ContentType.objects.get_for_model(ParentPost)
        self.assertEqual(list(TaggedItem.objects.all().order_by('tag')),
                         [t1, t2])
        self.assertEqual(
            list(TaggedItem.objects.filter(content_type=ppost_ct)),
            [t2])
        self.assertEqual(list(Post.objects.all()), [p])
        p.delete()
        self.assertEqual(list(TaggedItem.objects.all()), [])

The last line doesn't pass - the parent model's taggeditem isn't deleted. So, the patch just moves the problem around. At minimum both parent and child model deletion should delete the taggeditems for both parent and child.

There are more problems in parent <-> child deletions in general If there are more than one child for a parent and you delete one of the childs, then the other childs will not be deleted. Also, each parent model retrieval executes a separate query. A more generic solution to all of the problems would be welcome, though just solving this ticket's issue is a good approach, too...

comment:10 Changed 4 years ago by Anssi Kääriäinen

The proper deletion of inheritance chains is somewhat ugly problem. It seems the best approach is to start the deletion from base parents (that is, from those parent models which do not have any concrete parents), then cascade down from there. This should result in correct, though not totally optimal code. OTOH it doesn't matter how fast the deletion code is if it isn't correct...

I will see if I can write a patch using the above idea. I think it will be faster than current code even if it isn't optimal, as each parent model fetch will issue separate query at the moment.

comment:11 Changed 4 years ago by miracle2k

Cc: michael@… added

comment:12 Changed 3 years ago by Vajrasky Kok

This is the PR: https://github.com/django/django/pull/1970

I based my unit test on akaariai's unit test. I did not try to fix other problems (each parent model retrieval executes a separate query, if there are more than one child for a parent and you delete one of the childs, then the other childs will not be deleted), because I think those deserve separate tickets.

comment:13 Changed 3 years ago by Vajrasky Kok

Cc: sky.kok@… added

comment:14 Changed 3 years ago by Tim Graham

I rebased the PR, but there's a failing test:

======================================================================
FAIL: test_cascade_delete_proxy_model_admin_warning (proxy_models.tests.ProxyModelAdminTests)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/tim/code/django/tests/proxy_models/tests.py", line 386, in test_cascade_delete_proxy_model_admin_warning
    collector.collect(ProxyTrackerUser.objects.all())
  File "/home/tim/code/django/django/test/testcases.py", line 110, in __exit__
    query['sql'] for query in self.captured_queries
AssertionError: 13 queries executed, 7 expected

Be sure to uncheck "Patch needs improvement" if you can fix it so the ticket shows up for review.

comment:15 Changed 6 months ago by Tim Graham

Owner: Nicolas Lara deleted
Status: assignednew
Note: See TracTickets for help on using tickets.
Back to Top