Code

Opened 6 years ago

Last modified 16 months ago

#6870 new Bug

pre_delete should be sent before collecting ForeignKey relationships

Reported by: wkornewald Owned by: nobody
Component: Database layer (models, ORM) Version: master
Severity: Normal Keywords: pre_delete signals related
Cc: cgieringer, chrischambers, code@…, jakerothenbuhler@…, jashugan Triage Stage: Accepted
Has patch: yes Needs documentation: yes
Needs tests: no Patch needs improvement: yes
Easy pickings: no UI/UX: no

Description

If you have:

class MyModel(Model):
    user = ForeignKey(User, related_name='my_models', null=True)

You can't use a pre_delete signal to set user.my_models.clear() because the check for the related models is done before the signal is sent. It would be great if that were done after sending the signal.

Attachments (2)

6870.pre_delete. r13248.diff (5.2 KB) - added by cgieringer 4 years ago.
Adds a pre_signal argument to _collect_sub_objects to allow the notification of listeners to pre_deletes before related objects are collected.
6870.pre_delete.r13248.diff (8.3 KB) - added by cgieringer 4 years ago.
Removed "".format to make Python 2.4 compliant

Download all attachments as: .zip

Change History (17)

comment:1 Changed 6 years ago by ubernostrum

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

This feels like a symptom of a different problem, namely the inability to emulate anything other than ON DELETE CASCADE; if we had that you wouldn't be trying to use signals to do this.

comment:2 Changed 6 years ago by ericholscher

  • Component changed from Uncategorized to Core framework
  • Triage Stage changed from Unreviewed to Design decision needed

comment:3 Changed 5 years ago by semenov

While it actually is a symptom of a different (yet unsolved!) problem, the original suggestion makes sense per se. It shouldn't be disregarded just because that will magically allow to do some (useful) unrelated things, right?

comment:4 Changed 4 years ago by rudi

It seems, there's alternative approach for such usecase #12899, which I belive, is more consistent.

Changed 4 years ago by cgieringer

Adds a pre_signal argument to _collect_sub_objects to allow the notification of listeners to pre_deletes before related objects are collected.

comment:5 Changed 4 years ago by cgieringer

  • Cc cgieringer added
  • Has patch set
  • Needs documentation set
  • Needs tests set

I agree with the ticket creator that django should send the pre_delete signal at a time when it is still possible to modify the model in a way that affects deletion. It seems that the contract of all pre_xx signal should include informing listeners before any undoable changes have occurred in the deletion, save, etc.

The most common usage for a correctly behaving pre_delete signal is apparently to remove related objects to prevent cascade deletion, and the feature requested in #7539 would eventually provide another means to that usage. But that possible feature doesn't change the fact that pre_delete breaks its contract (or what it seems its contract should be) by collecting the related objects for deletion before informing listeners that the objects will be deleted, thereby offering no consistent way to prevent cascade deletion. The only way to get around this limitation is to define both a custom model delete and manager delete for each model requiring this functionality, which is a tedious and error-prone requirement.

Currently the pre_delete signal is sent from django.db.models.query.delete_objects. The least-complicated place to send the signal while meeting its contract is from the beginning of _collect_sub_objects, because this is both the first place that related objects are seen (as that method traverses relations from the original model) and the last place that a change to a model's related objects can be made before they are cached in seen_objects and then sent for deletion.

I have attached a patch which adds a pre_signal argument to django.db.model.base.Model's _collect_sub_objects. The argument should be one of Django's signals from django.db.models.signals, and it will be called at the beginning of the method if the model was not autocreated. The argument is called pre_signal because although _collect_sub_objects is presently only used for finding related objects for deletion, it could possibly be used for some other purpose for which a different pre_xx signal should be sent. The patch adds _collect_sub_objects(..., pre_signal=pre_signal) to recursions and _collect_sub_objects(..., pre_signal=pre_delete) to the two places the method is called. It removes the sending of pre_delete from delete_objects.

Thank you to all the Django developers, this framework is my favorite piece of software.

comment:6 Changed 4 years ago by cgieringer

  • Keywords pre_delete signals related added
  • Needs tests unset

I added a test that deletes a model and asserts that another model with a foreign key to it had the opportunity to be rescued from deletion via a pre_delete signal

Changed 4 years ago by cgieringer

Removed "".format to make Python 2.4 compliant

comment:7 Changed 4 years ago by chrischambers

  • Cc chrischambers added

comment:8 Changed 4 years ago by carljm

  • Patch needs improvement set
  • Triage Stage changed from Design decision needed to Accepted

Now that #7539 is fixed, the use-case scope here is reduced. Nevertheless, moving the signal to fire before related objects are collected is not particularly difficult, seems reasonable, and increases the usefulness of the signal. In my preliminary test, doing so causes only one test failure, and that is spurious (because pre_delete signal is being used to test the order of actual deletion).

The only concern I have here is the subtle backwards-incompatibility reflected by that test failure: pre-delete signals will now fire in a different order than previously. OTOH, the documentation makes no guarantees about order of pre_delete signals firing, and if anything the order after this change is more logical (it would now be the order of collection).

The patch will look quite different now that #7539 has landed. Here's an initial take on it, but I haven't yet resolved the failing deletion-order test: https://github.com/carljm/django/compare/master...t6870

comment:9 Changed 3 years ago by julien

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

See a possibly related feature request in #13251.

comment:10 Changed 3 years ago by koto

  • Cc code@… added

comment:11 Changed 3 years ago by julien

  • Severity set to Normal
  • Type set to Bug

comment:12 Changed 3 years ago by jrothenbuhler

  • Cc jakerothenbuhler@… added
  • Easy pickings unset
  • UI/UX unset

comment:13 Changed 3 years ago by jashugan

  • Cc jashugan added

The original use-case still exists for generic relations, which does not support the on_delete functionality given by #7593.

comment:14 Changed 3 years ago by ambv

There are more use-cases than that. One of those is Admin's object deletion. Let me give you an example:

I have a Team model and a Member model. Teams have Members. Trivial stuff. I also log every change on a Team or its Members by creating LogEntry objects. So, when a Member is deleted, I have a pre_delete signal that saves the LogEntry ("member parted") for the related Team. Now, when I try to delete a Team from the Admin, during deletion of existing Members, log entries are created by the pre_delete signal and the transaction fails.

Currently as a workaround you have to either let orphaned objects litter the DB by using on_delete or to create another pre_delete signal for the Team model which does instance.logentry_set.all().delete().

comment:15 Changed 16 months ago by anonymous

I've run in the same problem, not easy to debug. I'd be great to see it fixed someday.

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as new
The owner will be changed from nobody to anonymous. Next status will be 'assigned'
as The resolution will be set. Next status will be 'closed'
Author


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

 
Note: See TracTickets for help on using tickets.