loaddata management command should provide option to disable signals

[chrisdpratt@…]

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.

[Malcolm Tredinnick]

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.

[(none)]

Milestone post-1.0 deleted

[Bas Peschier]

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.

[George Song]

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

[George Song]

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

[George Song]

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

[George Song]

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

[Klaas van Schelven]

This appears to be closely related to #12610

[Anssi Kääriäinen]

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?

[Andi Albrecht]

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.

[Anssi Kääriäinen]

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.

[Andi Albrecht]

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.

[Anssi Kääriäinen]

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...

[Andi Albrecht]

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.

[Carl Meyer]

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.

[Anssi Kääriäinen]

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)?

[Carl Meyer]

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.

[Julien Phalip]

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

[Tim Graham]

Documentation about raw was added in #20136 and I don't see any other concrete actions steps for this ticket to move forward. Feel free to open a new ticket to clarify if something else is still needed.