Opened 2 years ago

Last modified 4 months ago

#29205 assigned Bug

MultiValueField ignores a required value of a sub field

Reported by: Takayuki Hirai Owned by: David Smith
Component: Forms Version: 1.11
Severity: Normal Keywords:
Cc: Herbert Fortes, Frank Sachsenheim Triage Stage: Accepted
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: yes
Easy pickings: no UI/UX: no

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

Change History (17)

comment:1 Changed 2 years ago by Takayuki Hirai

Description: modified (diff)

comment:2 Changed 2 years ago by Tim Graham

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

comment:3 Changed 2 years ago 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:

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 []
Last edited 2 years ago by Takayuki Hirai (previous) (diff)

comment:4 Changed 2 years ago by Tim Graham

Triage Stage: UnreviewedAccepted

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.

comment:5 Changed 2 years ago by Herbert Fortes

Cc: Herbert Fortes added

comment:6 Changed 21 months ago by Frank Sachsenheim

Cc: Frank Sachsenheim added

comment:7 Changed 21 months ago 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 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.

Last edited 21 months ago by jhrr (previous) (diff)

comment:8 Changed 21 months ago by jhrr

Possible culprit?

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

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

Last edited 21 months ago by jhrr (previous) (diff)

comment:9 Changed 21 months ago 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 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
Last edited 21 months ago by jhrr (previous) (diff)

comment:10 in reply to:  9 Changed 21 months ago by 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

comment:11 Changed 6 months ago by David Smith

Owner: changed from nobody to David Smith
Status: newassigned

comment:12 Changed 6 months ago 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 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')

comment:13 Changed 6 months ago 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.

comment:14 Changed 6 months ago by leon hughes

Owner: changed from David Smith to 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.

comment:15 Changed 6 months ago by leon hughes

Owner: changed from leon hughes to David Smith

comment:16 Changed 6 months ago 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?

comment:17 Changed 4 months ago by Carlton Gibson

Patch needs improvement: set
Note: See TracTickets for help on using tickets.
Back to Top