Move validation from reformat function to serializer

Author: koopmant

app/grandchallenge/components/serializers.py

@@ -29,41 +29,19 @@
 
 def reformat_serialized_civ_data(*, serialized_civ_data):
     """Takes serialized CIV data and returns list of CIVData objects."""
-
-    possible_keys = [
-        "image",
-        "value",
-        "file",
-        "user_upload",
-        "upload_session",
-        ("image_name", "user_uploads"),
-    ]
-
     civ_data_objects = []
     for civ in serialized_civ_data:
         interface = civ["interface"]
 
         keys = set(civ.keys()) - {"interface"}
 
-        if not keys:
-            raise serializers.ValidationError(
-                f"You must provide at least one of {possible_keys}."
-            )
-        elif keys == {"image_name", "user_uploads"}:
+        if keys == {"image_name", "user_uploads"}:
             value = DICOMUploadWithName(
                 name=civ["image_name"],
                 user_uploads=civ["user_uploads"],
             )
-        elif len(keys) > 1:
-            raise serializers.ValidationError(
-                f"You can only provide one of {possible_keys} for each socket."
-            )
-        elif (key := keys.pop()) in ("image_name", "user_uploads"):
-            raise serializers.ValidationError(
-                "You must provide 'image_name' and 'user_uploads' together."
-            )
         else:
-            value = civ[key]
+            value = civ[keys.pop()]
 
         try:
             civ_data_objects.append(
@@ -193,16 +171,15 @@ def __init__(self, *args, **kwargs):
             )
 
     def validate(self, attrs):
-        if "interface" not in attrs:
-            raise serializers.ValidationError("An interface must be specified")
+        self._validate_provided_fields(attrs=attrs)
 
         interface = attrs["interface"]
 
         if interface.super_kind == interface.SuperKind.IMAGE:
             if interface.is_dicom_image_kind:
-                self._validate_dicom_image(attrs, interface)
+                self._validate_dicom_image(attrs=attrs, interface=interface)
             else:
-                self._validate_panimg_image(attrs, interface)
+                self._validate_panimg_image(attrs=attrs, interface=interface)
         elif interface.super_kind == interface.SuperKind.VALUE:
             if (
                 attrs.get("value") is None
@@ -240,7 +217,33 @@ def validate(self, attrs):
 
         return attrs
 
-    def _validate_panimg_image(self, attrs, interface):
+    @staticmethod
+    def _validate_provided_fields(*, attrs):
+        if "interface" not in attrs:
+            raise serializers.ValidationError("An interface must be specified")
+
+        possible_keys = [
+            "image",
+            "value",
+            "file",
+            "user_upload",
+            "upload_session",
+            ("image_name", "user_uploads"),
+        ]
+
+        keys = set(attrs.keys()) - {"interface"}
+
+        if not keys:
+            raise serializers.ValidationError(
+                f"You must provide at least one of {possible_keys}."
+            )
+        elif len(keys) > 1 and keys != {"image_name", "user_uploads"}:
+            raise serializers.ValidationError(
+                f"You can only provide one of {possible_keys} for each socket."
+            )
+
+    @staticmethod
+    def _validate_panimg_image(*, attrs, interface):
         if not any(
             [
                 attrs.get("image"),
@@ -252,7 +255,8 @@ def _validate_panimg_image(self, attrs, interface):
                 f"kind {interface.kind}"
             )
 
-    def _validate_dicom_image(self, attrs, interface):
+    @staticmethod
+    def _validate_dicom_image(*, attrs, interface):
         if not (
             attrs.get("image")
             or (attrs.get("user_uploads") and attrs.get("image_name"))

app/tests/components_tests/test_serializers.py

@@ -1,8 +1,5 @@
-from contextlib import nullcontext
-
 import pytest
 from guardian.shortcuts import assign_perm
-from rest_framework import serializers
 
 from grandchallenge.cases.models import RawImageUploadSession
 from grandchallenge.components.models import (
@@ -312,34 +309,55 @@ def test_civ_post_value_validation(kind):
 
 @pytest.mark.django_db
 @pytest.mark.parametrize("kind", InterfaceKinds.json)
-@pytest.mark.parametrize(
-    "store_in_database, expected_error",
-    (
-        (True, "value is required for interface kind {kind}"),
-        (False, "user_upload or file is required for interface kind {kind}"),
-    ),
-)
-def test_civ_post_value_or_user_upload_required_validation(
-    kind, store_in_database, expected_error
-):
-    # setup
+def test_civ_post_value_required_validation(kind):
     interface = ComponentInterfaceFactory(
         kind=kind,
-        store_in_database=store_in_database,
+        store_in_database=True,
     )
 
-    payload = {"interface": interface.slug}
+    for kwargs in [
+        {"image": ""},
+        {"user_upload": ""},
+        {"upload_session": ""},
+        {"image_name": "foo", "user_uploads": [""]},
+    ]:
+        payload = {"interface": interface.slug, **kwargs}
 
-    # test
-    serializer = ComponentInterfaceValuePostSerializer(data=payload)
+        serializer = ComponentInterfaceValuePostSerializer(data=payload)
 
-    # verify
-    assert not serializer.is_valid()
-    assert (
-        expected_error.format(kind=kind)
-        in serializer.errors["non_field_errors"]
+        assert not serializer.is_valid()
+        assert (
+            f"value is required for interface kind {kind}"
+            in serializer.errors["non_field_errors"]
+        )
+
+
+@pytest.mark.django_db
+@pytest.mark.parametrize("kind", InterfaceKinds.json)
+def test_civ_post_file_or_user_upload_required_validation(kind):
+    interface = ComponentInterfaceFactory(
+        kind=kind,
+        store_in_database=False,
     )
 
+    for kwargs in [
+        {"image": ""},
+        {"value": "foo"},
+        {"upload_session": ""},
+        {"image_name": "foo", "user_uploads": [""]},
+    ]:
+        payload = {"interface": interface.slug, **kwargs}
+
+        # test
+        serializer = ComponentInterfaceValuePostSerializer(data=payload)
+
+        # verify
+        assert not serializer.is_valid()
+        assert (
+            f"user_upload or file is required for interface kind {kind}"
+            in serializer.errors["non_field_errors"]
+        )
+
 
 @pytest.mark.django_db
 @pytest.mark.parametrize("kind", InterfaceKinds.json)
@@ -398,32 +416,43 @@ def test_civ_post_image_or_upload_required_validation(kind):
     # setup
     interface = ComponentInterfaceFactory(kind=kind)
 
-    payload = {"interface": interface.slug}
+    for kwargs in [
+        {"value": "foo"},
+        {"upload_session": ""},
+        {"image_name": "foo", "user_uploads": [""]},
+    ]:
+        payload = {"interface": interface.slug, **kwargs}
 
-    # test
-    serializer = ComponentInterfaceValuePostSerializer(data=payload)
+        # test
+        serializer = ComponentInterfaceValuePostSerializer(data=payload)
 
-    # verify
-    assert not serializer.is_valid()
-    assert (
-        f"upload_session or image are required for interface kind {kind}"
-        in serializer.errors["non_field_errors"]
-    )
+        # verify
+        assert not serializer.is_valid()
+        assert (
+            f"upload_session or image are required for interface kind {kind}"
+            in serializer.errors["non_field_errors"]
+        )
 
 
 @pytest.mark.django_db
 @pytest.mark.parametrize("kind,", InterfaceKinds.dicom)
 def test_civ_post_dicom_image_or_upload_required_validation(kind, rf):
     interface = ComponentInterfaceFactory(kind=kind)
-    payload = {"interface": interface.slug}
 
-    serializer = ComponentInterfaceValuePostSerializer(data=payload)
+    for kwargs in [
+        {"value": "foo"},
+        {"user_upload": ""},
+        {"upload_session": ""},
+    ]:
+        payload = {"interface": interface.slug, **kwargs}
 
-    assert not serializer.is_valid()
-    assert (
-        f"either user_uploads with image_name, or image are required for interface kind {kind}"
-        in serializer.errors["non_field_errors"]
-    )
+        serializer = ComponentInterfaceValuePostSerializer(data=payload)
+
+        assert not serializer.is_valid()
+        assert (
+            f"either user_uploads with image_name, or image are required for interface kind {kind}"
+            in serializer.errors["non_field_errors"]
+        )
 
     payload = {"interface": interface.slug, "image_name": "foobar"}
 
@@ -733,55 +762,3 @@ def test_reformat_serialized_civ_data():
     assert civ_data_objects[3].dicom_upload_with_name.name == "a dcm"
     assert civ_data_objects[3].dicom_upload_with_name.user_uploads == uploads
     assert civ_data_objects[4].user_upload == uploads[0]
-
-
-def test_reformat_serialized_civ_data_invalid_no_keys():
-    data = [{"interface": ""}]
-
-    with pytest.raises(serializers.ValidationError) as e:
-        reformat_serialized_civ_data(serialized_civ_data=data)
-
-    assert (
-        "You must provide at least one of ['image', 'value', 'file', "
-        "'user_upload', 'upload_session', ('image_name', 'user_uploads')]."
-    ) in str(e.value)
-
-
-@pytest.mark.django_db
-def test_reformat_serialized_civ_data_invalid_multiple_keys():
-    data = [{"interface": "some_socket_slug", "value": "foo", "image": "foo"}]
-
-    with pytest.raises(serializers.ValidationError) as e:
-        reformat_serialized_civ_data(serialized_civ_data=data)
-
-    assert (
-        "You can only provide one of ['image', 'value', 'file', "
-        "'user_upload', 'upload_session', ('image_name', 'user_uploads')] for each socket."
-    ) in str(e.value)
-
-    # Using multiple keys (image_name and user_uploads) is necessary for DICOM
-    ci_dcm = ComponentInterfaceFactory(
-        kind=InterfaceKindChoices.DICOM_IMAGE_SET
-    )
-    data = [
-        {"interface": ci_dcm, "image_name": "foo", "user_uploads": ["foo"]}
-    ]
-
-    with nullcontext():
-        reformat_serialized_civ_data(serialized_civ_data=data)
-
-
-@pytest.mark.django_db
-def test_reformat_serialized_civ_data_invalid_dicom_keys():
-    ci_dcm = ComponentInterfaceFactory(
-        kind=InterfaceKindChoices.DICOM_IMAGE_SET
-    )
-    for key in ("image_name", "user_uploads"):
-        data = [{"interface": ci_dcm, key: "foo"}]
-
-        with pytest.raises(serializers.ValidationError) as e:
-            reformat_serialized_civ_data(serialized_civ_data=data)
-
-        assert (
-            "You must provide 'image_name' and 'user_uploads' together."
-        ) in str(e.value)