Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: Always return response from _do_http_method #27

Merged
merged 1 commit into from
Mar 14, 2024
Merged

Conversation

sarangj
Copy link
Contributor

@sarangj sarangj commented Mar 14, 2024

Noticed a bug where the first time we queried an API after a long time (in this case Sierra), we'd see a json decode error coming from the requests modules response.json(). Since we weren't calling it on our end, I dug through the trace a little and found that we call .json() in this library only when the token is expired and we do a refresh, which would explain the timing of the bug.

So simply, make sure to return the raw response object when we do a token refresh.

Here's an example stacktrace of the bug we are encountering:

File /ereadingserver/./app/main.py, line 186, in login
File /ereadingserver/./app/auth/login.py, line 62, in log_in_with_card
File /ereadingserver/./app/sierra.py, line 106, in patron_is_valid
File /usr/local/lib/python3.10/site-packages/nypl_py_utils/classes/oauth2_api_client.py, line 63, in post
File /usr/local/lib/python3.10/site-packages/nypl_py_utils/classes/oauth2_api_client.py, line 106, in _do_http_method
File /usr/local/lib/python3.10/site-packages/requests/models.py, line 975, in json

I updated a quick test that would repro this!

https://jira.nypl.org/browse/PJR-788

Noticed a bug where the *first* time we queried an API after a long time
(in this case Sierra), we'd see a json decode error coming from the
requests modules `response.json()`. Since we weren't calling it on our
end, I dug through the trace a little and found that we call `.json()`
in this library *only* when the token is expired and we do a refresh,
which would explain the timing of the bug.

So simply, make sure to return the raw response object when we do a
token refresh.

Here's an example stacktrace of the bug we are encountering:

File /ereadingserver/./app/main.py, line 186, in login
File /ereadingserver/./app/auth/login.py, line 62, in log_in_with_card
File /ereadingserver/./app/sierra.py, line 106, in patron_is_valid
File /usr/local/lib/python3.10/site-packages/nypl_py_utils/classes/oauth2_api_client.py, line 63, in post
File /usr/local/lib/python3.10/site-packages/nypl_py_utils/classes/oauth2_api_client.py, line 106, in _do_http_method
File /usr/local/lib/python3.10/site-packages/requests/models.py, line 975, in json
Copy link
Contributor

@aaronfriedman6 aaronfriedman6 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@sarangj sarangj merged commit ab33e6a into main Mar 14, 2024
2 checks passed
@sarangj sarangj deleted the fix_json_bug branch March 14, 2024 15:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants