Opened 2 years ago

Last modified 13 months ago

#19149 assigned Bug

Generic Relation not cascading with Multi table inheritance.

Reported by: thomaspurchas Owned by: nicolas
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 (14)

comment:1 Changed 2 years ago by nicolas

  • Needs documentation unset
  • Needs tests unset
  • Owner changed from nobody to nicolas
  • Patch needs improvement unset
  • Status changed from new to assigned

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:3 Changed 2 years ago by nicolas

  • Triage Stage changed from Unreviewed to Ready for checkin

comment:4 Changed 2 years ago by charettes

  • Triage Stage changed from Ready for checkin to Accepted
  • Version changed from 1.4 to master

Don't mark your own patch as RFC.

comment:5 Changed 2 years ago by jezdez

  • Triage Stage changed from Accepted to Ready for checkin

comment:6 Changed 2 years ago by akaariai

  • Patch needs improvement set
  • Triage Stage changed from Ready for checkin to Accepted

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

comment:7 Changed 2 years ago by michael@…

  • Cc michael@… added

comment:8 Changed 2 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 2 years ago by akaariai

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 2 years ago by akaariai

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 22 months ago by miracle2k

  • Cc michael@… added

comment:12 Changed 16 months ago by vajrasky

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 16 months ago by vajrasky

  • Cc sky.kok@… added

comment:14 Changed 13 months ago by timo

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.

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