Add "intermediary" kwarg to ModelForm._save_m2m

[Douglas Denhartog]

This is my first ticket, so while I attempted to follow the applicable guidelines, I apologize in advance if this ticket is not a model guideline-following example.

---

Django's documentation clearly explain Intermediary Model limitations: "[u]nlike normal many-to-many fields, you can’t use add(), create(), or set() to create relationships"

Thus, when ModelForm.save() is called and an intermediary model field is not in self._meta.exclude, users will get an error like so:

Cannot set values on a ManyToManyField which specifies an intermediary model. Use app.IntermediaryModelName's Manager instead.

This presents complexity when the intermediary field is included in the form.

My proposal is to add a kwarg intermediary to ModelForm._save_m2m. This kwarg behaves exactly like self._meta.exclude within ModelForm._save_m2m.

I have already created a github branch (​https://github.com/denhartog/django/tree/ticket_29346_2_0) with my solution, but have not created a pull request per contribution guidelines (but will do so if/when this ticket is approved). My solution adds three/four (3/4) lines of code, denoted in my solution below with EOL comment # ADDED. I believe my simple solution is more appropriate than a complex introspective analysis of every potential ManyToManyField's 'through' attribute.

The final code of my proposal is (in django.forms.models):

def _save_m2m(self, intermediary=None):  # ADDED
    """
    Save the many-to-many fields and generic relations for this form.
    """
    # could assert/convert intermediary == str, list, tuple  # ADDED
    cleaned_data = self.cleaned_data
    exclude = self._meta.exclude
    fields = self._meta.fields
    opts = self.instance._meta
    # Note that for historical reasons we want to include also
    # private_fields here. (GenericRelation was previously a fake
    # m2m field).
    for f in chain(opts.many_to_many, opts.private_fields):
        if not hasattr(f, 'save_form_data'):
            continue
        if fields and f.name not in fields:
            continue
        if exclude and f.name in exclude:
            continue
        if intermediary and f.name in intermediary:  # ADDED
            continue  # ADDED
        if f.name in cleaned_data:
            f.save_form_data(self.instance, cleaned_data[f.name])

And yes, this means adding the intermediary kwarg to ModelForm.save() so that intermediary can be passed to ModelForm._save_m2m.

[Tim Graham]

Why is this solution preferable to using Meta.exclude?

[Douglas Denhartog]

Replying to Tim Graham:

Why is this solution preferable to using Meta.exclude?

This solution is in addition to Meta.exclude (not but a Meta attribute). This permits a person to use, clean, validate, and manually save an intermediary model field WHILE being able to use the default ModelForm.save() without error by "excluding" intermediary fields during ModelForm._save_m2m

[Tim Graham]

Sorry, what I meant was, why isn't Meta.exclude sufficient for this use case. If you added a test to your branch that could demonstrate the use case.

Looking at the existing implementation of _save_m2m(), a possible alternative (that doesn't require changes to Django) could be to remove the field from self.cleaned_data after you save the intermediate model manually.

[Douglas Denhartog]

Replying to Tim Graham:

Sorry, what I meant was, why isn't Meta.exclude sufficient for this use case. If you added a test to your branch that could demonstrate the use case.

Looking at the existing implementation of _save_m2m(), a possible alternative (that doesn't require changes to Django) could be to remove the field from self.cleaned_data after you save the intermediate model manually.

To specifically answer your question: Meta.exclude is not sufficient when someone wants to include a model intermediary many-to-many field in a ModelForm. If one uses Meta.exclude, per the Django docs, one must "manually add the excluded fields back to the form, [and the field] will not be initialized from the model instance." Further, as you mentioned, without my solution, one must also manipulate self.cleaned_data.

Thus, without my solution, one must:
(1) manually add the intermediate data to the form instance (vs. my solution where Django does this itself);
(2) manually add the form field (both solutions require);
(3) (within ModelForm.save()) remove the field from cleaned_data, then super().save(), then add the field back into self.cleaned_data (vs. my solution where this is unnecessary).

I'll build some tests to demonstrate (will be a couple of days until I can do so).

[Carlton Gibson]

Hi Douglas. Thanks for the input.

My initial response here is favour the existing API. Given that, and the lack of activity on the branch, I'm going to close this as wontfix.

If you would like to add the test cases demonstrating the use-case, and maybe some docs, and open a PR on GitHub (against the master branch), then we can re-open/re-assess.
(It's likely that we won't want an addition here — but if the use-case is compelling...)