Address review comments

Author: edhall-nhs

lambdas/backend/src/controller/aws_apig_event_utils.py

@@ -5,7 +5,7 @@
 from aws_lambda_typing.events import APIGatewayProxyEventV1
 
 from controller.constants import E_TAG_HEADER_NAME, SUPPLIER_SYSTEM_HEADER_NAME
-from models.errors import ResourceVersionNotProvided, UnauthorizedError
+from models.errors import ResourceVersionNotProvidedError, UnauthorizedError
 from utils import dict_utils
 
 
@@ -30,6 +30,6 @@ def get_resource_version_header(event: APIGatewayProxyEventV1) -> str:
     resource_version_header: Optional[str] = dict_utils.get_field(dict(event), "headers", E_TAG_HEADER_NAME)
 
     if resource_version_header is None:
-        raise ResourceVersionNotProvided(resource_type="Immunization")
+        raise ResourceVersionNotProvidedError(resource_type="Immunization")
 
     return resource_version_header

lambdas/backend/src/controller/fhir_api_exception_handler.py

@@ -10,18 +10,18 @@
     IdentifierDuplicationError,
     InconsistentIdentifierError,
     InconsistentResourceVersion,
-    InvalidStoredData,
     ResourceNotFoundError,
 )
 from constants import GENERIC_SERVER_ERROR_DIAGNOSTICS_MESSAGE
 from controller.aws_apig_response_utils import create_response
 from models.errors import (
     Code,
     InconsistentIdError,
-    InvalidImmunizationId,
+    InvalidImmunizationIdError,
     InvalidJsonError,
-    InvalidResourceVersion,
-    ResourceVersionNotProvided,
+    InvalidResourceVersionError,
+    InvalidStoredDataError,
+    ResourceVersionNotProvidedError,
     Severity,
     UnauthorizedError,
     UnauthorizedVaxError,
@@ -33,17 +33,17 @@
     InconsistentResourceVersion: 400,
     InconsistentIdentifierError: 400,  # Identifier refers to the local FHIR identifier composed of system and value.
     InconsistentIdError: 400,  # ID refers to the top-level ID of the FHIR resource.
-    InvalidImmunizationId: 400,
+    InvalidImmunizationIdError: 400,
     InvalidJsonError: 400,
-    InvalidResourceVersion: 400,
+    InvalidResourceVersionError: 400,
     CustomValidationError: 400,
-    ResourceVersionNotProvided: 400,
+    ResourceVersionNotProvidedError: 400,
     UnauthorizedError: 403,
     UnauthorizedVaxError: 403,
     ResourceNotFoundError: 404,
     IdentifierDuplicationError: 422,
     UnhandledResponseError: 500,
-    InvalidStoredData: 500,
+    InvalidStoredDataError: 500,
 }
 
 

lambdas/backend/src/controller/fhir_controller.py

@@ -22,10 +22,10 @@
 from models.errors import (
     Code,
     InconsistentIdError,
-    InvalidImmunizationId,
+    InvalidImmunizationIdError,
     InvalidJsonError,
-    InvalidResourceVersion,
-    ParameterException,
+    InvalidResourceVersionError,
+    ParameterExceptionError,
     Severity,
     UnauthorizedError,
     UnauthorizedVaxError,
@@ -100,7 +100,7 @@ def get_immunization_by_id(self, aws_event: APIGatewayProxyEventV1) -> dict:
         imms_id = get_path_parameter(aws_event, "id")
 
         if not self._is_valid_immunization_id(imms_id):
-            raise InvalidImmunizationId()
+            raise InvalidImmunizationIdError()
 
         supplier_system = get_supplier_system_header(aws_event)
 
@@ -132,10 +132,10 @@ def update_immunization(self, aws_event: APIGatewayProxyEventV1) -> dict:
         resource_version = get_resource_version_header(aws_event)
 
         if not self._is_valid_immunization_id(imms_id):
-            raise InvalidImmunizationId()
+            raise InvalidImmunizationIdError()
 
         if not self._is_valid_resource_version(resource_version):
-            raise InvalidResourceVersion(resource_version=resource_version)
+            raise InvalidResourceVersionError(resource_version=resource_version)
 
         try:
             immunization = json.loads(aws_event["body"], parse_float=Decimal)
@@ -156,7 +156,7 @@ def delete_immunization(self, aws_event: APIGatewayProxyEventV1) -> dict:
         imms_id = get_path_parameter(aws_event, "id")
 
         if not self._is_valid_immunization_id(imms_id):
-            raise InvalidImmunizationId()
+            raise InvalidImmunizationIdError()
 
         supplier_system = get_supplier_system_header(aws_event)
 
@@ -167,10 +167,10 @@ def delete_immunization(self, aws_event: APIGatewayProxyEventV1) -> dict:
     def search_immunizations(self, aws_event: APIGatewayProxyEventV1) -> dict:
         try:
             search_params = process_search_params(process_params(aws_event))
-        except ParameterException as e:
+        except ParameterExceptionError as e:
             return self._create_bad_request(e.message)
         if search_params is None:
-            raise ParameterException("Failed to parse parameters.")
+            raise ParameterExceptionError("Failed to parse parameters.")
 
         # Check vaxx type permissions- start
         try:

lambdas/backend/src/models/errors.py

@@ -66,7 +66,7 @@ def to_operation_outcome() -> dict:
 
 
 @dataclass
-class ResourceVersionNotProvided(RuntimeError):
+class ResourceVersionNotProvidedError(RuntimeError):
     """Return this error when client has failed to provide the FHIR resource version where required"""
 
     resource_type: str
@@ -84,15 +84,15 @@ def to_operation_outcome(self) -> dict:
 
 
 @dataclass
-class ParameterException(RuntimeError):
+class ParameterExceptionError(RuntimeError):
     message: str
 
     def __str__(self):
         return self.message
 
 
 @dataclass
-class InvalidImmunizationId(ApiValidationError):
+class InvalidImmunizationIdError(ApiValidationError):
     """Use this when the unique Immunization ID is invalid"""
 
     def to_operation_outcome(self) -> dict:
@@ -105,7 +105,7 @@ def to_operation_outcome(self) -> dict:
 
 
 @dataclass
-class InvalidResourceVersion(ApiValidationError):
+class InvalidResourceVersionError(ApiValidationError):
     """Use this when the resource version is invalid"""
 
     resource_version: Any
@@ -155,3 +155,21 @@ def to_operation_outcome(self) -> dict:
             code=Code.invalid,
             diagnostics=self.message,
         )
+
+
+@dataclass
+class InvalidStoredDataError(RuntimeError):
+    """Use this when a piece of stored data is invalid and the operation cannot be completed"""
+
+    data_type: str
+
+    def __str__(self):
+        return f"Invalid data stored for immunization record: {self.data_type}"
+
+    def to_operation_outcome(self) -> dict:
+        return create_operation_outcome(
+            resource_id=str(uuid.uuid4()),
+            severity=Severity.error,
+            code=Code.server_error,
+            diagnostics=self.__str__(),
+        )

lambdas/backend/src/parameter_parser.py

@@ -9,7 +9,7 @@
 from common.models.constants import Constants
 from common.models.utils.generic_utils import nhs_number_mod11_check
 from common.redis_client import get_redis_client
-from models.errors import ParameterException
+from models.errors import ParameterExceptionError
 
 ERROR_MESSAGE_DUPLICATED_PARAMETERS = 'Parameters may not be duplicated. Use commas for "or".'
 
@@ -48,20 +48,20 @@ def process_patient_identifier(identifier_params: ParamContainer) -> str:
     patient_identifier = patient_identifiers[0] if len(patient_identifiers) == 1 else None
 
     if patient_identifier is None:
-        raise ParameterException(f"Search parameter {patient_identifier_key} must have one value.")
+        raise ParameterExceptionError(f"Search parameter {patient_identifier_key} must have one value.")
 
     patient_identifier_parts = patient_identifier.split("|")
     identifier_system = patient_identifier_parts[0]
     if len(patient_identifier_parts) != 2 or identifier_system != patient_identifier_system:
-        raise ParameterException(
+        raise ParameterExceptionError(
             "patient.identifier must be in the format of "
             f'"{patient_identifier_system}|{{NHS number}}" '
             f'e.g. "{patient_identifier_system}|9000000009"'
         )
 
     nhs_number = patient_identifier_parts[1]
     if not nhs_number_mod11_check(nhs_number):
-        raise ParameterException("Search parameter patient.identifier must be a valid NHS number.")
+        raise ParameterExceptionError("Search parameter patient.identifier must be a valid NHS number.")
 
     return nhs_number
 
@@ -75,11 +75,11 @@ def process_immunization_target(imms_params: ParamContainer) -> list[str]:
         vaccine_type for vaccine_type in set(imms_params.get(immunization_target_key, [])) if vaccine_type is not None
     ]
     if len(vaccine_types) < 1:
-        raise ParameterException(f"Search parameter {immunization_target_key} must have one or more values.")
+        raise ParameterExceptionError(f"Search parameter {immunization_target_key} must have one or more values.")
 
     valid_vaccine_types = get_redis_client().hkeys(Constants.VACCINE_TYPE_TO_DISEASES_HASH_KEY)
     if any(x not in valid_vaccine_types for x in vaccine_types):
-        raise ParameterException(
+        raise ParameterExceptionError(
             f"immunization-target must be one or more of the following: {', '.join(valid_vaccine_types)}"
         )
 
@@ -148,7 +148,7 @@ def process_search_params(params: ParamContainer) -> SearchParams:
         errors.append(f"Search parameter {date_from_key} must be before {date_to_key}")
 
     if errors:
-        raise ParameterException("; ".join(errors))
+        raise ParameterExceptionError("; ".join(errors))
 
     return SearchParams(patient_identifier, vaccine_types, date_from, date_to, include)
 
@@ -163,7 +163,7 @@ def parse_multi_value_query_parameters(
         multi_value_query_params: dict[str, list[str]],
     ) -> ParamContainer:
         if any(len(v) > 1 for k, v in multi_value_query_params.items()):
-            raise ParameterException(ERROR_MESSAGE_DUPLICATED_PARAMETERS)
+            raise ParameterExceptionError(ERROR_MESSAGE_DUPLICATED_PARAMETERS)
         params = [(k, split_and_flatten(v)) for k, v in multi_value_query_params.items()]
 
         return dict(params)
@@ -177,7 +177,7 @@ def parse_body_params(aws_event: APIGatewayProxyEventV1) -> ParamContainer:
             parsed_body = parse_qs(decoded_body)
 
             if any(len(v) > 1 for k, v in parsed_body.items()):
-                raise ParameterException(ERROR_MESSAGE_DUPLICATED_PARAMETERS)
+                raise ParameterExceptionError(ERROR_MESSAGE_DUPLICATED_PARAMETERS)
             items = {k: split_and_flatten(v) for k, v in parsed_body.items()}
             return items
         return {}
@@ -186,7 +186,7 @@ def parse_body_params(aws_event: APIGatewayProxyEventV1) -> ParamContainer:
     body_params = parse_body_params(aws_event)
 
     if len(set(query_params.keys()) & set(body_params.keys())) > 0:
-        raise ParameterException(ERROR_MESSAGE_DUPLICATED_PARAMETERS)
+        raise ParameterExceptionError(ERROR_MESSAGE_DUPLICATED_PARAMETERS)
 
     parsed_params = {
         key: sorted(query_params.get(key, []) + body_params.get(key, []))

lambdas/backend/src/repository/fhir_repository.py

@@ -15,7 +15,7 @@
 from responses import logger
 
 from common.models.constants import Constants
-from common.models.errors import InvalidStoredData, ResourceNotFoundError
+from common.models.errors import ResourceNotFoundError
 from common.models.immunization_record_metadata import ImmunizationRecordMetadata
 from common.models.utils.generic_utils import (
     get_contained_patient,
@@ -24,7 +24,7 @@
 from common.models.utils.validation_utils import (
     get_vaccine_type,
 )
-from models.errors import UnhandledResponseError
+from models.errors import InvalidStoredDataError, UnhandledResponseError
 
 
 def create_table(table_name=None, endpoint_url=None, region_name="eu-west-2"):
@@ -55,7 +55,7 @@ def get_fhir_identifier_from_identifier_pk(identifier_pk: str) -> Identifier:
     split_identifier = identifier_pk.split("#", 1)
 
     if len(split_identifier) != 2:
-        raise InvalidStoredData(data_type="identifier")
+        raise InvalidStoredDataError(data_type="identifier")
 
     supplier_code = split_identifier[0]
     supplier_unique_id = split_identifier[1]
@@ -133,13 +133,13 @@ def get_immunization_resource_and_metadata_by_id(
             return None, None
 
         # The FHIR Identifier which is returned in the metadata is based on the IdentifierPK from the database because
-        # it is valid for the IdentifierPK and Resource system and value to mismatch due to the V2 to V5 data uplift.
-        # Please see VED-893 for more details.
+        # we keep this attribute up to date in case of any changes rather than modifying the JSON resource. For example,
+        # when we performed the V2 to V5 data migration as part of issue VED-893.
 
         identifier_pk = item.get("IdentifierPK")
 
         if identifier_pk is None:
-            raise InvalidStoredData(data_type="identifier")
+            raise InvalidStoredDataError(data_type="identifier")
 
         identifier = get_fhir_identifier_from_identifier_pk(identifier_pk)
 

lambdas/backend/src/service/fhir_service.py

@@ -164,8 +164,10 @@ def update_immunization(self, imms_id: str, immunization: dict, supplier_system:
         ):
             raise UnauthorizedVaxError()
 
-        immunization_fhir_entity = Immunization.parse_obj(immunization)
-        identifier = cast(Identifier, immunization_fhir_entity.identifier[0])
+        identifier = Identifier.construct(
+            system=immunization["identifier"][0]["system"],
+            value=immunization["identifier"][0]["value"],
+        )
 
         validate_identifiers_match(identifier, existing_immunization_meta.identifier)
 

lambdas/backend/tests/controller/test_fhir_api_exception_handler.py

@@ -13,8 +13,9 @@
 from models.errors import (
     InconsistentIdError,
     InvalidJsonError,
-    InvalidResourceVersion,
-    ResourceVersionNotProvided,
+    InvalidResourceVersionError,
+    InvalidStoredDataError,
+    ResourceVersionNotProvidedError,
     UnauthorizedError,
     UnauthorizedVaxError,
     UnhandledResponseError,
@@ -58,13 +59,13 @@ def test_exception_handler_handles_custom_exception_and_returns_fhir_response(se
             (InvalidJsonError("Invalid JSON provided"), 400, "invalid", "Invalid JSON provided"),
             (CustomValidationError("This field was invalid"), 400, "invariant", "This field was invalid"),
             (
-                ResourceVersionNotProvided(resource_type="Immunization"),
+                ResourceVersionNotProvidedError(resource_type="Immunization"),
                 400,
                 "invariant",
                 "Validation errors: Immunization resource version not specified in the request headers",
             ),
             (
-                InvalidResourceVersion(resource_version="badVersion"),
+                InvalidResourceVersionError(resource_version="badVersion"),
                 400,
                 "invariant",
                 "Validation errors: Immunization resource version:badVersion in the request headers is invalid.",
@@ -94,6 +95,12 @@ def test_exception_handler_handles_custom_exception_and_returns_fhir_response(se
                 "exception",
                 "Critical error\n{'outcome': 'critical error'}",
             ),
+            (
+                InvalidStoredDataError("data_type"),
+                500,
+                "exception",
+                "Invalid data stored for immunization record: data_type",
+            ),
         ]
 
         for error, expected_status, expected_code, expected_message in test_cases:

lambdas/backend/tests/controller/test_fhir_controller.py

@@ -17,7 +17,7 @@
 from controller.aws_apig_response_utils import create_response
 from controller.fhir_controller import FhirController
 from models.errors import (
-    ParameterException,
+    ParameterExceptionError,
     UnauthorizedVaxError,
     UnhandledResponseError,
 )
@@ -1570,7 +1570,7 @@ def test_search_immunizations_returns_400_on_ParameterException_from_parameter_p
             }
         }
 
-        process_search_params.side_effect = ParameterException("Test")
+        process_search_params.side_effect = ParameterExceptionError("Test")
         response = self.controller.search_immunizations(lambda_event)
 
         # Then