Re: [Django] #9318: "Virtual" behaviour for signal dispatcher and model inheritance
Django
-------------------------------------+-------------------------------------
Reporter: | Owner: jdunck
svetlyak40wt | Status: assigned
Type: New | Component: Core (Other)
feature | Severity: Normal
Milestone: 1.3 | Keywords: model inheritance,
Version: 1.0 | signals, dispatch, proxy, subclass
Resolution: | Has patch: 1
Triage Stage: Accepted | Needs tests: 0
Needs documentation: 0 | Easy pickings: 0
Patch needs improvement: 1 |
UI/UX: 0 |
-------------------------------------+-------------------------------------
Changes (by erick.yellott@…):
* ui_ux: => 0
* easy: => 0
Comment:
The example towards the top with Parent and Child looks like it uses
Concrete Table Inheritance. Does this work for Single Table inheritance?
I'm coming from a symfony1.4 and Propel1.6 background and just switched to
Django1.3 within the week, so maybe I'm just doing it wrong, but for my
models that inherit an abstract class the signal isn't bubbling up to the
parent class.
{{{
#!div style="font-size: 80%"
Code highlighting:
{{{#!python
from django.db import models
from django.db.models.signals import pre_save
class BaseQBObject(models.Model):
class Meta:
abstract = True
def __unicode__(self):
if hasattr(self, 'fullname'):
return self.fullname
def callback(sender, *args, **kwargs):
print 'HERE'
pre_save.connect(callback, sender=BaseQBObject)
class Account(BaseQBObject):
# Stuff
}}}
}}}
I would expect saving the child class would print 'HERE', but specifying
sender=BaseQBObject doesn't seem to work as I would expect.
Sorry if I'm posting this in the wrong place.
--
Ticket URL: <https://code.djangoproject.com/ticket/9318#comment:15>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
-------------------------------------+-------------------------------------
Reporter: | Owner: jdunck
svetlyak40wt | Status: assigned
Type: New | Component: Core (Other)
feature | Severity: Normal
Milestone: 1.3 | Keywords: model inheritance,
Version: 1.0 | signals, dispatch, proxy, subclass
Resolution: | Has patch: 1
Triage Stage: Accepted | Needs tests: 0
Needs documentation: 0 | Easy pickings: 0
Patch needs improvement: 1 |
UI/UX: 0 |
-------------------------------------+-------------------------------------
Comment (by jdunck):
This is the right place. Sorry I still haven't had time to do a patch on
this.
--
Ticket URL: <https://code.djangoproject.com/ticket/9318#comment:16>
Django
-------------------------------------+-------------------------------------
Reporter: | Owner: jdunck
svetlyak40wt | Status: assigned
Type: New | Component: Core (Other)
feature | Severity: Normal
Milestone: 1.3 | Keywords: model inheritance,
Version: 1.0 | signals, dispatch, proxy, subclass
Resolution: | Has patch: 1
Triage Stage: Accepted | Needs tests: 0
Needs documentation: 0 | Easy pickings: 0
Patch needs improvement: 1 |
UI/UX: 0 |
-------------------------------------+-------------------------------------
Comment (by erick.yellott@…):
Replying to [comment:16 jdunck]:
> This is the right place. Sorry I still haven't had time to do a patch
on this.
Okay, cool. Thanks for the quick response. I've been scratching my head
for a couple of days.
--
Ticket URL: <https://code.djangoproject.com/ticket/9318#comment:17>
Django
-------------------------------------+-------------------------------------
Type: New feature | Status: assigned
Component: Core (Other) | Version: 1.0
Severity: Normal | Resolution:
Keywords: model inheritance, | Triage Stage: Accepted
signals, dispatch, proxy, | Needs documentation: 0
subclass | Patch needs improvement: 1
Has patch: 1 | UI/UX: 0
Needs tests: 0 |
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Changes (by Semmel):
* cc: Florian.Sening@… (added)
--
Ticket URL: <https://code.djangoproject.com/ticket/9318#comment:19>
Django
-------------------------------------+-------------------------------------
Reporter: svetlyak40wt | Owner: jdunck
Type: New feature | Status: assigned
Component: Core (Other) | Version: 1.0
Severity: Normal | Resolution:
Keywords: model inheritance, | Triage Stage: Accepted
signals, dispatch, proxy, | Needs documentation: 0
subclass | Patch needs improvement: 1
Has patch: 1 | UI/UX: 0
Needs tests: 0 |
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Comment (by carljm):
Closed #18094 as a duplicate - there's some useful discussion there. Also,
there's a mailing list thread at https://groups.google.com/d/msg/django-
developers/2aDFeNAXJgI/-riTuIgNLPQJ
If a fix is put into place within the signals framework itself, then the
pre/post_delete signal behavior should be changed so that it does not fire
separate signals for parent model classes, which it does now (unlike the
save and init signals).
I'll also repeat one comment I made on #18094: I think a fix that relies
on opt-in from the person registering the signal handler is not adequate,
as this is often not the same person who might be creating a subclass
(particularly a proxy subclass). A reusable app that registers a signal
handler for a model ought to be able to assume that that handler will fire
when instances of that model are involved, without needing to take special
opt-in steps to ensure that this is still the case when subclasses are
involved.
--
Ticket URL: <https://code.djangoproject.com/ticket/9318#comment:20>
Django
-------------------------------------+-------------------------------------
Reporter: svetlyak40wt | Owner: jdunck
Type: New feature | Status: assigned
Component: Core (Other) | Version: 1.0
Severity: Normal | Resolution:
Keywords: model inheritance, | Triage Stage: Accepted
signals, dispatch, proxy, | Needs documentation: 0
subclass | Patch needs improvement: 1
Has patch: 1 | UI/UX: 0
Needs tests: 0 |
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Comment (by carljm):
Replying to [comment:20 carljm]:
> I'll also repeat one comment I made on #18094: I think a fix that relies
on opt-in from the person registering the signal handler is not adequate,
as this is often not the same person who might be creating a subclass
(particularly a proxy subclass). A reusable app that registers a signal
handler for a model ought to be able to assume that that handler will fire
when instances of that model are involved, without needing to take special
opt-in steps to ensure that this is still the case when subclasses are
involved.
Although I suppose that this might be a necessary concession to backwards
compatibility - in that case, it should be featured in the documentation,
as I would think that most new code should register signal handlers using
this new opt-in flag (and perhaps we should even consider deprecating the
flag over time in favor of making this the default behavior).
--
Ticket URL: <https://code.djangoproject.com/ticket/9318#comment:21>
Django
-------------------------------------+-------------------------------------
Reporter: svetlyak40wt | Owner: jdunck
Type: New feature | Status: assigned
Component: Core (Other) | Version: 1.0
Severity: Normal | Resolution:
Keywords: model inheritance, | Triage Stage: Accepted
signals, dispatch, proxy, | Needs documentation: 0
subclass | Patch needs improvement: 1
Has patch: 1 | UI/UX: 0
Needs tests: 0 |
Easy pickings: 0 |
-------------------------------------+-------------------------------------
* cc: streeter (added)
--
Ticket URL: <https://code.djangoproject.com/ticket/9318#comment:22>
Django
-------------------------------------+-------------------------------------
Reporter: svetlyak40wt | Owner: jdunck
Type: New feature | Status: assigned
Component: Core (Other) | Version: 1.0
Severity: Normal | Resolution:
Keywords: model inheritance, | Triage Stage: Accepted
signals, dispatch, proxy, | Needs documentation: 0
subclass | Patch needs improvement: 1
Has patch: 1 | UI/UX: 0
Needs tests: 0 |
Easy pickings: 0 |
-------------------------------------+-------------------------------------
* cc: charette.s@… (added)
--
Ticket URL: <https://code.djangoproject.com/ticket/9318#comment:23>
Django
-------------------------------------+-------------------------------------
Reporter: svetlyak40wt | Owner: jdunck
Type: New feature | Status: assigned
Component: Core (Other) | Version: 1.0
Severity: Normal | Resolution:
Keywords: model inheritance, | Triage Stage: Accepted
signals, dispatch, proxy, | Needs documentation: 0
subclass | Patch needs improvement: 1
Has patch: 1 | UI/UX: 0
Needs tests: 0 |
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Comment (by jdunck):
I've started a branch to work on this problem:
https://github.com/votizen/django/tree/virtual_signals
--
Ticket URL: <https://code.djangoproject.com/ticket/9318#comment:24>
Django
-------------------------------------+-------------------------------------
Reporter: svetlyak40wt | Owner: jdunck
Type: New feature | Status: assigned
Component: Core (Other) | Version: 1.0
Severity: Normal | Resolution:
Keywords: model inheritance, | Triage Stage: Accepted
signals, dispatch, proxy, | Needs documentation: 0
subclass | Patch needs improvement: 1
Has patch: 1 | UI/UX: 0
Needs tests: 0 |
Easy pickings: 0 |
-------------------------------------+-------------------------------------
* cc: brooks.travis@… (added)
--
Ticket URL: <https://code.djangoproject.com/ticket/9318#comment:25>
Django
-------------------------------------+-------------------------------------
Reporter: svetlyak40wt | Owner: jdunck
Type: New feature | Status: assigned
Component: Core (Other) | Version: 1.0
Severity: Normal | Resolution:
Keywords: model inheritance, | Triage Stage: Accepted
signals, dispatch, proxy, | Needs documentation: 0
subclass | Patch needs improvement: 1
Has patch: 1 | UI/UX: 0
Needs tests: 0 |
Easy pickings: 0 |
-------------------------------------+-------------------------------------
* cc: loic@… (added)
--
Ticket URL: <https://code.djangoproject.com/ticket/9318#comment:26>
Django
-------------------------------------+-------------------------------------
Reporter: svetlyak40wt | Owner: jdunck
Type: New feature | Status: assigned
Component: Core (Other) | Version: 1.0
Severity: Normal | Resolution:
Keywords: model inheritance, | Triage Stage: Accepted
signals, dispatch, proxy, |
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by torstenrudolf):
Is there any update on this issue?
--
Ticket URL: <https://code.djangoproject.com/ticket/9318#comment:27>
Django
-------------------------------------+-------------------------------------
Reporter: svetlyak40wt | Owner: jdunck
Type: New feature | Status: assigned
Component: Core (Other) | Version: 1.0
Severity: Normal | Resolution:
Keywords: model inheritance, | Triage Stage: Accepted
signals, dispatch, proxy, |
subclass |
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by charettes):
I think the last effort toward fixing this was jdunck's branch which is
not available anymore.
--
Ticket URL: <https://code.djangoproject.com/ticket/9318#comment:28>
Django
-------------------------------------+-------------------------------------
Reporter: svetlyak40wt | Owner:
Component: Core (Other) | Version: 1.0
Severity: Normal | Resolution:
Keywords: model inheritance, | Triage Stage: Accepted
signals, dispatch, proxy, |
subclass |
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* status: assigned => new
* owner: jdunck =>
--
Ticket URL: <https://code.djangoproject.com/ticket/9318#comment:29>
Django
-------------------------------------+-------------------------------------
Reporter: svetlyak40wt | Owner:
Type: New feature | Status: new
Component: Core (Other) | Version: 1.0
Severity: Normal | Resolution:
Keywords: model inheritance, | Triage Stage: Accepted
signals, dispatch, proxy, |
subclass |
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by sassanh):
Any updates? Workarounds?
--
Ticket URL: <https://code.djangoproject.com/ticket/9318#comment:30>
Django
-------------------------------------+-------------------------------------
Type: Bug | Status: new
Component: Core (Other) | Version: 1.0
Severity: Normal | Resolution:
Keywords: model inheritance, | Triage Stage: Accepted
signals, dispatch, proxy, |
subclass |
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* type: New feature => Bug
Comment:
I hit this bug today.
It's really a silent data loss issue: creating a proxy model shouldn't
disable behavior of the original model.
I'm applying the following patch to Django until this is resolved:
{{{
--- a/django/db/models/base.py 2016-12-07 17:09:16.000000000 +0100
+++ b/django/db/models/base.py 2016-12-07 17:13:18.000000000 +0100
@@ -810,13 +810,12 @@
using = using or router.db_for_write(self.__class__,
instance=self)
assert not (force_insert and (force_update or update_fields))
assert update_fields is None or len(update_fields) > 0
- cls = origin = self.__class__
- # Skip proxies, but keep the origin as the proxy model.
+ cls = self.__class__
if cls._meta.proxy:
cls = cls._meta.concrete_model
meta = cls._meta
if not meta.auto_created:
- signals.pre_save.send(sender=origin, instance=self, raw=raw,
using=using,
+ signals.pre_save.send(sender=cls, instance=self, raw=raw,
using=using,
update_fields=update_fields)
with transaction.atomic(using=using, savepoint=False):
if not raw:
@@ -829,7 +828,7 @@
# Signal that the save is complete
if not meta.auto_created:
- signals.post_save.send(sender=origin, instance=self,
created=(not updated),
+ signals.post_save.send(sender=cls, instance=self,
created=(not updated),
update_fields=update_fields, raw=raw,
using=using)
save_base.alters_data = True
}}}
IMO this is the correct way to fix the issue. I understand the concern
about backwards compatibility but I have a hard time figuring out a
realistic scenario where developers would purposefully rely on this bug.
--
Ticket URL: <https://code.djangoproject.com/ticket/9318#comment:31>
Django
-------------------------------------+-------------------------------------
Reporter: Alexander Artemenko | Owner: (none)
Component: Core (Other) | Version: 1.0
Severity: Normal | Resolution:
Keywords: model inheritance, | Triage Stage: Accepted
signals, dispatch, proxy, |
subclass |
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Changes (by Aymeric Augustin):
* type: Bug => New feature
Comment:
For proxy models it's a straightforward bug.
For multi-table inheritance it's more complicated.
--
Ticket URL: <https://code.djangoproject.com/ticket/9318#comment:32>
Django
-------------------------------------+-------------------------------------
Reporter: Alexander Artemenko | Owner: (none)
Type: New feature | Status: new
Component: Core (Other) | Version: 1.0
Severity: Normal | Resolution:
Keywords: model inheritance, | Triage Stage: Accepted
signals, dispatch, proxy, |
subclass |
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Aymeric Augustin):
I found a workaround that doesn't require patching Django, but that may
affect performance:
{{{#!python
# Because of https://code.djangoproject.com/ticket/9318, declaring
# `@dispatch.receiver(..., sender=myapp.MyModel, ...)` wouldn't
# trigger this function when a subclass is saved. So we perform the check
# manually with `issubclass(sender, myapp.MyModel)`. This is
# inefficient because any save(), for any model, now calls this function.
@dispatch.receiver(signals.post_save)
def request_saved(sender, instance, created, **kwargs):
if not issubclass(sender, myapp.MyModel):
return
...
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/9318#comment:33>
Django
-------------------------------------+-------------------------------------
Reporter: Alexander Artemenko | Owner: (none)
Type: New feature | Status: new
Component: Core (Other) | Version: 1.0
Severity: Normal | Resolution:
Keywords: model inheritance, | Triage Stage: Accepted
signals, dispatch, proxy, |
subclass |
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Derek Frank):
This is also an issue for deferred querysets.
`queryset = Blog.objects.only('pk')`
`queryset.delete()` will not run handlers for pre_delete and post_delete
signals.
It appears a new class type is defined for the deferred object.
`id(queryset.first().__class__) == id(Blog)` --> `False`
--
Ticket URL: <https://code.djangoproject.com/ticket/9318#comment:34>
Django
-------------------------------------+-------------------------------------
Reporter: Alexander Artemenko | Owner: (none)
Type: New feature | Status: new
Component: Core (Other) | Version: 1.0
Severity: Normal | Resolution:
Keywords: model inheritance, | Triage Stage: Accepted
signals, dispatch, proxy, |
subclass |
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Simon Charette):
Derek, FWIW deferred models are not an issue anymore from Django 1.10+.
See #26207 for more details.
--
Ticket URL: <https://code.djangoproject.com/ticket/9318#comment:35>
Django
-------------------------------------+-------------------------------------
Reporter: Alexander Artemenko | Owner: (none)
Type: New feature | Status: new
Component: Core (Other) | Version: 1.0
Severity: Normal | Resolution:
Keywords: model inheritance, | Triage Stage: Accepted
signals, dispatch, proxy, |
subclass |
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Soheil Damangir):
A quick fix without a need to patch could be to override the save method
for the parent mode:
{{{
from django.db import models
class MyModel(models.Model):
"""
>>> m = MyModel(name="The model")
>>> m.save()
Hello!
"""
name = models.CharField(max_length=10)
def save(self, *args, **kwargs):
# To trigger the signals for parent model
klass = self.__class__
try:
if self.__class__._meta.proxy:
self.__class__ = self.__class__._meta.concrete_model
super(MyModel, self).save(*args, **kwargs)
finally:
self.__class__ = klass
class ProxyModel(MyModel):
"""
>>> p = ProxyModel(name="Proxy")
>>> p.save()
Hello!
"""
class Meta:
proxy = True
from django.db.models.signals import post_save
def say_hello(sender, *args, **kwargs):
print "Hello!"
post_save.connect(say_hello, sender=Parent)
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/9318#comment:36>
Django
-------------------------------------+-------------------------------------
Reporter: Alexander Artemenko | Owner: (none)
Type: New feature | Status: new
Component: Core (Other) | Version: 1.0
Severity: Normal | Resolution:
Keywords: model inheritance, | Triage Stage: Accepted
signals, dispatch, proxy, |
subclass |
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by rgangopadhya):
To handle pre_save and all other signals, I took the approach of sending
all signals to the parent model when triggered by child models:
{{{
from django.db import models
from django.db.models.signals import (
pre_save,
post_save,
pre_delete,
post_delete,
m2m_changed,
)
class A(models.Model): pass
class B(A):
class Meta:
proxy = True
signals = [
pre_save,
post_save,
pre_delete,
post_delete,
m2m_changed,
]
def make_sender_fn(model, signal):
def sender_fn(sender, **kwargs):
# signal send method passes itself
# as a signal kwarg to its receivers
kwargs.pop('signal')
signal.send(sender=model, **kwargs)
return sender_fn
def connect_all_signals(from_model, to_model):
# connecting signals from one model to another
# is useful when from_model inherits from to_model
# and hence should take on all behavior from the
# parent model
# Django does not send signals for parent model
# when child is triggered
# https://code.djangoproject.com/ticket/9318#no1
for signal in signals:
signal.connect(
make_sender_fn(to_model, signal),
sender=from_model,
weak=False,
)
connect_all_signals(B, A)
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/9318#comment:37>
Django
-------------------------------------+-------------------------------------
Reporter: Alexander Artemenko | Owner: (none)
Type: New feature | Status: new
Component: Core (Other) | Version: 1.0
Severity: Normal | Resolution:
Keywords: model inheritance, | Triage Stage: Accepted
signals, dispatch, proxy, |
subclass |
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* cc: Florian Demmer (added)
--
Ticket URL: <https://code.djangoproject.com/ticket/9318#comment:38>
Django
-------------------------------------+-------------------------------------
Reporter: Alexander Artemenko | Owner: (none)
Type: New feature | Status: new
Component: Core (Other) | Version: 1.0
Severity: Normal | Resolution:
Keywords: model inheritance, | Triage Stage: Accepted
signals, dispatch, proxy, |
subclass |
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* cc: Jacob Walls (added)
--
Ticket URL: <https://code.djangoproject.com/ticket/9318#comment:39>