TDL-24567: Fix refreshing-the-token logic (#35)

* Add logging so we know what's happening

* `refresh()` should not use the custom `request` method

because `refresh()` would recursively call `request` until the tap errors

* Fix the weird unittests

* Squish all unused variables into `*args`

* Bump to `v1.1.3`, update changelog [skip-ci]

CHANGELOG.md

@@ -1,5 +1,8 @@
 # Changelog
 
+## 1.1.3
+  * Fix refresh logic for long running pagination requests [#35](https://github.com/singer-io/tap-outreach/pull/35)
+
 ## 1.1.2
   * Add handling for new x-rate-limit-remaining structure [#33](https://github.com/singer-io/tap-outreach/pull/33)
 

setup.py

@@ -3,7 +3,7 @@
 from setuptools import setup
 
 setup(name='tap-outreach',
-      version='1.1.2',
+      version='1.1.3',
       description='Singer.io tap for extracting data from the Outreach.io API',
       author='Stitch',
       url='https://singer.io',

tap_outreach/client.py

@@ -47,23 +47,24 @@ def __exit__(self, type, value, traceback):
         self.__session.close()
 
     def refresh(self):
-        data = self.request(
+        resp = self.__session.request(
             'POST',
             url='https://api.outreach.io/oauth/token',
-            skip_quota=True,
             data={
                 'client_id': self.__client_id,
                 'client_secret': self.__client_secret,
                 'redirect_uri': self.__redirect_uri,
                 'refresh_token': self.__refresh_token,
                 'grant_type': 'refresh_token'
             })
+        data = resp.json()
 
         self.__access_token = data['access_token']
 
         self.__expires_at = datetime.utcnow() + \
             timedelta(seconds=data['expires_in'] -
                       10)  # pad by 10 seconds for clock drift
+        LOGGER.info(f"Refreshed access token, expires at {self.__expires_at}")
 
     @staticmethod
     def sleep_for_reset_period(response):
@@ -82,10 +83,11 @@ def sleep_for_reset_period(response):
     # Rate Limit: https://api.outreach.io/api/v2/docs#rate-limiting
     @utils.ratelimit(10000, 3600)
     def request(self, method, path=None, url=None, skip_quota=False, **kwargs):
-        if url is None and \
-            (self.__access_token is None or
+        if  (self.__access_token is None or
              self.__expires_at <= datetime.utcnow()):
             self.refresh()
+        else:
+            LOGGER.info("access_token is still valid; not refreshing")
 
         if url is None and path:
             url = '{}{}'.format(self.BASE_URL, path)

tests/unittests/test_request_timeout.py

@@ -45,7 +45,9 @@ class TestTimeoutValue(unittest.TestCase):
     Test case to verify the timeout value is set as expected
     """
 
-    json = {"key": "value"}
+    json = {"key": "value",
+            "access_token": "None",
+            "expires_in": 10,}
 
     @parameterized.expand(
         [

tests/unittests/test_retry_logic.py

@@ -18,8 +18,9 @@ def raise_for_status(self):
 class TestRetryLogic(unittest.TestCase):
     """A set of unit tests to ensure that the retry logic work as expected"""
 
+    @patch("tap_outreach.client.OutreachClient.refresh")
     @patch("tap_outreach.client.requests.Session.request")
-    def test_retries_on_5XX(self, mock_session_request):
+    def test_retries_on_5XX(self, mock_session_request, _):
         """`OutreachClient.get()` calls a `request` method,to make a request to the API.
         We set the mock response status code to `500`.
 
@@ -45,9 +46,10 @@ def test_retries_on_5XX(self, mock_session_request):
         # 5 is the max tries specified in the tap
         self.assertEquals(5, mock_session_request.call_count)
 
+    @patch("tap_outreach.client.OutreachClient.refresh")
     @patch("tap_outreach.client.OutreachClient.sleep_for_reset_period")
     @patch("tap_outreach.client.requests.Session.request")
-    def test_retries_on_429(self, mock_session_request, mock_period):
+    def test_retries_on_429(self, mock_session_request, *args):
         """`OutreachClient.get()` calls a `request` method,to make a request to the API.
         We set the mock response status code to `429`. Checks the execution on reaching
         rate limit.
@@ -74,9 +76,10 @@ def test_retries_on_429(self, mock_session_request, mock_period):
         # 5 is the max tries specified in the tap
         self.assertEquals(5, mock_session_request.call_count)
 
+    @patch("tap_outreach.client.OutreachClient.refresh")
     @patch("tap_outreach.client.OutreachClient.sleep_for_reset_period")
     @patch("tap_outreach.client.requests.Session.request")
-    def test_retries_on_404(self, mock_session_request, mock_period):
+    def test_retries_on_404(self, mock_session_request, *args):
         """`OutreachClient.get()` calls a `request` method,to make a request to the API.
         We set the mock response status code to `404`. Checks the execution on reaching
         rate limit.