Opened 3 years ago

Last modified 16 months ago

#18100 assigned Bug

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

Reported by: charettes Owned by: charettes
Component: Database layer (models, ORM) Version: 1.4
Severity: Normal Keywords: deferred delete signals
Cc: Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: yes
Easy pickings: no UI/UX: no


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(

The patch should be a simple check for _deferred here.

Attachments (1)

ticket-18100-with-test.diff (3.1 KB) - added by charettes 3 years ago.

Download all attachments as: .zip

Change History (10)

Changed 3 years ago by charettes

comment:1 Changed 3 years ago by charettes

  • Has patch set
  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset

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

comment:2 Changed 3 years ago by charettes

  • Owner changed from nobody to charettes
  • Status changed from new to assigned

comment:3 Changed 3 years ago by akaariai

  • Triage Stage changed from Unreviewed to Accepted

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

comment:4 Changed 3 years ago by carljm

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.

comment:5 follow-up: Changed 3 years ago by 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.

comment:6 in reply to: ↑ 5 Changed 3 years ago by carljm

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.

comment:7 Changed 3 years ago by 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)


Last edited 3 years ago by speedplane (previous) (diff)

comment:8 Changed 2 years ago by midiotthimble

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

comment:9 Changed 16 months ago by timo

  • Patch needs improvement set
Note: See TracTickets for help on using tickets.
Back to Top