Merge lp:~lifeless/launchpad/bug-711071 into lp:launchpad

[Robert Collins]

[r=adeuring][bug=711071] Use aggregation rather than repeated expensive queries in bug context stats portlets.

The bugtarget stats portlet was querying each statistic separately; this walks over the same bugs many times and is thus terribly slow. Doing the most expensive examine-all-bugs case and deriving the other information from it is much faster - so this patch does that for the most expensive properties. See the linked bug for analysis of which things are grouped and why.

[Abel Deuring (adeuring)]

review: Approve (code)

=== modified file 'lib/lp/bugs/browser/bugtask.py'
--- lib/lp/bugs/browser/bugtask.py	2011-03-18 15:19:34 +0000
+++ lib/lp/bugs/browser/bugtask.py	2011-03-20 22:37:28 +0000
@@ -77,6 +77,7 @@
 from lazr.uri import URI
 from pytz import utc
 from simplejson import dumps
+from storm.expr import SQL
 from z3c.ptcompat import ViewPageTemplateFile
 from zope import (
     component,
@@ -247,6 +248,8 @@
 from lp.bugs.interfaces.bugwatch import BugWatchActivityStatus
 from lp.bugs.interfaces.cve import ICveSet
 from lp.bugs.interfaces.malone import IMaloneApplication
+from lp.bugs.model.bug import Bug
+from lp.bugs.model.bugtask import BugTask
 from lp.registry.interfaces.distribution import (
     IDistribution,
     IDistributionSet,
@@ -274,6 +277,21 @@
     )
 
 
+DISPLAY_BUG_STATUS_FOR_PATCHES = {
+    BugTaskStatus.NEW:  True,
+    BugTaskStatus.INCOMPLETE: True,
+    BugTaskStatus.INVALID: False,
+    BugTaskStatus.WONTFIX: False,
+    BugTaskStatus.CONFIRMED: True,
+    BugTaskStatus.TRIAGED: True,
+    BugTaskStatus.INPROGRESS: True,
+    BugTaskStatus.FIXCOMMITTED: True,
+    BugTaskStatus.FIXRELEASED: False,
+    BugTaskStatus.UNKNOWN: False,
+    BugTaskStatus.EXPIRED: False
+    }
+
+
 @component.adapter(IBugTask, IReference, IWebServiceClientRequest)
 @implementer(IFieldHTMLRenderer)
 def bugtarget_renderer(context, field, request):
@@ -1765,12 +1783,54 @@
     These can be expensive to obtain.
     """
 
+    @cachedproperty
+    def _bug_stats(self):
+        bug_task_set = getUtility(IBugTaskSet)
+        upstream_open_bugs = bug_task_set.open_bugtask_search
+        upstream_open_bugs.setTarget(self.context)
+        upstream_open_bugs.resolved_upstream = True
+        fixed_upstream_clause = SQL(
+            bug_task_set.buildUpstreamClause(upstream_open_bugs))
+        open_bugs = bug_task_set.open_bugtask_search
+        open_bugs.setTarget(self.context)
+        groups = (BugTask.status, BugTask.importance,
+            Bug.latest_patch_uploaded != None, fixed_upstream_clause)
+        counts = bug_task_set.countBugs(open_bugs, groups)
+        # Sum the split out aggregates.
+        new = 0
+        open = 0
+        inprogress = 0
+        critical = 0
+        high = 0
+        with_patch = 0
+        resolved_upstream = 0
+        for metadata, count in counts.items():
+            status = metadata[0]
+            importance = metadata[1]
+            has_patch = metadata[2]
+            was_resolved_upstream = metadata[3]
+            if status == BugTaskStatus.NEW:
+                new += count
+            elif status == BugTaskStatus.INPROGRESS:
+                inprogress += count
+            if importance == BugTaskImportance.CRITICAL:
+                critical += count
+            elif importance == BugTaskImportance.HIGH:
+                high += count
+            if has_patch and DISPLAY_BUG_STATUS_FOR_PATCHES[status]:
+                with_patch += count
+            if was_resolved_upstream:
+                resolved_upstream += count
+            open += count
+        result = dict(new=new, open=open, inprogress=inprogress, high=high,
+            critical=critical, with_patch=with_patch,
+            resolved_upstream=resolved_upstream)
+        return result
+
     @property
     def bugs_fixed_elsewhere_count(self):
         """A count of bugs fixed elsewhere."""
-        params = get_default_search_params(self.user)
-        params.resolved_upstream = True
-        return self.context.searchTasks(params).count()
+        return self._bug_stats['resolved_upstream']
 
     @property
     def open_cve_bugs_count(self):
@@ -1813,27 +1873,27 @@
     @property
     def new_bugs_count(self):
         """A count of new bugs."""
-        return self.context.new_bugtasks.count()
+        return self._bug_stats['new']
 
     @property
     def open_bugs_count(self):
         """A count of open bugs."""
-        return self.context.open_bugtasks.count()
+        return self._bug_stats['open']
 
     @property
     def inprogress_bugs_count(self):
         """A count of in-progress bugs."""
-        return self.context.inprogress_bugtasks.count()
+        return self._bug_stats['inprogress']
 
     @property
     def critical_bugs_count(self):
         """A count of critical bugs."""
-        return self.context.critical_bugtasks.count()
+        return self._bug_stats['critical']
 
     @property
     def high_bugs_count(self):
         """A count of high priority bugs."""
-        return self.context.high_bugtasks.count()
+        return self._bug_stats['high']
 
     @property
     def my_bugs_count(self):
@@ -1848,10 +1908,7 @@
     @property
     def bugs_with_patches_count(self):
         """A count of unresolved bugs with patches."""
-        return self.context.searchTasks(
-            None, user=self.user,
-            status=UNRESOLVED_BUGTASK_STATUSES,
-            omit_duplicates=True, has_patch=True).count()
+        return self._bug_stats['with_patch']
 
 
 class BugListingPortletInfoView(LaunchpadView, BugsInfoMixin):

=== modified file 'lib/lp/bugs/browser/tests/special/bugs-fixed-elsewhere.txt'
--- lib/lp/bugs/browser/tests/special/bugs-fixed-elsewhere.txt	2010-10-09 16:36:22 +0000
+++ lib/lp/bugs/browser/tests/special/bugs-fixed-elsewhere.txt	2011-03-20 22:37:28 +0000
@@ -21,11 +21,14 @@
 calculate, so it is put on this separate view which can be requested
 asyncronously.
 
-    >>> view = getMultiAdapter(
-    ...     (bugtarget, LaunchpadTestRequest()),
-    ...     name='+bugtarget-portlet-bugfilters-stats')
-    >>> view.initialize()
+    >>> def get_view():
+    ...     view = getMultiAdapter(
+    ...         (bugtarget, LaunchpadTestRequest()),
+    ...         name='+bugtarget-portlet-bugfilters-stats')
+    ...     view.initialize()
+    ...     return view
 
+    >>> view = get_view()
     >>> view.bugs_fixed_elsewhere_url
     u'http://.../+bugs?field.status_upstream=resolved_upstream'
     >>> view.bugs_fixed_elsewhere_count
@@ -43,7 +46,7 @@
     >>> from lp.bugs.interfaces.bugtask import IBugTaskSet
     >>> elsewhere = getUtility(IBugTaskSet).createTask(
     ...     bug, owner=getUtility(ILaunchBag).user, product=evolution)
-    >>> view.bugs_fixed_elsewhere_count
+    >>> get_view().bugs_fixed_elsewhere_count
     0
 
 But if we mark the bug as fixed in the other, the count will increase
@@ -55,8 +58,8 @@
     ...     BugTaskStatus.FIXRELEASED, getUtility(ILaunchBag).user)
     >>> syncUpdate(elsewhere)
 
-    >>> view.bugs_fixed_elsewhere_count
-    1
+    >>> get_view().bugs_fixed_elsewhere_count
+    1L
 
 Bugs fixed elsewhere also show up when we perform an advanced bug
 search, using the appropriate query string parameter to ask for "bugs
@@ -93,20 +96,15 @@
     ...     BugTaskStatus.FIXRELEASED, getUtility(ILaunchBag).user)
     >>> syncUpdate(another_elsewhere)
 
-    >>> view.bugs_fixed_elsewhere_count
-    2
+    >>> get_view().bugs_fixed_elsewhere_count
+    2L
 
 This means that No Privileges Person will see that there is only one bug
 fixed elsewhere.
 
     >>> login('[e-mail]')
-    >>> view = getMultiAdapter(
-    ...     (bugtarget, LaunchpadTestRequest()),
-    ...     name='+bugtarget-portlet-bugfilters-stats')
-    >>> view.initialize()
-
-    >>> view.bugs_fixed_elsewhere_count
-    1
+    >>> get_view().bugs_fixed_elsewhere_count
+    1L
 
 If the private bug is made public again, he will of course see that
 there are two bugs fixed.
@@ -117,8 +115,8 @@
     >>> syncUpdate(another_bug)
 
     >>> login('[e-mail]')
-    >>> view.bugs_fixed_elsewhere_count
-    2
+    >>> get_view().bugs_fixed_elsewhere_count
+    2L
 
 
 == Duplicate Bugs ==
@@ -128,8 +126,8 @@
     >>> another_bug.markAsDuplicate(bug)
     >>> syncUpdate(another_bug)
 
-    >>> view.bugs_fixed_elsewhere_count
-    1
+    >>> get_view().bugs_fixed_elsewhere_count
+    1L
 
 
 == Resolved Bugs ==
@@ -145,5 +143,5 @@
     ...     BugTaskStatus.FIXRELEASED, getUtility(ILaunchBag).user)
     >>> syncUpdate(bugtask)
 
-    >>> view.bugs_fixed_elsewhere_count
+    >>> get_view().bugs_fixed_elsewhere_count
     0

=== modified file 'lib/lp/bugs/browser/tests/test_bugtarget_patches_view.py'
--- lib/lp/bugs/browser/tests/test_bugtarget_patches_view.py	2010-10-04 19:50:45 +0000
+++ lib/lp/bugs/browser/tests/test_bugtarget_patches_view.py	2011-03-20 22:37:28 +0000
@@ -10,26 +10,14 @@
 from canonical.launchpad.webapp.servers import LaunchpadTestRequest
 from canonical.testing.layers import LaunchpadFunctionalLayer
 from lp.bugs.browser.bugtarget import BugsPatchesView
-from lp.bugs.browser.bugtask import BugListingPortletStatsView
+from lp.bugs.browser.bugtask import (
+    BugListingPortletStatsView,
+    DISPLAY_BUG_STATUS_FOR_PATCHES,
+    )
 from lp.bugs.interfaces.bugtask import BugTaskStatus
 from lp.testing import TestCaseWithFactory
 
 
-DISPLAY_BUG_STATUS_FOR_PATCHES = {
-    BugTaskStatus.NEW:  True,
-    BugTaskStatus.INCOMPLETE: True,
-    BugTaskStatus.INVALID: False,
-    BugTaskStatus.WONTFIX: False,
-    BugTaskStatus.CONFIRMED: True,
-    BugTaskStatus.TRIAGED: True,
-    BugTaskStatus.INPROGRESS: True,
-    BugTaskStatus.FIXCOMMITTED: True,
-    BugTaskStatus.FIXRELEASED: False,
-    BugTaskStatus.UNKNOWN: False,
-    BugTaskStatus.EXPIRED: False
-    }
-
-
 class TestBugTargetPatchCountBase(TestCaseWithFactory):
 
     layer = LaunchpadFunctionalLayer
@@ -48,10 +36,6 @@
 
 class TestBugTargetPatchView(TestBugTargetPatchCountBase):
 
-    def setUp(self):
-        super(TestBugTargetPatchView, self).setUp()
-        self.view = BugsPatchesView(self.product, LaunchpadTestRequest())
-
     def test_status_of_bugs_with_patches_shown(self):
         # Bugs with patches that have the status FIXRELEASED, INVALID,
         # WONTFIX, UNKNOWN, EXPIRED are not shown in the +patches view; all
@@ -61,7 +45,8 @@
             if DISPLAY_BUG_STATUS_FOR_PATCHES[bugtask_status]:
                 number_of_bugs_shown += 1
             self.makeBugWithPatch(bugtask_status)
-            batched_tasks = self.view.batchedPatchTasks()
+            view = BugsPatchesView(self.product, LaunchpadTestRequest())
+            batched_tasks = view.batchedPatchTasks()
             self.assertEqual(
                 batched_tasks.batch.listlength, number_of_bugs_shown,
                 "Unexpected number of bugs with patches displayed for status "
@@ -70,11 +55,6 @@
 
 class TestBugListingPortletStatsView(TestBugTargetPatchCountBase):
 
-    def setUp(self):
-        super(TestBugListingPortletStatsView, self).setUp()
-        self.view = BugListingPortletStatsView(
-            self.product, LaunchpadTestRequest())
-
     def test_bugs_with_patches_count(self):
         # Bugs with patches that have the status FIXRELEASED, INVALID,
         # WONTFIX, or UNKNOWN are not counted in
@@ -85,18 +65,9 @@
             if DISPLAY_BUG_STATUS_FOR_PATCHES[bugtask_status]:
                 number_of_bugs_shown += 1
             self.makeBugWithPatch(bugtask_status)
+            view = BugListingPortletStatsView(
+                self.product, LaunchpadTestRequest())
             self.assertEqual(
-                self.view.bugs_with_patches_count, number_of_bugs_shown,
+                view.bugs_with_patches_count, number_of_bugs_shown,
                 "Unexpected number of bugs with patches displayed for status "
                 "%s" % bugtask_status)
-
-
-def test_suite():
-    suite = unittest.TestSuite()
-    suite.addTest(unittest.makeSuite(TestBugTargetPatchView))
-    suite.addTest(unittest.makeSuite(TestBugListingPortletStatsView))
-    return suite
-
-
-if __name__ == '__main__':
-    unittest.TextTestRunner().run(test_suite())

=== modified file 'lib/lp/bugs/configure.zcml'
--- lib/lp/bugs/configure.zcml	2011-03-16 13:26:30 +0000
+++ lib/lp/bugs/configure.zcml	2011-03-20 22:37:28 +0000
@@ -309,6 +309,11 @@
                 attributes="
                     setTarget
                     "/>
+            <require
+                permission="zope.Public"
+                set_attributes="
+                    resolved_upstream
+                    "/>
         </class>
 
         <!-- BugTaskSet -->

=== modified file 'lib/lp/bugs/interfaces/bugtask.py'
--- lib/lp/bugs/interfaces/bugtask.py	2011-03-17 15:25:04 +0000
+++ lib/lp/bugs/interfaces/bugtask.py	2011-03-20 22:37:28 +0000
@@ -1274,21 +1274,37 @@
             supported.
         """
         # Yay circular deps.
+        from lp.registry.interfaces.distribution import IDistribution
         from lp.registry.interfaces.distroseries import IDistroSeries
+        from lp.registry.interfaces.product import IProduct
         from lp.registry.interfaces.productseries import IProductSeries
         from lp.registry.interfaces.milestone import IMilestone
+        from lp.registry.interfaces.projectgroup import IProjectGroup
+        from lp.registry.interfaces.sourcepackage import ISourcePackage
+        from lp.registry.interfaces.distributionsourcepackage import \
+            IDistributionSourcePackage
         if isinstance(target, (any, all)):
             assert len(target.query_values), \
                 'cannot determine target with no targets'
             instance = target.query_values[0]
         else:
             instance = target
-        if IDistroSeries.providedBy(instance):
+        if IDistribution.providedBy(instance):
+            self.setDistribution(target)
+        elif IDistroSeries.providedBy(instance):
             self.setDistroSeries(target)
+        elif IProduct.providedBy(instance):
+            self.setProduct(target)
         elif IProductSeries.providedBy(instance):
             self.setProductSeries(target)
         elif IMilestone.providedBy(instance):
             self.milestone = target
+        elif ISourcePackage.providedBy(instance):
+            self.setSourcePackage(target)
+        elif IDistributionSourcePackage.providedBy(instance):
+            self.setSourcePackage(target)
+        elif IProjectGroup.providedBy(instance):
+            self.setProject(target)
         else:
             raise AssertionError("unknown target type %r" % target)
 
@@ -1600,6 +1616,12 @@
 
     open_bugtask_search = Attribute("A search returning open bugTasks.")
 
+    def buildUpstreamClause(params):
+        """Create a SQL clause to do upstream checks in a bug search.
+        
+        :return: A string SQL expression.
+        """
+
 
 
 def valid_remote_bug_url(value):

=== modified file 'lib/lp/bugs/model/bugtask.py'
--- lib/lp/bugs/model/bugtask.py	2011-03-18 15:19:34 +0000
+++ lib/lp/bugs/model/bugtask.py	2011-03-20 22:37:28 +0000
@@ -1690,6 +1690,14 @@
                     "project group, or distribution")
         return (join_tables, extra_clauses)
 
+    def _require_params(self, params):
+        assert zope_isinstance(params, BugTaskSearchParams)
+        if not isinstance(params, BugTaskSearchParams):
+            # Browser code let this get wrapped, unwrap it here as its just a
+            # dumb data store that has no security implications.
+            params = removeSecurityProxy(params)
+        return params
+
     def buildQuery(self, params):
         """Build and return an SQL query with the given parameters.
 
@@ -1698,11 +1706,7 @@
         :return: A query, the tables to query, ordering expression and a
             decorator to call on each returned row.
         """
-        assert zope_isinstance(params, BugTaskSearchParams)
-        if not isinstance(params, BugTaskSearchParams):
-            # Browser code let this get wrapped, unwrap it here as its just a
-            # dumb data store that has no security implications.
-            params = removeSecurityProxy(params)
+        params = self._require_params(params)
         from lp.bugs.model.bug import Bug
         extra_clauses = ['Bug.id = BugTask.bug']
         clauseTables = [BugTask, Bug]
@@ -1934,7 +1938,7 @@
                 """BugTask.sourcepackagename in (
                     select sourcepackagename from spns)""")
 
-        upstream_clause = self._buildUpstreamClause(params)
+        upstream_clause = self.buildUpstreamClause(params)
         if upstream_clause:
             extra_clauses.append(upstream_clause)
 
@@ -2085,12 +2089,13 @@
             query, clauseTables, orderby_arg, decorator, join_tables,
             has_duplicate_results, with_clause)
 
-    def _buildUpstreamClause(self, params):
+    def buildUpstreamClause(self, params):
         """Return an clause for returning upstream data if the data exists.
 
         This method will handles BugTasks that do not have upstream BugTasks
         as well as thoses that do.
         """
+        params = self._require_params(params)
         upstream_clauses = []
         if params.pending_bugwatch_elsewhere:
             if params.product: