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.

Sign up for GitHub

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

Jump to bottom

feat: asyncio http request logic and asynchronous credentials logic #572

Merged

tritone merged 9 commits into googleapis:async from anibadde:auth1

Jul 28, 2020

Contributor

anibadde commented Jul 28, 2020

1st Async Auth Class PR

asyncio http request logic.
asynchronous credentials functionality.


          feat: asyncio http request logic and asynchronous credentials log c

ef6d872

google-cla bot commented Jul 28, 2020

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here with @googlebot I signed it! and we'll verify it.

What to do if you already signed the CLA

Individual signers

It's possible we don't have your GitHub username or you're using a different email address on your commit. Check your existing CLA data and verify that your email is set on your git commits.

Corporate signers

Your company has a Point of Contact who decides which employees are authorized to participate. Ask your POC to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the Google project maintainer to go/cla#troubleshoot (Public version).
The email used to register you as an authorized contributor must be the email used for the Git commit. Check your existing CLA data and verify that your email is set on your git commits.
The email used to register you as an authorized contributor must also be attached to your GitHub account.

ℹ️ Googlers: Go here for more info.

google-cla bot added the cla: no label

tritone requested review from busunkim96, crwilcox, andrewsg and frankyn

July 28, 2020 15:04

frankyn changed the base branch from master to async

July 28, 2020 16:51

anibadde added 2 commits

July 28, 2020 12:39


          feat: added the private scope for Response class

06606ff


          feat: added docstring for Auth Session request method

cd57fad

crwilcox suggested changes

View reviewed changes

google/auth/credentials_async.py


		import abc

		import six

Contributor

crwilcox Jul 28, 2020

Should six be needed for this? this surface is only usable for 3.6+ correct?

Contributor Author

anibadde Jul 28, 2020

I used six to change the exception types (line 143, 147) so that we could raise our defined transport error.

Contributor

tseaver Jul 29, 2020

This module uses six only for the six.add_metaclass(abc.ABCMeta) class decorator. Given that it will only ever be imported under Python >= 3.6, it would be clearer to just use the direct Py3 syntax, e.g.:

class Credentials(credentials.Credentials, metaclass=abc.ABCMeta):
    ...

google/auth/transport/aiohttp_req.py Outdated

		from google.auth.transport import requests


		_OAUTH_SCOPES = [

Contributor

crwilcox Jul 28, 2020

are these shared with the other transport? Can we extract them to a common import to avoid duplication?

Contributor Author

anibadde Jul 28, 2020

Fixed in the latest commit.

google/auth/transport/aiohttp_req.py Outdated

+                  This class can be useful if you want to manually refresh a
+                  :class:`~google.auth.credentials.Credentials` instance::
+                      import google.auth.transport.aiohttp_req

Contributor

crwilcox Jul 28, 2020

I may have raised this before, but I am -1 on abbreviations. is there a motivation for naming this aiohttp_req? Instead of perhaps aiohttp or aiohttp_request?

Contributor Author

anibadde Jul 28, 2020

The motivation was the length, could change this to aiohttp_request though if that would be better. I'm worried about the name change to aiohttp just because of naming collision to the aiohttp library.

Contributor Author

anibadde Jul 28, 2020

Fixed, changed to aiohttp_requests in all 3 PRs

google/auth/transport/aiohttp_req.py Outdated

+                  :class:`~google.auth.credentials.Credentials` instance::
+                      import google.auth.transport.aiohttp_req
+                      import aiohttp

Contributor

crwilcox Jul 28, 2020

This import doesn't seem to be used in this sample. Is it required?

Contributor Author

anibadde Jul 28, 2020

Yes the aiohttp import is not required. Removed from the docstring.

google/auth/transport/aiohttp_req.py Outdated

+                  def __init__(self, session=None):
+                      """
+                      self.session = None

Contributor

crwilcox Jul 28, 2020

Is this commented out code?

Contributor Author

anibadde Jul 28, 2020

Made this change when switching the initialization of client session, deleted the commented out code now.

google/auth/transport/aiohttp_req.py Outdated

+                      Make an HTTP request using aiohttp.
+                      Args:
+                          url (str): The URI to be requested.

Contributor

crwilcox Jul 28, 2020

this may be carry-over, does the old surface use a param url and refer to it as a uri as well? If so we can leave this but it seems a little strange

Contributor Author

anibadde Jul 28, 2020

This was what the documentation was like in the old surface and therefore carried over on to here - I do agree the discrepancy is strange - it would seem that we want this to be URL. Changed in the current version.

google/auth/transport/aiohttp_req.py Outdated

+                      refresh_timeout=None,
+                      auth_request=None,
+                  ):
+                      super(AuthorizedSession, self).__init__()

Contributor

crwilcox Jul 28, 2020

are all of these vars unique to the implementation? It seems _loop and _refresh_lock are. Are any of these in the super() call and duplicate? If the args were passed?

Contributor Author

anibadde Jul 28, 2020

_loop and _refresh_lock are unique to this implementation. I think that as they are class variables with the self. syntax we wouldn't need to include them in the super() call, we could talk about this on our next call.

noxfile.py Outdated

		]

		TEST_DEPENDENCIES2 = [

Contributor

crwilcox Jul 28, 2020

can we share this list, composing test_dependencies from this list and adding the other two?

Contributor Author

anibadde Jul 28, 2020

Fixed in the latest commit to only have the 1 TEST_DEPENDENCIES list.


          fix: Changed initialization of client session to within an async cont…

a38e333

…ext manager

google-cla bot added cla: yes and removed cla: no labels

anibadde added 3 commits

July 28, 2020 14:44


          changed aiohttp_requests abbreviation for the async authorized sessio…

57d6d10

…n class


          fix: comments on PR regarding shared data between requests and aiohtt…

aa6ece2

…p_requests


          fix: fixed noxfile test dependency sharing

d88839b

crwilcox approved these changes

View reviewed changes

noxfile.py Outdated

@@ @@ -107,7 +91,7 @@ def unit(session): @@
               @nox.session(python=["2.7", "3.5"])
               def unit_prev_versions(session):
-                  session.install(*TEST_DEPENDENCIES2)
+                  session.install(*TEST_DEPENDENCIES[:-2])

Contributor

crwilcox Jul 28, 2020 •

edited

Loading

I am a little nervous about this, expecting to see folks add more deps :D.

can you make two lists one "dependencies" and one "additional_python_3_dependencies". And for python3 create a set with the two?

tritone Jul 28, 2020

+1

Contributor Author

anibadde Jul 28, 2020

Changed in the latest commit to have 2 lists and then a composition of both for the python3.6+ async tests.

tritone reviewed

View reviewed changes

tritone left a comment

This looks good to me aside from the minor issues raised by @crwilcox . Nice work!

noxfile.py Outdated

@@ @@ -107,7 +91,7 @@ def unit(session): @@
               @nox.session(python=["2.7", "3.5"])
               def unit_prev_versions(session):
-                  session.install(*TEST_DEPENDENCIES2)
+                  session.install(*TEST_DEPENDENCIES[:-2])

tritone Jul 28, 2020

+1


          fix: fixed the noxfile dependencies between sync and async unit tests

c62dd1a

crwilcox approved these changes

View reviewed changes


          fix: cover async dependency

7080d14

tritone approved these changes

View reviewed changes

tritone merged commit 8d0bca1 into googleapis:async

tseaver reviewed

View reviewed changes

google/auth/credentials_async.py


		import abc

		import six

Contributor

tseaver Jul 29, 2020

This module uses six only for the six.add_metaclass(abc.ABCMeta) class decorator. Given that it will only ever be imported under Python >= 3.6, it would be clearer to just use the direct Py3 syntax, e.g.:

class Credentials(credentials.Credentials, metaclass=abc.ABCMeta):
    ...

google/auth/transport/aiohttp_requests.py


		"""Transport adapter for Async HTTP (aiohttp)."""

		from __future__ import absolute_import

Contributor

tseaver Jul 29, 2020

Redundant under Python 3.x.

google/auth/transport/aiohttp_requests.py

+                      except aiohttp.ClientError as caught_exc:
+                          new_exc = exceptions.TransportError(caught_exc)
+                          six.raise_from(new_exc, caught_exc)

Contributor

tseaver Jul 29, 2020

Given Python 3 only, this can be simplified to:

        except aiohttp.ClientError as caught_exc:
            raise exceptions.TransportError(caught_exc) from caught_exc

google/auth/transport/aiohttp_requests.py

+                      except asyncio.TimeoutError as caught_exc:
+                          new_exc = exceptions.TransportError(caught_exc)
+                          six.raise_from(new_exc, caught_exc)

Contributor

tseaver Jul 29, 2020

Likewise here:

        except asyncio.TimeoutError as caught_exc:
            raise exceptions.TransportError(caught_exc) from caught_exc

tests_async/transport/async_compliance.py

+              import flask
+              import pytest
+              from pytest_localserver.http import WSGIServer
+              from six.moves import http_client

Contributor

tseaver Jul 29, 2020

Given Python3-only, this can be just:

import http.client

and then use http.client.OK, etc., below.

tseaver mentioned this pull request

feat: asyncio support and aiohttp transport #465

Closed

crwilcox mentioned this pull request

feat: add asyncio based auth flow #612

Merged

crwilcox added a commit that referenced this pull request


          feat: add asyncio based auth flow (#612)

7e15258

* feat: asyncio http request logic and asynchronous credentials logic  (#572)

Co-authored-by: Anirudh Baddepudi <43104821+anibadde@users.noreply.github.com>

busunkim96 added a commit that referenced this pull request


          chore: sync to master (#628)

cae69d5

* refactor: split 'with_quota_project' into separate base class (#561)

Co-authored-by: Tres Seaver <tseaver@palladion.com>

* fix: dummy commit to trigger a auto release (#597)

* chore: release 1.21.1 (#599)

* chore: updated CHANGELOG.md [ci skip]

* chore: updated setup.cfg [ci skip]

* chore: updated setup.py

Co-authored-by: release-please[bot] <55107282+release-please[bot]@users.noreply.github.com>

* fix: migrate signBlob to iamcredentials.googleapis.com (#600)

Migrate signBlob from iam.googleapis.com to iamcredentials.googleapis.com.

This API is deprecated and will be shutdown in one year.

This is used google.auth.iam.Signer.
Added a system_test to sanity check the implementation.

* chore: release 1.21.2 (#601)

Co-authored-by: release-please[bot] <55107282+release-please[bot]@users.noreply.github.com>

* fix: fix expiry for `to_json()` (#589)

* This patch for </issues/501> includes the following fixes:

- The access token is always set to `None`, so the fix involves using (the access) `token` from the saved JSON credentials file.
- For refresh needs, `expiry` also needs to be saved via `to_json()`.
    - DUMP: As `expiry` is a `datetime.datetime` object, serialize to `datetime.isoformat()` in the same [`oauth2client` format](https://github.com/googleapis/oauth2client/blob/master/oauth2client/client.py#L55) for consistency.
    - LOAD: Add code to restore `expiry` back to `datetime.datetime` object when imported.
    - LOAD: If `expiry` was unsaved, automatically set it as expired so refresh takes place.
- Minor `scopes` updates
    - DUMP: Add property for `scopes` so `to_json()` can grab it
    - LOAD: `scopes` may be saved as a string instead of a JSON array (Python list), so ensure it is Sequence[str] when imported.

* chore: add default CODEOWNERS (#609)

* chore: release 1.21.3 (#607)

* feat: add asyncio based auth flow (#612)

* feat: asyncio http request logic and asynchronous credentials logic  (#572)

Co-authored-by: Anirudh Baddepudi <43104821+anibadde@users.noreply.github.com>

* chore: release 1.22.0 (#615)

Co-authored-by: release-please[bot] <55107282+release-please[bot]@users.noreply.github.com>

* fix: move aiohttp to extra as it is currently internal surface (#619)

Fix #618. Removes aiohttp from required dependencies to lessen dependency tree for google-auth.

This will need to be looked at again as more folks use aiohttp and once the surfaces goes to public visibility.

* chore: release 1.22.1 (#620)

Co-authored-by: release-please[bot] <55107282+release-please[bot]@users.noreply.github.com>

* fix: remove checks for ancient versions of Cryptography (#596)

Refs #595 (comment) 

I see no point in checking whether someone is running a version of https://github.com/pyca/cryptography/ from 2014 that doesn't even compile against modern versions of OpenSSL anymore.

* chore: sync to master

Syncs to master.
Fixes broken unit tests in Python 3.6 and 3.7.
Aligns test_identity_pool.py with test_aws.py.

Co-authored-by: Bu Sun Kim <8822365+busunkim96@users.noreply.github.com>
Co-authored-by: Tres Seaver <tseaver@palladion.com>
Co-authored-by: arithmetic1728 <58957152+arithmetic1728@users.noreply.github.com>
Co-authored-by: release-please[bot] <55107282+release-please[bot]@users.noreply.github.com>
Co-authored-by: wesley chun <wescpy@gmail.com>
Co-authored-by: Christopher Wilcox <crwilcox@google.com>
Co-authored-by: Anirudh Baddepudi <43104821+anibadde@users.noreply.github.com>
Co-authored-by: Aarni Koskela <akx@iki.fi>

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

tseaver tseaver left review comments

tritone tritone approved these changes

crwilcox crwilcox approved these changes

busunkim96 Awaiting requested review from busunkim96

andrewsg Awaiting requested review from andrewsg

frankyn Awaiting requested review from frankyn

Labels