[Django] #18100: Deleting model instances with deferred fields don't trigger deletion signals

Django

unread,

Apr 10, 2012, 11:32:52 PM4/10/12

to django-...@googlegroups.com

#18100: Deleting model instances with deferred fields don't trigger deletion
signals
----------------------------------------------+----------------------------
Reporter: charettes | Owner: nobody
Type: Bug | Status: new
Component: Database layer (models, ORM) | Version: 1.4
Severity: Normal | Keywords: deferred
Triage Stage: Unreviewed | delete signals
Easy pickings: 0 | Has patch: 0
| UI/UX: 0
----------------------------------------------+----------------------------
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``
[https://code.djangoproject.com/browser/django/trunk/django/db/models/deletion.py#L155
here].

--
Ticket URL: <https://code.djangoproject.com/ticket/18100>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.

Django

unread,

Apr 11, 2012, 1:22:33 AM4/11/12

to django-...@googlegroups.com

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

-------------------------------------+-------------------------------------

Reporter: charettes | Owner: nobody
Type: Bug | Status: new

* needs_better_patch: => 0
* has_patch: 0 => 1
* needs_tests: => 0
* needs_docs: => 0

Comment:

Added a patch with test. I cannot run against the full test suite because
I'm getting the same failures as the
[http://ci.djangoproject.com/job/Django%20trunk/lastCompletedBuild/testReport/
"ci server"].

--
Ticket URL: <https://code.djangoproject.com/ticket/18100#comment:1>

Django

unread,

Apr 11, 2012, 1:22:39 AM4/11/12

to django-...@googlegroups.com

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

-------------------------------------+-------------------------------------
Reporter: charettes | Owner: charettes
Type: Bug | Status: assigned

* status: new => assigned
* owner: nobody => charettes

--
Ticket URL: <https://code.djangoproject.com/ticket/18100#comment:2>

Django

unread,

Apr 11, 2012, 6:47:44 AM4/11/12

to django-...@googlegroups.com

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

-------------------------------------+-------------------------------------
Reporter: charettes | Owner: charettes
Type: Bug | Status: assigned
Component: Database layer | Version: 1.4
(models, ORM) | Resolution:

* stage: Unreviewed => Accepted

Comment:

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

--
Ticket URL: <https://code.djangoproject.com/ticket/18100#comment:3>

Django

unread,

Apr 12, 2012, 2:22:59 PM4/12/12

to django-...@googlegroups.com

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

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

--
Ticket URL: <https://code.djangoproject.com/ticket/18100#comment:4>

Django

unread,

Apr 12, 2012, 2:46:08 PM4/12/12

to django-...@googlegroups.com

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

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

--
Ticket URL: <https://code.djangoproject.com/ticket/18100#comment:5>

Django

unread,

Apr 12, 2012, 2:51:22 PM4/12/12

to django-...@googlegroups.com

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

Comment (by carljm):

Replying to [comment:5 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.

--
Ticket URL: <https://code.djangoproject.com/ticket/18100#comment:6>

Django

unread,

Apr 7, 2013, 9:51:22 PM4/7/13

to django-...@googlegroups.com

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

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

--
Ticket URL: <https://code.djangoproject.com/ticket/18100#comment:7>

Django

unread,

Jul 9, 2013, 11:08:15 PM7/9/13

to django-...@googlegroups.com

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

Comment (by midiotthimble):

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

--
Ticket URL: <https://code.djangoproject.com/ticket/18100#comment:8>

Django

unread,

Jun 13, 2014, 8:56:14 PM6/13/14

to django-...@googlegroups.com

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

-------------------------------------+-------------------------------------
Reporter: charettes | Owner: charettes
Type: Bug | Status: assigned
Component: Database layer | Version: 1.4
(models, ORM) | Resolution:
Severity: Normal | Triage Stage: Accepted
Keywords: deferred delete | Needs documentation: 0

signals | Patch needs improvement: 1

Has patch: 1 | UI/UX: 0
Needs tests: 0 |
Easy pickings: 0 |
-------------------------------------+-------------------------------------

Changes (by timo):

* needs_better_patch: 0 => 1

--
Ticket URL: <https://code.djangoproject.com/ticket/18100#comment:9>

Django

unread,

May 7, 2016, 6:27:38 PM5/7/16

to django-...@googlegroups.com

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

-------------------------------------+-------------------------------------
Reporter: charettes | Owner: charettes
Type: Bug | Status: assigned
Component: Database layer | Version: 1.4
(models, ORM) |

Comment (by timgraham):

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

--
Ticket URL: <https://code.djangoproject.com/ticket/18100#comment:10>

Django

unread,

May 8, 2016, 8:05:57 PM5/8/16

to django-...@googlegroups.com

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

Comment (by charettes):

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

--
Ticket URL: <https://code.djangoproject.com/ticket/18100#comment:11>

Django

unread,

May 10, 2016, 10:45:48 AM5/10/16

to django-...@googlegroups.com

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

-------------------------------------+-------------------------------------
Reporter: charettes | Owner: charettes
Type: Bug | Status: assigned

Component: Database layer | Version: master

Needs tests: 0 | Patch needs improvement: 0

Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Changes (by charettes):

* needs_better_patch: 1 => 0
* version: 1.4 => master

Comment:

[https://github.com/django/django/pull/6581 PR]

--
Ticket URL: <https://code.djangoproject.com/ticket/18100#comment:12>

Django

unread,

May 10, 2016, 11:28:12 AM5/10/16

to django-...@googlegroups.com

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

Keywords: deferred delete | Triage Stage: Ready for
signals | checkin

Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Changes (by timgraham):

* stage: Accepted => Ready for checkin

--
Ticket URL: <https://code.djangoproject.com/ticket/18100#comment:13>

Django

unread,

May 10, 2016, 7:32:41 PM5/10/16

to django-...@googlegroups.com

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

-------------------------------------+-------------------------------------
Reporter: charettes | Owner: charettes
Type: Bug | Status: assigned
Component: Database layer | Version: master
(models, ORM) |
Severity: Normal | Resolution:
Keywords: deferred delete | Triage Stage: Ready for
signals | checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by Simon Charette <charette.s@…>):

In [changeset:"535660b852c8899a17b82d20a06c926d8d437db3" 535660b8]:
{{{
#!CommitTicketReference repository=""
revision="535660b852c8899a17b82d20a06c926d8d437db3"
Refs #18100 -- Added tests for deferred model deletion signals.

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

--
Ticket URL: <https://code.djangoproject.com/ticket/18100#comment:14>

Django

unread,

May 10, 2016, 7:33:03 PM5/10/16

to django-...@googlegroups.com

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

-------------------------------------+-------------------------------------
Reporter: charettes | Owner: charettes

Type: Bug | Status: closed

Component: Database layer | Version: master
(models, ORM) |

Severity: Normal | Resolution: fixed