-
Notifications
You must be signed in to change notification settings - Fork 309
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: session object was never used in aiohttp request (#700) #701
fix: session object was never used in aiohttp request (#700) #701
Conversation
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 What to do if you already signed the CLAIndividual signers
Corporate signers
ℹ️ Googlers: Go here for more info. |
@googlebot I signed it! |
804c4d3
to
dfb2440
Compare
# 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." |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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 theself.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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
.
There was a problem hiding this comment.
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.
@arithmetic1728 I hope I found the problem.
I moved the session creation process to a separate fixture. Hope it helps. |
@greshilov Great! Thank you! |
Fix for #700