From 44b5408d48385b82351642ea1caf16e3548887ab Mon Sep 17 00:00:00 2001 From: Vera Kahn Date: Tue, 10 Oct 2023 09:59:28 -0400 Subject: [PATCH 1/8] update readme --- README.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/README.md b/README.md index e384ccf..5c52272 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`. From 63341c7c81551ec588b47c12d7df5c66ffcdbcf9 Mon Sep 17 00:00:00 2001 From: Vera Kahn Date: Thu, 9 Nov 2023 14:42:03 -0500 Subject: [PATCH 2/8] add constant --- src/nypl_py_utils/classes/oauth2_api_client.py | 17 ++++++++++------- 1 file changed, 10 insertions(+), 7 deletions(-) diff --git a/src/nypl_py_utils/classes/oauth2_api_client.py b/src/nypl_py_utils/classes/oauth2_api_client.py index 1baf92b..a159364 100644 --- a/src/nypl_py_utils/classes/oauth2_api_client.py +++ b/src/nypl_py_utils/classes/oauth2_api_client.py @@ -10,9 +10,9 @@ class Oauth2ApiClient: """ Client for interacting with an Oauth2 authenticated API such as NYPL's Platform API endpoints. Note with_retries is a boolean flag which - determines if empty get requests will be retried 3 times or until - they are successful. This is to address a known issue with the Sierra - API where empty responses are returned intermittently. + determines if empty get requests will be retried self.MAX_RETRIES times or + until they are successful. This is to address a known issue with the Sierra + API where empty and misformed responses are returned intermittently. """ def __init__(self, client_id=None, client_secret=None, base_url=None, @@ -32,6 +32,8 @@ def __init__(self, client_id=None, client_secret=None, base_url=None, self.with_retries = with_retries + self.MAX_RETRIES = 3 + def get(self, request_path, **kwargs): """ Issue an HTTP GET on the given request_path @@ -40,7 +42,7 @@ def get(self, request_path, **kwargs): if resp.json() is None and self.with_retries is True: retries = \ kwargs.get('retries', 0) + 1 - if retries < 3: + if retries < self.MAX_RETRIES: self.logger.warning( f'Retrying get request due to empty response from\ Oauth2 Client using path: {request_path}. \ @@ -50,8 +52,9 @@ def get(self, request_path, **kwargs): resp = self.get(request_path, **kwargs) else: resp = Response() - resp.message = 'Oauth2 Client: Request failed after 3 \ - empty responses received from Oauth2 Client' + resp.message = f'Oauth2 Client: Request failed after \ + {self.MAX_RETRIES} empty responses received \ + from Oauth2 Client' resp.status_code = 500 return resp @@ -98,7 +101,7 @@ def _do_http_method(self, method, request_path, **kwargs): # Raise error after 3 successive token refreshes kwargs['_do_http_method_token_refreshes'] = \ kwargs.get('_do_http_method_token_refreshes', 0) + 1 - if kwargs['_do_http_method_token_refreshes'] > 3: + if kwargs['_do_http_method_token_refreshes'] > self.MAX_RETRIES: raise Oauth2ApiClientError('Exhausted token refreshes') \ from None From 1aefa62a394049d229eb01fd95279c0191c146cb Mon Sep 17 00:00:00 2001 From: Vera Kahn Date: Thu, 9 Nov 2023 14:43:41 -0500 Subject: [PATCH 3/8] update changelog and version --- CHANGELOG.md | 5 ++++- pyproject.toml | 2 +- 2 files changed, 5 insertions(+), 2 deletions(-) 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/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" }, ] From 137e4687a4e6f960cb6c9410e62050303ede123a Mon Sep 17 00:00:00 2001 From: Vera Kahn Date: Thu, 9 Nov 2023 14:49:35 -0500 Subject: [PATCH 4/8] update retry logic --- .../classes/oauth2_api_client.py | 58 +++++++++++-------- tests/test_oauth2_api_client.py | 17 +++++- 2 files changed, 50 insertions(+), 25 deletions(-) diff --git a/src/nypl_py_utils/classes/oauth2_api_client.py b/src/nypl_py_utils/classes/oauth2_api_client.py index a159364..12086fc 100644 --- a/src/nypl_py_utils/classes/oauth2_api_client.py +++ b/src/nypl_py_utils/classes/oauth2_api_client.py @@ -10,9 +10,9 @@ class Oauth2ApiClient: """ Client for interacting with an Oauth2 authenticated API such as NYPL's Platform API endpoints. Note with_retries is a boolean flag which - determines if empty get requests will be retried self.MAX_RETRIES times or - until they are successful. This is to address a known issue with the Sierra - API where empty and misformed responses are returned intermittently. + determines if empty get requests will be retried 3 times or until + they are successful. This is to address a known issue with the Sierra + API where empty responses are returned intermittently. """ def __init__(self, client_id=None, client_secret=None, base_url=None, @@ -32,30 +32,42 @@ def __init__(self, client_id=None, client_secret=None, base_url=None, self.with_retries = with_retries - self.MAX_RETRIES = 3 - 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 < self.MAX_RETRIES: - 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 = f'Oauth2 Client: Request failed after \ - {self.MAX_RETRIES} empty responses received \ - from Oauth2 Client' - resp.status_code = 500 + self.logger.debug('spaghetti') + # 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'Bad response text: {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): @@ -101,7 +113,7 @@ def _do_http_method(self, method, request_path, **kwargs): # Raise error after 3 successive token refreshes kwargs['_do_http_method_token_refreshes'] = \ kwargs.get('_do_http_method_token_refreshes', 0) + 1 - if kwargs['_do_http_method_token_refreshes'] > self.MAX_RETRIES: + if kwargs['_do_http_method_token_refreshes'] > 3: raise Oauth2ApiClientError('Exhausted token refreshes') \ from None diff --git a/tests/test_oauth2_api_client.py b/tests/test_oauth2_api_client.py index 306309c..29dbd0b 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,6 +159,16 @@ 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, + token_server_post, 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.patch.object(test_instance_with_retries, '_do_http_method', @@ -166,6 +177,8 @@ def test_http_retry_fail(self, requests_mock, test_instance_with_retries, 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, From 472fa7c8634cb5d7f7d595df8ed523c9eaf2ec07 Mon Sep 17 00:00:00 2001 From: Vera Kahn Date: Thu, 9 Nov 2023 14:51:18 -0500 Subject: [PATCH 5/8] fix tests --- tests/test_oauth2_api_client.py | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/tests/test_oauth2_api_client.py b/tests/test_oauth2_api_client.py index 29dbd0b..005c3ef 100644 --- a/tests/test_oauth2_api_client.py +++ b/tests/test_oauth2_api_client.py @@ -160,7 +160,7 @@ def test_token_refresh_failure_raises_error( assert len(token_server_post.request_history) == 4 def test_bad_response_no_retries(self, requests_mock, test_instance, - token_server_post, mocker): + mocker): mocker.patch.object(test_instance, '_do_http_method', return_value=MockEmptyResponse(empty=True)) get_spy = mocker.spy(test_instance, 'get') @@ -170,7 +170,7 @@ def test_bad_response_no_retries(self, requests_mock, test_instance, 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') @@ -181,8 +181,7 @@ def test_http_retry_fail(self, requests_mock, test_instance_with_retries, 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, From 95458d34302796429f9436a847fa6d05dfa5c95a Mon Sep 17 00:00:00 2001 From: Vera Kahn Date: Thu, 9 Nov 2023 14:52:59 -0500 Subject: [PATCH 6/8] rm print --- src/nypl_py_utils/classes/oauth2_api_client.py | 1 - 1 file changed, 1 deletion(-) diff --git a/src/nypl_py_utils/classes/oauth2_api_client.py b/src/nypl_py_utils/classes/oauth2_api_client.py index 12086fc..e5e2bca 100644 --- a/src/nypl_py_utils/classes/oauth2_api_client.py +++ b/src/nypl_py_utils/classes/oauth2_api_client.py @@ -37,7 +37,6 @@ def get(self, request_path, **kwargs): Issue an HTTP GET on the given request_path """ resp = self._do_http_method('GET', request_path, **kwargs) - self.logger.debug('spaghetti') # 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 From 2f75c1fb795cfe9050ff5ce21d2e18b00bdac3d6 Mon Sep 17 00:00:00 2001 From: Vera Kahn Date: Tue, 14 Nov 2023 09:57:46 -0500 Subject: [PATCH 7/8] add request path to log --- src/nypl_py_utils/classes/oauth2_api_client.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/nypl_py_utils/classes/oauth2_api_client.py b/src/nypl_py_utils/classes/oauth2_api_client.py index e5e2bca..e502136 100644 --- a/src/nypl_py_utils/classes/oauth2_api_client.py +++ b/src/nypl_py_utils/classes/oauth2_api_client.py @@ -48,7 +48,8 @@ def get(self, request_path, **kwargs): resp = Response() resp.message = 'Oauth2 Client: Bad response from OauthClient' resp.status_code = 500 - self.logger.warning(f'Bad response text: {resp.text}') + 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: From f8b08b823610631325f47bfaf4bf964eb9b8ce69 Mon Sep 17 00:00:00 2001 From: Vera Kahn Date: Mon, 20 Nov 2023 10:18:55 -0500 Subject: [PATCH 8/8] fix logging spacing --- src/nypl_py_utils/classes/oauth2_api_client.py | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/src/nypl_py_utils/classes/oauth2_api_client.py b/src/nypl_py_utils/classes/oauth2_api_client.py index e502136..c551bf3 100644 --- a/src/nypl_py_utils/classes/oauth2_api_client.py +++ b/src/nypl_py_utils/classes/oauth2_api_client.py @@ -49,7 +49,7 @@ def get(self, request_path, **kwargs): 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}') +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: @@ -57,8 +57,7 @@ def get(self, request_path, **kwargs): if retries < 3: self.logger.warning( f'Retrying get request due to empty response from\ - Oauth2 Client using path: {request_path}. \ - Retry #{retries}') +Oauth2 Client using path: {request_path}. Retry #{retries}') sleep(pow(2, retries - 1)) kwargs['retries'] = retries # try request again