Opened 16 years ago

Last modified 7 years 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@… 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)

0001-Propagate-message-to-parent-s-handler-sender-is-chil.patch​ (4.9 KB ) - added by Alexander Artemenko 16 years ago.
"Virtual" behaviour for signal dispatcher.

Download all attachments as: .zip

Change History (38)

by Alexander Artemenko, 16 years ago

Attachment: 0001-Propagate-message-to-parent-s-handler-sender-is-chil.patch​ added

"Virtual" behaviour for signal dispatcher.

comment:1 by Malcolm Tredinnick, 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 Marc Fargas, 15 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 Alex Gaynor, 15 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 Idan Gazit, 15 years ago

Cc: Idan Gazit added

comment:5 by shadfc, 15 years ago

Cc: shadfc 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 benbest86, 15 years ago

Cc: benbest86 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 Alex Robbins, 14 years ago

Cc: Alex Robbins 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 Simon Meers, 14 years ago

Cc: Simon Meers added
Keywords: proxy subclass added
milestone: → 1.3

Same problem with proxy models

comment:9 by Torsten Bronger, 14 years ago

Cc: Torsten Bronger added

comment:10 by Jeremy Dunck, 13 years ago

Owner: changed from nobody to Jeremy Dunck

I was somehow unaware of this problem but just tripped across it. Will try to sprint on at PyCon.

comment:11 by Jeremy Dunck, 13 years ago

Status: new → assigned

comment:12 by Łukasz Rekucki, 13 years ago

Patch needs improvement: set

Ticket gardening.

comment:13 by Vlastimil Zíma, 13 years ago

Cc: vlastimil.zima@… added

comment:14 by Luke Plant, 13 years ago

Severity: → Normal
Type: → New feature

comment:15 by erick.yellott@…, 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.

comment:16 by Jeremy Dunck, 13 years ago

This is the right place. Sorry I still haven't had time to do a patch on this.

in reply to:  16 comment:17 by erick.yellott@…, 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:18 by Jacob, 13 years ago

milestone: 1.3

Milestone 1.3 deleted

comment:19 by Florian Sening, 13 years ago

Cc: Florian.Sening@… added

comment:20 by Carl Meyer, 12 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.

in reply to:  20 comment:21 by Carl Meyer, 12 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 Chris Streeter, 12 years ago

Cc: Chris Streeter added

comment:23 by Simon Charette, 12 years ago

Cc: charette.s@… added

comment:24 by Jeremy Dunck, 12 years ago

I've started a branch to work on this problem:
​https://github.com/votizen/django/tree/virtual_signals

comment:25 by brooks.travis@…, 12 years ago

Cc: brooks.travis@… added

comment:26 by loic84, 11 years ago

Cc: loic@… added

comment:27 by torstenrudolf, 9 years ago

Is there any update on this issue?

comment:28 by Simon Charette, 9 years ago

I think the last effort toward fixing this was jdunck's branch which is not available anymore.

comment:29 by Tim Graham, 8 years ago

Owner: Jeremy Dunck removed
Status: assigned → new

comment:30 by Sassan Haradji, 8 years ago

Any updates? Workarounds?

comment:31 by Aymeric Augustin, 7 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 Aymeric Augustin, 7 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 Aymeric Augustin, 7 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 Derek Frank, 7 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 Simon Charette, 7 years ago

Derek, FWIW deferred models are not an issue anymore from Django 1.10+. See #26207 for more details.

comment:36 by Soheil Damangir, 7 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)
Last edited 7 years ago by Soheil Damangir (previous) (diff)

comment:37 by rgangopadhya, 7 years ago

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):
    for signal in signals:
        signal.connect(
            make_sender_fn(to_model, signal),
            sender=from_model,
            weak=False,
        )

connect_all_signals(B, A)
Version 1, edited 7 years ago by rgangopadhya (previous) (next) (diff)
Note: See TracTickets for help on using tickets.
Back to Top