From 6b84dbf7cc2fc82a79f35ace7adcd5b144dcdaae Mon Sep 17 00:00:00 2001 From: Daniel Yip Date: Tue, 21 Oct 2025 16:55:24 +0100 Subject: [PATCH 1/7] Initial refactoring --- .../controller/fhir_api_exception_handler.py | 8 ++ backend/src/controller/fhir_controller.py | 61 +++------- backend/src/create_imms_handler.py | 16 +-- backend/src/models/errors.py | 15 +++ backend/src/repository/fhir_repository.py | 31 +++-- backend/src/service/fhir_service.py | 20 +-- .../test_fhir_api_exception_handler.py | 10 +- .../tests/controller/test_fhir_controller.py | 39 ++---- .../tests/repository/test_fhir_repository.py | 115 +++--------------- backend/tests/service/test_fhir_service.py | 70 +++++++---- backend/tests/test_create_imms.py | 26 ---- 11 files changed, 158 insertions(+), 253 deletions(-) diff --git a/backend/src/controller/fhir_api_exception_handler.py b/backend/src/controller/fhir_api_exception_handler.py index bb69150a8..db60f00f0 100644 --- a/backend/src/controller/fhir_api_exception_handler.py +++ b/backend/src/controller/fhir_api_exception_handler.py @@ -10,18 +10,26 @@ from models.errors import ( Code, InvalidImmunizationId, + CustomValidationError, + IdentifierDuplicationError, + InvalidJsonError, ResourceNotFoundError, Severity, UnauthorizedError, UnauthorizedVaxError, + UnhandledResponseError, create_operation_outcome, ) _CUSTOM_EXCEPTION_TO_STATUS_MAP: dict[Type[Exception], int] = { InvalidImmunizationId: 400, + InvalidJsonError: 400, + CustomValidationError: 400, UnauthorizedError: 403, UnauthorizedVaxError: 403, ResourceNotFoundError: 404, + IdentifierDuplicationError: 422, + UnhandledResponseError: 500, } diff --git a/backend/src/controller/fhir_controller.py b/backend/src/controller/fhir_controller.py index de1530d13..cf6a7f7cc 100644 --- a/backend/src/controller/fhir_controller.py +++ b/backend/src/controller/fhir_controller.py @@ -5,6 +5,7 @@ import urllib.parse import uuid from decimal import Decimal +from json import JSONDecodeError from typing import Optional from aws_lambda_typing.events import APIGatewayProxyEventV1 @@ -20,6 +21,7 @@ Code, IdentifierDuplicationError, InvalidImmunizationId, + InvalidJsonError, ParameterException, Severity, UnauthorizedError, @@ -48,7 +50,8 @@ def make_controller( class FhirController: - immunization_id_pattern = r"^[A-Za-z0-9\-.]{1,64}$" + _IMMUNIZATION_ID_PATTERN = r"^[A-Za-z0-9\-.]{1,64}$" + _API_SERVICE_URL = get_service_url() def __init__( self, @@ -105,46 +108,22 @@ def get_immunization_by_id(self, aws_event: APIGatewayProxyEventV1) -> dict: return create_response(200, resource.json(), {E_TAG_HEADER_NAME: version}) - def create_immunization(self, aws_event): - if not aws_event.get("headers"): - return create_response( - 403, - create_operation_outcome( - resource_id=str(uuid.uuid4()), - severity=Severity.error, - code=Code.forbidden, - diagnostics="Unauthorized request", - ), - ) - - supplier_system = self._identify_supplier_system(aws_event) + @fhir_api_exception_handler + def create_immunization(self, aws_event: APIGatewayProxyEventV1) -> dict: + supplier_system = get_supplier_system_header(aws_event) try: - immunisation = json.loads(aws_event["body"], parse_float=Decimal) - except json.decoder.JSONDecodeError as e: - return self._create_bad_request(f"Request's body contains malformed JSON: {e}") - try: - resource = self.fhir_service.create_immunization(immunisation, supplier_system) - if "diagnostics" in resource: - exp_error = create_operation_outcome( - resource_id=str(uuid.uuid4()), - severity=Severity.error, - code=Code.invariant, - diagnostics=resource["diagnostics"], - ) - return create_response(400, json.dumps(exp_error)) - else: - location = f"{get_service_url()}/Immunization/{resource.id}" - version = "1" - return create_response(201, None, {"Location": location, "E-Tag": version}) - except ValidationError as error: - return create_response(400, error.to_operation_outcome()) - except IdentifierDuplicationError as duplicate: - return create_response(422, duplicate.to_operation_outcome()) - except UnhandledResponseError as unhandled_error: - return create_response(500, unhandled_error.to_operation_outcome()) - except UnauthorizedVaxError as unauthorized: - return create_response(403, unauthorized.to_operation_outcome()) + immunisation: dict = json.loads(aws_event["body"], parse_float=Decimal) + except JSONDecodeError as e: + raise InvalidJsonError(message=str(f"Request's body contains malformed JSON: {e}")) + + created_resource_id = self.fhir_service.create_immunization(immunisation, supplier_system) + + return create_response( + status_code=201, + body=None, + headers={"Location": f"{self._API_SERVICE_URL}/Immunization/{created_resource_id}", "E-Tag": "1"}, + ) def update_immunization(self, aws_event): try: @@ -188,7 +167,7 @@ def update_immunization(self, aws_event): ) return create_response(400, json.dumps(exp_error)) # Validate the imms id in the path params and body of request - end - except json.decoder.JSONDecodeError as e: + except JSONDecodeError as e: return self._create_bad_request(f"Request's body contains malformed JSON: {e}") except Exception as e: return self._create_bad_request(f"Request's body contains string: {e}") @@ -405,7 +384,7 @@ def search_immunizations(self, aws_event: APIGatewayProxyEventV1) -> dict: def _is_valid_immunization_id(self, immunization_id: str) -> bool: """Validates if the given unique Immunization ID is valid.""" - return False if not re.match(self.immunization_id_pattern, immunization_id) else True + return False if not re.match(self._IMMUNIZATION_ID_PATTERN, immunization_id) else True def _validate_identifier_system(self, _id: str, _elements: str) -> Optional[dict]: if not _id: diff --git a/backend/src/create_imms_handler.py b/backend/src/create_imms_handler.py index 5eee06358..10e48daf1 100644 --- a/backend/src/create_imms_handler.py +++ b/backend/src/create_imms_handler.py @@ -1,14 +1,10 @@ import argparse import logging import pprint -import uuid -from constants import GENERIC_SERVER_ERROR_DIAGNOSTICS_MESSAGE -from controller.aws_apig_response_utils import create_response from controller.fhir_controller import FhirController, make_controller from local_lambda import load_string from log_structure import function_info -from models.errors import Code, Severity, create_operation_outcome logging.basicConfig(level="INFO") logger = logging.getLogger() @@ -20,17 +16,7 @@ def create_imms_handler(event, _context): def create_immunization(event, controller: FhirController): - try: - return controller.create_immunization(event) - except Exception: # pylint: disable = broad-exception-caught - logger.exception("Unhandled exception") - exp_error = create_operation_outcome( - resource_id=str(uuid.uuid4()), - severity=Severity.error, - code=Code.server_error, - diagnostics=GENERIC_SERVER_ERROR_DIAGNOSTICS_MESSAGE, - ) - return create_response(500, exp_error) + return controller.create_immunization(event) if __name__ == "__main__": diff --git a/backend/src/models/errors.py b/backend/src/models/errors.py index 25d7e990e..392ebe341 100644 --- a/backend/src/models/errors.py +++ b/backend/src/models/errors.py @@ -214,6 +214,21 @@ def to_operation_outcome(self) -> dict: ) +@dataclass +class InvalidJsonError(RuntimeError): + """Raised when client provides an invalid JSON payload""" + + message: str + + def to_operation_outcome(self) -> dict: + return create_operation_outcome( + resource_id=str(uuid.uuid4()), + severity=Severity.error, + code=Code.invalid, + diagnostics=self.message, + ) + + def create_operation_outcome(resource_id: str, severity: Severity, code: Code, diagnostics: str) -> dict: """Create an OperationOutcome object. Do not use `fhir.resource` library since it adds unnecessary validations""" return { diff --git a/backend/src/repository/fhir_repository.py b/backend/src/repository/fhir_repository.py index 799b47d74..b866797b9 100644 --- a/backend/src/repository/fhir_repository.py +++ b/backend/src/repository/fhir_repository.py @@ -1,6 +1,5 @@ import os import time -import uuid from dataclasses import dataclass from typing import Optional, Tuple @@ -17,6 +16,7 @@ ResourceNotFoundError, UnhandledResponseError, ) +from models.utils.generic_utils import get_contained_patient from models.utils.validation_utils import ( check_identifier_system_value, get_vaccine_type, @@ -151,15 +151,22 @@ def get_immunization_by_id_all(self, imms_id: str, imms: dict) -> Optional[dict] else: return None - def create_immunization(self, immunization: dict, patient: any, supplier_system: str) -> dict: - new_id = str(uuid.uuid4()) - immunization["id"] = new_id - attr = RecordAttributes(immunization, patient) + def check_immunization_identifier_exists(self, system: str, unique_id: str) -> bool: + """Checks whether an immunization with the given immunization identifier (system + local ID) exists.""" + response = self.table.query( + IndexName="IdentifierGSI", + KeyConditionExpression=Key("IdentifierPK").eq(f"{system}#{unique_id}"), + ) - query_response = _query_identifier(self.table, "IdentifierGSI", "IdentifierPK", attr.identifier) + if "Items" in response and len(response["Items"]) > 0: + return True - if query_response is not None: - raise IdentifierDuplicationError(identifier=attr.identifier) + return False + + def create_immunization(self, immunization: dict, supplier_system: str) -> str: + """Creates a new immunization record returning the unique id if successful.""" + patient = get_contained_patient(immunization) + attr = RecordAttributes(immunization, patient) response = self.table.put_item( Item={ @@ -174,10 +181,10 @@ def create_immunization(self, immunization: dict, patient: any, supplier_system: } ) - if response["ResponseMetadata"]["HTTPStatusCode"] == 200: - return immunization - else: - raise UnhandledResponseError(message="Non-200 response from dynamodb", response=response) + if response["ResponseMetadata"]["HTTPStatusCode"] != 200: + raise UnhandledResponseError(message="Non-200 response from dynamodb", response=dict(response)) + + return immunization.get("id") def update_immunization( self, diff --git a/backend/src/service/fhir_service.py b/backend/src/service/fhir_service.py index 47056d571..8f2b657f5 100644 --- a/backend/src/service/fhir_service.py +++ b/backend/src/service/fhir_service.py @@ -1,6 +1,7 @@ import datetime import logging import os +import uuid from enum import Enum from typing import Optional, Union from uuid import uuid4 @@ -22,6 +23,7 @@ from filter import Filter from models.errors import ( CustomValidationError, + IdentifierDuplicationError, InvalidPatientId, MandatoryError, ResourceNotFoundError, @@ -132,7 +134,7 @@ def get_immunization_by_id_all(self, imms_id: str, imms: dict) -> Optional[dict] imms_resp = self.immunization_repo.get_immunization_by_id_all(imms_id, imms) return imms_resp - def create_immunization(self, immunization: dict, supplier_system: str) -> dict | Immunization: + def create_immunization(self, immunization: dict, supplier_system: str) -> str: if immunization.get("id") is not None: raise CustomValidationError("id field must not be present for CREATE operation") @@ -140,18 +142,22 @@ def create_immunization(self, immunization: dict, supplier_system: str) -> dict self.validator.validate(immunization) except (ValidationError, ValueError, MandatoryError) as error: raise CustomValidationError(message=str(error)) from error - patient = self._validate_patient(immunization) - - if "diagnostics" in patient: - return patient vaccination_type = get_vaccine_type(immunization) if not self.authoriser.authorise(supplier_system, ApiOperationCode.CREATE, {vaccination_type}): raise UnauthorizedVaxError() - immunisation = self.immunization_repo.create_immunization(immunization, patient, supplier_system) - return Immunization.parse_obj(immunisation) + # TODO - consider only using FHIR entities in service layer + identifier_system = immunization["identifier"][0]["system"] + identifier_value = immunization["identifier"][0]["value"] + + if self.immunization_repo.check_immunization_identifier_exists(identifier_system, identifier_value): + raise IdentifierDuplicationError(identifier=f"{identifier_system}#{identifier_value}") + + # Set ID for the requested new record + immunization["id"] = str(uuid.uuid4()) + return self.immunization_repo.create_immunization(immunization, supplier_system) def update_immunization( self, diff --git a/backend/tests/controller/test_fhir_api_exception_handler.py b/backend/tests/controller/test_fhir_api_exception_handler.py index 668bec628..368c39f77 100644 --- a/backend/tests/controller/test_fhir_api_exception_handler.py +++ b/backend/tests/controller/test_fhir_api_exception_handler.py @@ -3,7 +3,13 @@ from unittest.mock import patch from controller.fhir_api_exception_handler import fhir_api_exception_handler -from models.errors import ResourceNotFoundError, UnauthorizedError, UnauthorizedVaxError +from models.errors import ( + CustomValidationError, + InvalidJsonError, + ResourceNotFoundError, + UnauthorizedError, + UnauthorizedVaxError, +) class TestFhirApiExceptionHandler(unittest.TestCase): @@ -27,6 +33,8 @@ def dummy_func(): def test_exception_handler_handles_custom_exception_and_returns_fhir_response(self): """Test that custom exceptions are handled by the wrapper and a valid response is returned to the client""" test_cases = [ + (InvalidJsonError("Invalid JSON provided"), 400, "invalid", "Invalid JSON provided"), + (CustomValidationError("This field was invalid"), 400, "invariant", "This field was invalid"), (UnauthorizedError(), 403, "forbidden", "Unauthorized request"), ( UnauthorizedVaxError(), diff --git a/backend/tests/controller/test_fhir_controller.py b/backend/tests/controller/test_fhir_controller.py index 9fa372f17..7c622da63 100644 --- a/backend/tests/controller/test_fhir_controller.py +++ b/backend/tests/controller/test_fhir_controller.py @@ -15,7 +15,6 @@ from models.errors import ( CustomValidationError, IdentifierDuplicationError, - InvalidPatientId, ParameterException, ResourceNotFoundError, UnauthorizedVaxError, @@ -841,7 +840,7 @@ def test_create_immunization(self): "headers": {"SupplierSystem": "Test"}, "body": imms.json(), } - self.service.create_immunization.return_value = imms + self.service.create_immunization.return_value = imms_id response = self.controller.create_immunization(aws_event) @@ -851,16 +850,18 @@ def test_create_immunization(self): self.assertTrue("body" not in response) self.assertTrue(response["headers"]["Location"].endswith(f"Immunization/{imms_id}")) - def test_unauthorised_create_immunization(self): - """it should return authorization error""" + def test_create_immunization_returns_unauthorised_error_when_supplier_system_header_missing(self): + """it should return unauthorized error""" imms_id = str(uuid.uuid4()) imms = create_covid_19_immunization(imms_id) aws_event = {"body": imms.json()} + response = self.controller.create_immunization(aws_event) + self.assertEqual(response["statusCode"], 403) def test_create_immunization_for_unauthorized(self): - """It should create Immunization and return resource's location""" + """it should return an unauthorized error when the service finds that use lacks permissions""" # Given imms_id = str(uuid.uuid4()) imms = create_covid_19_immunization(imms_id) @@ -893,28 +894,8 @@ def test_malformed_resource(self): outcome = json.loads(response["body"]) self.assertEqual(outcome["resourceType"], "OperationOutcome") - def test_create_bad_request_for_superseded_number_for_create_immunization(self): - """it should return 400 if json has superseded nhs number.""" - # Given - create_result = { - "diagnostics": "Validation errors: contained[?(@.resourceType=='Patient')].identifier[0].value does not exists" - } - self.service.create_immunization.return_value = create_result - imms_id = str(uuid.uuid4()) - imms = create_covid_19_immunization(imms_id) - aws_event = { - "headers": {"SupplierSystem": "Test"}, - "body": imms.json(), - } - # When - response = self.controller.create_immunization(aws_event) - - self.assertEqual(response["statusCode"], 400) - body = json.loads(response["body"]) - self.assertEqual(body["resourceType"], "OperationOutcome") - - def test_invalid_nhs_number(self): - """it should handle ValidationError when patient doesn't exist""" + def test_custom_validation_error(self): + """it should handle ValidationError when patient NHS Number is invalid""" # Given imms = Immunization.construct() aws_event = { @@ -922,7 +903,9 @@ def test_invalid_nhs_number(self): "body": imms.json(), } invalid_nhs_num = "a-bad-id" - self.service.create_immunization.side_effect = InvalidPatientId(patient_identifier=invalid_nhs_num) + self.service.create_immunization.side_effect = CustomValidationError( + f"{invalid_nhs_num} is not a valid NHS number" + ) response = self.controller.create_immunization(aws_event) diff --git a/backend/tests/repository/test_fhir_repository.py b/backend/tests/repository/test_fhir_repository.py index efa484008..eed32b8fe 100644 --- a/backend/tests/repository/test_fhir_repository.py +++ b/backend/tests/repository/test_fhir_repository.py @@ -148,6 +148,8 @@ def _make_a_patient(nhs_number="1234567890") -> dict: class TestCreateImmunizationMainIndex(TestFhirRepositoryBase): + _MOCK_CREATED_UUID = "88ca94d9-4618-4dc1-9e94-e701d3b2dd06" + def setUp(self): super().setUp() self.table = MagicMock() @@ -155,98 +157,28 @@ def setUp(self): self.patient = {"id": "a-patient-id", "identifier": {"value": "an-identifier"}} def test_create_immunization(self): - """it should create Immunization, and return created object""" - imms = create_covid_19_immunization_dict(imms_id="an-id") - - self.table.put_item = MagicMock(return_value={"ResponseMetadata": {"HTTPStatusCode": 200}}) - self.table.query = MagicMock(return_value={}) - - res_imms = self.repository.create_immunization(imms, self.patient, "Test") - - self.assertDictEqual(res_imms, imms) - self.table.put_item.assert_called_once_with( - Item={ - "PK": ANY, - "PatientPK": ANY, - "PatientSK": ANY, - "Resource": json.dumps(imms), - "IdentifierPK": ANY, - "Operation": "CREATE", - "Version": 1, - "SupplierSystem": "Test", - } - ) - - def test_create_immunization_batch(self): - """it should create Immunization, and return created object""" - imms = create_covid_19_immunization_dict(imms_id="an-id") + """it should create Immunization, and return created object unique ID""" + imms = create_covid_19_immunization_dict(imms_id=self._MOCK_CREATED_UUID) self.table.put_item = MagicMock(return_value={"ResponseMetadata": {"HTTPStatusCode": 200}}) - self.table.query = MagicMock(return_value={}) + self.mock_redis_client.hget.return_value = "COVID19" - res_imms = self.repository.create_immunization(imms, None, "Test") + created_id = self.repository.create_immunization(imms, "Test") - self.assertDictEqual(res_imms, imms) + self.assertEqual(created_id, self._MOCK_CREATED_UUID) self.table.put_item.assert_called_once_with( Item={ - "PK": ANY, - "PatientPK": ANY, - "PatientSK": ANY, + "PK": f"Immunization#{self._MOCK_CREATED_UUID}", + "PatientPK": "Patient#9990548609", + "PatientSK": f"COVID19#{self._MOCK_CREATED_UUID}", "Resource": json.dumps(imms), - "IdentifierPK": ANY, - "Operation": "CREATE", - "Version": 1, - "SupplierSystem": "Test", - } - ) - - def test_add_patient(self): - """it should store patient along the Immunization resource""" - imms = create_covid_19_immunization_dict("an-id") - self.table.put_item = MagicMock(return_value={"ResponseMetadata": {"HTTPStatusCode": 200}}) - self.table.query = MagicMock(return_value={}) - - res_imms = self.repository.create_immunization(imms, self.patient, "Test") - - self.assertDictEqual(res_imms, imms) - self.table.put_item.assert_called_once_with( - Item={ - "PK": ANY, - "PatientPK": ANY, - "PatientSK": ANY, - "Resource": ANY, - "IdentifierPK": ANY, + "IdentifierPK": "https://supplierABC/identifiers/vacc#ACME-vacc123456", "Operation": "CREATE", "Version": 1, "SupplierSystem": "Test", } ) - def test_create_immunization_makes_new_id(self): - """create should create new Logical ID even if one is already provided""" - imms_id = "original-id-from-request" - - imms = create_covid_19_immunization_dict(imms_id) - self.table.put_item = MagicMock(return_value={"ResponseMetadata": {"HTTPStatusCode": 200}}) - self.table.query = MagicMock(return_value={}) - - _ = self.repository.create_immunization(imms, self.patient, "Test") - - item = self.table.put_item.call_args.kwargs["Item"] - self.assertTrue(item["PK"].startswith("Immunization#")) - self.assertNotEqual(item["PK"], "Immunization#original-id-from-request") - - def test_create_immunization_returns_new_id(self): - """create should return the persisted object i.e. with new id""" - imms_id = "original-id-from-request" - imms = create_covid_19_immunization_dict(imms_id) - self.table.put_item = MagicMock(return_value={"ResponseMetadata": {"HTTPStatusCode": 200}}) - self.table.query = MagicMock(return_value={}) - - response = self.repository.create_immunization(imms, self.patient, "Test") - - self.assertNotEqual(response["id"], imms_id) - def test_create_should_catch_dynamo_error(self): """it should throw UnhandledResponse when the response from dynamodb can't be handled""" bad_request = 400 @@ -256,25 +188,11 @@ def test_create_should_catch_dynamo_error(self): with self.assertRaises(UnhandledResponseError) as e: # When - self.repository.create_immunization(create_covid_19_immunization_dict("an-id"), self.patient, "Test") + self.repository.create_immunization(create_covid_19_immunization_dict("an-id"), "Test") # Then self.assertDictEqual(e.exception.response, response) - def test_create_throws_error_when_identifier_already_in_dynamodb(self): - """it should throw UnhandledResponse when trying to update an immunization with an identfier that is already stored""" - imms_id = "an-id" - imms = create_covid_19_immunization_dict(imms_id) - imms["patient"] = self.patient - - self.table.query = MagicMock(return_value={"Items": [{"Resource": '{"id": "different-id"}'}], "Count": 1}) - identifier = f"{imms['identifier'][0]['system']}#{imms['identifier'][0]['value']}" - with self.assertRaises(IdentifierDuplicationError) as e: - # When - self.repository.create_immunization(imms, self.patient, "Test") - - self.assertEqual(str(e.exception), f"The provided identifier: {identifier} is duplicated") - class TestCreateImmunizationPatientIndex(TestFhirRepositoryBase): """create_immunization should create a patient record with vaccine type""" @@ -296,7 +214,7 @@ def test_create_patient_gsi(self): self.table.query = MagicMock(return_value={}) # When - _ = self.repository.create_immunization(imms, self.patient, "Test") + _ = self.repository.create_immunization(imms, "Test") # Then item = self.table.put_item.call_args.kwargs["Item"] @@ -313,7 +231,7 @@ def test_create_patient_with_vaccine_type(self): self.table.put_item = MagicMock(return_value={"ResponseMetadata": {"HTTPStatusCode": 200}}) # When - _ = self.repository.create_immunization(imms, self.patient, "Test") + _ = self.repository.create_immunization(imms, "Test") # Then item = self.table.put_item.call_args.kwargs["Item"] @@ -634,10 +552,9 @@ def test_decimal_on_create(self): self.table.put_item = MagicMock(return_value={"ResponseMetadata": {"HTTPStatusCode": 200}}) self.table.query = MagicMock(return_value={}) - res_imms = self.repository.create_immunization(imms, self.patient, "Test") + res_imms = self.repository.create_immunization(imms, "Test") - self.assertEqual(res_imms["doseQuantity"], imms["doseQuantity"]) - self.assertDictEqual(res_imms, imms) + self.assertEqual(res_imms, "an-id") self.table.put_item.assert_called_once() diff --git a/backend/tests/service/test_fhir_service.py b/backend/tests/service/test_fhir_service.py index 30a66141e..e1731046d 100644 --- a/backend/tests/service/test_fhir_service.py +++ b/backend/tests/service/test_fhir_service.py @@ -18,6 +18,7 @@ from constants import NHS_NUMBER_USED_IN_SAMPLE_DATA from models.errors import ( CustomValidationError, + IdentifierDuplicationError, InvalidPatientId, ResourceNotFoundError, UnauthorizedVaxError, @@ -448,6 +449,8 @@ def test_immunization_not_found(self): class TestCreateImmunization(TestFhirServiceBase): """Tests for FhirService.create_immunization""" + _MOCK_NEW_UUID = "88ca94d9-4618-4dc1-9e94-e701d3b2dd06" + def setUp(self): super().setUp() self.authoriser = create_autospec(Authoriser) @@ -464,20 +467,24 @@ def test_create_immunization(self): """it should create Immunization and validate it""" self.mock_redis_client.hget.return_value = "COVID19" self.authoriser.authorise.return_value = True - self.imms_repo.create_immunization.return_value = create_covid_19_immunization_dict_no_id() + self.imms_repo.check_immunization_identifier_exists.return_value = False + self.imms_repo.create_immunization.return_value = self._MOCK_NEW_UUID nhs_number = VALID_NHS_NUMBER req_imms = create_covid_19_immunization_dict_no_id(nhs_number) - req_patient = get_contained_patient(req_imms) + # When - stored_imms = self.fhir_service.create_immunization(req_imms, "Test") + created_id = self.fhir_service.create_immunization(req_imms, "Test") # Then self.authoriser.authorise.assert_called_once_with("Test", ApiOperationCode.CREATE, {"COVID19"}) - self.imms_repo.create_immunization.assert_called_once_with(req_imms, req_patient, "Test") + self.imms_repo.check_immunization_identifier_exists.assert_called_once_with( + "https://supplierABC/identifiers/vacc", "ACME-vacc123456" + ) + self.imms_repo.create_immunization.assert_called_once_with(req_imms, "Test") self.validator.validate.assert_called_once_with(req_imms) - self.assertIsInstance(stored_imms, Immunization) + self.assertEqual(self._MOCK_NEW_UUID, created_id) def test_create_immunization_with_id_throws_error(self): """it should throw exception if id present in create Immunization""" @@ -545,33 +552,23 @@ def test_post_validation_failed_create_missing_patient_name(self): def test_patient_error(self): """it should throw error when patient ID is invalid""" - invalid_nhs_number = "a-bad-patient-id" - bad_patient_imms = create_covid_19_immunization_dict_no_id(invalid_nhs_number) - - with self.assertRaises(InvalidPatientId) as e: - # When - self.fhir_service.create_immunization(bad_patient_imms, "Test") - - # Then - self.assertEqual(e.exception.patient_identifier, invalid_nhs_number) - self.imms_repo.create_immunization.assert_not_called() - - def test_patient_error_invalid_nhs_number(self): - """it should throw error when NHS number checksum is incorrect""" invalid_nhs_number = "9434765911" # check digit 1 doesn't match result (9) - bad_patient_imms = create_covid_19_immunization_dict_no_id(invalid_nhs_number) + imms = create_covid_19_immunization_dict_no_id(invalid_nhs_number) + expected_msg = ( + "Validation errors: contained[?(@.resourceType=='Patient')].identifier[0].value is not a valid NHS number" + ) - with self.assertRaises(InvalidPatientId) as e: + with self.assertRaises(CustomValidationError) as error: # When - self.fhir_service.create_immunization(bad_patient_imms, "Test") + self.pre_validate_fhir_service.create_immunization(imms, "Test") # Then - self.assertEqual(e.exception.patient_identifier, invalid_nhs_number) + self.assertEqual(expected_msg, error.exception.message) self.imms_repo.create_immunization.assert_not_called() def test_unauthorised_error_raised_when_user_lacks_permissions(self): """it should raise error when user lacks permissions""" - self.mock_redis_client.hget.return_value = "FLU" + self.mock_redis_client.hget.return_value = "COVID19" self.authoriser.authorise.return_value = False self.imms_repo.create_immunization.return_value = create_covid_19_immunization_dict_no_id() @@ -583,10 +580,35 @@ def test_unauthorised_error_raised_when_user_lacks_permissions(self): self.fhir_service.create_immunization(req_imms, "Test") # Then - self.authoriser.authorise.assert_called_once_with("Test", ApiOperationCode.CREATE, {"FLU"}) + self.authoriser.authorise.assert_called_once_with("Test", ApiOperationCode.CREATE, {"COVID19"}) self.validator.validate.assert_called_once_with(req_imms) self.imms_repo.create_immunization.assert_not_called() + def test_raises_duplicate_error_if_identifier_already_exits(self): + """it should raise a duplicate error if the immunisation identifier (system + local ID) already exists""" + self.mock_redis_client.hget.return_value = "COVID19" + self.authoriser.authorise.return_value = True + self.imms_repo.check_immunization_identifier_exists.return_value = True + + nhs_number = VALID_NHS_NUMBER + req_imms = create_covid_19_immunization_dict_no_id(nhs_number) + + # When + with self.assertRaises(IdentifierDuplicationError) as error: + self.fhir_service.create_immunization(req_imms, "Test") + + # Then + self.authoriser.authorise.assert_called_once_with("Test", ApiOperationCode.CREATE, {"COVID19"}) + self.imms_repo.check_immunization_identifier_exists.assert_called_once_with( + "https://supplierABC/identifiers/vacc", "ACME-vacc123456" + ) + self.imms_repo.create_immunization.assert_not_called() + self.validator.validate.assert_called_once_with(req_imms) + self.assertEqual( + "The provided identifier: https://supplierABC/identifiers/vacc#ACME-vacc123456 is duplicated", + str(error.exception), + ) + class TestUpdateImmunization(TestFhirServiceBase): """Tests for FhirService.update_immunization""" diff --git a/backend/tests/test_create_imms.py b/backend/tests/test_create_imms.py index 6f1abc0a9..ed145bfdc 100644 --- a/backend/tests/test_create_imms.py +++ b/backend/tests/test_create_imms.py @@ -1,11 +1,8 @@ -import json import unittest from unittest.mock import create_autospec, patch -from constants import GENERIC_SERVER_ERROR_DIAGNOSTICS_MESSAGE from controller.fhir_controller import FhirController from create_imms_handler import create_immunization -from models.errors import Code, Severity, create_operation_outcome class TestCreateImmunizationById(unittest.TestCase): @@ -32,26 +29,3 @@ def test_create_immunization(self): # Then self.controller.create_immunization.assert_called_once_with(lambda_event) self.assertDictEqual(exp_res, act_res) - - def test_create_handle_exception(self): - """unhandled exceptions should result in 500""" - lambda_event = {"aws": "event"} - error_msg = "an unhandled error" - self.controller.create_immunization.side_effect = Exception(error_msg) - - exp_error = create_operation_outcome( - resource_id=None, - severity=Severity.error, - code=Code.server_error, - diagnostics=GENERIC_SERVER_ERROR_DIAGNOSTICS_MESSAGE, - ) - - # When - act_res = create_immunization(lambda_event, self.controller) - - # Then - act_body = json.loads(act_res["body"]) - act_body["id"] = None - - self.assertDictEqual(act_body, exp_error) - self.assertEqual(act_res["statusCode"], 500) From 5969a9ef64f80fd8b02d44716ec24cfd55c6468d Mon Sep 17 00:00:00 2001 From: Daniel Yip Date: Tue, 21 Oct 2025 17:13:48 +0100 Subject: [PATCH 2/7] Sonar and attempt to fix tf apply in pipeline --- backend/src/service/fhir_service.py | 1 - infrastructure/instance/file_name_processor.tf | 4 ++++ infrastructure/instance/mesh_processor.tf | 4 ++++ infrastructure/instance/redis_sync_lambda.tf | 2 +- 4 files changed, 9 insertions(+), 2 deletions(-) diff --git a/backend/src/service/fhir_service.py b/backend/src/service/fhir_service.py index 8f2b657f5..feb3b8c90 100644 --- a/backend/src/service/fhir_service.py +++ b/backend/src/service/fhir_service.py @@ -148,7 +148,6 @@ def create_immunization(self, immunization: dict, supplier_system: str) -> str: if not self.authoriser.authorise(supplier_system, ApiOperationCode.CREATE, {vaccination_type}): raise UnauthorizedVaxError() - # TODO - consider only using FHIR entities in service layer identifier_system = immunization["identifier"][0]["system"] identifier_value = immunization["identifier"][0]["value"] diff --git a/infrastructure/instance/file_name_processor.tf b/infrastructure/instance/file_name_processor.tf index 65defe1f8..a7855deaf 100644 --- a/infrastructure/instance/file_name_processor.tf +++ b/infrastructure/instance/file_name_processor.tf @@ -312,6 +312,10 @@ resource "aws_s3_bucket_notification" "datasources_lambda_notification" { lambda_function_arn = aws_lambda_function.file_processor_lambda.arn events = ["s3:ObjectCreated:*"] } + + depends_on = [ + aws_lambda_permission.s3_invoke_permission + ] } resource "aws_cloudwatch_log_group" "file_name_processor_log_group" { diff --git a/infrastructure/instance/mesh_processor.tf b/infrastructure/instance/mesh_processor.tf index 2dabc1336..f0c8fe9d0 100644 --- a/infrastructure/instance/mesh_processor.tf +++ b/infrastructure/instance/mesh_processor.tf @@ -244,6 +244,10 @@ resource "aws_s3_bucket_notification" "mesh_datasources_lambda_notification" { events = ["s3:ObjectCreated:*"] filter_prefix = "inbound/" } + + depends_on = [ + aws_lambda_permission.mesh_s3_invoke_permission + ] } resource "aws_cloudwatch_log_group" "mesh_file_converter_log_group" { diff --git a/infrastructure/instance/redis_sync_lambda.tf b/infrastructure/instance/redis_sync_lambda.tf index 18cfa4efc..38f1e4dd3 100644 --- a/infrastructure/instance/redis_sync_lambda.tf +++ b/infrastructure/instance/redis_sync_lambda.tf @@ -264,7 +264,7 @@ resource "aws_s3_bucket_notification" "config_lambda_notification" { } depends_on = [ - aws_lambda_function.redis_sync_lambda + aws_lambda_permission.new_s3_invoke_permission ] } From 19dff69f5b400ad1f1ca26346e7d884ed339a9b2 Mon Sep 17 00:00:00 2001 From: Daniel Yip Date: Wed, 22 Oct 2025 09:35:22 +0100 Subject: [PATCH 3/7] Added tests for new repository method --- .../tests/repository/test_fhir_repository.py | 37 +++++++++++++++++++ 1 file changed, 37 insertions(+) diff --git a/backend/tests/repository/test_fhir_repository.py b/backend/tests/repository/test_fhir_repository.py index eed32b8fe..4c30a2c00 100644 --- a/backend/tests/repository/test_fhir_repository.py +++ b/backend/tests/repository/test_fhir_repository.py @@ -89,6 +89,43 @@ def test_immunization_not_found(self): self.assertIsNone(immunisation) self.assertIsNone(immunisation_type) + def test_check_immunization_identifier_exists_returns_true(self): + """it should return true when a record does exist with the given identifier""" + imms_id = "https://system.com#id-123" + self.table.query = MagicMock( + return_value={ + "Items": [ + { + "Resource": json.dumps({"item": "exists"}), + "Version": 1, + "PatientSK": "COVID19#2516525251", + "IdentifierPK": "https://system.com#id-123", + } + ] + } + ) + + result = self.repository.check_immunization_identifier_exists("https://system.com", "id-123") + + self.table.query.assert_called_once_with( + IndexName="IdentifierGSI", + KeyConditionExpression=Key("IdentifierPK").eq(imms_id), + ) + self.assertTrue(result) + + def test_check_immunization_identifier_exists_returns_false_when_no_record_exists(self): + """it should return false when a record does not exist with the given identifier""" + imms_id = "https://system.com#id-123" + self.table.query = MagicMock(return_value={}) + + result = self.repository.check_immunization_identifier_exists("https://system.com", "id-123") + + self.table.query.assert_called_once_with( + IndexName="IdentifierGSI", + KeyConditionExpression=Key("IdentifierPK").eq(imms_id), + ) + self.assertFalse(result) + class TestGetImmunization(unittest.TestCase): def setUp(self): From ea3909144d8eeb1d06e86dfdf41752185a5a1218 Mon Sep 17 00:00:00 2001 From: Daniel Yip Date: Wed, 22 Oct 2025 10:45:05 +0100 Subject: [PATCH 4/7] Use entities instead of passing dicts around --- backend/src/repository/fhir_repository.py | 14 ++++++++----- backend/src/service/fhir_service.py | 21 +++++++++++-------- .../tests/repository/test_fhir_repository.py | 21 +++++++++++-------- backend/tests/service/test_fhir_service.py | 2 +- 4 files changed, 34 insertions(+), 24 deletions(-) diff --git a/backend/src/repository/fhir_repository.py b/backend/src/repository/fhir_repository.py index b866797b9..8500892e8 100644 --- a/backend/src/repository/fhir_repository.py +++ b/backend/src/repository/fhir_repository.py @@ -8,6 +8,8 @@ import simplejson as json from boto3.dynamodb.conditions import Attr, Key from botocore.config import Config +from fhir.resources.R4B.fhirtypes import Id +from fhir.resources.R4B.immunization import Immunization from mypy_boto3_dynamodb.service_resource import DynamoDBServiceResource, Table from responses import logger @@ -163,17 +165,19 @@ def check_immunization_identifier_exists(self, system: str, unique_id: str) -> b return False - def create_immunization(self, immunization: dict, supplier_system: str) -> str: + def create_immunization(self, immunization: Immunization, supplier_system: str) -> Id: """Creates a new immunization record returning the unique id if successful.""" - patient = get_contained_patient(immunization) - attr = RecordAttributes(immunization, patient) + immunization_as_dict = immunization.dict() + + patient = get_contained_patient(immunization_as_dict) + attr = RecordAttributes(immunization_as_dict, patient) response = self.table.put_item( Item={ "PK": attr.pk, "PatientPK": attr.patient_pk, "PatientSK": attr.patient_sk, - "Resource": json.dumps(attr.resource, use_decimal=True), + "Resource": immunization.json(use_decimal=True), "IdentifierPK": attr.identifier, "Operation": "CREATE", "Version": 1, @@ -184,7 +188,7 @@ def create_immunization(self, immunization: dict, supplier_system: str) -> str: if response["ResponseMetadata"]["HTTPStatusCode"] != 200: raise UnhandledResponseError(message="Non-200 response from dynamodb", response=dict(response)) - return immunization.get("id") + return immunization.id def update_immunization( self, diff --git a/backend/src/service/fhir_service.py b/backend/src/service/fhir_service.py index feb3b8c90..ad5303c4c 100644 --- a/backend/src/service/fhir_service.py +++ b/backend/src/service/fhir_service.py @@ -3,7 +3,7 @@ import os import uuid from enum import Enum -from typing import Optional, Union +from typing import Optional, Union, cast from uuid import uuid4 from fhir.resources.R4B.bundle import ( @@ -14,6 +14,8 @@ BundleEntrySearch, BundleLink, ) +from fhir.resources.R4B.fhirtypes import Id +from fhir.resources.R4B.identifier import Identifier from fhir.resources.R4B.immunization import Immunization from pydantic import ValidationError @@ -134,7 +136,7 @@ def get_immunization_by_id_all(self, imms_id: str, imms: dict) -> Optional[dict] imms_resp = self.immunization_repo.get_immunization_by_id_all(imms_id, imms) return imms_resp - def create_immunization(self, immunization: dict, supplier_system: str) -> str: + def create_immunization(self, immunization: dict, supplier_system: str) -> Id: if immunization.get("id") is not None: raise CustomValidationError("id field must not be present for CREATE operation") @@ -148,15 +150,16 @@ def create_immunization(self, immunization: dict, supplier_system: str) -> str: if not self.authoriser.authorise(supplier_system, ApiOperationCode.CREATE, {vaccination_type}): raise UnauthorizedVaxError() - identifier_system = immunization["identifier"][0]["system"] - identifier_value = immunization["identifier"][0]["value"] - - if self.immunization_repo.check_immunization_identifier_exists(identifier_system, identifier_value): - raise IdentifierDuplicationError(identifier=f"{identifier_system}#{identifier_value}") - # Set ID for the requested new record immunization["id"] = str(uuid.uuid4()) - return self.immunization_repo.create_immunization(immunization, supplier_system) + + immunization_fhir_entity = Immunization.parse_obj(immunization) + identifier = cast(Identifier, immunization_fhir_entity.identifier[0]) + + if self.immunization_repo.check_immunization_identifier_exists(identifier.system, identifier.value): + raise IdentifierDuplicationError(identifier=f"{identifier.system}#{identifier.value}") + + return self.immunization_repo.create_immunization(immunization_fhir_entity, supplier_system) def update_immunization( self, diff --git a/backend/tests/repository/test_fhir_repository.py b/backend/tests/repository/test_fhir_repository.py index 4c30a2c00..210edc498 100644 --- a/backend/tests/repository/test_fhir_repository.py +++ b/backend/tests/repository/test_fhir_repository.py @@ -6,6 +6,7 @@ import botocore.exceptions import simplejson as json from boto3.dynamodb.conditions import Attr, Key +from fhir.resources.R4B.immunization import Immunization from models.errors import ( IdentifierDuplicationError, @@ -195,7 +196,7 @@ def setUp(self): def test_create_immunization(self): """it should create Immunization, and return created object unique ID""" - imms = create_covid_19_immunization_dict(imms_id=self._MOCK_CREATED_UUID) + imms = Immunization.parse_obj(create_covid_19_immunization_dict(imms_id=self._MOCK_CREATED_UUID)) self.table.put_item = MagicMock(return_value={"ResponseMetadata": {"HTTPStatusCode": 200}}) self.mock_redis_client.hget.return_value = "COVID19" @@ -208,7 +209,7 @@ def test_create_immunization(self): "PK": f"Immunization#{self._MOCK_CREATED_UUID}", "PatientPK": "Patient#9990548609", "PatientSK": f"COVID19#{self._MOCK_CREATED_UUID}", - "Resource": json.dumps(imms), + "Resource": imms.json(), "IdentifierPK": "https://supplierABC/identifiers/vacc#ACME-vacc123456", "Operation": "CREATE", "Version": 1, @@ -225,7 +226,9 @@ def test_create_should_catch_dynamo_error(self): with self.assertRaises(UnhandledResponseError) as e: # When - self.repository.create_immunization(create_covid_19_immunization_dict("an-id"), "Test") + self.repository.create_immunization( + Immunization.parse_obj(create_covid_19_immunization_dict("an-id")), "Test" + ) # Then self.assertDictEqual(e.exception.response, response) @@ -251,7 +254,7 @@ def test_create_patient_gsi(self): self.table.query = MagicMock(return_value={}) # When - _ = self.repository.create_immunization(imms, "Test") + _ = self.repository.create_immunization(Immunization.parse_obj(imms), "Test") # Then item = self.table.put_item.call_args.kwargs["Item"] @@ -268,7 +271,7 @@ def test_create_patient_with_vaccine_type(self): self.table.put_item = MagicMock(return_value={"ResponseMetadata": {"HTTPStatusCode": 200}}) # When - _ = self.repository.create_immunization(imms, "Test") + _ = self.repository.create_immunization(Immunization.parse_obj(imms), "Test") # Then item = self.table.put_item.call_args.kwargs["Item"] @@ -584,12 +587,12 @@ def setUp(self): def test_decimal_on_create(self): """it should create Immunization, and preserve decimal value""" imms = create_covid_19_immunization_dict(imms_id="an-id") - imms["doseQuantity"] = 0.7477 + imms["doseQuantity"]["value"] = 0.7477 self.table.put_item = MagicMock(return_value={"ResponseMetadata": {"HTTPStatusCode": 200}}) self.table.query = MagicMock(return_value={}) - res_imms = self.repository.create_immunization(imms, "Test") + res_imms = self.repository.create_immunization(Immunization.parse_obj(imms), "Test") self.assertEqual(res_imms, "an-id") @@ -599,7 +602,7 @@ def test_decimal_on_create(self): "PK": ANY, "PatientPK": ANY, "PatientSK": ANY, - "Resource": json.dumps(imms, use_decimal=True), + "Resource": Immunization.parse_obj(imms).json(use_decimal=True), "IdentifierPK": ANY, "Operation": "CREATE", "Version": 1, @@ -612,7 +615,7 @@ def test_decimal_on_create(self): resource_from_item = json.loads(item_passed_to_put_item["Resource"]) self.assertEqual( - resource_from_item["doseQuantity"], + resource_from_item["doseQuantity"]["value"], 0.7477, ) diff --git a/backend/tests/service/test_fhir_service.py b/backend/tests/service/test_fhir_service.py index e1731046d..92e74dc8d 100644 --- a/backend/tests/service/test_fhir_service.py +++ b/backend/tests/service/test_fhir_service.py @@ -481,7 +481,7 @@ def test_create_immunization(self): self.imms_repo.check_immunization_identifier_exists.assert_called_once_with( "https://supplierABC/identifiers/vacc", "ACME-vacc123456" ) - self.imms_repo.create_immunization.assert_called_once_with(req_imms, "Test") + self.imms_repo.create_immunization.assert_called_once_with(Immunization.parse_obj(req_imms), "Test") self.validator.validate.assert_called_once_with(req_imms) self.assertEqual(self._MOCK_NEW_UUID, created_id) From 0d36200013e7e2fa212f7a71d238bb8144fca548 Mon Sep 17 00:00:00 2001 From: Daniel Yip Date: Wed, 22 Oct 2025 11:02:45 +0100 Subject: [PATCH 5/7] Added test cases for additional error codes --- .../controller/test_fhir_api_exception_handler.py | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/backend/tests/controller/test_fhir_api_exception_handler.py b/backend/tests/controller/test_fhir_api_exception_handler.py index 368c39f77..2deed6e2c 100644 --- a/backend/tests/controller/test_fhir_api_exception_handler.py +++ b/backend/tests/controller/test_fhir_api_exception_handler.py @@ -5,10 +5,12 @@ from controller.fhir_api_exception_handler import fhir_api_exception_handler from models.errors import ( CustomValidationError, + IdentifierDuplicationError, InvalidJsonError, ResourceNotFoundError, UnauthorizedError, UnauthorizedVaxError, + UnhandledResponseError, ) @@ -48,6 +50,18 @@ def test_exception_handler_handles_custom_exception_and_returns_fhir_response(se "not-found", "Immunization resource does not exist. ID: 123", ), + ( + IdentifierDuplicationError("system#id"), + 422, + "duplicate", + "The provided identifier: system#id is duplicated", + ), + ( + UnhandledResponseError(message="Critical error", response={"outcome": "critical error"}), + 500, + "exception", + "Critical error\n{'outcome': 'critical error'}", + ), ] for error, expected_status, expected_code, expected_message in test_cases: From 39b4b370db2365fbab1919952ae3b13fa1bd241c Mon Sep 17 00:00:00 2001 From: Daniel Yip Date: Wed, 22 Oct 2025 14:39:23 +0100 Subject: [PATCH 6/7] Minor spelling update --- backend/tests/controller/test_fhir_controller.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/backend/tests/controller/test_fhir_controller.py b/backend/tests/controller/test_fhir_controller.py index 7c622da63..d7a67be63 100644 --- a/backend/tests/controller/test_fhir_controller.py +++ b/backend/tests/controller/test_fhir_controller.py @@ -861,7 +861,7 @@ def test_create_immunization_returns_unauthorised_error_when_supplier_system_hea self.assertEqual(response["statusCode"], 403) def test_create_immunization_for_unauthorized(self): - """it should return an unauthorized error when the service finds that use lacks permissions""" + """it should return an unauthorized error when the service finds that user lacks permissions""" # Given imms_id = str(uuid.uuid4()) imms = create_covid_19_immunization(imms_id) From a3093f8edc5ab519a3f1038e9eb71849ce66b49f Mon Sep 17 00:00:00 2001 From: Daniel Yip Date: Thu, 23 Oct 2025 17:01:32 +0100 Subject: [PATCH 7/7] Linting post rebasing onto master --- backend/src/controller/fhir_api_exception_handler.py | 2 +- backend/src/controller/fhir_controller.py | 1 - 2 files changed, 1 insertion(+), 2 deletions(-) diff --git a/backend/src/controller/fhir_api_exception_handler.py b/backend/src/controller/fhir_api_exception_handler.py index db60f00f0..1135768e6 100644 --- a/backend/src/controller/fhir_api_exception_handler.py +++ b/backend/src/controller/fhir_api_exception_handler.py @@ -9,9 +9,9 @@ from controller.aws_apig_response_utils import create_response from models.errors import ( Code, - InvalidImmunizationId, CustomValidationError, IdentifierDuplicationError, + InvalidImmunizationId, InvalidJsonError, ResourceNotFoundError, Severity, diff --git a/backend/src/controller/fhir_controller.py b/backend/src/controller/fhir_controller.py index cf6a7f7cc..5dec7e54d 100644 --- a/backend/src/controller/fhir_controller.py +++ b/backend/src/controller/fhir_controller.py @@ -26,7 +26,6 @@ Severity, UnauthorizedError, UnauthorizedVaxError, - UnhandledResponseError, ValidationError, create_operation_outcome, )