Opened 11 months ago

Last modified 7 weeks ago

#29205 new Bug

MultiValueField ignores a required value of a sub field

Reported by: Takayuki Hirai Owned by: nobody
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: no
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 (10)

comment:1 Changed 11 months ago by Takayuki Hirai

Description: modified (diff)

comment:2 Changed 10 months 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 10 months 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 10 months ago by Takayuki Hirai (previous) (diff)

comment:4 Changed 10 months 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 8 months ago by Herbert Fortes

Cc: Herbert Fortes added

comment:6 Changed 3 months ago by Frank Sachsenheim

Cc: Frank Sachsenheim added

comment:7 Changed 8 weeks 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 7 weeks ago by jhrr (previous) (diff)

comment:8 Changed 7 weeks 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 7 weeks ago by jhrr (previous) (diff)

comment:9 Changed 7 weeks 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 7 weeks ago by jhrr (previous) (diff)

comment:10 in reply to:  9 Changed 7 weeks 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
Note: See TracTickets for help on using tickets.
Back to Top