feat: asyncio http request logic and asynchronous credentials logic by anibadde · Pull Request #572 · googleapis/google-auth-library-python

anibadde · 2020-07-28T01:57:16Z

1st Async Auth Class PR

asyncio http request logic.
asynchronous credentials functionality.

google-cla · 2020-07-28T01:57:21Z

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.

crwilcox · 2020-07-28T18:13:40Z

+
+import abc
+
+import six


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

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

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): ...

crwilcox · 2020-07-28T18:20:10Z

+from google.auth.transport import requests
+
+
+_OAUTH_SCOPES = [


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

Fixed in the latest commit.

crwilcox · 2020-07-28T18:21:22Z

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


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?

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.

Fixed, changed to aiohttp_requests in all 3 PRs

crwilcox · 2020-07-28T18:22:10Z

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


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

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

crwilcox · 2020-07-28T18:22:27Z

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


Is this commented out code?

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

crwilcox · 2020-07-28T18:24:16Z

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


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

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.

crwilcox · 2020-07-28T18:27:26Z

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


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?

_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.

crwilcox · 2020-07-28T18:32:23Z

+    "aioresponses",
 ]
+
+TEST_DEPENDENCIES2 = [


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

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

…ext manager

…n class

…p_requests

crwilcox · 2020-07-28T20:44:41Z

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


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?

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

tritone

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

tritone · 2020-07-28T20:57:13Z

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


tseaver · 2020-07-29T13:11:56Z

+
+import abc
+
+import six


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): ...

tseaver · 2020-07-29T13:14:51Z

+
+"""Transport adapter for Async HTTP (aiohttp)."""
+
+from __future__ import absolute_import


Redundant under Python 3.x.

tseaver · 2020-07-29T13:17:16Z

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


Given Python 3 only, this can be simplified to:

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

tseaver · 2020-07-29T13:17:51Z

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


Likewise here:

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

tseaver · 2020-07-29T13:21:11Z

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


Given Python3-only, this can be just:

import http.client

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

* feat: asyncio http request logic and asynchronous credentials logic (#572) Co-authored-by: Anirudh Baddepudi <43104821+anibadde@users.noreply.github.com>

* 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>

feat: asyncio http request logic and asynchronous credentials log c

ef6d872

google-cla bot added the cla: no This human has *not* signed the Contributor License Agreement. label Jul 28, 2020

tritone requested review from andrewsg, busunkim96, crwilcox 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 Jul 28, 2020

View reviewed changes

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

a38e333

…ext manager

google-cla bot added cla: yes This human has signed the Contributor License Agreement. and removed cla: no This human has *not* signed the Contributor License Agreement. labels Jul 28, 2020

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 Jul 28, 2020

View reviewed changes

tritone reviewed Jul 28, 2020

View reviewed changes

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

c62dd1a

crwilcox approved these changes Jul 28, 2020

View reviewed changes

fix: cover async dependency

7080d14

tritone approved these changes Jul 28, 2020

View reviewed changes

tritone merged commit 8d0bca1 into googleapis:async Jul 28, 2020

tseaver reviewed Jul 29, 2020

View reviewed changes

tseaver mentioned this pull request Jul 30, 2020

feat: asyncio support and aiohttp transport #465

Closed

crwilcox mentioned this pull request Sep 25, 2020

feat: add asyncio based auth flow #612

Merged

crwilcox added a commit that referenced this pull request Sep 28, 2020

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>


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

		from __future__ import absolute_import

Conversation

anibadde commented Jul 28, 2020

Uh oh!

google-cla bot commented Jul 28, 2020

What to do if you already signed the CLA

Individual signers

Corporate signers

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

crwilcox Jul 28, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tritone left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

crwilcox Jul 28, 2020 •

edited

Loading