Opened 8 years ago

Last modified 33 hours 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 8 years ago.
"Virtual" behaviour for signal dispatcher.

Download all attachments as: .zip

Change History (33)

Changed 8 years ago by Alexander Artemenko

"Virtual" behaviour for signal dispatcher.

comment:1 Changed 8 years ago by Malcolm Tredinnick

Triage Stage: UnreviewedAccepted

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 Changed 8 years ago by Marc Fargas

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 Changed 8 years ago by Alex Gaynor

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 Changed 8 years ago by Idan Gazit

Cc: Idan Gazit added

comment:5 Changed 7 years ago by shadfc

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 Changed 7 years ago by benbest86

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 Changed 7 years ago by Alex Robbins

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 Changed 7 years ago by Simon Meers

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

Same problem with proxy models

comment:9 Changed 6 years ago by Torsten Bronger

Cc: Torsten Bronger added

comment:10 Changed 6 years ago by Jeremy Dunck

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 Changed 6 years ago by Jeremy Dunck

Status: newassigned

comment:12 Changed 6 years ago by Łukasz Rekucki

Patch needs improvement: set

Ticket gardening.

comment:13 Changed 6 years ago by Vlastimil Zíma

Cc: vlastimil.zima@… added

comment:14 Changed 6 years ago by Luke Plant

Severity: Normal
Type: New feature

comment:15 Changed 5 years ago by erick.yellott@…

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 Changed 5 years ago by Jeremy Dunck

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

comment:17 in reply to:  16 Changed 5 years ago by erick.yellott@…

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 Changed 5 years ago by Jacob

milestone: 1.3

Milestone 1.3 deleted

comment:19 Changed 5 years ago by Florian Sening

Cc: Florian.Sening@… added

comment:20 Changed 5 years ago by Carl Meyer

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 in reply to:  20 Changed 5 years ago by Carl Meyer

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 Changed 5 years ago by Chris Streeter

Cc: Chris Streeter added

comment:23 Changed 5 years ago by Simon Charette

Cc: charette.s@… added

comment:24 Changed 5 years ago by Jeremy Dunck

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

comment:25 Changed 4 years ago by brooks.travis@…

Cc: brooks.travis@… added

comment:26 Changed 3 years ago by loic84

Cc: loic@… added

comment:27 Changed 2 years ago by torstenrudolf

Is there any update on this issue?

comment:28 Changed 2 years ago by Simon Charette

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

comment:29 Changed 7 months ago by Tim Graham

Owner: Jeremy Dunck deleted
Status: assignednew

comment:30 Changed 4 months ago by Sassan Haradji

Any updates? Workarounds?

comment:31 Changed 33 hours ago by Aymeric Augustin

Type: New featureBug

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 Changed 33 hours ago by Aymeric Augustin

Type: BugNew feature

For proxy models it's a straightforward bug.

For multi-table inheritance it's more complicated.

Note: See TracTickets for help on using tickets.
Back to Top