#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)
Change History (13)
by , 15 years ago
Attachment: | 12953_r12557.diff added |
---|
comment:1 by , 15 years ago
Component: | Uncategorized → Database layer (models, ORM) |
---|---|
milestone: | → 1.2 |
Version: | 1.1 → SVN |
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 , 15 years ago
Triage Stage: | Unreviewed → Accepted |
---|
comment:3 by , 15 years ago
Owner: | changed from | to
---|
I have a fix working for this, also removes the contenttypes dependency; just need to update it for the #6191 fix.
comment:4 by , 15 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 , 15 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 , 15 years ago
Cc: | added |
---|
comment:7 by , 15 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 , 15 years ago
Resolution: | → fixed |
---|---|
Status: | new → closed |
comment:9 by , 15 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.
test-case only