Opened 16 years ago
Last modified 5 months ago
#9318 new New feature
"Virtual" behaviour for signal dispatcher and model inheritance
| Reported by: | Alexander Artemenko | Owned by: | |
|---|---|---|---|
| Component: | Core (Other) | Version: | 1.0 |
| Severity: | Normal | Keywords: | model inheritance, signals, dispatch, proxy, subclass |
| Cc: | Idan Gazit, shadfc, benbest86, Alex Robbins, Simon Meers, Torsten Bronger, vlastimil.zima@…, Florian.Sening@…, Chris Streeter, charette.s@…, brooks.travis@…, loic@…, Florian Demmer | Triage Stage: | Accepted |
| Has patch: | yes | Needs documentation: | no |
| Needs tests: | no | Patch needs improvement: | yes |
| Easy pickings: | no | UI/UX: | no |
Description
In presence of model inheritance, we need to propagate signal not only to handlers, attached to the Child classes, but also to handlers, attached to the Parent class.
Please, see unittests in the patch, for example of such model inheritance.
Attachments (1)
Change History (39)
by , 16 years ago
| Attachment: | 0001-Propagate-message-to-parent-s-handler-sender-is-chil.patch added |
|---|
comment:1 by , 16 years ago
| Triage Stage: | Unreviewed → Accepted |
|---|
The principle seems sound enough, although we need to think hard about whether to turn this on by default, since it will change existing behaviour in backwards-incompatible ways for some signal handlers. It could be controlled in an "off by default" way by adding a flag to the connect method, indicating that we also wish to receive emits that originated on the child. We should consider adding a parameter like "from_child" to signal emits to indicate if the signal is coming from a sub-class so that handlers have an opportunity to avoid duplicate work.
On an implementation note (and the patch needs more careful examination than I've given it just now, but this stood out), if we're going to "leak" information about _meta and parents into dispatcher.py, we might as well just test if it's an instance of django.db.models.Model, rather than seeing if it's a class with a _meta attribute. But there could be some other way to structure that entirely so that working out the right emitting classes to use can be more self-contained within a model itself and the dispatcher.py could avoid knowing about that. Needs some thought/creativity.
I'll make this "accepted", since it's a feature worth adding, but what shape the final feature will take still needs a bit of consideration, I feel.
comment:2 by , 16 years ago
I guess you are talking about:
from django.db import models class Parent(models.Model): """ >>> p = Parent(name="Hola") >>> p.save() Hello! """ name = models.CharField(max_length=10, blank=True, default='') class Child(Parent): """ >>> p = Child(name="Hola ahora") >>> p.save() Hello! """ childname = models.CharField(max_length=10, blank=True, default='') from django.db.models.signals import post_init from django.db.models.signals import post_save def say_hello(sender, *args, **kwargs): print "Hello!" post_save.connect(say_hello, sender=Parent)
?? The doctest that fails (the one in Child) should be considered a bug.
One would really expect that it's signal handlers still fire on a class althought having been inherited (I'd guess that's how most languages handle this).
A side, reading the docs about pre_init and post_init, you'd expect that if you Model's init() method is run, handlers attached to this should also be fired, which isn't the case.
So, in brief, handlers attached to a Model should be fired when signals get sent from Child Models. Having it disabled by default can confuse people, but maybe we could disable this until 2.0 :)
comment:3 by , 16 years ago
Why not just use klass.bases to avoid any special dependence on knowing it's a model. I realize it isn't a common case to have signals who's sender isn't a model, but I don't see any overhead in allowing it.
comment:4 by , 16 years ago
| Cc: | added |
|---|
comment:5 by , 16 years ago
| Cc: | added |
|---|
This would be helpful for me. I have a Payment class with subclasses of things like PaypalPayment, CreditCardPayment, GiftCertificatePayment, etc. Payment FKs to an Order and I want to update some status stuff about Order whenever its payments change. So it appears that as of now, I just have to listen for post_* on all of the child classes. Not terrible, but I expected that Payment would emit a signal when a child class save()s, for instance.
comment:6 by , 15 years ago
| Cc: | added |
|---|
I have come across the need for this as well - especially when overriding defaults of other projects by inheritance instead of forking.
One (relatively) easy way to deal with this is to listen on the relevant child class signals and send the base class signal from there. This way you can still attach all of your common signals to the base class and not worry about adding a new one to each child for every new listening function.
Ex:
class Child(Parent):
pass
def call_parent_post_save(sender, instance, created, **kwargs):
post_save.send(sender=Parent, instance=Parent.objects.get(id=instance.id), created=created, **kwargs)
post_save.connect(call_parent_post_save, sender=Child)
Not sure if there is a better way to get the proper instance though - haven't found a great way to do this directly from the existing instance instead of using a query. Any thoughts would be appreciated.
comment:7 by , 15 years ago
| Cc: | added |
|---|
You can get around this pretty easily. When you register the signal, don't specify a sender. Then, inside the signal handler, just do an isinstance test.
For example:
signals.py
def my_handler(sender, **kwargs):
from app.models import Parent #Avoid circular import
if not isinstance(instance, Parent):
return
#do normal signal stuff here
models.py
post_save.connect(my_handler)
comment:8 by , 15 years ago
| Cc: | added |
|---|---|
| Keywords: | proxy subclass added |
| milestone: | → 1.3 |
Same problem with proxy models
comment:9 by , 14 years ago
| Cc: | added |
|---|
comment:10 by , 14 years ago
| Owner: | changed from to |
|---|
I was somehow unaware of this problem but just tripped across it. Will try to sprint on at PyCon.
comment:11 by , 14 years ago
| Status: | new → assigned |
|---|
comment:13 by , 14 years ago
| Cc: | added |
|---|
comment:14 by , 14 years ago
| Severity: | → Normal |
|---|---|
| Type: | → New feature |
comment:15 by , 13 years ago
| Easy pickings: | unset |
|---|---|
| UI/UX: | unset |
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.
Code highlighting:
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.
follow-up: 17 comment:16 by , 13 years ago
This is the right place. Sorry I still haven't had time to do a patch on this.
comment:17 by , 13 years ago
Replying to 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.
comment:19 by , 13 years ago
| Cc: | added |
|---|
follow-up: 21 comment:20 by , 13 years ago
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.
comment:21 by , 13 years ago
Replying to 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).
comment:22 by , 13 years ago
| Cc: | added |
|---|
comment:23 by , 13 years ago
| Cc: | added |
|---|
comment:24 by , 13 years ago
I've started a branch to work on this problem:
https://github.com/votizen/django/tree/virtual_signals
comment:25 by , 12 years ago
| Cc: | added |
|---|
comment:26 by , 11 years ago
| Cc: | added |
|---|
comment:28 by , 10 years ago
I think the last effort toward fixing this was jdunck's branch which is not available anymore.
comment:29 by , 9 years ago
| Owner: | removed |
|---|---|
| Status: | assigned → new |
comment:31 by , 8 years ago
| Type: | New feature → Bug |
|---|
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.
comment:32 by , 8 years ago
| Type: | Bug → New feature |
|---|
For proxy models it's a straightforward bug.
For multi-table inheritance it's more complicated.
comment:33 by , 8 years ago
I found a workaround that doesn't require patching Django, but that may affect performance:
# 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 ...
comment:34 by , 8 years ago
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
comment:35 by , 8 years ago
Derek, FWIW deferred models are not an issue anymore from Django 1.10+. See #26207 for more details.
comment:36 by , 8 years ago
In case of proxy models, for always getting the signal on the parent model without a need to patch, a quick fix could be to override the save method for the parent model:
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= MyModel)
comment:37 by , 8 years ago
For proxy models, to handle pre_save and all other signals, rather than overriding save, 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):
for signal in signals:
signal.connect(
make_sender_fn(to_model, signal),
sender=from_model,
weak=False,
)
connect_all_signals(B, A)
comment:38 by , 5 months ago
| Cc: | added |
|---|
"Virtual" behaviour for signal dispatcher.