Opened 7 years ago

Closed 7 years ago

#27350 closed Bug (needsinfo)

Attaching signals to abstract models don't work as it used to be

Reported by: Florent Messa Owned by: nobody
Component: Database layer (models, ORM) Version: 1.10
Severity: Normal Keywords:
Cc: Loic Bistuer Triage Stage: Unreviewed
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

Hi,

I'm currently porting an application from Django 1.9.x to 1.10.x, we are relying on managers to attach signals to allow developers overriding them in our generic applications, this does not work anymore.

A complete example:

from django.db import models
from django.db.models import signals


class Poll(models.Model):
    question = models.CharField(max_length=255)
    answers_count = models.PositiveIntegerField(default=0)


class AnswerManager(models.Manager):
    def contribute_to_class(self, cls, name):
        signals.post_save.connect(self.post_save, sender=cls)

        return super(AnswerManager, self).contribute_to_class(cls, name)

    def post_save(self, instance, **kwargs):
        poll = instance.poll
        poll.answers_count = poll.answers.count()
        poll.save(update_fields=('answers_count', ))


class AbstractAnswer(models.Model):
    text = models.CharField(max_length=255)
    poll = models.ForeignKey(Poll, related_name='answers')

    objects = AnswerManager()

    class Meta:
        abstract = True


class Answer(AbstractAnswer):
    class Meta:
        abstract = False


class ModelsTests(TestCase):
    def test_simple_compute(self):
        poll = Poll.objects.create(question='Weird?')

        Answer.objects.create(poll=poll, text='Yes')

        # refresh
        poll = Poll.objects.get(pk=poll.pk)

        # still 0 on Django 1.10 and 1 on Django 1.9
        assert poll.answers_count == 1

The complete application is available here and travis is available here to test again both 1.10 and 1.9.

Is this some kind of undocumented new behavior or a bug?

Change History (6)

comment:1 by Tim Graham, 7 years ago

Hi, you should bisect to find the commit where the behavior changed. If I had to guess it might be ed0ff913c648b16c4471fc9a9441d1ee48cb5420 or 3a47d42fa33012b2156bf04058d933df6b3082d2.

in reply to:  1 comment:2 by Florent Messa, 7 years ago

Replying to Tim Graham:

Hi, you should bisect to find the commit where the behavior changed. If I had to guess it might be ed0ff913c648b16c4471fc9a9441d1ee48cb5420 or 3a47d42fa33012b2156bf04058d933df6b3082d2.

It was introduced by 3a47d42fa33012b2156bf04058d933df6b3082d2

Build is available on travis, 110a is the parent commit of 3a47d42fa33012b2156bf04058d933df6b3082d2 and 110b is located at 3a47d42fa33012b2156bf04058d933df6b3082d2

comment:3 by Tim Graham, 7 years ago

Cc: Loic Bistuer added

Looking at your code, I don't think we broke any public APIs, so I'd say it's up to you to investigate and say why it's a bug and to propose a fix. Maybe Loic can advise without too much effort.

in reply to:  3 comment:4 by Florent Messa, 7 years ago

Replying to Tim Graham:

Looking at your code, I don't think we broke any public APIs, so I'd say it's up to you to investigate and say why it's a bug and to propose a fix. Maybe Loic can advise without too much effort.

The contribute_to_class is called twice in 1.9.x on the abstract level and child.

That's not the case on 1.10.x your child model needs to declare explicitly the manager even if it's inherited.

I can update my code on my side on every child to redeclare the manager, as you have said it's not a public API as it's not documented. I can investigate more on my side and provide a fix for that but will you consider it?

Btw, thank you for your reactivity ;)

Last edited 7 years ago by Florent Messa (previous) (diff)

comment:5 by Tim Graham, 7 years ago

I didn't follow the manager changes closely enough to say what the expected behavior is.

comment:6 by Tim Graham, 7 years ago

Resolution: needsinfo
Status: newclosed
Note: See TracTickets for help on using tickets.
Back to Top