Opened 7 years ago

Last modified 8 months ago

#9318 assigned New feature

"Virtual" behaviour for signal dispatcher and model inheritance

Reported by: svetlyak40wt Owned by: jdunck
Component: Core (Other) Version: 1.0
Severity: Normal Keywords: model inheritance, signals, dispatch, proxy, subclass
Cc: idangazit, shadfc, benbest86, alexr, DrMeers, bronger, vlastimil.zima@…, Florian.Sening@…, 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 svetlyak40wt 7 years ago.
"Virtual" behaviour for signal dispatcher.

Download all attachments as: .zip

Change History (29)

Changed 7 years ago by svetlyak40wt

"Virtual" behaviour for signal dispatcher.

comment:1 Changed 7 years ago by mtredinnick

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Triage Stage changed from Unreviewed to 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 Changed 7 years ago by telenieko

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

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 7 years ago by idangazit

  • Cc idangazit added

comment:5 Changed 6 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 6 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 6 years ago by alexr

  • Cc alexr 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 5 years ago by DrMeers

  • Cc DrMeers added
  • Keywords proxy subclass added
  • milestone set to 1.3

Same problem with proxy models

comment:9 Changed 5 years ago by bronger

  • Cc bronger added

comment:10 Changed 5 years ago by jdunck

  • Owner changed from nobody to jdunck

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

comment:11 Changed 5 years ago by jdunck

  • Status changed from new to assigned

comment:12 Changed 5 years ago by lrekucki

  • Patch needs improvement set

Ticket gardening.

comment:13 Changed 4 years ago by vzima

  • Cc vlastimil.zima@… added

comment:14 Changed 4 years ago by lukeplant

  • Severity set to Normal
  • Type set to New feature

comment:15 Changed 4 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 follow-up: Changed 4 years ago by jdunck

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 4 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 4 years ago by jacob

  • milestone 1.3 deleted

Milestone 1.3 deleted

comment:19 Changed 4 years ago by Semmel

  • Cc Florian.Sening@… added

comment:20 follow-up: Changed 3 years ago 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.

comment:21 in reply to: ↑ 20 Changed 3 years ago by carljm

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 3 years ago by streeter

  • Cc streeter added

comment:23 Changed 3 years ago by charettes

  • Cc charette.s@… added

comment:24 Changed 3 years ago by jdunck

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

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

  • Cc brooks.travis@… added

comment:26 Changed 2 years ago by loic84

  • Cc loic@… added

comment:27 Changed 8 months ago by torstenrudolf

Is there any update on this issue?

comment:28 Changed 8 months ago by charettes

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

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