Skip to content

Commit

Permalink
🚑 [#4068] Fix broken backend validation
Browse files Browse the repository at this point in the history
Backend validation was being applied more strict than equivalent in the
frontend, which lead to submissions not being able to be completed.

Fixes the following aspects:

* Treat empty string is empty value for date field
* Don't reject textfield (and derivatives) with multiple=True when
  items inside are null (treat them as empty value/string)
* Allow empty lists for edit grid/repeating group when field is
  not required
* Skip validation for layout components, they never get data
* Ensure that empty string values for optional text fields are
  allowed (also covers derived fields)
* Fixed validation error being returned that doesn't point to
  a particular component

Backport-of: #4070
  • Loading branch information
SilviaAmAm authored and sergei-maertens committed Mar 28, 2024
1 parent f9796d4 commit b16e14d
Show file tree
Hide file tree
Showing 13 changed files with 247 additions and 17 deletions.
22 changes: 19 additions & 3 deletions src/openforms/formio/components/custom.py
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,17 @@
logger = logging.getLogger(__name__)


class FormioDateField(serializers.DateField):
def validate_empty_values(self, data):
is_empty, data = super().validate_empty_values(data)
# base field only treats `None` as empty, but formio uses empty strings
if data == "":
if self.required:
self.fail("required")
return (True, "")
return is_empty, data


@register("date")
class Date(BasePlugin[DateComponent]):
formatter = DateFormatter
Expand All @@ -56,7 +67,7 @@ def mutate_config_dynamically(

def build_serializer_field(
self, component: DateComponent
) -> serializers.DateField | serializers.ListField:
) -> FormioDateField | serializers.ListField:
"""
Accept date values.
Expand All @@ -73,7 +84,7 @@ def build_serializer_field(
validators.append(MinValueValidator(date.fromisoformat(min_date)))
if max_date := date_picker.get("maxDate"):
validators.append(MaxValueValidator(date.fromisoformat(max_date)))
base = serializers.DateField(
base = FormioDateField(
required=required,
allow_null=not required,
validators=validators,
Expand Down Expand Up @@ -248,7 +259,12 @@ def build_serializer_field(
extra["validators"] = validators

base = serializers.CharField(
required=required, allow_blank=not required, allow_null=False, **extra
required=required,
allow_blank=not required,
# FIXME: should always be False, but formio client sends `null` for
# untouched fields :( See #4068
allow_null=multiple,
**extra,
)
return serializers.ListField(child=base) if multiple else base

Expand Down
20 changes: 16 additions & 4 deletions src/openforms/formio/components/vanilla.py
Original file line number Diff line number Diff line change
Expand Up @@ -112,7 +112,12 @@ def build_serializer_field(
extra["validators"] = validators

base = serializers.CharField(
required=required, allow_blank=not required, allow_null=False, **extra
required=required,
allow_blank=not required,
# FIXME: should always be False, but formio client sends `null` for
# untouched fields :( See #4068
allow_null=multiple,
**extra,
)
return serializers.ListField(child=base) if multiple else base

Expand Down Expand Up @@ -143,7 +148,9 @@ def build_serializer_field(
base = serializers.EmailField(
required=required,
allow_blank=not required,
allow_null=False,
# FIXME: should always be False, but formio client sends `null` for
# untouched fields :( See #4068
allow_null=multiple,
**extra,
)
return serializers.ListField(child=base) if multiple else base
Expand Down Expand Up @@ -191,7 +198,12 @@ def build_serializer_field(
extra["validators"] = validators

base = serializers.CharField(
required=required, allow_blank=not required, allow_null=False, **extra
required=required,
allow_blank=not required,
# FIXME: should always be False, but formio client sends `null` for
# untouched fields :( See #4068
allow_null=multiple,
**extra,
)
return serializers.ListField(child=base) if multiple else base

Expand Down Expand Up @@ -396,6 +408,6 @@ def build_serializer_field(
child=nested,
required=required,
allow_null=not required,
allow_empty=False,
allow_empty=not required,
**kwargs,
)
14 changes: 9 additions & 5 deletions src/openforms/formio/registry.py
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@
from openforms.typing import DataMapping

from .typing import Component
from .utils import is_layout_component

if TYPE_CHECKING:
from openforms.submissions.models import Submission
Expand Down Expand Up @@ -90,11 +91,14 @@ def build_serializer_field(self, component: ComponentT) -> serializers.Field:
DeprecationWarning,
)

required = (
validate.get("required", False)
if (validate := component.get("validate"))
else False
)
if is_layout_component(component):
required = False # they do not hold data, they can never be required
else:
required = (
validate.get("required", False)
if (validate := component.get("validate"))
else False
)

# Allow anything that is valid JSON, taking into account the 'required'
# validation which is common for most components.
Expand Down
5 changes: 5 additions & 0 deletions src/openforms/formio/serializers.py
Original file line number Diff line number Diff line change
Expand Up @@ -75,6 +75,11 @@ def _remove_validations_from_field(self, field: serializers.Field) -> None:
# added based on component['validate']
field.validators = field.get_validators()

# apply additional attributes depending on the field type
match field:
case serializers.CharField():
field.allow_blank = True


def dict_to_serializer(
fields: dict[str, FieldOrNestedFields], **kwargs
Expand Down
17 changes: 15 additions & 2 deletions src/openforms/formio/tests/validation/test_date.py
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
from django.test import SimpleTestCase
from django.test import SimpleTestCase, tag

from ...typing import DateComponent
from .helpers import extract_error, validate_formio_data
Expand All @@ -12,12 +12,12 @@ def test_datefield_required_validation(self):
"key": "foo",
"label": "Foo",
"validate": {"required": True},
"datePicker": None,
}

invalid_values = [
({}, "required"),
({"foo": None}, "null"),
({"foo": ""}, "required"),
]

for data, error_code in invalid_values:
Expand Down Expand Up @@ -54,3 +54,16 @@ def test_min_max_date(self):
self.assertIn(component["key"], errors)
error = extract_error(errors, component["key"])
self.assertEqual(error.code, error_code)

@tag("gh-4068")
def test_empty_default_value(self):
component: DateComponent = {
"type": "date",
"key": "date",
"label": "Optional date",
"validate": {"required": False},
}

is_valid, _ = validate_formio_data(component, {"date": ""})

self.assertTrue(is_valid)
26 changes: 25 additions & 1 deletion src/openforms/formio/tests/validation/test_editgrid.py
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
from django.test import SimpleTestCase
from django.test import SimpleTestCase, tag

from openforms.submissions.tests.factories import SubmissionFactory
from openforms.typing import JSONObject
Expand Down Expand Up @@ -165,3 +165,27 @@ def test_valid_configuration_nested_field_not_present_in_top_level_serializer(se

self.assertIn("nested", nested_fields)
self.assertNotIn("toplevel", nested_fields)

@tag("gh-4068")
def test_optional_editgrid(self):
component: EditGridComponent = {
"type": "editgrid",
"key": "optionalRepeatingGroup",
"label": "Optional repeating group",
"validate": {"required": False},
"components": [
{
"type": "textfield",
"key": "optionalTextfield",
"label": "Optional Text field",
"validate": {"required": False},
}
],
}

is_valid, errors = validate_formio_data(
component,
{"optionalRepeatingGroup": []},
)

self.assertTrue(is_valid)
60 changes: 60 additions & 0 deletions src/openforms/formio/tests/validation/test_fieldset.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,60 @@
from django.test import SimpleTestCase, tag

from ...typing import FieldsetComponent
from .helpers import validate_formio_data


class FieldSetValidationTests(SimpleTestCase):

@tag("gh-4068")
def test_fieldset_doesnt_require_value(self):
# Fieldset is a layout component, so there's never any data to validate
component: FieldsetComponent = {
"type": "fieldset",
"key": "fieldset",
"label": "A field set",
# don't ask me how, but somehow this got injected at some point. It's not
# our new form builder, but it could be our own JS code in the backend in
# src/openforms/js/components/formio_builder/WebformBuilder.js or they're
# old forms that still suffer from #1724
"validate": {
"required": True,
},
"components": [
{
"type": "textfield",
"key": "textfield",
"label": "Text field",
},
],
}

is_valid, _ = validate_formio_data(component, {})

self.assertTrue(is_valid)

@tag("gh-4068")
def test_hidden_fieldset_with_nested_required_fields(self):
component: FieldsetComponent = {
"type": "fieldset",
"key": "fieldset",
"label": "A field set",
"components": [
{
"type": "textfield",
"key": "textfield",
"label": "Text field",
"validate": {"required": True},
},
],
"hidden": True,
"conditional": {
"show": True,
"when": "missing",
"eq": "never-triggered",
},
}

is_valid, _ = validate_formio_data(component, {"textfield": ""})

self.assertTrue(is_valid)
19 changes: 19 additions & 0 deletions src/openforms/formio/tests/validation/test_textfield.py
Original file line number Diff line number Diff line change
Expand Up @@ -111,6 +111,25 @@ def test_multiple(self):
with self.subTest("valid item"):
self.assertNotIn(0, errors["numbers"])

@tag("gh-4068")
def test_multiple_with_form_builder_empty_defaults(self):
# Our own form builder does funky stuff here by setting the defaultValue to
# a list with `null` item.
# XXX null is really not a correct value, but we need to rework the rest of our
# builder (in the backend) for this first.
component: TextFieldComponent = {
"type": "textfield",
"key": "manyTextfield",
"label": "Optional Text fields",
"validate": {"required": False},
"multiple": True,
"defaultValue": [None], # FIXME: should really be [""] or []
}

is_valid, _ = validate_formio_data(component, {"manyTextfield": [None]})

self.assertTrue(is_valid)

@given(validator=st.sampled_from(["phonenumber-international", "phonenumber-nl"]))
def test_textfield_with_plugin_validator(self, validator: str):
component: TextFieldComponent = {
Expand Down
2 changes: 2 additions & 0 deletions src/openforms/formio/typing/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
ContentComponent,
DatetimeComponent,
EditGridComponent,
FieldsetComponent,
FileComponent,
RadioComponent,
SelectBoxesComponent,
Expand All @@ -37,6 +38,7 @@
"ContentComponent",
"Column",
"ColumnsComponent",
"FieldsetComponent",
# special
"EditGridComponent",
# deprecated
Expand Down
4 changes: 3 additions & 1 deletion src/openforms/formio/typing/custom.py
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
from typing_extensions import NotRequired

from .base import Component
from .dates import DatePickerConfig


class DateComponent(Component):
datePicker: DatePickerConfig | None
datePicker: NotRequired[DatePickerConfig]
4 changes: 4 additions & 0 deletions src/openforms/formio/typing/vanilla.py
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,10 @@ class DatetimeComponent(Component):
datePicker: NotRequired[DatePickerConfig | None]


class FieldsetComponent(Component):
components: list[Component]


class Column(TypedDict):
size: int
sizeMobile: int
Expand Down
2 changes: 1 addition & 1 deletion src/openforms/submissions/api/validation.py
Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,7 @@ def validate(self, attrs: dict):
if errors:
formio_validation_errors.append({"data": errors})
else:
formio_validation_errors.append(None)
formio_validation_errors.append({})

if any(formio_validation_errors):
raise serializers.ValidationError({"steps": formio_validation_errors})
Expand Down
Loading

0 comments on commit b16e14d

Please sign in to comment.