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

Handle encoded response content in batch http request in Python 3 #250

Merged
merged 2 commits into from
Jan 22, 2019

Conversation

markflyhigh
Copy link
Contributor

Fix issue reported in #249.

The problem happens in Python 3 when execute a http request and encoded content is returned. This breaks email_parser.Parser().parsestr() call in batch.py since the function argument only accepted str (not bytes). There is a callback __ProcessHttpResponse that handles encoded response content but the call happens later.

@coveralls
Copy link

coveralls commented Dec 20, 2018

Coverage Status

Coverage increased (+0.01%) to 91.714% when pulling 28509a8 on markflyhigh:fix-py3-encoding into effc2e5 on google:master.

@markflyhigh
Copy link
Contributor Author

friendly ping.

@kevinli7
Copy link
Contributor

When I test this out manually in gcloud, this line in batch.py is failing in python2:
mime_response = parser.parsestr(header + content)

Also, I feel like this decoding should have some sort of switch to turn it on and off.

@markflyhigh
Copy link
Contributor Author

Interesting. The unit tests are passing in Python 2 and 3. Can you give more details how you test with gcloud?

Add a switch to this decoding sounds good to me. I think we can add a response_encoding field to BatchHttpRequest and BatchApiRequest, which also allow people to specify the encoding of the response content.

@markflyhigh
Copy link
Contributor Author

@kevinli7 PTAL

@kevinli7
Copy link
Contributor

LGTM. @houglum not sure if you guys use batch? Do you want to take a look as well?

@kevinli7 kevinli7 merged commit ecbcd3e into google:master Jan 22, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants