FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

Switched headers to be normalized as strings, not bytes, in keeping with httplib2. by foozlevazquez · Pull Request #136 · googleapis/oauth2client · GitHub

This repository was archived by the owner on Jan 18, 2025. It is now read-only.
/ oauth2client Public archive

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

Closed
foozlevazquez wants to merge 1 commit into
googleapis:masterfrom
foozlevazquez:fix-header-strings-for-httplib2
Closed

foozlevazquez wants to merge 1 commit into
googleapis:masterfrom
foozlevazquez:fix-header-strings-for-httplib2

Conversation

Copy link
Copy Markdown

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

Copy link
Copy Markdown

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

Copy link
Copy Markdown
Contributor

isn't this the same issue as #104 ?

Copy link
Copy Markdown
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.

Comment thread tests/test_appengine.py

This comment was marked as spam.

dhermes commented Feb 21, 2015

Copy link
Copy Markdown
Contributor

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 commented Mar 1, 2015

Copy link
Copy Markdown

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 commented Mar 1, 2015

Copy link
Copy Markdown
Contributor

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

cganterh commented Mar 1, 2015

Copy link
Copy Markdown

@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?

Copy link
Copy Markdown
Contributor

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

cganterh commented Mar 1, 2015

Copy link
Copy Markdown

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.

Copy link
Copy Markdown
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 commented Mar 1, 2015

Copy link
Copy Markdown

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 commented Mar 2, 2015

Copy link
Copy Markdown
Contributor

@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 commented Mar 2, 2015

Copy link
Copy Markdown

@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.

Copy link
Copy Markdown
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 commented Mar 3, 2015

Copy link
Copy Markdown

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 commented Mar 3, 2015

Copy link
Copy Markdown

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

Copy link
Copy Markdown
Contributor

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

dhermes commented Aug 14, 2015

Copy link
Copy Markdown
Contributor

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

dhermes closed this Aug 14, 2015
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
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


Back | FazBrowse Home | New Git URL