Opened 4 years ago

Closed 5 months ago

#18100 closed Bug (fixed)

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

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

Description

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.

Attachments (1)

ticket-18100-with-test.diff (3.1 KB) - added by Simon Charette 4 years ago.

Download all attachments as: .zip

Change History (16)

Changed 4 years ago by Simon Charette

Attachment: ticket-18100-with-test.diff added

comment:1 Changed 4 years ago by Simon Charette

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 4 years ago by Simon Charette

Owner: changed from nobody to Simon Charette
Status: newassigned

comment:3 Changed 4 years ago by Anssi Kääriäinen

Triage Stage: UnreviewedAccepted

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 4 years ago by 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.

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

comment:6 in reply to:  5 Changed 4 years ago by 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.

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)

django\dispatch\dispatcher.py

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

comment:8 Changed 3 years ago by midiotthimble

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

comment:9 Changed 2 years ago by Tim Graham

Patch needs improvement: set

comment:10 Changed 5 months ago by Tim Graham

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

comment:11 Changed 5 months ago by Simon Charette

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

comment:12 Changed 5 months ago by Simon Charette

Patch needs improvement: unset
Version: 1.4master

comment:13 Changed 5 months ago by Tim Graham

Triage Stage: AcceptedReady for checkin

comment:14 Changed 5 months ago by 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.

comment:15 Changed 5 months ago by Simon Charette

Resolution: fixed
Status: assignedclosed
Note: See TracTickets for help on using tickets.
Back to Top