launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #22653
[Merge] lp:~cjwatson/launchpad/explicit-proxy-rt into lp:launchpad
Colin Watson has proposed merging lp:~cjwatson/launchpad/explicit-proxy-rt into lp:launchpad with lp:~cjwatson/launchpad/explicit-proxy-roundup as a prerequisite.
Commit message:
Convert the RequestTracker external bug tracker to urlfetch.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
For more details, see:
https://code.launchpad.net/~cjwatson/launchpad/explicit-proxy-rt/+merge/348429
We need a bit of care to persist cookies in tests due to a bug in responses.
--
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~cjwatson/launchpad/explicit-proxy-rt into lp:launchpad.
=== 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):
Follow ups
