I would like to discuss my proposal for a working way to call .add() on an m2m with through= model

185 views
Skip to first unread message

Luis Masuelli

unread,
Mar 20, 2017, 5:46:10 PM3/20/17
to Django developers (Contributions to Django itself)
I was reading this link in the official history and this other link in this group, but still do not understand why the issue against a way to call .add() to add a through model. I thought several approaches (they are all backward compatible for the end-user) that could work:
  1. Avoid the restriction to call add() if the model has only the two FK fields.
  2. An additional way to call .add() specifying additional fields to fill (this one is not mine; was discussed in the linked thread). You risk getting a (wrapped?) exception if you do not populate other fields appropriately.
  3. (This one was the first I thought and perhaps the easiest to implement) Allow the ManyToManyField to specify a kwarg like `through_factory=` which expects a callable which would populate more data. The restriction to call .add() would remain if no `through_factory=` is specified.
  4. Avoid the restriction to call delete() if the model has only the two FK fields. 
I considered these cases:
  • It is quite trivial a model with only two fields, but perhaps the intermediate models could have additional useful methods. I see no trouble having such model and allowing .add() or .delete() calls there.
  • Having a special factory method would allow calls to .add() since we'd be providing a way to make .add() actually know how to create an instance of the intermediate model.
  • You can combine these points, implement one, none, or all of them.
  • (I did not consider extended use cases for delete() intentionally, since perhaps a strange model (with different field pairs) could fit in different relationships, although I cannot think in a case with multiple relationships not incurrin in, perhaps, duplicate data when tried to be used as through= model, but anyway I prefer to keep silence for other cases involving delete(), but the case I stated).
  • It is up to the user to be careful regarding migrating an intermediate model regarding adding, changing, or deleting fields. But anyway, this applies to any modification in the database models right now.
  • Points 1, 2, 3 and/or 4 could be implemented with no clash. A combined approach of 1, 2, 3 would look like this (this flow only applies for the case when the through= is present - such scenario right now only consists of raising an exception; the case with no through= model would not be affected at all):
    • Instantiate `instance = ThroughModel(fka=a, fkb=b, **kwargs_from_add)` with the respective model instances a and b, from classes A and B which hold the desired M2M relationship. In this case, the point 2 just adds the **kwargs_from_add. If point 2 is not implemented, no **kwargs_from_add would be present.
    • (If point 3 is implemented) Call the callable in `through_factory=` invoking it `like_this(instance)`, if the callable is present. It is expected to save the instance.
    • (If either point 1 is implemented and the model has only two fields, or point 2 is implemented) Manually save the instance (if point 3 was not implemented or it was but the factory callable was not specified). (Otherwise - point 2 not implemented AND (point 1 not implemented or model with more than two fields)) Raise the currently implemented exception for the .add() method with through= specified (with a different string message) because the through_factory was not present, and so the framework does not know how to populate additional fields.
    • Catch-and-reraise (or don't catch at all and let them be) the error for missing data in the tried-to-save model.
  • An example of the callable in point 3 would be like this (just an example for, say, a game!):
    • def on_added(through_model_instance):
          through_model_instance
      .balance = 1000.0 #although this one could be a default value at db level.
          through_model_instance
      .save()
          through_model_instance
      .achievements.create(tag='joined-this-relation')
I understand these approaches, combined or not, could not be perfect or bug-free. But I'd like to discuss them instead of plainly discard them. AFAIK right now the calls to .add() are disallowed when through is specified.

Could we work or at least discuss these use cases now? The history I quoted seems to be pretty.... historic, and we're in 2017. The first thing I'd like to know right not (with django 1.10 alive) is the feasability of this feature in a future django version.

I await your comments and/or criticism ^^.

Collin Anderson

unread,
Mar 20, 2017, 8:47:28 PM3/20/17
to django-d...@googlegroups.com
Hi,


Seems like it would be fine if Django allowed add() and let any errors about missing data bubble-up. I personally think passing in a defaults dict (just like get_or_create does) would also be fine, but a callback seems like overkill.


Collin


--
You received this message because you are subscribed to the Google Groups "Django developers (Contributions to Django itself)" group.
To unsubscribe from this group and stop receiving emails from it, send an email to django-developers+unsubscribe@googlegroups.com.
To post to this group, send email to django-developers@googlegroups.com.
Visit this group at https://groups.google.com/group/django-developers.
To view this discussion on the web visit https://groups.google.com/d/msgid/django-developers/ec5ffdfc-ef4b-4c30-a53d-d427cbe053e2%40googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

Adam Johnson

unread,
Mar 21, 2017, 2:38:06 AM3/21/17
to django-d...@googlegroups.com
It does seem like a somewhat arbitrary historical restriction. Collin's PoC change is surprisingly small and simple.

Seems like it would be fine if Django allowed add() and let any errors about missing data bubble-up.

Agree, this is also a precedent from get_or_create.
 
I personally think passing in a defaults dict (just like get_or_create does) would also be fine, but a callback seems like overkill.
 
This is a more consistent approach to the callback. One could always use custom logic in the through model's save method, or a signal, to achieve things that can't be done with through_defaults.


For more options, visit https://groups.google.com/d/optout.



--
Adam

Brice PARENT

unread,
Mar 21, 2017, 4:52:28 AM3/21/17
to django-d...@googlegroups.com

Seems interesting.

Also, added to the case where the only fields are the ones with only the two FK, there is the case where every other field has a default value (or at least accepts None). A call to .add, has no reason to fail in such cases.

For the other cases, a quick way to allow this, without depending on complex behaviours, would be to have allow something like :

publications = models.ManyToManyField(Publication, through=OtherModel, auto_create=True)
Of course, auto_create is probably not the name we'd use, it would have to be thought about a bit more.

But my point is that when this parametter is set, a call to makemigrations would only succeed if :
- Either all fields of OtherModel (excluding both FK of course) have a default value
- or OtherModel defines a special class method, like OtherModel.auto_create(model1, model2) which creates the instance, or OtherModel.get_default() which returns default values for the auto_creation process.
It makes it easy to use .add when we're explicitely allowed to, and the behaviour we have now to be maintained in all other cases.

Anyway, I just thought about this now, so I'm not sure if it covers enough use cases, and there are probably some side effects I didn't think of.

Brice

Le 20/03/17 à 22:46, Luis Masuelli a écrit :
--
You received this message because you are subscribed to the Google Groups "Django developers (Contributions to Django itself)" group.
To unsubscribe from this group and stop receiving emails from it, send an email to django-develop...@googlegroups.com.
To post to this group, send email to django-d...@googlegroups.com.

Russell Keith-Magee

unread,
Mar 21, 2017, 5:57:24 PM3/21/17
to Django Developers
On Tue, Mar 21, 2017 at 2:37 PM, Adam Johnson <m...@adamj.eu> wrote:
It does seem like a somewhat arbitrary historical restriction. Collin's PoC change is surprisingly small and simple.

Seems like it would be fine if Django allowed add() and let any errors about missing data bubble-up.
 
As the person who *made* the “somewhat arbitrary historical restriction”… :-)

Honestly - the reason it was made was scope creep. 

I was trying to land Eric’s work on #6095, which was already moderately complex. Nobody disagreed that adding an object to an m2m with a through model was a *bad* idea - there were just a few design decisions that had to be made. But if we made those decisions, #6095 was going to take *another* couple of months to land; the perfect being the enemy of the good, we decided to limit scope and land “simple” m2m add, and revisit the issue later.

I guess now is “later”… :-)

Yours,
Russ Magee %-)

Alexander Hill

unread,
Mar 21, 2017, 9:29:33 PM3/21/17
to Django Developers
Here's a little bit more historical discussion on the topic: https://groups.google.com/d/topic/django-developers/uWe31AjzZX0/discussion

--
You received this message because you are subscribed to the Google Groups "Django developers (Contributions to Django itself)" group.
To unsubscribe from this group and stop receiving emails from it, send an email to django-develop...@googlegroups.com.
To post to this group, send email to django-d...@googlegroups.com.

Luis Masuelli

unread,
Apr 17, 2017, 3:53:42 PM4/17/17
to Django developers (Contributions to Django itself)
I'm quite happy to see the topic is at least being considered! <3.

Although I suggested a solution, I like the solution posted by Collin in the PR (I'd prefer solutions not involving signature changes in methods, but anyway those signature changes Colin posted are not so... obtrusive).

I'd like to, however, propose a little change in Collin's code. Immediately before this code in _add_items:

                        self.through(**dict(through_defaults, **{
                            '%s_id' % source_field_name: self.related_val[0],
                            '%s_id' % target_field_name: obj_id,
                        }))

This one:

                        if callable(through_defaults):
                            through_defaults = through_defaults(self.related_val[0], obj_id)

With these two lines, we could allow passing a callable to through_fields (as we pass callables for defaults in django fields). The return value of such callable should be a dictionary, while the arguments should be source and target ids.

But even if this little change is not implemented, I like the Collin's solution anyway.

Another subtopic to discuss is about enhancing the Collin's solution, is to allow through_defaults be specified on field definition (In the same way we define default values in... scalar... fields; I leave open the discussion on whether such value should be overriden when using add, create, or any of these methods as changed by Collin).

I like how this is going and hope that any solution (even if it is just the as-is solution provided by Collin, with no changes) be accepted in any near-future version <3.

Collin Anderson

unread,
Aug 26, 2017, 11:18:53 AM8/26/17
to django-d...@googlegroups.com
Hi All,

I have a pull request for simple add()/create() etc with m2m through tables if any wants to try it out: https://github.com/django/django/pull/8981

If people are happy with the API, I'll add the docs too.

Collin


To unsubscribe from this group and stop receiving emails from it, send an email to django-developers+unsubscribe@googlegroups.com.
To post to this group, send email to django-developers@googlegroups.com.
Reply all
Reply to author
Forward
0 new messages