Launchpad itself

Merge lp://staging/~allenap/launchpad/kde-bugzilla-being-odd into lp://staging/launchpad

  1. kde-bugzilla-being-odd
  2. Merge into devel
Proposed by Gavin Panella
Status: Merged
Approved by: Gavin Panella
Approved revision: not available
Merged at revision: not available
Proposed branch: lp://staging/~allenap/launchpad/kde-bugzilla-being-odd
Merge into: lp://staging/launchpad
Diff against target: 125 lines (+39/-23)
2 files modified
lib/lp/bugs/doc/externalbugtracker-bugzilla.txt (+28/-3)
lib/lp/bugs/externalbugtracker/bugzilla.py (+11/-20)
To merge this branch: bzr merge lp://staging/~allenap/launchpad/kde-bugzilla-being-odd
Reviewer Review Type Date Requested Status
Brad Crittenden (community) code Approve
Review via email: mp+17142@code.staging.launchpad.net

Commit message

Don't break when an XML-RPC reply from a remote Bugzilla instance is not a mapping. Older Bugzilla instances return tuples. Also, simplify Bugzilla version parsing.

To post a comment you must log in.
Revision history for this message
Gavin Panella (allenap) wrote :

KDE bugs returns tuples instead of mappings to XML-RPC calls. This causes the Bugzilla sniffer to fall over. I've changed it to consider anything other than a dict as unsupported (for API consideration). This means Launchpad will fall-back to the standard boring stupid Bugzilla support, which should work at least. This addresses bug 505958. I discussed this with Graham Binns briefly before implementation.

I also noticed that I could have a go at fixing Bugzilla._parseVersion() to simply extract the numeric parts of the version. This addresses bug 334980.

Lint free.

Test: bin/test -vvt 'external.*bug'

Revision history for this message
Brad Crittenden (bac) wrote :

Looks nice Gavin.

Since 'remote_version_dict' may be a dict or a tuple how about just calling it 'remote_version'?

review: Approve (code)
Revision history for this message
Gavin Panella (allenap) wrote :

Thanks Brad! Good catch; I've changed the name.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'lib/lp/bugs/doc/externalbugtracker-bugzilla.txt'
2--- lib/lp/bugs/doc/externalbugtracker-bugzilla.txt 2009-09-23 12:24:10 +0000
3+++ lib/lp/bugs/doc/externalbugtracker-bugzilla.txt 2010-01-13 10:28:18 +0000
4@@ -24,11 +24,11 @@
5
6 >>> from canonical.testing import LaunchpadZopelessLayer
7 >>> txn = LaunchpadZopelessLayer.txn
8- >>> external_bugzilla = Bugzilla('http://example.com/', version='A.B4')
9+ >>> external_bugzilla = Bugzilla('http://example.com/', version='A.B')
10 Traceback (most recent call last):
11 ...
12 UnparseableBugTrackerVersion:
13- Failed to parse version 'A.B4' for http://...
14+ Failed to parse version 'A.B' for http://...
15
16 The version parsing is carried out by the Bugzilla._parseVersion()
17 method, which takes a version string and returns a tuple of
18@@ -51,7 +51,7 @@
19 + characters in the version string will be removed.
20
21 >>> print external_bugzilla._parseVersion('3.2+1')
22- (3, 21)
23+ (3, 2, 1)
24
25 Since we don't want to depend on a working network connection, we use a
26 slightly modified implementation.
27@@ -201,6 +201,31 @@
28 >>> isinstance(bugzilla_to_use, BugzillaLPPlugin)
29 True
30
31+Older versions of the Bugzilla API return tuples rather than mappings
32+in response to XML-RPC calls. When something other than a mapping is
33+returned, the standard non-API non-plugin external bug tracker is
34+selected.
35+
36+ >>> class OldXMLRPCTransport(xmlrpclib.Transport):
37+ ... def request(self, host, handler, request, verbose=None):
38+ ... args, method_name = xmlrpclib.loads(request)
39+ ...
40+ ... if method_name == 'Bugzilla.version':
41+ ... return ('versionResponse', {'version': '3.2.5+'})
42+ ... else:
43+ ... raise xmlrpclib.Fault('Client', 'No such method')
44+ ...
45+ >>> test_transport = OldXMLRPCTransport()
46+
47+ >>> bugzilla._test_xmlrpc_proxy = xmlrpclib.ServerProxy(
48+ ... 'http://example.com/xmlrpc.cgi',
49+ ... transport=test_transport)
50+
51+ >>> bugzilla_to_use = bugzilla.getExternalBugTrackerToUse()
52+ >>> (isinstance(bugzilla_to_use, BugzillaAPI) or
53+ ... isinstance(bugzilla_to_use, BugzillaLPPlugin))
54+ False
55+
56
57 == Status Conversion ==
58
59
60=== modified file 'lib/lp/bugs/externalbugtracker/bugzilla.py'
61--- lib/lp/bugs/externalbugtracker/bugzilla.py 2010-01-06 14:35:52 +0000
62+++ lib/lp/bugs/externalbugtracker/bugzilla.py 2010-01-13 10:28:18 +0000
63@@ -12,6 +12,7 @@
64 ]
65
66 import pytz
67+import re
68 import xml.parsers.expat
69 import xmlrpclib
70
71@@ -69,7 +70,7 @@
72 try:
73 # We try calling Bugzilla.version() on the remote
74 # server because it's the most lightweight method there is.
75- remote_version_dict = proxy.Bugzilla.version()
76+ remote_version = proxy.Bugzilla.version()
77 except xmlrpclib.Fault, fault:
78 if fault.faultCode == 'Client':
79 return False
80@@ -88,10 +89,12 @@
81 # The server returned an unparsable response.
82 return False
83 else:
84- if remote_version_dict['version'] >= '3.4':
85- return True
86- else:
87- return False
88+ # Older versions of the Bugzilla API return tuples. We
89+ # consider anything other than a mapping to be unsupported.
90+ if isinstance(remote_version, dict):
91+ if remote_version['version'] >= '3.4':
92+ return True
93+ return False
94
95 def _remoteSystemHasPluginAPI(self):
96 """Return True if the remote host has the Launchpad plugin installed.
97@@ -191,25 +194,13 @@
98 if version is None:
99 return None
100
101- try:
102- # XXX 2008-09-15 gmb bug 270695:
103- # We can clean this up by just stripping out anything
104- # not in [0-9\.].
105- # Get rid of trailing -rh, -debian, etc.
106- version = version.split("-")[0]
107- # Ignore plusses in the version.
108- version = version.replace("+", "")
109- # Ignore the 'rc' string in release candidate versions.
110- version = version.replace("rc", "")
111- # We need to convert the version to a tuple of integers if
112- # we are to compare it correctly.
113- version = tuple(int(x) for x in version.split("."))
114- except ValueError:
115+ version_numbers = re.findall('[0-9]+', version)
116+ if len(version_numbers) == 0:
117 raise UnparseableBugTrackerVersion(
118 'Failed to parse version %r for %s' %
119 (version, self.baseurl))
120
121- return version
122+ return tuple(int(number) for number in version_numbers)
123
124 def convertRemoteImportance(self, remote_importance):
125 """See `ExternalBugTracker`.