TDL-20359 Add custom exception handling.

README.md

@@ -36,11 +36,13 @@ This tap:
     Create a JSON file called `config.json` containing the api token you just found and
     the subdomain to your Freshdesk account. The subdomain will take the format
     `subdomain.freshdesk.com`.
+    The `request_timeout` is an optional parameter to set the timeout for requests. Default: 300 seconds
 
     ```json
     {
       "api_key": "your-api-token",
       "domain": "subdomain",
+      "request_timeout": 300,
       "start_date": "2017-01-17T20:32:05Z"
     }
     ```

tap_freshdesk/client.py

@@ -7,7 +7,113 @@
 
 LOGGER = singer.get_logger()
 BASE_URL = "https://{}.freshdesk.com"
+REQUEST_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 to make 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"))
+        code = response_json.get("code", "")
+        if code:
+            error_code = f"{str(error_code)} {code}"
+        formatted_message = "HTTP-error-code: {}, Error: {}".format(error_code, message)
+        raise exc(formatted_message) from None
 
 class FreshdeskClient:
     """
@@ -18,6 +124,8 @@ def __init__(self, config):
         self.config = config
         self.session = requests.Session()
         self.base_url = BASE_URL.format(config.get("domain"))
+        self.timeout = REQUEST_TIMEOUT
+        self.set_timeout()
 
     def __enter__(self):
         self.check_access_token()
@@ -27,16 +135,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("request_timeout", REQUEST_TIMEOUT)
+        if ((type(timeout) in [int, float]) or 
+            (type(timeout)==str and timeout.replace('.', '', 1).isdigit())) and float(timeout):
+            self.timeout = 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),
+                          (requests.Timeout, requests.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=None):
@@ -49,7 +168,7 @@ def request(self, url, params=None):
 
         req = requests.Request('GET', url, params=params, auth=(self.config['api_key'], ""), headers=headers).prepare()
         LOGGER.info("GET %s", req.url)
-        response = self.session.send(req)
+        response = self.session.send(req, timeout=self.timeout)
 
         # Call the function again if the rate limit is exceeded
         if 'Retry-After' in response.headers:
@@ -58,6 +177,7 @@ def request(self, url, params=None):
             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

@@ -2,6 +2,7 @@
 from datetime import datetime as dt
 import singer
 from singer.bookmarks import get_bookmark
+from tap_freshdesk.client import FreshdeskNotFoundError
 
 
 LOGGER = singer.get_logger()
@@ -126,7 +127,7 @@ def write_records(self, catalog, state, selected_streams, start_date, data, max_
                             child_obj.parent_id = row['id']
                             child_max_bookmark = get_bookmark(state, child_obj.tap_stream_id, child_obj.replication_keys[0], start_date)
                             # Update the child's max_bookmark as the max of the already present max value and the return value
-                            child_max_bookmark = max(child_max_bookmarks.get(child, child_max_bookmark), child_obj.sync_obj(state, start_date, client, catalog, selected_streams, streams_to_sync))
+                            child_max_bookmark = max(child_max_bookmarks.get(child, child_max_bookmark), child_obj.sync_obj(state, start_date, client, catalog, selected_streams, streams_to_sync) or "")
                             child_max_bookmarks[child] = child_max_bookmark
         return max_bookmark, child_max_bookmarks
 
@@ -301,6 +302,14 @@ class TimeEntries(ChildStream):
     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))
+
 
 STREAMS = {
     "agents": Agents,

tests/unittests/test_exception_handling.py

@@ -0,0 +1,181 @@
+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", None, client.FreshdeskValidationError],
+        [401, "Authentication Failure", "invalid_credentials", client.FreshdeskAuthenticationError],
+        [403, "Access Denied", "access_denied", client.FreshdeskAccessDeniedError],
+        [404, "Requested Resource not Found", None, client.FreshdeskNotFoundError],
+        [405, "Method not allowed", None, client.FreshdeskMethodNotAllowedError],
+        [406, "Unsupported Accept Header", None, client.FreshdeskUnsupportedAcceptHeaderError],
+        [409, "AInconsistent/Conflicting State", "inconsistent_state", client.FreshdeskConflictingStateError],
+        [415, "Unsupported Content-type", "invalid_json", client.FreshdeskUnsupportedContentError],
+        [429, "Rate Limit Exceeded", None, client.FreshdeskRateLimitError],
+        [500, "Unexpected Server Error", None, client.FreshdeskServerError],
+        [503, "Service Unavailable", None, client.Server5xxError],    # Unknown 5xx error
+    ])
+    def test_error_response_message(self, status_code, message, code, error):
+        """
+        Test that error is thrown with description in the response.
+        """
+
+        error_code = status_code
+        if code:
+            error_code = f"{str(status_code)} {code}"
+        expected_message = "HTTP-error-code: {}, Error: {}".format(error_code, message)
+        with self.assertRaises(error) as e:
+            raise_for_error(get_response(status_code, {"description": message, "code": code}))
+
+        # Verify that an error message is expected
+        self.assertEqual(str(e.exception), expected_message)
+
+    def test_json_decoder_error(self):
+        """Test for invalid json response, tap does not throw JSON decoder error."""
+        mock_response = get_response(400)
+        mock_response._content = "ABC".encode()
+        expected_message = "HTTP-error-code: {}, Error: {}".format(400, "The request body/query string is not in the correct format.")
+        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)
+
+
+
+class TestBackoffHandling(unittest.TestCase):
+    """
+    Test backoff handling for all 5xx, timeout and connection error.
+    """
+
+    @parameterized.expand([
+        ["For error 500", lambda *x,**y: get_response(500), client.FreshdeskServerError],
+        ["For 503 (unknown 5xx error)", lambda *x,**y:get_response(503), client.Server5xxError],   # Unknown 5xx error
+        ["For Connection Error", requests.ConnectionError, requests.ConnectionError],
+        ["For timeout Error", requests.Timeout, requests.Timeout],
+    ])
+    @mock.patch("requests.Session.send")
+    @mock.patch("time.sleep")
+    def test_backoff(self, name, mock_response, error, mock_sleep, mock_request):
+        """
+        Test that for 500, timeout and connection error `request` method will back off 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_limit_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.REQUEST_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.")