Use consistent error format in delta. (#580)

* Use consistent error format in delta. Wait until service is ready to run e2e tests.

* Increase number of attempts. Run check for sandbox too.

* Remove unused switch. Tidy up error handling in Converter init.

* Refactor error handling and improve test coverage for Converter and Extractor classes

* Enhance error reporting in DeltaHandlerTestCase to include detailed conversion error records

* Refactor Converter class to improve type annotations and enhance code clarity

---------

Co-authored-by: Thomas-Boyle <thomasboyle@kainos.com>
Co-authored-by: Thomas-Boyle <45789537+Thomas-Boyle@users.noreply.github.com>

Author: mfjarvis

lambdas/delta_backend/src/converter.py

@@ -1,32 +1,29 @@
 # Main validation engine
+from typing import Any
+
 import exception_messages
 from conversion_layout import ConversionField, ConversionLayout
 from extractor import Extractor
 from mappings import ActionFlag
 
+ConversionErrorRecord = dict[str, Any]
+ConvertedRecord = dict[str, Any]
+
 
 class Converter:
-    def __init__(self, fhir_data, action_flag=ActionFlag.UPDATE, report_unexpected_exception=True):
-        self.converted = {}
-        self.error_records = []
+    def __init__(self, fhir_data: str | dict[str, Any], action_flag: str = ActionFlag.UPDATE) -> None:
+        self.converted: ConvertedRecord = {}
+        self.error_records: list[ConversionErrorRecord] = []
         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)
-        except Exception as e:
-            if report_unexpected_exception:
-                self._log_error(f"Initialization failed: [{e.__class__.__name__}] {e}")
-            raise
+        if not fhir_data:
+            raise ValueError("FHIR data is required for initialization.")
 
-    def run_conversion(self):
-        conversions = self.conversion_layout.get_conversion_layout()
+        self.extractor = Extractor(fhir_data)
+        self.conversion_layout = ConversionLayout(self.extractor)
 
-        for conversion in conversions:
+    def run_conversion(self) -> ConvertedRecord:
+        for conversion in self.conversion_layout.get_conversion_layout():
             self._convert_data(conversion)
 
         self.error_records.extend(self.extractor.get_error_records())
@@ -35,29 +32,33 @@ def run_conversion(self):
         self.converted["CONVERSION_ERRORS"] = self.error_records
         return self.converted
 
-    def _convert_data(self, conversion: ConversionField):
-        try:
-            flat_field = conversion.field_name_flat
+    def _convert_data(self, conversion: ConversionField) -> None:
+        flat_field = conversion.field_name_flat
 
+        try:
             if flat_field == "ACTION_FLAG":
                 self.converted[flat_field] = self.action_flag
-            else:
-                converted = conversion.expression_rule()
-                if converted is not None:
-                    self.converted[flat_field] = converted
+                return
 
-        except Exception as e:
+            if (converted := conversion.expression_rule()) is not None:
+                self.converted[flat_field] = converted
+        except Exception as error:
             self._log_error(
-                f"Conversion error [{e.__class__.__name__}]: {e}",
+                flat_field,
+                f"Conversion error [{error.__class__.__name__}]: {error}",
                 code=exception_messages.PARSING_ERROR,
             )
             self.converted[flat_field] = ""
 
-    def _log_error(self, e, code=exception_messages.UNEXPECTED_EXCEPTION):
-        error_obj = {"code": code, "message": str(e)}
-
-        if self.report_unexpected_exception:
-            self.error_records.append(error_obj)
+    def _log_error(self, field_name: str, e: Exception | str, code: str) -> None:
+        self.error_records.append(
+            {
+                "code": code,
+                "field": field_name,
+                "value": None,
+                "message": str(e),
+            }
+        )
 
-    def get_error_records(self):
+    def get_error_records(self) -> list[ConversionErrorRecord]:
         return self.error_records

lambdas/delta_backend/src/extractor.py

@@ -22,13 +22,12 @@ 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):
@@ -174,23 +173,20 @@ 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,
-                }
-            )
+        message = (
+            exception_messages.MESSAGES[exception_messages.UNEXPECTED_EXCEPTION] % (e.__class__.__name__, str(e))
+            if isinstance(e, Exception)
+            else 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:
         """

lambdas/delta_backend/tests/test_convert.py

@@ -89,6 +89,13 @@ def get_event(event_name=EventName.CREATE, operation="operation", supplier="EMIS
         """Returns test event data."""
         return ValuesForTests.get_event(event_name, operation, supplier)
 
+    def assert_structured_error_records(self, error_records):
+        self.assertTrue(error_records)
+
+        for error_record in error_records:
+            self.assertEqual(set(error_record), {"code", "field", "value", "message"})
+            self.assertTrue(error_record["message"])
+
     def assert_dynamodb_record(
         self,
         operation_flag,
@@ -139,68 +146,47 @@ 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)
         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(result, expected_imms_value)
+        self.assertFalse(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,
-        ]
+        error_test_cases = {
+            "missing_json": ErrorValuesForTests.missing_json,
+            "json_dob_error": ErrorValuesForTests.json_dob_error,
+        }
 
-        for test_case in error_test_cases:
-            json_data = json.dumps(test_case)
+        for test_name, test_case in error_test_cases.items():
+            with self.subTest(test_name=test_name):
+                fhir_converter = Converter(json.dumps(test_case))
+                fhir_converter.run_conversion()
 
-            fhir_converter = Converter(json_data)
-            fhir_converter.run_conversion()
+                self.assert_structured_error_records(fhir_converter.get_error_records())
 
-            errorRecords = 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):
+    def test_fhir_converter_json_incorrect_data_scenario(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()
+        with self.assertRaisesRegex(ValueError, "FHIR data is required for initialization."):
+            Converter(None)
 
-            # Check if bad data creates error records
-            self.assertTrue(len(errorRecords) == 0)
+    def test_handler_persists_structured_conversion_errors(self):
+        event = self.get_event(operation=Operation.UPDATE)
+        event["Records"][0]["dynamodb"]["NewImage"]["Resource"]["S"] = json.dumps(ErrorValuesForTests.json_dob_error)
 
-    def test_fhir_converter_json_incorrect_data_scenario_reporting_on(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)
+        response = handler(event, None)
 
-    def test_fhir_converter_json_incorrect_data_scenario_reporting_off(self):
-        """it should convert fhir json data to flat json - error scenarios"""
+        result = self.table.scan()
+        items = result.get("Items", [])
+        self.assertTrue(response)
+        self.assertEqual(len(items), 1)
 
-        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 = items[0]["Imms"]["CONVERSION_ERRORS"]
+        self.assert_structured_error_records(error_records)
+        self.assertEqual(error_records[0]["field"], "PERSON_DOB")
+        self.assertEqual(error_records[0]["value"], "196513-28")
 
     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."""
@@ -235,8 +221,6 @@ def test_handler_imms_convert_to_flat_json(self):
                     response,
                 )
 
-                result = self.table.scan()
-                items = result.get("Items", [])
                 self.clear_table()
 
     def test_handler_imms_convert_to_flat_json_legacy_patientsk_compatibility(self):

lambdas/delta_backend/tests/test_delta.py

@@ -511,9 +511,17 @@ def test_dps_record_skipped(self, mock_logger_info):
 
     @patch("delta.Converter")
     def test_partial_success_with_errors(self, mock_converter):
+        expected_error_records = [
+            {
+                "code": 10,
+                "field": "PERSON_DOB",
+                "value": "196513-28",
+                "message": "Unexpected exception [ValueError]: Invalid isoformat string: '196513-28'",
+            }
+        ]
         mock_converter_instance = MagicMock()
         mock_converter_instance.run_conversion.return_value = {"ABC": "DEF"}
-        mock_converter_instance.get_error_records.return_value = [{"error": "Invalid field"}]
+        mock_converter_instance.get_error_records.return_value = expected_error_records
         mock_converter.return_value = mock_converter_instance
 
         # Mock DynamoDB put_item success
@@ -533,13 +541,16 @@ def test_partial_success_with_errors(self, mock_converter):
         args, kwargs = self.mock_send_log_to_firehose.call_args
         sent_payload = args[1]  # Second positional arg
 
-        # Navigate to the specific message
-        status_desc = sent_payload["operation_outcome"]["statusDesc"]
+        operation_outcome = sent_payload["operation_outcome"]
 
-        # Assert the expected message is present
         self.assertIn(
             "Partial success: successfully synced into delta, but issues found within record",
-            status_desc,
+            operation_outcome["statusDesc"],
+        )
+        self.assertEqual(operation_outcome["diagnostics"], expected_error_records)
+        self.mock_logger.warning.assert_called_once_with(
+            "Partial success: record synced with conversion errors",
+            extra={"conversion_errors": expected_error_records},
         )
 
     def test_send_message_multi_records_diverse(self):