[Django] #29205: MultiValueField ignores a required value of a sub field
Django
------------------------------------------+------------------------
Reporter: Takayuki Hirai | Owner: nobody
Type: Bug | Status: new
Component: Forms | Version: 1.11
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 |
------------------------------------------+------------------------
A field and a form definition:
{{{
#!python
from django.forms import (
Form,
CharField,
MultiValueField,
MultiWidget,
)
class MF(MultiValueField):
widget = MultiWidget
def __init__(self):
fields = [
CharField(required=False),
CharField(required=True),
]
widget = self.widget(widgets=[
f.widget
for f in fields
], attrs={})
super(MF, self).__init__(
fields=fields,
widget=widget,
require_all_fields=False,
required=False,
)
def compress(self, value):
return []
class F(Form):
mf = MF()
}}}
When the form is passed empty values for both sub fields,
{{{form.is_valid() == True}}}.
But I expected {{{is_valid()}}} returns False, because one of the sub
fields is set as required.
{{{
#!python
f = F({
'mf_0': '',
'mf_1': '',
})
assert f.is_valid() == True # I expect this should return False
}}}
On the other hand, When one of its sub field is passed a non-empty value,
{{{form.is_valid() == False}}}
{{{
#!python
f = F({
'mf_0': ''xxx,
'mf_1': '',
})
assert f.is_valid() == Flase
}}}
If above behavior is not expected, please fix this problem.
--
Ticket URL: <https://code.djangoproject.com/ticket/29205>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
Reporter: Takayuki Hirai | Owner: nobody
Type: Bug | Status: new
Component: Forms | Version: 1.11
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Description changed by Takayuki Hirai:
Old description:
New description:
def compress(self, value):
return []
{{{
#!python
{{{
#!python
f = F({
'mf_0': 'xxx',
'mf_1': '',
})
assert f.is_valid() == Flase
}}}
If above behavior is not expected, please fix this problem.
--
--
Ticket URL: <https://code.djangoproject.com/ticket/29205#comment:1>
Django
Reporter: Takayuki Hirai | Owner: nobody
Type: Bug | Status: new
Component: Forms | Version: 1.11
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 do you pass `required=False` in `super(MF, self).__init__()`? Removing
that seems to resolve the issue.
--
Ticket URL: <https://code.djangoproject.com/ticket/29205#comment:2>
Django
Reporter: Takayuki Hirai | Owner: nobody
Type: Bug | Status: new
Component: Forms | Version: 1.11
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 Takayuki Hirai):
I tried to remove {{{required=False}}}, then both INPUT elements in HTML
became {{{required}}}.
{{{
<tr><th><label for="id_mf_0">Mf:</label></th><td><input type="text"
name="mf_0" required id="id_mf_0" />
<input type="text" name="mf_1" required id="id_mf_1" /></td></tr>
}}}
Code:
{{{
#!python
class MW(MultiWidget):
def decompress(self, value):
return []
class MF(MultiValueField):
widget = MW
def __init__(self):
fields = [
CharField(required=False),
CharField(required=True),
]
widget = self.widget(widgets=[
f.widget
for f in fields
], attrs={})
super(MF, self).__init__(
fields=fields,
widget=widget,
require_all_fields=False,
required=True,
)
def compress(self, value):
return []
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/29205#comment:3>
Django
Reporter: Takayuki Hirai | Owner: nobody
Type: Bug | Status: new
Component: Forms | Version: 1.11
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Tim Graham):
* stage: Unreviewed => Accepted
Comment:
If this is indeed an incorrect behavior (I'm not certain that there isn't
some other way to achieve the use case), it might be that
`Bound.build_widget_attrs()` incorrectly adds the required attribute for
this case. I'm not sure what a fix would look like.
--
Ticket URL: <https://code.djangoproject.com/ticket/29205#comment:4>
Django
Reporter: Takayuki Hirai | Owner: nobody
Type: Bug | Status: new
Component: Forms | Version: 1.11
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Herbert Fortes):
* cc: Herbert Fortes (added)
--
Ticket URL: <https://code.djangoproject.com/ticket/29205#comment:5>
Django
Reporter: Takayuki Hirai | Owner: nobody
Type: Bug | Status: new
Component: Forms | Version: 1.11
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Frank Sachsenheim):
* cc: Frank Sachsenheim (added)
--
Ticket URL: <https://code.djangoproject.com/ticket/29205#comment:6>
Django
Reporter: Takayuki Hirai | Owner: nobody
Type: Bug | Status: new
Component: Forms | Version: 1.11
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by jhrr):
I also bumped into this a while back during a project and promptly forgot
about it. In my case I fixed it by overrriding `render` like so:
{{{
def render(self, name, value, attrs=None):
if self.is_localized:
for widget in self.widgets:
widget.is_localized = self.is_localized
if not isinstance(value, list):
value = self.decompress(value)
output = []
final_attrs = self.build_attrs(attrs)
id_ = final_attrs.get("id")
for i, widget in enumerate(self.widgets):
try:
widget_value = value[i]
except IndexError:
widget_value = None
if id_:
final_attrs = dict(final_attrs, id="%s_%s" % (id_, i))
final_attrs["required"] = widget.is_required # ADDED
output.append(widget.render(name + "_%s" % i, widget_value,
final_attrs))
return mark_safe(self.format_output(output))
}}}
Whether that is optimal however I wasn't able to fully ascertain.
--
Ticket URL: <https://code.djangoproject.com/ticket/29205#comment:7>
Django
Reporter: Takayuki Hirai | Owner: nobody
Type: Bug | Status: new
Component: Forms | Version: 1.11
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by jhrr):
Possible culprit?
--
Ticket URL: <https://code.djangoproject.com/ticket/29205#comment:8>
Django
Reporter: Takayuki Hirai | Owner: nobody
Type: Bug | Status: new
Component: Forms | Version: 1.11
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by jhrr):
Adding something like this certainly does cause some failures in test
cases where the `required` string has been disappeared from the emitted
`HTML`. Looking into further isolating some better test cases.
{{{
def build_widget_attrs(self, attrs, widget=None):
widget = widget or self.field.widget
attrs = dict(attrs) # Copy attrs to avoid modifying the argument.
if widget.use_required_attribute(self.initial) and self.field.required
and self.form.use_required_attribute:
if hasattr(self.field, 'require_all_fields'):
if not widget.is_required and not
self.field.require_all_fields:
attrs['required'] = False
else:
attrs['required'] = True
if self.field.disabled:
attrs['disabled'] = True
return attrs
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/29205#comment:9>
Django
Reporter: Takayuki Hirai | Owner: nobody
Type: Bug | Status: new
Component: Forms | Version: 1.11
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by jhrr):
Nope this was a red herring, seeing some other things though, will
report...
Replying to [comment:9 jhrr]:
> Adding something like this certainly does cause some failures in test
cases where the `required` string has been disappeared from the emitted
`HTML` from `MultiValueField` tests. Looking into further isolating some
better cases.
>
>
> {{{
> def build_widget_attrs(self, attrs, widget=None):
> widget = widget or self.field.widget
> attrs = dict(attrs) # Copy attrs to avoid modifying the argument.
> if widget.use_required_attribute(self.initial) and
self.field.required and self.form.use_required_attribute:
> if hasattr(self.field, 'require_all_fields'):
> if not widget.is_required and not
self.field.require_all_fields:
> attrs['required'] = False
> else:
> attrs['required'] = True
> if self.field.disabled:
> attrs['disabled'] = True
> return attrs
> }}}
--
Ticket URL: <https://code.djangoproject.com/ticket/29205#comment:10>
Django
Reporter: Takayuki Hirai | Owner: David Smith
Type: Bug | Status: assigned
Component: Forms | Version: 1.11
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by David Smith):
* owner: nobody => David Smith
* status: new => assigned
--
Ticket URL: <https://code.djangoproject.com/ticket/29205#comment:11>
Django
Reporter: Takayuki Hirai | Owner: David Smith
Type: Bug | Status: assigned
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by David Smith):
I've done some investigation into this. Here are some notes which explain
the behaviour outlined in the ticket.
**is_valid()**
The
[https://github.com/django/django/blob/49275c548887769cd70bbd85a3b125491f0c4062/django/forms/fields.py#L1006
clean] method of MultiValueField is driving the validation behaviour
explained in the original ticket.
The example in the ticket shows `required=False` being set on the
`MultiValueField`. When the fields are all blank (or two empty strings)
the logic follows this if statement and therefore doesn't raise a
validation error and `return self.compress([])`
{{{
if not value or isinstance(value, (list, tuple)):
if not value or not [v for v in value if v not in
self.empty_values]:
if self.required:
raise ValidationError(self.error_messages['required'],
code='required')
else:
return self.compress([])
}}}
In the case where one of the fields has some data, it skips this `if`
statement. The next section of clean loops over `fields` and therefore
raises an error as one of them contains `required=True`
**Required attribute**
The notes above also explain that is the field is `required=True` (default
setting) then whilst is_valid() gives the expected output both of the
fields input html tags both become `required`. This is due to this piece
of code in
[https://github.com/django/django/blob/b9cf764be62e77b4777b3a75ec256f6209a57671/django/forms/boundfield.py#L221
boundfield.py]. This code looks at the field required status rather than
subwidgets. Therefore all of the inputs for subwidgets are treated the
same, irrespective of each subwidgets `required` attribute.
I therefore think that the two areas above are where we should be looking
at for a potential fix. However, I'm not quite sure what is intended to
happen where some widgets are required and others are not. Some questions,
appreciate thoughts.
- Should inputs have required html attributes (all, some, none)
- How should the help / validation text work. (e.g. 'this field is
required')
--
Ticket URL: <https://code.djangoproject.com/ticket/29205#comment:12>
Django
Reporter: Takayuki Hirai | Owner: David Smith
Type: Bug | Status: assigned
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Carlton Gibson):
Hi David, good work.
I'll hazard an opinion.
Required **should** work at two levels, rather than overriding from the
parent.
Required on the MWF should mean "Can this set of sub-fields be skipped
entirely?".
Assuming No to that, i.e. that the MWF is required, then the individual
fields should respect their own required attributes.
I imagine a Date + Time MWF where the Time is optional. (🤷♀️)
In the limit, a required MWF with all optional sub-fields would be
required in name only.
That would be my first stab at the correct behaviour. It would be
interesting to see what the existing tests say about that.
--
Ticket URL: <https://code.djangoproject.com/ticket/29205#comment:13>
Django
Reporter: Takayuki Hirai | Owner: itergator
Type: Bug | Status: assigned
Component: Forms | Version: 1.11
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by itergator):
* owner: David Smith => itergator
Comment:
I am experiencing this issue with the 2.2.10 LTS release too:
{{{
class RowSelectorField(forms.fields.MultiValueField):
def __init__(self, *args, **kwargs):
choices = kwargs.pop('choice')
fields = [forms.fields.ChoiceField(),
forms.fields.IntegerField(required=False)]
super(RowSelectorField, self).__init__(require_all_fields=False,
fields=fields, *args, **kwargs)
self.widget = RowSelectorWidget(choices=choices)
def compress(self, values):
return values
}}}
all of the fields in HTML are set to required, even the integer field. I
have noticed if I change the super call to:
{{{
super(RowSelectorField, self).__init__(required=False,
require_all_fields=False, fields=fields, *args, **kwargs)
}}}
and in the MultiWidget I use:
{{{
class RowSelectorWidget(forms.widgets.MultiWidget):
def __init__(self, choices, attrs=None):
choices.append((len(choices), _('custom')))
widgets = [forms.RadioSelect(choices=choices, attrs={'required':
True}),
forms.NumberInput(attrs={'required': False})]
super(RowSelectorWidget, self).__init__(widgets, attrs)
}}}
I do get the correct expected behaviour.
--
Ticket URL: <https://code.djangoproject.com/ticket/29205#comment:14>
Django
Reporter: Takayuki Hirai | Owner: David Smith
Type: Bug | Status: assigned
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by itergator):
* owner: itergator => David Smith
--
Ticket URL: <https://code.djangoproject.com/ticket/29205#comment:15>
Django
Reporter: Takayuki Hirai | Owner: David Smith
Type: Bug | Status: assigned
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by David Smith):
Hi Leon, thanks for your message.
In your last example where you experience your expected behaviour both
`required` and `require_all_fields` are set to false on the MVF. In this
case I'd expect `form.is_valid()` to return `True` where none of the
fields are completed even though the `ChoiceField()` is `required`.
Is this the case and is this what you would expect to happen?
--
Ticket URL: <https://code.djangoproject.com/ticket/29205#comment:16>
Django
Reporter: Takayuki Hirai | Owner: David Smith
Type: Bug | Status: assigned
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Easy pickings: 0 | UI/UX: 0
Changes (by Carlton Gibson):
* needs_better_patch: 0 => 1
--
Ticket URL: <https://code.djangoproject.com/ticket/29205#comment:17>
Django
Reporter: Takayuki Hirai | Owner: David Smith
Type: Bug | Status: assigned
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Easy pickings: 0 | UI/UX: 0
Comment (by Jacob Walls):
Greetings, all. I think we need to take a step back. A form with every
subfield empty on an optional MVF should pass validation, and it does, and
there is a test in the suite for it. (The implication in the body of the
ticket is not quite right.) So I suggest the linked patch avoid making
changes to validation.
The reporter wanted the field to be required in reality but didn't want to
let `required=True` on the MVF because it made every subfield have the
HTML required attribute. I suggest renaming the ticket to focus on this
specific piece. I suggest we only handle what solves the reporter's use
case: when the MVF is `required` but `require_all_fields=False` set the
required attribute on the widget based on the subfield.
--
Ticket URL: <https://code.djangoproject.com/ticket/29205#comment:18>
Django
Reporter: Takayuki Hirai | Owner: David Smith
Type: Bug | Status: assigned
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Easy pickings: 0 | UI/UX: 0
Comment (by plidauer):
Hi all, I agree with Jacob. This doesn't seem to be an issue about whether
a MVF is semantically valid or required - it's about a specific behavior
which we can't currently achieve, at least not while using MVF.
The docs state:
When [require_all_fields] set to False, the Field.required attribute can
be set to False for individual fields to make them optional. If no value
is supplied for a required field, an incomplete validation error will be
raised.
But setting {{{required=True}}} on MVF level ignores the {{{required}}}
attribute on subfields regarding the HTML attribute which in turn comes
from the widget attrs.
--
Ticket URL: <https://code.djangoproject.com/ticket/29205#comment:19>
Django
Reporter: Takayuki Hirai | Owner: David Smith
Type: Bug | Status: assigned
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
Changes (by Jacob Walls):
* cc: Jacob Walls (added)
* has_patch: 0 => 1
Comment:
[David's PR https://github.com/django/django/pull/12642]
--
Ticket URL: <https://code.djangoproject.com/ticket/29205#comment:20>
Django
Reporter: Takayuki Hirai | Owner: Jacob Walls
Type: Bug | Status: assigned
Component: Forms | Version: 1.11
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Jacob Walls):
* owner: David Smith => Jacob Walls
* needs_better_patch: 1 => 0
Comment:
Alternative in [https://github.com/django/django/pull/14034 PR]
--
Ticket URL: <https://code.djangoproject.com/ticket/29205#comment:21>
Django
Reporter: Takayuki Hirai | Owner: Jacob Walls
Type: Bug | Status: assigned
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
Changes (by Carlton Gibson):
* needs_better_patch: 0 => 1
Comment:
Suggested
[https://github.com/django/django/pull/14034#pullrequestreview-720969152
patch in latest PR] doesn't solve the issue that `form.is_valid()` needs
to track the truth-table of expected cases (when
`require_all_fields=False`).
I think progress here probably requires first constructing the clear set
of test cases for the possibilities. With that in hand would could
probably conclude this.
--
Ticket URL: <https://code.djangoproject.com/ticket/29205#comment:22>
Django
Reporter: Takayuki Hirai | Owner: Jacob Walls
Type: Bug | Status: assigned
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 1 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
Changes (by Jacob Walls):
* needs_tests: 0 => 1
Comment:
In writing a test case, I reused some models that were too complex for the
case I was trying to demonstrate. I'll take a pass at writing a better
test, and then it should be clear whether changes to validation are needed
or not. (I still suspect not, but I'm happy to be proven wrong.)
--
Ticket URL: <https://code.djangoproject.com/ticket/29205#comment:23>
Django
Reporter: Takayuki Hirai | Owner: Jacob Walls
Type: Bug | Status: assigned
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Jacob Walls):
* needs_better_patch: 1 => 0
* needs_tests: 1 => 0
--
Ticket URL: <https://code.djangoproject.com/ticket/29205#comment:24>
Django
Reporter: Takayuki Hirai | Owner: Jacob
| Walls
Type: Bug | Status: assigned
Severity: Normal | Resolution:
Keywords: | Triage Stage: Ready for
| checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Carlton Gibson):
* stage: Accepted => Ready for checkin
--
Ticket URL: <https://code.djangoproject.com/ticket/29205#comment:25>
Django
-------------------------------------+-------------------------------------
Reporter: Takayuki Hirai | Owner: Jacob
| Walls
Component: Forms | Version: 1.11
Severity: Normal | Resolution: fixed
Keywords: | Triage Stage: Ready for
| checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* status: assigned => closed
* resolution: => fixed
Comment:
In [changeset:"2d0ae8da80e1ed37e94f3abad5c80145a00e688f" 2d0ae8da]:
{{{
#!CommitTicketReference repository=""
revision="2d0ae8da80e1ed37e94f3abad5c80145a00e688f"
Fixed #29205 -- Corrected rendering of required attributes for
MultiValueField subfields.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/29205#comment:26>
Django
-------------------------------------+-------------------------------------
Reporter: Takayuki Hirai | Owner: Jacob
| Walls
Type: Bug | Status: closed
Component: Forms | Version: 1.11
Severity: Normal | Resolution: fixed
Keywords: | Triage Stage: Ready for
| checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Mariusz Felisiak <felisiak.mariusz@…>):
In [changeset:"3a6431db5461b88e691d770abdba6a2793ef699d" 3a6431db]:
{{{
#!CommitTicketReference repository=""
revision="3a6431db5461b88e691d770abdba6a2793ef699d"
Refs #29205 -- Added MultiValueField test for rendering of optional
subfields.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/29205#comment:27>