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 , 12 years ago
Owner: | changed from | to
---|---|
Status: | new → assigned |
comment:3 by , 12 years ago
Triage Stage: | Unreviewed → Ready for checkin |
---|
comment:4 by , 12 years ago
Triage Stage: | Ready for checkin → Accepted |
---|---|
Version: | 1.4 → master |
Don't mark your own patch as RFC.
comment:5 by , 12 years ago
Triage Stage: | Accepted → Ready for checkin |
---|
comment:6 by , 12 years ago
Patch needs improvement: | set |
---|---|
Triage Stage: | Ready for checkin → Accepted |
The patch isn't correct - doing if not objs: return [] doesn't return a queryset.
comment:7 by , 12 years ago
Cc: | added |
---|
comment:8 by , 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 , 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 , 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 , 12 years ago
Cc: | added |
---|
comment:12 by , 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 , 11 years ago
Cc: | added |
---|
comment:14 by , 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 , 8 years ago
Owner: | removed |
---|---|
Status: | assigned → new |
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.