diff --git a/README.md b/README.md index 1f5849a..cc91a9b 100644 --- a/README.md +++ b/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" } ``` diff --git a/tap_freshdesk/client.py b/tap_freshdesk/client.py index 9ac440c..79335a9 100644 --- a/tap_freshdesk/client.py +++ b/tap_freshdesk/client.py @@ -2,13 +2,116 @@ import backoff import requests import singer +from simplejson import JSONDecodeError from singer import utils - LOGGER = singer.get_logger() BASE_URL = "https://{}.freshdesk.com" +REQUEST_TIMEOUT = 300 DEFAULT_PAGE_SIZE = 100 +class FreshdeskException(Exception): + """Custom error class for all the freshdesk errors.""" + +class FreshdeskValidationError(FreshdeskException): + """Custom error class for validation error.""" + +class FreshdeskAuthenticationError(FreshdeskException): + """Custom error class for authentication error.""" + +class FreshdeskAccessDeniedError(FreshdeskException): + """Custom error class for access denied error.""" + +class FreshdeskNotFoundError(FreshdeskException): + """Custom error class for not found error.""" + +class FreshdeskMethodNotAllowedError(FreshdeskException): + """Custom error class for method not allowed.""" + +class FreshdeskUnsupportedAcceptHeaderError(FreshdeskException): + """Custom error class for unsupported accept error.""" + +class FreshdeskConflictingStateError(FreshdeskException): + """Custom error class for conflicting state.""" + +class FreshdeskUnsupportedContentError(FreshdeskException): + """Custom error class for unsupported content.""" + +class FreshdeskRateLimitError(FreshdeskException): + """Custom error class for rate limit error.""" + +class Server5xxError(FreshdeskException): + """Custom error class for all the 5xx errors.""" + +class FreshdeskServerError(Server5xxError): + """Custom error class for 500 server error""" + + +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. + """ + error_code = response.status_code + # Forming a response message for raising a custom exception + try: + response_json = response.json() + except JSONDecodeError: + 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: """ @@ -19,6 +122,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() self.page_size = self.get_page_size() def __enter__(self): @@ -29,6 +134,18 @@ 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 + (isinstance(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 get_page_size(self): """ This function will get page size from config, @@ -54,9 +171,8 @@ def check_access_token(self): 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): @@ -69,7 +185,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: @@ -78,6 +194,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() diff --git a/tap_freshdesk/streams.py b/tap_freshdesk/streams.py index edfd1cc..602975e 100644 --- a/tap_freshdesk/streams.py +++ b/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() @@ -330,6 +331,15 @@ 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 %s. This may be caused by tickets " + "marked as spam or deleted.", self.parent_id) + return None + STREAMS = { "agents": Agents, diff --git a/tests/unittests/test_exception_handling.py b/tests/unittests/test_exception_handling.py new file mode 100644 index 0000000..4384ba6 --- /dev/null +++ b/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 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", "code": None}) + 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) + + + +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 timeour 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 %s. This may be caused by tickets marked as spam or deleted.", 10) diff --git a/tests/unittests/test_timeout.py b/tests/unittests/test_timeout.py new file mode 100644 index 0000000..6885d93 --- /dev/null +++ b/tests/unittests/test_timeout.py @@ -0,0 +1,56 @@ +import unittest +from unittest import mock +from parameterized import parameterized +from tap_freshdesk.client import FreshdeskClient, REQUEST_TIMEOUT + + +class TestTimeOut(unittest.TestCase): + """ + Test `set_timeout` method of the client. + """ + + @parameterized.expand([ + ["integer_value", 10, 10.0], + ["float_value", 100.5, 100.5], + ["string_integer", "10", 10.0], + ["string_float", "100.5", 100.5], + ]) + def test_timeout_values(self, name, timeout_value, expected_value): + """ + Test that for the valid value of timeout, + No exception is raised and the expected value is set. + """ + config = {"request_timeout": timeout_value} + _client = FreshdeskClient(config) + + # Verify timeout value is expected + self.assertEqual(_client.timeout, expected_value) + + @parameterized.expand([ + ["integer_zero", 0], + ["float_zero", 0.0], + ["string_zero", "0"], + ["string_float_zero", "0.0"], + ["string_alphabate", "abc"], + ]) + def test_invalid_value(self, name, timeout_value): + """ + Test that for invalid value exception is raised. + """ + config = {"request_timeout": timeout_value} + with self.assertRaises(Exception) as e: + _client = FreshdeskClient(config) + + # Verify that the exception message is expected. + self.assertEqual(str(e.exception), "The entered timeout is invalid, it should be a valid none-zero integer.") + + + def test_none_value(self): + """ + Test if no timeout is not passed in the config, then set it to the default value. + """ + config = {} + _client = FreshdeskClient(config) + + # Verify that the default timeout value is set. + self.assertEqual(_client.timeout, REQUEST_TIMEOUT)