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

[Zygmunt Krynicki]

[Colin Watson (cjwatson)]

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

[Colin Watson (cjwatson)]

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?

[Zygmunt Krynicki (zyga)]

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.

[Dimitri John Ledkov (xnox)]

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.

[Dimitri John Ledkov (xnox)]

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.

[Colin Watson (cjwatson)]

This was superseded by https://code.launchpad.net/~xnox/launchpadlib/fix-1471927/+merge/264480, merged some years ago. I'll reject this one.

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>

=== modified file 'src/launchpadlib/credentials.py'
--- src/launchpadlib/credentials.py	2014-11-08 12:26:25 +0000
+++ src/launchpadlib/credentials.py	2015-07-06 18:32:23 +0000
@@ -247,6 +247,11 @@
     @classmethod
     def from_string(cls, query_string):
         """Create and return a new `AccessToken` from the given string."""
+        # Ensure the query_string is 'str' on both python2 and python3
+        # and not unicode on python2 and not bytes on python3
+        if (isinstance(query_string, bytes)
+                and not isinstance(query_string, str)):
+            query_string = query_string.decode('utf-8')
         params = cgi.parse_qs(query_string, keep_blank_values=False)
         key = params['oauth_token']
         assert len(key) == 1, (