Merge lp:~zyga/launchpadlib/fix-1471927 into lp:launchpadlib

Proposed by Zygmunt Krynicki
Status: Needs review
Proposed branch: lp:~zyga/launchpadlib/fix-1471927
Merge into: lp:launchpadlib
Diff against target: 15 lines (+5/-0)
1 file modified
src/launchpadlib/credentials.py (+5/-0)
To merge this branch: bzr merge lp:~zyga/launchpadlib/fix-1471927
Reviewer Review Type Date Requested Status
Colin Watson Needs Fixing
Review via email: mp+263953@code.launchpad.net
To post a comment you must log in.
Revision history for this message
Colin Watson (cjwatson) wrote :

You say that this is ensuring that it's not unicode on Python 2, but if you pass unicode in then it will try to decode it and (depending on the contents) either crash or leave it as unicode. Either way it's a bit inconsistent with the comment.

I haven't looked at the context further down, but can you just decode if it's bytes and drop the not str condition? You'll get unicode on Python 2, but it should tolerate that fine here; and it would be simpler code requiring a simpler comment (or probably none at all).

review: Needs Fixing
Revision history for this message
Colin Watson (cjwatson) wrote :

Er, wait, it won't try to decode it because of the first test, so that part of my comment is wrong, sorry. I'd still rather see the simpler version if it works - could you check that?

Revision history for this message
Zygmunt Krynicki (zyga) wrote :

Yep, certainly. I think nothing ever calls the unicode code path there
so in reality you either get str -> str (on python2) or you get bytes
-> str (on python3). The complex test was so that I don't introduce
str -> unicode (on python2) that people may not expect to handle.

On Tue, Jul 7, 2015 at 2:02 AM, Colin Watson <email address hidden> wrote:
> Er, wait, it won't try to decode it because of the first test, so that part of my comment is wrong, sorry. I'd still rather see the simpler version if it works - could you check that?
> --
> https://code.launchpad.net/~zyga/launchpadlib/fix-1471927/+merge/263953
> You are the owner of lp:~zyga/launchpadlib/fix-1471927.

Revision history for this message
Dimitri John Ledkov (xnox) wrote :

In a few other places in that file, the following is done:

        if not isinstance(value, unicode_type):
            value = value.decode('utf-8')

the assumption being that although over the wire things are bytes, we expect them to actually be unicode in most places and try to decode to native string straight away.

Wouldn't above pattern work here as well?

Will check later when reviewing other stuff.

Revision history for this message
Dimitri John Ledkov (xnox) wrote :

Right the query_string should be string here. Check for "bytes & ! string" is redundant, cause in python2 both are the same. Simple check for ! string should be sufficient then. Pushed new branch with a fix, i will request a review from zyga.

Unmerged revisions

141. By Zygmunt Krynicki on 2015-07-06

Fix AccessToken.from_string() crash on python3

This patch fixes a backtrace that happens when the non-anonymous login
API is used and the user doesn't have a pre-cached credentials.

Fixes: https://bugs.launchpad.net/launchpadlib/+bug/1471927
Signed-off-by: Zygmunt Krynicki <email address hidden>

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'src/launchpadlib/credentials.py'
2--- src/launchpadlib/credentials.py 2014-11-08 12:26:25 +0000
3+++ src/launchpadlib/credentials.py 2015-07-06 18:32:23 +0000
4@@ -247,6 +247,11 @@
5 @classmethod
6 def from_string(cls, query_string):
7 """Create and return a new `AccessToken` from the given string."""
8+ # Ensure the query_string is 'str' on both python2 and python3
9+ # and not unicode on python2 and not bytes on python3
10+ if (isinstance(query_string, bytes)
11+ and not isinstance(query_string, str)):
12+ query_string = query_string.decode('utf-8')
13 params = cgi.parse_qs(query_string, keep_blank_values=False)
14 key = params['oauth_token']
15 assert len(key) == 1, (

Subscribers

People subscribed via source and target branches