Code

Opened 6 years ago

Last modified 2 years ago

#8399 new New feature

loaddata management command should provide option to disable signals

Reported by: chrisdpratt@… Owned by: nobody
Component: Core (Management commands) Version: master
Severity: Normal Keywords:
Cc: albrecht.andi@…, anssi.kaariainen@… Triage Stage: Accepted
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

Given a scenario such as a signal being called post_save to send a welcome email to a new user, the loaddata management command attempts to call this signal resulting in, at best but still no good, an email being sent to the user, or at worst, loaddata failure. Obviously, with as many times as one is likely to need to resync and reload data during development, it's not ideal to have a welcome email sent on every iteration.

Ideally, I'd like to see an argument that could be passed to loaddata to disable signal processing. Something along the lines of: "python manage.py loaddata mydata.json --nosignals". This method seems best, since potentially, there might be circumstances where you would want the signals processed. It would perhaps be even better if you could somehow pass a list of signals to disable, so you could selectively still have some signals processed.

As it stands, the issue can be solved easily enough by creating a wrapper around manage.py that disabled the necessary signals and then called loaddata, but this feel clunky.

As I have time, I will try to come up with a patch myself, if no one beats me to it. But I'm new to this kind of thing, so it may take me a little bit.

Attachments (0)

Change History (20)

comment:1 Changed 6 years ago by mtredinnick

  • milestone set to post-1.0
  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Triage Stage changed from Unreviewed to Accepted

This solution is a bit too heavy-handed. Disabling all signal handling just because some of your signal methods don't need to be run in that situation is wrong. There will be plenty of cases where loading data should trigger other behaviour that must be run whether the data is coming in through loaddata or otherwise (e.g. creating a related object, such as a user profile).

Your problem has some legitimacy, but your proposed solution is throwing out the baby with the bathwater.

comment:2 Changed 5 years ago by anonymous

  • milestone post-1.0 deleted

Milestone post-1.0 deleted

comment:3 Changed 5 years ago by bpeschier

Maybe some global information about the current thread is more granular? I have used the sys-lib to work out whether or not the process was started by a manage.py command. This works, but it kinda feeled as dirty as disabling signals. It was possible with this solution however to disable some signal handlers with a simple 'if not LOADDATA' in front of the method.

comment:4 follow-up: Changed 5 years ago by gsong

How about just a simple decorator for signal_handlers?

Something like:

try:
    from functools import wraps
except ImportError:
    from django.utils.functional import wraps

import inspect


def noop_handler(*args, **kwargs):
    pass


def disable_for_loaddata(signal_handler):
    for fr in inspect.stack():
        if inspect.getmodulename(fr[1]) == 'django-admin':
            return wraps(signal_handler)(noop_handler)
    return signal_handler

comment:5 in reply to: ↑ 4 ; follow-up: Changed 5 years ago by gsong

Um. The above code won't work of course as that's evaluated when django-admin fires up.

The code should be like this:

try:
    from functools import wraps
except ImportError:
    from django.utils.functional import wraps

import inspect


def noop_handler():
    pass


def disable_for_loaddata(signal_handler):
    @wraps(signal_handler)
    def wrapper(*args, **kwargs):
        for fr in inspect.stack():
            if inspect.getmodulename(fr[1]) == 'loaddata':
                noop_handler()
        signal_handler(*args, **kwargs)
    return wrapper

comment:6 in reply to: ↑ 5 ; follow-up: Changed 5 years ago by gsong

More simplification. We obviously don't need the noop_handler().

try:
    from functools import wraps
except ImportError:
    from django.utils.functional import wraps

import inspect


def disable_for_loaddata(signal_handler):
    @wraps(signal_handler)
    def wrapper(*args, **kwargs):
        for fr in inspect.stack():
            if inspect.getmodulename(fr[1]) == 'loaddata':
                pass
        signal_handler(*args, **kwargs)
    return wrapper

comment:7 in reply to: ↑ 6 Changed 5 years ago by gsong

Found a bug in my code, should be return instead of pass for no-op.

try:
    from functools import wraps
except ImportError:
    from django.utils.functional import wraps

import inspect


def disable_for_loaddata(signal_handler):
    @wraps(signal_handler)
    def wrapper(*args, **kwargs):
        for fr in inspect.stack():
            if inspect.getmodulename(fr[1]) == 'loaddata':
                return
        signal_handler(*args, **kwargs)
    return wrapper

comment:8 Changed 3 years ago by lukeplant

  • Severity set to Normal
  • Type set to New feature

comment:9 Changed 3 years ago by vanschelven

  • Easy pickings unset
  • UI/UX unset

This appears to be closely related to #12610

comment:10 Changed 2 years ago by aalbrecht

  • Cc albrecht.andi@… added

comment:11 Changed 2 years ago by akaariai

If I am not mistaken there is a "raw" argument to the pre/post save signals. If that is set, the signal came from loaddata. Thus, if you check that argument, you will not need this ticket's added behavior. Are there other convincing usecases for this feature? If not, maybe this should be closed as wontfix?

comment:12 Changed 2 years ago by aalbrecht

I came across this issue when I tried to load example data for development and the fixtures contained data of a installed third-party apps. So I'm not able to look for the "raw" keyword since the signal handler is in that third-party app.

comment:13 Changed 2 years ago by akaariai

  • Cc anssi.kaariainen@… added

Maybe that should be considered a bug in the app?

I do understand the wish for disabling all signals in loaddata. However, as said before in this ticket, there are some signals you should not disable. For example, pre/post_init signals can do some additional instance initialization setup, and those signals should fire in loaddata setup, too. So, unconditional "kill all signals" doesn't sound right to me. If there is no unconditional signal killer, what then?

My feeling is what we have now (a way for listeners to know when a signal was fired under loaddata) is the best we can do. Good ideas welcome.

comment:14 Changed 2 years ago by aalbrecht

I totally agree. Disabling all signals sounds wrong and may have bad side-effects. And after adding me on CC to track this issue I've written a simple loaddata wrapper command that selectively disables only that signal handlers that caused the troubles.

IMO it's a bug in the app in the way that the app doesn't deal with the fact that data for the app's models *could* be loaded via fixtures - IOW the app should decide which signals could be disabled when fixtures are loaded and which not.

comment:15 follow-up: Changed 2 years ago by akaariai

I was actually thinking of providing an API for signal filtering. But then again, that seems pretty dangerous if used carelessly (concurrency issues lurking here).

So, what do you think? Should this ticket be closed as wontfix, or should we wait if there will be some new usecases or ideas for dealing with this issue? I am thinking of wontfixing this. If somebody comes up with a good idea for this, a new ticket or reopen is always possible...

comment:16 Changed 2 years ago by aalbrecht

For reference, this is the bug from the apps perspective: https://github.com/divio/django-cms/issues/1031
I'll leave a note there about the raw keyword.

comment:17 in reply to: ↑ 15 Changed 2 years ago by carljm

Replying to akaariai:

I was actually thinking of providing an API for signal filtering. But then again, that seems pretty dangerous if used carelessly (concurrency issues lurking here).

So, what do you think? Should this ticket be closed as wontfix, or should we wait if there will be some new usecases or ideas for dealing with this issue? I am thinking of wontfixing this. If somebody comes up with a good idea for this, a new ticket or reopen is always possible...

I don't think this should be closed wontfix. At the very least, if we can't figure out any appropriate code changes (and I haven't fully reviewed the ticket yet), I think some documentation additions may be in order.

comment:18 follow-up: Changed 2 years ago by akaariai

In the github ticket mentioned above a pre_loaddata signal was suggested.

That might actually be a really good idea, although it could be a more general "pre/post_management_command" signal, and the actual command together with passed arguments could be provided as signal arguments. The signal could be handy for other issues, too. And you could solve this ticket's problem by just doing pre_save.disconnect(wanted_signal) in the handler. Should I open a ticket for this idea (assuming there isn't one already)?

comment:19 in reply to: ↑ 18 Changed 2 years ago by carljm

Replying to akaariai:

In the github ticket mentioned above a pre_loaddata signal was suggested.

That might actually be a really good idea, although it could be a more general "pre/post_management_command" signal, and the actual command together with passed arguments could be provided as signal arguments. The signal could be handy for other issues, too. And you could solve this ticket's problem by just doing pre_save.disconnect(wanted_signal) in the handler. Should I open a ticket for this idea (assuming there isn't one already)?

This sounds pretty good to me; I think it'd be worth a thread on django-developers to discuss whether it should just be loaddata, or generalized to all management commands. Personally I can't see much reason not to do the latter; it's not like pre/post-management-command is a performance-sensitive spot, any additional overhead from firing a no-receivers signal (and I think there's very little in the current signals implementation) would be drowned out by interpreter startup anyway. That signal would immediately open up lots of interesting possibilities for things you currently have to do by overriding a management command (such as South's override of the "test" command), and overriding commands is fragile.

So I think I'm in favor, but I'd like to raise it on django-developers, and I probably won't do that until after 1.4 is out, since this would be a new feature and can't go in now anyway.

comment:20 Changed 2 years ago by julien

See also #13299 which suggests to document raw=True in the loaddata doc.

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.