launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #02550
[Merge] lp:~allenap/launchpad/cw-bugzilla-sniffing-expat-errors into lp:launchpad
Gavin Panella has proposed merging lp:~allenap/launchpad/cw-bugzilla-sniffing-expat-errors into lp:launchpad with lp:~allenap/launchpad/use-zope-tb-formatter as a prerequisite.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
Related bugs:
#714820 Many ExpatErrors from checkwatches
https://bugs.launchpad.net/bugs/714820
For more details, see:
https://code.launchpad.net/~allenap/launchpad/cw-bugzilla-sniffing-expat-errors/+merge/48985
This branch does a few small things:
- Demonstrates that, under certain conditions, the Transport used for
XML-RPC to remote Bugzilla instances can blow up with an
ExpatError. This indicates that the remote system has probably not
understood the XML-RPC request and has sent back an HTML error page.
- Catches ExpatErrors raised while sniffing remote Bugzilla instances
for XML-RPC capabilities.
- Uses the traceback_info() helper found in the prerequisite branch to
add the response to XML-RPC probing into the stack trace.
This will help us to more quickly diagnose future problems similar
to this. I spent the best part of a day figuring this out because I
could not replicate it locally (until I read the xmlrpclib source
code and the lightbulb turned on).
- Removes unused test method patch_transport_opener().
- Fixes some lint, including fixing a misnamed test class.
--
https://code.launchpad.net/~allenap/launchpad/cw-bugzilla-sniffing-expat-errors/+merge/48985
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~allenap/launchpad/cw-bugzilla-sniffing-expat-errors into lp:launchpad.
=== modified file 'lib/lp/bugs/externalbugtracker/bugzilla.py'
--- lib/lp/bugs/externalbugtracker/bugzilla.py 2011-01-27 15:23:53 +0000
+++ lib/lp/bugs/externalbugtracker/bugzilla.py 2011-02-08 21:11:42 +0000
@@ -102,7 +102,7 @@
return False
else:
raise
- except xmlrpclib.ResponseError:
+ except (xmlrpclib.ResponseError, xml.parsers.expat.ExpatError):
# The server returned an unparsable response.
return False
else:
@@ -145,7 +145,7 @@
return False
else:
raise
- except xmlrpclib.ResponseError:
+ except (xmlrpclib.ResponseError, xml.parsers.expat.ExpatError):
# The server returned an unparsable response.
return False
else:
=== modified file 'lib/lp/bugs/externalbugtracker/tests/test_bugzilla.py'
--- lib/lp/bugs/externalbugtracker/tests/test_bugzilla.py 2011-01-19 00:10:48 +0000
+++ lib/lp/bugs/externalbugtracker/tests/test_bugzilla.py 2011-02-08 21:11:42 +0000
@@ -6,11 +6,18 @@
__metaclass__ = type
from StringIO import StringIO
+from xml.parsers.expat import ExpatError
+import xmlrpclib
+
+import transaction
from canonical.testing.layers import DatabaseFunctionalLayer
from lp.bugs.externalbugtracker.base import UnparsableBugData
from lp.bugs.externalbugtracker.bugzilla import Bugzilla
-from lp.testing import TestCaseWithFactory
+from lp.testing import (
+ TestCase,
+ TestCaseWithFactory,
+ )
from lp.testing.fakemethod import FakeMethod
@@ -55,3 +62,29 @@
"""
bugzilla = self._makeInstrumentedBugzilla(content=result_text)
self.assertRaises(UnparsableBugData, bugzilla.getRemoteBugBatch, [])
+
+
+class TestBugzillaSniffing(TestCase):
+ """Tests for sniffing remote Bugzilla capabilities."""
+
+ layer = DatabaseFunctionalLayer
+
+ def test_expat_error(self):
+ # If an `ExpatError` is raised when sniffing for XML-RPC capabilities,
+ # it is taken to mean that no XML-RPC capabilities exist.
+ bugzilla = Bugzilla("http://not.real";)
+
+ class Transport(xmlrpclib.Transport):
+ def request(self, host, handler, request, verbose=None):
+ raise ExpatError("mismatched tag")
+
+ bugzilla._test_xmlrpc_proxy = xmlrpclib.ServerProxy(
+ '%s/xmlrpc.cgi' % bugzilla.baseurl, transport=Transport())
+
+ # We must abort any existing transactions before attempting to call
+ # the _remoteSystemHas*() functions because they require that none be
+ # in progress.
+ transaction.abort()
+
+ self.assertFalse(bugzilla._remoteSystemHasBugzillaAPI())
+ self.assertFalse(bugzilla._remoteSystemHasPluginAPI())
=== added file 'lib/lp/bugs/externalbugtracker/tests/test_xmlrpc.py'
--- lib/lp/bugs/externalbugtracker/tests/test_xmlrpc.py 1970-01-01 00:00:00 +0000
+++ lib/lp/bugs/externalbugtracker/tests/test_xmlrpc.py 2011-02-08 21:11:42 +0000
@@ -0,0 +1,35 @@
+# Copyright 2011 Canonical Ltd. This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+"""Tests for `lp.bugs.externalbugtracker.xmlrpc`."""
+
+__metaclass__ = type
+
+from xml.parsers.expat import ExpatError
+
+from lp.bugs.externalbugtracker.xmlrpc import UrlLib2Transport
+from lp.bugs.tests.externalbugtracker import (
+ ensure_response_parser_is_expat,
+ Urlib2TransportTestHandler,
+ )
+from lp.testing import TestCase
+
+
+class TestUrlLib2Transport(TestCase):
+ """Tests for `UrlLib2Transport`."""
+
+ def test_expat_error(self):
+ # Malformed XML-RPC responses cause xmlrpclib to raise an ExpatError.
+ handler = Urlib2TransportTestHandler()
+ handler.setResponse("<params><mis></match></params>")
+ transport = UrlLib2Transport("http://not.real/";)
+ transport.opener.add_handler(handler)
+
+ # The Launchpad production environment selects Expat at present. This
+ # is quite strict compared to the other parsers that xmlrpclib can
+ # possibly select.
+ ensure_response_parser_is_expat(transport)
+
+ self.assertRaises(
+ ExpatError, transport.request,
+ 'www.example.com', 'xmlrpc', "<methodCall />")
=== modified file 'lib/lp/bugs/externalbugtracker/xmlrpc.py'
--- lib/lp/bugs/externalbugtracker/xmlrpc.py 2010-08-20 20:31:18 +0000
+++ lib/lp/bugs/externalbugtracker/xmlrpc.py 2011-02-08 21:11:42 +0000
@@ -6,11 +6,12 @@
__metaclass__ = type
__all__ = [
'UrlLib2Transport',
- 'XMLRPCRedirectHandler'
+ 'XMLRPCRedirectHandler',
]
from cookielib import Cookie
+from cStringIO import StringIO
from urllib2 import (
build_opener,
HTTPCookieProcessor,
@@ -27,6 +28,8 @@
Transport,
)
+from lp.services.utils import traceback_info
+
class XMLRPCRedirectHandler(HTTPRedirectHandler):
"""A handler for HTTP redirections of XML-RPC requests."""
@@ -105,8 +108,10 @@
headers = {'Content-type': 'text/xml'}
request = Request(url, request_body, headers)
try:
- response = self._parse_response(self.opener.open(request), None)
+ response = self.opener.open(request).read()
except HTTPError, he:
raise ProtocolError(
request.get_full_url(), he.code, he.msg, he.hdrs)
- return response
+ else:
+ traceback_info(response)
+ return self._parse_response(StringIO(response), None)
=== modified file 'lib/lp/bugs/tests/externalbugtracker-xmlrpc-transport.txt'
--- lib/lp/bugs/tests/externalbugtracker-xmlrpc-transport.txt 2010-04-09 12:42:15 +0000
+++ lib/lp/bugs/tests/externalbugtracker-xmlrpc-transport.txt 2011-02-08 21:11:42 +0000
@@ -1,4 +1,5 @@
-= XMLRPC urllib2 transport =
+XMLRPC urllib2 transport
+------------------------
When using XMLRPC for connecting to external bug trackers, we need to
use a special transport, which processes http cookies correctly, and
@@ -60,7 +61,7 @@
Cookie(version=0, name='foo', value='bar'...)]>
If an error occurs trying to make the request, an
-`xmlrpclib.ProtocolError` is raised.
+``xmlrpclib.ProtocolError`` is raised.
>>> from urllib2 import HTTPError
>>> test_handler.setError(
@@ -107,7 +108,8 @@
('http://www.example.com/xmlrpc/redirected',)
-== The XMLRPCRedirectHandler ==
+The XMLRPCRedirectHandler
+=========================
The UrlLib2Transport uses a custom HTTP redirection handler to handle
redirect responses. This is a subclass of urllib2's HTTPRedirectHandler.
@@ -188,4 +190,3 @@
>>> print redirected_request.data
None
-
=== modified file 'lib/lp/bugs/tests/externalbugtracker.py'
--- lib/lp/bugs/tests/externalbugtracker.py 2011-01-18 18:36:49 +0000
+++ lib/lp/bugs/tests/externalbugtracker.py 2011-02-08 21:11:42 +0000
@@ -31,14 +31,12 @@
commit,
ZopelessTransactionManager,
)
-from lp.bugs.model.bugtracker import BugTracker
from canonical.launchpad.ftests import (
login,
logout,
)
from canonical.launchpad.interfaces.logintoken import ILoginTokenSet
from canonical.launchpad.testing.systemdocs import ordered_dict_as_string
-from lp.bugs.xmlrpc.bug import ExternalBugTrackerTokenAPI
from canonical.testing.layers import LaunchpadZopelessLayer
from lp.bugs.externalbugtracker import (
BATCH_SIZE_UNLIMITED,
@@ -70,7 +68,9 @@
UNKNOWN_REMOTE_IMPORTANCE,
UNKNOWN_REMOTE_STATUS,
)
+from lp.bugs.model.bugtracker import BugTracker
from lp.bugs.scripts import debbugs
+from lp.bugs.xmlrpc.bug import ExternalBugTrackerTokenAPI
from lp.registry.interfaces.person import IPersonSet
@@ -1644,6 +1644,7 @@
def __init__(self):
self.redirect_url = None
self.raise_error = None
+ self.response = None
self.accessed_urls = []
def setRedirect(self, new_url):
@@ -1655,6 +1656,9 @@
self.raise_error = error
self.raise_url = url
+ def setResponse(self, response):
+ self.response = response
+
def default_open(self, req):
"""Catch all requests and return a hard-coded response.
@@ -1681,6 +1685,14 @@
self.redirect_url = None
response = self.parent.error(
'http', req, response, 302, 'Moved', headers)
+ elif self.response is not None:
+ response = StringIO(self.response)
+ info = Urlib2TransportTestInfo()
+ response.info = lambda: info
+ response.code = 200
+ response.geturl = lambda: req.get_full_url()
+ response.msg = ''
+ self.response = None
else:
xmlrpc_response = xmlrpclib.dumps(
(req.get_full_url(), ), methodresponse=True)
@@ -1694,6 +1706,20 @@
return response
-def patch_transport_opener(transport):
- """Patch the transport's opener to use a test handler."""
- transport.opener.add_handler(Urlib2TransportTestHandler())
+def ensure_response_parser_is_expat(transport):
+ """Ensure the transport always selects the Expat-based response parser.
+
+ The response parser is chosen by xmlrpclib at runtime from a number of
+ choices, but the main Launchpad production environment selects Expat at
+ present.
+
+ Developer's machines could have other packages, `python-reportlab-accel`
+ (which provides the `sgmlop` module) for example, that cause different
+ response parsers to be chosen.
+ """
+ def getparser():
+ target = xmlrpclib.Unmarshaller(
+ use_datetime=transport._use_datetime)
+ parser = xmlrpclib.ExpatParser(target)
+ return parser, target
+ transport.getparser = getparser
