Decisions needed on signals for add/remove/clear on many-to-many relations

[rvdrijst]

I know there has already been some discussion about adding signals to
the ManyRelatedManager so that add(), remove() and clear() on m2m
relations emit signals. This functionality, [ORM-18] in the v1.1
roadmap, would complement the behavior of pre/post_save and pre/
post_delete very nicely and provide a easy and transparent way to
track changes to m2m relationships.
Since I really like to have this functionality for a project I am
working on, I'm trying to push this feature for v1.1alpha. However,
some design decisions need to be made, which is why I am starting this
discussion.

The ticket for this functionality is http://code.djangoproject.com/ticket/5390
which has a patch that works to some extent. It adds three signals:
m2m_add_items, m2m_remove_items and m2m_clear_items. Extra arguments
to the signal are the instance on which the field is accessed, the
field_name of the m2m field and the list of added/removed objects.
The main problem with the patch is the unintuitive sender of signals.
The signal is sent by the _related_ model, not the model containing
the ManyToManyField. For example:

class Car(models.Model)
parts = models.ManyToManyField('Part')

class Part(models.Model)
pass

Now if we want to handle the signal sent by some_car.parts.add
(some_part), we would have to connect the handler to Part:
signals.m2m_add_items(add_handler_func, Part)
To me, this is not very intuitive. I would like to connect to Car,
since that model has the 'parts' field, so this is something I'm
suggesting to change in the patch (not that hard to do).

Another issue that has come up while playing around with these
signals, is that only one end of the relation is handled by one
signal. So if you want to handle the reverse relation of 'parts',
some_part.car_set.add(some_car), you will have to connect to the Car
(or Part if the previously suggested change is made). But since this
is actually a change on the same relationship, in this case between
Cars and Parts, it might make sense to have these changes on the
reverse side _also_ be sent by the same sender (Car, ideally), with an
extra parameter 'reverse' that is True for changes to the reverse
side, indicating that not Parts are added to a Car, but Cars are added
to a Part. If you only want to handle one side, you can simply ignore
the case for reverse=True or False. I'm not sure if this is a good
solution, so any thoughts are welcome.

An issue with the current patch regarding the two sides of the
relation and the signals they sent, is that _both_ sides send the same
'field_name' argument to the signal. I would say that if you have two
signals, one for each side, then each side should send the appropriate
field_name: so 'parts' for one side and 'car_set' or a specific
related_name for the other side.

A final problem with the current implementation and one to which I do
not have an answer, is an inconsistency in the signal definitions and
emitting moments. Both the add_m2m_items and remove_m2m_items are
emitted after the changes have been applied (cursor.execute) and
include the list of added/removed objects in the 'objs' argument to
the signal. The clear_m2m_items however, is emitted _before_ the
cursor.execute is called, and contains no 'objs' argument. So with add
() and remove() the database is already in the changed state, while
with clear() the database is still in the state before clear. The
problem however is that if you emit the clear signal after the change,
there is no way of telling which objects have been removed from the
relationship without making an extra call to get these objects prior
to clearing the relation. The question is, is this extra call worth
it, when the result is only ever used by a signal handler. Or should
there be a pre/post variant for this signal that so handlers can save
this list on pre, and handle the changes after the database is up-to-
date?

I am willing to write a better patch, tests and documentation for this
feature, but before putting in a lot of time and effort, I would
appreciate some feedback. So, ideas anyone? These are the main issues:
* sender of signal, currently related model, should be model (so
some_car.parts.add() will be sent by Car). Ok?
* changes to reverse side of relation sent by same sender (both Car
in the example) with extra reverse=True/False?
* Should the field_name be the 'related_name'/<model_name>_set for
reverse side?
* difference between add/remove and clear signals

-Robin

[Malcolm Tredinnick]

I don't have answers to all the issues you raise in this message, but I
agree with you here. The ManyToMany is part of Car, so that should be
the model that is passed as the model, if at all possible. You might
want to consider passing the other end's model as well, if you can. The
current parameters you list make it sound like it would be fiddly to
work out which m2m was changed on a model that contains more than one of
them. However that washes out in the end, we need to be able to do that.

I also suspect that three signals is about three times more than the
ideal number. A "changed" signal that accepts a parameter for the action
(added, removed, cleared) and a list of what was added or removed in the
former two cases would cut down on the number of new signals that need
to be added. I don't feel too uncomfortable with the overloading of
purpose here, since the signal is really handling "changes to the m2m
field" and the parameters are just identifying variations on that.

Regards,
Malcolm

[rvdrijst]

I recently found the time to create a better patch for the m2m
signals, including some tests and documentation and have posted this
patch on http://code.djangoproject.com/ticket/5390
I have taken into account Malcom's suggestions resulting in one signal
called m2m_changed that has the following arguments:

sender
The model class containing the ManyToManyField
instance
The instance whose many-to-many relation is updated.
action
A string indicating the type of update that is done on the
relation: "add", "remove" or "clear".
model
The class of the objects that are added to, removed from or cleared
from the relation.
field_name
The name of the ManyToManyField in the sender class, can be used in
case of multiple m2m fields to the same model.
reverse
False if the 'normal' side of the relation is updated, True if the
reverse side (i.e. on the related model) is used.
objects
With the "add" and "remove" action, this is a list of objects that
have been added to or removed from the relation. The class of these
objects is given in the model argument. For the "clear" action, this
is None .

The signal is emitted _after_ the change for the "add" and "remove"
action, and _before_ the change for the "clear" action.

For an example see the docs and tests in the patch, suggestions for
improvements are of course still appreciated, so please discuss!

Hopefully this will speed up inclusion of this feature into django.

Regards,
-Robin

On Dec 21 2008, 6:26 am, Malcolm Tredinnick <malc...@pointy-stick.com>
wrote:

> On Fri, 2008-12-19 at 08:42 -0800, rvdrijst wrote:
> > I know there has already been some discussion about addingsignalsto
> > the ManyRelatedManager so that add(), remove() and clear() on m2m

> > relations emitsignals. This functionality, [ORM-18] in the v1.1

> > roadmap, would complement the behavior of pre/post_save and pre/
> > post_delete very nicely and provide a easy and transparent way to
> > track changes to m2m relationships.
> > Since I really like to have this functionality for a project I am
> > working on, I'm trying to push this feature for v1.1alpha. However,
> > some design decisions need to be made, which is why I am starting this
> > discussion.
>

> > The ticket for this functionality ishttp://code.djangoproject.com/ticket/5390

> > which has a patch that works to some extent. It adds threesignals:
> > m2m_add_items, m2m_remove_items and m2m_clear_items. Extra arguments
> > to the signal are the instance on which the field is accessed, the
> > field_name of the m2m field and the list of added/removed objects.
> > The main problem with the patch is the unintuitive sender ofsignals.
> > The signal is sent by the _related_ model, not the model containing
> > the ManyToManyField. For example:
>
> > class Car(models.Model)
> >     parts = models.ManyToManyField('Part')
>
> > class Part(models.Model)
> >     pass
>
> > Now if we want to handle the signal sent by some_car.parts.add
> > (some_part), we would have to connect the handler to Part:
> >signals.m2m_add_items(add_handler_func, Part)
> > To me, this is not very intuitive. I would like to connect to Car,
> > since that model has the 'parts' field, so this is something I'm
> > suggesting to change in the patch (not that hard to do).
>
> I don't have answers to all the issues you raise in this message, but I
> agree with you here. The ManyToMany is part of Car, so that should be
> the model that is passed as the model, if at all possible. You might
> want to consider passing the other end's model as well, if you can. The
> current parameters you list make it sound like it would be fiddly to
> work out which m2m was changed on a model that contains more than one of
> them. However that washes out in the end, we need to be able to do that.
>

> I also suspect that threesignalsis about three times more than the

> ideal number. A "changed" signal that accepts a parameter for the action
> (added, removed, cleared) and a list of what was added or removed in the

> former two cases would cut down on the number of newsignalsthat need