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 unicode issues in HttpError and BatchHttpRequest #77

Merged
merged 1 commit into from
Mar 26, 2015
Merged

Fix unicode issues in HttpError and BatchHttpRequest #77

merged 1 commit into from
Mar 26, 2015

Conversation

methane
Copy link
Contributor

@methane methane commented Mar 24, 2015

  1. HttpError parses http response by json.loads. It accepts only
    unicode on PY3 while response is bytes. So decode response first.

  2. BatchHttpRequest uses email.parser.FeedParser. It accepts only
    unicode on PY3, too. So encode parsed response to emulate normal
    http response.

@methane methane mentioned this pull request Mar 24, 2015
Closed
@nathanielmanistaatgoogle
Copy link
Contributor

(1) Squash commits?
(2) Write a longer, more detailed commit message? (http://chris.beams.io/posts/git-commit/#seven-rules)
(3) Why does Travis CI show test failure for this change?

pferate
pferate reviewed Mar 24, 2015
View reviewed changes
googleapiclient/errors.py
@@ -44,9 +44,9 @@ def _get_reason(self):
"""Calculate the reason for the error from the response content."""
reason = self.resp.reason
try:
data = json.loads(self.content)
data = json.loads(self.content.decode('utf-8'))

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

@pferate
Copy link
Contributor

pferate commented Mar 24, 2015

@nathanielmanistaatgoogle

  1. I was going to say the same, I've fought that battle a few times here recently
  2. I like that post, what do you think about linking it somewhere in the README?
  3. The wrong Error was being caught, I pointed it out in the line notes.

pferate
pferate reviewed Mar 24, 2015
View reviewed changes
googleapiclient/http.py
@@ -1252,11 +1252,10 @@ def _execute(self, http, order, requests):
if resp.status >= 300:
raise HttpError(resp, content, uri=self._batch_uri)

# Now break out the individual responses and store each one.

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

@pferate
Copy link
Contributor

pferate commented Mar 24, 2015

There are 3 ways that you have for getting to the decode/encode calls:

  1. try/except
  2. if not six.PY2:
  3. if isinstance(content, six.text_type):

Personally, I like "It's better to ask forgiveness than permission", so I would go with number 1. I'm not sure if @nathanielmanistaatgoogle has a preference.

@methane methane force-pushed the bytes-content branch 3 times, most recently from bd7641a to 226cf7b Compare March 24, 2015 19:00
@methane
Copy link
Contributor Author

methane commented Mar 24, 2015

(1) Squash commits?

Done

(2) Write a longer, more detailed commit message? (http://chris.beams.io/posts/git-commit/#seven-rules)

Done, bug I'm not a good English writer.

(3) Why does Travis CI show test failure for this change?

There were bugs remained relating BatchHttpRequest. I've fixed it.

@methane methane changed the title Mock and fake return content in bytes Fix unicode issues in HttpError and BatchHttpRequest Mar 24, 2015
methane
methane reviewed Mar 24, 2015
View reviewed changes
googleapiclient/http.py
@@ -1257,6 +1257,8 @@ def _execute(self, http, order, requests):

# Prepend with a content-type header so FeedParser can handle it.
header = 'content-type: %s\r\n\r\n' % resp['content-type']
if not six.PY2:
content = content.decode('utf-8')

This comment was marked as spam.

Sign in to view

@methane methane force-pushed the bytes-content branch 2 times, most recently from 9852b26 to e463e5e Compare March 25, 2015 02:25
@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Mar 25, 2015
nathanielmanistaatgoogle
nathanielmanistaatgoogle reviewed Mar 25, 2015
View reviewed changes
googleapiclient/errors.py
@@ -37,6 +37,9 @@ class HttpError(Error):
@util.positional(3)
def __init__(self, resp, content, uri=None):
self.resp = resp
if not isinstance(content, bytes):
# When see this exception in test, testdata may be wrong

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

@methane
Fix unicode issues in HttpError and BatchHttpRequest
0915761 
1) HttpError parses http response by json.loads. It accepts only
unicode on PY3 while response is bytes. So decode response first.

2) BatchHttpRequest uses email.parser.FeedParser. It accepts only
unicode on PY3, too. So encode parsed response to emulate normal
http response.
@pferate
Copy link
Contributor

pferate commented Mar 26, 2015

LGTM.

nathanielmanistaatgoogle added a commit that referenced this pull request Mar 26, 2015
@nathanielmanistaatgoogle
Merge pull request #77 from methane/bytes-content
b184ed6 
Fix unicode issues in HttpError and BatchHttpRequest
@nathanielmanistaatgoogle nathanielmanistaatgoogle merged commit b184ed6 into googleapis:master Mar 26, 2015
@nathanielmanistaatgoogle
Copy link
Contributor

Thanks for the contribution. :-)

@methane methane mentioned this pull request Apr 12, 2015
Closed
@pferate pferate mentioned this pull request Oct 12, 2015
Closed
akrherz pushed a commit to akrherz/google-api-python-client that referenced this pull request Apr 1, 2019
@craigcitro
Fix a bad six rename.
4de95cd 
Fixes googleapis#77.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@methane @nathanielmanistaatgoogle @pferate @googlebot