Opened 12 years ago

Last modified 8 years ago

#19149 new Bug

Generic Relation not cascading with Multi table inheritance.

Reported by: thomaspurchas Owned by:
Component: Database layer (models, ORM) Version: dev
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 by Nicolas Lara, 12 years ago

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:3 by Nicolas Lara, 12 years ago

Triage Stage: UnreviewedReady for checkin

comment:4 by Simon Charette, 12 years ago

Triage Stage: Ready for checkinAccepted
Version: 1.4master

Don't mark your own patch as RFC.

comment:5 by Jannis Leidel, 12 years ago

Triage Stage: AcceptedReady for checkin

comment:6 by Anssi Kääriäinen, 12 years ago

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 by michael@…, 12 years ago

Cc: michael@… added

comment:8 by miracle2k, 12 years ago

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

comment:9 by Anssi Kääriäinen, 12 years ago

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 by Anssi Kääriäinen, 12 years ago

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 by miracle2k, 12 years ago

Cc: michael@… added

comment:12 by Vajrasky Kok, 11 years ago

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 by Vajrasky Kok, 11 years ago

Cc: sky.kok@… added

comment:14 by Tim Graham, 11 years ago

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 by Tim Graham, 8 years ago

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