Avoid passing empty string in flexible fields data validation (#4528)

Remove the hidden input fields with the interface form field prefix to
avoid passing these empty values.

These fields were used for easy parsing in the `MultipleCIVForm`;
however, they were not removed in `InterfaceFormFieldsMixin.clean` if
the choice was `UNDEFINED`, effectively passing an empty string as the
desired value for the socket.

We now recognise the flexible source choice field as an interface field,
so we do not need the hidden fields anymore.

Author: koopmant

app/grandchallenge/components/forms.py

@@ -10,7 +10,6 @@
 from django.contrib.auth import get_user_model
 from django.forms import (
     CheckboxSelectMultiple,
-    Field,
     Form,
     HiddenInput,
     ModelChoiceField,
@@ -222,11 +221,6 @@ def get_fields_for_interface(
                     required=False,
                     bound_field_class=BoundFieldWithDNoneClass,
                 ),
-                # Add hidden input field for parsing when rendering
-                # dynamically added fields.
-                prefixed_interface_slug: Field(
-                    required=False, widget=HiddenInput()
-                ),
             }
         elif interface.super_kind == interface.SuperKind.FILE:
             return {
@@ -254,11 +248,6 @@ def get_fields_for_interface(
                     required=False,
                     bound_field_class=BoundFieldWithDNoneClass,
                 ),
-                # Add hidden input field for parsing when rendering
-                # dynamically added fields.
-                prefixed_interface_slug: Field(
-                    required=False, widget=HiddenInput()
-                ),
             }
         elif interface.super_kind == interface.SuperKind.VALUE:
             return {
@@ -448,36 +437,33 @@ def __init__(self, *args, instance, base_obj, user, **kwargs):  # noqa C901
                 )
             )
 
-        # Add fields for dynamically added new interfaces
-        for slug in self.data.keys():
-            interface_slug = self.parse_slug(slug=slug)
-
-            if (
-                interface_slug
-                and slug not in self.fields.keys()
-                and ComponentInterface.objects.filter(
-                    slug=interface_slug
-                ).exists()
-            ):
-                interface = ComponentInterface.objects.filter(
-                    slug=interface_slug
-                ).get()
-
-                self.fields.update(
-                    self.get_fields_for_interface(
-                        interface=interface,
-                        user=self.user,
-                        required=False,
-                    )
+        for interface in self.get_dynamically_added_interfaces():
+            self.fields.update(
+                self.get_fields_for_interface(
+                    interface=interface,
+                    user=self.user,
+                    required=False,
                 )
+            )
 
-    @staticmethod
-    def parse_slug(*, slug):
-        if not slug.startswith(INTERFACE_FORM_FIELD_PREFIX):
-            return None
+    def get_dynamically_added_interfaces(self):
+        new_interface_slugs = set()
+        for field_name in self.data.keys():
+            interface_slug = self.get_interface_slug(field_name=field_name)
 
-        interface_slug = slug[len(INTERFACE_FORM_FIELD_PREFIX) :]
+            if interface_slug and field_name not in self.fields.keys():
+                new_interface_slugs.add(interface_slug)
 
+        return ComponentInterface.objects.filter(slug__in=new_interface_slugs)
+
+    @staticmethod
+    def get_interface_slug(*, field_name):
+        if field_name.startswith(INTERFACE_FORM_FIELD_PREFIX):
+            interface_slug = field_name[len(INTERFACE_FORM_FIELD_PREFIX) :]
+        elif field_name.startswith(FlexibleWidgetPrefixes.CHOICE):
+            interface_slug = field_name[len(FlexibleWidgetPrefixes.CHOICE) :]
+        else:
+            interface_slug = None
         return interface_slug
 
     def clean(self):

app/tests/algorithms_tests/test_forms.py

@@ -224,7 +224,6 @@ def try_create_algorithm():
                 f'name="{FlexibleWidgetPrefixes.UPLOAD}some-overlay"',
                 f'name="{FlexibleWidgetPrefixes.SEARCH}some-overlay_{SearchWidgetSuffixes.INPUT}"',
                 f'name="{FlexibleWidgetPrefixes.SEARCH}some-overlay_{SearchWidgetSuffixes.CHOICE}"',
-                f'name="{INTERFACE_FORM_FIELD_PREFIX}some-overlay"',
             ],
         ),
         (
@@ -237,7 +236,6 @@ def try_create_algorithm():
                 f'name="{FlexibleWidgetPrefixes.UPLOAD}some-medical-image"',
                 f'name="{FlexibleWidgetPrefixes.SEARCH}some-medical-image_{SearchWidgetSuffixes.INPUT}"',
                 f'name="{FlexibleWidgetPrefixes.SEARCH}some-medical-image_{SearchWidgetSuffixes.CHOICE}"',
-                f'name="{INTERFACE_FORM_FIELD_PREFIX}some-medical-image"',
             ],
         ),
         (
@@ -251,7 +249,6 @@ def try_create_algorithm():
                 f'name="{FlexibleWidgetPrefixes.UPLOAD}some-medical-image_{DICOMUploadWidgetSuffixes.UPLOADS}"',
                 f'name="{FlexibleWidgetPrefixes.SEARCH}some-medical-image_{SearchWidgetSuffixes.INPUT}"',
                 f'name="{FlexibleWidgetPrefixes.SEARCH}some-medical-image_{SearchWidgetSuffixes.CHOICE}"',
-                f'name="{INTERFACE_FORM_FIELD_PREFIX}some-medical-image"',
             ],
         ),
         (
@@ -386,7 +383,6 @@ def try_create_algorithm():
                 f'name="{FlexibleWidgetPrefixes.UPLOAD}anything"',
                 f'name="{FlexibleWidgetPrefixes.SEARCH}anything_{SearchWidgetSuffixes.INPUT}"',
                 f'name="{FlexibleWidgetPrefixes.SEARCH}anything_{SearchWidgetSuffixes.CHOICE}"',
-                f'name="{INTERFACE_FORM_FIELD_PREFIX}anything"',
             ],
         ),
     ),

app/tests/archives_tests/test_views.py

@@ -26,6 +26,7 @@
 from grandchallenge.subdomains.utils import reverse
 from grandchallenge.uploads.models import UserUpload
 from tests.algorithms_tests.factories import AlgorithmInterfaceFactory
+from tests.algorithms_tests.test_forms import extract_form_data_from_response
 from tests.archives_tests.factories import (
     ArchiveFactory,
     ArchiveItemFactory,
@@ -1182,12 +1183,11 @@ def test_archive_item_create_view(
         client=client,
         user=editor,
     )
-    assert len(response.context["form"].fields) == 6
     assert response.context["form"].fields[
         f"{INTERFACE_FORM_FIELD_PREFIX}{ci_str.slug}"
     ]
     assert response.context["form"].fields[
-        f"{INTERFACE_FORM_FIELD_PREFIX}{ci_img.slug}"
+        f"{FlexibleWidgetPrefixes.CHOICE}{ci_img.slug}"
     ]
     assert response.context["form"].fields["title"]
 
@@ -1261,22 +1261,37 @@ def test_archive_item_create_view_with_empty_value(
     existing_archive_item = ArchiveItemFactory(archive=archive)
     ci_str = ComponentInterfaceFactory(kind=InterfaceKindChoices.STRING)
     ci_img = ComponentInterfaceFactory(kind=InterfaceKindChoices.PANIMG_IMAGE)
+    ci_file = ComponentInterfaceFactory(
+        kind=InterfaceKindChoices.ANY, store_in_database=False
+    )
     civ_str = ci_str.create_instance(value="foo")
     civ_img = ci_img.create_instance(image=ImageFactory())
-    existing_archive_item.values.set([civ_str, civ_img])
+    civ_file = ComponentInterfaceValueFactory(interface=ci_file)
+    existing_archive_item.values.set([civ_str, civ_img, civ_file])
+
+    response = get_view_for_user(
+        viewname="archives:item-create",
+        client=client,
+        reverse_kwargs={"slug": archive.slug},
+        user=editor,
+        method=client.get,
+    )
+    data = extract_form_data_from_response(response)
+    data.update(
+        {
+            **get_interface_form_data(interface_slug=ci_str.slug, data="bar"),
+            f"{FlexibleWidgetPrefixes.CHOICE}{ci_img.slug}": ImageSourceChoices.UNDEFINED.value,
+            f"{FlexibleWidgetPrefixes.CHOICE}{ci_file.slug}": ImageSourceChoices.UNDEFINED.value,
+            "title": "archive-item title",
+        }
+    )
 
     with django_capture_on_commit_callbacks(execute=True):
         response = get_view_for_user(
             viewname="archives:item-create",
             client=client,
             reverse_kwargs={"slug": archive.slug},
-            data={
-                **get_interface_form_data(
-                    interface_slug=ci_str.slug, data="bar"
-                ),
-                f"{FlexibleWidgetPrefixes.CHOICE}{ci_img.slug}": ImageSourceChoices.UNDEFINED.value,
-                "title": "archive-item title",
-            },
+            data=data,
             user=editor,
             method=client.post,
         )
@@ -1289,6 +1304,8 @@ def test_archive_item_create_view_with_empty_value(
     assert new_archive_item.values.get(interface=ci_str).value == "bar"
     with pytest.raises(ObjectDoesNotExist):
         new_archive_item.values.get(interface=ci_img)
+    with pytest.raises(ObjectDoesNotExist):
+        new_archive_item.values.get(interface=ci_file)
 
 
 @pytest.mark.django_db

app/tests/cases_tests/test_forms.py

@@ -4,6 +4,7 @@
 from grandchallenge.cases.models import RawImageUploadSession
 from grandchallenge.components.forms import (
     INTERFACE_FORM_FIELD_PREFIX,
+    FlexibleWidgetPrefixes,
     MultipleCIVForm,
 )
 from grandchallenge.components.models import ComponentInterface
@@ -79,10 +80,15 @@ def test_upload_some_images(
     assert response.status_code == 403
 
 
-def test_parse_slug():
-    assert (
-        MultipleCIVForm.parse_slug(
-            slug=f"{INTERFACE_FORM_FIELD_PREFIX}foo-bar"
-        )
-        == "foo-bar"
-    )
+@pytest.mark.parametrize(
+    "field_name, slug",
+    (
+        (f"{INTERFACE_FORM_FIELD_PREFIX}foo-bar", "foo-bar"),
+        (f"{FlexibleWidgetPrefixes.CHOICE}foo-bar", "foo-bar"),
+        # Ignore because there should be a choice field for the interface
+        (f"{FlexibleWidgetPrefixes.SEARCH}foo-bar", None),
+        ("foo-bar", None),  # not an interface field
+    ),
+)
+def test_get_interface_slug(field_name, slug):
+    assert MultipleCIVForm.get_interface_slug(field_name=field_name) == slug

app/tests/conftest.py

@@ -530,7 +530,6 @@ def get_interface_form_data(
             form_data = {
                 f"{FlexibleWidgetPrefixes.CHOICE}{interface_slug}": SourceChoices.SEARCH.value,
                 f"{FlexibleWidgetPrefixes.SEARCH}{interface_slug}_{SearchWidgetSuffixes.CHOICE}": data,
-                f"{INTERFACE_FORM_FIELD_PREFIX}{interface_slug}": "",
             }
         else:
             if ci.is_dicom_image_kind:
@@ -542,29 +541,27 @@ def get_interface_form_data(
                     f"{FlexibleWidgetPrefixes.UPLOAD}{interface_slug}_{DICOMUploadWidgetSuffixes.UPLOADS}": data[
                         1
                     ],
-                    f"{INTERFACE_FORM_FIELD_PREFIX}{interface_slug}": "",
                 }
             else:
                 form_data = {
                     f"{FlexibleWidgetPrefixes.CHOICE}{interface_slug}": SourceChoices.UPLOAD.value,
                     f"{FlexibleWidgetPrefixes.UPLOAD}{interface_slug}": data,
-                    f"{INTERFACE_FORM_FIELD_PREFIX}{interface_slug}": "",
                 }
     elif ci.super_kind == ci.SuperKind.FILE:
         if existing_data:
             form_data = {
                 f"{FlexibleWidgetPrefixes.CHOICE}{interface_slug}": SourceChoices.SEARCH.value,
                 f"{FlexibleWidgetPrefixes.SEARCH}{interface_slug}_{SearchWidgetSuffixes.CHOICE}": data,
-                f"{INTERFACE_FORM_FIELD_PREFIX}{interface_slug}": "",
             }
         else:
             form_data = {
                 f"{FlexibleWidgetPrefixes.CHOICE}{interface_slug}": SourceChoices.UPLOAD.value,
                 f"{FlexibleWidgetPrefixes.UPLOAD}{interface_slug}": data,
-                f"{INTERFACE_FORM_FIELD_PREFIX}{interface_slug}": "",
             }
-    else:
+    elif ci.super_kind == ci.SuperKind.VALUE:
         form_data = {f"{INTERFACE_FORM_FIELD_PREFIX}{interface_slug}": data}
+    else:
+        raise NotImplementedError
 
     return form_data
 

app/tests/evaluation_tests/test_forms.py

@@ -4,12 +4,10 @@
 from django.core.validators import MaxValueValidator, MinValueValidator
 from django.forms import (
     CharField,
-    HiddenInput,
     ModelChoiceField,
     ModelMultipleChoiceField,
     TextInput,
 )
-from django.forms.fields import Field
 from factory.django import ImageField
 
 from grandchallenge.algorithms.forms import (
@@ -1350,7 +1348,6 @@ def test_additional_inputs_on_submission_form():
             ImageSearchMultiField,
             ImageSearchMultiWidget,
         ),
-        f"{INTERFACE_FORM_FIELD_PREFIX}{ci_img.slug}": (Field, HiddenInput),
         f"{FlexibleWidgetPrefixes.CHOICE}{ci_dicom.slug}": (
             ImageSourceChoiceField,
             SourceSelect,
@@ -1363,7 +1360,6 @@ def test_additional_inputs_on_submission_form():
             ImageSearchMultiField,
             ImageSearchMultiWidget,
         ),
-        f"{INTERFACE_FORM_FIELD_PREFIX}{ci_dicom.slug}": (Field, HiddenInput),
         f"{INTERFACE_FORM_FIELD_PREFIX}{ci_str.slug}": (CharField, TextInput),
         f"{FlexibleWidgetPrefixes.CHOICE}{ci_file.slug}": (
             FileSourceChoiceField,
@@ -1377,7 +1373,6 @@ def test_additional_inputs_on_submission_form():
             FileSearchMultiField,
             FileSearchMultiWidget,
         ),
-        f"{INTERFACE_FORM_FIELD_PREFIX}{ci_file.slug}": (Field, HiddenInput),
     }
 
     form = SubmissionForm(

app/tests/reader_studies_tests/test_forms.py

@@ -9,7 +9,6 @@
 from django.db.models import BLANK_CHOICE_DASH
 from django.forms import (
     CharField,
-    Field,
     HiddenInput,
     JSONField,
     ModelChoiceField,
@@ -1478,7 +1477,6 @@ def test_display_set_update_form(form_class):
             FileSearchMultiField,
             FileSearchMultiWidget,
         ),
-        f"{INTERFACE_FORM_FIELD_PREFIX}{ci_file.slug}": (Field, HiddenInput),
     }
 
     form = form_class(user=user, instance=instance, base_obj=rs)
@@ -1680,7 +1678,6 @@ def test_display_set_add_interface_form():
             FileSearchMultiField,
             FileSearchMultiWidget,
         ),
-        f"{INTERFACE_FORM_FIELD_PREFIX}{ci_file.slug}": (Field, HiddenInput),
     }
 
     assert form.fields.keys() == set(expected_fields).union({"interface"})
@@ -1729,7 +1726,6 @@ def test_display_set_add_interface_form():
             ImageSearchMultiField,
             ImageSearchMultiWidget,
         ),
-        f"{INTERFACE_FORM_FIELD_PREFIX}{ci_image.slug}": (Field, HiddenInput),
     }
 
     assert form.fields.keys() == set(expected_fields).union({"interface"})
@@ -1759,7 +1755,6 @@ def test_display_set_add_interface_form():
             ImageSearchMultiField,
             ImageSearchMultiWidget,
         ),
-        f"{INTERFACE_FORM_FIELD_PREFIX}{ci_dicom.slug}": (Field, HiddenInput),
     }
 
     assert form.fields.keys() == set(expected_fields).union({"interface"})