From 1b705c79403f7226cf776e40cd16731d357784b6 Mon Sep 17 00:00:00 2001 From: Matt Jarvis Date: Thu, 12 Jun 2025 10:55:00 +0100 Subject: [PATCH 1/3] Use consistent error format in delta. Wait until service is ready to run e2e tests. --- azure/templates/post-deploy.yml | 27 +++++++++++--------- delta_backend/src/converter.py | 32 ++++++++++++++---------- delta_backend/src/extractor.py | 44 ++++++++++++++++++--------------- 3 files changed, 58 insertions(+), 45 deletions(-) diff --git a/azure/templates/post-deploy.yml b/azure/templates/post-deploy.yml index 83346a33c..a2fc8967d 100644 --- a/azure/templates/post-deploy.yml +++ b/azure/templates/post-deploy.yml @@ -96,7 +96,8 @@ steps: response=$(curl -H "apikey: $(status-endpoint-api-key)" -s "$endpoint") response_code=$(jq -r '.checks.healthcheck.responseCode' <<< "$response") response_body=$(jq -r '.checks.healthcheck.outcome' <<< "$response") - if [ "$response_code" -eq 200 ] && [ "$response_body" == "OK" ]; then + status=$(jq -r '.status' <<< "$response") + if [ "$response_code" -eq 200 ] && [ "$response_body" == "OK" ] && [ "$status" == "pass" ]; then echo "Status test successful" break else @@ -106,8 +107,10 @@ steps: sleep 30 fi done - if [ $counter -eq 11 ]; then + if [ $counter -eq 21 ]; then echo "Status test failed: Maximum number of attempts reached" + echo "Last response received:" + echo "$response" exit 1 fi fi @@ -164,7 +167,7 @@ steps: export DEFAULT_CLIENT_SECRET="$(INT_CLIENT_SECRET)" echo "running: $test_cmd -v -c test_deployment.py test_proxy.py" $test_cmd -v -c test_deployment.py test_proxy.py - + elif [[ $APIGEE_ENVIRONMENT == "prod" ]]; then echo "Proxy test completed successfully as part of terraform resource up status check" @@ -175,35 +178,35 @@ steps: workingDirectory: "$(Pipeline.Workspace)/s/$(SERVICE_NAME)/$(SERVICE_ARTIFACT_NAME)/e2e" displayName: Run Full Test Suite - + - bash: | set -e if ! [[ "$APIGEE_ENVIRONMENT" == "prod" || "$APIGEE_ENVIRONMENT" == "int" || "$APIGEE_ENVIRONMENT" == *"sandbox" ]]; then echo "Running E2E batch folder test cases" - + export AWS_PROFILE="apim-dev" aws_account_no="$(aws sts get-caller-identity --query Account --output text)" echo "Using AWS Account: $aws_account_no" - + service_name="${FULLY_QUALIFIED_SERVICE_NAME}" - + pr_no=$(echo "$service_name" | { grep -oE '[0-9]+$' || true; }) if [ -z "$pr_no" ]; then workspace="$APIGEE_ENVIRONMENT" else workspace="pr-$pr_no" fi - + poetry install --no-root # Install dependencies defined in pyproject.toml - + ENV="$workspace" poetry run python -m unittest -v -c - + echo "E2E batch folder test cases executed successfully" else echo "Skipping E2E batch folder test cases as the environment is prod-int-sandbox" fi - + displayName: Run full batch test suite workingDirectory: "$(Pipeline.Workspace)/s/$(SERVICE_NAME)/$(SERVICE_ARTIFACT_NAME)/e2e_batch" condition: eq(1, 2) # Disable task but make this step visible in the pipeline @@ -213,4 +216,4 @@ steps: condition: always() inputs: testResultsFiles: '$(Pipeline.Workspace)/s/$(SERVICE_NAME)/$(SERVICE_ARTIFACT_NAME)/tests/test-report.xml' - failTaskOnFailedTests: true \ No newline at end of file + failTaskOnFailedTests: true diff --git a/delta_backend/src/converter.py b/delta_backend/src/converter.py index 270104154..91f801990 100644 --- a/delta_backend/src/converter.py +++ b/delta_backend/src/converter.py @@ -11,24 +11,24 @@ def __init__(self, fhir_data, action_flag = ActionFlag.UPDATE, report_unexpected self.error_records = [] self.action_flag = action_flag self.report_unexpected_exception = report_unexpected_exception - + try: if not fhir_data: raise ValueError("FHIR data is required for initialization.") - + self.extractor = Extractor(fhir_data, self.report_unexpected_exception) - self.conversion_layout = ConversionLayout(self.extractor) + self.conversion_layout = ConversionLayout(self.extractor) except Exception as e: if report_unexpected_exception: - self._log_error(f"Initialization failed: [{e.__class__.__name__}] {e}") + self._log_error(None, None, f"Initialization failed: [{e.__class__.__name__}] {e}") raise def run_conversion(self): conversions = self.conversion_layout.get_conversion_layout() - + for conversion in conversions: self._convert_data(conversion) - + self.error_records.extend(self.extractor.get_error_records()) # Add CONVERSION_ERRORS as the 35th field @@ -36,9 +36,8 @@ def run_conversion(self): return self.converted def _convert_data(self, conversion: ConversionField): + flat_field = conversion.field_name_flat try: - flat_field = conversion.field_name_flat - if flat_field == "ACTION_FLAG": self.converted[flat_field] = self.action_flag else: @@ -47,17 +46,24 @@ def _convert_data(self, conversion: ConversionField): self.converted[flat_field] = converted except Exception as e: - self._log_error(f"Conversion error [{e.__class__.__name__}]: {e}", code=exception_messages.PARSING_ERROR) + self._log_error( + flat_field, + None, + f"Conversion error [{e.__class__.__name__}]: {e}", + code=exception_messages.PARSING_ERROR + ) self.converted[flat_field] = "" - def _log_error(self,e,code=exception_messages.UNEXPECTED_EXCEPTION): + def _log_error(self, field_name, field_value, e, code=exception_messages.UNEXPECTED_EXCEPTION): error_obj = { "code": code, + "field": field_name, + "value": field_value, "message": str(e) } - + if self.report_unexpected_exception: self.error_records.append(error_obj) - + def get_error_records(self): - return self.error_records \ No newline at end of file + return self.error_records diff --git a/delta_backend/src/extractor.py b/delta_backend/src/extractor.py index e93ebc6d6..44bfb008c 100644 --- a/delta_backend/src/extractor.py +++ b/delta_backend/src/extractor.py @@ -29,7 +29,6 @@ def _get_patient(self): return next((c for c in contained if isinstance(c, dict) and c.get("resourceType") == "Patient"), "") def _get_valid_names(self, names, occurrence_time): - official_names = [n for n in names if n.get("use") == "official" and self._is_current_period(n, occurrence_time)] if official_names: return official_names[0] @@ -37,13 +36,11 @@ def _get_valid_names(self, names, occurrence_time): valid_names = [n for n in names if self._is_current_period(n, occurrence_time) and n.get("use") != "old"] if valid_names: return valid_names[0] - - return names[0] - + return names[0] def _get_person_names(self): - occurrence_time = self._get_occurance_date_time() + occurrence_time = self._get_occurrence_date_time() patient = self._get_patient() names = patient.get("name", []) names = [n for n in names if "given" in n and "family" in n] @@ -56,12 +53,12 @@ def _get_person_names(self): if person_forename and person_surname: return person_forename, person_surname - + return "", "" - + def _get_practitioner_names(self): contained = self.fhir_json_data.get("contained", []) - occurrence_time = self._get_occurance_date_time() + occurrence_time = self._get_occurrence_date_time() practitioner = next((c for c in contained if isinstance(c, dict) and c.get("resourceType") == "Practitioner"), None) if not practitioner or "name" not in practitioner: return "", "" @@ -77,7 +74,6 @@ def _get_practitioner_names(self): return performing_professional_forename, performing_professional_surname - def _is_current_period(self, name, occurrence_time): period = name.get("period") if not isinstance(period, dict): @@ -94,18 +90,26 @@ def _is_current_period(self, name, occurrence_time): return (not start or start <= occurrence_time) and (not end or occurrence_time <= end) - def _get_occurance_date_time(self) -> str: + def _get_occurrence_date_time(self) -> datetime: + occurrence_datetime_str = self.fhir_json_data.get("occurrenceDateTime", "") + try: - occurrence_time = datetime.fromisoformat(self.fhir_json_data.get("occurrenceDateTime", "")) - if occurrence_time and occurrence_time.tzinfo is None: - occurrence_time = occurrence_time.replace(tzinfo=timezone.utc) - return occurrence_time - return occurrence_time + occurrence_datetime = datetime.fromisoformat(occurrence_datetime_str) + + if occurrence_datetime and occurrence_datetime.tzinfo is None: + occurrence_datetime = occurrence_datetime.replace(tzinfo=timezone.utc) + + return occurrence_datetime except Exception as e: message = "DateTime conversion error [%s]: %s" % (e.__class__.__name__, e) - error = self._log_error(ConversionFieldName.DATE_AND_TIME, message, e, code=exception_messages.UNEXPECTED_EXCEPTION) - return error + self._log_error( + ConversionFieldName.DATE_AND_TIME, + occurrence_datetime_str, + message, + code=exception_messages.UNEXPECTED_EXCEPTION + ) + raise def _get_first_snomed_code(self, coding_container: dict) -> str: codings = coding_container.get("coding", []) @@ -255,7 +259,7 @@ def normalize(self, value): return value.lower() if isinstance(value, str) else value def extract_valid_address(self): - occurrence_time = self._get_occurance_date_time() + occurrence_time = self._get_occurrence_date_time() patient = self._get_patient() addresses = patient.get("address", []) @@ -279,8 +283,8 @@ def extract_valid_address(self): ) return selected_address.get("postalCode") or self.DEFAULT_POSTCODE - - def extract_date_time(self) -> str: + + def extract_date_time(self) -> str: date = self.fhir_json_data.get("occurrenceDateTime","") if date: return self._convert_date_to_safe_format(ConversionFieldName.DATE_AND_TIME, date) From 47e1f405a8993362f526e0f08e7ad162e33e922c Mon Sep 17 00:00:00 2001 From: Matt Jarvis Date: Thu, 12 Jun 2025 11:29:23 +0100 Subject: [PATCH 2/3] Increase number of attempts. Run check for sandbox too. --- azure/templates/post-deploy.yml | 55 ++++++++++++++++----------------- 1 file changed, 26 insertions(+), 29 deletions(-) diff --git a/azure/templates/post-deploy.yml b/azure/templates/post-deploy.yml index a2fc8967d..6efca433e 100644 --- a/azure/templates/post-deploy.yml +++ b/azure/templates/post-deploy.yml @@ -80,41 +80,38 @@ steps: - bash: | set -ex - if ! [[ $APIGEE_ENVIRONMENT =~ .*-*sandbox ]]; then - counter=0 - base_path="$SERVICE_BASE_PATH" - endpoint="" + endpoint="" + if [[ $APIGEE_ENVIRONMENT =~ "prod" ]]; then + endpoint="https://api.service.nhs.uk/${SERVICE_BASE_PATH}/_status" + else + endpoint="https://${APIGEE_ENVIRONMENT}.api.service.nhs.uk/${SERVICE_BASE_PATH}/_status" + fi - if [[ $APIGEE_ENVIRONMENT =~ "prod" ]]; then - endpoint="https://api.service.nhs.uk/${base_path}/_status" + counter=0 + while [[ $counter -lt 21 ]]; do + response=$(curl -H "apikey: $(status-endpoint-api-key)" -s "$endpoint") + response_code=$(jq -r '.checks.healthcheck.responseCode' <<< "$response") + response_body=$(jq -r '.checks.healthcheck.outcome' <<< "$response") + status=$(jq -r '.status' <<< "$response") + if [ "$response_code" -eq 200 ] && [ "$response_body" == "OK" ] && [ "$status" == "pass" ]; then + echo "Status test successful" + break else - endpoint="https://${APIGEE_ENVIRONMENT}.api.service.nhs.uk/${base_path}/_status" + echo "Waiting for $endpoint to return a 200 response with 'OK' body..." + ((counter=counter+1)) # Increment counter by 1 + echo "Attempt $counter" + sleep 30 fi + done - while [[ $counter -lt 11 ]]; do - response=$(curl -H "apikey: $(status-endpoint-api-key)" -s "$endpoint") - response_code=$(jq -r '.checks.healthcheck.responseCode' <<< "$response") - response_body=$(jq -r '.checks.healthcheck.outcome' <<< "$response") - status=$(jq -r '.status' <<< "$response") - if [ "$response_code" -eq 200 ] && [ "$response_body" == "OK" ] && [ "$status" == "pass" ]; then - echo "Status test successful" - break - else - echo "Waiting for $endpoint to return a 200 response with 'OK' body..." - ((counter=counter+1)) # Increment counter by 1 - echo "Attempt $counter" - sleep 30 - fi - done - if [ $counter -eq 21 ]; then - echo "Status test failed: Maximum number of attempts reached" - echo "Last response received:" - echo "$response" - exit 1 - fi + if [ $counter -eq 21 ]; then + echo "Status test failed: Maximum number of attempts reached" + echo "Last response received:" + echo "$response" + exit 1 fi - displayName: Waiting for TF resources to be UP + displayName: Waiting for API to be available workingDirectory: "$(Pipeline.Workspace)/s/$(SERVICE_NAME)/$(SERVICE_ARTIFACT_NAME)" - bash: | From d441a4a296e6823ff7d0eb978afdc0cfe3e05f82 Mon Sep 17 00:00:00 2001 From: Matt Jarvis Date: Tue, 7 Oct 2025 15:38:48 +0100 Subject: [PATCH 3/3] Remove unused switch. Tidy up error handling in Converter init. --- delta_backend/src/converter.py | 29 ++++-------- delta_backend/src/extractor.py | 26 +++++------ delta_backend/tests/test_convert.py | 70 ++++++----------------------- 3 files changed, 34 insertions(+), 91 deletions(-) diff --git a/delta_backend/src/converter.py b/delta_backend/src/converter.py index 91f801990..f78da264c 100644 --- a/delta_backend/src/converter.py +++ b/delta_backend/src/converter.py @@ -6,22 +6,16 @@ class Converter: - def __init__(self, fhir_data, action_flag = ActionFlag.UPDATE, report_unexpected_exception=True): + def __init__(self, fhir_data, action_flag = ActionFlag.UPDATE): self.converted = {} self.error_records = [] self.action_flag = action_flag - self.report_unexpected_exception = report_unexpected_exception - try: - if not fhir_data: - raise ValueError("FHIR data is required for initialization.") + if not fhir_data: + raise ValueError("FHIR data is required for initialization.") - self.extractor = Extractor(fhir_data, self.report_unexpected_exception) - self.conversion_layout = ConversionLayout(self.extractor) - except Exception as e: - if report_unexpected_exception: - self._log_error(None, None, f"Initialization failed: [{e.__class__.__name__}] {e}") - raise + self.extractor = Extractor(fhir_data) + self.conversion_layout = ConversionLayout(self.extractor) def run_conversion(self): conversions = self.conversion_layout.get_conversion_layout() @@ -44,26 +38,21 @@ def _convert_data(self, conversion: ConversionField): converted = conversion.expression_rule() if converted is not None: self.converted[flat_field] = converted - except Exception as e: self._log_error( flat_field, - None, f"Conversion error [{e.__class__.__name__}]: {e}", code=exception_messages.PARSING_ERROR ) self.converted[flat_field] = "" - def _log_error(self, field_name, field_value, e, code=exception_messages.UNEXPECTED_EXCEPTION): - error_obj = { + def _log_error(self, field_name, e, code): + self.error_records.append({ "code": code, "field": field_name, - "value": field_value, + "value": None, "message": str(e) - } - - if self.report_unexpected_exception: - self.error_records.append(error_obj) + }) def get_error_records(self): return self.error_records diff --git a/delta_backend/src/extractor.py b/delta_backend/src/extractor.py index 2c4688120..b08cc3354 100644 --- a/delta_backend/src/extractor.py +++ b/delta_backend/src/extractor.py @@ -19,9 +19,8 @@ class Extractor: DATE_CONVERT_FORMAT = "%Y%m%d" DEFAULT_POSTCODE = "ZZ99 3CZ" - def __init__(self, fhir_json_data, report_unexpected_exception = True): + def __init__(self, fhir_json_data): self.fhir_json_data = json.loads(fhir_json_data, parse_float=decimal.Decimal) if isinstance(fhir_json_data, str) else fhir_json_data - self.report_unexpected_exception = report_unexpected_exception self.error_records = [] def _get_patient(self): @@ -163,18 +162,17 @@ def _get_site_information(self): return site_code, site_code_type_uri def _log_error(self, field_name, field_value, e, code=exception_messages.RECORD_CHECK_FAILED): - if self.report_unexpected_exception: - if isinstance(e, Exception): - message = exception_messages.MESSAGES[exception_messages.UNEXPECTED_EXCEPTION] % (e.__class__.__name__, str(e)) - else: - message = str(e) - - self.error_records.append({ - "code": code, - "field": field_name, - "value": field_value, - "message": message - }) + if isinstance(e, Exception): + message = exception_messages.MESSAGES[exception_messages.UNEXPECTED_EXCEPTION] % (e.__class__.__name__, str(e)) + else: + message = str(e) + + self.error_records.append({ + "code": code, + "field": field_name, + "value": field_value, + "message": message + }) def _convert_date(self, field_name, date, format) -> str: """ diff --git a/delta_backend/tests/test_convert.py b/delta_backend/tests/test_convert.py index a8e2d2025..e0d10a813 100644 --- a/delta_backend/tests/test_convert.py +++ b/delta_backend/tests/test_convert.py @@ -28,7 +28,7 @@ def setUp(self): # Start moto AWS mocks self.mock = mock_aws() self.mock.start() - + """Set up mock DynamoDB table.""" self.dynamodb_resource = boto3_resource("dynamodb", "eu-west-2") self.table = self.dynamodb_resource.create_table( @@ -74,7 +74,7 @@ def tearDown(self): self.logger_exception_patcher.stop() self.logger_info_patcher.stop() self.mock_firehose_logger.stop() - + self.mock.stop() @staticmethod @@ -126,34 +126,18 @@ def test_fhir_converter_json_direct_data(self): json_data = json.dumps(ValuesForTests.json_data) fhir_converter = Converter(json_data) - FlatFile = fhir_converter.run_conversion() + result = fhir_converter.run_conversion() - flatJSON = json.dumps(FlatFile) + result_str = json.dumps(result) expected_imms_value = deepcopy(ValuesForTests.expected_imms2) # UPDATE is currently the default action-flag expected_imms = json.dumps(expected_imms_value) - self.assertEqual(flatJSON, expected_imms) + self.assertEqual(result_str, expected_imms) - errorRecords = fhir_converter.get_error_records() + error_records = fhir_converter.get_error_records() - self.assertEqual(len(errorRecords), 0) - - def test_fhir_converter_json_direct_data(self): - """it should convert fhir json data to flat json""" - json_data = json.dumps(ValuesForTests.json_data) - - fhir_converter = Converter(json_data) - FlatFile = fhir_converter.run_conversion() + self.assertEqual(len(error_records), 0) - flatJSON = json.dumps(FlatFile) - expected_imms_value = deepcopy(ValuesForTests.expected_imms2) # UPDATE is currently the default action-flag - expected_imms = json.dumps(expected_imms_value) - self.assertEqual(flatJSON, expected_imms) - - errorRecords = fhir_converter.get_error_records() - - self.assertEqual(len(errorRecords), 0) - - def test_fhir_converter_json_error_scenario_reporting_on(self): + def test_fhir_converter_json_error_scenario(self): """it should convert fhir json data to flat json - error scenarios""" error_test_cases = [ErrorValuesForTests.missing_json, ErrorValuesForTests.json_dob_error] @@ -163,42 +147,18 @@ def test_fhir_converter_json_error_scenario_reporting_on(self): fhir_converter = Converter(json_data) fhir_converter.run_conversion() - errorRecords = fhir_converter.get_error_records() + error_records = fhir_converter.get_error_records() # Check if bad data creates error records - self.assertTrue(len(errorRecords) > 0) - - def test_fhir_converter_json_error_scenario_reporting_off(self): - """it should convert fhir json data to flat json - error scenarios""" - error_test_cases = [ErrorValuesForTests.missing_json, ErrorValuesForTests.json_dob_error] - - for test_case in error_test_cases: - json_data = json.dumps(test_case) - - fhir_converter = Converter(json_data, report_unexpected_exception=False) - fhir_converter.run_conversion() - - errorRecords = fhir_converter.get_error_records() + self.assertTrue(len(error_records) > 0) - # Check if bad data creates error records - self.assertTrue(len(errorRecords) == 0) - - def test_fhir_converter_json_incorrect_data_scenario_reporting_on(self): + def test_fhir_converter_json_incorrect_data_scenario(self): """it should convert fhir json data to flat json - error scenarios""" with self.assertRaises(ValueError): fhir_converter = Converter(None) - errorRecords = fhir_converter.get_error_records() - self.assertTrue(len(errorRecords) > 0) - - - def test_fhir_converter_json_incorrect_data_scenario_reporting_off(self): - """it should convert fhir json data to flat json - error scenarios""" - - with self.assertRaises(ValueError): - fhir_converter = Converter(None, report_unexpected_exception=False) - errorRecords = fhir_converter.get_error_records() - self.assertTrue(len(errorRecords) == 0) + error_records = fhir_converter.get_error_records() + self.assertTrue(len(error_records) > 0) def test_handler_imms_convert_to_flat_json(self): """Test that the Imms field contains the correct flat JSON data for CREATE, UPDATE, and DELETE operations.""" @@ -231,8 +191,6 @@ def test_handler_imms_convert_to_flat_json(self): response ) - result = self.table.scan() - items = result.get("Items", []) self.clear_table() def clear_table(self): @@ -240,8 +198,6 @@ def clear_table(self): with self.table.batch_writer() as batch: for item in scan.get("Items", []): batch.delete_item(Key={"PK": item["PK"]}) - result = self.table.scan() - items = result.get("Items", []) if __name__ == "__main__": unittest.main()