VED-872 Tighten given name validation (#915)

Author: dlzhry2nhs

backend/src/models/constants.py

@@ -48,6 +48,11 @@ class Constants:
 
     ALLOWED_CONTAINED_RESOURCES = {"Practitioner", "Patient"}
 
+    # As per Personal Demographics Service FHIR API, the maximum length of a given name element or surname is 35 chars.
+    # Given name is a list with a maximum 5 elements. For more info see:
+    # https://digital.nhs.uk/developer/api-catalogue/personal-demographics-service-fhir#post-/Patient
+    PERSON_NAME_ELEMENT_MAX_LENGTH = 35
+
     SUPPLIER_PERMISSIONS_KEY = "supplier_permissions"
     VACCINE_TYPE_TO_DISEASES_HASH_KEY = "vacc_to_diseases"
     DISEASES_TO_VACCINE_TYPE_HASH_KEY = "diseases_to_vacc"

backend/src/models/fhir_immunization_pre_validators.py

@@ -297,22 +297,26 @@ def pre_validate_patient_name(self, values: dict) -> dict:
         except (KeyError, IndexError):
             pass
 
-    def pre_validate_patient_name_given(self, values: dict) -> dict:
+    def pre_validate_patient_name_given(self, values: dict) -> None:
         """
         Pre-validate that, if contained[?(@.resourceType=='Patient')].name[{index}].given index dynamically determined
-        (legacy CSV field name:PERSON_FORENAME) exists, then it is a an array containing a single non-empty string
+        (legacy CSV field name:PERSON_FORENAME) exists, then it is an array containing a single non-empty string
         """
         field_location = patient_name_given_field_location(values)
 
         try:
             field_value, _ = patient_and_practitioner_value_and_index(values, "given", "Patient")
-            PreValidation.for_list(field_value, field_location, elements_are_strings=True)
+            PreValidation.for_list(
+                field_value,
+                field_location,
+                elements_are_strings=True,
+                max_length=5,
+                string_element_max_length=Constants.PERSON_NAME_ELEMENT_MAX_LENGTH,
+            )
         except (KeyError, IndexError, AttributeError):
             pass
 
-    PERSON_SURNAME_MAX_LENGTH = 35
-
-    def pre_validate_patient_name_family(self, values: dict) -> dict:
+    def pre_validate_patient_name_family(self, values: dict) -> None:
         """
         Pre-validate that, if a contained[?(@.resourceType=='Patient')].name[{index}].family (legacy CSV field name:
         PERSON_SURNAME) exists, index dynamically determined then it is a non-empty string of maximum length
@@ -321,7 +325,7 @@ def pre_validate_patient_name_family(self, values: dict) -> dict:
         field_location = patient_name_family_field_location(values)
         try:
             field_value, _ = patient_and_practitioner_value_and_index(values, "family", "Patient")
-            PreValidation.for_string(field_value, field_location, max_length=self.PERSON_SURNAME_MAX_LENGTH)
+            PreValidation.for_string(field_value, field_location, max_length=Constants.PERSON_NAME_ELEMENT_MAX_LENGTH)
         except (KeyError, IndexError, AttributeError):
             pass
 
@@ -489,8 +493,8 @@ def pre_validate_practitioner_name(self, values: dict) -> dict:
     def pre_validate_practitioner_name_given(self, values: dict) -> dict:
         """
         Pre-validate that, if contained[?(@.resourceType=='Practitioner')].name[{index}].given index dynamically
-        determined (legacy CSV field name:PERSON_FORENAME) exists,
-        then it is a an array containing a single non-empty string
+        determined (legacy CSV field name:PERSON_FORENAME) exists, then it is an array containing a single non-empty
+        string
         """
         field_location = practitioner_name_given_field_location(values)
         try:

backend/src/models/utils/pre_validator_utils.py

@@ -1,6 +1,6 @@
 from datetime import date, datetime
 from decimal import Decimal
-from typing import Union
+from typing import Optional, Union
 
 from .generic_utils import is_valid_simple_snomed, nhs_number_mod11_check
 
@@ -49,14 +49,15 @@ def for_string(
     def for_list(
         field_value: list,
         field_location: str,
-        defined_length: int = None,
+        defined_length: Optional[int] = None,
+        max_length: Optional[int] = None,
         elements_are_strings: bool = False,
+        string_element_max_length: Optional[int] = None,
         elements_are_dicts: bool = False,
     ):
         """
-        Apply pre-validation to a list field to ensure it is a non-empty list which meets the length
-        requirements and requirements, if applicable, for each list element to be a non-empty string
-        or non-empty dictionary
+        Apply pre-validation to a list field to ensure it is a non-empty list which meets the length requirements and
+        requirements, if applicable, for each list element to be a non-empty string or non-empty dictionary
         """
         if not isinstance(field_value, list):
             raise TypeError(f"{field_location} must be an array")
@@ -68,12 +69,12 @@ def for_list(
             if len(field_value) == 0:
                 raise ValueError(f"{field_location} must be a non-empty array")
 
+        if max_length is not None and len(field_value) > max_length:
+            raise ValueError(f"{field_location} must be an array of maximum length {max_length}")
+
         if elements_are_strings:
-            for element in field_value:
-                if not isinstance(element, str):
-                    raise TypeError(f"{field_location} must be an array of strings")
-                if len(element) == 0:
-                    raise ValueError(f"{field_location} must be an array of non-empty strings")
+            for idx, element in enumerate(field_value):
+                PreValidation.for_string(element, f"{field_location}[{idx}]", max_length=string_element_max_length)
 
         if elements_are_dicts:
             for element in field_value:

backend/tests/test_immunization_pre_validator.py

@@ -7,6 +7,7 @@
 
 from jsonpath_ng.ext import parse
 
+from models.constants import Constants
 from models.fhir_immunization import ImmunizationValidator
 from models.fhir_immunization_pre_validators import PreValidators
 from models.utils.generic_utils import (
@@ -487,18 +488,55 @@ def test_pre_validate_patient_name(self):
             valid_list_element=[{"family": "Test", "given": ["TestA"]}],
         )
 
-    def test_pre_validate_patient_name_given(self):
-        """Test pre_validate_patient_name_given accepts valid values and rejects invalid values"""
-        valid_json_data = deepcopy(self.json_data)
-        # invalid_json
+    def test_pre_validate_patient_name_given_rejects_invalid_input(self):
+        """Test pre_validate_patient_name_given rejects invalid values"""
+        given_name_field_loc = patient_name_given_field_location(self.json_data)
+
+        test_cases = [
+            ([None], "contained[?(@.resourceType=='Patient')].name[0].given[0] must be a string"),
+            ([], "contained[?(@.resourceType=='Patient')].name[0].given must be a non-empty array"),
+            ([""], "contained[?(@.resourceType=='Patient')].name[0].given[0] must be a non-empty string"),
+            (["   \n"], "contained[?(@.resourceType=='Patient')].name[0].given[0] must be a non-empty string"),
+            (["Test", "    "], "contained[?(@.resourceType=='Patient')].name[0].given[1] must be a non-empty string"),
+            (
+                ["Too", "many", "items", "1", "2", "5"],
+                "contained[?(@.resourceType=='Patient')].name[0].given must be an array of maximum length 5",
+            ),
+            (
+                ["Stringtoolongeruti olgkriahfyrtoiuhg"],
+                "contained[?(@.resourceType=='Patient')].name[0].given[0] must be 35 or fewer characters",
+            ),
+        ]
 
-        ValidatorModelTests.test_list_value(
-            self,
-            field_location=patient_name_given_field_location(valid_json_data),
-            valid_lists_to_test=[["Test"], ["Test test"]],
-            valid_list_element="Test",
-            is_list_of_strings=True,
-        )
+        for invalid_data, expected_error in test_cases:
+            with self.subTest():
+                invalid_json_data = deepcopy(self.json_data)
+                parse(given_name_field_loc).update(invalid_json_data, invalid_data)
+
+            with self.assertRaises(Exception) as error:
+                PreValidators(invalid_json_data).pre_validate_patient_name_given(values=invalid_json_data)
+
+            self.assertEqual(str(error.exception), expected_error)
+            self.assertIsInstance(error.exception, (ValueError, TypeError))
+
+    def test_pre_validate_patient_name_given_accepts_valid_input(self):
+        """Test pre_validate_patient_name_given accepts valid values"""
+        given_name_field_loc = patient_name_given_field_location(self.json_data)
+
+        test_cases = [
+            ["Test"],
+            ["Multiple test", "values"],
+            ["One", "Two", "Three", "Four", "Five"],
+            ["  Can have spaces "],
+            ["Exactlythirtyfivecharactersuiopasdf"],
+        ]
+
+        for valid_input in test_cases:
+            with self.subTest():
+                valid_json_data = deepcopy(self.json_data)
+                parse(given_name_field_loc).update(valid_json_data, valid_input)
+
+                PreValidators(valid_json_data).pre_validate_patient_name_given(values=valid_json_data)
 
     def test_pre_validate_patient_name_family(self):
         """Test pre_validate_patient_name_family accepts valid values and rejects invalid values"""
@@ -507,7 +545,7 @@ def test_pre_validate_patient_name_family(self):
             self,
             field_location=patient_name_family_field_location(valid_json_data),
             valid_strings_to_test=["test", "Quitelongsurname", "Surnamewithjustthirtyfivecharacters"],
-            max_length=PreValidators.PERSON_SURNAME_MAX_LENGTH,
+            max_length=Constants.PERSON_NAME_ELEMENT_MAX_LENGTH,
             invalid_length_strings_to_test=["Surnamethathasgotthirtysixcharacters"],
         )
 
@@ -666,16 +704,50 @@ def test_pre_validate_practitioner_name(self):
             valid_list_element={"family": "Test"},
         )
 
-    def test_pre_validate_practitioner_name_given(self):
-        """Test pre_validate_practitioner_name_given accepts valid values and rejects invalid values"""
-        valid_json_data = deepcopy(self.json_data)
-        ValidatorModelTests.test_list_value(
-            self,
-            field_location=practitioner_name_given_field_location(valid_json_data),
-            valid_lists_to_test=[["Test"], ["Test test"]],
-            valid_list_element="Test",
-            is_list_of_strings=True,
-        )
+    def test_pre_validate_practitioner_name_given_rejects_invalid_data(self):
+        """Test pre_validate_practitioner_name_given rejects invalid values"""
+        practitioner_name_field_loc = practitioner_name_given_field_location(self.json_data)
+
+        test_cases = [
+            ([None], "contained[?(@.resourceType=='Practitioner')].name[0].given[0] must be a string"),
+            ([123, 456], "contained[?(@.resourceType=='Practitioner')].name[0].given[0] must be a string"),
+            ([], "contained[?(@.resourceType=='Practitioner')].name[0].given must be a non-empty array"),
+            ([""], "contained[?(@.resourceType=='Practitioner')].name[0].given[0] must be a non-empty string"),
+            (["   \n"], "contained[?(@.resourceType=='Practitioner')].name[0].given[0] must be a non-empty string"),
+            (
+                ["Test", "    "],
+                "contained[?(@.resourceType=='Practitioner')].name[0].given[1] must be a non-empty string",
+            ),
+        ]
+
+        for invalid_data, expected_error in test_cases:
+            with self.subTest():
+                invalid_json_data = deepcopy(self.json_data)
+                parse(practitioner_name_field_loc).update(invalid_json_data, invalid_data)
+
+            with self.assertRaises(Exception) as error:
+                PreValidators(invalid_json_data).pre_validate_practitioner_name_given(values=invalid_json_data)
+
+            self.assertEqual(str(error.exception), expected_error)
+            self.assertIsInstance(error.exception, (ValueError, TypeError))
+
+    def test_pre_validate_practitioner_name_given_accepts_valid_input(self):
+        """Test pre_validate_practitioner_name_given accepts valid values"""
+        practitioner_name_field_loc = practitioner_name_given_field_location(self.json_data)
+
+        test_cases = [
+            ["Test"],
+            ["Multiple test", "values"],
+            ["One", "Two", "Three", "Four", "Five", "Very many not restricted by length"],
+            ["  Can have spaces "],
+        ]
+
+        for valid_input in test_cases:
+            with self.subTest():
+                valid_json_data = deepcopy(self.json_data)
+                parse(practitioner_name_field_loc).update(valid_json_data, valid_input)
+
+                PreValidators(valid_json_data).pre_validate_practitioner_name_given(values=valid_json_data)
 
     def test_pre_validate_practitioner_name_family(self):
         """Test pre_validate_practitioner_name_family accepts valid values and rejects invalid values"""

backend/tests/testing_utils/pre_validation_test_utils.py

@@ -158,7 +158,7 @@ def test_list_value(
             that a valid list element must be supplied when a predefined list length is given as
             the valid element will be used to populate lists of incorrect length to ensure
             that the error is being raised due to length, not due to use of an invalid list element)
-        * If there is no predfined list length: Empty list
+        * If there is no predefined list length: Empty list
         * If is a list of strings: Lists with non-string or empty string elements
         * If is a list of dicts: Lists with non-dict or empty dict elements