[Django] #29346: Add "intermediary" kwarg to ModelForm._save_m2m
Django
---------------------------------------------+------------------------
Reporter: Douglas Denhartog | Owner: nobody
Type: New feature | Status: new
Component: Forms | Version: 2.0
Severity: Normal | Keywords:
Triage Stage: Unreviewed | Has patch: 0
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 0
UI/UX: 0 |
---------------------------------------------+------------------------
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 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`.
--
Ticket URL: <https://code.djangoproject.com/ticket/29346>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
Reporter: Douglas Denhartog | Owner: nobody
Type: New feature | Status: new
Component: Forms | Version: 2.0
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Tim Graham):
Why is this solution preferable to using `Meta.exclude`?
--
Ticket URL: <https://code.djangoproject.com/ticket/29346#comment:1>
Django
Reporter: Douglas Denhartog | Owner: nobody
Type: New feature | Status: new
Component: Forms | Version: 2.0
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Douglas Denhartog):
Replying to [comment:1 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`
--
Ticket URL: <https://code.djangoproject.com/ticket/29346#comment:2>
Django
Reporter: Douglas Denhartog | Owner: nobody
Type: New feature | Status: new
Component: Forms | Version: 2.0
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
-----------------------------------+--------------------------------------
Changes (by Douglas Denhartog):
* easy: 0 => 1
Old description:
New description:
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
--
--
Ticket URL: <https://code.djangoproject.com/ticket/29346#comment:3>
Django
Reporter: Douglas Denhartog | Owner: nobody
Type: New feature | Status: new
Component: Forms | Version: 2.0
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Tim Graham):
* easy: 1 => 0
Comment:
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/29346#comment:4>
Django
Reporter: Douglas Denhartog | Owner: nobody
Type: New feature | Status: new
Component: Forms | Version: 2.0
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Douglas Denhartog):
Replying to [comment:4 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).
--
Ticket URL: <https://code.djangoproject.com/ticket/29346#comment:5>
Django
Reporter: Douglas Denhartog | Owner: nobody
Component: Forms | Version: master
Severity: Normal | Resolution: wontfix
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 1
Needs tests: 1 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Carlton Gibson):
* status: new => closed
* needs_docs: 0 => 1
* version: 2.0 => master
* resolution: => wontfix
* needs_tests: 0 => 1
Comment:
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...)
--
Ticket URL: <https://code.djangoproject.com/ticket/29346#comment:6>