[Django] #17688: No m2m_changed signal sent to when referenced object is deleted
Django
----------------------------------------------+--------------------
Reporter: jblaine@… | Owner: nobody
Type: Bug | Status: new
Component: Database layer (models, ORM) | Version: 1.3
Severity: Normal | Keywords:
Triage Stage: Unreviewed | Has patch: 0
Easy pickings: 0 | UI/UX: 0
----------------------------------------------+--------------------
{{{
class Topping(models.Model):
name = models.CharField()
class Pizza(models.Model):
name = models.CharField()
toppings = models.ManyToManyField(Topping, null=true, blank=true)
}}}
And this data established using those models:
{{{
TOPPING
id=1, name="Pepperoni"
id=2, name="Onion"
id=3, name="Mushroom"
PIZZA
id=1, name="foopizza"
toppings=1,2,3
}}}
1. Deleting any Topping object (for example, deleting id=1,
name="Pepperoni") also removes it from foopizza.toppings. '''GOOD'''
2. No m2m_changed signal is sent when 1 (above happens). '''BAD?'''
--
Ticket URL: <https://code.djangoproject.com/ticket/17688>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
Type: Bug | Status: new
(models, ORM) | Resolution:
Severity: Normal | Triage Stage: Accepted
Keywords: | Needs documentation: 0
Has patch: 0 | Patch needs improvement: 0
Needs tests: 0 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Changes (by akaariai):
* cc: anssi.kaariainen@… (added)
* needs_better_patch: => 0
* needs_docs: => 0
* needs_tests: => 0
* stage: Unreviewed => Accepted
Comment:
I confirmed that no m2m_changed signal is sent. It seems it would be
somewhat easy to send m2m_changed signals from models/deletion.py. I have
no clue what the correct arguments for all the m2m_signal args should be.
The attached test shows that no signal is sent.
--
Ticket URL: <https://code.djangoproject.com/ticket/17688#comment:1>
Django
Type: Bug | Status: new
(models, ORM) | Resolution:
Severity: Normal | Triage Stage: Accepted
Keywords: | Needs documentation: 0
Has patch: 0 | Patch needs improvement: 0
Needs tests: 0 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Comment (by akaariai):
jblaine: could you (or somebody else) provide the details what arguments
do you expect the signal to send in this case? (I am talking about the
pk_list, model, action, ...) arguments. I guess the right answer is the
same as what you would send in tobbing.pizza_set.clear() case. However
maybe it would be better to send all the deleted toppings in one batch
(which seems to be possible from the delete code), so that you could get
"all the pizzas for which toppings were removed" more effectively.
I am not 100% sure if the delete code really allows easily sending the
m2m_changed signal. But it seems like it.
--
Ticket URL: <https://code.djangoproject.com/ticket/17688#comment:2>
Django
Type: Bug | Status: new
(models, ORM) | Resolution:
Severity: Normal | Triage Stage: Accepted
Keywords: | Needs documentation: 0
Has patch: 0 | Patch needs improvement: 0
Needs tests: 0 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Comment (by anonymous):
Being pretty new, I don't know that I am qualified to say. I just
expected it to work the same as what the following scenario would cause to
happen:
{{{
from django.db.models.signals import m2m_changed
def some_callback(sender, **kwargs):
# custom code
m2m_changed.connect(some_callback, sender=Pizza.toppings.through)
}}}
{{{
foopizza.toppings.remove(ToppingObjectHere)
}}}
Because, in effect, that's what is happening for each Pizza with a
relationship to the Topping that was deleted.
I think anything other than that will cause a new special case to document
("Note: when deleting an object referenced by a ManyToManyField,
m2m_changed works as follows...").
No?
--
Ticket URL: <https://code.djangoproject.com/ticket/17688#comment:3>
Django
Type: Bug | Status: new
(models, ORM) | Resolution:
Severity: Normal | Triage Stage: Accepted
Keywords: | Needs documentation: 0
Has patch: 0 | Patch needs improvement: 0
Needs tests: 0 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
* cc: gamesbook (added)
--
Ticket URL: <https://code.djangoproject.com/ticket/17688#comment:4>
Django
Reporter: jblaine@… | Owner: nobody
Type: Bug | Status: new
(models, ORM) | Resolution:
Severity: Normal | Triage Stage: Accepted
Keywords: | Needs documentation: 0
Has patch: 0 | Patch needs improvement: 0
Needs tests: 0 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Comment (by jblaine):
As we only use Django for 1 internal site, are not Django-internals savvy,
and I don't have the time to become Django-internals savvy, I have instead
made a personal $25 donation to Django Project made in support of getting
this straightened out properly in Django. That's all I can offer aside
from the suggestion that this ticket among several others shows that, IMO,
the overall signals stuff perhaps has not been looked at with a "Forget
what's there for a second. What's the RIGHT thing to be doing in designing
the signals stuff and addressing the various tickets that point out
original design shortsightedness." mindset.
Thank you for your consideration.
--
Ticket URL: <https://code.djangoproject.com/ticket/17688#comment:5>
Django
Reporter: jblaine@… | Owner: nobody
Type: Bug | Status: new
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by abhaga):
#24738 closed as duplicate.
--
Ticket URL: <https://code.djangoproject.com/ticket/17688#comment:6>
Django
Reporter: jblaine@… | Owner: nobody
Type: Bug | Status: new
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by No-0n3):
* cc: No-0n3 (added)
Comment:
Is there any plans on fixing this bug in the coming version?
--
Ticket URL: <https://code.djangoproject.com/ticket/17688#comment:7>
Django
Reporter: jblaine@… | Owner: nobody
Type: Bug | Status: new
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by timgraham):
Given the most recent activity was 2 years ago, it doesn't appear anyone
is working on it.
--
Ticket URL: <https://code.djangoproject.com/ticket/17688#comment:8>
Django
Reporter: jblaine@… | Owner:
| jorgecarleitao
Type: Bug | Status: assigned
Component: Database layer | Version: 1.3
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by jorgecarleitao):
* status: new => assigned
* owner: nobody => jorgecarleitao
* has_patch: 0 => 1
* cc: jorgecarleitao@… (added)
Comment:
Created PR for this issue: https://github.com/django/django/pull/5505
I'm not convinced that using `getattr` and an import in the function is
the best solution, but I didn't find a better option so far.
--
Ticket URL: <https://code.djangoproject.com/ticket/17688#comment:9>
Django
Reporter: jblaine@… | Owner:
| jorgecarleitao
Type: Bug | Status: assigned
Component: Database layer | Version: 1.3
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by timgraham):
* needs_better_patch: 0 => 1
Comment:
Left some comments for improvement on the PR.
--
Ticket URL: <https://code.djangoproject.com/ticket/17688#comment:10>
Django
Reporter: jblaine@… | Owner:
| jorgecarleitao
Type: Bug | Status: assigned
Component: Database layer | Version: 1.3
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Changes (by bronger):
* cc: bronger@… (added)
--
Ticket URL: <https://code.djangoproject.com/ticket/17688#comment:11>
Django
Reporter: jblaine@… | Owner:
| jorgecarleitao
Type: Bug | Status: assigned
Component: Database layer | Version: 1.3
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by jorgecarleitao):
* needs_better_patch: 1 => 0
Comment:
New PR with suggestions from old PR implemented:
https://github.com/django/django/pull/6579
--
Ticket URL: <https://code.djangoproject.com/ticket/17688#comment:12>
Django
Reporter: jblaine@… | Owner:
| jorgecarleitao
Type: Bug | Status: assigned
Component: Database layer | Version: 1.3
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by timgraham):
* needs_better_patch: 0 => 1
Comment:
Left some more comments for improvement.
--
Ticket URL: <https://code.djangoproject.com/ticket/17688#comment:13>
Django
Reporter: jblaine@… | Owner:
| jorgecarleitao
Type: Bug | Status: assigned
Component: Database layer | Version: 1.3
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Changes (by Adam Wróbel):
* cc: adam@… (added)
Comment:
I've just been bitten by this issue and would be interested in helping
merge this PR into master. Otherwise I will need to work around the issue
in my app.
--
Ticket URL: <https://code.djangoproject.com/ticket/17688#comment:14>
Django
Reporter: jblaine@… | Owner:
| jorgecarleitao
Type: Bug | Status: assigned
Component: Database layer | Version: 1.3
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Comment (by Tim Graham):
With no action on the pull request in some months, feel free to claim the
ticket and update the patch as you see fit.
--
Ticket URL: <https://code.djangoproject.com/ticket/17688#comment:15>
Django
Reporter: jblaine@… | Owner:
| jorgecarleitao
Type: Bug | Status: assigned
Component: Database layer | Version: 1.3
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Comment (by Adam Wróbel):
In terms of SQL operations when you delete a referenced object you have
to:
1. Delete the rows from m2m association table to loosen up foreign
constraints
2. Delete the object's row itself
I think this should work the same on the Django side, i.e.:
1. remove the object from m2m set just as you would normally (sending all
the usual notifications)
2. delete the referenced object
@jorgecarleitao has created two new event labels – "pre_delete" and
"post_delete" – but I think they are redundant and we should reuse
"pre_remove" and "post_remove" that we trigger when item is simply removed
from the relation.
Pizza has a set of toppings. Before a topping is deleted it is removed
from all the pizzas. Probably in 99% cases the pizza will not care whether
the topping has been entirely deleted from the store or just removed from
this particular pizza.
Having this new, special event names would complicate code in every
m2m_changed hook. We just want to do something about the pizza now that
this topping is no longer on it: maybe update its price.
In either case the order of events should be:
- send "pre" hook
- delete rows from m2m table
- send "post" hook
- delete the related model
The reason for having "post" hook in between these two SQL DELETEs is is
that user might want to access the related model using its ID inside this
callback, e.g. check the price of the topping before adjusting the price
of the pizza. Just as when the item is simply removed from relation you
could still query it from DB in both hooks.
--
Ticket URL: <https://code.djangoproject.com/ticket/17688#comment:16>
Django
Reporter: jblaine@… | Owner:
| jorgecarleitao
Type: Bug | Status: assigned
Component: Database layer | Version: 1.3
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Comment (by Stephen Wolff):
I can see `pre_remove` and `post_remove` now in the `m2m_changed`
documentation:
- https://docs.djangoproject.com/en/4.1/ref/signals/#m2m-changed
Does that mean this might have been fixed as part of a feature addition or
something?
--
Ticket URL: <https://code.djangoproject.com/ticket/17688#comment:17>
Django
Reporter: jblaine@… | Owner:
| jorgecarleitao
Type: Bug | Status: assigned
Component: Database layer | Version: 1.3
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Comment (by ITJamie):
pretty sure this is still an active issue. it was reported on
https://github.com/netbox-community/netbox/issues/14079 when change_log
didn't log changes of deletes on related m2m objects.
--
Ticket URL: <https://code.djangoproject.com/ticket/17688#comment:18>
Django
Reporter: jblaine@… | Owner:
| jorgecarleitao
Type: Bug | Status: assigned
Component: Database layer | Version: 1.3
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Comment (by Gary Chen):
Interested in taking on this change. I'm thinking it'll involve:
* Undoing [26c4be2ebe] to disable fast_deletes when m2m_changed signals
are connected
* Took at look at [https://github.com/django/django/pull/6579] but I'm not
sure the approach there is correct, the hidden through model is already
included and cascaded in the collector. We just need to fire the correct
signal in that case, and we can do so easily by checking
`model._meta.auto_created`
[https://github.com/django/django/blob/a97d6b198eec13a98ef60d7f440528ba82889071/django/db/models/deletion.py#L327
which is already being done].
One question is if `model._meta.auto_created` is indeed a determiner for
if a model is a generated m2m through model--I'm assuming it's already
being used as such to skip pre/post_delete signals.
There is a design question I think whether the `m2m_changed` signal should
only be triggered explicitly via methods on ManyRelatedManager, or any
changes to the autogenerated m2m model should in fact be captured. I think
there's precedent with deletes at least, in that cascading deletes trigger
pre/post_delete signals. So it's not a far stretch that deleted m2m
relations trigger m2m_changed signals.
--
Ticket URL: <https://code.djangoproject.com/ticket/17688#comment:19>