Opened 14 years ago

Last modified 4 years ago

#13757 new Bug

Signal inconsistency between auto_created and manually defined intermediate models for m2m fields

Reported by: George Sakkis Owned by: nobody
Component: Database layer (models, ORM) Version: dev
Severity: Normal Keywords: m2m signals
Cc: Yiling-J Triage Stage: Accepted
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

In Django 1.2 the the signal handling for m2m fields differs based on whether the intermediate model is auto_created or defined manually. As per http://groups.google.com/group/django-developers/msg/2aa826b200e60294:

  • For m2m fields with auto_created intermediate model, calling Model.m2m_field.add/remove/clear sends m2m_changed. However adding/removing relations using directly the through model (e.g. Model.m2m_field.through.objects.create(...)) does not send m2m_changed. Adding handlers for pre/post_save, pre/post_delete on the through model has no effect; the code specifically checks for auto_created models and does not send these signals in this case.
  • For m2m fields with a given intermediate model, Model.m2m_field.add/remove are not exposed (at least for now but this may change, see #9475) and therefore m2m_changed is not sent; instead you have to handle addition/removal in pre/post_save, pre/post_delete handlers on the through model. However Model.m2m_field.clear() is exposed and does send m2m_changed; if there are pre/post_delete handlers on the through model, they are called as well after the m2m_changed.

Change History (7)

comment:1 by George Sakkis, 14 years ago

After looking into it more closely, I think it's harder to do right than I thought initially. Basically, my idea is to have all changes related to m2m fields sending (one or more) m2m_changed signals, and nothing else, regardless of (a) whether the intermediate model is created automatically or not, and (b) whether the change is applied through the m2m descriptor or directly through the intermediate model. This last one is what makes things interesting. I see three main issues here:

  1. We'd need a way to distinguish intermediate models from regular ones. This could be done with a new 'm2m_through' Meta option. This could be set automatically, both for auto_created intermediate models (obviously) and for manually provided one; users won't need to set it, or for that matter be aware of it at all.
  2. Modifying an m2m field using the descriptor sends m2m_changed and then calls the underlying save() or delete() of the intermediate model. We don't want to send m2m_changed (or any other signals for that matter) twice, so we'd have to somehow say to save()/delete() to not emit signals. AFAIK this is not possible now; we'd probably need to add a new 'send_signals=True' optional parameter and pass False to prevent the double signals.
  3. m2m_changed has quite different signature from the others. Basically m2m_changed is directional; it distinguishes the source from the targets and whether the modification is applied forward or reverse. The others do not have this information, all they have is the intermediate model's instance.

The last one seems the hardest to overcome. Unless we're willing to cheat and assign arbitrarily the source, the targets and the direction, I don't see a way around it. I'd love to see ideas along this line or an alternative one.

comment:2 by Russell Keith-Magee, 14 years ago

Triage Stage: UnreviewedAccepted

comment:3 by Julien Phalip, 13 years ago

Severity: Normal
Type: Bug

comment:4 by Jacob, 13 years ago

milestone: 1.3

Milestone 1.3 deleted

comment:11 by Aymeric Augustin, 12 years ago

UI/UX: unset

Change UI/UX from NULL to False.

comment:12 by Aymeric Augustin, 12 years ago

Easy pickings: unset

Change Easy pickings from NULL to False.

comment:13 by Yiling-J, 4 years ago

Cc: Yiling-J added
Version: 1.2master

Can we enable post_save and other signals for intermediate model now? Django 1.2 using for loop and create() for m2m add, so post signal will be called many times. But now Django already using bulk_create(), this is not an issue anymore.

Note: See TracTickets for help on using tickets.
Back to Top