improve exception handling

CHANGELOG.md

@@ -1,5 +1,8 @@
 # Changelog
 
+## 4.0.1
+  * Improve Exception Handling and error messages [#105](https://github.com/singer-io/tap-mambu/pull/105)
+
 ## 4.0.0
   * Update `custom_fields` formatting to handle grouped custom field sets correctly [#102](https://github.com/singer-io/tap-mambu/pull/102)
 

setup.py

@@ -3,7 +3,7 @@
 from setuptools import setup, find_packages
 
 setup(name='tap-mambu',
-      version='4.0.0',
+      version='4.0.1',
       description='Singer.io tap for extracting data from the Mambu 2.0 API',
       author='[email protected]',
       classifiers=['Programming Language :: Python :: 3 :: Only'],
@@ -19,6 +19,7 @@
           'dev': [
               'ipdb',
               'pylint==2.5.3',
+              "mock==5.0.2",
               'pytest==6.2.4'
           ],
           'mambu-tests': [

tap_mambu/helpers/client.py

@@ -2,71 +2,74 @@
 import requests
 import requests.adapters
 from requests.exceptions import ConnectionError
-from singer import metrics
-import singer
+from singer import metrics, get_logger
 
-LOGGER = singer.get_logger()
 
+LOGGER = get_logger()
+class ClientError(Exception):
+    """class representing Generic Http error."""
 
-class Server5xxError(Exception):
-    pass
+    message = None
 
+    def __init__(self, message=None, response=None):
+        super().__init__(message or self.message)
+        self.response = response
 
-class Server429Error(Exception):
-    pass
-
-
-class MambuError(Exception):
-    pass
+class MambuError(ClientError):
 
+    message = "Unable to process request"
 
 class MambuBadRequestError(MambuError):
-    pass
 
+    message = "400: Unable to process request"
 
 class MambuUnauthorizedError(MambuError):
-    pass
 
+    message = "401: Invalid credentials provided"
 
 class MambuRequestFailedError(MambuError):
-    pass
 
+    message = "402: Unable to process request"
 
 class MambuNotFoundError(MambuError):
-    pass
 
+    message = "404: Resource not found"
 
 class MambuMethodNotAllowedError(MambuError):
-    pass
 
+    message = "405: Method Not Allowed"
 
 class MambuConflictError(MambuError):
-    pass
 
+    message = "409: Conflict"
 
 class MambuForbiddenError(MambuError):
-    pass
 
+    message = "403: Insufficient permission to access resource"
 
 class MambuUnprocessableEntityError(MambuError):
-    pass
 
+    message = "422: Unable to process request"
+
+class MambuApiLimitError(ClientError):
+
+    message = "429: The API limit exceeded"
 
 class MambuInternalServiceError(MambuError):
-    pass
 
+    message = "Server Fault, Unable to process request"
 
 class MambuNoCredInConfig(MambuError):
-    pass
 
+    message = "Creds Not Provided"
 
 class MambuNoSubdomainInConfig(MambuError):
-    pass
 
+    message = "Subdomain not Configured"
 
 class MambuNoAuditApikeyInConfig(MambuError):
-    pass
 
+    message = "API Key not provided"
 
 ERROR_CODE_EXCEPTION_MAPPING = {
     400: MambuBadRequestError,
@@ -77,37 +80,35 @@ class MambuNoAuditApikeyInConfig(MambuError):
     405: MambuMethodNotAllowedError,
     409: MambuConflictError,
     422: MambuUnprocessableEntityError,
+    429: MambuApiLimitError,
     500: MambuInternalServiceError}
 
-
-def get_exception_for_error_code(error_code):
-    return ERROR_CODE_EXCEPTION_MAPPING.get(error_code, MambuError)
-
-
 def raise_for_error(response):
-    LOGGER.error('ERROR {}: {}, REASON: {}'.format(response.status_code,
-                                                   response.text, response.reason))
+    """
+    Raises the associated response exception.
+    :param resp: requests.Response object
+    """
     try:
         response.raise_for_status()
     except (requests.HTTPError, requests.ConnectionError) as error:
         try:
-            content_length = len(response.content)
-            if content_length == 0:
-                # There is nothing we can do here since Mambu has neither sent
-                # us a 2xx response nor a response content.
-                return
-            response = response.json()
-            if ('error' in response) or ('errorCode' in response):
-                message = '%s: %s' % (response.get('error', str(error)),
-                                      response.get('message', 'Unknown Error'))
-                error_code = response.get('status')
-                ex = get_exception_for_error_code(error_code)
-                raise ex(message)
+            error_code = response.status_code
+            if response.status_code >= 500:
+                exc = MambuInternalServiceError
+                message = f"{response.status_code}: {MambuInternalServiceError.message}"
             else:
-                raise MambuError(error)
+                exc, message = ERROR_CODE_EXCEPTION_MAPPING.get(error_code, MambuError), None
+            try:
+                if len(response.content) != 0:
+                    response = response.json()
+                    if ('error' in response) or ('errorCode' in response):
+                        message = '%s: %s' % (response.get('error', str(error)),
+                                            response.get('message', 'Unknown Error'))
+            except (ValueError, TypeError):
+                pass
+            raise exc(message, response) from None
         except (ValueError, TypeError):
-            raise MambuError(error)
-
+            raise MambuError(error) from error
 
 class MambuClient(object):
     def __init__(self,
@@ -140,7 +141,7 @@ def __exit__(self, exception_type, exception_value, traceback):
         self.__session.close()
 
     @backoff.on_exception(backoff.expo,
-                          Server5xxError,
+                          MambuInternalServiceError,
                           max_tries=5,
                           factor=2)
     def check_access(self):
@@ -177,7 +178,7 @@ def check_access(self):
             return True
 
     @backoff.on_exception(backoff.expo,
-                          (Server5xxError, ConnectionError, Server429Error),
+                          (MambuInternalServiceError, ConnectionError, MambuApiLimitError),
                           max_tries=7,
                           factor=3)
     def request(self, method, path=None, url=None, json=None, version=None, apikey_type=None, **kwargs):
@@ -224,8 +225,6 @@ def request(self, method, path=None, url=None, json=None, version=None, apikey_t
                 **kwargs)
             timer.tags[metrics.Tag.http_status_code] = response.status_code
 
-        if response.status_code >= 500:
-            raise Server5xxError()
 
         if response.status_code != 200:
             raise_for_error(response)

tap_mambu/helpers/exceptions.py

@@ -1,6 +1,5 @@
 class NoDeduplicationCapabilityException(Exception):
     pass
 
-
 class NoDeduplicationKeyException(Exception):
     pass

mambu_tests/test_client.py → tests/unittests/test_client.py

@@ -5,8 +5,19 @@
 
 from tap_mambu import MambuClient, DEFAULT_PAGE_SIZE
 from tap_mambu.helpers.client import MambuError, MambuNoCredInConfig, MambuNoSubdomainInConfig, \
-    MambuNoAuditApikeyInConfig, Server5xxError, raise_for_error, ERROR_CODE_EXCEPTION_MAPPING
-from .constants import config_json
+    MambuNoAuditApikeyInConfig, MambuInternalServiceError, raise_for_error, ERROR_CODE_EXCEPTION_MAPPING
+
+
+config_json = {'username': 'unit',
+               'password': 'test',
+               'apikey': '',
+               'subdomain': 'unit.test',
+               'test_full_url': 'http://127.0.0.1:1080/test',
+               'start_date': '2021-06-01T00:00:00Z',
+               'lookback_window': 30,
+               'user_agent': '',
+               'page_size': '200',
+               'apikey_audit': 'apikey_audit_test'}
 
 
 @mock.patch("tap_mambu.helpers.client.MambuClient.check_access")
@@ -220,29 +231,22 @@ def test_client_request_no_audit_apikey(mock_check_access):
         client.request(method='GET', apikey_type='audit')
 
 
-@mock.patch("tap_mambu.helpers.client.backoff._decorator._sync.time.sleep")
-@mock.patch("tap_mambu.helpers.client.metrics.http_request_timer")
-@mock.patch("tap_mambu.helpers.client.requests.Session.request")
-@mock.patch("tap_mambu.helpers.client.MambuClient.check_access")
-def test_client_request_500_error(mock_check_access,
-                                  mock_requests_session_request,
-                                  mock_metrics_http_request_timer,
-                                  mock_backoff_on_exception, ):
-    client = MambuClient(username=config_json.get('username'),
-                         password=config_json.get('password'),
-                         subdomain=config_json['subdomain'],
-                         page_size=int(config_json.get('page_size', 500)),
-                         user_agent=config_json['user_agent'],
-                         apikey='',
-                         apikey_audit='')
+@mock.patch("tap_mambu.helpers.client.requests.models.Response.raise_for_status")
+def test_client_request_500_error(mock_requests_raise_for_status):
+    mock_requests_raise_for_status.side_effect = Mock(side_effect=requests.HTTPError('test'))
 
     for status_code in range(500, 600):
+        content = {'error': status_code,
+                   'status': status_code,
+                   'message': 'test'}
         response = Mock()
+        response.content = content
+        response.json.return_value = content
         response.status_code = status_code
-        mock_requests_session_request.return_value = response
-
-        with pytest.raises(Server5xxError):
-            client.request(method='GET')
+        response.raise_for_status = mock_requests_raise_for_status
+        with pytest.raises(MambuInternalServiceError):
+            raise_for_error(response)
+        response.json.assert_called_once()
 
 
 @mock.patch("tap_mambu.helpers.client.raise_for_error")
@@ -288,12 +292,6 @@ def test_raise_for_error_no_content(mock_requests_raise_for_status):
     response.content = []
     response.raise_for_status = mock_requests_raise_for_status
 
-    func_response = raise_for_error(response)
-
-    # test case: when response content is empty -> the function should just return and do nothing
-    assert func_response is None
-    response.json.assert_not_called()
-
     # test case: when something from the try/except HTTPError block throws ValueError, TypeError ->
     # it should raise MambuError
     response.content = None
@@ -309,7 +307,6 @@ def test_raise_for_error_no_content(mock_requests_raise_for_status):
     response.json.return_value = {}
     with pytest.raises(MambuError):
         raise_for_error(response)
-    response.json.assert_called_once()
 
 
 @mock.patch("tap_mambu.helpers.client.requests.models.Response.raise_for_status")
@@ -323,6 +320,7 @@ def test_raise_for_error_known_error_codes(mock_requests_raise_for_status):
         response = Mock()
         response.content = content
         response.json.return_value = content
+        response.status_code = status_code
         response.raise_for_status = mock_requests_raise_for_status
 
         # test if the exceptions are handled correctly, meaning ->
@@ -335,3 +333,32 @@ def test_raise_for_error_known_error_codes(mock_requests_raise_for_status):
             with pytest.raises(ERROR_CODE_EXCEPTION_MAPPING[status_code]):
                 raise_for_error(response)
         response.json.assert_called_once()
+
+
+@pytest.mark.parametrize("status_code, verify_message, expected_exception", [
+    (400, "400: Unable to process request",ERROR_CODE_EXCEPTION_MAPPING[400]),
+    (401, "401: Invalid credentials provided",ERROR_CODE_EXCEPTION_MAPPING[401]),
+    (402, "402: Unable to process request",ERROR_CODE_EXCEPTION_MAPPING[402]),
+    (403, "403: Insufficient permission to access resource",ERROR_CODE_EXCEPTION_MAPPING[403]),
+    (404, "404: Resource not found",ERROR_CODE_EXCEPTION_MAPPING[404]),
+    (405, "405: Method Not Allowed",ERROR_CODE_EXCEPTION_MAPPING[405]),
+    (409, "409: Conflict",ERROR_CODE_EXCEPTION_MAPPING[409]),
+    (422, "422: Unable to process request",ERROR_CODE_EXCEPTION_MAPPING[422]),
+    (500, "500: Server Fault, Unable to process request",ERROR_CODE_EXCEPTION_MAPPING[500]),
+    (503, "503: Server Fault, Unable to process request",ERROR_CODE_EXCEPTION_MAPPING[500]),
+    (504, "504: Server Fault, Unable to process request",ERROR_CODE_EXCEPTION_MAPPING[500])
+    ])
+@mock.patch("tap_mambu.helpers.client.requests.models.Response.raise_for_status")
+def test_error_messages(mock_requests_raise_for_status, status_code, verify_message, expected_exception):
+    mock_requests_raise_for_status.side_effect = Mock(side_effect=requests.HTTPError())
+
+    response = Mock()
+    response.status_code = status_code
+    response.raise_for_status = mock_requests_raise_for_status
+
+    with pytest.raises(expected_exception):
+        try: 
+            raise_for_error(response)
+        except expected_exception as err:
+            assert str(err) == verify_message
+            raise err