VED-1049: Make invalid -immunization.target behaviour consistent in the search API (#1235)

* Better FHIR search functionality to handle invalid immunization targets

- Added support for `invalid_immunization_targets` in `FhirController` and `FhirService`.
- Updated `process_immunization_target` to return both valid and invalid vaccine types.
- Modified `validate_and_retrieve_search_params` to include invalid targets in the search parameters.
- Implemented logic to include an `OperationOutcome` in the response when invalid targets are provided.
- Updated tests to cover scenarios for invalid immunization targets and ensure correct handling in search results.

* Empty-Commit

* Rename test method to clarify error handling for invalid immunization targets

* Refactor search parameter validation to return structured results

- Updated `validate_and_retrieve_search_params` to return a `SearchParamsResult` object, encapsulating both valid parameters and invalid immunization targets.
- Adjusted `_search_immunizations` method in `FhirController` to utilize the new result structure.
- Modified tests to reflect changes in the validation function and ensure correct handling of search parameters.

* Implement smoke tests for Search API with mixed valid and invalid Disease Types

- Added a new scenario to verify that the Search API returns a 200 status code with results and an OperationOutcome when both valid and invalid Disease Types are provided.
- Implemented corresponding step definitions for GET and POST requests to handle mixed Disease Types.
- Better validation to ensure the response includes search results and appropriate OperationOutcome for invalid immunization targets.

* ruff fix

* more ruff fixes

* ruff fix

* ruff fixes

Author: Thomas-Boyle

lambdas/backend/src/controller/fhir_controller.py

@@ -154,15 +154,16 @@ def _get_immunization_by_identifier(self, search_params: dict[str, list[str]], s
         return create_response(200, prepared_search_bundle)
 
     def _search_immunizations(self, search_params: dict[str, list[str]], supplier_system: str) -> dict:
-        validated_search_params = validate_and_retrieve_search_params(search_params)
+        result = validate_and_retrieve_search_params(search_params)
 
         search_bundle = self.fhir_service.search_immunizations(
-            validated_search_params.patient_identifier,
-            validated_search_params.immunization_targets,
+            result.params.patient_identifier,
+            result.params.immunization_targets,
             supplier_system,
-            validated_search_params.date_from,
-            validated_search_params.date_to,
-            validated_search_params.include,
+            result.params.date_from,
+            result.params.date_to,
+            result.params.include,
+            result.invalid_immunization_targets,
         )
 
         if self._has_too_many_search_results(search_bundle):

lambdas/backend/src/controller/parameter_parser.py

@@ -1,5 +1,5 @@
 import datetime
-from dataclasses import dataclass
+from dataclasses import dataclass, field
 from typing import Optional
 
 from common.models.constants import RedisHashKeys
@@ -33,6 +33,12 @@ def __repr__(self):
         return str(self.__dict__)
 
 
+@dataclass
+class SearchParamsResult:
+    params: SearchParams
+    invalid_immunization_targets: list[str] = field(default_factory=list)
+
+
 def process_patient_identifier(identifier_params: dict[str, list[str]]) -> str:
     """Validate and parse patient identifier parameter.
 
@@ -65,10 +71,9 @@ def process_patient_identifier(identifier_params: dict[str, list[str]]) -> str:
     return nhs_number
 
 
-def process_immunization_target(imms_params: dict[str, list[str]]) -> set[str]:
-    """Validate and parse immunization target parameter.
-
-    :raises ParameterExceptionError:
+def process_immunization_target(imms_params: dict[str, list[str]]) -> tuple[list[str], list[str]]:
+    """Validate and parse immunization target parameter. Returns (valid_vaccine_types, invalid_vaccine_types).
+    Raises ParameterExceptionError only when no values provided or all values are invalid.
     """
     vaccine_types = [
         vaccine_type
@@ -81,24 +86,27 @@ def process_immunization_target(imms_params: dict[str, list[str]]) -> set[str]:
             f"Search parameter {ImmunizationSearchParameterName.IMMUNIZATION_TARGET} must have one or more values."
         )
 
-    valid_vaccine_types = get_redis_client().hkeys(RedisHashKeys.VACCINE_TYPE_TO_DISEASES_HASH_KEY)
-    if any(x not in valid_vaccine_types for x in vaccine_types):
+    valid_vaccine_types_set = set(get_redis_client().hkeys(RedisHashKeys.VACCINE_TYPE_TO_DISEASES_HASH_KEY))
+    valid = [v for v in vaccine_types if v in valid_vaccine_types_set]
+    invalid = [v for v in vaccine_types if v not in valid_vaccine_types_set]
+
+    if not valid:
         raise ParameterExceptionError(
             f"{ImmunizationSearchParameterName.IMMUNIZATION_TARGET} must be one or more of the following: "
-            f"{', '.join(valid_vaccine_types)}"
+            f"{', '.join(sorted(valid_vaccine_types_set))}"
         )
 
-    return set(vaccine_types)
+    return valid, invalid
 
 
-def process_mandatory_params(params: dict[str, list[str]]) -> tuple[str, set[str]]:
-    """Validate mandatory params and return (patient_identifier, vaccine_types).
+def process_mandatory_params(params: dict[str, list[str]]) -> tuple[str, list[str], list[str]]:
+    """Validate mandatory params and return (patient_identifier, valid_vaccine_types, invalid_vaccine_types).
     Raises ParameterExceptionError for any validation error.
     """
     patient_identifier = process_patient_identifier(params)
-    vaccine_types = process_immunization_target(params)
+    vaccine_types, invalid_vaccine_types = process_immunization_target(params)
 
-    return patient_identifier, vaccine_types
+    return patient_identifier, vaccine_types, invalid_vaccine_types
 
 
 def process_optional_params(
@@ -146,11 +154,11 @@ def process_optional_params(
     return date_from, date_to, include
 
 
-def validate_and_retrieve_search_params(params: dict[str, list[str]]) -> SearchParams:
+def validate_and_retrieve_search_params(params: dict[str, list[str]]) -> SearchParamsResult:
     """Validate and retrieve search parameters.
     :raises ParameterExceptionError:
     """
-    patient_identifier, vaccine_types = process_mandatory_params(params)
+    patient_identifier, vaccine_types, invalid_vaccine_types = process_mandatory_params(params)
     date_from, date_to, include = process_optional_params(params)
 
     if date_from and date_to and date_from > date_to:
@@ -159,7 +167,8 @@ def validate_and_retrieve_search_params(params: dict[str, list[str]]) -> SearchP
             f"{ImmunizationSearchParameterName.DATE_TO}"
         )
 
-    return SearchParams(patient_identifier, vaccine_types, date_from, date_to, include)
+    search_params = SearchParams(patient_identifier, set(vaccine_types), date_from, date_to, include)
+    return SearchParamsResult(params=search_params, invalid_immunization_targets=invalid_vaccine_types)
 
 
 def parse_search_params(search_params_in_req: dict[str, list[str]]) -> dict[str, list[str]]:

lambdas/backend/src/service/fhir_service.py

@@ -198,6 +198,7 @@ def search_immunizations(
         date_from: Optional[datetime.date],
         date_to: Optional[datetime.date],
         include: Optional[str],
+        invalid_immunization_targets: Optional[list[str]] = None,
     ) -> FhirBundle:
         """
         Finds all instances of Immunization(s) for a specified patient for the given specified vaccine type(s).
@@ -262,6 +263,21 @@ def search_immunizations(
                 )
             )
 
+        if invalid_immunization_targets:
+            invalid_list = ", ".join(sorted(invalid_immunization_targets))
+            entries.append(
+                BundleEntry(
+                    resource=OperationOutcome.construct(
+                        **create_operation_outcome(
+                            resource_id=str(uuid.uuid4()),
+                            severity=Severity.warning,
+                            code=Code.invalid,
+                            diagnostics=f"Your search included invalid -immunization.target value(s) that were ignored: {invalid_list}. The search was performed using the valid value(s) only.",
+                        )
+                    )
+                )
+            )
+
         return FhirBundle(
             type="searchset",
             entry=entries,

lambdas/backend/tests/controller/test_fhir_controller.py

@@ -995,7 +995,7 @@ def test_search_immunizations_is_successful(self):
 
         # Then
         self.service.search_immunizations.assert_called_once_with(
-            self.nhs_number_valid_value, {vaccine_type}, "test", None, None, None
+            self.nhs_number_valid_value, {vaccine_type}, "test", None, None, None, []
         )
         self.assertEqual(response["statusCode"], 200)
         self.assertEqual(
@@ -1038,7 +1038,7 @@ def test_search_immunizations_is_successful_when_using_the_post_endpoint(self):
 
         # Then
         self.service.search_immunizations.assert_called_once_with(
-            self.nhs_number_valid_value, {vaccine_type}, "test", None, None, None
+            self.nhs_number_valid_value, {vaccine_type}, "test", None, None, None, []
         )
         self.assertEqual(response["statusCode"], 200)
         self.assertEqual(
@@ -1111,17 +1111,17 @@ def test_search_immunizations_returns_a_validation_error_when_patient_identifier
                 )
                 self.service.search_immunizations.assert_not_called()
 
-    def test_search_immunizations_returns_a_validation_error_when_immunization_target_invalid(self):
-        """it should return a 400 invalid error for multiple invalid -immunization.target scenarios"""
+    def test_search_immunizations_returns_400_when_immunization_target_empty_or_all_invalid(self):
+        """it should return 400 when -immunization.target has no values or only invalid values"""
         test_cases = [
             ([], "Search parameter -immunization.target must have one or more values."),
-            (["COVID,FLU,CHICKENS"], "-immunization.target must be one or more of the following: COVID, FLU"),
+            (["CHICKENS"], "-immunization.target must be one or more of the following: COVID, FLU"),
         ]
 
-        for test_patient_id, expected_error in test_cases:
+        for target_values, expected_error in test_cases:
             with self.subTest(msg=expected_error):
                 test_lambda_event = copy.deepcopy(self.test_lambda_event)
-                test_lambda_event["multiValueQueryStringParameters"]["-immunization.target"] = test_patient_id
+                test_lambda_event["multiValueQueryStringParameters"]["-immunization.target"] = target_values
 
                 # When
                 response = self.controller.search_immunizations(test_lambda_event)
@@ -1134,6 +1134,32 @@ def test_search_immunizations_returns_a_validation_error_when_immunization_targe
                 )
                 self.service.search_immunizations.assert_not_called()
 
+    def test_search_immunizations_returns_200_with_operation_outcome_for_invalid_targets(self):
+        """it should return searchset with data for valid targets and OperationOutcome for invalid -immunization.target"""
+        self.service.search_immunizations.return_value = Bundle.construct(
+            entry=[BundleEntry.construct(resource=Immunization.construct(**{"id": "something"}))],
+            link=[BundleLink.construct(relation="self", url="patient-search-url")],
+            type="searchset",
+            total=1,
+        )
+        test_lambda_event = copy.deepcopy(self.test_lambda_event)
+        test_lambda_event["multiValueQueryStringParameters"]["-immunization.target"] = ["COVID,FLU,CHICKENS"]
+
+        # When
+        response = self.controller.search_immunizations(test_lambda_event)
+
+        # Then
+        self.assertEqual(response["statusCode"], 200)
+        self.service.search_immunizations.assert_called_once_with(
+            self.nhs_number_valid_value,
+            {"COVID", "FLU"},
+            "test",
+            datetime.date(2000, 1, 1),
+            datetime.date(2024, 1, 1),
+            "Immunization:patient",
+            ["CHICKENS"],
+        )
+
     def test_search_immunizations_returns_a_validation_error_when_optional_params_invalid(self):
         """it should return a 400 invalid error for multiple invalid optional parameter scenarios"""
         test_cases = [
@@ -1186,6 +1212,7 @@ def test_search_immunizations_raises_error_if_too_many_results_found(self):
             datetime.date(2000, 1, 1),
             datetime.date(2024, 1, 1),
             "Immunization:patient",
+            [],
         )
         self.assertEqual(response["statusCode"], 400)
         self.assertEqual(

lambdas/backend/tests/controller/test_parameter_parser.py

@@ -43,43 +43,50 @@ def test_process_search_params_checks_patient_identifier_format(self):
             }
         )
 
-    def test_process_search_params_whitelists_immunization_target(self):
-        mock_redis_key = "RSV"
-        self.mock_redis.hkeys.return_value = [mock_redis_key]
+    def test_process_search_params_raises_error_when_all_immunization_targets_invalid(self):
+        self.mock_redis.hkeys.return_value = ["RSV"]
         self.mock_redis_getter.return_value = self.mock_redis
 
         with self.assertRaises(ParameterExceptionError) as e:
             validate_and_retrieve_search_params(
                 {
                     self.patient_identifier_key: ["https://fhir.nhs.uk/Id/nhs-number|9000000009"],
-                    self.immunization_target_key: [
-                        "FLU",
-                        "COVID",
-                        "NOT-A-REAL-VALUE",
-                    ],
+                    self.immunization_target_key: ["NOT-A-REAL-VALUE"],
                 }
             )
-        self.assertEqual(
-            str(e.exception), f"-immunization.target must be one or more of the following: {mock_redis_key}"
+        self.assertEqual(str(e.exception), "-immunization.target must be one or more of the following: RSV")
+
+    def test_process_search_params_filters_invalid_immunization_targets_and_returns_valid_plus_invalid_list(self):
+        self.mock_redis.hkeys.return_value = ["RSV", "FLU", "COVID"]
+        self.mock_redis_getter.return_value = self.mock_redis
+
+        result = validate_and_retrieve_search_params(
+            {
+                self.patient_identifier_key: ["https://fhir.nhs.uk/Id/nhs-number|9000000009"],
+                self.immunization_target_key: ["FLU", "COVID", "NOT-A-REAL-VALUE"],
+            }
         )
+        self.assertCountEqual(result.params.immunization_targets, {"FLU", "COVID"})
+        self.assertCountEqual(result.invalid_immunization_targets, ["NOT-A-REAL-VALUE"])
 
     def test_process_search_params_immunization_target(self):
         mock_redis_key = "RSV"
         self.mock_redis.hkeys.return_value = [mock_redis_key]
         self.mock_redis_getter.return_value = self.mock_redis
-        params = validate_and_retrieve_search_params(
+        result = validate_and_retrieve_search_params(
             {
                 self.patient_identifier_key: ["https://fhir.nhs.uk/Id/nhs-number|9000000009"],
                 self.immunization_target_key: ["RSV"],
             }
         )
 
-        self.assertIsNotNone(params)
+        self.assertIsNotNone(result.params)
+        self.assertEqual(result.invalid_immunization_targets, [])
 
     def test_search_params_date_from_must_be_before_date_to(self):
         self.mock_redis.hkeys.return_value = ["RSV"]
         self.mock_redis_getter.return_value = self.mock_redis
-        params = validate_and_retrieve_search_params(
+        result = validate_and_retrieve_search_params(
             {
                 self.patient_identifier_key: ["https://fhir.nhs.uk/Id/nhs-number|9000000009"],
                 self.immunization_target_key: ["RSV"],
@@ -88,9 +95,9 @@ def test_search_params_date_from_must_be_before_date_to(self):
             }
         )
 
-        self.assertIsNotNone(params)
+        self.assertIsNotNone(result.params)
 
-        params = validate_and_retrieve_search_params(
+        result = validate_and_retrieve_search_params(
             {
                 self.patient_identifier_key: ["https://fhir.nhs.uk/Id/nhs-number|9000000009"],
                 self.immunization_target_key: ["RSV"],
@@ -99,7 +106,7 @@ def test_search_params_date_from_must_be_before_date_to(self):
             }
         )
 
-        self.assertIsNotNone(params)
+        self.assertIsNotNone(result.params)
 
         with self.assertRaises(ParameterExceptionError) as e:
             _ = validate_and_retrieve_search_params(
@@ -149,7 +156,7 @@ def test_process_search_params_dedupes_immunization_targets_and_respects_include
         self.mock_redis.hkeys.return_value = ["RSV", "FLU"]
         self.mock_redis_getter.return_value = self.mock_redis
 
-        params = validate_and_retrieve_search_params(
+        result = validate_and_retrieve_search_params(
             {
                 self.patient_identifier_key: ["https://fhir.nhs.uk/Id/nhs-number|9000000009"],
                 self.immunization_target_key: ["RSV", "RSV", "FLU"],
@@ -158,11 +165,11 @@ def test_process_search_params_dedupes_immunization_targets_and_respects_include
         )
 
         # immunization targets should be deduped and preserve valid entries
-        self.assertIsInstance(params.immunization_targets, set)
-        self.assertCountEqual(params.immunization_targets, {"RSV", "FLU"})
+        self.assertIsInstance(result.params.immunization_targets, set)
+        self.assertCountEqual(result.params.immunization_targets, {"RSV", "FLU"})
 
         # include should be returned as provided
-        self.assertEqual(params.include, "immunization:patient")
+        self.assertEqual(result.params.include, "immunization:patient")
 
     def test_process_search_params_raises_date_errors(self):
         """When multiple date-related errors occur they should be returned together."""

lambdas/backend/tests/service/test_fhir_service.py

@@ -966,3 +966,39 @@ def test_search_raises_unauthorised_error_if_no_permissions(self):
             self.MOCK_SUPPLIER_SYSTEM_NAME, ApiOperationCode.SEARCH, {vaccine_type}
         )
         self.imms_repo.find_immunizations.assert_not_called()
+
+    def test_search_immunizations_includes_operation_outcome_when_invalid_immunization_targets_provided(self):
+        """it should include an OperationOutcome in the bundle when invalid -immunization.target values were provided"""
+        mock_resource = create_covid_immunization_dict("1234-some-id")
+        vaccine_type = "COVID"
+        self.authoriser.filter_permitted_vacc_types.return_value = {vaccine_type}
+        self.imms_repo.find_immunizations.return_value = [mock_resource]
+
+        result = self.fhir_service.search_immunizations(
+            VALID_NHS_NUMBER,
+            {vaccine_type},
+            self.MOCK_SUPPLIER_SYSTEM_NAME,
+            None,
+            None,
+            None,
+            invalid_immunization_targets=["TEST_VALUE", "CHICKENS"],
+        )
+
+        self.assertEqual(result.type, "searchset")
+        self.assertEqual(result.total, 1)
+        self.assertEqual(len(result.entry), 3)
+        self.assertEqual(result.entry[0].resource.resource_type, "Immunization")
+        self.assertEqual(result.entry[1].resource.resource_type, "Patient")
+        self.assertEqual(result.entry[2].resource.resource_type, "OperationOutcome")
+        issue_0 = result.entry[2].resource.issue[0]
+        diagnostics = issue_0["diagnostics"] if isinstance(issue_0, dict) else issue_0.diagnostics
+        self.assertIn("TEST_VALUE", diagnostics)
+        self.assertIn("CHICKENS", diagnostics)
+        self.assertIn("invalid -immunization.target value(s) that were ignored", diagnostics)
+        self.assertEqual(
+            result.link[0].url,
+            "https://internal-dev.api.service.nhs.uk/immunisation-fhir-api/FHIR/R4/Immunization"
+            "?immunization.target=COVID"
+            "&-immunization.target=COVID"
+            "&patient.identifier=https%3A%2F%2Ffhir.nhs.uk%2FId%2Fnhs-number%7C9990548609",
+        )

lambdas/shared/src/common/api_clients/errors.py

@@ -38,7 +38,7 @@ def to_operation_outcome() -> dict:
         return create_operation_outcome(
             resource_id=str(uuid.uuid4()),
             severity=Severity.error,
-            code=Code.invalid,
+            code=Code.invalid_access_token,
             diagnostics=msg,
         )
 

lambdas/shared/src/common/models/errors.py

@@ -6,7 +6,8 @@
 class Code(str, Enum):
     forbidden = "forbidden"
     not_found = "not-found"
-    invalid = "invalid or missing access token"
+    invalid = "invalid"
+    invalid_access_token = "invalid access token"
     exception = "exception"
     server_error = "internal server error"
     invariant = "invariant"