Deleting model instances with deferred fields don't trigger deletion signals

[Simon Charette]

While working on a patch for #18094 I stumbled on this issue.

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

The patch should be a simple check for _deferred here.

[Simon Charette]

Added a patch with test. I cannot run against the full test suite because I'm getting the same failures as the ​ci server.

[Anssi Kääriäinen]

Deferred model signals is a general problem - not only for delete (see #16679 for example - warning: a good old brain-dump ticket). However, I think fixing it just for delete does make sense: other signals are not as likely to cause problems. Syncdb isn't valid, init is seldom used and if you save deferred models you are going to fetch all the fields one at a time from the DB, so you are in for an interesting ride anyways.

Still, .save() should be fixed with something similar. Restructuring the whole save_base() might make sense. It is a bit strangely set up currently and it is hard to see what exactly is happening in there. (#17341 for one approach).

[Carl Meyer]

Hmm, I'm still not convinced that this fix makes sense. The fact that deferred/only use a proxy model is something that can't be made transparent to the user (a simple check of the class of the returned model instances will reveal it), so I'm not sure there's any point to adding extra complexity to the code just to try to hide it in this one case. (I'm not sure that proxy models should have been used for deferred/only, but that's water under the bridge now.)

The real problem here is that receivers registered for the superclass currently won't run, but IMO the right fix for that is to fix #9318.

Contrary to akaariai, if we do decide to implement this special-case fix, I don't think there's any compelling reason not to fix it for all the model signals at once.

[Simon Charette]

Even if #9318 is fixed we wan't to avoid dispatching signals for the DeferredFieldProxy for performance reasons.

Anyway what's the use case of dispatching it since no one can really subscribe? IMHO we should really avoid doing so.

[Carl Meyer]

Replying to charettes:

Even if #9318 is fixed we wan't to avoid dispatching signals for the DeferredFieldProxy for performance reasons.

Anyway what's the use case of dispatching it since no one can really subscribe? IMHO we should really avoid doing so.

There's not a use case for it, clearly, but there is code-maintainability value in not having to think about this special case whenever we fire model signals.

The performance concern is relevant if #9318 is fixed by firing signals multiple times (in that case I agree that we should fix this), but not if it's fixed in the signals framework itself, to fire the signal only once but include superclass receivers.

I don't feel strongly about _not_ doing this, but I do think that if we do fix it, we should fix it consistently for all model signals.

[speedplane]

Hi, I just spent several hours tracking down this bug. I have a two-line fix that should work for all signals, not just the deletion signals, and will not result in multiple signal firing. That said, I'm not terribly experienced with the django code-base, so there may be some other negative effects.

The fix is directed to the function _make_id which makes a unique identifier for an object. We just detect when the target is a deferred model, and if it is, we get the original.

def _make_id(target):
    if hasattr(target, '_deferred') and target._deferred:
        return id(target._meta.proxy_for_model)
    if hasattr(target, 'im_func'):
        return (id(target.im_self), id(target.im_func))
    return id(target)

[midiotthimble]

Why is this still not fixed? Nobody used deferred fields?

[Tim Graham]

Simon, your tests pass after 7f51876f99851fdc3fef63aecdfbcffa199c26b9. Can we close the ticket? Is there value in adding the tests?

[Simon Charette]

I think adding the tests can't hurt. I'll come up with a rebased version of the patch this week.

[Simon Charette]

​PR

[Simon Charette <charette.s@…>]

In 535660b8:

Refs #18100 -- Added tests for deferred model deletion signals.

Thanks Tim for the review and pointing out this was fixed by #26207.