resolve type hinting and remove try catch

Author: motola

lambdas/mns_publisher/src/create_notification.py

@@ -63,36 +63,32 @@ def calculate_age_at_vaccination(birth_date: str, vaccination_date: str) -> int:
 
 
 def get_practitioner_details_from_pds(nhs_number: str) -> str | None:
-    try:
-        patient_details = pds_get_patient_details(nhs_number)
+    patient_details = pds_get_patient_details(nhs_number)
 
-        general_practitioners = patient_details.get("generalPractitioner", [])
-        if not general_practitioners or len(general_practitioners) == 0:
-            logger.warning("No GP details found for patient")
-            return None
+    general_practitioners = patient_details.get("generalPractitioner", [])
+    if not general_practitioners or len(general_practitioners) == 0:
+        logger.warning("No GP details found for patient")
+        return None
 
-        patient_gp = general_practitioners[0]
-        patient_gp_identifier = patient_gp.get("identifier", {})
+    patient_gp = general_practitioners[0]
+    patient_gp_identifier = patient_gp.get("identifier", {})
 
-        gp_ods_code = patient_gp_identifier.get("value")
-        if not gp_ods_code:
-            logger.warning("GP ODS code not found in practitioner details")
-            return None
+    gp_ods_code = patient_gp_identifier.get("value")
+    if not gp_ods_code:
+        logger.warning("GP ODS code not found in practitioner details")
+        return None
 
-        # Check if registration is current
-        period = patient_gp_identifier.get("period", {})
-        gp_period_end_date = period.get("end", None)
+    # Check if registration is current
+    period = patient_gp_identifier.get("period", {})
+    gp_period_end_date = period.get("end", None)
 
-        if gp_period_end_date:
-            # Parse end date (format: YYYY-MM-DD)
-            end_date = datetime.strptime(gp_period_end_date, "%Y-%m-%d").date()
-            today = datetime.now().date()
+    if gp_period_end_date:
+        # Parse end date (format: YYYY-MM-DD)
+        end_date = datetime.strptime(gp_period_end_date, "%Y-%m-%d").date()
+        today = datetime.now().date()
 
-            if end_date < today:
-                logger.warning("GP registration has ended")
-                return None
+        if end_date < today:
+            logger.warning("No current GP registration found for patient")
+            return None
 
-        return gp_ods_code
-    except Exception as error:
-        logger.exception("Failed to get practitioner details from pds", error)
-        raise
+    return gp_ods_code

lambdas/mns_publisher/src/lambda_handler.py

@@ -1,16 +1,9 @@
 from aws_lambda_typing import context, events
 
-from common.clients import logger
 from process_records import process_records
 
 
 def lambda_handler(event: events.SQSEvent, _: context.Context) -> dict[str, list]:
     event_records = event.get("Records", [])
-    batch_item_failures = process_records(event_records)
 
-    if batch_item_failures:
-        logger.warning(f"Batch completed with {len(batch_item_failures)} failures")
-    else:
-        logger.info(f"Successfully processed all {len(event_records)} messages")
-
-    return {"batchItemFailures": batch_item_failures}
+    return process_records(event_records)

lambdas/mns_publisher/src/process_records.py

@@ -2,17 +2,17 @@
 import os
 from typing import Tuple
 
-from aws_lambda_typing import events
 from aws_lambda_typing.events.sqs import SQSMessage
 
+from common.api_clients.mns_service import MnsService
 from common.api_clients.mns_setup import get_mns_service
 from common.clients import logger
 from create_notification import create_mns_notification
 
 mns_env = os.getenv("MNS_ENV", "int")
 
 
-def process_records(records: events.SQSEvent) -> list[dict]:
+def process_records(records: list[SQSMessage]) -> list[dict]:
     """
     Process multiple SQS records.
     Args: records: List of SQS records to process
@@ -22,14 +22,23 @@ def process_records(records: events.SQSEvent) -> list[dict]:
     mns_service = get_mns_service(mns_env=mns_env)
 
     for record in records:
-        failed_batch_item = process_record(record, mns_service)
-        if failed_batch_item:
-            batch_item_failures.append(failed_batch_item)
+        try:
+            failed_batch_item = process_record(record, mns_service)
+            if failed_batch_item:
+                batch_item_failures.append(failed_batch_item)
+        except Exception:
+            message_id = record.get("messageId", "unknown")
+            batch_item_failures.append({"itemIdentifier": message_id})
 
-    return batch_item_failures
+    if batch_item_failures:
+        logger.warning(f"Batch completed with {len(batch_item_failures)} failures")
+    else:
+        logger.info(f"Successfully processed all {len(records)} messages")
 
+    return {"batchItemFailures": batch_item_failures}
 
-def process_record(record: SQSMessage, mns_service) -> dict | None:
+
+def process_record(record: SQSMessage, mns_service: MnsService) -> dict | None:
     """
     Process a single SQS record.
     Args:
@@ -40,40 +49,25 @@ def process_record(record: SQSMessage, mns_service) -> dict | None:
     message_id, immunisation_id = extract_trace_ids(record)
     notification_id = None
 
-    try:
-        # Create notification payload
-        mns_notification_payload = create_mns_notification(record)
-        notification_id = mns_notification_payload.get("id")
-        action_flag = mns_notification_payload.get("filtering", {}).get("action")
-        logger.info(
-            "Processing message",
-            trace_ids={
-                "notification_id": notification_id,
-                "message_id": message_id,
-                "immunisation_id": immunisation_id,
-                "action_flag": action_flag,
-            },
-        )
-
-        # Publish to MNS
-        mns_pub_response = mns_service.publish_notification(mns_notification_payload)
-        if mns_pub_response["status_code"] != 200:
-            raise RuntimeError("MNS publish failed")
-        logger.info("Successfully created MNS notification", trace_ids={"mns_notification_id": notification_id})
-
-        return None
-
-    except Exception as e:
-        logger.exception(
-            "Failed to process message",
-            trace_ids={
-                "message_id": message_id,
-                "immunisation_id": immunisation_id,
-                "mns_notification_id": notification_id,
-                "error": str(e),
-            },
-        )
-        return {"itemIdentifier": message_id}
+    # Create notification payload
+    mns_notification_payload = create_mns_notification(record)
+    notification_id = mns_notification_payload.get("id")
+    action_flag = mns_notification_payload.get("filtering", {}).get("action")
+    logger.info(
+        "Processing message",
+        trace_ids={
+            "notification_id": notification_id,
+            "message_id": message_id,
+            "immunisation_id": immunisation_id,
+            "action_flag": action_flag,
+        },
+    )
+
+    # Publish to MNS
+    mns_service.publish_notification(mns_notification_payload)
+    logger.info("Successfully created MNS notification", trace_ids={"mns_notification_id": notification_id})
+
+    return None
 
 
 def extract_trace_ids(record: SQSMessage) -> Tuple[str, str | None]:

lambdas/mns_publisher/src/sqs_dynamo_utils.py

@@ -33,7 +33,7 @@ def extract_sqs_imms_data(sqs_record: dict) -> ImmsData:
     }
 
 
-def _unwrap_dynamodb_value(value) -> Any:
+def _unwrap_dynamodb_value(value: dict) -> Any:
     """
     Unwrap DynamoDB type descriptor to get the actual value.
     DynamoDB types: S (String), N (Number), BOOL, M (Map), L (List), NULL

lambdas/mns_publisher/tests/test_create_notification.py

@@ -143,12 +143,10 @@ def test_get_practitioner_empty_value(self, mock_logger, mock_pds_get):
     def test_get_practitioner_pds_exception(self, mock_logger, mock_pds_get):
         """Test when PDS API raises exception."""
         mock_pds_get.side_effect = Exception("PDS API error")
-
         with self.assertRaises(Exception) as context:
             get_practitioner_details_from_pds("9481152782")
-
-        self.assertEqual(str(context.exception), "PDS API error")
-        mock_logger.exception.assert_called_once()
+            self.assertEqual(str(context.exception), "PDS API error")
+            mock_logger.exception.assert_called_once()
 
     @patch("create_notification.pds_get_patient_details")
     @patch("create_notification.logger")

lambdas/mns_publisher/tests/test_lambda_handler.py

@@ -87,7 +87,7 @@ def setUp(self):
         self.sample_notification = {
             "id": "notif-789",
             "specversion": "1.0",
-            "type": "imms-vaccinations-2",
+            "type": "imms-vaccinations-1",
             "filtering": {"action": "CREATE"},
         }
         self.mock_mns_service = Mock()
@@ -97,11 +97,11 @@ def setUp(self):
     def test_process_record_success(self, mock_logger, mock_create_notification):
         """Test successful processing of a single record."""
         mock_create_notification.return_value = self.sample_notification
-        self.mock_mns_service.publish_notification.return_value = {"status_code": 200}
+        self.mock_mns_service.publish_notification.return_value = None
 
-        result = process_record(self.sample_sqs_record, self.mock_mns_service)
+        # Should not raise exception
+        process_record(self.sample_sqs_record, self.mock_mns_service)
 
-        self.assertIsNone(result)
         mock_create_notification.assert_called_once_with(self.sample_sqs_record)
         self.mock_mns_service.publish_notification.assert_called_once_with(self.sample_notification)
         mock_logger.exception.assert_not_called()
@@ -112,10 +112,10 @@ def test_process_record_create_notification_failure(self, mock_logger, mock_crea
         """Test handling when notification creation fails."""
         mock_create_notification.side_effect = Exception("Creation error")
 
-        result = process_record(self.sample_sqs_record, self.mock_mns_service)
+        # Should raise exception
+        with self.assertRaises(Exception):
+            process_record(self.sample_sqs_record, self.mock_mns_service)
 
-        self.assertEqual(result, {"itemIdentifier": "98ed30eb-829f-41df-8a73-57fef70cf161"})
-        mock_logger.exception.assert_called_once()
         self.mock_mns_service.publish_notification.assert_not_called()
 
     @patch("process_records.create_mns_notification")
@@ -125,22 +125,9 @@ def test_process_record_publish_failure(self, mock_logger, mock_create_notificat
         mock_create_notification.return_value = self.sample_notification
         self.mock_mns_service.publish_notification.side_effect = Exception("Publish error")
 
-        result = process_record(self.sample_sqs_record, self.mock_mns_service)
-
-        self.assertEqual(result, {"itemIdentifier": "98ed30eb-829f-41df-8a73-57fef70cf161"})
-        mock_logger.exception.assert_called_once()
-
-    @patch("process_records.create_mns_notification")
-    @patch("process_records.logger")
-    def test_process_record_logs_trace_ids(self, mock_logger, mock_create_notification):
-        """Test that trace IDs are logged correctly."""
-        mock_create_notification.return_value = self.sample_notification
-
-        process_record(self.sample_sqs_record, self.mock_mns_service)
-
-        # Check info log was called with trace IDs
-        info_calls = [call for call in mock_logger.info.call_args_list if "Processing message" in str(call)]
-        self.assertEqual(len(info_calls), 1)
+        # Should raise exception
+        with self.assertRaises(Exception):
+            process_record(self.sample_sqs_record, self.mock_mns_service)
 
 
 class TestProcessRecords(unittest.TestCase):
@@ -158,33 +145,36 @@ def setUpClass(cls):
 
         cls.sample_sqs_record = raw_event
 
+    @patch("process_records.logger")
     @patch("process_records.get_mns_service")
     @patch("process_records.process_record")
-    def test_process_records_all_success(self, mock_process_record, mock_get_mns):
+    def test_process_records_all_success(self, mock_process_record, mock_get_mns, mock_logger):
         """Test processing multiple records with all successes."""
         mock_mns_service = Mock()
         mock_get_mns.return_value = mock_mns_service
-        mock_process_record.return_value = None  # Success
+        mock_process_record.return_value = None  # No exception
 
         record_2 = self.sample_sqs_record.copy()
         record_2["messageId"] = "different-id"
         records = [self.sample_sqs_record, record_2]
 
         result = process_records(records)
 
-        self.assertEqual(result, [])
+        self.assertEqual(result, {"batchItemFailures": []})
         self.assertEqual(mock_process_record.call_count, 2)
         mock_get_mns.assert_called_once()
+        mock_logger.info.assert_called_with("Successfully processed all 2 messages")
 
+    @patch("process_records.logger")
     @patch("process_records.get_mns_service")
     @patch("process_records.process_record")
-    def test_process_records_partial_failure(self, mock_process_record, mock_get_mns):
+    def test_process_records_partial_failure(self, mock_process_record, mock_get_mns, mock_logger):
         """Test processing with some failures."""
         mock_mns_service = Mock()
         mock_get_mns.return_value = mock_mns_service
         mock_process_record.side_effect = [
             None,  # Success
-            {"itemIdentifier": "msg-456"},  # Failure
+            Exception("Processing error"),  # Failure
         ]
 
         record_2 = self.sample_sqs_record.copy()
@@ -193,24 +183,28 @@ def test_process_records_partial_failure(self, mock_process_record, mock_get_mns
 
         result = process_records(records)
 
-        self.assertEqual(len(result), 1)
-        self.assertEqual(result[0]["itemIdentifier"], "msg-456")
+        self.assertEqual(len(result["batchItemFailures"]), 1)
+        self.assertEqual(result["batchItemFailures"][0]["itemIdentifier"], "msg-456")
+        mock_logger.warning.assert_called_with("Batch completed with 1 failures")
 
+    @patch("process_records.logger")
     @patch("process_records.get_mns_service")
     @patch("process_records.process_record")
-    def test_process_records_empty_list(self, mock_process_record, mock_get_mns):
+    def test_process_records_empty_list(self, mock_process_record, mock_get_mns, mock_logger):
         """Test processing empty record list."""
         mock_mns_service = Mock()
         mock_get_mns.return_value = mock_mns_service
 
         result = process_records([])
 
-        self.assertEqual(result, [])
+        self.assertEqual(result, {"batchItemFailures": []})
         mock_process_record.assert_not_called()
+        mock_logger.info.assert_called_with("Successfully processed all 0 messages")
 
+    @patch("process_records.logger")
     @patch("process_records.get_mns_service")
     @patch("process_records.process_record")
-    def test_process_records_mns_service_created_once(self, mock_process_record, mock_get_mns):
+    def test_process_records_mns_service_created_once(self, mock_process_record, mock_get_mns, mock_logger):
         """Test that MNS service is created only once for batch."""
         mock_mns_service = Mock()
         mock_get_mns.return_value = mock_mns_service
@@ -239,35 +233,30 @@ def setUpClass(cls):
         cls.sample_sqs_record = raw_event
 
     @patch("lambda_handler.process_records")
-    @patch("lambda_handler.logger")
-    def test_lambda_handler_all_success(self, mock_logger, mock_process_records):
+    def test_lambda_handler_all_success(self, mock_process_records):
         """Test lambda handler with all records succeeding."""
-        mock_process_records.return_value = []
+        mock_process_records.return_value = {"batchItemFailures": []}
 
         event = {"Records": [self.sample_sqs_record]}
         result = lambda_handler(event, Mock())
 
         self.assertEqual(result, {"batchItemFailures": []})
         mock_process_records.assert_called_once_with([self.sample_sqs_record])
-        mock_logger.info.assert_called_with("Successfully processed all 1 messages")
 
     @patch("lambda_handler.process_records")
-    @patch("lambda_handler.logger")
-    def test_lambda_handler_with_failures(self, mock_logger, mock_process_records):
+    def test_lambda_handler_with_failures(self, mock_process_records):
         """Test lambda handler with some failures."""
-        mock_process_records.return_value = [{"itemIdentifier": "msg-123"}]
+        mock_process_records.return_value = {"batchItemFailures": [{"itemIdentifier": "msg-123"}]}
 
         event = {"Records": [self.sample_sqs_record]}
         result = lambda_handler(event, Mock())
 
         self.assertEqual(result, {"batchItemFailures": [{"itemIdentifier": "msg-123"}]})
-        mock_logger.warning.assert_called_with("Batch completed with 1 failures")
 
     @patch("lambda_handler.process_records")
-    @patch("lambda_handler.logger")
-    def test_lambda_handler_empty_records(self, mock_logger, mock_process_records):
+    def test_lambda_handler_empty_records(self, mock_process_records):
         """Test lambda handler with no records."""
-        mock_process_records.return_value = []
+        mock_process_records.return_value = {"batchItemFailures": []}
 
         event = {"Records": []}
         result = lambda_handler(event, Mock())

lambdas/shared/src/common/api_clients/mns_service.py

@@ -79,7 +79,6 @@ def get_subscription(self) -> dict | None:
         headers = self._build_headers()
         response = request_with_retry_backoff("GET", f"{MNS_BASE_URL}/subscriptions", headers, timeout=10)
         logging.info(f"GET {MNS_BASE_URL}/subscriptions")
-        logging.debug(f"Headers: {headers}")
 
         if response.status_code == 200:
             bundle = response.json()