VED-903: Update identifier validation for update immunisation path (#986)

* Update identifier validation logic

* Add custom error for invalid stored data

* Add error handling if IdentifierPK is None

* Update unit tests

* Tidy up after rebase

* Tidy up after rebase

* Fix linting error

* Address review comments

* Address review round 2 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

@@ -9,18 +9,19 @@
     CustomValidationError,
     IdentifierDuplicationError,
     InconsistentIdentifierError,
-    InconsistentResourceVersion,
+    InconsistentResourceVersionError,
     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,
@@ -29,19 +30,20 @@
 )
 
 _CUSTOM_EXCEPTION_TO_STATUS_MAP: dict[Type[Exception], int] = {
-    InconsistentResourceVersion: 400,
+    InconsistentResourceVersionError: 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,
+    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

@@ -9,6 +9,7 @@
 from boto3.dynamodb.conditions import Attr, Key
 from botocore.config import Config
 from fhir.resources.R4B.fhirtypes import Id
+from fhir.resources.R4B.identifier import Identifier
 from fhir.resources.R4B.immunization import Immunization
 from mypy_boto3_dynamodb.service_resource import DynamoDBServiceResource, Table
 from responses import logger
@@ -23,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"):
@@ -50,6 +51,18 @@ def _query_identifier(table, index, pk, identifier):
         return queryresponse
 
 
+def get_fhir_identifier_from_identifier_pk(identifier_pk: str) -> Identifier:
+    split_identifier = identifier_pk.split("#", 1)
+
+    if len(split_identifier) != 2:
+        raise InvalidStoredDataError(data_type="identifier")
+
+    supplier_code = split_identifier[0]
+    supplier_unique_id = split_identifier[1]
+
+    return Identifier(system=supplier_code, value=supplier_unique_id)
+
+
 @dataclass
 class RecordAttributes:
     pk: str
@@ -101,10 +114,10 @@ def get_immunization_by_identifier(self, identifier_pk: str) -> tuple[Optional[d
         else:
             return None, None
 
-    def get_immunization_and_resource_meta_by_id(
+    def get_immunization_resource_and_metadata_by_id(
         self, imms_id: str, include_deleted: bool = False
     ) -> tuple[Optional[dict], Optional[ImmunizationRecordMetadata]]:
-        """Retrieves the immunization and resource metadata from the VEDS table"""
+        """Retrieves the immunization resource and metadata from the VEDS table"""
         response = self.table.get_item(Key={"PK": _make_immunization_pk(imms_id)})
         item = response.get("Item")
 
@@ -119,7 +132,18 @@ def get_immunization_and_resource_meta_by_id(
         if is_deleted and not include_deleted:
             return None, None
 
-        imms_record_meta = ImmunizationRecordMetadata(int(item.get("Version")), is_deleted, is_reinstated)
+        # The FHIR Identifier which is returned in the metadata is based on the IdentifierPK from the database because
+        # 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 InvalidStoredDataError(data_type="identifier")
+
+        identifier = get_fhir_identifier_from_identifier_pk(identifier_pk)
+
+        imms_record_meta = ImmunizationRecordMetadata(identifier, int(item.get("Version")), is_deleted, is_reinstated)
 
         return json.loads(item.get("Resource", {})), imms_record_meta