diff --git a/CHANGELOG.md b/CHANGELOG.md index 49efe0f..981556a 100644 --- a/CHANGELOG.md +++ b/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) diff --git a/setup.py b/setup.py index b411f4d..c27150c 100644 --- a/setup.py +++ b/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='jeff.huth@bytecode.io', 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': [ diff --git a/tap_mambu/helpers/client.py b/tap_mambu/helpers/client.py index 1ce03a6..12c2284 100644 --- a/tap_mambu/helpers/client.py +++ b/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) diff --git a/tap_mambu/helpers/exceptions.py b/tap_mambu/helpers/exceptions.py index b80edf1..ed1ad8a 100644 --- a/tap_mambu/helpers/exceptions.py +++ b/tap_mambu/helpers/exceptions.py @@ -1,6 +1,5 @@ class NoDeduplicationCapabilityException(Exception): pass - class NoDeduplicationKeyException(Exception): pass diff --git a/mambu_tests/test_client.py b/tests/unittests/test_client.py similarity index 84% rename from mambu_tests/test_client.py rename to tests/unittests/test_client.py index bb38eef..00f096f 100644 --- a/mambu_tests/test_client.py +++ b/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