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

Added b prefix for string used in BaseHTTPRequestHandler wfile.write #122

Merged
merged 2 commits into from
Feb 12, 2015
Merged

Added b prefix for string used in BaseHTTPRequestHandler wfile.write #122

merged 2 commits into from
Feb 12, 2015

Conversation

kcs
Copy link
Contributor

@kcs kcs commented Jan 30, 2015

In Python 3 the BaseHTTPRequestHandler need bytes streams to write to the output, with simple strings the following exception occurs:

----------------------------------------
Exception happened during processing of request from ('127.0.0.1', 43494)
Traceback (most recent call last):
  File "/usr/lib/python3.4/socketserver.py", line 305, in _handle_request_noblock
    self.process_request(request, client_address)
  File "/usr/lib/python3.4/socketserver.py", line 331, in process_request
    self.finish_request(request, client_address)
  File "/usr/lib/python3.4/socketserver.py", line 344, in finish_request
    self.RequestHandlerClass(request, client_address, self)
  File "/usr/lib/python3.4/socketserver.py", line 665, in __init__
    self.handle()
  File "/usr/lib/python3.4/http/server.py", line 398, in handle
    self.handle_one_request()
  File "/usr/lib/python3.4/http/server.py", line 386, in handle_one_request
    method()
  File "/usr/local/lib/python3.4/dist-packages/oauth2client/tools.py", line 101, in do_GET
    self.wfile.write("<html><head><title>Authentication Status</title></head>")
  File "/usr/lib/python3.4/socket.py", line 391, in write
    return self._sock.send(b)
TypeError: 'str' does not support the buffer interface
----------------------------------------

Using b"" prefix for these strings is safe because the response is plain ASCII and it will also be compatible with Python 2.6 and 2.7

@nathanielmanistaatgoogle
Copy link
Contributor

Pleased to meet you and thank you for your change proposal! Two things:
(1) Have you signed the CLA (http://google.github.io/oauth2client/contributing.html#contributor-license-agreements)?
(2) Please add a test that fails without this change and passes with this change.

@kcs
Copy link
Contributor Author

kcs commented Jan 30, 2015

(1) yes, I did
(2) I just realized that there is no unit test for the tools module. Should I add one for it with just a test for the ClientRedirectHandler class?

@craigcitro
Copy link
Contributor

@cskertesz yeah, making a new file would be awesome. thanks!

@kcs
Copy link
Contributor Author

kcs commented Feb 1, 2015

I have added also a test for the ClientRedirectHandler class, which runs a redirect server, send a query with an access code and check if there is a valid response and if the stored access code matches the queried one.
Ran it with tox on python 2.7 and 3.4 (right now only have these), the test passes both with my change from previous commit, and fails on python 3.4 without it

nathanielmanistaatgoogle
nathanielmanistaatgoogle reviewed Feb 2, 2015
View reviewed changes
tests/test_tools.py
code = 'foo'
url = 'http://localhost:%i?code=%s' % (httpd.server_address[1], code)
t = threading.Thread(target = httpd.handle_request)
t.setDaemon(True)

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

@nathanielmanistaatgoogle
Copy link
Contributor

Thank you for the contribution!

nathanielmanistaatgoogle added a commit that referenced this pull request Feb 12, 2015
@nathanielmanistaatgoogle
Merge pull request #122 from cskertesz/master
c6b2318
@nathanielmanistaatgoogle nathanielmanistaatgoogle merged commit c6b2318 into googleapis:master Feb 12, 2015
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers
No reviews
Assignees
No one assigned
Labels
cla: yes
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@kcs @nathanielmanistaatgoogle @craigcitro @googlebot