TDL-20359 Add custom exception handling.

setup.py

@@ -14,6 +14,11 @@
           'requests==2.20.0',
           'backoff==1.8.0'
       ],
+      extras_require={
+          'dev': [
+              'pylint',
+          ]
+      },
       entry_points='''
           [console_scripts]
           tap-freshdesk=tap_freshdesk:main

tap_freshdesk/client.py

@@ -7,7 +7,110 @@
 
 LOGGER = singer.get_logger()
 BASE_URL = "https://{}.freshdesk.com"
+DEFAULT_TIMEOUT = 300
 
+class FreshdeskException(Exception):
+    pass
+
+class FreshdeskValidationError(FreshdeskException):
+    pass
+
+class FreshdeskAuthenticationError(FreshdeskException):
+    pass
+
+class FreshdeskAccessDeniedError(FreshdeskException):
+    pass
+
+class FreshdeskNotFoundError(FreshdeskException):
+    pass
+
+class FreshdeskMethodNotAllowedError(FreshdeskException):
+    pass
+
+class FreshdeskUnsupportedAcceptHeaderError(FreshdeskException):
+    pass
+
+class FreshdeskConflictingStateError(FreshdeskException):
+    pass
+
+class FreshdeskUnsupportedContentError(FreshdeskException):
+    pass
+
+class FreshdeskRateLimitError(FreshdeskException):
+    pass
+
+class Server5xxError(FreshdeskException):
+    pass
+
+class FreshdeskServerError(Server5xxError):
+    pass
+
+
+ERROR_CODE_EXCEPTION_MAPPING = {
+    400: {
+        "raise_exception": FreshdeskValidationError,
+        "message": "The request body/query string is not in the correct format."
+    },
+    401: {
+        "raise_exception": FreshdeskAuthenticationError,
+        "message": "The Authorization header is either missing or incorrect."
+    },
+    403: {
+        "raise_exception": FreshdeskAccessDeniedError,
+        "message": "The agent whose credentials were used in making this request was not authorized to perform this API call."
+    },
+    404: {
+        "raise_exception": FreshdeskNotFoundError,
+        "message": "The request contains invalid ID/Freshdesk domain in the URL or an invalid URL itself."
+    },
+    405: {
+        "raise_exception": FreshdeskMethodNotAllowedError,
+        "message": "This API request used the wrong HTTP verb/method."
+    },
+    406: {
+        "raise_exception": FreshdeskUnsupportedAcceptHeaderError,
+        "message": "Only application/json and */* are supported in 'Accepted' header."
+    },
+    409: {
+        "raise_exception": FreshdeskConflictingStateError,
+        "message": "The resource that is being created/updated is in an inconsistent or conflicting state."
+    },
+    415: {
+        "raise_exception": FreshdeskUnsupportedContentError,
+        "message": "Content type application/xml is not supported. Only application/json is supported."
+    },
+    429: {
+        "raise_exception": FreshdeskRateLimitError,
+        "message": "The API rate limit allotted for your Freshdesk domain has been exhausted."
+    },
+    500: {
+        "raise_exception": FreshdeskServerError,
+        "message": "Unexpected Server Error."
+    },
+}
+
+def raise_for_error(response):
+    """
+    Retrieve the error code and the error message from the response and return custom exceptions accordingly.
+    """
+    try:
+        response.raise_for_status()
+    except (requests.HTTPError) as error:
+        error_code = response.status_code
+        # Forming a response message for raising a custom exception
+        try:
+            response_json = response.json()
+        except Exception:
+            response_json = {}
+
+        if error_code not in ERROR_CODE_EXCEPTION_MAPPING and error_code > 500:
+            # Raise `Server5xxError` for all 5xx unknown error
+            exc = Server5xxError
+        else:
+            exc = ERROR_CODE_EXCEPTION_MAPPING.get(error_code, {}).get("raise_exception", FreshdeskException)
+        message = response_json.get("description", ERROR_CODE_EXCEPTION_MAPPING.get(error_code, {}).get("message", "Unknown Error"))
+        formatted_message = "HTTP-error-code: {}, Error: {}".format(error_code, message)
+        raise exc(formatted_message) from None
 
 class FreshdeskClient:
     """
@@ -18,6 +121,8 @@ def __init__(self, config):
         self.config = config
         self.session = requests.Session()
         self.base_url = BASE_URL.format(config.get("domain"))
+        self.timeout = DEFAULT_TIMEOUT
+        self.set_timeout()
 
     def __enter__(self):
         self.check_access_token()
@@ -27,16 +132,27 @@ def __exit__(self, exception_type, exception_value, traceback):
         # Kill the session instance.
         self.session.close()
 
+    def set_timeout(self):
+        """
+        Set timeout value from config, if the value is passed. 
+        Else raise an exception.
+        """
+        timeout = self.config.get("timeout", DEFAULT_TIMEOUT)
+        if ((type(timeout) in [int, float]) or 
+            (type(timeout)==str and timeout.replace('.', '', 1).isdigit())) and float(timeout):
+            self.timeout = int(float(timeout))
+        else:
+            raise Exception("The entered timeout is invalid, it should be a valid none-zero integer.")
+
     def check_access_token(self):
         """
         Check if the access token is valid.
         """
         self.request(self.base_url+"/api/v2/roles", {"per_page": 1, "page": 1})
 
     @backoff.on_exception(backoff.expo,
-                          (requests.exceptions.RequestException),
+                          (TimeoutError, ConnectionError, Server5xxError),
                           max_tries=5,
-                          giveup=lambda e: e.response is not None and 400 <= e.response.status_code < 500,
                           factor=2)
     @utils.ratelimit(1, 2)
     def request(self, url, params={}):
@@ -49,7 +165,7 @@ def request(self, url, params={}):
 
         req = requests.Request('GET', url, params=params, auth=(self.config['api_key'], ""), headers=headers).prepare()
         LOGGER.info("GET {}".format(req.url))
-        response = self.session.send(req)
+        response = self.session.send(req, timeout=DEFAULT_TIMEOUT)
 
         # Call the function again if the rate limit is exceeded
         if 'Retry-After' in response.headers:
@@ -58,6 +174,7 @@ def request(self, url, params={}):
             time.sleep(retry_after)
             return self.request(url, params)
 
-        response.raise_for_status()
+        if response.status_code != 200:
+            raise_for_error(response)
 
         return response.json()

tap_freshdesk/streams.py

@@ -1,7 +1,7 @@
 from datetime import datetime
 import singer
 from singer import bookmarks
-
+from tap_freshdesk.client import FreshdeskNotFoundError
 
 LOGGER = singer.get_logger()
 PAGE_SIZE = 100
@@ -204,6 +204,16 @@ class TimeEntries(Stream):
     path = 'tickets/{}/time_entries'
     parent = 'tickets'
 
+    def sync_obj(self, state, start_date, client, catalog, selected_streams, streams_to_sync, predefined_filter=None):
+        try:
+            return super().sync_obj(state, start_date, client, catalog, selected_streams, streams_to_sync, predefined_filter)
+        except FreshdeskNotFoundError:
+            # Skipping 404 error as it is returned for deleted tickets and spam
+            LOGGER.warning("Could not retrieve time entries for ticket id {}. This may be caused by tickets "
+                           "marked as spam or deleted.".format(self.parent_id))
+            pass
+
+
 STREAMS = {
     "agents": Agents,
     "companies": Companies,

tests/unittests/test_exception_handling.py

@@ -0,0 +1,175 @@
+import unittest
+from unittest import mock
+import json
+import requests
+from parameterized import parameterized
+from tap_freshdesk import client
+from tap_freshdesk.client import raise_for_error, ERROR_CODE_EXCEPTION_MAPPING
+from tap_freshdesk.streams import Tickets, TimeEntries
+
+def get_response(status_code, json_resp={}, headers = None):
+    """
+    Returns mock response
+    """
+    response = requests.Response()
+    response.status_code = status_code
+    response._content = json.dumps(json_resp).encode()
+    if headers:
+        response.headers = headers
+    return response
+
+
+class TestExceptionHanfling(unittest.TestCase):
+    """
+    Test Error is thrown with the expected error message.
+    """
+
+    @parameterized.expand([
+        (400, client.FreshdeskValidationError),
+        (401, client.FreshdeskAuthenticationError),
+        (403, client.FreshdeskAccessDeniedError),
+        (404, client.FreshdeskNotFoundError),
+        (405, client.FreshdeskMethodNotAllowedError),
+        (406, client.FreshdeskUnsupportedAcceptHeaderError),
+        (409, client.FreshdeskConflictingStateError),
+        (415, client.FreshdeskUnsupportedContentError),
+        (429, client.FreshdeskRateLimitError),
+        (500, client.FreshdeskServerError),
+        (503, client.Server5xxError),  # Unknown 5xx error
+    ])
+    def test_custom_error_message(self, error_code, error):
+        """
+        Test that error is thrown with the custom error message
+        if no description is provided in response.
+        """
+        expected_message = "HTTP-error-code: {}, Error: {}".format(error_code, ERROR_CODE_EXCEPTION_MAPPING.get(error_code, {}).get("message", "Unknown Error"))
+        with self.assertRaises(error) as e:
+            raise_for_error(get_response(error_code))
+
+        # Verify that an error message is expected
+        self.assertEqual(str(e.exception), expected_message)
+
+    @parameterized.expand([
+        (400, "Client or Validation Error", client.FreshdeskValidationError),
+        (401, "Authentication Failure", client.FreshdeskAuthenticationError),
+        (403, "Access Denied", client.FreshdeskAccessDeniedError),
+        (404, "Requested Resource not Found", client.FreshdeskNotFoundError),
+        (405, "Method not allowed", client.FreshdeskMethodNotAllowedError),
+        (406, "Unsupported Accept Header", client.FreshdeskUnsupportedAcceptHeaderError),
+        (409, "AInconsistent/Conflicting State", client.FreshdeskConflictingStateError),
+        (415, "Unsupported Content-type", client.FreshdeskUnsupportedContentError),
+        (429, "Rate Limit Exceeded", client.FreshdeskRateLimitError),
+        (500, "Unexpected Server Error", client.FreshdeskServerError),
+    ])
+    def test_error_response_message(self, error_code, message, error):
+        """
+        Test that error is thrown with description in the response.
+        """
+        expected_message = "HTTP-error-code: {}, Error: {}".format(error_code, message)
+        with self.assertRaises(error) as e:
+            raise_for_error(get_response(error_code, {"description": message}))
+
+        # Verify that an error message is expected
+        self.assertEqual(str(e.exception), expected_message)
+
+    def json_decoder_error(self):
+        """Test for invalid json response, tap does not throw JSON decoder error."""
+        mock_response = get_response(400, {"description": "Client or Validation Error"})
+        mock_response._content = "ABC".encode()
+        expected_message = "HTTP-error-code: {}, Error: {}".format(400, "Client or Validation Error")
+        with self.assertRaises(client.FreshdeskValidationError) as e:
+            raise_for_error(mock_response)
+
+        # Verify that an error message is expected
+        self.assertEqual(str(e.exception), expected_message)
+
+
+@mock.patch("requests.Session.send")
+@mock.patch("time.sleep")
+class TestBackoffHandling(unittest.TestCase):
+    """
+    Test backoff handling for all 5xx, timeout and connection error.
+    """
+
+    @parameterized.expand([
+        (lambda *x,**y:get_response(500), client.FreshdeskServerError),
+        (lambda *x,**y:get_response(503), client.Server5xxError),   # Unknown 5xx error
+        (ConnectionError, ConnectionError),
+        (TimeoutError, TimeoutError),
+    ])
+    def test_backoff(self, mock_sleep, mock_request, mock_response, error):
+        """
+        Test that for 500, timeout and connection error `request` method will backoff 5 times.
+        """
+        mock_request.side_effect = mock_response
+        config = {"user_agent": "SAMPLE_AGENT", "api_key": "TEST_API_KEY"}
+        _client = client.FreshdeskClient(config)
+        with self.assertRaises(error) as e:
+            _client.request("https://TEST_URL.com")
+
+        # Verify that `request` method was called 5 times.
+        self.assertEqual(mock_request.call_count, 5)
+
+
+@mock.patch("requests.Session.send")
+@mock.patch("tap_freshdesk.client.time.sleep")
+class TestRateLimitHandling(unittest.TestCase):
+    """
+    Test rate-limit exception handling.
+    """
+
+    @parameterized.expand([
+        ("30",),
+        ("5",),
+        ("50",),
+    ])
+    def test_rate_limit_exceeded(self, mock_sleep, mock_request, retry_seconds):
+        """
+        Test that when the rate limit is exceeded, the function is called again after `Retry-After` seconds.
+        """
+        mock_request.side_effect = [get_response(429, headers={"Retry-After": retry_seconds}), get_response(200)]
+        config = {"user_agent": "SAMPLE_AGENT", "api_key": "TEST_API_KEY"}
+        _client = client.FreshdeskClient(config)
+        _client.request("https://TEST_URL.com")
+
+        # Verify that `requests` method is called twice.
+        self.assertEqual(mock_request.call_count, 2)
+
+        # Verify that `time.sleep` was called for 'Retry-After' seconds from the header.
+        mock_sleep.assert_any_call(int(retry_seconds))
+
+    def test_rate_limite_not_exceeded(self, mock_sleep, mock_request):
+        """
+        Test that the function will not retry for the success response.
+        """
+        mock_request.side_effect = [get_response(200)]
+        config = {"user_agent": "SAMPLE_AGENT", "api_key": "TEST_API_KEY"}
+        _client = client.FreshdeskClient(config)
+        _client.request("https://TEST_URL.com")
+
+        # Verify that `requests` method is called once.
+        self.assertEqual(mock_request.call_count, 1)
+        mock_request.assert_called_with(mock.ANY, timeout=client.DEFAULT_TIMEOUT)
+
+
+class TestSkip404(unittest.TestCase):
+    """
+    Test handling of 404 for a specific child.
+    """
+
+    @mock.patch("tap_freshdesk.streams.LOGGER.warning")
+    @mock.patch("tap_freshdesk.client.FreshdeskClient.request")
+    def test_child_stream_skips(self, mock_request, mock_logger):
+        """
+        Test that on 404 error is skipped for `TimeEntries`.
+        """
+        stream = TimeEntries()
+        _client = mock.Mock()
+        _client.base_url = ""
+        _client.request.side_effect = client.FreshdeskNotFoundError
+
+        stream.parent_id = 10
+        stream.sync_obj({}, "START_DATE", _client, {}, [], [])
+
+        # Verify that error is not raised and the warning logger is called.
+        mock_logger.assert_called_with("Could not retrieve time entries for ticket id 10. This may be caused by tickets marked as spam or deleted.")