launchpad-reviewers team mailing list archive

[William Grant <me@xxxxxxxxxxxxxxxxxx>]

William Grant has proposed merging lp:~wgrant/launchpad/bug-727024 into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  #727024 Trac ExternalBugTracker needs to catch connection errors
  https://bugs.launchpad.net/bugs/727024

For more details, see:
https://code.launchpad.net/~wgrant/launchpad/bug-727024/+merge/52642

URLErrors were escaping Trac.getExternalBugTrackerToUse, resulting in unhandled exceptions like OOPS-1885CCW1005. I've wrapped all requests in the class to ensure that no HTTPErrors or URLErrors can escape -- in most cases by replacing urlopen calls with _fetchPage. But some callsites need to know the HTTP status code, so I've added manual exception handlers where they were missing.
-- 
https://code.launchpad.net/~wgrant/launchpad/bug-727024/+merge/52642
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~wgrant/launchpad/bug-727024 into lp:launchpad.

=== modified file 'lib/lp/bugs/doc/externalbugtracker-trac-lp-plugin.txt'
--- lib/lp/bugs/doc/externalbugtracker-trac-lp-plugin.txt	2011-03-01 23:51:13 +0000
+++ lib/lp/bugs/doc/externalbugtracker-trac-lp-plugin.txt	2011-03-09 06:16:21 +0000
@@ -144,7 +144,7 @@
     Traceback (most recent call last):
       ...
     BugTrackerAuthenticationError: http://example.com:
-    401 "Denied!"...
+    HTTP Error 401: Denied!
 
 
 == Current time ==

=== modified file 'lib/lp/bugs/doc/externalbugtracker-trac.txt'
--- lib/lp/bugs/doc/externalbugtracker-trac.txt	2011-03-01 23:51:13 +0000
+++ lib/lp/bugs/doc/externalbugtracker-trac.txt	2011-03-09 06:16:21 +0000
@@ -110,6 +110,21 @@
     >>> chosen_trac is trac
     True
 
+In the event that a connection error is returned, we return a normal Trac
+instance. It will deal with the connection error later, if the situation
+persists.
+
+    >>> class TracWithConnectionError(Trac):
+    ...     def urlopen(self, url, data=None):
+    ...         print url
+    ...         raise urllib2.URLError("Connection timed out")
+    >>> trac = TracWithConnectionError('http://example.com/')
+    >>> chosen_trac = trac.getExternalBugTrackerToUse()
+    http://example.com/launchpad-auth/check
+
+    >>> chosen_trac is trac
+    True
+
 
 == Status Conversion ==
 

=== modified file 'lib/lp/bugs/externalbugtracker/trac.py'
--- lib/lp/bugs/externalbugtracker/trac.py	2011-02-24 15:30:54 +0000
+++ lib/lp/bugs/externalbugtracker/trac.py	2011-03-09 06:16:21 +0000
@@ -26,6 +26,7 @@
 from lp.bugs.externalbugtracker.base import (
     BugNotFound,
     BugTrackerAuthenticationError,
+    BugTrackerConnectError,
     ExternalBugTracker,
     InvalidBugId,
     LookupTree,
@@ -76,6 +77,8 @@
                 return TracLPPlugin(self.baseurl)
             else:
                 return self
+        except urllib2.URLError, error:
+            return self
         else:
             # If the response contains a trac_auth cookie then we're
             # talking to the LP plugin. However, it's unlikely that
@@ -105,7 +108,7 @@
                 # We try to retrive the ticket in HTML form, since that will
                 # tell us whether or not it is actually a valid ticket
                 ticket_id = int(bug_ids.pop())
-                self.urlopen(html_ticket_url % ticket_id)
+                self._fetchPage(html_ticket_url % ticket_id)
             except (ValueError, urllib2.HTTPError):
                 # If we get an HTTP error we can consider the ticket to be
                 # invalid. If we get a ValueError then the ticket_id couldn't
@@ -116,7 +119,7 @@
                 # CSV form. If this fails then we can consider single ticket
                 # exports to be unsupported.
                 try:
-                    csv_data = self.urlopen(
+                    csv_data = self._fetchPage(
                         "%s/%s" % (self.baseurl, self.ticket_url % ticket_id))
                     return csv_data.headers.subtype == 'csv'
                 except (urllib2.HTTPError, urllib2.URLError):
@@ -372,10 +375,9 @@
         auth_url = urlappend(base_auth_url, token_text)
 
         try:
-            self.urlopen(auth_url)
-        except urllib2.HTTPError, error:
-            raise BugTrackerAuthenticationError(
-                self.baseurl, '%s "%s"' % (error.code, error.msg))
+            self._fetchPage(auth_url)
+        except BugTrackerConnectError, e:
+            raise BugTrackerAuthenticationError(self.baseurl, e.error)
 
     @ensure_no_transaction
     @needs_authentication