Opened 14 years ago

Closed 14 years ago

Last modified 12 years ago

#12953 closed (fixed)

Generic-related objects are cascade-deleted, but the cascade does not extend beyond them

Reported by: Carl Meyer Owned by: Carl Meyer
Component: Database layer (models, ORM) Version: dev
Severity: Keywords:
Cc: miracle2k Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

Say I have these models:

class Award(models.Model):
    name = models.CharField(max_length=25)
    object_id = models.PositiveIntegerField()
    content_type = models.ForeignKey(ContentType)
    content_object = generic.GenericForeignKey()

class AwardNote(models.Model):
    award = models.ForeignKey(Award)
    note = models.CharField(max_length=100)

class Person(models.Model):
    name = models.CharField(max_length=25)
    awards = generic.GenericRelation(Award)

If I create one of each, then delete the Person, Django currently cascade-deletes the generically-related Award, but does not follow the relations any further, and thus the AwardNote is left undeleted. Databases which enforce referential integrity may delete the AwardNote themselves, or raise an error; databases which don't will leave the AwardNote with a broken ForeignKey.

Since Django's deletion behavior is advertised as equivalent to ON DELETE CASCADE, Django should explicitly delete the AwardNote in this case.

Currently deletion of generically-related objects is done in DeleteQuery.delete_batch_related(). I think the fix for this will involve moving it up to Model._collect_sub_objects() instead.

Testcase attached.

Attachments (3)

12953_r12557.diff (2.3 KB ) - added by Carl Meyer 14 years ago.
test-case only
12953_r12599.diff (9.7 KB ) - added by Carl Meyer 14 years ago.
possible fix
12953_r12756.diff (11.5 KB ) - added by Carl Meyer 14 years ago.
patch with additional test for odd M2M case

Download all attachments as: .zip

Change History (13)

by Carl Meyer, 14 years ago

Attachment: 12953_r12557.diff added

test-case only

comment:1 by Carl Meyer, 14 years ago

Component: UncategorizedDatabase layer (models, ORM)
milestone: 1.2
Version: 1.1SVN

If we're smart with the fix for this, it might be possible to avoid special-casing generic relations at all, and thus get rid of one dependency of the ORM on django.contrib.contenttypes.

comment:2 by Russell Keith-Magee, 14 years ago

Triage Stage: UnreviewedAccepted

comment:3 by Carl Meyer, 14 years ago

Owner: changed from nobody to Carl Meyer

I have a fix working for this, also removes the contenttypes dependency; just need to update it for the #6191 fix.

by Carl Meyer, 14 years ago

Attachment: 12953_r12599.diff added

possible fix

comment:4 by Carl Meyer, 14 years ago

Has patch: set

Attached a thorough fix that takes advantage of the fact that M2M intermediaries are now always models, which means _collect_sub_objects can now take care of both GenericRelations and M2Ms, and DeleteQuery.delete_batch_related() is no longer needed. This fixes cascades for generic relations, makes the ORM deletion logic clearer (since it's all in one place) and as a side benefit removes the ORM's last remaining dependency on django.contrib.

The one possible concern is performance. This will be slower for M2Ms, as m2m intermediaries are now instantiated and added to the _collect_sub_objects collector; the question is how much slower. I'm seeing about a 1% increase in overall test suite runtime, but what is really needed is a profiled benchmark that does a lot of deletions with various related objects.

One possible low-hanging fruit would be to check whether an m2m intermediary model is auto-created, and if so just directly add it to seen_objs instead of calling _collect_sub_objects on it (as an auto-created m2m model will not have other FKs pointing to it). Not sure how much this will help. It may be that what's really needed is a more thorough rethinking of _collect_sub_objects for better efficiency; even the existing _collect_sub_objects code was less than ideal in that regard.

Feedback welcome.

comment:5 by Carl Meyer, 14 years ago

Oh, also: the attached patch passes the full test suite on both SQLite and Postgres. Running the tests on MySQL ISAM is crazy slow, I'll try to get to it tonight.

comment:6 by anonymous, 14 years ago

Cc: miracle2k added

by Carl Meyer, 14 years ago

Attachment: 12953_r12756.diff added

patch with additional test for odd M2M case

comment:7 by Carl Meyer, 14 years ago

Added a test for the case of an M2M with explicit "through" where there are FKs directly to the through model.

comment:8 by Russell Keith-Magee, 14 years ago

Resolution: fixed
Status: newclosed

(In [12790]) Fixed #12953 -- Ensure that deletion cascades through generic relations. Also cleans up the special-casing of generic relations in the deleted object discovery process. Thanks to carljm for the report and patch.

comment:9 by Russell Keith-Magee, 14 years ago

This issue still exists in 1.1. However, backporting the fix to 1.1 is non trivial -- the generic approach to solving the problem is only possible because of the m2m refactor introduced in 1.2.

comment:10 by Jacob, 12 years ago

milestone: 1.2

Milestone 1.2 deleted

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