MultiValueField ignores a required value of a sub field

[Takayuki Hirai]

A field and a form definition:

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.

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

f = F({
    'mf_0': 'xxx',
    'mf_1': '',
})
assert f.is_valid() == Flase

If above behavior is not expected, please fix this problem.

[Tim Graham]

Why do you pass required=False in super(MF, self).__init__()? Removing that seems to resolve the issue.

[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:

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

    def compress(self, value):
        return []

[Tim Graham]

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.

[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 on the widget 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 doubt. As mentioned above seems like you'd need to fix it by descending into the build_attrs logic itself.

[jhrr]

Possible culprit?

​https://github.com/django/django/blob/b9cf764be62e77b4777b3a75ec256f6209a57671/django/forms/boundfield.py#L221

Does this also need to account for the require_all_fields value on MultiValueField?

[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

[jhrr]

Nope this was a red herring, seeing some other things though, will report...

Replying to 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

[David Smith]

I've done some investigation into this. Here are some notes which explain the behaviour outlined in the ticket.

is_valid()

The ​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 ​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')

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

[leon hughes]

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.

[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?

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

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

[Jacob Walls]

David's ​PR

[Jacob Walls]

Alternative in ​PR

[Carlton Gibson]

Suggested ​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.

[Jacob Walls]

In writing a test case, I reused some fields 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.)

[Carlton Gibson <carlton.gibson@…>]

In 2d0ae8da:

Fixed #29205 -- Corrected rendering of required attributes for MultiValueField subfields.

[Mariusz Felisiak <felisiak.mariusz@…>]

In 3a6431db:

Refs #29205 -- Added MultiValueField test for rendering of optional subfields.