Merge pull request #26 from NYPL/qa

add retries to oauth client

CHANGELOG.md

@@ -1,9 +1,12 @@
 # Changelog
+## v1.1.3 11/9/23
+- Finalize retry logic in Oauth2 Client
+
 ## v1.1.2
 - Update config_helper to accept list environment variables
 
 ## v1.1.0/v1.1.1
-- Add retries for empty responses in oauth2 client. This was added to address a known quirk in the Sierra API where this response is returned:
+- Add retries for empty responses in Oauth2 Client. This was added to address a known quirk in the Sierra API where this response is returned:
 ```
 > GET / HTTP/1.1
 > Host: ilsstaff.nypl.org

README.md

@@ -106,4 +106,4 @@ This repo uses the [Main-QA-Production](https://github.com/NYPL/engineering-gene
 - Deploy app to production on GitHub and confirm it works
 
 ## Deployment
-The utils repo is deployed as a PyPI package [here](https://pypi.org/project/nypl-py-utils/) and as a Test PyPI package for QA purposes [here](https://test.pypi.org/project/nypl-py-utils/). In order to be deployed, the version listed in `pyproject.toml` **must be updated**. To deploy to Test PyPI, create a new release in GitHub and tag it `qa-vX.X.X`. The GitHub Actions deploy-qa workflow will then build and publish the package. To deploy to production PyPI, create a release and tag it `production-vX.X.X`.
+The utils repo is deployed as a PyPI package [here](https://pypi.org/project/nypl-py-utils/) and as a Test PyPI package for QA purposes [here](https://test.pypi.org/project/nypl-py-utils/). In order to be deployed, the version listed in `pyproject.toml` **must be updated**. To deploy to Test PyPI, [create a new release](https://github.com/NYPL/python-utils/releases) in GitHub and tag it `qa-vX.X.X`. The GitHub Actions deploy-qa workflow will then build and publish the package. To deploy to production PyPI, create a release and tag it `production-vX.X.X`.

pyproject.toml

@@ -4,7 +4,7 @@ build-backend = "hatchling.build"
 
 [project]
 name = "nypl_py_utils"
-version = "1.1.2"
+version = "1.1.3"
 authors = [
   { name="Aaron Friedman", email="[email protected]" },
 ]

src/nypl_py_utils/classes/oauth2_api_client.py

@@ -37,22 +37,36 @@ def get(self, request_path, **kwargs):
         Issue an HTTP GET on the given request_path
         """
         resp = self._do_http_method('GET', request_path, **kwargs)
-        if resp.json() is None and self.with_retries is True:
-            retries = \
-                kwargs.get('retries', 0) + 1
-            if retries < 3:
-                self.logger.warning(
-                    f'Retrying get request due to empty response from\
-                         Oauth2 Client using path: {request_path}. \
-                         Retry #{retries}')
-                sleep(pow(2, retries - 1))
-                kwargs['retries'] = retries
-                resp = self.get(request_path, **kwargs)
-            else:
-                resp = Response()
-                resp.message = 'Oauth2 Client: Request failed after 3 \
-                        empty responses received from Oauth2 Client'
-                resp.status_code = 500
+        # This try/except block is to handle one of at least two possible
+        # Sierra server errors. One is an empty response, and another is a
+        # response with a 200 status code but response text in HTML declaring
+        # Status 500.
+        try:
+            resp.json()
+        except Exception:
+            # build default server error response
+            resp = Response()
+            resp.message = 'Oauth2 Client: Bad response from OauthClient'
+            resp.status_code = 500
+            self.logger.warning(f'Get request using path {request_path} \
+returned response text:\n{resp.text}')
+            # if client has specified that we want to retry failed requests and
+            # we haven't hit max retries
+            if self.with_retries is True:
+                retries = kwargs.get('retries', 0) + 1
+                if retries < 3:
+                    self.logger.warning(
+                        f'Retrying get request due to empty response from\
+Oauth2 Client using path: {request_path}. Retry #{retries}')
+                    sleep(pow(2, retries - 1))
+                    kwargs['retries'] = retries
+                    # try request again
+                    resp = self.get(request_path, **kwargs)
+                else:
+                    resp.message = 'Oauth2 Client: Request failed after 3 \
+                            empty responses received from Oauth2 Client'
+        # Return request. If retries returned real data, it will be here,
+        # otherwise it will be the default 500 response generated earlier.
         return resp
 
     def post(self, request_path, json, **kwargs):

tests/test_oauth2_api_client.py

@@ -3,7 +3,7 @@
 import json
 import pytest
 from requests_oauthlib import OAuth2Session
-from requests import HTTPError
+from requests import HTTPError, JSONDecodeError
 
 from nypl_py_utils.classes.oauth2_api_client import (Oauth2ApiClient,
                                                      Oauth2ApiClientError)
@@ -24,10 +24,11 @@ class MockEmptyResponse:
     def __init__(self, empty, status_code=None):
         self.status_code = status_code
         self.empty = empty
+        self.text = "error text"
 
     def json(self):
         if self.empty:
-            return None
+            raise JSONDecodeError
         else:
             return 'success'
 
@@ -158,18 +159,29 @@ def test_token_refresh_failure_raises_error(
         # Expect 1 initial token fetch, plus 3 retries:
         assert len(token_server_post.request_history) == 4
 
+    def test_bad_response_no_retries(self, requests_mock, test_instance,
+                                     mocker):
+        mocker.patch.object(test_instance, '_do_http_method',
+                            return_value=MockEmptyResponse(empty=True))
+        get_spy = mocker.spy(test_instance, 'get')
+        resp = test_instance.get('spaghetti')
+        assert get_spy.call_count == 1
+        assert resp.status_code == 500
+        assert resp.message == 'Oauth2 Client: Bad response from OauthClient'
+
     def test_http_retry_fail(self, requests_mock, test_instance_with_retries,
-                             token_server_post, mocker):
+                             mocker):
         mocker.patch.object(test_instance_with_retries, '_do_http_method',
                             return_value=MockEmptyResponse(empty=True))
         get_spy = mocker.spy(test_instance_with_retries, 'get')
         resp = test_instance_with_retries.get('spaghetti')
         assert get_spy.call_count == 3
         assert resp.status_code == 500
+        assert resp.message == 'Oauth2 Client: Request failed after 3 \
+                            empty responses received from Oauth2 Client'
 
     def test_http_retry_success(self, requests_mock,
-                                test_instance_with_retries,
-                                token_server_post, mocker):
+                                test_instance_with_retries, mocker):
         mocker.patch.object(test_instance_with_retries, '_do_http_method',
                             side_effect=[MockEmptyResponse(empty=True),
                                          MockEmptyResponse(empty=False,