`pre_delete` and `post_delete` signals are not correctly sent in inheritance scenarios involving proxy models

[Simon Charette]

This is a follow up of #18083.

Given the following models and signals (as of r17887):

from django.db import models
from django.db.models import signals


class A(models.Model):
    pass

class AProxy(A):
    class Meta:
        proxy = True

class B(A):
    pass

class BFirstProxy(B):
    class Meta:
        proxy = True

class BSecondProxy(B):
    class Meta:
        proxy = True

class BFirstProxyFirstProxy(BFirstProxy):
    class Meta:
        proxy = True

class BFirstProxySecondProxy(BFirstProxy):
    class Meta:
        proxy = True


def pre_delete(sender, instance, **kwargs):
    print "`pre_delete` %r %r" % (sender, instance)

signals.pre_delete.connect(pre_delete, A)
signals.pre_delete.connect(pre_delete, AProxy)
signals.pre_delete.connect(pre_delete, B)
signals.pre_delete.connect(pre_delete, BFirstProxy)
signals.pre_delete.connect(pre_delete, BSecondProxy)
signals.pre_delete.connect(pre_delete, BFirstProxyFirstProxy)
signals.pre_delete.connect(pre_delete, BFirstProxySecondProxy)

def post_delete(sender, instance, **kwargs):
    return # Avoid dispatching for output clarity
    print "`post_delete` %r %r" % (sender, instance)

signals.post_delete.connect(post_delete, A)
signals.post_delete.connect(post_delete, AProxy)
signals.post_delete.connect(post_delete, B)
signals.post_delete.connect(post_delete, BFirstProxy)
signals.post_delete.connect(post_delete, BSecondProxy)
signals.post_delete.connect(post_delete, BFirstProxyFirstProxy)
signals.post_delete.connect(post_delete, BFirstProxySecondProxy)

pre_delete and post_delete signals dispatching depends on the deletion source and don't propagate up the proxy chain nor to the proxy leaves. i.e. (here post_delete's dispatching should behave the same)

Basic model proxy

Actual behaviours

>>> A.objects.create().delete()
`pre_delete` <class 'proxy_signals.models.A'> <A: A object>
>>> AProxy.objects.create().delete()
`pre_delete` <class 'proxy_signals.models.AProxy'> <AProxy: AProxy object>

Expected behaviour in both cases

`pre_delete` <class 'proxy_signals.models.AProxy'> <AProxy: AProxy object>
`pre_delete` <class 'proxy_signals.models.A'> <A: A object>

Multi-table inheritance model proxy

Actual behaviours

>>> B.objects.create().delete()
`pre_delete` <class 'proxy_signals.models.B'> <B: B object>
`pre_delete` <class 'proxy_signals.models.A'> <A: A object>
>>> BFirstProxy.objects.create().delete()
`pre_delete` <class 'proxy_signals.models.BFirstProxy'> <BFirstProxy: BFirstProxy object>
`pre_delete` <class 'proxy_signals.models.A'> <A: A object>
>>> BSecondProxy.objects.create().delete()
`pre_delete` <class 'proxy_signals.models.BSecondProxy'> <BSecondProxy: BSecondProxy object>
`pre_delete` <class 'proxy_signals.models.A'> <A: A object>
>>> BFirstProxyFirstProxy.objects.create().delete()
`pre_delete` <class 'proxy_signals.models.BFirstProxyFirstProxy'> <BFirstProxyFirstProxy: BFirstProxyFirstProxy object>
`pre_delete` <class 'proxy_signals.models.A'> <A: A object>
>>> BFirstProxySecondProxy.objects.create().delete()
`pre_delete` <class 'proxy_signals.models.BFirstProxySecondProxy'> <BFirstProxySecondProxy: BFirstProxySecondProxy object>
`pre_delete` <class 'proxy_signals.models.A'> <A: A object>

Expected behaviour in all cases

`pre_delete` <class 'proxy_signals.models.BFirstProxyFirstProxy'> <BFirstProxyFirstProxy: BFirstProxyFirstProxy object>
`pre_delete` <class 'proxy_signals.models.BFirstProxySecondProxy'> <BFirstProxySecondProxy: BFirstProxySecondProxy object>
`pre_delete` <class 'proxy_signals.models.BFirstProxy'> <BFirstProxy: BFirstProxy object>
`pre_delete` <class 'proxy_signals.models.BSecondProxy'> <BSecondProxy: BSecondProxy object>
`pre_delete` <class 'proxy_signals.models.B'> <B: B object>
`pre_delete` <class 'proxy_signals.models.AProxy'> <AProxy: AProxy object>
`pre_delete` <class 'proxy_signals.models.A'> <A: A object>

What needs to be fixed:

• Dispatch the concrete_model signals even when the source is an instance of a proxy
• Dispatch the signals for all proxy of concrete models involved in the deletion
• Preserve the expected dispatching order

Failing integrated TestCase to come.

[Simon Charette]

Added a test suite integrated TestCase that highlights the issue.

[Anssi Kääriäinen]

A small warning: the signal firing for proxy models can be a bit weird. I believe that if you save a proxy-model no signal is fired for the proxy model, just for the first base model and then again not for any of the parents of that model. I might be wrong here, it has been a while since when I last dealt with model.save_base().

My main point here is that signal firing should be as consistent as possible. So, investigating that .save() behaves in somewhat consistent way to .delete() should be done.

[Simon Charette]

@akaariai, actually the signal is only fired for the model class of the instance that triggered the save, proxy or not.

The save signals dispatching logic should be unified according to this one, but lets keep that for another ticket. Should we?

[Anssi Kääriäinen]

You are correct about the .save() signal.

I am fine with unifying the dispatching logic in another ticket. However, we need to know what the unified behavior is. Just changing the signal firing of .save() to match this one needs to be considered from backwards compatibility point. And this one needs to be considered from backwards compatibility point, too.

One approach is to make inheritable signals: You connect to pre/post_delete for class A with something like:

signals.pre_delete.connect(pre_delete, sender=A, inherit=True)

and when BFirstProxySecondProxy sends its pre/post_delete signals A's pre_delete hears it. This should be backwards compatible.

I just want to make sure it is possible to make the signal firing consistent between save and delete signals. I am actually pretty sure that changing pre/post save to fire for each parent model will break user code, and this break can be data-corrupting, as those signals can be used to do auditing for example. Sending the signals for each parent model is maybe correct, but can we change that now?

[Anssi Kääriäinen]

One more point: I don't believe the signals should be propagated to the proxy-leaf direction. To the other way, sure. But you should be able to define signals for proxy models which are specifically not sent when the parent model is deleted. Subclasses specialize behavior, and you can't do that for signals if they are always fired.

What is the use case for reverse-propagation which is not better served by attaching directly to the concrete parent .delete()?

The current behavior of model signals is:

• save: just for the actual saved model
• init: just for the actual initialized model
• delete: for the actual deleted model + all its parent models in the multitable inheritance chain.

So, unifying those without breaking backwards compatibility for any of them seems a bit hard. If at all possible it should be done, the question is if it is possible?

[Carl Meyer]

Just to toss in my several cents:

• I agree with akaariai that signals for a child proxy should definitely not be sent when a parent proxy is deleted.
• It initially seemed wrong to me to send a separate signal for each proxy-equivalent model class, but I was swayed by the argument of internal consistency with non-proxy (concrete) parents, where a separate signal is sent for each concrete parent, and that fact that the common idiom for attaching to model signals doesn't attach to signals sent by "this class or any subclasses," which means that e.g. deleting a proxy User will not execute signal handlers people have attached to User deletion, even though the User is in fact deleted. This seems like a serious problem, and adding an inherit=True to handler connection doesn't solve it, because it places the onus on the wrong person; the signal author, not the proxy User author. Somebody with a reusable app that attaches a deletion handler to User ought to be able to presume that it will get called when a User is deleted, and they shouldn't need to explicitly consider the case of proxy models and add inherit=True in order to make that assumption true.
• I'm not sure about consistency with save. I think the practical back-compat consequences of firing more signals for the different classes is probably minimal, since the common pattern is to use sender=Foo, which will still catch only one of the several signals (though it's true that some auditing schemes might attach to pre_save/post_save indiscriminately and would then get duplicated data). In any case, I'm more concerned about getting sensible internal consistency within deletion signals than about consistent behavior between various ORM methods (though I agree that is also desirable, if we can make it happen).

[Simon Charette]

I think I overlooked the complexity of this issue.

In lights of your comments I also agree that sending signals the proxy-leaf direction is not desirable.

However I still beleive signals should be sent for all subclasses of the source including proxy ones (for the reason invoked in @carljm's second point). This can be achieved using the newly redefined proxy_for_model model option.

i.e.

>>> A.objects.create().delete()
`pre_delete` <class 'proxy_signals.models.A'> <A: A object>

>>> AProxy.objects.create().delete()
`pre_delete` <class 'proxy_signals.models.AProxy'> <AProxy: AProxy object>
`pre_delete` <class 'proxy_signals.models.A'> <A: A object>

>>> B.objects.create().delete()
`pre_delete` <class 'proxy_signals.models.B'> <B: B object>
`pre_delete` <class 'proxy_signals.models.A'> <A: A object>

>>> BFirstProxy.objects.create().delete()
`pre_delete` <class 'proxy_signals.models.BFirstProxy'> <BFirstProxy: BFirstProxy object>
`pre_delete` <class 'proxy_signals.models.B'> <B: B object>
`pre_delete` <class 'proxy_signals.models.A'> <A: A object>

>>> BSecondProxy.objects.create().delete()
`pre_delete` <class 'proxy_signals.models.BSecondProxy'> <BSecondProxy: BSecondProxy object>
`pre_delete` <class 'proxy_signals.models.B'> <B: B object>
`pre_delete` <class 'proxy_signals.models.A'> <A: A object>

>>> BFirstProxyFirstProxy.objects.create().delete()
`pre_delete` <class 'proxy_signals.models.BFirstProxyFirstProxy'> <BFirstProxyFirstProxy: BFirstProxyFirstProxy object>
`pre_delete` <class 'proxy_signals.models.BFirstProxy'> <BFirstProxy: BFirstProxy object>
`pre_delete` <class 'proxy_signals.models.B'> <B: B object>
`pre_delete` <class 'proxy_signals.models.A'> <A: A object>

>>> BFirstProxySecondProxy.objects.create().delete()
`pre_delete` <class 'proxy_signals.models.BFirstProxySecondProxy'> <BFirstProxySecondProxy: BFirstProxySecondProxy object>
`pre_delete` <class 'proxy_signals.models.BFirstProxy'> <BFirstProxy: BFirstProxy object>
`pre_delete` <class 'proxy_signals.models.B'> <B: B object>
`pre_delete` <class 'proxy_signals.models.A'> <A: A object>

While playing with this I also noticed quite a nasty existing data-loss prone behaviour when mixing only or defer with deletion signals:

>>> from django.contrib.auth.models import User
>>> from django.db import models
>>> def user_post_delete(sender, instance, **kwargs):
...     print "User post delete sent"
...
>>> User.objects.create().delete()
User post delete sent 
>>> u = User.objects.create()
>>> User.objects.only('id').get(pk=u.pk).delete()

This happens because ònly and defer create proxies under the hood. I think this issue can be fixed more easily checking for _deferred and I'll open another ticket for that.

[Anssi Kääriäinen]

Making init signals consistent isn't likely for performance reasons - in fact it might make sense to replace them with init hooks. That would fit the use case of the signals, and currently the init signals can make model init around 2x slower in the not so uncommon case that model init signals are defined for some other model in the project - not necessarily the one being initialized.

Altering save signals to fire for each parent class (proxy or not) would make sense. Assuming Foo and FooBar(Foo) models, it is more than likely that if you have a signal for Foo that then you have attached the same signal for FooBar too. Because you want to catch "Foo saved" when FooBar is saved - the exact issue we are trying to fix here. So, fixing the signals in the way suggested in this ticket removes the need of attaching the same signal multiple times but at same time is going to break existing usage. For how many users is a different question.

I am not sure if the above backwards compatibility issue is meaningful for delete signals, so if consistency is not possible then fixing just delete signals makes sense.

All in all, maybe the consistency issue should be discussed at django-developers?

[Carl Meyer]

@charettes - I think your summary of what signals ought to be sent here is correct. Re the only/defer issue - if I'm not mistaken, that would be fixed by the fix we are discussing here, correct? Given that, I'm not sure it deserves it's own ticket, and I don't think we should put in place a special-case fix for it.

@akaariai - Yeah, I think the question of consistency with save signals and backwards compatibility is probably worth a thread on -developers - I'll try to find time to post about it today.

[Carl Meyer]

I started a mailing list thread (​https://groups.google.com/d/msg/django-developers/2aDFeNAXJgI/-riTuIgNLPQJ) and jdunck pointed out that #9318 already exists for this, though that ticket is geared towards a fix within the signals framework itself. I'm closing this ticket as a duplicate of that one, as it's the same problem, regardless of which way we fix it.

[Jeremy Dunck]

I've started a branch to work on this problem:
​https://github.com/votizen/django/tree/virtual_signals