mozilla / django-browserid

Django application for adding BrowserID support.
Mozilla Public License 2.0
180 stars 80 forks source link

Fix json decoding error on python3 #221

Closed gilpinheiro closed 10 years ago

gilpinheiro commented 10 years ago

response.content is returned as a byte array, and json.loads will choke on it, leading to a failed log-in attempt, which you'll see in the django logs as:

Failed to parse remote verifier response: b'{"audience":"[redacted]","expires":1387148033218,"issuer":"gmail.login.persona.org","email":"[redacted]","status":"okay"}'

Osmose commented 10 years ago

Thanks for the PR!

The tests are failing because we mock out requests.post and return a str instead of a byte array.

However, I think the better fix would be to use Requests's built-in json method instead of calling json.loads ourselves.

Do you want to amend your PR? I'm not sure if changing to response.json() will require a change to the tests, either.

gilpinheiro commented 10 years ago

I've update the repo to match your fix. Let me know if there is something else I should do.

gilpinheiro commented 10 years ago

I just got the tests running, and I can see that there is still a failure. It seems to be caused by the test's mocked _request object not having a json() method available. I don't know Mock or requests well enough to fix it. I don't know if the right thing to do is to add the mock for the json function based on the regular json.loads, or to somehow actually use the function from the request package.

Osmose commented 10 years ago

IMO the right thing would be to mock out the json method so we avoid failures due to requests' implementation. Something like:

response = self._response()
response.json.return_value = {'fake': 'assertion'}

# Or even redefine _response to be more convenient:
def _response(self, data):
    mock = Mock(spec=requests.Response)
    mock.json.return_value = data
    return mock
mvasilkov commented 10 years ago

Since this wasn't merged, can anyone clarify: django-browserid does not work in Python 3?

Osmose commented 10 years ago

Just merged in e0674517b38b99f55924604b7ddd8b5a85aab258. Thanks, @gilpinheiro, and sorry for the wait!

As for whether this works in Python 3, I'd say yes, except in cases (like this one) where our tests are masking a Py3 issue. Which, as far as I know, is now not the case. So go for it! (And file issues if it fails, so that we may fix it :D).

mvasilkov commented 10 years ago

Yay, thanks for the reply, will try now.