Merge lp:~cjwatson/launchpad/explicit-proxy-rt into lp:launchpad

[Colin Watson]

Convert the RequestTracker external bug tracker to urlfetch.

We need a bit of care to persist cookies in tests due to a bug in responses.

[William Grant (wgrant)]

review: Approve (code)

=== modified file 'lib/lp/bugs/doc/externalbugtracker-rt.txt'
--- lib/lp/bugs/doc/externalbugtracker-rt.txt	2012-12-26 01:32:19 +0000
+++ lib/lp/bugs/doc/externalbugtracker-rt.txt	2018-06-23 00:10:25 +0000
@@ -21,23 +21,6 @@
     ...     RequestTracker('http://example.com/'))
     True
 
-The RequestTracker class offers an _opener property, an instance of
-urllib2.OpenerDirector which will handle cookies and so allow the
-RequestTracker instance to work correctly with RT cookies.
-
-We can demonstrate this by creating a test class which contains a stub
-method for RequestTracker._logIn().
-
-    >>> class NoLogInRequestTracker(RequestTracker):
-    ...     def _logIn(self, opener):
-    ...         """This method does nothing but say it's been called."""
-    ...         print "_logIn() has been called."
-
-    >>> request_tracker = NoLogInRequestTracker('http://example.com/')
-    >>> request_tracker._opener
-    _logIn() has been called.
-    <urllib2.OpenerDirector...>
-
 
 == Authentication Credentials ==
 
@@ -108,13 +91,32 @@
 of these tests, which allows us to not rely on a working network
 connection.
 
-    >>> from lp.bugs.tests.externalbugtracker import (
-    ...     TestRequestTracker)
+    >>> from lp.bugs.tests.externalbugtracker import TestRequestTracker
     >>> rt = TestRequestTracker('http://example.com/')
-    >>> rt.initializeRemoteBugDB([1585, 1586, 1587, 1588, 1589])
+    >>> with rt.responses(trace_calls=True):
+    ...     rt.initializeRemoteBugDB([1585, 1586, 1587, 1588, 1589])
+    GET http://example.com/?...
+    GET http://example.com/REST/1.0/search/ticket/?...
     >>> sorted(rt.bugs.keys())
     [1585, 1586, 1587, 1588, 1589]
 
+The first request logs into RT and saves the resulting cookie.
+
+    >>> def print_cookie_jar(jar):
+    ...     for name, value in sorted(jar.items()):
+    ...         print('%s=%s' % (name, value))
+
+    >>> print_cookie_jar(rt._cookie_jar)
+    rt_credentials=guest:guest
+
+Subsequent requests use this.
+
+    >>> with rt.responses(trace_calls=True) as requests_mock:
+    ...     rt.initializeRemoteBugDB([1585, 1586, 1587, 1588, 1589])
+    ...     print(requests_mock.calls[0].request.headers['Cookie'])
+    rt_credentials=guest:guest
+    GET http://example.com/REST/1.0/search/ticket/?...
+
 
 == Export Methods ==
 
@@ -126,9 +128,9 @@
     >>> rt.batch_query_threshold
     1
 
-    >>> rt.trace_calls = True
-    >>> rt.initializeRemoteBugDB([1585])
-    CALLED urlopen('REST/1.0/ticket/1585/show')
+    >>> with rt.responses(trace_calls=True):
+    ...     rt.initializeRemoteBugDB([1585])
+    GET http://example.com/REST/1.0/ticket/1585/show
 
     >>> rt.bugs.keys()
     [1585]
@@ -136,8 +138,9 @@
 If there are more than batch_query_threshold bugs to update then they are
 fetched as a batch:
 
-    >>> rt.initializeRemoteBugDB([1585, 1586, 1587, 1588, 1589])
-    CALLED urlopen('REST/1.0/search/ticket/')
+    >>> with rt.responses(trace_calls=True):
+    ...     rt.initializeRemoteBugDB([1585, 1586, 1587, 1588, 1589])
+    GET http://example.com/REST/1.0/search/ticket/?...
 
     >>> sorted(rt.bugs.keys())
     [1585, 1586, 1587, 1588, 1589]
@@ -146,19 +149,19 @@
 BugTrackerConnectError will be raised. We can demonstrate this by making
 our test RT instance simulate such a situation.
 
-    >>> rt.simulate_bad_response = True
-    >>> rt.initializeRemoteBugDB([1585])
+    >>> with rt.responses(bad=True):
+    ...     rt.initializeRemoteBugDB([1585])
     Traceback (most recent call last):
       ...
     BugTrackerConnectError...
 
 This can also be demonstrated for importing bugs as a batch:
 
-    >>> rt.initializeRemoteBugDB([1585, 1586, 1587, 1588, 1589])
+    >>> with rt.responses(bad=True):
+    ...     rt.initializeRemoteBugDB([1585, 1586, 1587, 1588, 1589])
     Traceback (most recent call last):
       ...
     BugTrackerConnectError...
-    >>> rt.simulate_bad_response = False
 
 == Updating Bug Watches ==
 
@@ -198,7 +201,9 @@
     >>> bug_watch_updater = CheckwatchesMaster(
     ...     txn, logger=FakeLogger())
     >>> rt = TestRequestTracker(example_bug_tracker.baseurl)
-    >>> bug_watch_updater.updateBugWatches(rt, example_bug_tracker.watches)
+    >>> with rt.responses():
+    ...     bug_watch_updater.updateBugWatches(
+    ...         rt, example_bug_tracker.watches)
     INFO Updating 1 watches for 1 bugs on http://bugs.some.where
 
     >>> print_bugwatches(example_bug_tracker.watches)
@@ -225,10 +230,11 @@
     ...         bugtracker=example_bug_tracker,
     ...         remotebug=str(remote_bug_id))
 
-    >>> rt.trace_calls = True
-    >>> bug_watch_updater.updateBugWatches(rt, example_bug_tracker.watches)
+    >>> with rt.responses(trace_calls=True):
+    ...     bug_watch_updater.updateBugWatches(
+    ...         rt, example_bug_tracker.watches)
     INFO Updating 5 watches for 5 bugs on http://bugs.some.where
-    CALLED urlopen(u'REST/1.0/search/ticket/')
+    GET http://bugs.some.where/REST/1.0/search/ticket/?...
 
 The bug statuses have now been imported from the Example.com bug
 tracker, so the bug watches should now have valid Launchpad bug

=== modified file 'lib/lp/bugs/externalbugtracker/rt.py'
--- lib/lp/bugs/externalbugtracker/rt.py	2013-04-22 06:43:16 +0000
+++ lib/lp/bugs/externalbugtracker/rt.py	2018-06-23 00:10:25 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2018 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """RT ExternalBugTracker Utility."""
@@ -7,13 +7,15 @@
 __all__ = ['RequestTracker']
 
 import email
-import urllib
-import urllib2
+import re
+
+import requests
+from requests.cookies import RequestsCookieJar
 
 from lp.bugs.externalbugtracker import (
     BugNotFound,
     BugTrackerConnectError,
-    ExternalBugTracker,
+    ExternalBugTrackerRequests,
     InvalidBugId,
     LookupTree,
     UnknownRemoteStatusError,
@@ -24,18 +26,24 @@
     )
 from lp.bugs.interfaces.externalbugtracker import UNKNOWN_REMOTE_IMPORTANCE
 from lp.services.config import config
-from lp.services.database.isolation import ensure_no_transaction
-from lp.services.propertycache import cachedproperty
 from lp.services.webapp.url import urlparse
 
 
-class RequestTracker(ExternalBugTracker):
+class RequestTracker(ExternalBugTrackerRequests):
     """`ExternalBugTracker` subclass for handling RT imports."""
 
     ticket_url = 'REST/1.0/ticket/%s/show'
     batch_url = 'REST/1.0/search/ticket/'
     batch_query_threshold = 1
 
+    def __init__(self, baseurl, cookie_jar=None):
+        super(RequestTracker, self).__init__(baseurl)
+
+        if cookie_jar is None:
+            cookie_jar = RequestsCookieJar()
+        self._cookie_jar = cookie_jar
+        self._logged_in = False
+
     @property
     def credentials(self):
         """Return the authentication credentials needed to log in.
@@ -53,68 +61,36 @@
         except KeyError:
             return {'user': 'guest', 'pass': 'guest'}
 
-    def _logIn(self, opener):
-        """Attempt to log in to the remote RT service.
-
-        :param opener: An instance of urllib2.OpenerDirector
-            to be used to connect to the remote server.
-
-        If HTTPError or URLErrors are encountered at any point in this
-        process, they will be raised to be caught at the callsite.
-
-        This method is separate from the _opener property so as to allow
-        us to test the _opener property without having to connect to a
-        remote server.
-        """
-        # To log in to an RT instance we must pass a username and
-        # password to its login form, as a user would from the web.
-        opener.open('%s/' % self.baseurl, urllib.urlencode(
-            self.credentials))
-
-    @cachedproperty
-    def _opener(self):
-        """Return a urllib2.OpenerDirector for the remote RT instance.
-
-        An attempt will be made to log in to the remote instance before
-        the opener is returned. If logging in is not successful a
-        BugTrackerConnectError will be raised
-        """
-        opener = urllib2.build_opener(urllib2.HTTPCookieProcessor())
-
-        # Attempt to log in to the remote system. Raise an error if we
-        # can't.
-        try:
-            self._logIn(opener)
-        except (urllib2.HTTPError, urllib2.URLError) as error:
-            raise BugTrackerConnectError('%s/' % self.baseurl,
-                "Unable to authenticate with remote RT service: "
-                "Could not submit login form: %s" % error)
-
-        return opener
-
-    @ensure_no_transaction
-    def urlopen(self, request, data=None):
-        """Return a handle to a remote resource.
-
-        This method overrides that of `ExternalBugTracker` so that the
-        custom URL opener for RequestTracker instances can be used.
-        """
-        # We create our own opener so as to handle the RT authentication
-        # cookies that need to be passed around.
-        return self._opener.open(request, data)
+    def makeRequest(self, method, url, **kwargs):
+        """See `ExternalBugTracker`."""
+        if not self._logged_in:
+            # To log in to an RT instance we must pass a username and
+            # password to its login form, as a user would from the web.
+            try:
+                super(RequestTracker, self).makeRequest(
+                    'GET', '%s/' % self.baseurl,
+                    params=self.credentials, cookies=self._cookie_jar)
+            except requests.RequestException as e:
+                raise BugTrackerConnectError('%s/' % self.baseurl,
+                    "Unable to authenticate with remote RT service: "
+                    "Could not submit login form: %s" % e)
+            self._logged_in = True
+        return super(RequestTracker, self).makeRequest(
+            method, url, cookies=self._cookie_jar, **kwargs)
 
     def getRemoteBug(self, bug_id):
         """See `ExternalBugTracker`."""
         ticket_url = self.ticket_url % str(bug_id)
         query_url = '%s/%s' % (self.baseurl, ticket_url)
         try:
-            bug_data = self.urlopen(query_url)
-        except urllib2.HTTPError as error:
-            raise BugTrackerConnectError(ticket_url, str(error))
+            bug_data = self.makeRequest('GET', query_url).text
+        except requests.HTTPError as error:
+            raise BugTrackerConnectError(ticket_url, error)
 
         # We use the first line of the response to ensure that we've
         # made a successful request.
-        firstline = bug_data.readline().strip().split(' ')
+        bug_firstline, bug_rest = re.split(r'\r?\n', bug_data, maxsplit=1)
+        firstline = bug_firstline.strip().split(' ')
         if firstline[1] != '200':
             # If anything goes wrong we raise a BugTrackerConnectError.
             # We included in the error message the status code and error
@@ -127,7 +103,7 @@
 
         # RT's REST interface returns tickets in RFC822 format, so we
         # can use the email module to parse them.
-        bug = email.message_from_string(bug_data.read().strip())
+        bug = email.message_from_string(bug_rest.strip())
         if bug.get('id') is None:
             return None, None
         else:
@@ -138,19 +114,21 @@
         """See `ExternalBugTracker`."""
         # We need to ensure that all the IDs are strings first.
         id_list = [str(id) for id in bug_ids]
-        query = "id = " + "OR id = ".join(id_list)
+        query = "id = " + " OR id = ".join(id_list)
 
         query_url = '%s/%s' % (self.baseurl, self.batch_url)
         request_params = {'query': query, 'format': 'l'}
         try:
-            bug_data = self.urlopen(query_url, urllib.urlencode(
-                request_params))
-        except urllib2.HTTPError as error:
-            raise BugTrackerConnectError(query_url, error.message)
+            bug_data = self.makeRequest(
+                'GET', query_url, params=request_params,
+                headers={'Referer': self.baseurl}).text
+        except requests.HTTPError as error:
+            raise BugTrackerConnectError(query_url, error)
 
         # We use the first line of the response to ensure that we've
         # made a successful request.
-        firstline = bug_data.readline().strip().split(' ')
+        bug_firstline, bug_rest = re.split(r'\r?\n', bug_data, maxsplit=1)
+        firstline = bug_firstline.strip().split(' ')
         if firstline[1] != '200':
             # If anything goes wrong we raise a BugTrackerConnectError.
             # We included in the error message the status code and error
@@ -164,7 +142,7 @@
 
         # Tickets returned in RT multiline format are separated by lines
         # containing only --\n.
-        tickets = bug_data.read().split("--\n")
+        tickets = bug_rest.split("--\n")
         bugs = {}
         for ticket in tickets:
             ticket = ticket.strip()

=== modified file 'lib/lp/bugs/tests/externalbugtracker.py'
--- lib/lp/bugs/tests/externalbugtracker.py	2018-06-23 00:10:25 +0000
+++ lib/lp/bugs/tests/externalbugtracker.py	2018-06-23 00:10:25 +0000
@@ -26,8 +26,6 @@
 import responses
 from six.moves.urllib_parse import (
     parse_qs,
-    urljoin,
-    urlparse,
     urlsplit,
     )
 from zope.component import getUtility
@@ -1575,33 +1573,39 @@
             'GET', re.compile(re.escape(self.baseurl)), self._getCallback)
 
 
-class TestRequestTracker(RequestTracker):
-    """A Test-oriented `RequestTracker` implementation.
+class TestRequestTracker(BugTrackerResponsesMixin, RequestTracker):
+    """A Test-oriented `RequestTracker` implementation."""
 
-    Overrides _getPage() and _postPage() so that access to an RT
-    instance is not needed.
-    """
-    trace_calls = False
     simulate_bad_response = False
 
-    def urlopen(self, page, data=None):
-        file_path = os.path.join(os.path.dirname(__file__), 'testfiles')
-        path = urlparse(page)[2].lstrip('/')
-        if self.trace_calls:
-            print "CALLED urlopen(%r)" % path
-
-        if self.simulate_bad_response:
-            return open(file_path + '/' + 'rt-sample-bug-bad.txt')
-
-        if path == self.batch_url:
-            return open(file_path + '/' + 'rt-sample-bug-batch.txt')
-        else:
-            # We extract the ticket ID from the url and use that to find
-            # the test file we want.
-            page_re = re.compile('REST/1.0/ticket/([0-9]+)/show')
-            bug_id = page_re.match(path).groups()[0]
-
-            return open(file_path + '/' + 'rt-sample-bug-%s.txt' % bug_id)
+    def _getCallback(self, request):
+        url = urlsplit(request.url)
+        headers = {}
+        if url.path == '/':
+            params = parse_qs(url.query)
+            headers['Set-Cookie'] = 'rt_credentials=%s:%s' % (
+                params['user'][0], params['pass'][0])
+            body = ''
+        else:
+            if url.path == '/REST/1.0/search/ticket/':
+                body = read_test_file('rt-sample-bug-batch.txt')
+            else:
+                # Extract the ticket ID from the URL and use that to
+                # find the test file we want.
+                bug_id = re.match(
+                    r'/REST/1\.0/ticket/([0-9]+)/show', url.path).group(1)
+                body = read_test_file('rt-sample-bug-%s.txt' % bug_id)
+        return 200, headers, body
+
+    def addResponses(self, requests_mock, bad=False):
+        """Add test responses."""
+        if bad:
+            requests_mock.add(
+                'GET', re.compile(re.escape(self.baseurl)),
+                body=read_test_file('rt-sample-bug-bad.txt'))
+        else:
+            requests_mock.add_callback(
+                'GET', re.compile(re.escape(self.baseurl)), self._getCallback)
 
 
 class TestSourceForge(SourceForge):

=== modified file 'lib/lp/services/timeout.py'
--- lib/lp/services/timeout.py	2018-06-05 01:50:30 +0000
+++ lib/lp/services/timeout.py	2018-06-23 00:10:25 +0000
@@ -355,6 +355,12 @@
         response.raise_for_status()
         # Make sure the content has been consumed before returning.
         response.content
+        # The responses library doesn't persist cookies in the session
+        # (https://github.com/getsentry/responses/issues/80).  Work around
+        # this.
+        session_cookies = request_kwargs.get("cookies")
+        if session_cookies is not None and response.cookies:
+            session_cookies.update(response.cookies)
         return response
 
     def cleanup(self):