Merge lp:~thumper/launchpad/fix-mantis-warnings-bug-218384 into lp:launchpad

[Tim Penhey]

[r=lifeless,wgrant][bug=214982,218384] Stop two Mantis OOPSes that occur during batch processing.

This branch fixes two old Mantis issues.

HTTPError when trying to determine if the remote site can
handle batches, and the oops handled when trying to use
a non-existant warning method.

The HTTPError is handled by wrapping the urlopen method with
the _fetchPage method, which translates the urllib2 errors to
a BugTrackerConnectionError.

Fixing the warning usage was trivial, but I refactored the
code somewhat to make testing easier. A class was created
that is responsible for parsing the CVS data.

Also an unused method (_getStatusFromCSV) was deleted.

[William Grant (wgrant)]

review: Approve (code*)

[Robert Collins (lifeless)]

review: Approve

[William Grant (wgrant)]

As long as you land it through ec2 this time :)

review: Approve (code*)

[Robert Collins (lifeless)]

Sure, lets try this again.

review: Approve

=== modified file 'lib/lp/bugs/doc/externalbugtracker-trac-lp-plugin.txt'
--- lib/lp/bugs/doc/externalbugtracker-trac-lp-plugin.txt	2011-02-07 20:41:06 +0000
+++ lib/lp/bugs/doc/externalbugtracker-trac-lp-plugin.txt	2011-03-02 00:14:04 +0000
@@ -40,7 +40,7 @@
     ...         self.headers = {}
 
     >>> class TestTracLPPlugin(TracLPPlugin):
-    ...     def urlopen(self, url):
+    ...     def urlopen(self, url, data=None):
     ...         with lp_dbuser():
     ...             base_auth_url = urlappend(self.baseurl, 'launchpad-auth')
     ...             if not url.startswith(base_auth_url + '/'):
@@ -133,7 +133,7 @@
 
     >>> from urllib2 import HTTPError
     >>> class TestFailingTracLPPlugin(TracLPPlugin):
-    ...     def urlopen(self, url):
+    ...     def urlopen(self, url, data=None):
     ...         raise HTTPError(url, 401, "Denied!", {}, None)
 
     >>> test_trac = TestFailingTracLPPlugin(

=== modified file 'lib/lp/bugs/doc/externalbugtracker-trac.txt'
--- lib/lp/bugs/doc/externalbugtracker-trac.txt	2011-01-19 00:10:48 +0000
+++ lib/lp/bugs/doc/externalbugtracker-trac.txt	2011-03-02 00:14:04 +0000
@@ -33,7 +33,7 @@
 
     >>> import urllib2
     >>> class TracHavingLPPlugin401(Trac):
-    ...     def urlopen(self, url):
+    ...     def urlopen(self, url, data=None):
     ...         print url
     ...         raise urllib2.HTTPError(
     ...             url, 401, "Unauthorized", None, None)
@@ -60,13 +60,13 @@
     >>> from urllib import addinfourl
 
     >>> class TracReturning200ForPageNotFound(Trac):
-    ...     def urlopen(self, url):
+    ...     def urlopen(self, url, data=None):
     ...         print url
     ...         return addinfourl(
     ...             StringIO(''), HTTPMessage(StringIO('')), url)
 
     >>> class TracHavingLPPlugin200(Trac):
-    ...     def urlopen(self, url):
+    ...     def urlopen(self, url, data=None):
     ...         print url
     ...         return addinfourl(
     ...             StringIO(''), HTTPMessage(
@@ -99,7 +99,7 @@
 If a 404 is returned, the normal Trac instance is returned.
 
     >>> class TracNotHavingLPPlugin(Trac):
-    ...     def urlopen(self, url):
+    ...     def urlopen(self, url, data=None):
     ...         print url
     ...         raise urllib2.HTTPError(
     ...             url, 404, "Not found", None, None)

=== modified file 'lib/lp/bugs/externalbugtracker/base.py'
--- lib/lp/bugs/externalbugtracker/base.py	2011-03-01 11:49:59 +0000
+++ lib/lp/bugs/externalbugtracker/base.py	2011-03-02 00:14:04 +0000
@@ -238,13 +238,13 @@
         """
         return None
 
-    def _fetchPage(self, page):
+    def _fetchPage(self, page, data=None):
         """Fetch a page from the remote server.
 
         A BugTrackerConnectError will be raised if anything goes wrong.
         """
         try:
-            return self.urlopen(page)
+            return self.urlopen(page, data)
         except (urllib2.HTTPError, urllib2.URLError), val:
             raise BugTrackerConnectError(self.baseurl, val)
 
@@ -260,7 +260,7 @@
     def _post(self, url, data):
         """Post to a given URL."""
         request = urllib2.Request(url, headers={'User-agent': LP_USER_AGENT})
-        return self.urlopen(request, data=data)
+        return self._fetchPage(request, data=data)
 
     def _postPage(self, page, form, repost_on_redirect=False):
         """POST to the specified page and form.

=== modified file 'lib/lp/bugs/externalbugtracker/mantis.py'
--- lib/lp/bugs/externalbugtracker/mantis.py	2011-03-01 11:49:59 +0000
+++ lib/lp/bugs/externalbugtracker/mantis.py	2011-03-02 00:14:04 +0000
@@ -8,6 +8,7 @@
 
 import cgi
 import csv
+import logging
 import urllib
 import urllib2
 from urlparse import urlunparse
@@ -98,6 +99,70 @@
             self, request, fp, code, msg, hdrs, self.rewrite_url(new_url))
 
 
+class MantisBugBatchParser:
+    """A class that parses the batch of bug data.
+
+    Using the CSV reader is pretty much essential since the data that comes
+    back can include title text which can in turn contain field separators.
+    You don't want to handle the unquoting yourself.
+    """
+
+    def __init__(self, csv_data, logger):
+        # Clean out stray, unquoted newlines inside csv_data to avoid the CSV
+        # module blowing up.  IDEA: perhaps if the size of csv_data is large
+        # in the future, this could be moved into a generator.
+        csv_data = [s.replace("\r", "") for s in csv_data]
+        csv_data = [s.replace("\n", "") for s in csv_data]
+        self.reader = csv.reader(csv_data)
+        self.logger = logger
+
+    def processCSVBugLine(self, bug_line, headers):
+        """Processes a single line of the CSV."""
+        bug = {}
+        for index, header in enumerate(headers):
+            try:
+                data = bug_line[index]
+            except IndexError:
+                self.logger.warning("Line %r incomplete." % bug_line)
+                return None
+            bug[header] = data
+        try:
+            bug['id'] = int(bug['id'])
+        except ValueError:
+            self.logger.warning("Encountered invalid bug ID: %r." % bug['id'])
+            return None
+        return bug
+
+    def parseHeaderLine(self, reader):
+        # The first line of the CSV file is the header. We need to read
+        # it because different Mantis instances have different header
+        # ordering and even different columns in the export.
+        try:
+            headers = [h.lower() for h in reader.next()]
+        except StopIteration:
+            raise UnparsableBugData("Missing header line")
+        missing_headers = [
+            name for name in ('id', 'status', 'resolution')
+            if name not in headers]
+        if missing_headers:
+            raise UnparsableBugData(
+                "CSV header %r missing fields: %r" % (
+                    headers, missing_headers))
+        return headers
+
+    def getBugs(self):
+        headers = self.parseHeaderLine(self.reader)
+        bugs = {}
+        try:
+            for bug_line in self.reader:
+                bug = self.processCSVBugLine(bug_line, headers)
+                if bug is not None:
+                    bugs[bug['id']] = bug
+            return bugs
+        except csv.Error, error:
+            raise UnparsableBugData("Exception parsing CSV file: %s." % error)
+
+
 class Mantis(ExternalBugTracker):
     """An `ExternalBugTracker` for dealing with Mantis instances.
 
@@ -114,6 +179,7 @@
         self._cookie_handler = urllib2.HTTPCookieProcessor()
         self._opener = urllib2.build_opener(
             self._cookie_handler, MantisLoginHandler())
+        self._logger = logging.getLogger()
 
     @ensure_no_transaction
     def urlopen(self, request, data=None):
@@ -178,7 +244,10 @@
            'search': '',
            'filter': 'Apply Filter',
         }
-        self.page = self._postPage("view_all_set.php?f=3", data)
+        try:
+            self._postPage("view_all_set.php?f=3", data)
+        except BugTrackerConnectError:
+            return None
 
         # Finally grab the full CSV export, which uses the
         # MANTIS_VIEW_ALL_COOKIE set in the previous step to specify
@@ -275,65 +344,8 @@
         if not csv_data:
             raise UnparsableBugData("Empty CSV for %s" % self.baseurl)
 
-        # Clean out stray, unquoted newlines inside csv_data to avoid
-        # the CSV module blowing up.
-        csv_data = [s.replace("\r", "") for s in csv_data]
-        csv_data = [s.replace("\n", "") for s in csv_data]
-
-        # The first line of the CSV file is the header. We need to read
-        # it because different Mantis instances have different header
-        # ordering and even different columns in the export.
-        self.headers = [h.lower() for h in csv_data.pop(0).split(",")]
-        if len(self.headers) < 2:
-            raise UnparsableBugData("CSV header mangled: %r" % self.headers)
-
-        if not csv_data:
-            # A file with a header and no bugs is also useless.
-            raise UnparsableBugData("CSV for %s contained no bugs!"
-                                     % self.baseurl)
-
-        try:
-            bugs = {}
-            # Using the CSV reader is pretty much essential since the
-            # data that comes back can include title text which can in
-            # turn contain field separators -- you don't want to handle
-            # the unquoting yourself.
-            for bug_line in csv.reader(csv_data):
-                bug = self._processCSVBugLine(bug_line)
-                bugs[int(bug['id'])] = bug
-
-            return bugs
-
-        except csv.Error, error:
-            raise UnparsableBugData("Exception parsing CSV file: %s." % error)
-
-    def _processCSVBugLine(self, bug_line):
-        """Processes a single line of the CSV.
-
-        Adds the bug it represents to self.bugs.
-        """
-        required_fields = ['id', 'status', 'resolution']
-        bug = {}
-        for header in self.headers:
-            try:
-                data = bug_line.pop(0)
-            except IndexError:
-                self.warning("Line '%r' incomplete." % bug_line)
-                return
-            bug[header] = data
-        for field in required_fields:
-            if field not in bug:
-                self.warning("Bug %s lacked field '%r'." % (bug['id'], field))
-                return
-            try:
-                # See __init__ for an explanation of why we use integer
-                # IDs in the internal data structure.
-                bug_id = int(bug['id'])
-            except ValueError:
-                self.warning("Encountered invalid bug ID: %r." % bug['id'])
-                return
-
-        return bug
+        parser = MantisBugBatchParser(csv_data, self._logger)
+        return parser.getBugs()
 
     def _checkForApplicationError(self, page_soup):
         """If Mantis does not find the bug it still returns a 200 OK
@@ -467,14 +479,6 @@
         # it makes display of the data nicer.
         return "%(status)s: %(resolution)s" % bug
 
-    def _getStatusFromCSV(self, bug_id):
-        try:
-            bug = self.bugs[int(bug_id)]
-        except KeyError:
-            raise BugNotFound(bug_id)
-        else:
-            return bug['status'], bug['resolution']
-
     def convertRemoteImportance(self, remote_importance):
         """See `ExternalBugTracker`.
 

=== modified file 'lib/lp/bugs/externalbugtracker/tests/test_externalbugtracker.py'
--- lib/lp/bugs/externalbugtracker/tests/test_externalbugtracker.py	2011-03-01 11:49:59 +0000
+++ lib/lp/bugs/externalbugtracker/tests/test_externalbugtracker.py	2011-03-02 00:14:04 +0000
@@ -6,17 +6,25 @@
 __metaclass__ = type
 
 from StringIO import StringIO
+import urllib2
 
 from zope.interface import implements
 
-from lp.bugs.externalbugtracker.base import ExternalBugTracker
+from canonical.testing.layers import ZopelessLayer
+from lp.bugs.externalbugtracker.base import (
+    BugTrackerConnectError,
+    ExternalBugTracker,
+    )
 from lp.bugs.externalbugtracker.debbugs import DebBugs
 from lp.bugs.interfaces.externalbugtracker import (
     ISupportsBackLinking,
     ISupportsCommentImport,
     ISupportsCommentPushing,
     )
-from lp.testing import TestCase
+from lp.testing import (
+    monkey_patch,
+    TestCase,
+    )
 from lp.testing.fakemethod import FakeMethod
 
 
@@ -144,3 +152,21 @@
         self.assertEqual(2, bugtracker._post.call_count)
         last_args, last_kwargs = bugtracker._post.calls[-1]
         self.assertEqual((fake_form.url, ), last_args)
+
+
+class TestExternalBugTracker(TestCase):
+    """Tests for various methods of the ExternalBugTracker."""
+
+    layer = ZopelessLayer
+
+    def test_post_raises_on_404(self):
+        # When posting, a 404 is converted to a BugTrackerConnectError.
+        base_url = "http://example.com/"
+        bugtracker = ExternalBugTracker(base_url)
+        def raise404(request, data):
+            raise urllib2.HTTPError('url', 404, 'Not Found', None, None)
+        with monkey_patch(urllib2, urlopen=raise404):
+            self.assertRaises(
+                BugTrackerConnectError,
+                bugtracker._post,
+                'some-url', {'post-data': 'here'})

=== added file 'lib/lp/bugs/externalbugtracker/tests/test_mantis.py'
--- lib/lp/bugs/externalbugtracker/tests/test_mantis.py	1970-01-01 00:00:00 +0000
+++ lib/lp/bugs/externalbugtracker/tests/test_mantis.py	2011-03-02 00:14:04 +0000
@@ -0,0 +1,110 @@
+# Copyright 2011 Canonical Ltd.  This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+"""Tests for the Mantis BugTracker."""
+
+__metaclass__ = type
+
+from testtools.matchers import Equals, Is
+import urllib2
+
+from canonical.testing.layers import ZopelessLayer
+from lp.bugs.externalbugtracker import UnparsableBugData
+from lp.bugs.externalbugtracker.mantis import (
+    Mantis,
+    MantisBugBatchParser,
+    )
+from lp.testing import (
+    monkey_patch,
+    TestCase,
+    )
+from lp.services.log.logger import BufferLogger
+
+
+class TestMantisBugBatchParser(TestCase):
+    """Test the MantisBugBatchParser class."""
+
+    def setUp(self):
+        super(TestMantisBugBatchParser, self).setUp()
+        self.logger = BufferLogger()
+
+    def test_empty(self):
+        data = []
+        parser = MantisBugBatchParser(data, self.logger)
+        exc = self.assertRaises(
+            UnparsableBugData,
+            parser.getBugs)
+        self.assertThat(
+            str(exc), Equals("Missing header line"))
+
+    def test_missing_headers(self):
+        data = ['some,headers']
+        parser = MantisBugBatchParser(data, self.logger)
+        exc = self.assertRaises(
+            UnparsableBugData,
+            parser.getBugs)
+        self.assertThat(
+            str(exc),
+            Equals("CSV header ['some', 'headers'] missing fields:"
+                   " ['id', 'status', 'resolution']"))
+
+    def test_missing_some_headers(self):
+        data = ['some,headers,status,resolution']
+        parser = MantisBugBatchParser(data, self.logger)
+        exc = self.assertRaises(
+            UnparsableBugData,
+            parser.getBugs)
+        self.assertThat(
+            str(exc),
+            Equals("CSV header ['some', 'headers', 'status', 'resolution'] "
+                   "missing fields: ['id']"))
+
+    def test_no_bugs(self):
+        data = ['other,fields,id,status,resolution']
+        parser = MantisBugBatchParser(data, self.logger)
+        self.assertThat(parser.getBugs(), Equals({}))
+
+    def test_passing(self):
+        data = ['ignored,id,resolution,status',
+                'foo,42,not,complete',
+                'boo,13,,confirmed']
+        parser = MantisBugBatchParser(data, self.logger)
+        bug_42 = dict(
+            id=42, status='complete', resolution='not', ignored='foo')
+        bug_13 = dict(
+            id=13, status='confirmed', resolution='', ignored='boo')
+        self.assertThat(parser.getBugs(), Equals({42: bug_42, 13: bug_13}))
+
+    def test_incomplete_line(self):
+        data = ['ignored,id,resolution,status',
+                '42,not,complete']
+        parser = MantisBugBatchParser(data, self.logger)
+        self.assertThat(parser.getBugs(), Equals({}))
+        log = self.logger.getLogBuffer()
+        self.assertThat(
+            log, Equals("WARNING Line ['42', 'not', 'complete'] incomplete.\n"))
+
+    def test_non_integer_id(self):
+        data = ['ignored,id,resolution,status',
+                'foo,bar,not,complete']
+        parser = MantisBugBatchParser(data, self.logger)
+        self.assertThat(parser.getBugs(), Equals({}))
+        log = self.logger.getLogBuffer()
+        self.assertThat(
+            log, Equals("WARNING Encountered invalid bug ID: 'bar'.\n"))
+
+
+class TestMantisBugTracker(TestCase):
+    """Tests for various methods of the Manits bug tracker."""
+
+    layer = ZopelessLayer
+
+    def test_csv_data_on_post_404(self):
+        # If the 'view_all_set.php' request raises a 404, then the csv_data
+        # attribute is None.
+        base_url = "http://example.com/"
+        def raise404(self, request, data):
+            raise urllib2.HTTPError('url', 404, 'Not Found', None, None)
+        with monkey_patch(Mantis, urlopen=raise404):
+            bugtracker = Mantis(base_url)
+            self.assertThat(bugtracker.csv_data, Is(None))

=== modified file 'lib/lp/bugs/tests/externalbugtracker.py'
--- lib/lp/bugs/tests/externalbugtracker.py	2011-02-09 11:37:19 +0000
+++ lib/lp/bugs/tests/externalbugtracker.py	2011-03-02 00:14:04 +0000
@@ -1149,7 +1149,7 @@
         """See `Trac`."""
         return self.supports_single_exports
 
-    def urlopen(self, url):
+    def urlopen(self, url, data=None):
         file_path = os.path.join(os.path.dirname(__file__), 'testfiles')
 
         if self.trace_calls:
@@ -1489,7 +1489,7 @@
     batch_size = None
     trace_calls = False
 
-    def urlopen(self, url):
+    def urlopen(self, url, data=None):
         if self.trace_calls:
             print "CALLED urlopen(%r)" % (url)