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

[Carl Meyer]

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.

[Carl Meyer]

test-case only

[Carl Meyer]

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.

[Carl Meyer]

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

[Carl Meyer]

possible fix

[Carl Meyer]

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.

[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.

[Carl Meyer]

patch with additional test for odd M2M case

[Carl Meyer]

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

[Russell Keith-Magee]

(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.

[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.

[Jacob]

Milestone 1.2 deleted