Opened 6 years ago

Closed 6 years ago

#29346 closed New feature (wontfix)

Add "intermediary" kwarg to ModelForm._save_m2m

Reported by: Douglas Denhartog Owned by: nobody
Component: Forms Version: dev
Severity: Normal Keywords:
Cc: Triage Stage: Unreviewed
Has patch: no Needs documentation: yes
Needs tests: yes Patch needs improvement: no
Easy pickings: no UI/UX: no

Description (last modified by 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.

Change History (6)

comment:1 by Tim Graham, 6 years ago

Why is this solution preferable to using Meta.exclude?

in reply to:  1 comment:2 by Douglas Denhartog, 6 years ago

Replying to Tim Graham:

Why is this solution preferable to using Meta.exclude?

This solution is in addition to Meta.exclude (but not 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

Last edited 6 years ago by Douglas Denhartog (previous) (diff)

comment:3 by Douglas Denhartog, 6 years ago

Description: modified (diff)
Easy pickings: set

comment:4 by Tim Graham, 6 years ago

Easy pickings: unset

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.

in reply to:  4 comment:5 by Douglas Denhartog, 6 years ago

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).

comment:6 by Carlton Gibson, 6 years ago

Needs documentation: set
Needs tests: set
Resolution: wontfix
Status: newclosed
Version: 2.0master

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...)

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