Merge lp:~adeuring/launchpad/bug-904461 into lp:launchpad

[Abel Deuring]

[r=jcsackett,rharding][bug=904461] render +cve views more efficiently

This branch fixes bug 904461: Timeout viewing CVE reports in an Ubuntu series

This timeout is not caused by slow SQL queries, but by several bottlenecks
on Python level. Related OOPS reports show SQL times of less than one second.

Thanks to lifeless, who gave me a number good hints where to look and
how to reproduce the timeout locally.

I created test data that should be roughly similar to real world data:
a few hundred bugs/bug tasks and ca 250 CVE, often linked to more than
one bug: The +cve page for Oneiric (or Natty -- I can't remeber where I
looked...) contains approximately 4800 links to CVEs, but only 240
or 250 _different_ CVEs: CVEs are often linked to several bugs.

Probably the worst bottleneck is the query

        BugCve.select("""
            BugCve.bug IN %s""" % sqlvalues(bug_ids),
            prejoins=["cve"],
            orderBy=['bug', 'cve'])

in lp.bugs.model.cve.CveSet.getBugCvesForBugTasks() (line 434 in the
diff), where one CVE is on average loaded 20 times. The SQL itself is
quite fast, but Storm spends much time in methods like load_object().

I changed this in r14627 so that only the tuples (bug_id, cve_id) are
loaded from the table BugCve; the related Bug and Cve instances are
retrieved via load_related().

Another serious bottleneck is the function canonical_url(): profile
data showed that several seconds are spent there when a page is
rendered.

The number of calls can be considerably reduced: Most calls are for
Cve instances, so we can avoid a large part of them, if we call
the function just once for each Cve. I implemented this by adding
an optional parameter cve_mapper to getBugCvesForBugTasks();
the function get_cve_display_data() in lp.bugs.browser.cvereport
is now used as cve_mapper when the +cve view is rendered.
This saved 2.5 seconds in my test setup (r14629).

Antoher noticeable improvement was to retrieve the (bug, cve) tuples
for open and resolved CVEs in one query (r14628). I am not sure though
if the improvement in my test data resembles the real world: The number
of CVEs that are linked to open _and_ resovled bug tasks was probably
too big in the test data.

The last big bottleneck was the TAL loop that renders the CVE links.
Replacing it with simple Python list and string operations (r14630,
method CVEReportView.renderCVELinks()) saved 2.9 seconds.

A final, smaller improvemet was to bulk load the Bugs and the Person
records for bug task assignees (r14631).

Timing data from this test:

    def test_open_cve_bugtasks(self):
        from time import time
        distroseries = getUtility(IDistroSeriesSet).get(15)
        total = 0.0
        LOOPS = 3
        for i in range(LOOPS):
            start = time()
            view = create_initialized_view(distroseries, '+cve')
            view.render()
            delta = time() - start
            total += delta
            print "render time", delta
        print "avg", total/LOOPS

    (Note that this test is not included in the branch: Running it makes
    sense with the right test data: The script that created my test data
    ran more than one hour -- doing this in a regular unit test as a part
    of the complete test suite does not make sense. And the sample data
    file current.sql has a size of 9MB with the new test data...)

r14626 (original timeout) 14.3 sec
r14627 more efficent retrieval of (Bug, Cve) tuples 8.4 sec
r14628 one query to retrieve open and resolved CVEs 8.1 sec
r14629 efficient generation of CVE related display data 5.6 sec
r14630 render CVE links and join them in a browser method 2.9 sec
r14631 bulk load bugs and bugtask assignees 2.9 sec

(In r14631 and r14630, caching is noticable: The first loop execution
needed ca 4 seconds, the two following loop run needed ca 2.4 seconds.)

I did _not_ test that the number of SQL queries is constant, regardless
of the number of bugs/bug tasks/CVEs because it is not, unfortunately:
IPersonSet.getPrecachedPersonsFromIDs() does not avoid later SQL queries
for ValidPersonCache records of single Person records. Bulk loading
Person records works fine though.

tests:

./bin/test bugs -vvt test_cve

no lint.

[Richard Harding (rharding)]

Thanks Abel, this looks like it'll save a bunch of cpu cycles. I have a few questions/notes.

In the html generation, should the string that defines the template be moved outside of the method so that the variable instance isn't created each call (potentially thousands?) and instead be placed as some sort of class level constant?

Should we add a docstring to the getBugCvesForBugTasks and make note of the cve_mapper param as a callable method meant for that mapping?

I don't think it matters, but in looking at the TestCveSet test cases, I wonder if a test case with multiple bugs per cve should be exercised as well since much of the code is about dealing with that condition.

I don't think any of these are deal breakers so approving.

review: Approve (code*)

[j.c.sackett (jcsackett)]

> Should we add a docstring to the getBugCvesForBugTasks and make note of the
> cve_mapper param as a callable method meant for that mapping?

I see that you have a docstring for this method in the interface; given the complexity of what's going on, I think in the model code it may be worthwhile to simply add the usual "See `ICveSet`." docstring so people needing help quickly know where to go for it. I'll still approve, but would like this added before it lands.

Your tests look pretty extensive; I concur that adding multiple bugs per cve might be useful, but as you've got reasonable testing of performance and behavior of that situation elsewhere and the cve set tests verify the implementation sufficiently Rick is correct to say it's not a blocker.

One very minor point in the diff below:

> === modified file 'lib/lp/bugs/model/cve.py'
> --- lib/lp/bugs/model/cve.py 2011-12-30 06:14:56 +0000
> +++ lib/lp/bugs/model/cve.py 2012-01-19 12:21:17 +0000
> @@ -178,15 +182,41 @@
> raise AssertionError('MessageChunk without content or blob.')
> return sorted(cves, key=lambda a: a.sequence)
>
> - def getBugCvesForBugTasks(self, bugtasks):
> - bug_ids = set(task.bug.id for task in bugtasks)
> + def getBugCvesForBugTasks(self, bugtasks, cve_mapper=None):
> + bugs = load_related(Bug, bugtasks, ('bugID', ))
> + if len(bugs) == 0:
> + return []
> + bug_ids = [bug.id for bug in bugs]
> assert bug_ids, "bugtasks must be non-empty, received %r" % bugtasks

I think you can remove the assert--you return if bugs is len 0, and the
only other thing the assert would catch would be NoneType, which would blow up
on the len check. Obviously it's not hurting anything, but it's not really
necessary either.

review: Approve

[Abel Deuring (adeuring)]

Hi Rick,

thanks for your review. I made all changes you suggested, though the doc string for getBugCvesForBu() is just the usual terse "see `ICveSet`".

The test data generated in TestCVEReportView.setUp() now uses only five CVEs; two CVEs are associated with each test bug. This required some more changes in test_open_resolved_cve_bugtasks(), because some bugs are now associated with [cve_4, cve_0] -- but CVE are sorted by ID in CVEReportView.(open|resolved)_cve_bugtasks.

I also forgot to mention in my original MP that I think that test_open_resolved_cve_bugtasks() is quite long for unit test. I opted not to split it into a couple of smaller tests because generating the bug tasks in setUp() needs quite some time -- all three tests run in one minute on my i7 laptop, and I don't want to make that two minutes or so by splitting the long test into smaller ones.

[Abel Deuring (adeuring)]

On 19.01.2012 15:45, j.c.sackett wrote:
> Review: Approve
>
>> Should we add a docstring to the getBugCvesForBugTasks and make note of the
>> cve_mapper param as a callable method meant for that mapping?
>
> I see that you have a docstring for this method in the interface; given the complexity of what's going on, I think in the model code it may be worthwhile to simply add the usual "See `ICveSet`." docstring so people needing help quickly know where to go for it. I'll still approve, but would like this added before it lands.
>
> Your tests look pretty extensive; I concur that adding multiple bugs per cve might be useful, but as you've got reasonable testing of performance and behavior of that situation elsewhere and the cve set tests verify the implementation sufficiently Rick is correct to say it's not a blocker.
>
> One very minor point in the diff below:
>
>> === modified file 'lib/lp/bugs/model/cve.py'
>> --- lib/lp/bugs/model/cve.py 2011-12-30 06:14:56 +0000
>> +++ lib/lp/bugs/model/cve.py 2012-01-19 12:21:17 +0000
>> @@ -178,15 +182,41 @@
>> raise AssertionError('MessageChunk without content or blob.')
>> return sorted(cves, key=lambda a: a.sequence)
>>
>> - def getBugCvesForBugTasks(self, bugtasks):
>> - bug_ids = set(task.bug.id for task in bugtasks)
>> + def getBugCvesForBugTasks(self, bugtasks, cve_mapper=None):
>> + bugs = load_related(Bug, bugtasks, ('bugID', ))
>> + if len(bugs) == 0:
>> + return []
>> + bug_ids = [bug.id for bug in bugs]
>> assert bug_ids, "bugtasks must be non-empty, received %r" % bugtasks
>
> I think you can remove the assert--you return if bugs is len 0, and the
> only other thing the assert would catch would be NoneType, which would blow up
> on the len check. Obviously it's not hurting anything, but it's not really
> necessary either.

good spot! I noticed the assert too when working on the branch but then
simple forgot it...

[j.c.sackett (jcsackett)]

Thanks for removing the assert, Abel.

=== modified file 'lib/lp/bugs/browser/cvereport.py'
--- lib/lp/bugs/browser/cvereport.py	2012-01-01 02:58:52 +0000
+++ lib/lp/bugs/browser/cvereport.py	2012-01-20 17:20:32 +0000
@@ -6,23 +6,24 @@
 __metaclass__ = type
 
 __all__ = [
+    'BugTaskCve',
     'CVEReportView',
     ]
 
 from zope.component import getUtility
 
+from lp.app.browser.stringformatter import escape
 from lp.bugs.browser.bugtask import BugTaskListingItem
 from lp.bugs.interfaces.bugtask import (
     BugTaskSearchParams,
     IBugTaskSet,
     RESOLVED_BUGTASK_STATUSES,
-    UNRESOLVED_BUGTASK_STATUSES,
     )
 from lp.bugs.interfaces.cve import ICveSet
+from lp.registry.interfaces.person import IPersonSet
 from lp.services.helpers import shortlist
-from lp.services.propertycache import cachedproperty
-from lp.services.searchbuilder import any
 from lp.services.webapp import LaunchpadView
+from lp.services.webapp.publisher import canonical_url
 
 
 class BugTaskCve:
@@ -39,6 +40,19 @@
         return self.bugtasks[0].bug
 
 
+def get_cve_display_data(cve):
+    """Return the data we need for display for the given CVE."""
+    return {
+        'displayname': cve.displayname,
+        'url': canonical_url(cve),
+        }
+
+cve_link_template = (
+    '<a style="text-decoration: none" href=%s">'
+    '<img src="/@@/link" alt="" />'
+    '<span style="text-decoration: underline">%s</span></a>')
+
+
 class CVEReportView(LaunchpadView):
     """View that builds data to be displayed in CVE reports."""
 
@@ -46,38 +60,29 @@
     def page_title(self):
         return 'CVE reports for %s' % self.context.title
 
-    @cachedproperty
-    def open_cve_bugtasks(self):
-        """Find BugTaskCves for bugs with open bugtasks in the context."""
-        search_params = BugTaskSearchParams(
-            self.user, status=any(*UNRESOLVED_BUGTASK_STATUSES))
-        return self._buildBugTaskCves(search_params)
-
-    @cachedproperty
-    def resolved_cve_bugtasks(self):
-        """Find BugTaskCves for bugs with resolved bugtasks in the context."""
-        search_params = BugTaskSearchParams(
-            self.user, status=any(*RESOLVED_BUGTASK_STATUSES))
-        return self._buildBugTaskCves(search_params)
-
     def setContextForParams(self, params):
         """Update the search params for the context for a specific view."""
         raise NotImplementedError
 
-    def _buildBugTaskCves(self, search_params):
-        """Construct a list of BugTaskCve objects, sorted by bug ID."""
-        search_params.has_cve = True
+    def initialize(self):
+        """See `LaunchpadView`."""
+        super(CVEReportView, self).initialize()
+        search_params = BugTaskSearchParams(
+            self.user, has_cve=True)
         bugtasks = shortlist(
             self.context.searchTasks(search_params),
-            longest_expected=300)
+            longest_expected=600)
 
         if not bugtasks:
-            return []
+            self.open_cve_bugtasks = []
+            self.resolved_cve_bugtasks = []
+            return
 
         badge_properties = getUtility(IBugTaskSet).getBugTaskBadgeProperties(
             bugtasks)
 
-        bugtaskcves = {}
+        open_bugtaskcves = {}
+        resolved_bugtaskcves = {}
         for bugtask in bugtasks:
             badges = badge_properties[bugtask]
             # Wrap the bugtask in a BugTaskListingItem, to avoid db
@@ -87,15 +92,43 @@
                 has_bug_branch=badges['has_branch'],
                 has_specification=badges['has_specification'],
                 has_patch=badges['has_patch'])
-            if bugtask.bug.id not in bugtaskcves:
-                bugtaskcves[bugtask.bug.id] = BugTaskCve()
-            bugtaskcves[bugtask.bug.id].bugtasks.append(bugtask)
-
-        bugcves = getUtility(ICveSet).getBugCvesForBugTasks(bugtasks)
-        for bugcve in bugcves:
-            assert bugcve.bug.id in bugtaskcves, "Bug missing in bugcves."
-            bugtaskcves[bugcve.bug.id].cves.append(bugcve.cve)
-
-        # Order the dictionary items by bug ID and then return only the
+            if bugtask.status in RESOLVED_BUGTASK_STATUSES:
+                current_bugtaskcves = resolved_bugtaskcves
+            else:
+                current_bugtaskcves = open_bugtaskcves
+            if bugtask.bug.id not in current_bugtaskcves:
+                current_bugtaskcves[bugtask.bug.id] = BugTaskCve()
+            current_bugtaskcves[bugtask.bug.id].bugtasks.append(bugtask)
+
+        bugcves = getUtility(ICveSet).getBugCvesForBugTasks(
+            bugtasks, get_cve_display_data)
+        for bug, cve in bugcves:
+            if bug.id in open_bugtaskcves:
+                open_bugtaskcves[bug.id].cves.append(cve)
+            if bug.id in resolved_bugtaskcves:
+                resolved_bugtaskcves[bug.id].cves.append(cve)
+
+        # Order the dictionary items by bug ID and then store only the
         # bugtaskcve objects.
-        return [bugtaskcve for bug, bugtaskcve in sorted(bugtaskcves.items())]
+        self.open_cve_bugtasks = [
+            bugtaskcve for bug, bugtaskcve
+            in sorted(open_bugtaskcves.items())]
+        self.resolved_cve_bugtasks = [
+            bugtaskcve for bug, bugtaskcve
+            in sorted(resolved_bugtaskcves.items())]
+
+        # The page contains links to the bug task assignees:
+        # Pre-load the related Person and ValidPersonCache records
+        assignee_ids = [task.assigneeID for task in bugtasks]
+        list(getUtility(IPersonSet).getPrecachedPersonsFromIDs(
+            assignee_ids, need_validity=True))
+
+    def renderCVELinks(self, cves):
+        """Render the CVE links related to the given bug.
+
+        Doing this in a TAL expression is too inefficient for thousands
+        of CVEs.
+        """
+        return '<br />\n'.join(
+            cve_link_template % (cve['url'], escape(cve['displayname']))
+            for cve in cves)

=== added file 'lib/lp/bugs/browser/tests/test_cve.py'
--- lib/lp/bugs/browser/tests/test_cve.py	1970-01-01 00:00:00 +0000
+++ lib/lp/bugs/browser/tests/test_cve.py	2012-01-20 17:20:32 +0000
@@ -0,0 +1,176 @@
+# Copyright 2012 Canonical Ltd.  This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+"""CVE related tests."""
+
+from operator import attrgetter
+import re
+
+from lp.bugs.interfaces.bugtask import (
+    RESOLVED_BUGTASK_STATUSES,
+    UNRESOLVED_BUGTASK_STATUSES,
+    )
+from lp.bugs.browser.cvereport import BugTaskCve
+from lp.services.webapp.publisher import canonical_url
+from lp.testing import (
+    person_logged_in,
+    TestCaseWithFactory,
+    )
+from lp.testing.layers import DatabaseFunctionalLayer
+from lp.testing.views import create_initialized_view
+
+
+class TestCVEReportView(TestCaseWithFactory):
+    """Tests for CveSet."""
+
+    layer = DatabaseFunctionalLayer
+
+    def setUp(self):
+        """Create a few bugtasks and CVEs."""
+        super(TestCVEReportView, self).setUp()
+        distroseries = self.factory.makeDistroSeries()
+        self.resolved_bugtasks = []
+        self.unresolved_bugtasks = []
+        self.cves = {}
+        self.getCVE = self.cveGenerator().next
+        with person_logged_in(distroseries.owner):
+            for status in RESOLVED_BUGTASK_STATUSES:
+                tasks, cves = self.makeBugTasksWithCve(status, distroseries)
+                self.resolved_bugtasks.append(tasks)
+                self.cves[tasks[0].bug] = cves
+            for status in UNRESOLVED_BUGTASK_STATUSES:
+                tasks, cves = self.makeBugTasksWithCve(status, distroseries)
+                self.unresolved_bugtasks.append(tasks)
+                self.cves[tasks[0].bug] = cves
+        self.view = create_initialized_view(distroseries, '+cve')
+
+    def makeBugTasksWithCve(self, status, distroseries):
+        """Return two bugtasks for one bug linked to CVE."""
+        task = self.factory.makeBugTask(
+            target=self.factory.makeSourcePackage(distroseries=distroseries))
+        task.transitionToStatus(status, distroseries.owner)
+        bug = task.bug
+        task_2 = self.factory.makeBugTask(
+            target=self.factory.makeSourcePackage(distroseries=distroseries),
+            bug=bug)
+        task_2.transitionToStatus(status, distroseries.owner)
+        cve_1 = self.getCVE()
+        cve_2 = self.getCVE()
+        bug.linkCVE(cve_1, distroseries.owner)
+        bug.linkCVE(cve_2, distroseries.owner)
+        return [task, task_2], (cve_1, cve_2)
+
+    def cveGenerator(self):
+        """A generator returning five CVEs cyclically."""
+        NUM_CVES = 5
+        cves = [
+            self.factory.makeCVE('2000-%04i' % count)
+            for count in range(NUM_CVES)]
+        cve_index = 0
+        while True:
+            yield cves[cve_index]
+            cve_index = (cve_index + 1) % NUM_CVES
+
+    def test_render(self):
+        # The rendered page contains all expected CVE links.
+        html_data = self.view.render()
+        cve_links = re.findall(
+            r'<a style="text-decoration: none" '
+            r'href=http://bugs.launchpad.dev/bugs/cve/\d{4}-\d{4}">'
+            r'<img src="/@@/link" alt="" />'
+            r'<span style="text-decoration: underline">CVE-\d{4}-\d{4}</span>'
+            r'</a>',
+            html_data)
+        # We have two CVEs per bug, and one bug per bugtask status.
+        expected = 2 * (
+            len(RESOLVED_BUGTASK_STATUSES) + len(UNRESOLVED_BUGTASK_STATUSES))
+        self.assertEqual(expected, len(cve_links))
+
+    def test_open_resolved_cve_bugtasks(self):
+        # The properties CVEReportView.open_cve_bugtasks and
+        # CVEReportView.resolved_cve_bugtasks are lists of
+        # BugTaskCve instances.
+        for item in self.view.open_cve_bugtasks:
+            self.assertIsInstance(item, BugTaskCve)
+        for item in self.view.resolved_cve_bugtasks:
+            self.assertIsInstance(item, BugTaskCve)
+
+        open_bugs = [
+            bugtaskcve.bug for bugtaskcve in self.view.open_cve_bugtasks]
+        expected_open_bugs = [
+            task.bug for task, task_2 in self.unresolved_bugtasks]
+        self.assertEqual(expected_open_bugs, open_bugs)
+        resolved_bugs = [
+            bugtaskcve.bug for bugtaskcve in self.view.resolved_cve_bugtasks]
+        expected_resolved_bugs = [
+            task.bug for task, task_2 in self.resolved_bugtasks]
+        self.assertEqual(expected_resolved_bugs, resolved_bugs)
+
+        def unwrap_bugtask_listing_items(seq):
+            return [
+                [item1.bugtask, item2.bugtask] for item1, item2 in seq]
+
+        open_bugtasks = [
+            bugtaskcve.bugtasks
+            for bugtaskcve in self.view.open_cve_bugtasks]
+        open_bugtasks = unwrap_bugtask_listing_items(open_bugtasks)
+        self.assertEqual(self.unresolved_bugtasks, open_bugtasks)
+        resolved_bugtasks = [
+            bugtaskcve.bugtasks
+            for bugtaskcve in self.view.resolved_cve_bugtasks]
+        resolved_bugtasks = unwrap_bugtask_listing_items(resolved_bugtasks)
+        self.assertEqual(self.resolved_bugtasks, resolved_bugtasks)
+
+        def get_cve_data_for_task(task):
+            """Sort the CVEs for the given bug task by ID and return
+            their display data."""
+            cves = sorted(self.cves[task.bug], key=attrgetter('id'))
+            return [
+                {
+                    'url': canonical_url(cve),
+                    'displayname': cve.displayname,
+                    }
+                for cve in cves
+                ]
+
+        open_cves = [
+            bugtaskcve.cves for bugtaskcve in self.view.open_cve_bugtasks]
+        expected_open_cves = [
+            get_cve_data_for_task(task)
+            for task, task_2 in self.unresolved_bugtasks]
+        self.assertEqual(expected_open_cves, open_cves)
+
+        resolved_cves = [
+            bugtaskcve.cves for bugtaskcve in self.view.resolved_cve_bugtasks]
+        expected_resolved_cves = [
+            get_cve_data_for_task(task)
+            for task, task_2 in self.resolved_bugtasks]
+        self.assertEqual(expected_resolved_cves, resolved_cves)
+
+    def test_renderCVELinks(self):
+        # renderCVELinks() takes a sequence of items with CVE related
+        # data and returns an HTML representation with links to
+        # Launchpad pages for the CVEs.
+        result = self.view.renderCVELinks(
+            [
+                {
+                    'displayname': 'CVE-2011-0123',
+                    'url': 'http://bugs.launchpad.dev/bugs/cve/2011-0123',
+                    },
+                {
+                    'displayname': 'CVE-2011-0456',
+                    'url': 'http://bugs.launchpad.dev/bugs/cve/2011-0456',
+                    },
+                ])
+        expected = (
+            '<a style="text-decoration: none" '
+            'href=http://bugs.launchpad.dev/bugs/cve/2011-0123">'
+            '<img src="/@@/link" alt="" />'
+            '<span style="text-decoration: underline">CVE-2011-0123</span>'
+            '</a><br />\n'
+            '<a style="text-decoration: none" '
+            'href=http://bugs.launchpad.dev/bugs/cve/2011-0456">'
+            '<img src="/@@/link" alt="" />'
+            '<span style="text-decoration: underline">CVE-2011-0456</span>'
+            '</a>')
+        self.assertEqual(expected, result)

=== modified file 'lib/lp/bugs/doc/bugtask-search.txt'
--- lib/lp/bugs/doc/bugtask-search.txt	2011-12-30 06:14:56 +0000
+++ lib/lp/bugs/doc/bugtask-search.txt	2012-01-20 17:20:32 +0000
@@ -382,8 +382,8 @@
 
     >>> from lp.bugs.interfaces.cve import ICveSet
     >>> def getCves(bugtask):
-    ...     bugcve = getUtility(ICveSet).getBugCvesForBugTasks([bugtask])[0]
-    ...     return bugcve.cve.sequence
+    ...     bug, cve = getUtility(ICveSet).getBugCvesForBugTasks([bugtask])[0]
+    ...     return cve.sequence
     >>> params = BugTaskSearchParams(
     ...     has_cve=True, orderby='id', user=None)
     >>> tasks_with_cves = ubuntu.searchTasks(params)

=== modified file 'lib/lp/bugs/doc/cve.txt'
--- lib/lp/bugs/doc/cve.txt	2011-12-24 17:49:30 +0000
+++ lib/lp/bugs/doc/cve.txt	2012-01-20 17:20:32 +0000
@@ -112,7 +112,7 @@
     >>> ubuntu = Distribution.selectOneBy(name="ubuntu")
     >>> ubuntu_tasks = ubuntu.searchTasks(params)
     >>> bugcves = cveset.getBugCvesForBugTasks(ubuntu_tasks)
-    >>> [(bugcve.bug.id, bugcve.cve.title) for bugcve in bugcves]
+    >>> [(bug.id, cve.title) for (bug, cve) in bugcves]
     [(1, u'CVE-1999-8979 (Entry)'),
      (2, u'CVE-1999-2345 (Candidate)')]
 
@@ -130,11 +130,11 @@
 
     >>> for bugtaskcve in cve_report.open_cve_bugtasks:
     ...    ([bugtask.title for bugtask in bugtaskcve.bugtasks],
-    ...     [cve.title for cve in bugtaskcve.cves])
+    ...     [cve['displayname'] for cve in bugtaskcve.cves])
     ([u'Bug #1 in mozilla-firefox (Ubuntu): "Firefox does not support SVG"'],
-     [u'CVE-1999-8979 (Entry)'])
+     [u'CVE-1999-8979'])
     ([u'Bug #2 in Ubuntu: "Blackhole Trash folder"'],
-     [u'CVE-1999-2345 (Candidate)'])
+     [u'CVE-1999-2345'])
 
 There are no resolved bugtasks linked to CVEs in Ubuntu:
 

=== modified file 'lib/lp/bugs/interfaces/cve.py'
--- lib/lp/bugs/interfaces/cve.py	2011-12-24 16:54:44 +0000
+++ lib/lp/bugs/interfaces/cve.py	2012-01-20 17:20:32 +0000
@@ -110,7 +110,7 @@
         CollectionField(
             title=_('Bugs related to this CVE entry.'),
             readonly=True,
-            value_type=Reference(schema=Interface))) #  Redefined in bug.py.
+            value_type=Reference(schema=Interface))) # Redefined in bug.py.
 
     # Other attributes.
     url = exported(
@@ -181,11 +181,14 @@
         message.
         """
 
-    def getBugCvesForBugTasks(bugtasks):
-        """Return BugCve objects that correspond to the supplied bugtasks.
+    def getBugCvesForBugTasks(bugtasks, cve_mapper=None):
+        """Return (Bug, Cve) tuples that correspond to the supplied bugtasks.
 
-        Returns an iterable of BugCve objects for bugs related to the
+        Returns an iterable of (Bug, Cve) tuples for bugs related to the
         supplied sequence of bugtasks.
+
+        If a function cve_mapper is specified, a sequence of tuples
+        (bug, cve_mapper(cve)) is returned.
         """
 
     def getBugCveCount():

=== modified file 'lib/lp/bugs/model/cve.py'
--- lib/lp/bugs/model/cve.py	2011-12-30 06:14:56 +0000
+++ lib/lp/bugs/model/cve.py	2012-01-20 17:20:32 +0000
@@ -19,6 +19,8 @@
     SQLRelatedJoin,
     StringCol,
     )
+from storm.store import Store
+from storm.expr import In
 # Zope
 from zope.interface import implements
 
@@ -29,9 +31,11 @@
     ICve,
     ICveSet,
     )
+from lp.bugs.model.bug import Bug
 from lp.bugs.model.bugcve import BugCve
 from lp.bugs.model.buglinktarget import BugLinkTargetMixin
 from lp.bugs.model.cvereference import CveReference
+from lp.services.database.bulk import load_related
 from lp.services.database.constants import UTC_NOW
 from lp.services.database.datetimecol import UtcDateTimeCol
 from lp.services.database.enumcol import EnumCol
@@ -43,6 +47,7 @@
 
 cverefpat = re.compile(r'(CVE|CAN)-((19|20)\d{2}\-\d{4})')
 
+
 class Cve(SQLBase, BugLinkTargetMixin):
     """A CVE database record."""
 
@@ -148,7 +153,6 @@
         cves = set()
         for match in cverefpat.finditer(text):
             # let's get the core CVE data
-            cvestate = match.group(1)
             sequence = match.group(2)
             # see if there is already a matching CVE ref in the db, and if
             # not, then create it
@@ -178,15 +182,41 @@
                 raise AssertionError('MessageChunk without content or blob.')
         return sorted(cves, key=lambda a: a.sequence)
 
-    def getBugCvesForBugTasks(self, bugtasks):
-        bug_ids = set(task.bug.id for task in bugtasks)
-        assert bug_ids, "bugtasks must be non-empty, received %r" % bugtasks
-        return BugCve.select("""
-            BugCve.bug IN %s""" % sqlvalues(bug_ids),
-            prejoins=["cve"],
-            orderBy=['bug', 'cve'])
+    def getBugCvesForBugTasks(self, bugtasks, cve_mapper=None):
+        """See ICveSet."""
+        bugs = load_related(Bug, bugtasks, ('bugID', ))
+        if len(bugs) == 0:
+            return []
+        bug_ids = [bug.id for bug in bugs]
+
+        # Do not use BugCve instances: Storm may need a very long time
+        # to look up the bugs and CVEs referenced by a BugCve instance
+        # when the +cve view of a distroseries is rendered: There may
+        # be a few thousand (bug, CVE) tuples, while the number of bugs
+        # and CVEs is in the order of hundred. It is much more efficient
+        # to retrieve just (bug_id, cve_id) from the BugCve table and
+        # to map this to (Bug, CVE) here, instead of letting Storm
+        # look up the CVE and bug for a BugCve instance, even if bugs
+        # and CVEs are bulk loaded.
+        store = Store.of(bugtasks[0])
+        bugcve_ids = store.find(
+            (BugCve.bugID, BugCve.cveID), In(BugCve.bugID, bug_ids))
+        bugcve_ids.order_by(BugCve.bugID, BugCve.cveID)
+        bugcve_ids = list(bugcve_ids)
+
+        cve_ids = set(cve_id for bug_id, cve_id in bugcve_ids)
+        cves = store.find(Cve, In(Cve.id, list(cve_ids)))
+
+        if cve_mapper is None:
+            cvemap = dict((cve.id, cve) for cve in cves)
+        else:
+            cvemap = dict((cve.id, cve_mapper(cve)) for cve in cves)
+        bugmap = dict((bug.id, bug) for bug in bugs)
+        return [
+            (bugmap[bug_id], cvemap[cve_id])
+            for bug_id, cve_id in bugcve_ids
+            ]
 
     def getBugCveCount(self):
         """See ICveSet."""
         return BugCve.select().count()
-

=== modified file 'lib/lp/bugs/templates/bugtask-macros-cve.pt'
--- lib/lp/bugs/templates/bugtask-macros-cve.pt	2009-07-17 17:59:07 +0000
+++ lib/lp/bugs/templates/bugtask-macros-cve.pt	2012-01-20 17:20:32 +0000
@@ -20,14 +20,9 @@
               <a tal:attributes="href bugtaskcve/bug/fmt:url"
                  tal:content="string:Bug #${bugtaskcve/bug/id}: ${bugtaskcve/bug/title}">Foo Bar does not work</a>
             </td>
-            <td style="text-align: center">
-              <tal:block repeat="cve bugtaskcve/cves">
-                <a style="text-decoration: none" 
-                   tal:attributes="href cve/fmt:url"><img src="/@@/link"
-                   alt="" /><span style="text-decoration: underline"
-                                  tal:content="cve/displayname">CVE-1999-1234</span></a>
-                <br />
-              </tal:block>
+            <td style="text-align: center"
+                tal:define="content python: view.renderCVELinks(bugtaskcve.cves)"
+                tal:content="structure content">
             </td>
           </tr>
           <tal:bugtask tal:repeat="bugtask bugtaskcve/bugtasks">

=== added file 'lib/lp/bugs/tests/test_cve.py'
--- lib/lp/bugs/tests/test_cve.py	1970-01-01 00:00:00 +0000
+++ lib/lp/bugs/tests/test_cve.py	2012-01-20 17:20:32 +0000
@@ -0,0 +1,77 @@
+# Copyright 2012 Canonical Ltd.  This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+"""CVE related tests."""
+
+from zope.component import getUtility
+
+from lp.bugs.interfaces.bugtask import BugTaskSearchParams
+from lp.bugs.interfaces.cve import ICveSet
+from lp.services.webapp.testing import verifyObject
+from lp.testing import (
+    person_logged_in,
+    TestCaseWithFactory,
+    )
+from lp.testing.layers import DatabaseFunctionalLayer
+
+
+class TestCveSet(TestCaseWithFactory):
+    """Tests for CveSet."""
+
+    layer = DatabaseFunctionalLayer
+
+    def setUp(self):
+        """Create a few bugtasks and CVEs."""
+        super(TestCveSet, self).setUp()
+        self.distroseries = self.factory.makeDistroSeries()
+        self.bugs = []
+        self.cves = []
+        self.cve_index = 0
+        with person_logged_in(self.distroseries.owner):
+            for count in range(4):
+                task = self.factory.makeBugTask(target=self.distroseries)
+                bug = task.bug
+                self.bugs.append(bug)
+                cve = self.makeCVE()
+                self.cves.append(cve)
+                bug.linkCVE(cve, self.distroseries.owner)
+
+    def makeCVE(self):
+        """Create a CVE."""
+        self.cve_index += 1
+        return self.factory.makeCVE('2000-%04i' % self.cve_index)
+
+    def test_CveSet_implements_ICveSet(self):
+        cveset = getUtility(ICveSet)
+        self.assertTrue(verifyObject(ICveSet, cveset))
+
+    def test_getBugCvesForBugTasks(self):
+        # ICveSet.getBugCvesForBugTasks() returns tuples (bug, cve)
+        # for the given bugtasks.
+        bugtasks = self.distroseries.searchTasks(
+            BugTaskSearchParams(self.distroseries.owner, has_cve=True))
+        bug_cves = getUtility(ICveSet).getBugCvesForBugTasks(bugtasks)
+        found_bugs = [bug for bug, cve in bug_cves]
+        found_cves = [cve for bug, cve in bug_cves]
+        self.assertEqual(self.bugs, found_bugs)
+        self.assertEqual(self.cves, found_cves)
+
+    def test_getBugCvesForBugTasks_with_mapper(self):
+        # ICveSet.getBugCvesForBugTasks() takes a function f as an
+        # optional argeument. This function is applied to each CVE
+        # related to the given bugs; the method return a sequence of
+        # tuples (bug, f(cve)).
+        def cve_name(cve):
+            return cve.displayname
+
+        bugtasks = self.distroseries.searchTasks(
+            BugTaskSearchParams(self.distroseries.owner, has_cve=True))
+        bug_cves = getUtility(ICveSet).getBugCvesForBugTasks(
+            bugtasks, cve_name)
+        found_bugs = [bug for bug, cve in bug_cves]
+        cve_data = [cve for bug, cve in bug_cves]
+        self.assertEqual(self.bugs, found_bugs)
+        expected = [
+            u'CVE-2000-0001', u'CVE-2000-0002', u'CVE-2000-0003',
+            u'CVE-2000-0004']
+        self.assertEqual(expected, cve_data)