diff --git a/CHANGELOG.md b/CHANGELOG.md index d5f26fd..cc441fa 100644 --- a/CHANGELOG.md +++ b/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 diff --git a/README.md b/README.md index 41d6d39..8f260da 100644 --- a/README.md +++ b/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`. diff --git a/pyproject.toml b/pyproject.toml index 4cb794e..6112671 100644 --- a/pyproject.toml +++ b/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="aaronfriedman@nypl.org" }, ] diff --git a/src/nypl_py_utils/classes/oauth2_api_client.py b/src/nypl_py_utils/classes/oauth2_api_client.py index 1baf92b..c551bf3 100644 --- a/src/nypl_py_utils/classes/oauth2_api_client.py +++ b/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): diff --git a/tests/test_oauth2_api_client.py b/tests/test_oauth2_api_client.py index 306309c..005c3ef 100644 --- a/tests/test_oauth2_api_client.py +++ b/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,