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

json.loads error with Python 3.4 in gcloud.storage.connection.Connection.api_request #697

Closed
wants to merge 3 commits into from

Conversation

craigloftus
Copy link

When using Python 3 httplib2 returns request content as bytes but json.loads requires str.

It might be desirable to examine the content-type header returned by httplib2 to decided which encoding to use, although it is likely to always be utf-8?

…nnection.api_request is decoded to a string before being passed to json.loads
@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Mar 11, 2015
@craigloftus
Copy link
Author

The tests that have failed because they mock the response and assume a str will be returned by httplib2, I am using jcgregorio/httplib2@71a3b7b and see bytes (which is the documented behaviour).

Of course, I could update the PR to change the assumption in the tests, but somehow that feels like the wrong approach?

@tseaver
Copy link
Contributor

tseaver commented Mar 11, 2015

Please do update the tests to make the mocked payloads bytes.

@craigloftus
Copy link
Author

Darn. I didn't realise github would keep updating the PR - I am having to run the test suite with Travis (on my fork) as I couldn't get Tox to run locally.

@craigloftus
Copy link
Author

Going to kill this PR and come back when I have something that works.

@dhermes
Copy link
Contributor

dhermes commented Mar 11, 2015

@craigloftus I'm happy to help get tox working locally. If there are issues with it, we'd like to know so we can fix them.

@craigloftus
Copy link
Author

@dhermes Thanks. I have travis building against my fork, which has let me get somewhere.

After a couple of attempts I changed the mocked httplib2 responses to always deliver binary_type content (not headers), str for Python 2.6 and 2.7 and bytes for Python 3.4.

This travis build of my fork shows the remaining issue, which is actually the root cause of Issue #653; that the exception handling code is expecting str, but receiving bytes in Python 3.4.

@dhermes
Copy link
Contributor

dhermes commented Mar 11, 2015

@craigloftus Can you send error output from your issues running tox locally?

As for the remaining errors, it looks like you aren't JSON parsing the errors. It can be addressed by changing one line

    if isinstance(content, str):

to

    if isinstance(content, six.string_types):

UPDATE: I just realized six.string_types does not include bytes in PY3. Oh joy.

@craigloftus
Copy link
Author

@dhermes My issue is mainly that I have no idea how to use tox. Trying again this morning using tox with pyenv I got py26, py27 and py33 working, but py34 failed with an InvocationError that seemed to be associated with a problem with virtualenv: ImportError: No module named '_collections_abc'.

UPDATE: I just realized six.string_types does not include bytes in PY3. Oh joy.

Yeah. I think six.binary_type is the appropriate one to use. It maps to str in Python 2 (which is what the byte notation maps to in Python 2) and to bytes in Python 3. This issue is like a rabbit hole.

I now have a commit (craigloftus@e7ffadc7) which is passing on all python versions. It will probably be tomorrow before I get a chance to figure out how to rebase and squash my changes into a neat PR.

@dhermes
Copy link
Contributor

dhermes commented Mar 12, 2015

@craigloftus You can use tox just by running tox from the command line. The error you're seeing is not common, maybe you have an out of date version of tox?

Googling for your virtualenv snippet, it seems there was a bug at some point. I recommend doing the following and trying again:

[sudo] pip install --upgrade tox virtualenv

(The virtualenv upgrade is superfluous (I think) since tox will use custom ones for each environment it creates. Or maybe not?)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants