Enhance CMAB client error handling and logging; add unit tests for fetch methods

Author: FarhanAnjum-opti

optimizely/cmab/cmab_client.py

@@ -68,29 +68,29 @@ def fetch_decision(
             return variation_id
 
         except requests.RequestException as e:
-            self.logger.error(f"Error fetching CMAB decision: {e}")
-            pass
+            self.logger.error(Errors.CMAB_FETCH_FAILED.format(str(e)))
+            return None
 
     def _do_fetch(self, url: str, request_body: str) -> Optional[str]:
         headers = {'Content-Type': 'application/json'}
         try:
             response = self.http_client.post(url, data=json.dumps(request_body), headers=headers, timeout=MAX_WAIT_TIME)
         except requests.exceptions.RequestException as e:
-            self.logger.exception(str(e))
+            self.logger.exception(Errors.CMAB_FETCH_FAILED.format(str(e)))
             return None
 
         if not 200 <= response.status_code < 300:
-            self.logger.exception(f'CMAB Request failed with status code: {response.status_code}')
+            self.logger.exception(Errors.CMAB_FETCH_FAILED.format(str(response.status_code)))
             return None
 
         try:
             body = response.json()
         except json.JSONDecodeError as e:
-            self.logger.exception(str(e))
+            self.logger.exception(Errors.INVALID_CMAB_FETCH_RESPONSE)
             return None
 
         if not self.validate_response(body):
-            self.logger.exception('Invalid response')
+            self.logger.exception(Errors.INVALID_CMAB_FETCH_RESPONSE)
             return None
 
         return str(body['predictions'][0]['variation_id'])
@@ -115,5 +115,5 @@ def _do_fetch_with_retry(self, url: str, request_body: dict, retry_config: CmabR
                 self.logger.info(f"Retrying CMAB request (attempt: {attempt + 1}) after {backoff} seconds...")
                 time.sleep(backoff)
                 backoff = min(backoff * math.pow(retry_config.backoff_multiplier, attempt + 1), retry_config.max_backoff)
-        self.logger.error("Exhausted all retries for CMAB request.")
+        self.logger.error(Errors.CMAB_FETCH_FAILED.format('Exhausted all retries for CMAB request.'))
         return None

optimizely/helpers/enums.py

@@ -127,6 +127,8 @@ class Errors:
     ODP_INVALID_DATA: Final = 'ODP data is not valid.'
     ODP_INVALID_ACTION: Final = 'ODP action is not valid (cannot be empty).'
     MISSING_SDK_KEY: Final = 'SDK key not provided/cannot be found in the datafile.'
+    CMAB_FETCH_FAILED: Final = 'CMAB decision fetch failed with status: {}'
+    INVALID_CMAB_FETCH_RESPONSE = 'Invalid CMAB fetch response'
 
 
 class ForcedDecisionLogs:

tests/test_cmab_client.py

@@ -0,0 +1,157 @@
+import unittest
+import json
+from unittest.mock import MagicMock, patch, call
+from optimizely.cmab.cmab_client import DefaultCmabClient, CmabRetryConfig
+from requests.exceptions import RequestException
+from optimizely.helpers.enums import Errors
+
+class TestDefaultCmabClient_do_fetch(unittest.TestCase):
+    def setUp(self):
+        self.mock_http_client = MagicMock()
+        self.mock_logger = MagicMock()
+        self.client = DefaultCmabClient(http_client=self.mock_http_client, logger=self.mock_logger)
+
+    def test_do_fetch_success(self):
+        mock_response = MagicMock()
+        mock_response.status_code = 200
+        mock_response.json.return_value = {
+            'predictions': [{'variation_id': 'abc123'}]
+        }
+        self.mock_http_client.post.return_value = mock_response
+
+        result = self.client._do_fetch('http://fake-url', {'some': 'data'})
+        self.assertEqual(result, 'abc123')
+
+    def test_do_fetch_http_exception(self):
+        self.mock_http_client.post.side_effect = RequestException('Connection error')
+        result = self.client._do_fetch('http://fake-url', {'some': 'data'})
+        self.assertIsNone(result)
+        self.mock_logger.exception.assert_called_with(Errors.CMAB_FETCH_FAILED.format('Connection error'))
+
+    def test_do_fetch_non_2xx_status(self):
+        mock_response = MagicMock()
+        mock_response.status_code = 500
+        self.mock_http_client.post.return_value = mock_response
+        result = self.client._do_fetch('http://fake-url', {'some': 'data'})
+        self.assertIsNone(result)
+        self.mock_logger.exception.assert_called_with(Errors.CMAB_FETCH_FAILED.format(str(mock_response.status_code)))
+
+    def test_do_fetch_invalid_json(self):
+        mock_response = MagicMock()
+        mock_response.status_code = 200
+        mock_response.json.side_effect = json.JSONDecodeError("Expecting value", "", 0)
+        self.mock_http_client.post.return_value = mock_response
+        result = self.client._do_fetch('http://fake-url', {'some': 'data'})
+        self.assertIsNone(result)
+        self.mock_logger.exception.assert_called_with(Errors.INVALID_CMAB_FETCH_RESPONSE)
+
+    def test_do_fetch_invalid_response_structure(self):
+        mock_response = MagicMock()
+        mock_response.status_code = 200
+        mock_response.json.return_value = {'no_predictions': []}
+        self.mock_http_client.post.return_value = mock_response
+        result = self.client._do_fetch('http://fake-url', {'some': 'data'})
+        self.assertIsNone(result)
+        self.mock_logger.exception.assert_called_with(Errors.INVALID_CMAB_FETCH_RESPONSE)
+
+class TestDefaultCmabClientWithRetry(unittest.TestCase):
+    def setUp(self):
+        self.mock_http_client = MagicMock()
+        self.mock_logger = MagicMock()
+        self.retry_config = CmabRetryConfig(max_retries=2, initial_backoff=0.01, max_backoff=1, backoff_multiplier=2)
+        self.client = DefaultCmabClient(
+            http_client=self.mock_http_client,
+            logger=self.mock_logger,
+            retry_config=self.retry_config
+        )
+
+    @patch("time.sleep", return_value=None)
+    def test_do_fetch_with_retry_success_on_first_try(self, _):
+        mock_response = MagicMock()
+        mock_response.status_code = 200
+        mock_response.json.return_value = {
+            "predictions": [{"variation_id": "abc123"}]
+        }
+        self.mock_http_client.post.return_value = mock_response
+
+        result = self.client._do_fetch_with_retry("http://fake-url", {}, self.retry_config)
+        self.assertEqual(result, "abc123")
+        self.assertEqual(self.mock_http_client.post.call_count, 1)
+
+    @patch("time.sleep", return_value=None)
+    def test_do_fetch_with_retry_success_on_retry(self, _):
+        # First call fails, second call succeeds
+        failure_response = MagicMock()
+        failure_response.status_code = 500
+
+        success_response = MagicMock()
+        success_response.status_code = 200
+        success_response.json.return_value = {
+            "predictions": [{"variation_id": "xyz456"}]
+        }
+
+        self.mock_http_client.post.side_effect = [
+            failure_response,
+            success_response
+        ]
+
+        result = self.client._do_fetch_with_retry("http://fake-url", {}, self.retry_config)
+        self.assertEqual(result, "xyz456")
+        self.assertEqual(self.mock_http_client.post.call_count, 2)
+        self.mock_logger.info.assert_called_with("Retrying CMAB request (attempt: 1) after 0.01 seconds...")
+
+    @patch("time.sleep", return_value=None)
+    def test_do_fetch_with_retry_exhausts_all_attempts(self, _):
+        failure_response = MagicMock()
+        failure_response.status_code = 500
+
+        self.mock_http_client.post.return_value = failure_response
+
+        result = self.client._do_fetch_with_retry("http://fake-url", {}, self.retry_config)
+        self.assertIsNone(result)
+        self.assertEqual(self.mock_http_client.post.call_count, 3)  # 1 original + 2 retries
+        self.mock_logger.error.assert_called_with(Errors.CMAB_FETCH_FAILED.format("Exhausted all retries for CMAB request."))
+
+class TestDefaultCmabClientFetchDecision(unittest.TestCase):
+    def setUp(self):
+        self.mock_http_client = MagicMock()
+        self.mock_logger = MagicMock()
+        self.retry_config = CmabRetryConfig(max_retries=2, initial_backoff=0.01, max_backoff=1, backoff_multiplier=2)
+        self.client = DefaultCmabClient(
+            http_client=self.mock_http_client,
+            logger=self.mock_logger,
+            retry_config=self.retry_config
+        )
+        self.rule_id = 'test_rule'
+        self.user_id = 'user123'
+        self.attributes = {'attr1': 'value1'}
+        self.cmab_uuid = 'uuid-1234'
+
+    @patch.object(DefaultCmabClient, '_do_fetch', return_value='var-abc')
+    def test_fetch_decision_success_no_retry(self, mock_do_fetch):
+        result = self.client.fetch_decision(self.rule_id, self.user_id, self.attributes, self.cmab_uuid)
+        self.assertEqual(result, 'var-abc')
+        mock_do_fetch.assert_called_once()
+
+    @patch.object(DefaultCmabClient, '_do_fetch_with_retry', return_value='var-xyz')
+    def test_fetch_decision_success_with_retry(self, mock_do_fetch_with_retry):
+        client_with_retry = DefaultCmabClient(
+            http_client=self.mock_http_client,
+            logger=self.mock_logger,
+            retry_config=self.retry_config
+        )
+        result = client_with_retry.fetch_decision(self.rule_id, self.user_id, self.attributes, self.cmab_uuid)
+        self.assertEqual(result, 'var-xyz')
+        mock_do_fetch_with_retry.assert_called_once()
+
+    @patch.object(DefaultCmabClient, '_do_fetch', side_effect=RequestException("Network error"))
+    def test_fetch_decision_request_exception(self, mock_do_fetch):
+        result = self.client.fetch_decision(self.rule_id, self.user_id, self.attributes, self.cmab_uuid)
+        self.assertIsNone(result)
+        self.mock_logger.error.assert_called_with(Errors.CMAB_FETCH_FAILED.format("Network error"))
+
+    @patch.object(DefaultCmabClient, '_do_fetch', return_value=None)
+    def test_fetch_decision_invalid_response(self, mock_do_fetch):
+        result = self.client.fetch_decision(self.rule_id, self.user_id, self.attributes, self.cmab_uuid)
+        self.assertIsNone(result)
+        self.mock_logger.error.assert_called_once()