launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #01997
[Merge] lp:~adeuring/launchpad/bug-675595 into lp:launchpad/devel
Abel Deuring has proposed merging lp:~adeuring/launchpad/bug-675595 into lp:launchpad/devel.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
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
--
https://code.launchpad.net/~adeuring/launchpad/bug-675595/+merge/41595
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~adeuring/launchpad/bug-675595 into lp:launchpad/devel.
=== 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`."""
