← Back to team overview

launchpad-reviewers team mailing list archive

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

 

Abel Deuring has proposed merging lp:~adeuring/launchpad/bug-904461 into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~adeuring/launchpad/bug-904461/+merge/89230

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.

-- 
https://code.launchpad.net/~adeuring/launchpad/bug-904461/+merge/89230
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~adeuring/launchpad/bug-904461 into lp:launchpad.
=== 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-19 12:21:17 +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,14 @@
         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),
+        }
+
+
 class CVEReportView(LaunchpadView):
     """View that builds data to be displayed in CVE reports."""
 
@@ -46,30 +55,18 @@
     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 []
@@ -77,7 +74,8 @@
         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 +85,48 @@
                 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.
+        """
+        cve_link_template = (
+            '<a style="text-decoration: none" href=%s">'
+            '<img src="/@@/link" alt="" />'
+            '<span style="text-decoration: underline">%s</span></a>')
+
+        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-19 12:21:17 +0000
@@ -0,0 +1,163 @@
+# Copyright 2012 Canonical Ltd.  This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+"""CVE related tests."""
+
+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.cve_index = 0
+        with person_logged_in(distroseries.owner):
+            for status in RESOLVED_BUGTASK_STATUSES:
+                tasks, cve = self.makeBugTasksWithCve(status, distroseries)
+                self.resolved_bugtasks.append(tasks)
+                self.cves[tasks[0].bug] = cve
+            for status in UNRESOLVED_BUGTASK_STATUSES:
+                tasks, cve = self.makeBugTasksWithCve(status, distroseries)
+                self.unresolved_bugtasks.append(tasks)
+                self.cves[tasks[0].bug] = cve
+        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 = self.makeCVE()
+        bug.linkCVE(cve, distroseries.owner)
+        return [task, task_2], cve
+
+    def makeCVE(self):
+        """Create a CVE."""
+        self.cve_index += 1
+        return self.factory.makeCVE('2000-%04i' % self.cve_index)
+
+    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)
+        self.assertEqual(len(self.cves), 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)
+
+        open_cves = [
+            bugtaskcve.cves for bugtaskcve in self.view.open_cve_bugtasks]
+        expected_open_cves = [
+            self.cves[task.bug] for task, task_2 in self.unresolved_bugtasks]
+        expected_open_cves = [
+            [{
+                'url': canonical_url(cve),
+                'displayname': cve.displayname,
+                }]
+            for cve in expected_open_cves
+            ]
+        self.assertEqual(expected_open_cves, open_cves)
+        resolved_cves = [
+            bugtaskcve.cves for bugtaskcve in self.view.resolved_cve_bugtasks]
+        expected_resolved_cves = [
+            self.cves[task.bug] for task, task_2 in self.resolved_bugtasks]
+        expected_resolved_cves = [
+            [{
+                'url': canonical_url(cve),
+                'displayname': cve.displayname,
+                }]
+            for cve in expected_resolved_cves
+            ]
+        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-19 12:21:17 +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-19 12:21:17 +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)')]
 

=== 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-19 12:21:17 +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-19 12:21:17 +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)
+    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
-        return BugCve.select("""
-            BugCve.bug IN %s""" % sqlvalues(bug_ids),
-            prejoins=["cve"],
-            orderBy=['bug', 'cve'])
+
+        # 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-19 12:21:17 +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-19 12:21:17 +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)