Code

Opened 3 years ago

Closed 18 months ago

#16131 closed Bug (fixed)

When deleting a model instance, deletion signals for its relations are fired after deleting the instance

Reported by: brodie 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 3 years ago.

Download all attachments as: .zip

Change History (8)

Changed 3 years ago by brodie

comment:1 Changed 3 years ago by brodie

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset

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

comment:2 Changed 3 years ago by brodie

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

  • Component changed from Database layer (models, ORM) to Documentation
  • Triage Stage changed from Unreviewed to 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:4 Changed 2 years ago by aaugustin

  • UI/UX unset

Change UI/UX from NULL to False.

comment:5 Changed 18 months ago by aaugustin

  • Component changed from Documentation to 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 Changed 18 months ago by aaugustin

  • Component changed from Core (URLs) to Database layer (models, ORM)

comment:7 Changed 18 months ago by Alex

  • Resolution set to fixed
  • Status changed from new to closed

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as closed
as The resolution will be set. Next status will be 'closed'
The resolution will be deleted. Next status will be 'new'
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.