Opened 7 years ago

Closed 7 years ago

Last modified 5 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: master
Severity: Keywords:
Cc: miracle2k Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: UI/UX:


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 7 years ago.
test-case only
12953_r12599.diff (9.7 KB) - added by Carl Meyer 7 years ago.
possible fix
12953_r12756.diff (11.5 KB) - added by Carl Meyer 7 years ago.
patch with additional test for odd M2M case

Download all attachments as: .zip

Change History (13)

Changed 7 years ago by Carl Meyer

Attachment: 12953_r12557.diff added

test-case only

comment:1 Changed 7 years ago by Carl Meyer

Component: UncategorizedDatabase layer (models, ORM)
milestone: 1.2
Needs documentation: unset
Needs tests: unset
Patch needs improvement: unset
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 Changed 7 years ago by Russell Keith-Magee

Triage Stage: UnreviewedAccepted

comment:3 Changed 7 years ago by Carl Meyer

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.

Changed 7 years ago by Carl Meyer

Attachment: 12953_r12599.diff added

possible fix

comment:4 Changed 7 years ago by Carl Meyer

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 Changed 7 years ago by Carl Meyer

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 Changed 7 years ago by anonymous

Cc: miracle2k added

Changed 7 years ago by Carl Meyer

Attachment: 12953_r12756.diff added

patch with additional test for odd M2M case

comment:7 Changed 7 years ago by Carl Meyer

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

comment:8 Changed 7 years ago by Russell Keith-Magee

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 Changed 7 years ago by Russell Keith-Magee

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 Changed 5 years ago by Jacob

milestone: 1.2

Milestone 1.2 deleted

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