Code

Opened 6 years ago

Closed 3 years ago

#8077 closed (wontfix)

Add ability to disable signals one a per-object-per-event basis.

Reported by: brooks.travis@… Owned by: nobody
Component: Core (Other) Version: master
Severity: Keywords: signals
Cc: brooks.travis@…, michel@… Triage Stage: Design decision needed
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: UI/UX:

Description

Right now, built-in signals like pre_save and post_save are triggered for all models every time a new or existing instance is saved to the database. I would propose adding the capability to granularly disable any built-in signals on a per-event basis. Here's an example syntax:

>>>MyModel(name='John Doe', disabled_signals=['post_save'])

You should be able to pass this argument to a ModelForm instance, as well, and you could probably make it work for custom signals, but that's probably less necessary.

Adding the capability would make working with the build-in signals a lot less of a headache, particularly in my use-case, where a modification to one model with a signal listener attached, triggers a change to another model with a signal listener attached, but I don't want latter listener triggered every time the former is. Let me know if I'm not being clear.

Attachments (3)

send_signals_false_patch.diff (3.2 KB) - added by brooks.travis@… 6 years ago.
Patch against trunk using the send_signals=False method.
send_signals_false_patch_correct.diff (3.3 KB) - added by brooks.travis@… 6 years ago.
Patch against trunk using the send_signals=False method.
signals_patch3.diff (2.7 KB) - added by brooks.travis@… 6 years ago.
A new patch against trunk (8964) using the send_signals=False method.

Download all attachments as: .zip

Change History (17)

comment:1 Changed 6 years ago by mir

  • milestone set to post-1.0
  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset

comment:2 Changed 6 years ago by mir

  • Triage Stage changed from Unreviewed to Design decision needed

comment:3 Changed 6 years ago by anonymous

Alternative api implementation, since I would imagine the most common use-case would be to suppress all signals for a given object:

>>>MyModel(name='John Doe', send_signals=False)

Then, some conditionals in the base Model definitions could just test for the send_signals kwarg (True by default) before running dispatcher.send.

Changed 6 years ago by brooks.travis@…

Patch against trunk using the send_signals=False method.

Changed 6 years ago by brooks.travis@…

Patch against trunk using the send_signals=False method.

comment:4 Changed 6 years ago by brooks.travis@…

  • Has patch set

The send_signals_false_patch_correct.diff is the one that is formatted properly. If an admin could delete the first patch that would be great! Thanks.

comment:5 Changed 6 years ago by anonymous

Correction to api for proposed patch:

Don't send pre/post_init signals:

>>>a = MyModel(name='John Doe', send_signals=False)

Don't send pre/post_save signals:

>>>a = MyModel(name='John Doe')
>>>a.save(send_signals=False)

Changed 6 years ago by brooks.travis@…

A new patch against trunk (8964) using the send_signals=False method.

comment:6 Changed 5 years ago by anonymous

  • milestone post-1.0 deleted

Milestone post-1.0 deleted

comment:7 Changed 5 years ago by mtredinnick

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

comment:8 Changed 5 years ago by michelts

  • Cc michel@… added

comment:9 Changed 4 years ago by oliland

Has there been any update on this? This gets a +1 from me, but I see it hasn't been updated in 11 months.

comment:10 Changed 4 years ago by lukeplant

It's difficult to assess whether this is really needed without a concrete example. It could well be an inappropriate use of signals in the first place — if the signal should only sometimes be run, should it really ever be run via a signal?

comment:11 Changed 3 years ago by davedash

Can't your signal check for a flag that you set on your model instance and run accordingly?

def myhandler(sender, instance, kw):

if instance.skip_post_save:

return

comment:12 Changed 3 years ago by EmilStenstrom

lukeplant: Here's a realworld example:

I have a two models, dialog and comments, where the comment have a foreignkey to the dialog. To speed up the display of number of comments I have a denormalized field on dialog, called num_comments. Now, this needs to be updated whenever a comment is added or deleted, so I listen to post_save and post_delete:

# sender=Comment
def recalc_comments(sender, instance, created=False, **kwargs):
    dialog = instance.dialog
    dialog.num_comments = sender.public.filter(dialog=dialog).count()
    dialog.save()

But this means that save will be run twice, since save() inside the signal triggers the signal too. Adding disable_signals to the save inside the signal would solve the problem I think.

Please let me know if this is not the right way to do this kind of thing. Thanks.

comment:13 Changed 3 years ago by EmilStenstrom

No, sorry: Saving a dialog does OF COURSE not trigger the comment signal. It does only trigger the Dialog signal. Which still is something one might want to prevent, but which is not part of my "realworld example".

comment:14 Changed 3 years ago by lukeplant

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

Given the kind of examples given so far, I think there is a misunderstanding about the purpose of signals.

As the documentation says, signals are to help decoupled applications know about what is going on elsewhere in the framework. As the person writing MyModel(params) or my_model.save(), you don't know what signals are attached to the model, so you don't know whether it is appropriate to turn them off. The proposal is therefore not granular enough - you really need to disable a single handler that is attached via signals, not all of them.

On the other hand, if you are creating instances of the model, you are more like to be the person in charge of the model class, and you can always override __init__() and save() to do some custom work, and you can even pass custom parameters that will control whether the work needs to be done or not.

In short, it sounds like, given the defined purpose of signals, it is the attached signal handler that needs to become more intelligent (like in davedash's suggestion), rather than the code that emits the signal. Disabling signals is just a quick fix that will work when you know exactly what handlers are attached to a signal, and it hides the underlying problem by putting the fix in the wrong place.

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.