launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

Colin Watson has proposed merging lp:~cjwatson/launchpad/git-better-timeout into lp:launchpad.

Commit message:
Refactor GitHostingClient to use JSON-handling facilities from newer versions of requests and to use urlfetch.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~cjwatson/launchpad/git-better-timeout/+merge/292313

Refactor GitHostingClient to use JSON-handling facilities from newer versions of requests and to use urlfetch, now that the latter uses requests.  This lets it use the remaining request timeout in webapp contexts, which will be important for upcoming changes that need to talk to the hosting service from appservers.  It also means that urlfetch is somewhat more widely used, which is helpful for making sure it works well.

I added unit tests for GitHostingClient, which have been sorely lacking for some time.
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~cjwatson/launchpad/git-better-timeout into lp:launchpad.

=== modified file 'lib/lp/code/model/githosting.py'
--- lib/lp/code/model/githosting.py	2015-11-11 18:29:23 +0000
+++ lib/lp/code/model/githosting.py	2016-04-19 17:24:32 +0000
@@ -1,4 +1,4 @@
-# Copyright 2015 Canonical Ltd.  This software is licensed under the
+# Copyright 2015-2016 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Communication with the Git hosting service."""
@@ -8,7 +8,6 @@
     'GitHostingClient',
     ]
 
-import json
 from urlparse import urljoin
 
 import requests
@@ -21,6 +20,7 @@
     )
 from lp.code.interfaces.githosting import IGitHostingClient
 from lp.services.config import config
+from lp.services.timeout import urlfetch
 
 
 class HTTPResponseNotOK(Exception):
@@ -34,32 +34,14 @@
     def __init__(self):
         self.endpoint = config.codehosting.internal_git_api_endpoint
 
-    def _makeSession(self):
-        session = requests.Session()
-        session.trust_env = False
-        return session
-
-    @property
-    def timeout(self):
-        # XXX cjwatson 2015-03-01: The hardcoded timeout at least means that
-        # we don't lock tables indefinitely if the hosting service falls
-        # over, but is there some more robust way to do this?
-        return 30.0
-
     def _request(self, method, path, json_data=None, **kwargs):
-        session = self._makeSession()
-        if json_data is not None:
-            # XXX cjwatson 2015-03-01: Once we're on requests >= 2.4.2, we
-            # should just pass json through directly and drop the explicit
-            # Content-Type header.
-            kwargs.setdefault("headers", {})["Content-Type"] = (
-                "application/json")
-            kwargs["data"] = json.dumps(json_data)
-        response = getattr(session, method)(
-            urljoin(self.endpoint, path), timeout=self.timeout, **kwargs)
-        if (response.status_code // 100) != 2:
-            raise HTTPResponseNotOK(response.text)
-        elif response.content:
+        try:
+            response = urlfetch(
+                urljoin(self.endpoint, path), trust_env=False, method=method,
+                **kwargs)
+        except requests.HTTPError as e:
+            raise HTTPResponseNotOK(e.response.content)
+        if response.content:
             return response.json()
         else:
             return None
@@ -83,7 +65,7 @@
                 request = {"repo_path": path, "clone_from": clone_from}
             else:
                 request = {"repo_path": path}
-            self._post("/repo", json_data=request)
+            self._post("/repo", json=request)
         except Exception as e:
             raise GitRepositoryCreationFault(
                 "Failed to create Git repository: %s" % unicode(e))
@@ -99,7 +81,7 @@
     def setProperties(self, path, **props):
         """See `IGitHostingClient`."""
         try:
-            self._patch("/repo/%s" % path, json_data=props)
+            self._patch("/repo/%s" % path, json=props)
         except Exception as e:
             raise GitRepositoryScanFault(
                 "Failed to set properties of Git repository: %s" % unicode(e))
@@ -119,7 +101,7 @@
             if logger is not None:
                 logger.info("Requesting commit details for %s" % commit_oids)
             return self._post(
-                "/repo/%s/commits" % path, json_data={"commits": commit_oids})
+                "/repo/%s/commits" % path, json={"commits": commit_oids})
         except Exception as e:
             raise GitRepositoryScanFault(
                 "Failed to get commit details from Git repository: %s" %
@@ -151,7 +133,7 @@
                         path, sources, target))
             return self._post(
                 "/repo/%s/detect-merges/%s" % (path, target),
-                json_data={"sources": sources})
+                json={"sources": sources})
         except Exception as e:
             raise GitRepositoryScanFault(
                 "Failed to detect merges in Git repository: %s" % unicode(e))
@@ -161,7 +143,7 @@
         try:
             if logger is not None:
                 logger.info("Deleting repository %s" % path)
-            return self._delete("/repo/%s" % path)
+            self._delete("/repo/%s" % path)
         except Exception as e:
             raise GitRepositoryDeletionFault(
                 "Failed to delete Git repository: %s" % unicode(e))

=== added file 'lib/lp/code/model/tests/test_githosting.py'
--- lib/lp/code/model/tests/test_githosting.py	1970-01-01 00:00:00 +0000
+++ lib/lp/code/model/tests/test_githosting.py	2016-04-19 17:24:32 +0000
@@ -0,0 +1,184 @@
+# Copyright 2016 Canonical Ltd.  This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+"""Unit tests for `GitHostingClient`.
+
+We don't currently do integration testing against a real hosting service,
+but we at least check that we're sending the right requests.
+"""
+
+from __future__ import absolute_import, print_function, unicode_literals
+
+__metaclass__ = type
+
+from fixtures import MonkeyPatch
+import requests
+from zope.component import getUtility
+from zope.security.proxy import removeSecurityProxy
+
+from lp.code.errors import (
+    GitRepositoryCreationFault,
+    GitRepositoryDeletionFault,
+    GitRepositoryScanFault,
+    )
+from lp.code.interfaces.githosting import IGitHostingClient
+from lp.services.timeout import URLFetcher
+from lp.services.webapp.url import urlappend
+from lp.testing import TestCase
+from lp.testing.fakemethod import FakeMethod
+from lp.testing.layers import LaunchpadZopelessLayer
+
+
+class TestGitHostingClient(TestCase):
+
+    layer = LaunchpadZopelessLayer
+
+    def setUp(self):
+        super(TestGitHostingClient, self).setUp()
+        self.client = getUtility(IGitHostingClient)
+        self.endpoint = removeSecurityProxy(self.client).endpoint
+        self.session = URLFetcher()._makeSession()
+        response = requests.Response()
+        response.status_code = 200
+        response._content = b""
+        self.session.request = FakeMethod(result=response)
+        self.useFixture(MonkeyPatch(
+            "lp.services.timeout.URLFetcher._makeSession",
+            lambda _, trust_env=None: self.session))
+
+    def assertCalled(self, *args, **kwargs):
+        self.assertEqual([(args, kwargs)], self.session.request.calls)
+
+    def test_create(self):
+        self.client.create("123")
+        self.assertCalled(
+            url=urlappend(self.endpoint, "repo"), method="post",
+            json={"repo_path": "123"})
+
+    def test_create_clone_from(self):
+        self.client.create("123", clone_from="122")
+        self.assertCalled(
+            url=urlappend(self.endpoint, "repo"), method="post",
+            json={"repo_path": "123", "clone_from": "122"})
+
+    def test_create_failure(self):
+        self.session.request.result.status_code = 400
+        self.session.request.result._content = b"Bad request"
+        self.assertRaisesWithContent(
+            GitRepositoryCreationFault,
+            "Failed to create Git repository: Bad request",
+            self.client.create, "123")
+
+    def test_getProperties(self):
+        self.session.request.result._content = (
+            b'{"default_branch": "refs/heads/master"}')
+        props = self.client.getProperties("123")
+        self.assertEqual({"default_branch": "refs/heads/master"}, props)
+        self.assertCalled(
+            url=urlappend(self.endpoint, "repo/123"), method="get")
+
+    def test_getProperties_failure(self):
+        self.session.request.result.status_code = 400
+        self.session.request.result._content = b"Bad request"
+        self.assertRaisesWithContent(
+            GitRepositoryScanFault,
+            "Failed to get properties of Git repository: Bad request",
+            self.client.getProperties, "123")
+
+    def test_setProperties(self):
+        self.client.setProperties("123", default_branch="refs/heads/a")
+        self.assertCalled(
+            url=urlappend(self.endpoint, "repo/123"), method="patch",
+            json={"default_branch": "refs/heads/a"})
+
+    def test_setProperties_failure(self):
+        self.session.request.result.status_code = 400
+        self.session.request.result._content = b"Bad request"
+        self.assertRaisesWithContent(
+            GitRepositoryScanFault,
+            "Failed to set properties of Git repository: Bad request",
+            self.client.setProperties, "123", default_branch="refs/heads/a")
+
+    def test_getRefs(self):
+        self.session.request.result._content = b'{"refs/heads/master": {}}'
+        refs = self.client.getRefs("123")
+        self.assertEqual({"refs/heads/master": {}}, refs)
+        self.assertCalled(
+            url=urlappend(self.endpoint, "repo/123/refs"), method="get")
+
+    def test_getRefs_failure(self):
+        self.session.request.result.status_code = 400
+        self.session.request.result._content = b"Bad request"
+        self.assertRaisesWithContent(
+            GitRepositoryScanFault,
+            "Failed to get refs from Git repository: Bad request",
+            self.client.getRefs, "123")
+
+    def test_getCommits(self):
+        self.session.request.result._content = b'[{"sha1": "0"}]'
+        commits = self.client.getCommits("123", ["0"])
+        self.assertEqual([{"sha1": "0"}], commits)
+        self.assertCalled(
+            url=urlappend(self.endpoint, "repo/123/commits"), method="post",
+            json={"commits": ["0"]})
+
+    def test_getCommits_failure(self):
+        self.session.request.result.status_code = 400
+        self.session.request.result._content = b"Bad request"
+        self.assertRaisesWithContent(
+            GitRepositoryScanFault,
+            "Failed to get commit details from Git repository: Bad request",
+            self.client.getCommits, "123", ["0"])
+
+    def test_getMergeDiff(self):
+        self.session.request.result._content = b'{"patch": ""}'
+        diff = self.client.getMergeDiff("123", "a", "b")
+        self.assertEqual({"patch": ""}, diff)
+        self.assertCalled(
+            url=urlappend(self.endpoint, "repo/123/compare-merge/a:b"),
+            method="get")
+
+    def test_getMergeDiff_prerequisite(self):
+        self.session.request.result._content = b'{"patch": ""}'
+        diff = self.client.getMergeDiff("123", "a", "b", prerequisite="c")
+        self.assertEqual({"patch": ""}, diff)
+        expected_url = urlappend(
+            self.endpoint, "repo/123/compare-merge/a:b?sha1_prerequisite=c")
+        self.assertCalled(url=expected_url, method="get")
+
+    def test_getMergeDiff_failure(self):
+        self.session.request.result.status_code = 400
+        self.session.request.result._content = b"Bad request"
+        self.assertRaisesWithContent(
+            GitRepositoryScanFault,
+            "Failed to get merge diff from Git repository: Bad request",
+            self.client.getMergeDiff, "123", "a", "b")
+
+    def test_detectMerges(self):
+        self.session.request.result._content = b'{"b": "0"}'
+        merges = self.client.detectMerges("123", "a", ["b", "c"])
+        self.assertEqual({"b": "0"}, merges)
+        self.assertCalled(
+            url=urlappend(self.endpoint, "repo/123/detect-merges/a"),
+            method="post", json={"sources": ["b", "c"]})
+
+    def test_detectMerges_failure(self):
+        self.session.request.result.status_code = 400
+        self.session.request.result._content = b"Bad request"
+        self.assertRaisesWithContent(
+            GitRepositoryScanFault,
+            "Failed to detect merges in Git repository: Bad request",
+            self.client.detectMerges, "123", "a", ["b", "c"])
+
+    def test_delete(self):
+        self.client.delete("123")
+        self.assertCalled(
+            url=urlappend(self.endpoint, "repo/123"), method="delete")
+
+    def test_delete_failed(self):
+        self.session.request.result.status_code = 400
+        self.session.request.result._content = b"Bad request"
+        self.assertRaisesWithContent(
+            GitRepositoryDeletionFault,
+            "Failed to delete Git repository: Bad request",
+            self.client.delete, "123")

=== modified file 'lib/lp/services/timeout.py'
--- lib/lp/services/timeout.py	2016-04-19 12:16:01 +0000
+++ lib/lp/services/timeout.py	2016-04-19 17:24:32 +0000
@@ -243,14 +243,21 @@
 class URLFetcher:
     """Object fetching remote URLs with a time out."""
 
+    @staticmethod
+    def _makeSession(trust_env=None):
+        session = Session()
+        if trust_env is not None:
+            session.trust_env = trust_env
+        # Mount our custom adapters.
+        session.mount("https://";, CleanableHTTPAdapter())
+        session.mount("http://";, CleanableHTTPAdapter())
+        return session
+
     @with_timeout(cleanup='cleanup')
-    def fetch(self, url, **request_kwargs):
+    def fetch(self, url, trust_env=None, **request_kwargs):
         """Fetch the URL using a custom HTTP handler supporting timeout."""
         request_kwargs.setdefault("method", "GET")
-        self.session = Session()
-        # Mount our custom adapters.
-        self.session.mount("https://";, CleanableHTTPAdapter())
-        self.session.mount("http://";, CleanableHTTPAdapter())
+        self.session = self._makeSession(trust_env=trust_env)
         response = self.session.request(url=url, **request_kwargs)
         response.raise_for_status()
         # Make sure the content has been consumed before returning.
@@ -262,9 +269,9 @@
         self.session.close()
 
 
-def urlfetch(url, **request_kwargs):
+def urlfetch(url, trust_env=None, **request_kwargs):
     """Wrapper for `requests.get()` that times out."""
-    return URLFetcher().fetch(url, **request_kwargs)
+    return URLFetcher().fetch(url, trust_env=trust_env, **request_kwargs)
 
 
 class TransportWithTimeout(Transport):