[Django] #34733: m2m_changed signal is unaware if .set() method is being called
Django
-------------------------------------+-------------------------------------
Reporter: lekjos | Owner: nobody
Type: New | Status: new
feature |
Component: Database | Version: dev
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 |
-------------------------------------+-------------------------------------
When a `related_m2m_field.set()` method is called, the `m2m_changed`
signal is fired four times for action: `pre_remove`, `post_remove`,
`pre_add`, and `post_add` (or `pre_clear` / `post_clear`).
This poses a problem if the signal is supposed to run validation on the
final state of the model. For example, let's say I have a model called
`Customer` with a many-to-many relation to `SubscriptionPlan` and I have a
`m2m_changed` signal that is supposed to validate if their
SubscriptionPlan is valid.
If I call `subscription_plans.set([some_plans])`, the many-to-many manager
will first call `subscription_plans.remove()` then `.add()` inside an
atomic transaction. If my signal validates on the first `remove()`, it
could be in an invalid state, even though it won't be by the time the
`.add()` completes.
To get around this, I had to create a custom ManyToManyField with a custom
RelatedManager that set an instance variable on the model when the
`.set()` method was called. I'd like to propose adding a feature to the
RelatedManager or to the signals to make the `m2m_changed` signal aware of
if the `.set()` method was called when running. This could be a private
attribute on the instance or extra information passed to the signal
receiver.
If this feature already exists or there's a decent workaround, let me know
and i'll close the ticket! Otherwise, I have a patch in mind that I can
raise a PR for and attach.
--
Ticket URL: <https://code.djangoproject.com/ticket/34733>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
-------------------------------------+-------------------------------------
Type: New feature | Status: assigned
Component: Database layer | Version: dev
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* owner: nobody => lekjos
* status: new => assigned
Old description:
> When a `related_m2m_field.set()` method is called, the `m2m_changed`
> signal is fired four times for action: `pre_remove`, `post_remove`,
> `pre_add`, and `post_add` (or `pre_clear` / `post_clear`).
>
> This poses a problem if the signal is supposed to run validation on the
> final state of the model. For example, let's say I have a model called
> `Customer` with a many-to-many relation to `SubscriptionPlan` and I have
> a `m2m_changed` signal that is supposed to validate if their
> SubscriptionPlan is valid.
>
> If I call `subscription_plans.set([some_plans])`, the many-to-many
> manager will first call `subscription_plans.remove()` then `.add()`
> inside an atomic transaction. If my signal validates on the first
> `remove()`, it could be in an invalid state, even though it won't be by
> the time the `.add()` completes.
>
> To get around this, I had to create a custom ManyToManyField with a
> custom RelatedManager that set an instance variable on the model when the
> `.set()` method was called. I'd like to propose adding a feature to the
> RelatedManager or to the signals to make the `m2m_changed` signal aware
> of if the `.set()` method was called when running. This could be a
> private attribute on the instance or extra information passed to the
> signal receiver.
>
> If this feature already exists or there's a decent workaround, let me
> know and i'll close the ticket! Otherwise, I have a patch in mind that I
> can raise a PR for and attach.
New description:
When a `related_m2m_field.set()` method is called, the `m2m_changed`
signal is fired four times for action: `pre_remove`, `post_remove`,
`pre_add`, and `post_add` (or `pre_clear` / `post_clear`).
This poses a problem if the signal is supposed to run validation on the
final state of the model. For example, let's say I have a model called
`Customer` with a many-to-many relation to `SubscriptionPlan` and I have a
`m2m_changed` signal that is supposed to validate if their
SubscriptionPlan is valid.
If I call `Customer.subscription_plans.set([some_plans])`, the many-to-
many manager will first call `subscription_plans.remove()` then `.add()`
inside an atomic transaction. If my signal validates on the first
`.remove()`, it could be in an invalid state, even though it won't be by
the time the `.add()` completes. However, I still need to be able to
validate on the signal if the standard `.remove()` method is called.
To get around this, I had to create a custom ManyToManyField with a custom
RelatedManager that set an instance variable on the model when the
`.set()` method was called. I'd like to propose adding a feature to the
RelatedManager or to the signals to make the `m2m_changed` signal aware of
if the `.set()` method was called when running. This could be a private
attribute on the instance or extra information passed to the signal
receiver.
If this feature already exists or there's a decent workaround, let me know
and i'll close the ticket! Otherwise, I have a patch in mind that I can
raise a PR for and attach.
--
--
Ticket URL: <https://code.djangoproject.com/ticket/34733#comment:1>
Django
-------------------------------------+-------------------------------------
Type: New feature | Status: closed
Component: Database layer | Version: dev
(models, ORM) |
Keywords: | Triage Stage:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* status: assigned => closed
* resolution: => wontfix
Comment:
Thanks for this ticket, however, signals are the last resort, required in
really rare cases and could probably be avoided in yours. We don't want to
add new features to them. Personally, I'd use a custom intermediate
models, and add this logic to the `.create()` or `.save()` methods.
Please see TicketClosingReasons/UseSupportChannels for ways to get help
with Django usage, where folks can help you in re-designing this
validation.
You can start a discussion on DevelopersMailingList if you don't agree,
where you'll reach a wider audience and see what other think, and
[https://docs.djangoproject.com/en/stable/internals/contributing/triaging-
tickets/#closing-tickets follow triaging guidelines with regards to
wontfix tickets.]
--
Ticket URL: <https://code.djangoproject.com/ticket/34733#comment:2>
Django
-------------------------------------+-------------------------------------
Reporter: Leif Kjos | Owner: Leif Kjos
Type: New feature | Status: closed
Component: Database layer | Version: dev
(models, ORM) |
Severity: Normal | Resolution: wontfix
Keywords: | Triage Stage:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Leif Kjos):
Replying to [comment:2 Mariusz Felisiak]:
> Thanks for this ticket, however, signals are the last resort, required
in really rare cases and could probably be avoided in yours. We don't want
to add new features to them. Personally, I'd use a custom intermediate
models, and add this logic to the `.create()` or `.save()` methods.
>
> Please see TicketClosingReasons/UseSupportChannels for ways to get help
with Django usage, where folks can help you in re-designing this
validation.
>
> You can start a discussion on DevelopersMailingList if you don't agree,
where you'll reach a wider audience and see what other think, and
[https://docs.djangoproject.com/en/stable/internals/contributing/triaging-
tickets/#closing-tickets follow triaging guidelines with regards to
wontfix tickets.]
Thanks for the response! Will try that.
--
Ticket URL: <https://code.djangoproject.com/ticket/34733#comment:3>