Opened 13 years ago
Closed 12 years ago
#16131 closed Bug (fixed)
When deleting a model instance, deletion signals for its relations are fired after deleting the instance
Reported by: | Brodie Rao | Owned by: | nobody |
---|---|---|---|
Component: | Database layer (models, ORM) | Version: | 1.3 |
Severity: | Normal | Keywords: | delete signals |
Cc: | Triage Stage: | Accepted | |
Has patch: | yes | Needs documentation: | no |
Needs tests: | no | Patch needs improvement: | no |
Easy pickings: | no | UI/UX: | no |
Description
In Django 1.2, you could be sure that if you had a post_delete signal on a model with a ForeignKey relation, you would be able to access that relation in the signal handler. Even if code called .delete() on the related object, the signal would be fired before the related object was deleted from the database.
As of Django 1.3, if you call .delete() on that related object, the signal handler gets fired *after* the related object is deleted from the database. In other words, it deletes the object and its relations in the wrong order.
This regression was introduced by [14507]. It's still present on the releases/1.3.X branch as of [16296], and it's still present on trunk as of [16299].
I'm attaching a patch that adds regression tests for this behavior. The first test registers a post_delete signal handler for child object, deletes the related/parent object, and queries the database in the signal handler for the related object. The second test does the same, but its signal handler simply accesses the related object.
When applied to releases/1.2.X, both tests pass. When applied to releases/1.3.X, both tests fail. The first test fails to find it in the DB, and the second test encounters a DoesNotExist exception.
Also, this is somewhat unrelated, but when writing the tests I noticed that the objects received by the signal handler did not equal (with ==) the objects I instantiated in the test. The PKs were all the same, but ._get_pk_val() returned None for the objects passed to the signal handler. That might be worth another bug report.
Attachments (1)
Change History (8)
by , 13 years ago
Attachment: | signal-order-test.txt added |
---|
comment:1 by , 13 years ago
comment:2 by , 13 years ago
Thinking about this some more, maybe the new behavior is correct. You could argue that firing post_delete after an instance and all of its relations are deleted from the database is the more correct thing to do, and that if one needs to access a relation during deletion, pre_delete would be a better choice. Maybe this just deserves a mention in the release notes.
For a real world example, django-piston's oauth Consumer model has an FK on django.contrib.auth.models.User. When a consumer is deleted, a post_delete signal for it is fired. The signal sends an email to the user telling them that consumer has been canceled/removed. This works fine most of the time, except when the consumer is being deleted because the user is being deleted. Should it be able to send that email? Should it instead use pre_delete? What if the deletion fails after pre_delete's fired? It can't roll back sending the email.
comment:3 by , 13 years ago
Component: | Database layer (models, ORM) → Documentation |
---|---|
Triage Stage: | Unreviewed → Accepted |
Like you, I think it make sense to fire post_delete
after the object is actually deleted from the database. In your last example, you'd have to store the user's details in pre_delete
and actually send the email in post_delete
. Sure, that's a bit cumbersome.
However, introducing an undocumented backwards incompatibility is a problem: +1 for a mention in the release notes.
comment:5 by , 12 years ago
Component: | Documentation → Core (URLs) |
---|---|
Has patch: | set |
I'm not sure I still stand by what I said two years ago...
PR with a fix : https://github.com/django/django/pull/642
comment:6 by , 12 years ago
Component: | Core (URLs) → Database layer (models, ORM) |
---|
comment:7 by , 12 years ago
Resolution: | → fixed |
---|---|
Status: | new → closed |
Resolved with c346d358fdca2e375a0840664165a51a47f4ae69
Whoops, I messed up the attachment. It should really have .patch or .diff as the file extension.