Opened 7 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 )
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)
follow-up: 2 comment:1 by , 7 years ago
comment:2 by , 7 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
comment:3 by , 7 years ago
Description: | modified (diff) |
---|---|
Easy pickings: | set |
follow-up: 5 comment:4 by , 7 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.
comment:5 by , 7 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 fromself.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 , 6 years ago
Needs documentation: | set |
---|---|
Needs tests: | set |
Resolution: | → wontfix |
Status: | new → closed |
Version: | 2.0 → master |
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...)
Why is this solution preferable to using
Meta.exclude
?