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

[Abel Deuring]

This branch is a first step to fix bug 675595:
BugTaskSet.getAssignedMilestonesFromSearch() can call BugTaskSet._search()
directly and retrieve the milestones with one SQL query instead of two.

It adds an optional parameter "prejoins" to BugTaskSet.search()
and to HasBugs.searchTasks().

BugTaskSet.search() already prejoined a number of tables in order to
reduce the total query count; in certain cases it makes sense to
prejoin other tables too.

tests:

./bin/test -vvt test_bugtarget -t test_bugtask_search

no lint

[Graham Binns (gmb)]

review: Approve (code)

=== modified file 'lib/lp/bugs/interfaces/bugtarget.py'
--- lib/lp/bugs/interfaces/bugtarget.py	2010-10-21 16:40:10 +0000
+++ lib/lp/bugs/interfaces/bugtarget.py	2010-11-23 13:40:38 +0000
@@ -82,7 +82,8 @@
     "omit_duplicates": copy_field(IBugTaskSearch['omit_dupes']),
     "omit_targeted": copy_field(IBugTaskSearch['omit_targeted']),
     "status_upstream": copy_field(IBugTaskSearch['status_upstream']),
-    "milestone_assignment": copy_field(IBugTaskSearch['milestone_assignment']),
+    "milestone_assignment": copy_field(
+        IBugTaskSearch['milestone_assignment']),
     "milestone": copy_field(IBugTaskSearch['milestone']),
     "component": copy_field(IBugTaskSearch['component']),
     "nominated_for": Reference(schema=Interface),
@@ -232,7 +233,7 @@
                     hardware_owner_is_subscribed_to_bug=False,
                     hardware_is_linked_to_bug=False, linked_branches=None,
                     structural_subscriber=None, modified_since=None,
-                    created_since=None):
+                    created_since=None, prejoins=[]):
         """Search the IBugTasks reported on this entity.
 
         :search_params: a BugTaskSearchParams object
@@ -337,8 +338,10 @@
     :istargeted: Is there a fix targeted to this series?
     :sourcepackage: The sourcepackage to which the fix would be targeted.
     :assignee: An IPerson, or None if no assignee.
-    :status: A BugTaskStatus dbschema item, or None, if series is not targeted.
+    :status: A BugTaskStatus dbschema item, or None, if series is not
+        targeted.
     """
+
     def __init__(self, series, istargeted=False, sourcepackage=None,
                  assignee=None, status=None):
         self.series = series

=== modified file 'lib/lp/bugs/interfaces/bugtask.py'
--- lib/lp/bugs/interfaces/bugtask.py	2010-11-03 16:50:05 +0000
+++ lib/lp/bugs/interfaces/bugtask.py	2010-11-23 13:40:38 +0000
@@ -667,10 +667,11 @@
         underlying bug for this bugtask.
 
         This method was documented as being required here so that
-        MentorshipOffers could happen on IBugTask. If that was the sole reason
-        then this method should be deletable.  When we move to context-less bug
-        presentation (where the bug is at /bugs/n?task=ubuntu) then we can
-        eliminate this if it is no longer useful.
+        MentorshipOffers could happen on IBugTask. If that was the sole
+        reason then this method should be deletable.  When we move to
+        context-less bug presentation (where the bug is at
+        /bugs/n?task=ubuntu) then we can eliminate this if it is no
+        longer useful.
         """
 
     @mutator_for(milestone)
@@ -1387,15 +1388,18 @@
         Only BugTasks that the user has access to will be returned.
         """
 
-    def search(params, *args):
+    def search(params, *args, **kwargs):
         """Search IBugTasks with the given search parameters.
 
         Note: only use this method of BugTaskSet if you want to query
         tasks across multiple IBugTargets; otherwise, use the
         IBugTarget's searchTasks() method.
 
-        :search_params: a BugTaskSearchParams object
-        :args: any number of BugTaskSearchParams objects
+        :param search_params: a BugTaskSearchParams object
+        :param args: any number of BugTaskSearchParams objects
+        :param prejoins: (keyword) A sequence of tuples
+            (table, table_join) which should be pre-joined in addition
+            to the default prejoins.
 
         If more than one BugTaskSearchParams is given, return the union of
         IBugTasks which match any of them, with the results ordered by the

=== modified file 'lib/lp/bugs/model/bugtarget.py'
--- lib/lp/bugs/model/bugtarget.py	2010-10-21 16:40:10 +0000
+++ lib/lp/bugs/model/bugtarget.py	2010-11-23 13:40:38 +0000
@@ -72,6 +72,7 @@
     All `IHasBugs` implementations should inherit from this class
     or from `BugTargetBase`.
     """
+
     def searchTasks(self, search_params, user=None,
                     order_by=None, search_text=None,
                     status=None,
@@ -93,7 +94,7 @@
                     hardware_owner_is_affected_by_bug=False,
                     hardware_owner_is_subscribed_to_bug=False,
                     hardware_is_linked_to_bug=False, linked_branches=None,
-                    modified_since=None, created_since=None):
+                    modified_since=None, created_since=None, prejoins=[]):
         """See `IHasBugs`."""
         if status is None:
             # If no statuses are supplied, default to the
@@ -109,9 +110,10 @@
             del kwargs['self']
             del kwargs['user']
             del kwargs['search_params']
+            del kwargs['prejoins']
             search_params = BugTaskSearchParams.fromSearchForm(user, **kwargs)
         self._customizeSearchParams(search_params)
-        return BugTaskSet().search(search_params)
+        return BugTaskSet().search(search_params, prejoins=prejoins)
 
     def _customizeSearchParams(self, search_params):
         """Customize `search_params` for a specific target."""
@@ -328,7 +330,6 @@
             self.project.recalculateBugHeatCache()
 
 
-
 class OfficialBugTagTargetMixin:
     """See `IOfficialBugTagTarget`.
 
@@ -441,4 +442,3 @@
                 'IDistribution instance or an IProduct instance.')
 
     target = property(target, _settarget, doc=target.__doc__)
-

=== modified file 'lib/lp/bugs/model/bugtask.py'
--- lib/lp/bugs/model/bugtask.py	2010-11-18 13:09:22 +0000
+++ lib/lp/bugs/model/bugtask.py	2010-11-23 13:40:38 +0000
@@ -2277,6 +2277,9 @@
         :param _noprejoins: Private internal parameter to BugTaskSet which
             disables all use of prejoins : consolidated from code paths that
             claim they were inefficient and unwanted.
+        :param prejoins: A sequence of tuples (table, table_join) which
+            which should be pre-joined in addition to the default prejoins.
+            This parameter has no effect if _noprejoins is True.
         """
         # Prevent circular import problems.
         from lp.registry.model.product import Product
@@ -2286,6 +2289,7 @@
             prejoins = []
             resultrow = BugTask
         else:
+            requested_joins = kwargs.get('prejoins', [])
             prejoins = [
                 (Bug, LeftJoin(Bug, BugTask.bug == Bug.id)),
                 (Product, LeftJoin(Product, BugTask.product == Product.id)),
@@ -2293,8 +2297,12 @@
                  LeftJoin(
                      SourcePackageName,
                      BugTask.sourcepackagename == SourcePackageName.id)),
-                ]
-            resultrow = (BugTask, Bug, Product, SourcePackageName, )
+                ] + requested_joins
+            resultrow = (BugTask, Bug, Product, SourcePackageName)
+            additional_result_objects = [
+                table for table, join in requested_joins
+                if table not in resultrow]
+            resultrow = resultrow + tuple(additional_result_objects)
         return self._search(resultrow, prejoins, params, *args)
 
     def searchBugIds(self, params):

=== modified file 'lib/lp/bugs/tests/test_bugtarget.py'
--- lib/lp/bugs/tests/test_bugtarget.py	2010-10-04 19:50:45 +0000
+++ lib/lp/bugs/tests/test_bugtarget.py	2010-11-23 13:40:38 +0000
@@ -15,8 +15,11 @@
 __all__ = []
 
 import random
+from testtools.matchers import Equals
 import unittest
 
+from storm.expr import LeftJoin
+from storm.store import Store
 from zope.component import getUtility
 
 from canonical.launchpad.testing.systemdocs import (
@@ -28,15 +31,24 @@
 from canonical.testing.layers import LaunchpadFunctionalLayer
 from lp.bugs.interfaces.bug import CreateBugParams
 from lp.bugs.interfaces.bugtask import (
+    BugTaskSearchParams,
     BugTaskStatus,
     IBugTaskSet,
     )
+from lp.bugs.model.bugtask import BugTask
 from lp.registry.interfaces.distribution import (
     IDistribution,
     IDistributionSet,
     )
 from lp.registry.interfaces.product import IProductSet
 from lp.registry.interfaces.projectgroup import IProjectGroupSet
+from lp.registry.model.milestone import Milestone
+from lp.testing import (
+    person_logged_in,
+    StormStatementRecorder,
+    TestCaseWithFactory,
+    )
+from lp.testing.matchers import HasQueryCount
 
 
 def bugtarget_filebug(bugtarget, summary, status=None):
@@ -176,6 +188,101 @@
     test.globs['question_target'] = ubuntu.getSourcePackage('mozilla-firefox')
 
 
+class TestBugTargetSearchTasks(TestCaseWithFactory):
+    """Tests of IHasBugs.searchTasks()."""
+
+    layer = LaunchpadFunctionalLayer
+
+    def setUp(self):
+        super(TestBugTargetSearchTasks, self).setUp()
+        self.bug = self.factory.makeBug()
+        self.target = self.bug.default_bugtask.target
+        self.milestone = self.factory.makeMilestone(product=self.target)
+        with person_logged_in(self.target.owner):
+            self.bug.default_bugtask.transitionToMilestone(
+                self.milestone, self.target.owner)
+        self.store = Store.of(self.bug)
+        self.store.flush()
+        self.store.invalidate()
+
+    def test_preload_other_objects(self):
+        # We can prejoin objects in calls of searchTasks().
+
+        # Without prejoining the table Milestone, accessing the
+        # BugTask property milestone requires an extra query.
+        with StormStatementRecorder() as recorder:
+            params = BugTaskSearchParams(user=None)
+            found_tasks = self.target.searchTasks(params)
+            found_tasks[0].milestone
+        self.assertThat(recorder, HasQueryCount(Equals(2)))
+
+        # When we prejoin Milestone, the milestone of our bugtask is
+        # already loaded during the main search query.
+        self.store.invalidate()
+        with StormStatementRecorder() as recorder:
+            params = BugTaskSearchParams(user=None)
+            prejoins = [(Milestone,
+                         LeftJoin(Milestone,
+                                  BugTask.milestone == Milestone.id))]
+            found_tasks = self.target.searchTasks(params, prejoins=prejoins)
+            found_tasks[0].milestone
+        self.assertThat(recorder, HasQueryCount(Equals(1)))
+
+    def test_preload_other_objects_for_person_search_no_params_passed(self):
+        # We can prejoin objects in calls of Person.searchTasks().
+        owner = self.bug.owner
+        with StormStatementRecorder() as recorder:
+            found_tasks = owner.searchTasks(None, user=None)
+            found_tasks[0].milestone
+        self.assertThat(recorder, HasQueryCount(Equals(2)))
+
+        self.store.invalidate()
+        with StormStatementRecorder() as recorder:
+            prejoins = [(Milestone,
+                         LeftJoin(Milestone,
+                                  BugTask.milestone == Milestone.id))]
+            found_tasks = owner.searchTasks(
+                None, user=None, prejoins=prejoins)
+            found_tasks[0].milestone
+        self.assertThat(recorder, HasQueryCount(Equals(1)))
+
+    def test_preload_other_objects_for_person_search_no_keywords_passed(self):
+        # We can prejoin objects in calls of Person.searchTasks().
+        owner = self.bug.owner
+        params = BugTaskSearchParams(user=None, owner=owner)
+        with StormStatementRecorder() as recorder:
+            found_tasks = owner.searchTasks(params)
+            found_tasks[0].milestone
+        self.assertThat(recorder, HasQueryCount(Equals(2)))
+
+        self.store.invalidate()
+        with StormStatementRecorder() as recorder:
+            prejoins = [(Milestone,
+                         LeftJoin(Milestone,
+                                  BugTask.milestone == Milestone.id))]
+            found_tasks = owner.searchTasks(params, prejoins=prejoins)
+            found_tasks[0].milestone
+        self.assertThat(recorder, HasQueryCount(Equals(1)))
+
+    def test_preload_other_objects_for_person_search_keywords_passed(self):
+        # We can prejoin objects in calls of Person.searchTasks().
+        owner = self.bug.owner
+        params = BugTaskSearchParams(user=None, owner=owner)
+        with StormStatementRecorder() as recorder:
+            found_tasks = owner.searchTasks(params, order_by=BugTask.id)
+            found_tasks[0].milestone
+        self.assertThat(recorder, HasQueryCount(Equals(2)))
+
+        self.store.invalidate()
+        with StormStatementRecorder() as recorder:
+            prejoins = [(Milestone,
+                         LeftJoin(Milestone,
+                                  BugTask.milestone == Milestone.id))]
+            found_tasks = owner.searchTasks(params, prejoins=prejoins)
+            found_tasks[0].milestone
+        self.assertThat(recorder, HasQueryCount(Equals(1)))
+
+
 def test_suite():
     """Return the `IBugTarget` TestSuite."""
     suite = unittest.TestSuite()
@@ -195,7 +302,6 @@
             layer=LaunchpadFunctionalLayer)
         suite.addTest(test)
 
-
     setUpMethods.remove(sourcePackageForQuestionSetUp)
     setUpMethods.append(sourcePackageSetUp)
     setUpMethods.append(projectSetUp)
@@ -206,4 +312,5 @@
             layer=LaunchpadFunctionalLayer)
         suite.addTest(test)
 
+    suite.addTest(unittest.TestLoader().loadTestsFromName(__name__))
     return suite

=== modified file 'lib/lp/bugs/tests/test_bugtask_search.py'
--- lib/lp/bugs/tests/test_bugtask_search.py	2010-11-12 17:42:43 +0000
+++ lib/lp/bugs/tests/test_bugtask_search.py	2010-11-23 13:40:38 +0000
@@ -10,10 +10,14 @@
 from new import classobj
 import pytz
 import sys
+from testtools.matchers import Equals
 import unittest
 
 from zope.component import getUtility
 
+from storm.expr import Join
+from storm.store import Store
+
 from canonical.launchpad.searchbuilder import (
     all,
     any,
@@ -31,6 +35,7 @@
     BugTaskStatus,
     IBugTaskSet,
     )
+from lp.bugs.model.bugtask import BugTask
 from lp.registry.interfaces.distribution import IDistribution
 from lp.registry.interfaces.distributionsourcepackage import (
     IDistributionSourcePackage,
@@ -39,10 +44,13 @@
 from lp.registry.interfaces.person import IPersonSet
 from lp.registry.interfaces.product import IProduct
 from lp.registry.interfaces.sourcepackage import ISourcePackage
+from lp.registry.model.person import Person
 from lp.testing import (
     person_logged_in,
+    StormStatementRecorder,
     TestCaseWithFactory,
     )
+from lp.testing.matchers import HasQueryCount
 
 
 class SearchTestBase:
@@ -907,13 +915,40 @@
     def setUp(self):
         super(PreloadBugtaskTargets, self).setUp()
 
-    def runSearch(self, params, *args):
+    def runSearch(self, params, *args, **kw):
         """Run BugTaskSet.search() and preload bugtask target objects."""
-        return list(self.bugtask_set.search(params, *args, _noprejoins=False))
+        return list(self.bugtask_set.search(
+            params, *args, _noprejoins=False, **kw))
 
     def resultValuesForBugtasks(self, expected_bugtasks):
         return expected_bugtasks
 
+    def test_preload_additional_objects(self):
+        # It is possible to join additional tables in the search query
+        # in order to load related Storm objects during the query.
+        store = Store.of(self.bugtasks[0])
+        store.invalidate()
+
+        # If we do not prejoin the owner, two queries a run
+        # in order to retrieve the owner of the bugtask.
+        with StormStatementRecorder() as recorder:
+            params = self.getBugTaskSearchParams(user=None)
+            found_tasks = self.runSearch(params)
+            found_tasks[0].owner
+            self.assertTrue(len(recorder.statements) > 1)
+
+        # If we join the table person on bugtask.owner == person.id
+        # the owner object is loaded in the query that retrieves the
+        # bugtasks.
+        store.invalidate()
+        with StormStatementRecorder() as recorder:
+            params = self.getBugTaskSearchParams(user=None)
+            found_tasks = self.runSearch(
+                params,
+                prejoins=[(Person, Join(Person, BugTask.owner == Person.id))])
+            found_tasks[0].owner
+            self.assertThat(recorder, HasQueryCount(Equals(1)))
+
 
 class NoPreloadBugtaskTargets(MultipleParams):
     """Do not preload bug targets during a BugTaskSet.search() query."""

=== modified file 'lib/lp/registry/model/person.py'
--- lib/lp/registry/model/person.py	2010-11-18 12:05:34 +0000
+++ lib/lp/registry/model/person.py	2010-11-23 13:40:38 +0000
@@ -882,6 +882,7 @@
 
     def searchTasks(self, search_params, *args, **kwargs):
         """See `IHasBugs`."""
+        prejoins = kwargs.pop('prejoins', [])
         if search_params is None and len(args) == 0:
             # this method is called via webapi directly
             # calling this method on a Person object directly via the
@@ -896,15 +897,18 @@
                 # method, see docstring of
                 # `lazr.restful.declarations.webservice_error()`
                 raise e
-            return getUtility(IBugTaskSet).search(*search_params)
+            return getUtility(IBugTaskSet).search(
+                *search_params, prejoins=prejoins)
         if len(kwargs) > 0:
             # if keyword arguments are supplied, use the deault
             # implementation in HasBugsBase.
-            return HasBugsBase.searchTasks(self, search_params, **kwargs)
+            return HasBugsBase.searchTasks(
+                self, search_params, prejoins=prejoins, **kwargs)
         else:
             # Otherwise pass all positional arguments to the
             # implementation in BugTaskSet.
-            return getUtility(IBugTaskSet).search(search_params, *args)
+            return getUtility(IBugTaskSet).search(
+                search_params, *args, prejoins=prejoins)
 
     def getProjectsAndCategoriesContributedTo(self, limit=5):
         """See `IPerson`."""