[Django] #33505: Inconsistent Behavior with Abstract Models and Signals
Django
-------------------------------------+-------------------------------------
Reporter: Ken | Owner: nobody
Weaver |
Type: Bug | Status: new
Component: Database | Version: 3.2
layer (models, ORM) |
Severity: Normal | Keywords:
Triage Stage: | Has patch: 0
Unreviewed |
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 0
UI/UX: 0 |
-------------------------------------+-------------------------------------
Recently we came across an issue with abstract models and signals,
specifically many to many signals. It seems that for most signals, models
that have inherited from an abstract class (or perhaps any class) will not
also inherit the signals of that class. This is not unexpected. However,
for `m2m_changed` signals, the signal '''fires for every many to many
change in the entire application'''. I have created a minimum (ish)
working example [https://github.com/kenjones21/django_signal_bug here],
and I will paste the relevant code below as well.
I think ideally django would stop you from binding signals to abstract
models if they can't ever work, or spat out a warning when it was
attempted. If that isn't possible, never firing the signal would be
preferable to always firing the signal; the latter is highly likely to
break parts of your application you weren't testing.
Admittedly, I don't think the documentation ever specifies what the
behavior should be related to abstract/inherited models and signals. If
none of the changes here are possible, I think a warning in the
documentation on the model page, the signals page, or both would be
helpful, and I would be more than happy to work on that.
{{{
class Related(models.Model):
pass
class Abstract(models.Model):
m2m_field = models.ManyToManyField(to=Related)
int_field = models.IntegerField(default=0)
class Meta:
abstract = True
class OtherRelated(models.Model):
pass
class Other(models.Model):
m2m_field = models.ManyToManyField(to=OtherRelated)
@receiver(m2m_changed, sender=Abstract.m2m_field.through)
def abstract_m2m_signal(sender, **kwargs):
wrapped_abstract_m2m(sender, **kwargs)
}}}
{{{
@patch('app.models.wrapped_abstract_m2m', side_effect=mock_signal)
def test_abstract_m2m_signal_not_called(self, signal_mock):
other = Other.objects.create()
related = OtherRelated.objects.create()
other.m2m_field.add(related)
self.assertEqual(signal_mock.call_count, 0) # this fails
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/33505>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
-------------------------------------+-------------------------------------
Type: Bug | Status: closed
Component: Database layer | Version: 3.2
(models, ORM) |
Severity: Normal | Resolution: invalid
Keywords: | Triage Stage:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* status: new => closed
* resolution: => invalid
Comment:
Thanks for the report. You can create custom signals and send them from
anywhere, so I don't think any validation of a `sender` is feasible here.
When connecting the receivers,
we don't know if the code sending the signals is reachable. For example,
folks can send built-in signals from abstract models of their own, and
this will work fine. `pre_save`, `post_save`, `pre_delete`, and
`post_delete` signals connected to an abstract models will never be send
(without custom senders) because `save()` and `delete()` methods are not
called on abstract models. I'm afraid that any additional note in docs
will confuse folks even more, however I'd be happy to review your concrete
proposal.
--
Ticket URL: <https://code.djangoproject.com/ticket/33505#comment:1>
Django
-------------------------------------+-------------------------------------
Type: Bug | Status: closed
Component: Database layer | Version: 3.2
(models, ORM) |
Severity: Normal | Resolution: invalid
Keywords: | Triage Stage:
| Unreviewed
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Ken Weaver):
Thanks for the response. The inability to validate sender makes sense.
However, it does still strike me as strange that `sender` is essentially
ignored in the example I gave; is that truly intended behavior?
Replying to [comment:1 Mariusz Felisiak]:
> Thanks for the report. You can create custom signals and send them from
anywhere, so I don't think any validation of a `sender` is feasible here.
When connecting the receivers,
> we don't know if the code sending the signals is reachable. For example,
folks can send built-in signals from abstract models of their own, and
this will work fine. `pre_save`, `post_save`, `pre_delete`, and
`post_delete` signals connected to an abstract models will never be send
(without custom senders) because `save()` and `delete()` methods are not
called on abstract models. I'm afraid that any additional note in docs
will confuse folks even more, however I'd be happy to review your concrete
proposal.
--
Ticket URL: <https://code.djangoproject.com/ticket/33505#comment:2>
Django
-------------------------------------+-------------------------------------
Type: Bug | Status: closed
Component: Database layer | Version: 3.2
(models, ORM) |
Severity: Normal | Resolution: invalid
Keywords: | Triage Stage:
| Unreviewed
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Mariusz Felisiak):
> However, it does still strike me as strange that sender is essentially
ignored in the example I gave; is that truly intended behavior?
`sender` is not ignored, see my comment:
> `pre_save`, `post_save`, `pre_delete`, and `post_delete` signals
connected to an abstract models will never be send (without custom
senders) because `save()` and `delete()` methods are not called on
abstract models.
So these signals are not send for abstract models by Django itself.
However, users can send them manually, for example by overwriting `save()`
in a child model:
{{{
def save(
self, force_insert=False, force_update=False, using=None,
update_fields=None
):
pre_save.send(sender=Parent, ...)
super().save(...)
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/33505#comment:3>
Django
-------------------------------------+-------------------------------------
Type: Bug | Status: closed
Component: Database layer | Version: 3.2
(models, ORM) |
Severity: Normal | Resolution: invalid
Keywords: | Triage Stage:
| Unreviewed
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Ken Weaver):
Yes I understand, but as the code above demonstrates, when adding a
receiver for a `m2m_changed` signal and `Abstract.m2m_field.through` field
as the sender, the code operates the same as if the sender was `None`.
This, not the fact that `pre_save`, `post_save` etc. signals are not
automatically sent for abstract models, is the peculiar behavior that led
to this ticket.
--
Ticket URL: <https://code.djangoproject.com/ticket/33505#comment:4>
Django
-------------------------------------+-------------------------------------
Type: Bug | Status: closed
Component: Database layer | Version: 3.2
(models, ORM) |
Severity: Normal | Resolution: invalid
Keywords: | Triage Stage:
| Unreviewed
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Mariusz Felisiak):
> but as the code above demonstrates, when adding a receiver for a
`m2m_changed` signal and `Abstract.m2m_field.through` field as the sender,
the code operates the same as if the sender was `None`.
`Abstract` is not used in models from the ticket description🤔, it's an
abstract model that is not inherited from any concrete model. I'm puzzled.
--
Ticket URL: <https://code.djangoproject.com/ticket/33505#comment:5>
Django
-------------------------------------+-------------------------------------
Type: Bug | Status: closed
Component: Database layer | Version: 3.2
(models, ORM) |
Severity: Normal | Resolution: invalid
Keywords: | Triage Stage:
| Unreviewed
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Ken Weaver):
Yes it seemed very strange to me too, I thought the signal should be
received either when an inherited model's `ManyToManyField` changed (this
was my mistake) or not at all. But instead it is received for every
`m2m_changed`, even for completely unrelated models.
I now understand why; I just checked and `Abstract.m2m_field.through`
itself is actually `None`. This makes sense (somewhat) since of course you
can't have a concrete many to many table for an abstract model. I would
think one could change `ManyToManyField` to itself be some kind of
abstract model or a dummy class when attached to an abstract model, but
you will know more about the inners of Django to know if something like
that is feasible or justified.
Replying to [comment:5 Mariusz Felisiak]:
> `Abstract` is not used in models from the ticket description🤔, it's an
abstract model that is not inherited from any concrete model. I'm puzzled.
--
Ticket URL: <https://code.djangoproject.com/ticket/33505#comment:6>
Django
-------------------------------------+-------------------------------------
Type: Bug | Status: closed
Component: Database layer | Version: 3.2
(models, ORM) |
Severity: Normal | Resolution: invalid
Keywords: | Triage Stage:
| Unreviewed
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Mariusz Felisiak):
Replying to [comment:6 Ken Weaver]:
> I now understand why; I just checked and `Abstract.m2m_field.through`
itself is actually `None`. This makes sense (somewhat) since of course you
can't have a concrete many to many table for an abstract model.
Ahh, yes. You can set `sender` to `None` to receive events from any
sender.
> I would think one could change `ManyToManyField` to itself be some kind
of abstract model or a dummy class when attached to an abstract model, but
you will know more about the inners of Django to know if something like
that is feasible or justified.
I don't think it is feasible.
--
Ticket URL: <https://code.djangoproject.com/ticket/33505#comment:7>