Opened 13 years ago
Last modified 8 months ago
#17688 assigned Bug
No m2m_changed signal sent to when referenced object is deleted
Reported by: | Owned by: | jorgecarleitao | |
---|---|---|---|
Component: | Database layer (models, ORM) | Version: | 1.3 |
Severity: | Normal | Keywords: | |
Cc: | anssi.kaariainen@…, Derek Hohls, No-0n3, jorgecarleitao@…, bronger@…, adam@… | Triage Stage: | Accepted |
Has patch: | yes | Needs documentation: | no |
Needs tests: | no | Patch needs improvement: | yes |
Easy pickings: | no | UI/UX: | no |
Description
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
- Deleting any Topping object (for example, deleting id=1, name="Pepperoni") also removes it from foopizza.toppings. GOOD
- No m2m_changed signal is sent when 1 (above happens). BAD?
Attachments (1)
Change History (20)
comment:1 by , 13 years ago
Cc: | added |
---|---|
Triage Stage: | Unreviewed → Accepted |
by , 13 years ago
Attachment: | 17688.diff added |
---|
comment:2 by , 13 years ago
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.
comment:3 by , 13 years ago
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?
comment:4 by , 13 years ago
Cc: | added |
---|
comment:5 by , 11 years ago
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.
comment:8 by , 10 years ago
Given the most recent activity was 2 years ago, it doesn't appear anyone is working on it.
comment:9 by , 9 years ago
Cc: | added |
---|---|
Has patch: | set |
Owner: | changed from | to
Status: | new → assigned |
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.
comment:10 by , 9 years ago
Patch needs improvement: | set |
---|
Left some comments for improvement on the PR.
comment:11 by , 8 years ago
Cc: | added |
---|
comment:12 by , 8 years ago
Patch needs improvement: | unset |
---|
New PR with suggestions from old PR implemented: https://github.com/django/django/pull/6579
comment:14 by , 8 years ago
Cc: | added |
---|
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.
comment:15 by , 8 years ago
With no action on the pull request in some months, feel free to claim the ticket and update the patch as you see fit.
comment:16 by , 8 years ago
In terms of SQL operations when you delete a referenced object you have to:
- Delete the rows from m2m association table to loosen up foreign constraints
- Delete the object's row itself
I think this should work the same on the Django side, i.e.:
- remove the object from m2m set just as you would normally (sending all the usual notifications)
- 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.
comment:17 by , 2 years ago
I can see pre_remove
and post_remove
now in the m2m_changed
documentation:
Does that mean this might have been fixed as part of a feature addition or something?
comment:18 by , 10 months ago
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.
comment:19 by , 8 months ago
Interested in taking on this change. I'm thinking it'll involve undoing [26c4be2ebe] to disable fast_deletes when m2m_changed signals are connected on top of https://github.com/django/django/pull/6579.
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.
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.