Skip to content
This repository has been archived by the owner on Nov 5, 2019. It is now read-only.

Switched headers to be normalized as strings, not bytes, in keeping with httplib2. #136

Conversation

foozlevazquez
Copy link

It appears that earlier contributors made the attempt to keep everything as bytes, mostly deducing from the test code. This doesn't work with the Python3 version of httplib2, and it is exactly this issue (bytes being passed to the Python3 version of httplib2 instead of str) that causes corrupt headers. httplib2 is very clear about what it expects.

Given the dependency on httplib2, in oauth2client, it makes sense to me that oauth2client should follow its lead and use whatever version of str for headers makes sense in the version of Python it is using.

  • Fixed header strings to use strings and not bytes.
  • Fixed tests to work with Python3

@coveralls
Copy link

Coverage Status

Changes Unknown when pulling f41eb74 on foozlevazquez:fix-header-strings-for-httplib2 into * on google:master*.

@craigcitro
Copy link
Contributor

isn't this the same issue as #104 ?

@foozlevazquez
Copy link
Author

@craigcitro #104 is the same root problem, but the conclusion that the bug lies in httplib2 is mistaken: httplib2 clearly expects strings and not bytes in headers, so this is not a bug with httplib2.

try:
import urlparse
except ImportError:
from urllib.parse import urlparse

This comment was marked as spam.

@dhermes
Copy link
Contributor

dhermes commented Feb 21, 2015

I would argue #104 is "on the right track" and that httplib2 should handle this.

Uses of str to mean bytes in Py2 and to mean unicode in Py3 is just asking for bugs.

@cganterh
Copy link

cganterh commented Mar 1, 2015

Should I expect this to be solved soon or should I find a workaround?
This is delaying my project. If I could help please say so.

@dhermes
Copy link
Contributor

dhermes commented Mar 1, 2015

@cganterh This is only needed for Python 3, if that helps.

@cganterh
Copy link

cganterh commented Mar 1, 2015

@dhermes my whole project is based on Python 3 and is strongly dependent on Python 3 libraries. Is it a good idea to fork oauth2client or httplib2 and apply the pull requests myself so I can go on while this issue is being fixed?

@nathanielmanistaatgoogle
Copy link
Contributor

@cganterh: Why fork httplib2? What change would you want to make to httplib2 that hasn't been accepted by the httplib2 maintainers?

@cganterh
Copy link

cganterh commented Mar 1, 2015

Correct me if im wrong, but I think that #136, #104 and jcgregorio/httplib2#291 are trying to solve the same issue. However I haven't read the pull requests in depth.
In jcgregorio/httplib2#291 they describe exactly the same error I'm getting in my code. The reason I posted here is that this pull request was more active than the others.
Sorry if I'm completely wrong, I'm using oauth2client for the first time.

@nathanielmanistaatgoogle
Copy link
Contributor

Okay, I suspected that they were all the same thing. It looks like getting a fix into httplib2 is blocked on writing test coverage for the fix, which is something that anyone can do.

@cganterh
Copy link

cganterh commented Mar 1, 2015

Yes, I know. But I think I'm not the most appropriate person to do this, since I have little experience on writing tests and it is my first time using the library. So I thought it was a bad idea to do it myself and maybe a better one was to just use some of the pull requests while the problem was actually being fixed. Now I think it doesn't make sense to fork the whole library rather than using the fork that the author of the pull request already made.

@dhermes
Copy link
Contributor

dhermes commented Mar 2, 2015

@anthmgoogle I don't think @jcgregorio is actively maintaining httplib2, so it may be best to just get the support in oauth2client. Maybe I am mistaken though.

@cganterh
Copy link

cganterh commented Mar 2, 2015

@dhermes I think it's a good idea, but I don't know what is stopping #136 and #104 to be merged. On the other hand, fixing httplib2 would be better because it may be an improvement for other libraries that depend on httplib2.

@nathanielmanistaatgoogle
Copy link
Contributor

All that's stopping httplib2 from having the change is that the change has to come with unit test coverage. That's not-at-all an unmaintained project. :-)

@cganterh
Copy link

cganterh commented Mar 3, 2015

So I finally decided to write the unit test for jcgregorio/httplib2#291.
@nathanielmanistaatgoogle you were right, it was actually easy! (If I did it right ...)

@cganterh
Copy link

cganterh commented Mar 3, 2015

My pull request was accepted. Someone should see if this solves #136 or #104.

@nathanielmanistaatgoogle
Copy link
Contributor

What action remains to be taken in this pull request now that the fix in httplib2 has been made?

@dhermes
Copy link
Contributor

dhermes commented Aug 14, 2015

@nathanielmanistaatgoogle I am going through this to see if there is anything we missed in #250

@dhermes dhermes closed this Aug 14, 2015
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants