Merge lp:~sjakthol/software-properties/fix-1111291 into lp:software-properties

[Sami Jaktholm]

* softwareproperties/ppa.py
  - Use PycURL only with python2, not if urllib raises an unexpected exception
    in python3. Fixes LP: #1111291.
  - Catch http.client.HTTPExceptions from urlopen and raise a PPAException to
    show a formatted error message instead of traceback.

This is hard to test as the code is only run if urllib fails. Luckily the changes are pretty straightforward.

[Michael Vogt (mvo)]

On Sat, Apr 06, 2013 at 12:30:28PM -0000, Sami Jaktholm wrote:
> Sami Jaktholm has proposed merging lp:~sjakthol/software-properties/fix-1111291 into lp:software-properties.
>
> Commit message:
> * softwareproperties/ppa.py:
> - Convert bytes to string in CurlCallback.body_callback. Testcase added to
> tests/test_lp.py. Fixes LP: #1111291.

Hi Sami, thanks for your branch.

[..]
> This is hard to test as the code is only run if urllib fails. Luckily the changes are pretty straightforward.

The change itself it looks fine, but I'm curious why its needed? It
seems like the fallback code is only intended for python2 and the fix
is only needed when the pycurl code is run with py3.

We should probably rewrite this function to make that clear, attached
is a patch that shows what I have in mind (not tested, I have no
network connection while writing this).

So it would be great to look a bit deeper and figure out what is
causing the fallback code to get activated in the first place. The
current code with the big try/except is hiding the real problem it
seems.

Cheers,
 Michael

[lp:~sjakthol/software-properties/fix-1111291]

844. By Sami Jaktholm on 2013-04-08

Don't use pycurl when running with python3.

[Sami Jaktholm (sjakthol)]

The related bug report shows that the fallback code is activated due to invalid response (http.client.BadStatusLine: ''). Looking at the urrlib.request code around the exception it only seems to convert socket.error to URLError but exceptions from httplib propagate all the way back to clients. These exceptions then trigger the pycurl code that fails in python3.

But anyways, I'll change this to fit your suggestion (it's way cleaner than the current try/except mess).

[Michael Vogt (mvo)]

On Mon, Apr 08, 2013 at 12:17:22PM -0000, Sami Jaktholm wrote:
> The related bug report shows that the fallback code is activated due to invalid response (http.client.BadStatusLine: ''). Looking at the urrlib.request code around the exception it only seems to convert socket.error to URLError but exceptions from httplib propagate all the way back to clients. These exceptions then trigger the pycurl code that fails in python3.
>
> But anyways, I'll change this to fit your suggestion (it's way cleaner than the current try/except mess).

Great! Thanks a lot and let me know once the merge proposal is updated :)

[Michael Vogt (mvo)]

On Mon, Apr 08, 2013 at 12:17:22PM -0000, Sami Jaktholm wrote:
> The related bug report shows that the fallback code is activated due to invalid response (http.client.BadStatusLine: ''). Looking at the urrlib.request code around the exception it only seems to convert socket.error to URLError but exceptions from httplib propagate all the way back to clients. These exceptions then trigger the pycurl code that fails in python3.
>
> But anyways, I'll change this to fit your suggestion (it's way cleaner than the current try/except mess).

Great! Thanks a lot and let me know once the merge proposal is updated :)

[lp:~sjakthol/software-properties/fix-1111291]

845. By Sami Jaktholm on 2013-04-09

Catch http.client.HTTPExceptions from urlopen and raise a PPAException to show a formatted error message instead of a traceback.

[Sami Jaktholm (sjakthol)]

Changed the python3 code to also catch exceptions from http.client as they are mostly caused by connection issues. Now a "check your connection" message is shown instead of a long traceback (this was the case where the pycurl code was triggered). Other exceptions are not caught.

Also updated the commit message so this should be ready to go.

[Michael Vogt (mvo)]

Thanks Sami! I like this change, +1 from me.

review: Approve

[Sami Jaktholm (sjakthol)]

Ping. Still waiting to be merged...

[Brian Murray (brian-murray)]

I've gone ahead and merged this, thanks!

=== modified file 'softwareproperties/ppa.py'
--- softwareproperties/ppa.py	2013-02-27 20:15:21 +0000
+++ softwareproperties/ppa.py	2013-04-09 07:26:20 +0000
@@ -35,7 +35,10 @@
     import urllib.request
     from urllib.error import URLError
     import urllib.parse
+    from http.client import HTTPException
+    NEED_PYCURL = False
 except ImportError:
+    NEED_PYCURL = True
     import pycurl
 
 DEFAULT_KEYSERVER = "hkp://keyserver.ubuntu.com:80/"
@@ -92,34 +95,43 @@
 
 def get_ppa_info_from_lp(owner_name, ppa_name):
     lp_url = LAUNCHPAD_PPA_API % (owner_name, ppa_name)
-    try:
-        try:
-            request = urllib.request.Request(str(lp_url), headers={"Accept":" application/json"})
-            lp_page = urllib.request.urlopen(request, cafile=LAUNCHPAD_PPA_CERT)
-            json_data = lp_page.read().decode("utf-8", "strict")
-        except URLError as e:
-            raise PPAException("Error reading %s: %s" % (lp_url, e.reason), e)
-    except PPAException:
-        raise
-    except:
-        import pycurl
-        try:
-            callback = CurlCallback()
-            curl = pycurl.Curl()
-            curl.setopt(pycurl.SSL_VERIFYPEER, 1)
-            curl.setopt(pycurl.SSL_VERIFYHOST, 2)
-            curl.setopt(pycurl.WRITEFUNCTION, callback.body_callback)
-            if LAUNCHPAD_PPA_CERT:
-                curl.setopt(pycurl.CAINFO, LAUNCHPAD_PPA_CERT)
-            curl.setopt(pycurl.URL, str(lp_url))
-            curl.setopt(pycurl.HTTPHEADER, ["Accept: application/json"])
-            curl.perform()
-            curl.close()
-            json_data = callback.contents
-        except pycurl.error as e:
-            raise PPAException("Error reading %s: %s" % (lp_url, e), e)
-    return json.loads(json_data)
-
+    if NEED_PYCURL:
+        # python2 has no cert verification so we need pycurl
+        return _get_https_content_pycurl(lp_url)
+    else:
+        # python3 has cert verification so we can use the buildin urllib
+        return _get_https_content_py3(lp_url)
+
+def _get_https_content_py3(lp_url):
+    try:
+        request = urllib.request.Request(str(lp_url), headers={"Accept":" application/json"})
+        lp_page = urllib.request.urlopen(request, cafile=LAUNCHPAD_PPA_CERT)
+        json_data = lp_page.read().decode("utf-8", "strict")
+    except (URLError, HTTPException) as e:
+        # HTTPException doesn't have a reason but might have a string
+        # representation
+        reason = hasattr(e, "reason") and e.reason or e
+        raise PPAException("Error reading %s: %s" % (lp_url, reason), e)
+    return json.loads(json_data)
+
+def _get_https_content_pycurl(lp_url):
+    # this is the fallback code for python2
+    try:
+        callback = CurlCallback()
+        curl = pycurl.Curl()
+        curl.setopt(pycurl.SSL_VERIFYPEER, 1)
+        curl.setopt(pycurl.SSL_VERIFYHOST, 2)
+        curl.setopt(pycurl.WRITEFUNCTION, callback.body_callback)
+        if LAUNCHPAD_PPA_CERT:
+            curl.setopt(pycurl.CAINFO, LAUNCHPAD_PPA_CERT)
+        curl.setopt(pycurl.URL, str(lp_url))
+        curl.setopt(pycurl.HTTPHEADER, ["Accept: application/json"])
+        curl.perform()
+        curl.close()
+        json_data = callback.contents
+    except pycurl.error as e:
+        raise PPAException("Error reading %s: %s" % (lp_url, e), e)
+    return json.loads(json_data)
 
 def verify_keyid_is_v4(signing_key_fingerprint):
     """Verify that the keyid is a v4 fingerprint with at least 160bit"""