TDL-16314 added request timeout

.circleci/config.yml

@@ -21,12 +21,14 @@ jobs:
           name: 'pylint'
           command: |
             source /usr/local/share/virtualenvs/tap-xero/bin/activate
-            pylint tap_xero --disable 'broad-except,chained-comparison,empty-docstring,fixme,invalid-name,line-too-long,missing-class-docstring,missing-function-docstring,missing-module-docstring,no-else-raise,no-else-return,too-few-public-methods,too-many-arguments,too-many-branches,too-many-lines,too-many-locals,ungrouped-imports,wrong-spelling-in-comment,wrong-spelling-in-docstring,bad-whitespace,unspecified-encoding'
+            pylint tap_xero --disable 'broad-except,chained-comparison,empty-docstring,fixme,invalid-name,line-too-long,missing-class-docstring,missing-function-docstring,missing-module-docstring,no-else-raise,no-else-return,too-few-public-methods,too-many-arguments,too-many-branches,too-many-lines,too-many-locals,ungrouped-imports,wrong-spelling-in-comment,wrong-spelling-in-docstring,bad-whitespace,unspecified-encoding,consider-using-f-string'
       - run:
           name: 'Unit Tests'
           command: |
             source /usr/local/share/virtualenvs/tap-xero/bin/activate
-            nosetests tests/unittests/
+            pip install nose coverage
+            nosetests --with-coverage --cover-erase --cover-package=tap_xero --cover-html-dir=htmlcov tests/unittests
+            coverage html
       - run:
           name: 'Integration Tests'
           command: |

tap_xero/client.py

@@ -12,11 +12,13 @@
 import pytz
 import backoff
 import singer
+from requests.exceptions import Timeout
 
 LOGGER = singer.get_logger()
 
 BASE_URL = "https://api.xero.com/api.xro/2.0"
 
+REQUEST_TIMEOUT = 300
 
 class XeroError(Exception):
     def __init__(self, message=None, response=None):
@@ -183,7 +185,18 @@ def __init__(self, config):
         self.user_agent = config.get("user_agent")
         self.tenant_id = None
         self.access_token = None
-
+        request_timeout = config.get('request_timeout')
+        # 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 for 1 minute when the Timeout error occurs as the request will again backoff
+    # when timeout occurs in `check_platform_access()`, hence instead of setting max_tries as 5
+    # setting the max_time of 60 seconds
+    @backoff.on_exception(backoff.constant, Timeout, max_time=60, interval=10, jitter=None) # Interval value not consistent if jitter not None
     def refresh_credentials(self, config, config_path):
 
         header_token = b64encode((config["client_id"] + ":" + config["client_secret"]).encode('utf-8'))
@@ -197,7 +210,7 @@ def refresh_credentials(self, config, config_path):
             "grant_type": "refresh_token",
             "refresh_token": config["refresh_token"],
         }
-        resp = self.session.post("https://identity.xero.com/connect/token", headers=headers, data=post_body)
+        resp = self.session.post("https://identity.xero.com/connect/token", headers=headers, data=post_body, timeout=self.request_timeout)
 
         if resp.status_code != 200:
             raise_for_error(resp)
@@ -210,7 +223,10 @@ def refresh_credentials(self, config, config_path):
             self.access_token = resp["access_token"]
             self.tenant_id = config['tenant_id']
 
-
+    # Backoff for 1 minute when the Timeout error occurs as the request will again backoff
+    # when timeout occurs in `refresh_credentials()`, hence instead of setting max_tries as 5
+    # setting the max_time of 60 seconds
+    @backoff.on_exception(backoff.constant, Timeout, max_time=60, interval=10, jitter=None) # Interval value not consistent if jitter not None
     @backoff.on_exception(backoff.expo, (json.decoder.JSONDecodeError, XeroInternalError), max_tries=3)
     @backoff.on_exception(retry_after_wait_gen, XeroTooManyInMinuteError, giveup=is_not_status_code_fn([429]), jitter=None, max_tries=3)
     def check_platform_access(self, config, config_path):
@@ -227,12 +243,14 @@ def check_platform_access(self, config, config_path):
         # Validating the authorization of the provided configuration
         contacts_url = join(BASE_URL, "Contacts")
         request = requests.Request("GET", contacts_url, headers=headers)
-        response = self.session.send(request.prepare())
+        response = self.session.send(request.prepare(), timeout=self.request_timeout)
 
         if response.status_code != 200:
             raise_for_error(response)
 
 
+    # Backoff till 60 seconds in case of Timeout error to keep it consistent with other backoffs
+    @backoff.on_exception(backoff.constant, Timeout, max_time=60, interval=10, jitter=None) # Interval value not consistent if jitter not None
     @backoff.on_exception(backoff.expo, (json.decoder.JSONDecodeError, XeroInternalError), max_tries=3)
     @backoff.on_exception(retry_after_wait_gen, XeroTooManyInMinuteError, giveup=is_not_status_code_fn([429]), jitter=None, max_tries=3)
     def filter(self, tap_stream_id, since=None, **params):
@@ -247,7 +265,7 @@ def filter(self, tap_stream_id, since=None, **params):
             headers["If-Modified-Since"] = since
 
         request = requests.Request("GET", url, headers=headers, params=params)
-        response = self.session.send(request.prepare())
+        response = self.session.send(request.prepare(), timeout=self.request_timeout)
 
         if response.status_code != 200:
             raise_for_error(response)

tests/unittests/test_request_timeout.py

@@ -0,0 +1,191 @@
+from tap_xero.client import XeroClient
+import unittest
+from unittest import mock
+from unittest.case import TestCase
+from requests.exceptions import Timeout, ConnectTimeout
+import datetime
+
+class TestBackoffError(unittest.TestCase):
+    '''
+    Test that backoff logic works properly.
+    '''
+    @mock.patch('tap_xero.client.requests.Request')
+    @mock.patch('tap_xero.client.requests.Session.send')
+    @mock.patch('tap_xero.client.requests.Session.post')
+    def test_backoff_check_platform_access_timeout_error(self, mock_post, mock_send, mock_request):
+        """
+        Check whether the request backoffs properly  for 60 seconds in case of Timeout error.
+        """
+        mock_send.side_effect = Timeout
+        mock_post.side_effect = Timeout
+        before_time = datetime.datetime.now()
+        with self.assertRaises(Timeout):
+            config = {"start_date": "dummy_st", "client_id": "dummy_ci", "client_secret": "dummy_cs", "tenant_id": "dummy_ti", "refresh_token": "dummy_rt"}
+            client = XeroClient(config)   
+            client.check_platform_access(config, "dummy_path")
+        after_time = datetime.datetime.now()
+        time_difference = (after_time - before_time).total_seconds()
+        self.assertGreaterEqual(time_difference, 120)
+
+    @mock.patch('tap_xero.client.requests.Request')
+    @mock.patch('tap_xero.client.requests.Session.send')
+    @mock.patch('tap_xero.client.requests.Session.post')
+    def test_backoff_check_platform_access_connect_timeout_error(self, mock_post, mock_send, mock_request):
+        """
+        Check whether the request backoffs properly for 60 seconds in case of ConnectTimeout error.
+        """
+        mock_send.side_effect = ConnectTimeout
+        mock_post.side_effect = ConnectTimeout
+        before_time = datetime.datetime.now()
+        with self.assertRaises(Timeout):
+            config = {"start_date": "dummy_st", "client_id": "dummy_ci", "client_secret": "dummy_cs", "tenant_id": "dummy_ti", "refresh_token": "dummy_rt"}
+            client = XeroClient(config)   
+            client.check_platform_access(config, "dummy_path")
+        after_time = datetime.datetime.now()
+        time_difference = (after_time - before_time).total_seconds()
+        self.assertGreaterEqual(time_difference, 120)
+
+    @mock.patch('tap_xero.client.requests.Session.post')
+    def test_backoff_refresh_credentials_timeout_error(self, mock_post):
+        """
+        Check whether the request backoffs properly for 60 seconds in case of Timeout error.
+        """
+        mock_post.side_effect = Timeout
+        before_time = datetime.datetime.now()
+        with self.assertRaises(Timeout):
+            config = {"start_date": "dummy_st", "client_id": "dummy_ci", "client_secret": "dummy_cs", "tenant_id": "dummy_ti", "refresh_token": "dummy_rt"}
+            client = XeroClient(config)   
+            client.refresh_credentials(config, "dummy_path")
+        after_time = datetime.datetime.now()
+        time_difference = (after_time - before_time).total_seconds()
+        self.assertGreaterEqual(time_difference, 60)
+
+    @mock.patch('tap_xero.client.requests.Session.post')
+    def test_backoff_refresh_credentials_connect_timeout_error(self, mock_post):
+        """
+        Check whether the request backoffs properly for 60 seconds in case of ConnectTimeout error.
+        """
+        mock_post.side_effect = ConnectTimeout
+        before_time = datetime.datetime.now()
+        with self.assertRaises(Timeout):
+            config = {"start_date": "dummy_st", "client_id": "dummy_ci", "client_secret": "dummy_cs", "tenant_id": "dummy_ti", "refresh_token": "dummy_rt"}
+            client = XeroClient(config)   
+            client.refresh_credentials(config, "dummy_path")
+        after_time = datetime.datetime.now()
+        time_difference = (after_time - before_time).total_seconds()
+        self.assertGreaterEqual(time_difference, 60)
+
+    @mock.patch('tap_xero.client.requests.Session.send')
+    @mock.patch('tap_xero.client.requests.Request')
+    def test_backoff_filter_timeout_error(self, mock_request, mock_send):
+        """
+        Check whether the request backoffs properly for 5 times in case of Timeout error.
+        """
+        mock_send.side_effect = Timeout
+        before_time = datetime.datetime.now()
+        with self.assertRaises(Timeout):
+            config = {"start_date": "dummy_st", "client_id": "dummy_ci", "client_secret": "dummy_cs", "tenant_id": "dummy_ti", "refresh_token": "dummy_rt"}
+            client = XeroClient(config)
+            client.access_token = "dummy_token"
+            client.filter(tap_stream_id='dummy_stream')
+        after_time = datetime.datetime.now()
+        time_difference = (after_time - before_time).total_seconds()
+        self.assertGreaterEqual(time_difference, 60)
+
+class MockResponse():
+    '''
+    Mock response  object for the requests call 
+    '''
+    def __init__(self, resp, status_code, content=[""], headers=None, raise_error=False, text={}):
+        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"
+
+    def prepare(self):
+        return (self.json_data, self.status_code, self.content, self.headers, self.raise_error)
+
+    def json(self):
+        return self.text
+
+class MockRequest():
+    '''
+    Mock Request object for mocking the Requests()
+    '''
+    def __init__(self):
+        pass
+
+    def prepare(self):
+        pass
+
+class TestRequestTimeoutValue(unittest.TestCase):
+    '''
+    Test that request timeout parameter works properly in various cases
+    '''
+    @mock.patch('tap_xero.client.requests.Session.send', return_value = MockResponse("", status_code=200))
+    @mock.patch('tap_xero.client.requests.Request', return_value = MockRequest())
+    @mock.patch('tap_xero.client.XeroClient.refresh_credentials')
+    def test_config_provided_request_timeout(self, mock_refresh_creds, mock_request, mock_send):
+        """ 
+            Unit tests to ensure that request timeout is set based on config value
+        """
+        config = {"start_date": "dummy_st", "client_id": "dummy_ci", "client_secret": "dummy_cs", "tenant_id": "dummy_ti", "refresh_token": "dummy_rt", "request_timeout": 100}
+        client = XeroClient(config)
+        client.access_token = "dummy_access_token"
+        client.check_platform_access("GET", "dummy_path")
+        mock_send.assert_called_with(MockRequest().prepare(), timeout=100.0)
+
+    @mock.patch('tap_xero.client.requests.Session.send', return_value = MockResponse("", status_code=200))
+    @mock.patch('tap_xero.client.requests.Request', return_value = MockRequest())
+    @mock.patch('tap_xero.client.XeroClient.refresh_credentials')
+    def test_default_value_request_timeout(self, mock_refresh_creds, mock_request, mock_send):
+        """ 
+            Unit tests to ensure that request timeout is set based default value
+        """
+        config = {"start_date": "dummy_st", "client_id": "dummy_ci", "client_secret": "dummy_cs", "tenant_id": "dummy_ti", "refresh_token": "dummy_rt"}
+        client = XeroClient(config)
+        client.access_token = "dummy_access_token"
+        client.check_platform_access("GET", "dummy_path")
+        mock_send.assert_called_with(MockRequest().prepare(), timeout=300.0)
+
+    @mock.patch('tap_xero.client.requests.Session.send', return_value = MockResponse("", status_code=200))
+    @mock.patch('tap_xero.client.requests.Request', return_value = MockRequest())
+    @mock.patch('tap_xero.client.XeroClient.refresh_credentials')
+    def test_config_provided_empty_request_timeout(self, mock_refresh_creds, mock_request, mock_send):
+        """ 
+            Unit tests to ensure that request timeout is set based on default value if empty value is given in config
+        """
+        config = {"start_date": "dummy_st", "client_id": "dummy_ci", "client_secret": "dummy_cs", "tenant_id": "dummy_ti", "refresh_token": "dummy_rt", "request_timeout": ""}
+        client = XeroClient(config)
+        client.access_token = "dummy_access_token"
+        client.check_platform_access("GET", "dummy_path")
+        mock_send.assert_called_with(MockRequest().prepare(), timeout=300.0)
+
+    @mock.patch('tap_xero.client.requests.Session.send', return_value = MockResponse("", status_code=200))
+    @mock.patch('tap_xero.client.requests.Request', return_value = MockRequest())
+    @mock.patch('tap_xero.client.XeroClient.refresh_credentials')
+    def test_config_provided_string_request_timeout(self, mock_refresh_creds, mock_request, mock_send):
+        """ 
+            Unit tests to ensure that request timeout is set based on config string value
+        """
+        config = {"start_date": "dummy_st", "client_id": "dummy_ci", "client_secret": "dummy_cs", "tenant_id": "dummy_ti", "refresh_token": "dummy_rt", "request_timeout": "100"}
+        client = XeroClient(config)
+        client.access_token = "dummy_access_token"
+        client.check_platform_access("GET", "dummy_path")
+        mock_send.assert_called_with(MockRequest().prepare(), timeout=100.0)
+
+    @mock.patch('tap_xero.client.requests.Session.send', return_value = MockResponse("", status_code=200))
+    @mock.patch('tap_xero.client.requests.Request', return_value = MockRequest())
+    @mock.patch('tap_xero.client.XeroClient.refresh_credentials')
+    def test_config_provided_float_request_timeout(self, mock_refresh_creds, mock_request, mock_send):
+        """ 
+            Unit tests to ensure that request timeout is set based on config float value
+        """
+        config = {"start_date": "dummy_st", "client_id": "dummy_ci", "client_secret": "dummy_cs", "tenant_id": "dummy_ti", "refresh_token": "dummy_rt", "request_timeout": 100.8}
+        client = XeroClient(config)
+        client.access_token = "dummy_access_token"
+        client.check_platform_access("GET", "dummy_path")
+        mock_send.assert_called_with(MockRequest().prepare(), timeout=100.8)