From 15750ea2c71c75fcb75be8ef1de1b6f43c07a011 Mon Sep 17 00:00:00 2001 From: namrata270998 Date: Tue, 16 Nov 2021 11:19:37 +0000 Subject: [PATCH 01/10] TDL-16368 ad request timeout --- tap_recharge/__init__.py | 4 +- tap_recharge/client.py | 32 ++++-- tests/unittests/test_request_timeout.py | 129 ++++++++++++++++++++++++ 3 files changed, 157 insertions(+), 8 deletions(-) create mode 100644 tests/unittests/test_request_timeout.py diff --git a/tap_recharge/__init__.py b/tap_recharge/__init__.py index dfdaf9f..e0e252f 100644 --- a/tap_recharge/__init__.py +++ b/tap_recharge/__init__.py @@ -30,7 +30,9 @@ def main(): with RechargeClient( parsed_args.config['access_token'], - parsed_args.config['user_agent']) as client: + parsed_args.config['user_agent'], + parsed_args.config.get('request_timeout') + ) as client: state = {} if parsed_args.state: diff --git a/tap_recharge/client.py b/tap_recharge/client.py index 108f65f..38633fe 100644 --- a/tap_recharge/client.py +++ b/tap_recharge/client.py @@ -3,9 +3,10 @@ import singer from singer import metrics, utils +from requests.exceptions import Timeout LOGGER = singer.get_logger() - +REQUEST_TIMEOUT = 300 class Server5xxError(Exception): pass @@ -101,13 +102,26 @@ class RechargeClient: def __init__( self, access_token, - user_agent=None): + user_agent=None, + request_timeout=REQUEST_TIMEOUT): self.__access_token = access_token self.__user_agent = user_agent self.__session = requests.Session() self.__base_url = None self.__verified = False - + # if request_timeout is other than 0,"0" or "" then use request_timeout + if request_timeout and float(request_timeout): + request_timeout = float(request_timeout) + else: # If value is 0,"0" or "" then set default to 300 seconds. + request_timeout = REQUEST_TIMEOUT + self.request_timeout = request_timeout + + # Backoff the request for 5 times when Timeout error occurs + @backoff.on_exception( + backoff.expo, + Timeout, + max_tries=5, + factor=2) def __enter__(self): self.__verified = self.check_access_token() return self @@ -115,6 +129,7 @@ def __enter__(self): def __exit__(self, exception_type, exception_value, traceback): self.__session.close() + # Backoff the request for 5 times when Timeout error occurs @backoff.on_exception( backoff.expo, Server5xxError, @@ -131,14 +146,18 @@ def check_access_token(self): response = self.__session.get( # Simple endpoint that returns 1 record w/ default organization URN url='https://api.rechargeapps.com', - headers=headers) + headers=headers, + timeout=self.request_timeout) if response.status_code != 200: LOGGER.error('Error status_code = %s', response.status_code) raise_for_error(response) else: return True - + @backoff.on_exception( + backoff.constant, + Timeout, + max_tries=5) # Interval value not consistent if jitter not None @backoff.on_exception( backoff.expo, (Server5xxError, requests.ConnectionError, Server429Error), @@ -174,7 +193,7 @@ def request(self, method, path=None, url=None, **kwargs): kwargs['headers']['Content-Type'] = 'application/json' with metrics.http_request_timer(endpoint) as timer: - response = self.__session.request(method, url, stream=True, **kwargs) + response = self.__session.request(method, url, stream=True, timeout=self.request_timeout, **kwargs) timer.tags[metrics.Tag.http_status_code] = response.status_code if response.status_code >= 500: @@ -192,7 +211,6 @@ def request(self, method, path=None, url=None, **kwargs): except Exception as err: LOGGER.error(err) raise Exception(err) - return response_json, response.links def get(self, path, **kwargs): diff --git a/tests/unittests/test_request_timeout.py b/tests/unittests/test_request_timeout.py new file mode 100644 index 0000000..05e000f --- /dev/null +++ b/tests/unittests/test_request_timeout.py @@ -0,0 +1,129 @@ +from tap_recharge.client import RechargeClient +import unittest +from unittest import mock +from unittest.case import TestCase +from requests.exceptions import Timeout + +class TestBackoffError(unittest.TestCase): + ''' + Test that backoff logic works properly. + ''' + @mock.patch('tap_recharge.client.requests.Session.request') + @mock.patch('tap_recharge.client.RechargeClient.check_access_token') + def test_request_timeout_and_backoff(self, mock_get_token, mock_request): + """ + Check whether the request backoffs properly for request() for 5 times in case of Timeout error. + """ + mock_request.side_effect = Timeout + with self.assertRaises(Timeout): + client = RechargeClient("dummy_access_token", "dummy_user_agent", 300) + client.request("GET") + self.assertEquals(mock_request.call_count, 5) + + @mock.patch('tap_recharge.client.requests.Session.request') + def test_check_access_token_timeout_and_backoff(self, mocked_request): + """ + Check whether the request backoffs properly for __enter__() for 5 times in case of Timeout error. + """ + mocked_request.side_effect = Timeout + + config = { + "access_token": "dummy_at", + "user_agent": "dummy_ua" + } + # initialize 'LinkedinClient' + try: + with RechargeClient( + config['access_token'], + config['user_agent'], + config.get('request_timeout')) as client: + pass + except Timeout: + pass + + # verify that we backoff for 5 times + self.assertEquals(mocked_request.call_count, 5) + +class MockResponse(): + ''' + Mock response object for the requests call + ''' + def __init__(self, resp, status_code, content=[""], headers=None, raise_error=False, text={}, links=""): + self.json_data = resp + self.status_code = status_code + self.content = content + self.headers = headers + self.raise_error = raise_error + self.text = text + self.reason = "error" + self.links = links + + def prepare(self): + return (self.json_data, self.status_code, self.content, self.headers, self.raise_error) + + def json(self): + return self.text + +class TestRequestTimeoutValue(unittest.TestCase): + ''' + Test that request timeout parameter works properly in various cases + ''' + @mock.patch('tap_recharge.client.requests.Session.request', return_value = MockResponse("", status_code=200)) + @mock.patch('tap_recharge.client.RechargeClient.check_access_token') + def test_config_provided_request_timeout(self, mock_get, mock_request): + """ + Unit tests to ensure that request timeout is set based on config value + """ + config = {"access_token": "dummy_at", "user_agent": "dummy_ua", "request_timeout": 100} + client = RechargeClient(**config) + client.request("GET", "dummy_path") + + mock_request.assert_called_with('GET', 'https://api.rechargeapps.com/dummy_path', stream=True, timeout=100.0, headers={'X-Recharge-Access-Token': 'dummy_at', 'Accept': 'application/json', 'User-Agent': 'dummy_ua'}) + + @mock.patch('tap_recharge.client.requests.Session.request', return_value = MockResponse("", status_code=200)) + @mock.patch('tap_recharge.client.RechargeClient.check_access_token') + def test_default_value_request_timeout(self, mock_get, mock_request): + """ + Unit tests to ensure that request timeout is set based default value + """ + config = {"access_token": "dummy_at", "user_agent": "dummy_ua"} + client = RechargeClient(**config) + client.request("GET", "dummy_path") + + mock_request.assert_called_with('GET', 'https://api.rechargeapps.com/dummy_path', stream=True, timeout=300.0, headers={'X-Recharge-Access-Token': 'dummy_at', 'Accept': 'application/json', 'User-Agent': 'dummy_ua'}) + + @mock.patch('tap_recharge.client.requests.Session.request', return_value = MockResponse("", status_code=200)) + @mock.patch('tap_recharge.client.RechargeClient.check_access_token') + def test_config_provided_empty_request_timeout(self, mock_get, mock_request): + """ + Unit tests to ensure that request timeout is set based on default value if empty value is given in config + """ + config = {"access_token": "dummy_at", "user_agent": "dummy_ua", "request_timeout": ""} + client = RechargeClient(**config) + client.request("GET", "dummy_path") + + mock_request.assert_called_with('GET', 'https://api.rechargeapps.com/dummy_path', stream=True, timeout=300.0, headers={'X-Recharge-Access-Token': 'dummy_at', 'Accept': 'application/json', 'User-Agent': 'dummy_ua'}) + + @mock.patch('tap_recharge.client.requests.Session.request', return_value = MockResponse("", status_code=200)) + @mock.patch('tap_recharge.client.RechargeClient.check_access_token') + def test_config_provided_string_request_timeout(self, mock_get, mock_request): + """ + Unit tests to ensure that request timeout is set based on config string value + """ + config = {"access_token": "dummy_at", "user_agent": "dummy_ua", "request_timeout": "100"} + client = RechargeClient(**config) + client.request("GET", "dummy_path") + + mock_request.assert_called_with('GET', 'https://api.rechargeapps.com/dummy_path', stream=True, timeout=100.0, headers={'X-Recharge-Access-Token': 'dummy_at', 'Accept': 'application/json', 'User-Agent': 'dummy_ua'}) + + @mock.patch('tap_recharge.client.requests.Session.request', return_value = MockResponse("", status_code=200)) + @mock.patch('tap_recharge.client.RechargeClient.check_access_token') + def test_config_provided_float_request_timeout(self, mock_get, mock_request): + """ + Unit tests to ensure that request timeout is set based on config float value + """ + config = {"access_token": "dummy_at", "user_agent": "dummy_ua", "request_timeout": 100.8} + client = RechargeClient(**config) + client.request("GET", "dummy_path") + + mock_request.assert_called_with('GET', 'https://api.rechargeapps.com/dummy_path', stream=True, timeout=100.8, headers={'X-Recharge-Access-Token': 'dummy_at', 'Accept': 'application/json', 'User-Agent': 'dummy_ua'}) From 6c78c910ee9066614b62633fb1c1cfc266e33718 Mon Sep 17 00:00:00 2001 From: namrata270998 Date: Tue, 16 Nov 2021 11:20:54 +0000 Subject: [PATCH 02/10] added coverage --- .circleci/config.yml | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/.circleci/config.yml b/.circleci/config.yml index 307b36d..a68ed38 100644 --- a/.circleci/config.yml +++ b/.circleci/config.yml @@ -16,6 +16,17 @@ jobs: command: | source /usr/local/share/virtualenvs/tap-recharge/bin/activate pylint tap_recharge --disable 'missing-module-docstring,missing-function-docstring,missing-class-docstring,no-else-raise,raise-missing-from,inconsistent-return-statements' + - run: + name: 'Unit Tests' + command: | + source /usr/local/share/virtualenvs/tap-recharge/bin/activate + pip install nose coverage + nosetests --with-coverage --cover-erase --cover-package=tap_recharge --cover-html-dir=htmlcov tests/unittests + coverage html + - store_test_results: + path: test_output/report.xml + - store_artifacts: + path: htmlcov - run: when: always name: 'Integration Tests' From 0c2e94140f984d4255d65a6fed2c12eaf38da815 Mon Sep 17 00:00:00 2001 From: namrata270998 Date: Tue, 16 Nov 2021 11:26:18 +0000 Subject: [PATCH 03/10] resolved pylint --- .circleci/config.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.circleci/config.yml b/.circleci/config.yml index a68ed38..365f9bb 100644 --- a/.circleci/config.yml +++ b/.circleci/config.yml @@ -15,7 +15,7 @@ jobs: name: 'pylint' command: | source /usr/local/share/virtualenvs/tap-recharge/bin/activate - pylint tap_recharge --disable 'missing-module-docstring,missing-function-docstring,missing-class-docstring,no-else-raise,raise-missing-from,inconsistent-return-statements' + pylint tap_recharge --disable 'missing-module-docstring,missing-function-docstring,missing-class-docstring,no-else-raise,raise-missing-from,inconsistent-return-statements,line-too-long' - run: name: 'Unit Tests' command: | From cfe703af0cfa6d7ee317e18e2ecf66133eeb0a94 Mon Sep 17 00:00:00 2001 From: namrata270998 Date: Tue, 16 Nov 2021 11:45:08 +0000 Subject: [PATCH 04/10] added connectionerror in backoff --- tap_recharge/client.py | 4 ++-- tests/unittests/test_request_timeout.py | 26 +++++++++++++++++++++++-- 2 files changed, 26 insertions(+), 4 deletions(-) diff --git a/tap_recharge/client.py b/tap_recharge/client.py index 38633fe..0674f36 100644 --- a/tap_recharge/client.py +++ b/tap_recharge/client.py @@ -116,10 +116,10 @@ def __init__( request_timeout = REQUEST_TIMEOUT self.request_timeout = request_timeout - # Backoff the request for 5 times when Timeout error occurs + # Backoff the request for 5 times when Timeout or Connection error occurs @backoff.on_exception( backoff.expo, - Timeout, + (Timeout, requests.ConnectionError), max_tries=5, factor=2) def __enter__(self): diff --git a/tests/unittests/test_request_timeout.py b/tests/unittests/test_request_timeout.py index 05e000f..15c2ccf 100644 --- a/tests/unittests/test_request_timeout.py +++ b/tests/unittests/test_request_timeout.py @@ -2,7 +2,7 @@ import unittest from unittest import mock from unittest.case import TestCase -from requests.exceptions import Timeout +from requests.exceptions import Timeout, ConnectionError class TestBackoffError(unittest.TestCase): ''' @@ -31,7 +31,7 @@ def test_check_access_token_timeout_and_backoff(self, mocked_request): "access_token": "dummy_at", "user_agent": "dummy_ua" } - # initialize 'LinkedinClient' + # initialize 'RechargeClient' try: with RechargeClient( config['access_token'], @@ -40,7 +40,29 @@ def test_check_access_token_timeout_and_backoff(self, mocked_request): pass except Timeout: pass + # verify that we backoff for 5 times + self.assertEquals(mocked_request.call_count, 5) + @mock.patch('tap_recharge.client.requests.Session.request') + def test_check_access_token_connection_error_and_backoff(self, mocked_request): + """ + Check whether the request backoffs properly for __enter__() for 5 times in case of Timeout error. + """ + mocked_request.side_effect = ConnectionError + + config = { + "access_token": "dummy_at", + "user_agent": "dummy_ua" + } + # initialize 'RechargeClient' + try: + with RechargeClient( + config['access_token'], + config['user_agent'], + config.get('request_timeout')) as client: + pass + except ConnectionError: + pass # verify that we backoff for 5 times self.assertEquals(mocked_request.call_count, 5) From 9cad972f80793782b8f6b2fafe091878fe83f4b2 Mon Sep 17 00:00:00 2001 From: namrata270998 Date: Tue, 16 Nov 2021 11:47:54 +0000 Subject: [PATCH 05/10] updated readme --- README.md | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/README.md b/README.md index 1d183a8..388b737 100644 --- a/README.md +++ b/README.md @@ -167,11 +167,12 @@ This tap: { "access_token": "YOUR_ACCESS_TOKEN", "start_date": "2019-01-01T00:00:00Z", - "user_agent": "tap-recharge " + "user_agent": "tap-recharge ", + "request_timeout": 300 } ``` - Optionally, also create a `state.json` file. `currently_syncing` is an optional attribute used for identifying the last object to be synced in case the job is interrupted mid-stream. The next run would begin where the last job left off. + Optionally, also create a `state.json` file. `currently_syncing` is an optional attribute used for identifying the last object to be synced in case the job is interrupted mid-stream. The next run would begin where the last job left off. The `request_timeout` is an optional paramater to set timeout for requests. Default: 300 seconds ```json { From c0ff5196e793167e63d9b913aa2b29943077b8f5 Mon Sep 17 00:00:00 2001 From: namrata270998 <75604662+namrata270998@users.noreply.github.com> Date: Fri, 19 Nov 2021 12:13:25 +0530 Subject: [PATCH 06/10] Update README.md Co-authored-by: Umang Agrawal <80704207+umangagrawal-crest@users.noreply.github.com> --- README.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/README.md b/README.md index 388b737..b98fe3f 100644 --- a/README.md +++ b/README.md @@ -172,7 +172,7 @@ This tap: } ``` - Optionally, also create a `state.json` file. `currently_syncing` is an optional attribute used for identifying the last object to be synced in case the job is interrupted mid-stream. The next run would begin where the last job left off. The `request_timeout` is an optional paramater to set timeout for requests. Default: 300 seconds + Optionally, also create a `state.json` file. `currently_syncing` is an optional attribute used for identifying the last object to be synced in case the job is interrupted mid-stream. The next run would begin where the last job left off. The `request_timeout` is an optional parameter to set a timeout for requests. Default: 300 seconds ```json { From 4ad5ca5ce675219a22592548aa017ac1ddbd5153 Mon Sep 17 00:00:00 2001 From: namrata270998 Date: Fri, 19 Nov 2021 08:03:34 +0000 Subject: [PATCH 07/10] added factor in backoff --- tap_recharge/client.py | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/tap_recharge/client.py b/tap_recharge/client.py index 0674f36..50939fe 100644 --- a/tap_recharge/client.py +++ b/tap_recharge/client.py @@ -154,10 +154,12 @@ def check_access_token(self): else: return True + # Backoff for 5 times when Timeout error occurs @backoff.on_exception( - backoff.constant, + backoff.expo, Timeout, - max_tries=5) # Interval value not consistent if jitter not None + max_tries=5, + factor=2) @backoff.on_exception( backoff.expo, (Server5xxError, requests.ConnectionError, Server429Error), From acbcb56825ad8fc64d62f0786a51b100f5e753e1 Mon Sep 17 00:00:00 2001 From: namrata270998 Date: Fri, 19 Nov 2021 09:52:47 +0000 Subject: [PATCH 08/10] resolved commets --- tap_recharge/client.py | 9 ++------- 1 file changed, 2 insertions(+), 7 deletions(-) diff --git a/tap_recharge/client.py b/tap_recharge/client.py index 50939fe..d6c45e1 100644 --- a/tap_recharge/client.py +++ b/tap_recharge/client.py @@ -154,15 +154,10 @@ def check_access_token(self): else: return True - # Backoff for 5 times when Timeout error occurs + # Added backoff for 5 times when Timeout error occurs @backoff.on_exception( backoff.expo, - Timeout, - max_tries=5, - factor=2) - @backoff.on_exception( - backoff.expo, - (Server5xxError, requests.ConnectionError, Server429Error), + (Timeout, Server5xxError, requests.ConnectionError, Server429Error), max_tries=5, factor=2) # Call/rate limit: https://developer.rechargepayments.com/#rate-limits From f61529e9363cde9b9c2c3541227eca3db2234af5 Mon Sep 17 00:00:00 2001 From: namrata270998 Date: Wed, 1 Dec 2021 15:56:15 +0530 Subject: [PATCH 09/10] resolved comments --- tests/unittests/test_request_timeout.py | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/tests/unittests/test_request_timeout.py b/tests/unittests/test_request_timeout.py index 15c2ccf..fa29f76 100644 --- a/tests/unittests/test_request_timeout.py +++ b/tests/unittests/test_request_timeout.py @@ -1,7 +1,6 @@ from tap_recharge.client import RechargeClient import unittest from unittest import mock -from unittest.case import TestCase from requests.exceptions import Timeout, ConnectionError class TestBackoffError(unittest.TestCase): @@ -15,8 +14,8 @@ def test_request_timeout_and_backoff(self, mock_get_token, mock_request): Check whether the request backoffs properly for request() for 5 times in case of Timeout error. """ mock_request.side_effect = Timeout + client = RechargeClient("dummy_access_token", "dummy_user_agent", 300) with self.assertRaises(Timeout): - client = RechargeClient("dummy_access_token", "dummy_user_agent", 300) client.request("GET") self.assertEquals(mock_request.call_count, 5) From 9e34a7329869c6e6c0ceecee7c2da99b191b3935 Mon Sep 17 00:00:00 2001 From: namrata270998 Date: Wed, 1 Dec 2021 17:43:22 +0530 Subject: [PATCH 10/10] resolved comments --- tests/unittests/test_request_timeout.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/unittests/test_request_timeout.py b/tests/unittests/test_request_timeout.py index fa29f76..5f50c68 100644 --- a/tests/unittests/test_request_timeout.py +++ b/tests/unittests/test_request_timeout.py @@ -111,7 +111,7 @@ def test_default_value_request_timeout(self, mock_get, mock_request): client = RechargeClient(**config) client.request("GET", "dummy_path") - mock_request.assert_called_with('GET', 'https://api.rechargeapps.com/dummy_path', stream=True, timeout=300.0, headers={'X-Recharge-Access-Token': 'dummy_at', 'Accept': 'application/json', 'User-Agent': 'dummy_ua'}) + mock_request.assert_called_with('GET', 'https://api.rechargeapps.com/dummy_path', stream=True, timeout=300, headers={'X-Recharge-Access-Token': 'dummy_at', 'Accept': 'application/json', 'User-Agent': 'dummy_ua'}) @mock.patch('tap_recharge.client.requests.Session.request', return_value = MockResponse("", status_code=200)) @mock.patch('tap_recharge.client.RechargeClient.check_access_token')