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

Add .fetch(req) method to base OAuthenticator #415

Merged
merged 1 commit into from
Mar 10, 2021

Conversation

minrk
Copy link
Member

@minrk minrk commented Mar 10, 2021

consolidates:

  • http client instantiation
  • error handling and logging (many failed upstream requests are hard to debug because the upstream error message is not logged)
  • JSON response parsing

consolidates:

- httpclient instantiation
- error logging
- json deserialization

reduces duplicate code across implementations
@@ -39,128 +39,131 @@ def generic_client(client):
return client


async def test_generic(generic_client):
Copy link
Member Author

Choose a reason for hiding this comment

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

This diff is huge, but only because the http_client trait means the mock context is no longer necessary (it could always have been done with a fixture). These tests are just dedented with the context removed, in favor of the get_authenticator fixture.


resp_json = json.loads(resp.body.decode('utf8', 'replace'))

resp_json = await self.fetch(req)
Copy link
Member Author

Choose a reason for hiding this comment

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

~all of the changes are this: replace two-line request & json.loads with single self.fetch, removing AsyncHTTPClient and the associated imports.

Copy link
Member

@consideRatio consideRatio left a comment

Choose a reason for hiding this comment

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

This LGTM overall and is an excellent refactoring in my mind, but I raised two questions that should be considered before merge!

oauthenticator/gitlab.py Show resolved Hide resolved
oauthenticator/oauth2.py Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants