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)

signal-order-test.txt (2.7 KB ) - added by Brodie Rao 13 years ago.

Download all attachments as: .zip

Change History (8)

by Brodie Rao, 13 years ago

Attachment: signal-order-test.txt added

comment:1 by Brodie Rao, 13 years ago

Whoops, I messed up the attachment. It should really have .patch or .diff as the file extension.

comment:2 by Brodie Rao, 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 Aymeric Augustin, 13 years ago

Component: Database layer (models, ORM)Documentation
Triage Stage: UnreviewedAccepted

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:4 by Aymeric Augustin, 13 years ago

UI/UX: unset

Change UI/UX from NULL to False.

comment:5 by Aymeric Augustin, 12 years ago

Component: DocumentationCore (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 Aymeric Augustin, 12 years ago

Component: Core (URLs)Database layer (models, ORM)

comment:7 by Alex Gaynor, 12 years ago

Resolution: fixed
Status: newclosed
Note: See TracTickets for help on using tickets.
Back to Top