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.

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

fix: session object was never used in aiohttp request (#700) #701

Merged
merged 5 commits into from Jun 3, 2021
Merged

fix: session object was never used in aiohttp request (#700) #701

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
dfb2440
fix: session object was never used in aiohttp request (#700)
greshilov Feb 16, 2021
556ca83
Merge branch 'master' into fix-request-session-arg
arithmetic1728 May 6, 2021
baf7d0c
fixup! fix: session object was never used in aiohttp request (#700)
greshilov May 6, 2021
a8137c9
fixup! fixup! fix: session object was never used in aiohttp request (…
greshilov Jun 2, 2021
0de9cd1
Merge branch 'master' into fix-request-session-arg
arithmetic1728 Jun 2, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
7 changes: 6 additions & 1 deletion google/auth/transport/_aiohttp_requests.py
View file
Open in desktop
Expand Up @@ -138,7 +138,12 @@ class Request(transport.Request):
"""

def __init__(self, session=None):
self.session = None
# TODO: Use auto_decompress property for aiohttp 3.7+
if session is not None and session._auto_decompress:
raise ValueError(
"Client sessions with auto_decompress=True are not supported."
Copy link
Contributor

Choose a reason for hiding this comment

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

why raise the error here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Modifying the session object provided by the user is inappropriate in this place, so we can't disable auto_decompress ourselves. Remaining options are:

  • Ignore the session object provided by the user and leave the self.session = None. Log error in console.
  • Raise the error to stop execution and warn user, that's something went wrong with one's session.

I chose the latter option because it's better to fail early in my opinion. A user may not notice that one's session object is ignored.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry I meant why auto_decompress=True are not supported. I am not familiar with aiohttp.

I think in this PR you also need to pass auto_decompress=False to https://github.com/googleapis/google-auth-library-python/blob/master/system_tests/system_tests_async/conftest.py#L29 to make the system test work.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry I meant why auto_decompress=True are not supported. I am not familiar with aiohttp.

I think in this PR you also need to pass auto_decompress=False to https://github.com/googleapis/google-auth-library-python/blob/master/system_tests/system_tests_async/conftest.py#L29 to make the system test work.

Ah, auto_decompress=True is supported by aiohttp, but this library prefers to do decompression on its own:

google-auth-library-python/google/auth/transport/_aiohttp_requests.py

Lines 80 to 88 in 04c870a

async def content(self):
# Load raw_content if necessary
await self.raw_content()
if self._is_compressed():
decoder = urllib3.response.MultiDecoder(
self._response.headers["Content-Encoding"]
)
decompressed = decoder.decompress(self._raw_content)
return decompressed

I've made required changes in conftest.py.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah I see. Thank you! The system test is still failing. I will take a look at why it failed.

)
self.session = session

async def __call__(
self,
Expand Down
14 changes: 10 additions & 4 deletions system_tests/system_tests_async/conftest.py
View file
Open in desktop
Expand Up @@ -26,9 +26,7 @@
from google.auth.transport import _aiohttp_requests as aiohttp_requests
from system_tests.system_tests_sync import conftest as sync_conftest

ASYNC_REQUESTS_SESSION = aiohttp.ClientSession()

ASYNC_REQUESTS_SESSION.verify = False
TOKEN_INFO_URL = "https://www.googleapis.com/oauth2/v3/tokeninfo"


Expand All @@ -49,10 +47,18 @@ def authorized_user_file():
"""The full path to a valid authorized user file."""
yield sync_conftest.AUTHORIZED_USER_FILE


@pytest.fixture
async def aiohttp_session():
async with aiohttp.ClientSession(auto_decompress=False) as session:
yield session


@pytest.fixture(params=["aiohttp"])
async def http_request(request):
async def http_request(request, aiohttp_session):
"""A transport.request object."""
yield aiohttp_requests.Request(ASYNC_REQUESTS_SESSION)
yield aiohttp_requests.Request(aiohttp_session)


@pytest.fixture
async def token_info(http_request):
Expand Down
15 changes: 12 additions & 3 deletions tests_async/transport/test_aiohttp_requests.py
View file
Open in desktop
Expand Up @@ -112,11 +112,18 @@ def make_request(self):
return aiohttp_requests.Request()

def make_with_parameter_request(self):
http = mock.create_autospec(aiohttp.ClientSession, instance=True)
http = aiohttp.ClientSession(auto_decompress=False)
return aiohttp_requests.Request(http)

def test_unsupported_session(self):
http = aiohttp.ClientSession(auto_decompress=True)
with pytest.raises(ValueError):
aiohttp_requests.Request(http)

def test_timeout(self):
http = mock.create_autospec(aiohttp.ClientSession, instance=True)
http = mock.create_autospec(
aiohttp.ClientSession, instance=True, _auto_decompress=False
)
request = aiohttp_requests.Request(http)
request(url="http://example.com", method="GET", timeout=5)

Expand All @@ -142,7 +149,9 @@ def test_constructor(self):
assert authed_session.credentials == mock.sentinel.credentials

def test_constructor_with_auth_request(self):
http = mock.create_autospec(aiohttp.ClientSession)
http = mock.create_autospec(
aiohttp.ClientSession, instance=True, _auto_decompress=False
)
auth_request = aiohttp_requests.Request(http)

authed_session = aiohttp_requests.AuthorizedSession(
Expand Down