launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #02171
[Merge] lp:~edwin-grubbs/launchpad/bug-638924-testfix into lp:launchpad
Edwin Grubbs has proposed merging lp:~edwin-grubbs/launchpad/bug-638924-testfix into lp:launchpad with lp:~edwin-grubbs/launchpad/bug-638924-milestone-page-eager-loading as a prerequisite.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
Summary
-------
Most of this branch was already landed and then rolled back. I am
including a smaller diff of just the changes that I have made since the
first time it was reviewed.
The query was excluding all the bugtasks for a bug, if the bug had even
one conjoined master. However, the conjoined master bugtask should only
exclude the conjoined slave. The conjoined master is the bugtask on the
project's development focus series or the distro's currentseries. The
conjoined slave is the bugtask on the corresponding project or distro.
In the Launchpad web UI, the milestone for the project or distro bugtask
will be hidden if there is a conjoined master bugtask, but in the
database, the project or distro bugtask is always updated with the same
milestone as the conjoined master bugtask.
Tests
-----
./bin/test -vv -t 'test_bugsearch_conjoined|object-milestones.txt'
Demo and Q/A
------------
These three pages should not timeout.
* https://launchpad.net/launchpad-project/+milestone/10.10
* https://launchpad.net/launchpad-registry/+milestone/10.10
* https://launchpad.net/ubuntu/+milestone/ubuntu-9.10/+index
Diff
----
=== modified file 'lib/lp/bugs/model/bugtask.py'
--- lib/lp/bugs/model/bugtask.py 2010-12-10 15:06:12 +0000
+++ lib/lp/bugs/model/bugtask.py 2010-12-15 16:40:51 +0000
@@ -39,6 +39,7 @@
In,
Join,
LeftJoin,
+ Not,
Or,
Select,
SQL,
@@ -1601,6 +1602,78 @@
raise AssertionError(
'Unrecognized status value: %s' % repr(status))
+ def _buildExcludeConjoinedClause(self, milestone):
+ """Exclude bugtasks with a conjoined master.
+
+ This search option only makes sense when searching for bugtasks
+ for a milestone. Only bugtasks for a project or a distribution
+ can have a conjoined master bugtask, which is a bugtask on the
+ project's development focus series or the distribution's
+ currentseries. The project bugtask or the distribution bugtask
+ will always have the same milestone set as its conjoined master
+ bugtask, if it exists on the bug. Therefore, this prevents a lot
+ of bugs having two bugtasks listed in the results. However, it
+ is ok if a bug has multiple bugtasks in the results as long as
+ those other bugtasks are on other series.
+ """
+ # XXX: EdwinGrubbs 2010-12-15 bug=682989
+ # (ConjoinedMaster.bug == X) produces the wrong sql, but
+ # (ConjoinedMaster.bugID == X) works right. This bug applies to
+ # all foreign keys on the ClassAlias.
+
+ # Perform a LEFT JOIN to the conjoined master bugtask. If the
+ # conjoined master is not null, it gets filtered out.
+ ConjoinedMaster = ClassAlias(BugTask, 'ConjoinedMaster')
+ extra_clauses = ["ConjoinedMaster.id IS NULL"]
+ if milestone.distribution is not None:
+ current_series = milestone.distribution.currentseries
+ join = LeftJoin(
+ ConjoinedMaster,
+ And(ConjoinedMaster.bugID == BugTask.bugID,
+ BugTask.distributionID == milestone.distribution.id,
+ ConjoinedMaster.distroseriesID == current_series.id,
+ Not(ConjoinedMaster.status.is_in(
+ BugTask._NON_CONJOINED_STATUSES))))
+ join_tables = [(ConjoinedMaster, join)]
+ else:
+ # Prevent import loop.
+ from lp.registry.model.milestone import Milestone
+ from lp.registry.model.product import Product
+ if IProjectGroupMilestone.providedBy(milestone):
+ # Since an IProjectGroupMilestone could have bugs with
+ # bugtasks on two different projects, the project
+ # bugtask is only excluded by a development focus series
+ # bugtask on the same project.
+ joins = [
+ Join(Milestone, BugTask.milestone == Milestone.id),
+ LeftJoin(Product, BugTask.product == Product.id),
+ LeftJoin(
+ ConjoinedMaster,
+ And(ConjoinedMaster.bugID == BugTask.bugID,
+ ConjoinedMaster.productseriesID
+ == Product.development_focusID,
+ Not(ConjoinedMaster.status.is_in(
+ BugTask._NON_CONJOINED_STATUSES)))),
+ ]
+ # join.right is the table name.
+ join_tables = [(join.right, join) for join in joins]
+ elif milestone.product is not None:
+ dev_focus_id = (
+ milestone.product.development_focusID)
+ join = LeftJoin(
+ ConjoinedMaster,
+ And(ConjoinedMaster.bugID == BugTask.bugID,
+ BugTask.productID == milestone.product.id,
+ ConjoinedMaster.productseriesID == dev_focus_id,
+ Not(ConjoinedMaster.status.is_in(
+ BugTask._NON_CONJOINED_STATUSES))))
+ join_tables = [(ConjoinedMaster, join)]
+ else:
+ raise AssertionError(
+ "A milestone must always have either a project, "
+ "project group, or distribution")
+ return (join_tables, extra_clauses)
+
def buildQuery(self, params):
"""Build and return an SQL query with the given parameters.
@@ -1671,41 +1744,10 @@
extra_clauses.append("BugTask.milestone %s" % where_cond)
if params.exclude_conjoined_tasks:
- # Perform a LEFT JOIN to the conjoined master bugtask.
- # If the conjoined master is not null, it gets filtered
- # out.
- ConjoinedMaster = ClassAlias(BugTask, 'ConjoinedMaster')
- extra_clauses.append("ConjoinedMaster.id IS NULL")
- if params.milestone.distribution is not None:
- current_series = (
- params.milestone.distribution.currentseries)
- join = LeftJoin(
- ConjoinedMaster,
- And(ConjoinedMaster.bug == BugTask.bugID,
- ConjoinedMaster.distroseries == current_series.id,
- ))
- join_tables.append((ConjoinedMaster, join))
- else:
- # Prevent import loop.
- from lp.registry.model.product import Product
- if IProjectGroupMilestone.providedBy(params.milestone):
- dev_focus_ids = list(
- IStore(Product).find(
- Product.development_focusID,
- Product.project == params.milestone.target))
- elif params.milestone.product is not None:
- dev_focus_ids = [
- params.milestone.product.development_focusID]
- else:
- raise AssertionError(
- "A milestone must always have either a project, "
- "project group, or distribution")
- join = LeftJoin(
- ConjoinedMaster,
- And(ConjoinedMaster.bug == BugTask.bugID,
- ConjoinedMaster.productseriesID.is_in(
- dev_focus_ids)))
- join_tables.append((ConjoinedMaster, join))
+ tables, clauses = self._buildExcludeConjoinedClause(
+ params.milestone)
+ join_tables += tables
+ extra_clauses += clauses
elif params.exclude_conjoined_tasks:
raise ValueError(
"BugTaskSearchParam.exclude_conjoined cannot be True if "
=== added file 'lib/lp/bugs/tests/test_bugsearch_conjoined.py'
--- lib/lp/bugs/tests/test_bugsearch_conjoined.py 1970-01-01 00:00:00 +0000
+++ lib/lp/bugs/tests/test_bugsearch_conjoined.py 2010-12-15 05:42:42 +0000
@@ -0,0 +1,340 @@
+# Copyright 2010 Canonical Ltd. This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+"""Test for the exclude_conjoined_tasks param for BugTaskSearchParams."""
+
+__metaclass__ = type
+
+__all__ = []
+
+from storm.store import Store
+from testtools.matchers import Equals
+from zope.component import getUtility
+
+from canonical.launchpad.interfaces.launchpad import ILaunchpadCelebrities
+from canonical.testing.layers import DatabaseFunctionalLayer
+from lp.bugs.interfaces.bugtask import (
+ BugTaskSearchParams,
+ BugTaskStatus,
+ IBugTaskSet,
+ )
+from lp.registry.interfaces.series import SeriesStatus
+from lp.testing import (
+ person_logged_in,
+ StormStatementRecorder,
+ TestCaseWithFactory,
+ )
+from lp.testing.matchers import HasQueryCount
+
+
+class TestSearchBase(TestCaseWithFactory):
+ """Tests of exclude_conjoined_tasks param."""
+
+ def makeBug(self, milestone):
+ bug = self.factory.makeBug(
+ product=milestone.product, distribution=milestone.distribution)
+ with person_logged_in(milestone.target.owner):
+ bug.default_bugtask.transitionToMilestone(
+ milestone, milestone.target.owner)
+ return bug
+
+
+class TestProjectExcludeConjoinedMasterSearch(TestSearchBase):
+ """Tests of exclude_conjoined_tasks param for project milestones."""
+
+ layer = DatabaseFunctionalLayer
+
+ def setUp(self):
+ super(TestProjectExcludeConjoinedMasterSearch, self).setUp()
+ self.bugtask_set = getUtility(IBugTaskSet)
+ self.product = self.factory.makeProduct()
+ self.milestone = self.factory.makeMilestone(
+ product=self.product, name='foo')
+ self.bug_count = 2
+ self.bugs = [
+ self.makeBug(self.milestone)
+ for i in range(self.bug_count)]
+ self.params = BugTaskSearchParams(
+ user=None, milestone=self.milestone, exclude_conjoined_tasks=True)
+
+ def test_search_results_count_simple(self):
+ # Verify number of results with no conjoined masters.
+ self.assertEqual(
+ self.bug_count,
+ self.bugtask_set.search(self.params).count())
+
+ def test_search_query_count(self):
+ # Verify query count.
+ Store.of(self.milestone).flush()
+ with StormStatementRecorder() as recorder:
+ list(self.bugtask_set.search(self.params))
+ self.assertThat(recorder, HasQueryCount(Equals(1)))
+
+ def test_search_results_count_with_other_productseries_tasks(self):
+ # Test with zero conjoined masters and bugtasks targeted to
+ # productseries that are not the development focus.
+ productseries = self.factory.makeProductSeries(product=self.product)
+ extra_bugtasks = 0
+ for bug in self.bugs:
+ extra_bugtasks += 1
+ bugtask = self.factory.makeBugTask(bug=bug, target=productseries)
+ with person_logged_in(self.product.owner):
+ bugtask.transitionToMilestone(
+ self.milestone, self.product.owner)
+ self.assertEqual(
+ self.bug_count + extra_bugtasks,
+ self.bugtask_set.search(self.params).count())
+
+ def test_search_results_count_with_conjoined_masters(self):
+ # Test with increasing numbers of conjoined masters.
+ # The conjoined masters will exclude the conjoined slaves from
+ # the results.
+ tasks = list(self.bugtask_set.search(self.params))
+ for bug in self.bugs:
+ # The product bugtask is in the results before the conjoined
+ # master is added.
+ self.assertIn(
+ (bug.id, self.product),
+ [(task.bug.id, task.product) for task in tasks])
+ bugtask = self.factory.makeBugTask(
+ bug=bug, target=self.product.development_focus)
+ tasks = list(self.bugtask_set.search(self.params))
+ # The product bugtask is excluded from the results.
+ self.assertEqual(self.bug_count, len(tasks))
+ self.assertNotIn(
+ (bug.id, self.product),
+ [(task.bug.id, task.product) for task in tasks])
+
+ def test_search_results_count_with_wontfix_conjoined_masters(self):
+ # Test that conjoined master bugtasks in the WONTFIX status
+ # don't cause the bug to be excluded.
+ masters = [
+ self.factory.makeBugTask(
+ bug=bug, target=self.product.development_focus)
+ for bug in self.bugs]
+ tasks = list(self.bugtask_set.search(self.params))
+ wontfix_masters_count = 0
+ for bugtask in masters:
+ wontfix_masters_count += 1
+ self.assertNotIn(
+ (bugtask.bug.id, self.product),
+ [(task.bug.id, task.product) for task in tasks])
+ with person_logged_in(self.product.owner):
+ bugtask.transitionToStatus(
+ BugTaskStatus.WONTFIX, self.product.owner)
+ tasks = list(self.bugtask_set.search(self.params))
+ self.assertEqual(self.bug_count + wontfix_masters_count,
+ len(tasks))
+ self.assertIn(
+ (bugtask.bug.id, self.product),
+ [(task.bug.id, task.product) for task in tasks])
+
+
+class TestProjectGroupExcludeConjoinedMasterSearch(TestSearchBase):
+ """Tests of exclude_conjoined_tasks param for project group milestones."""
+
+ layer = DatabaseFunctionalLayer
+
+ def setUp(self):
+ super(TestProjectGroupExcludeConjoinedMasterSearch, self).setUp()
+ self.bugtask_set = getUtility(IBugTaskSet)
+ self.projectgroup = self.factory.makeProject()
+ self.bug_count = 2
+ self.bug_products = {}
+ for i in range(self.bug_count):
+ product = self.factory.makeProduct(project=self.projectgroup)
+ product_milestone = self.factory.makeMilestone(
+ product=product, name='foo')
+ bug = self.makeBug(product_milestone)
+ self.bug_products[bug] = product
+ self.milestone = self.projectgroup.getMilestone('foo')
+ self.params = BugTaskSearchParams(
+ user=None, milestone=self.milestone, exclude_conjoined_tasks=True)
+
+ def test_search_results_count_simple(self):
+ # Verify number of results with no conjoined masters.
+ self.assertEqual(
+ self.bug_count,
+ self.bugtask_set.search(self.params).count())
+
+ def test_search_query_count(self):
+ # Verify query count.
+ Store.of(self.projectgroup).flush()
+ with StormStatementRecorder() as recorder:
+ list(self.bugtask_set.search(self.params))
+ self.assertThat(recorder, HasQueryCount(Equals(1)))
+
+ def test_search_results_count_with_other_productseries_tasks(self):
+ # Test with zero conjoined masters and bugtasks targeted to
+ # productseries that are not the development focus.
+ extra_bugtasks = 0
+ for bug, product in self.bug_products.items():
+ extra_bugtasks += 1
+ productseries = self.factory.makeProductSeries(product=product)
+ bugtask = self.factory.makeBugTask(bug=bug, target=productseries)
+ with person_logged_in(product.owner):
+ bugtask.transitionToMilestone(
+ product.getMilestone(self.milestone.name), product.owner)
+ self.assertEqual(
+ self.bug_count + extra_bugtasks,
+ self.bugtask_set.search(self.params).count())
+
+ def test_search_results_count_with_conjoined_masters(self):
+ # Test with increasing numbers of conjoined masters.
+ tasks = list(self.bugtask_set.search(self.params))
+ for bug, product in self.bug_products.items():
+ self.assertIn(
+ (bug.id, product),
+ [(task.bug.id, task.product) for task in tasks])
+ self.factory.makeBugTask(
+ bug=bug, target=product.development_focus)
+ tasks = list(self.bugtask_set.search(self.params))
+ self.assertEqual(
+ self.bug_count,
+ self.bugtask_set.search(self.params).count())
+ self.assertNotIn(
+ (bug.id, product),
+ [(task.bug.id, task.product) for task in tasks])
+
+ def test_search_results_count_with_irrelevant_conjoined_masters(self):
+ # Verify that a conjoined master in one project of the project
+ # group doesn't cause a bugtask on another project in the group
+ # to be excluded from the project group milestone's bugs.
+ extra_bugtasks = 0
+ for bug, product in self.bug_products.items():
+ extra_bugtasks += 1
+ other_product = self.factory.makeProduct(
+ project=self.projectgroup)
+ # Create a new milestone with the same name.
+ other_product_milestone = self.factory.makeMilestone(
+ product=other_product,
+ name=bug.default_bugtask.milestone.name)
+ # Add bugtask on the new product and select the milestone.
+ other_product_bugtask = self.factory.makeBugTask(
+ bug=bug, target=other_product)
+ with person_logged_in(other_product.owner):
+ other_product_bugtask.transitionToMilestone(
+ other_product_milestone, other_product.owner)
+ # Add conjoined master for the milestone on the new product.
+ self.factory.makeBugTask(
+ bug=bug, target=other_product.development_focus)
+ # The bug count should not change, since we are just adding
+ # bugtasks on existing bugs.
+ self.assertEqual(
+ self.bug_count + extra_bugtasks,
+ self.bugtask_set.search(self.params).count())
+
+ def test_search_results_count_with_wontfix_conjoined_masters(self):
+ # Test that conjoined master bugtasks in the WONTFIX status
+ # don't cause the bug to be excluded.
+ masters = [
+ self.factory.makeBugTask(
+ bug=bug, target=product.development_focus)
+ for bug, product in self.bug_products.items()]
+ unexcluded_count = 0
+ for bugtask in masters:
+ unexcluded_count += 1
+ with person_logged_in(product.owner):
+ bugtask.transitionToStatus(
+ BugTaskStatus.WONTFIX, bugtask.target.owner)
+ self.assertEqual(
+ self.bug_count + unexcluded_count,
+ self.bugtask_set.search(self.params).count())
+
+
+class TestDistributionExcludeConjoinedMasterSearch(TestSearchBase):
+ """Tests of exclude_conjoined_tasks param for distribution milestones."""
+
+ layer = DatabaseFunctionalLayer
+
+ def setUp(self):
+ super(TestDistributionExcludeConjoinedMasterSearch, self).setUp()
+ self.bugtask_set = getUtility(IBugTaskSet)
+ self.distro = getUtility(ILaunchpadCelebrities).ubuntu
+ self.milestone = self.factory.makeMilestone(
+ distribution=self.distro, name='foo')
+ self.bug_count = 2
+ self.bugs = [
+ self.makeBug(self.milestone)
+ for i in range(self.bug_count)]
+ self.params = BugTaskSearchParams(
+ user=None, milestone=self.milestone, exclude_conjoined_tasks=True)
+
+ def test_search_results_count_simple(self):
+ # Verify number of results with no conjoined masters.
+ self.assertEqual(
+ self.bug_count,
+ self.bugtask_set.search(self.params).count())
+
+ def test_search_query_count(self):
+ # Verify query count.
+ # 1. Query all the distroseries to determine the distro's
+ # currentseries.
+ # 2. Query the bugtasks.
+ Store.of(self.milestone).flush()
+ with StormStatementRecorder() as recorder:
+ list(self.bugtask_set.search(self.params))
+ self.assertThat(recorder, HasQueryCount(Equals(2)))
+
+ def test_search_results_count_with_other_productseries_tasks(self):
+ # Test with zero conjoined masters and bugtasks targeted to
+ # productseries that are not the development focus.
+ distroseries = self.factory.makeDistroSeries(
+ distribution=self.distro, status=SeriesStatus.SUPPORTED)
+ extra_bugtasks = 0
+ for bug in self.bugs:
+ extra_bugtasks += 1
+ bugtask = self.factory.makeBugTask(bug=bug, target=distroseries)
+ with person_logged_in(self.distro.owner):
+ bugtask.transitionToMilestone(
+ self.milestone, self.distro.owner)
+ self.assertEqual(
+ self.bug_count + extra_bugtasks,
+ self.bugtask_set.search(self.params).count())
+
+ def test_search_results_count_with_conjoined_masters(self):
+ # Test with increasing numbers of conjoined masters.
+ tasks = list(self.bugtask_set.search(self.params))
+ for bug in self.bugs:
+ # The distro bugtask is in the results before the conjoined
+ # master is added.
+ self.assertIn(
+ (bug.id, self.distro),
+ [(task.bug.id, task.distribution) for task in tasks])
+ self.factory.makeBugTask(
+ bug=bug, target=self.distro.currentseries)
+ tasks = list(self.bugtask_set.search(self.params))
+ # The product bugtask is excluded from the results.
+ self.assertEqual(self.bug_count, len(tasks))
+ self.assertNotIn(
+ (bug.id, self.distro),
+ [(task.bug.id, task.distribution) for task in tasks])
+
+ def test_search_results_count_with_wontfix_conjoined_masters(self):
+ # Test that conjoined master bugtasks in the WONTFIX status
+ # don't cause the bug to be excluded.
+ masters = [
+ self.factory.makeBugTask(
+ bug=bug, target=self.distro.currentseries)
+ for bug in self.bugs]
+ wontfix_masters_count = 0
+ tasks = list(self.bugtask_set.search(self.params))
+ for bugtask in masters:
+ wontfix_masters_count += 1
+ # The distro bugtask is still excluded by the conjoined
+ # master.
+ self.assertNotIn(
+ (bugtask.bug.id, self.distro),
+ [(task.bug.id, task.distribution) for task in tasks])
+ with person_logged_in(self.distro.owner):
+ bugtask.transitionToStatus(
+ BugTaskStatus.WONTFIX, self.distro.owner)
+ tasks = list(self.bugtask_set.search(self.params))
+ self.assertEqual(
+ self.bug_count + wontfix_masters_count,
+ self.bugtask_set.search(self.params).count())
+ # The distro bugtask is no longer excluded by the conjoined
+ # master, since its status is WONTFIX.
+ self.assertIn(
+ (bugtask.bug.id, self.distro),
+ [(task.bug.id, task.distribution) for task in tasks])
--
https://code.launchpad.net/~edwin-grubbs/launchpad/bug-638924-testfix/+merge/43791
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~edwin-grubbs/launchpad/bug-638924-testfix into lp:launchpad.
=== modified file 'lib/lp/bugs/interfaces/bugtask.py'
--- lib/lp/bugs/interfaces/bugtask.py 2010-12-10 07:52:21 +0000
+++ lib/lp/bugs/interfaces/bugtask.py 2010-12-15 16:58:33 +0000
@@ -1145,7 +1145,8 @@
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):
+ modified_since=None, created_since=None,
+ exclude_conjoined_tasks=False):
self.bug = bug
self.searchtext = searchtext
@@ -1191,6 +1192,7 @@
self.structural_subscriber = structural_subscriber
self.modified_since = modified_since
self.created_since = created_since
+ self.exclude_conjoined_tasks = exclude_conjoined_tasks
def setProduct(self, product):
"""Set the upstream context on which to filter the search."""
@@ -1530,6 +1532,12 @@
This takes into account bug subscription filters.
"""
+ def getPrecachedNonConjoinedBugTasks(user, milestone):
+ """List of non-conjoined bugtasks targeted to the milestone.
+
+ The assignee and the assignee's validity are precached.
+ """
+
def valid_remote_bug_url(value):
"""Verify that the URL is to a bug to a known bug tracker."""
=== modified file 'lib/lp/bugs/model/bugtask.py'
--- lib/lp/bugs/model/bugtask.py 2010-12-10 07:52:21 +0000
+++ lib/lp/bugs/model/bugtask.py 2010-12-15 16:58:33 +0000
@@ -39,18 +39,16 @@
In,
Join,
LeftJoin,
+ Not,
Or,
Select,
SQL,
)
+from storm.info import ClassAlias
from storm.store import (
EmptyResultSet,
Store,
)
-from storm.zope.interfaces import (
- IResultSet,
- ISQLObjectResultSet,
- )
from zope.component import getUtility
from zope.interface import (
alsoProvides,
@@ -142,6 +140,7 @@
from lp.registry.interfaces.milestone import IProjectGroupMilestone
from lp.registry.interfaces.person import (
IPerson,
+ IPersonSet,
validate_person,
validate_public_person,
)
@@ -1490,16 +1489,23 @@
from lp.bugs.model.bug import Bug
from lp.bugs.model.bugbranch import BugBranch
- bug_ids = list(set(bugtask.bugID for bugtask in bugtasks))
+ bug_ids = set(bugtask.bugID for bugtask in bugtasks)
bug_ids_with_specifications = set(IStore(SpecificationBug).find(
SpecificationBug.bugID,
SpecificationBug.bugID.is_in(bug_ids)))
bug_ids_with_branches = set(IStore(BugBranch).find(
BugBranch.bugID, BugBranch.bugID.is_in(bug_ids)))
- # Cache all bugs at once to avoid one query per bugtask. We
- # could rely on the Storm cache, but this is explicit.
- bugs = dict(IStore(Bug).find((Bug.id, Bug), Bug.id.is_in(bug_ids)))
+ # Check if the bugs are cached. If not, cache all uncached bugs
+ # at once to avoid one query per bugtask. We could rely on the
+ # Storm cache, but this is explicit.
+ bugs = dict(
+ (bug.id, bug)
+ for bug in IStore(Bug).find(Bug, Bug.id.is_in(bug_ids)).cached())
+ uncached_ids = bug_ids.difference(bug_id for bug_id in bugs)
+ if len(uncached_ids) > 0:
+ bugs.update(dict(IStore(Bug).find((Bug.id, Bug),
+ Bug.id.is_in(uncached_ids))))
badge_properties = {}
for bugtask in bugtasks:
@@ -1596,6 +1602,78 @@
raise AssertionError(
'Unrecognized status value: %s' % repr(status))
+ def _buildExcludeConjoinedClause(self, milestone):
+ """Exclude bugtasks with a conjoined master.
+
+ This search option only makes sense when searching for bugtasks
+ for a milestone. Only bugtasks for a project or a distribution
+ can have a conjoined master bugtask, which is a bugtask on the
+ project's development focus series or the distribution's
+ currentseries. The project bugtask or the distribution bugtask
+ will always have the same milestone set as its conjoined master
+ bugtask, if it exists on the bug. Therefore, this prevents a lot
+ of bugs having two bugtasks listed in the results. However, it
+ is ok if a bug has multiple bugtasks in the results as long as
+ those other bugtasks are on other series.
+ """
+ # XXX: EdwinGrubbs 2010-12-15 bug=682989
+ # (ConjoinedMaster.bug == X) produces the wrong sql, but
+ # (ConjoinedMaster.bugID == X) works right. This bug applies to
+ # all foreign keys on the ClassAlias.
+
+ # Perform a LEFT JOIN to the conjoined master bugtask. If the
+ # conjoined master is not null, it gets filtered out.
+ ConjoinedMaster = ClassAlias(BugTask, 'ConjoinedMaster')
+ extra_clauses = ["ConjoinedMaster.id IS NULL"]
+ if milestone.distribution is not None:
+ current_series = milestone.distribution.currentseries
+ join = LeftJoin(
+ ConjoinedMaster,
+ And(ConjoinedMaster.bugID == BugTask.bugID,
+ BugTask.distributionID == milestone.distribution.id,
+ ConjoinedMaster.distroseriesID == current_series.id,
+ Not(ConjoinedMaster.status.is_in(
+ BugTask._NON_CONJOINED_STATUSES))))
+ join_tables = [(ConjoinedMaster, join)]
+ else:
+ # Prevent import loop.
+ from lp.registry.model.milestone import Milestone
+ from lp.registry.model.product import Product
+ if IProjectGroupMilestone.providedBy(milestone):
+ # Since an IProjectGroupMilestone could have bugs with
+ # bugtasks on two different projects, the project
+ # bugtask is only excluded by a development focus series
+ # bugtask on the same project.
+ joins = [
+ Join(Milestone, BugTask.milestone == Milestone.id),
+ LeftJoin(Product, BugTask.product == Product.id),
+ LeftJoin(
+ ConjoinedMaster,
+ And(ConjoinedMaster.bugID == BugTask.bugID,
+ ConjoinedMaster.productseriesID
+ == Product.development_focusID,
+ Not(ConjoinedMaster.status.is_in(
+ BugTask._NON_CONJOINED_STATUSES)))),
+ ]
+ # join.right is the table name.
+ join_tables = [(join.right, join) for join in joins]
+ elif milestone.product is not None:
+ dev_focus_id = (
+ milestone.product.development_focusID)
+ join = LeftJoin(
+ ConjoinedMaster,
+ And(ConjoinedMaster.bugID == BugTask.bugID,
+ BugTask.productID == milestone.product.id,
+ ConjoinedMaster.productseriesID == dev_focus_id,
+ Not(ConjoinedMaster.status.is_in(
+ BugTask._NON_CONJOINED_STATUSES))))
+ join_tables = [(ConjoinedMaster, join)]
+ else:
+ raise AssertionError(
+ "A milestone must always have either a project, "
+ "project group, or distribution")
+ return (join_tables, extra_clauses)
+
def buildQuery(self, params):
"""Build and return an SQL query with the given parameters.
@@ -1665,6 +1743,17 @@
where_cond = search_value_to_where_condition(params.milestone)
extra_clauses.append("BugTask.milestone %s" % where_cond)
+ if params.exclude_conjoined_tasks:
+ tables, clauses = self._buildExcludeConjoinedClause(
+ params.milestone)
+ join_tables += tables
+ extra_clauses += clauses
+ elif params.exclude_conjoined_tasks:
+ raise ValueError(
+ "BugTaskSearchParam.exclude_conjoined cannot be True if "
+ "BugTaskSearchParam.milestone is not set")
+
+
if params.project:
# Prevent circular import problems.
from lp.registry.model.product import Product
@@ -2309,6 +2398,25 @@
"""See `IBugTaskSet`."""
return self._search(BugTask.bugID, [], params).result_set
+ def getPrecachedNonConjoinedBugTasks(self, user, milestone):
+ """See `IBugTaskSet`."""
+ params = BugTaskSearchParams(
+ user, milestone=milestone,
+ orderby=['status', '-importance', 'id'],
+ omit_dupes=True, exclude_conjoined_tasks=True)
+ non_conjoined_slaves = self.search(params)
+
+ def cache_people(rows):
+ assignee_ids = set(
+ bug_task.assigneeID for bug_task in rows)
+ assignees = getUtility(IPersonSet).getPrecachedPersonsFromIDs(
+ assignee_ids, need_validity=True)
+ # Execute query to load storm cache.
+ list(assignees)
+
+ return DecoratedResultSet(
+ non_conjoined_slaves, pre_iter_hook=cache_people)
+
def createTask(self, bug, owner, product=None, productseries=None,
distribution=None, distroseries=None,
sourcepackagename=None,
=== added file 'lib/lp/bugs/tests/test_bugsearch_conjoined.py'
--- lib/lp/bugs/tests/test_bugsearch_conjoined.py 1970-01-01 00:00:00 +0000
+++ lib/lp/bugs/tests/test_bugsearch_conjoined.py 2010-12-15 16:58:33 +0000
@@ -0,0 +1,340 @@
+# Copyright 2010 Canonical Ltd. This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+"""Test for the exclude_conjoined_tasks param for BugTaskSearchParams."""
+
+__metaclass__ = type
+
+__all__ = []
+
+from storm.store import Store
+from testtools.matchers import Equals
+from zope.component import getUtility
+
+from canonical.launchpad.interfaces.launchpad import ILaunchpadCelebrities
+from canonical.testing.layers import DatabaseFunctionalLayer
+from lp.bugs.interfaces.bugtask import (
+ BugTaskSearchParams,
+ BugTaskStatus,
+ IBugTaskSet,
+ )
+from lp.registry.interfaces.series import SeriesStatus
+from lp.testing import (
+ person_logged_in,
+ StormStatementRecorder,
+ TestCaseWithFactory,
+ )
+from lp.testing.matchers import HasQueryCount
+
+
+class TestSearchBase(TestCaseWithFactory):
+ """Tests of exclude_conjoined_tasks param."""
+
+ def makeBug(self, milestone):
+ bug = self.factory.makeBug(
+ product=milestone.product, distribution=milestone.distribution)
+ with person_logged_in(milestone.target.owner):
+ bug.default_bugtask.transitionToMilestone(
+ milestone, milestone.target.owner)
+ return bug
+
+
+class TestProjectExcludeConjoinedMasterSearch(TestSearchBase):
+ """Tests of exclude_conjoined_tasks param for project milestones."""
+
+ layer = DatabaseFunctionalLayer
+
+ def setUp(self):
+ super(TestProjectExcludeConjoinedMasterSearch, self).setUp()
+ self.bugtask_set = getUtility(IBugTaskSet)
+ self.product = self.factory.makeProduct()
+ self.milestone = self.factory.makeMilestone(
+ product=self.product, name='foo')
+ self.bug_count = 2
+ self.bugs = [
+ self.makeBug(self.milestone)
+ for i in range(self.bug_count)]
+ self.params = BugTaskSearchParams(
+ user=None, milestone=self.milestone, exclude_conjoined_tasks=True)
+
+ def test_search_results_count_simple(self):
+ # Verify number of results with no conjoined masters.
+ self.assertEqual(
+ self.bug_count,
+ self.bugtask_set.search(self.params).count())
+
+ def test_search_query_count(self):
+ # Verify query count.
+ Store.of(self.milestone).flush()
+ with StormStatementRecorder() as recorder:
+ list(self.bugtask_set.search(self.params))
+ self.assertThat(recorder, HasQueryCount(Equals(1)))
+
+ def test_search_results_count_with_other_productseries_tasks(self):
+ # Test with zero conjoined masters and bugtasks targeted to
+ # productseries that are not the development focus.
+ productseries = self.factory.makeProductSeries(product=self.product)
+ extra_bugtasks = 0
+ for bug in self.bugs:
+ extra_bugtasks += 1
+ bugtask = self.factory.makeBugTask(bug=bug, target=productseries)
+ with person_logged_in(self.product.owner):
+ bugtask.transitionToMilestone(
+ self.milestone, self.product.owner)
+ self.assertEqual(
+ self.bug_count + extra_bugtasks,
+ self.bugtask_set.search(self.params).count())
+
+ def test_search_results_count_with_conjoined_masters(self):
+ # Test with increasing numbers of conjoined masters.
+ # The conjoined masters will exclude the conjoined slaves from
+ # the results.
+ tasks = list(self.bugtask_set.search(self.params))
+ for bug in self.bugs:
+ # The product bugtask is in the results before the conjoined
+ # master is added.
+ self.assertIn(
+ (bug.id, self.product),
+ [(task.bug.id, task.product) for task in tasks])
+ bugtask = self.factory.makeBugTask(
+ bug=bug, target=self.product.development_focus)
+ tasks = list(self.bugtask_set.search(self.params))
+ # The product bugtask is excluded from the results.
+ self.assertEqual(self.bug_count, len(tasks))
+ self.assertNotIn(
+ (bug.id, self.product),
+ [(task.bug.id, task.product) for task in tasks])
+
+ def test_search_results_count_with_wontfix_conjoined_masters(self):
+ # Test that conjoined master bugtasks in the WONTFIX status
+ # don't cause the bug to be excluded.
+ masters = [
+ self.factory.makeBugTask(
+ bug=bug, target=self.product.development_focus)
+ for bug in self.bugs]
+ tasks = list(self.bugtask_set.search(self.params))
+ wontfix_masters_count = 0
+ for bugtask in masters:
+ wontfix_masters_count += 1
+ self.assertNotIn(
+ (bugtask.bug.id, self.product),
+ [(task.bug.id, task.product) for task in tasks])
+ with person_logged_in(self.product.owner):
+ bugtask.transitionToStatus(
+ BugTaskStatus.WONTFIX, self.product.owner)
+ tasks = list(self.bugtask_set.search(self.params))
+ self.assertEqual(self.bug_count + wontfix_masters_count,
+ len(tasks))
+ self.assertIn(
+ (bugtask.bug.id, self.product),
+ [(task.bug.id, task.product) for task in tasks])
+
+
+class TestProjectGroupExcludeConjoinedMasterSearch(TestSearchBase):
+ """Tests of exclude_conjoined_tasks param for project group milestones."""
+
+ layer = DatabaseFunctionalLayer
+
+ def setUp(self):
+ super(TestProjectGroupExcludeConjoinedMasterSearch, self).setUp()
+ self.bugtask_set = getUtility(IBugTaskSet)
+ self.projectgroup = self.factory.makeProject()
+ self.bug_count = 2
+ self.bug_products = {}
+ for i in range(self.bug_count):
+ product = self.factory.makeProduct(project=self.projectgroup)
+ product_milestone = self.factory.makeMilestone(
+ product=product, name='foo')
+ bug = self.makeBug(product_milestone)
+ self.bug_products[bug] = product
+ self.milestone = self.projectgroup.getMilestone('foo')
+ self.params = BugTaskSearchParams(
+ user=None, milestone=self.milestone, exclude_conjoined_tasks=True)
+
+ def test_search_results_count_simple(self):
+ # Verify number of results with no conjoined masters.
+ self.assertEqual(
+ self.bug_count,
+ self.bugtask_set.search(self.params).count())
+
+ def test_search_query_count(self):
+ # Verify query count.
+ Store.of(self.projectgroup).flush()
+ with StormStatementRecorder() as recorder:
+ list(self.bugtask_set.search(self.params))
+ self.assertThat(recorder, HasQueryCount(Equals(1)))
+
+ def test_search_results_count_with_other_productseries_tasks(self):
+ # Test with zero conjoined masters and bugtasks targeted to
+ # productseries that are not the development focus.
+ extra_bugtasks = 0
+ for bug, product in self.bug_products.items():
+ extra_bugtasks += 1
+ productseries = self.factory.makeProductSeries(product=product)
+ bugtask = self.factory.makeBugTask(bug=bug, target=productseries)
+ with person_logged_in(product.owner):
+ bugtask.transitionToMilestone(
+ product.getMilestone(self.milestone.name), product.owner)
+ self.assertEqual(
+ self.bug_count + extra_bugtasks,
+ self.bugtask_set.search(self.params).count())
+
+ def test_search_results_count_with_conjoined_masters(self):
+ # Test with increasing numbers of conjoined masters.
+ tasks = list(self.bugtask_set.search(self.params))
+ for bug, product in self.bug_products.items():
+ self.assertIn(
+ (bug.id, product),
+ [(task.bug.id, task.product) for task in tasks])
+ self.factory.makeBugTask(
+ bug=bug, target=product.development_focus)
+ tasks = list(self.bugtask_set.search(self.params))
+ self.assertEqual(
+ self.bug_count,
+ self.bugtask_set.search(self.params).count())
+ self.assertNotIn(
+ (bug.id, product),
+ [(task.bug.id, task.product) for task in tasks])
+
+ def test_search_results_count_with_irrelevant_conjoined_masters(self):
+ # Verify that a conjoined master in one project of the project
+ # group doesn't cause a bugtask on another project in the group
+ # to be excluded from the project group milestone's bugs.
+ extra_bugtasks = 0
+ for bug, product in self.bug_products.items():
+ extra_bugtasks += 1
+ other_product = self.factory.makeProduct(
+ project=self.projectgroup)
+ # Create a new milestone with the same name.
+ other_product_milestone = self.factory.makeMilestone(
+ product=other_product,
+ name=bug.default_bugtask.milestone.name)
+ # Add bugtask on the new product and select the milestone.
+ other_product_bugtask = self.factory.makeBugTask(
+ bug=bug, target=other_product)
+ with person_logged_in(other_product.owner):
+ other_product_bugtask.transitionToMilestone(
+ other_product_milestone, other_product.owner)
+ # Add conjoined master for the milestone on the new product.
+ self.factory.makeBugTask(
+ bug=bug, target=other_product.development_focus)
+ # The bug count should not change, since we are just adding
+ # bugtasks on existing bugs.
+ self.assertEqual(
+ self.bug_count + extra_bugtasks,
+ self.bugtask_set.search(self.params).count())
+
+ def test_search_results_count_with_wontfix_conjoined_masters(self):
+ # Test that conjoined master bugtasks in the WONTFIX status
+ # don't cause the bug to be excluded.
+ masters = [
+ self.factory.makeBugTask(
+ bug=bug, target=product.development_focus)
+ for bug, product in self.bug_products.items()]
+ unexcluded_count = 0
+ for bugtask in masters:
+ unexcluded_count += 1
+ with person_logged_in(product.owner):
+ bugtask.transitionToStatus(
+ BugTaskStatus.WONTFIX, bugtask.target.owner)
+ self.assertEqual(
+ self.bug_count + unexcluded_count,
+ self.bugtask_set.search(self.params).count())
+
+
+class TestDistributionExcludeConjoinedMasterSearch(TestSearchBase):
+ """Tests of exclude_conjoined_tasks param for distribution milestones."""
+
+ layer = DatabaseFunctionalLayer
+
+ def setUp(self):
+ super(TestDistributionExcludeConjoinedMasterSearch, self).setUp()
+ self.bugtask_set = getUtility(IBugTaskSet)
+ self.distro = getUtility(ILaunchpadCelebrities).ubuntu
+ self.milestone = self.factory.makeMilestone(
+ distribution=self.distro, name='foo')
+ self.bug_count = 2
+ self.bugs = [
+ self.makeBug(self.milestone)
+ for i in range(self.bug_count)]
+ self.params = BugTaskSearchParams(
+ user=None, milestone=self.milestone, exclude_conjoined_tasks=True)
+
+ def test_search_results_count_simple(self):
+ # Verify number of results with no conjoined masters.
+ self.assertEqual(
+ self.bug_count,
+ self.bugtask_set.search(self.params).count())
+
+ def test_search_query_count(self):
+ # Verify query count.
+ # 1. Query all the distroseries to determine the distro's
+ # currentseries.
+ # 2. Query the bugtasks.
+ Store.of(self.milestone).flush()
+ with StormStatementRecorder() as recorder:
+ list(self.bugtask_set.search(self.params))
+ self.assertThat(recorder, HasQueryCount(Equals(2)))
+
+ def test_search_results_count_with_other_productseries_tasks(self):
+ # Test with zero conjoined masters and bugtasks targeted to
+ # productseries that are not the development focus.
+ distroseries = self.factory.makeDistroSeries(
+ distribution=self.distro, status=SeriesStatus.SUPPORTED)
+ extra_bugtasks = 0
+ for bug in self.bugs:
+ extra_bugtasks += 1
+ bugtask = self.factory.makeBugTask(bug=bug, target=distroseries)
+ with person_logged_in(self.distro.owner):
+ bugtask.transitionToMilestone(
+ self.milestone, self.distro.owner)
+ self.assertEqual(
+ self.bug_count + extra_bugtasks,
+ self.bugtask_set.search(self.params).count())
+
+ def test_search_results_count_with_conjoined_masters(self):
+ # Test with increasing numbers of conjoined masters.
+ tasks = list(self.bugtask_set.search(self.params))
+ for bug in self.bugs:
+ # The distro bugtask is in the results before the conjoined
+ # master is added.
+ self.assertIn(
+ (bug.id, self.distro),
+ [(task.bug.id, task.distribution) for task in tasks])
+ self.factory.makeBugTask(
+ bug=bug, target=self.distro.currentseries)
+ tasks = list(self.bugtask_set.search(self.params))
+ # The product bugtask is excluded from the results.
+ self.assertEqual(self.bug_count, len(tasks))
+ self.assertNotIn(
+ (bug.id, self.distro),
+ [(task.bug.id, task.distribution) for task in tasks])
+
+ def test_search_results_count_with_wontfix_conjoined_masters(self):
+ # Test that conjoined master bugtasks in the WONTFIX status
+ # don't cause the bug to be excluded.
+ masters = [
+ self.factory.makeBugTask(
+ bug=bug, target=self.distro.currentseries)
+ for bug in self.bugs]
+ wontfix_masters_count = 0
+ tasks = list(self.bugtask_set.search(self.params))
+ for bugtask in masters:
+ wontfix_masters_count += 1
+ # The distro bugtask is still excluded by the conjoined
+ # master.
+ self.assertNotIn(
+ (bugtask.bug.id, self.distro),
+ [(task.bug.id, task.distribution) for task in tasks])
+ with person_logged_in(self.distro.owner):
+ bugtask.transitionToStatus(
+ BugTaskStatus.WONTFIX, self.distro.owner)
+ tasks = list(self.bugtask_set.search(self.params))
+ self.assertEqual(
+ self.bug_count + wontfix_masters_count,
+ self.bugtask_set.search(self.params).count())
+ # The distro bugtask is no longer excluded by the conjoined
+ # master, since its status is WONTFIX.
+ self.assertIn(
+ (bugtask.bug.id, self.distro),
+ [(task.bug.id, task.distribution) for task in tasks])
=== modified file 'lib/lp/registry/browser/milestone.py'
--- lib/lp/registry/browser/milestone.py 2010-12-10 07:52:21 +0000
+++ lib/lp/registry/browser/milestone.py 2010-12-15 16:58:33 +0000
@@ -41,7 +41,6 @@
precache_permission_for_objects,
)
from canonical.launchpad.webapp.breadcrumb import Breadcrumb
-from canonical.launchpad.webapp.interfaces import ILaunchBag
from canonical.launchpad.webapp.menu import (
ApplicationMenu,
ContextMenu,
@@ -57,7 +56,6 @@
)
from lp.bugs.browser.bugtask import BugTaskListingItem
from lp.bugs.interfaces.bugtask import (
- BugTaskSearchParams,
IBugTaskSet,
)
from lp.registry.browser import (
@@ -249,23 +247,11 @@
@cachedproperty
def _bugtasks(self):
"""The list of non-conjoined bugtasks targeted to this milestone."""
- user = getUtility(ILaunchBag).user
- params = BugTaskSearchParams(user, milestone=self.context,
- orderby=['status', '-importance', 'id'],
- omit_dupes=True)
- tasks = getUtility(IBugTaskSet).search(params)
- # We could replace all the code below with a simple
- # >>> [task for task in tasks if task.conjoined_master is None]
- # But that'd cause one extra hit to the DB for every bugtask returned
- # by the search above, so we do a single query to get all of a task's
- # siblings here and use that to find whether or not a given bugtask
- # has a conjoined master.
- bugs_and_tasks = getUtility(IBugTaskSet).getBugTasks(
- [task.bug.id for task in tasks])
- non_conjoined_slaves = []
- for task in tasks:
- if task.getConjoinedMaster(bugs_and_tasks[task.bug]) is None:
- non_conjoined_slaves.append(task)
+ # Put the results in a list so that iterating over it multiple
+ # times in this method does not make multiple queries.
+ non_conjoined_slaves = list(
+ getUtility(IBugTaskSet).getPrecachedNonConjoinedBugTasks(
+ self.user, self.context))
# Checking bug permissions is expensive. We know from the query that
# the user has at least launchpad.View on the bugtasks and their bugs.
precache_permission_for_objects(
=== modified file 'lib/lp/registry/browser/tests/test_milestone.py'
--- lib/lp/registry/browser/tests/test_milestone.py 2010-12-10 07:52:21 +0000
+++ lib/lp/registry/browser/tests/test_milestone.py 2010-12-15 16:58:33 +0000
@@ -5,9 +5,13 @@
__metaclass__ = type
+from textwrap import dedent
+
from testtools.matchers import LessThan
from zope.component import getUtility
+from canonical.config import config
+from canonical.launchpad.interfaces.launchpad import ILaunchpadCelebrities
from canonical.launchpad.webapp import canonical_url
from canonical.testing.layers import DatabaseFunctionalLayer
from lp.bugs.interfaces.bugtask import IBugTaskSet
@@ -15,14 +19,16 @@
ANONYMOUS,
login,
login_person,
+ login_team,
logout,
person_logged_in,
+ StormStatementRecorder,
TestCaseWithFactory,
)
+from lp.testing._webservice import QueryCollector
from lp.testing.matchers import HasQueryCount
from lp.testing.memcache import MemcacheTestCase
from lp.testing.views import create_initialized_view
-from lp.testing._webservice import QueryCollector
class TestMilestoneViews(TestCaseWithFactory):
@@ -163,10 +169,89 @@
self.assertEqual(0, product.development_focus.all_bugtasks.count())
-class TestMilestoneIndex(TestCaseWithFactory):
+class TestQueryCountBase(TestCaseWithFactory):
+
+ def assert_bugtasks_query_count(self, milestone, bugtask_count,
+ query_limit):
+ # Assert that the number of queries run by view.bugtasks is low.
+ self.add_bug(bugtask_count)
+ login_person(self.owner)
+ view = create_initialized_view(milestone, '+index')
+ # Eliminate permission check for the admin team from the
+ # recorded queries by loading it now. If the test ever breaks,
+ # the person fixing it won't waste time trying to track this
+ # query down.
+ getUtility(ILaunchpadCelebrities).admin
+ with StormStatementRecorder() as recorder:
+ bugtasks = list(view.bugtasks)
+ self.assertThat(recorder, HasQueryCount(LessThan(query_limit)))
+ self.assertEqual(bugtask_count, len(bugtasks))
+
+ def assert_milestone_page_query_count(self, milestone, query_limit):
+ collector = QueryCollector()
+ collector.register()
+ try:
+ milestone_url = canonical_url(milestone)
+ browser = self.getUserBrowser(user=self.owner)
+ browser.open(milestone_url)
+ self.assertThat(collector, HasQueryCount(LessThan(query_limit)))
+ finally:
+ # Unregister now in case this method is called multiple
+ # times in a single test.
+ collector.unregister()
+
+
+class TestProjectMilestoneIndexQueryCount(TestQueryCountBase):
layer = DatabaseFunctionalLayer
+ def setUp(self):
+ super(TestProjectMilestoneIndexQueryCount, self).setUp()
+ # Increase cache size so that the query counts aren't affected
+ # by objects being removed from the cache early.
+ config.push('storm-cache', dedent('''
+ [launchpad]
+ storm_cache_size: 1000
+ '''))
+ self.addCleanup(config.pop, 'storm-cache')
+ self.owner = self.factory.makePerson(name='product-owner')
+ self.product = self.factory.makeProduct(owner=self.owner)
+ login_person(self.product.owner)
+ self.milestone = self.factory.makeMilestone(
+ productseries=self.product.development_focus)
+
+ def add_bug(self, count):
+ login_person(self.product.owner)
+ for i in range(count):
+ bug = self.factory.makeBug(product=self.product)
+ bug.bugtasks[0].transitionToMilestone(
+ self.milestone, self.product.owner)
+ # This is necessary to test precaching of assignees.
+ bug.bugtasks[0].transitionToAssignee(
+ self.factory.makePerson())
+ logout()
+
+ def test_bugtasks_queries(self):
+ # The view.bugtasks attribute will make four queries:
+ # 1. Load bugtasks and bugs.
+ # 2. Load assignees (Person, Account, and EmailAddress).
+ # 3. Load links to specifications.
+ # 4. Load links to branches.
+ bugtask_count = 10
+ self.assert_bugtasks_query_count(
+ self.milestone, bugtask_count, query_limit=5)
+
+ def test_milestone_eager_loading(self):
+ # Verify that the number of queries does not increase with more
+ # bugs with different assignees.
+ query_limit = 34
+ self.add_bug(3)
+ self.assert_milestone_page_query_count(
+ self.milestone, query_limit=query_limit)
+ self.add_bug(10)
+ self.assert_milestone_page_query_count(
+ self.milestone, query_limit=query_limit)
+
def test_more_private_bugs_query_count_is_constant(self):
# This test tests that as we add more private bugs to a milestone
# index page, the number of queries issued by the page does not
@@ -175,6 +260,7 @@
# is very large already, if the test fails due to such a change,
# please cut some more of the existing fat out of it rather than
# increasing the cap.
+ page_query_limit = 34
product = self.factory.makeProduct()
login_person(product.owner)
milestone = self.factory.makeMilestone(
@@ -199,8 +285,6 @@
collector = QueryCollector()
collector.register()
self.addCleanup(collector.unregister)
- # This page is rather high in queries : 46 as a baseline. 20100817
- page_query_limit = 46
browser.open(milestone_url)
# Check that the test found the bug
self.assertTrue(bug1_url in browser.contents)
@@ -228,3 +312,127 @@
self.assertEqual(with_1_private_bug, with_3_private_bugs,
"different query count: \n%s\n******************\n%s\n" % (
'\n'.join(with_1_queries), '\n'.join(with_3_queries)))
+
+
+class TestProjectGroupMilestoneIndexQueryCount(TestQueryCountBase):
+
+ layer = DatabaseFunctionalLayer
+
+ def setUp(self):
+ super(TestProjectGroupMilestoneIndexQueryCount, self).setUp()
+ # Increase cache size so that the query counts aren't affected
+ # by objects being removed from the cache early.
+ config.push('storm-cache', dedent('''
+ [launchpad]
+ storm_cache_size: 1000
+ '''))
+ self.addCleanup(config.pop, 'storm-cache')
+ self.owner = self.factory.makePerson(name='product-owner')
+ self.project_group = self.factory.makeProject(owner=self.owner)
+ login_person(self.owner)
+ self.milestone_name = 'foo'
+ # A ProjectGroup milestone doesn't exist unless one of its
+ # Projects has a milestone of that name.
+ product = self.factory.makeProduct(
+ owner=self.owner, project=self.project_group)
+ self.product_milestone = self.factory.makeMilestone(
+ productseries=product.development_focus,
+ name=self.milestone_name)
+ self.milestone = self.project_group.getMilestone(
+ self.milestone_name)
+
+ def add_bug(self, count):
+ login_person(self.owner)
+ for i in range(count):
+ bug = self.factory.makeBug(product=self.product_milestone.product)
+ bug.bugtasks[0].transitionToMilestone(
+ self.product_milestone, self.owner)
+ # This is necessary to test precaching of assignees.
+ bug.bugtasks[0].transitionToAssignee(
+ self.factory.makePerson())
+ logout()
+
+ def test_bugtasks_queries(self):
+ # The view.bugtasks attribute will make five queries:
+ # 1. For each project in the group load all the dev focus series ids.
+ # 2. Load bugtasks and bugs.
+ # 3. Load assignees (Person, Account, and EmailAddress).
+ # 4. Load links to specifications.
+ # 5. Load links to branches.
+ bugtask_count = 10
+ self.assert_bugtasks_query_count(
+ self.milestone, bugtask_count, query_limit=6)
+
+ def test_milestone_eager_loading(self):
+ # Verify that the number of queries does not increase with more
+ # bugs with different assignees as long as the number of
+ # projects doesn't increase.
+ query_limit = 37
+ self.add_bug(1)
+ self.assert_milestone_page_query_count(
+ self.milestone, query_limit=query_limit)
+ self.add_bug(10)
+ self.assert_milestone_page_query_count(
+ self.milestone, query_limit=query_limit)
+
+
+class TestDistributionMilestoneIndexQueryCount(TestQueryCountBase):
+
+ layer = DatabaseFunctionalLayer
+
+ def setUp(self):
+ super(TestDistributionMilestoneIndexQueryCount, self).setUp()
+ # Increase cache size so that the query counts aren't affected
+ # by objects being removed from the cache early.
+ config.push('storm-cache', dedent('''
+ [launchpad]
+ storm_cache_size: 1000
+ '''))
+ self.addCleanup(config.pop, 'storm-cache')
+ self.ubuntu = getUtility(ILaunchpadCelebrities).ubuntu
+ self.owner = self.factory.makePerson(name='test-owner')
+ login_team(self.ubuntu.owner)
+ self.ubuntu.owner = self.owner
+ self.sourcepackagename = self.factory.makeSourcePackageName(
+ 'foo-package')
+ login_person(self.owner)
+ self.milestone = self.factory.makeMilestone(
+ distribution=self.ubuntu)
+
+ def add_bug(self, count):
+ login_person(self.owner)
+ for i in range(count):
+ bug = self.factory.makeBug(distribution=self.ubuntu)
+ distrosourcepackage = self.factory.makeDistributionSourcePackage(
+ distribution=self.ubuntu)
+ bug.bugtasks[0].transitionToTarget(distrosourcepackage)
+ bug.bugtasks[0].transitionToMilestone(
+ self.milestone, self.owner)
+ # This is necessary to test precaching of assignees.
+ bug.bugtasks[0].transitionToAssignee(
+ self.factory.makePerson())
+ logout()
+
+ def test_bugtasks_queries(self):
+ # The view.bugtasks attribute will make seven queries:
+ # 1. Load ubuntu.currentseries.
+ # 2. Check if the user is in the admin team.
+ # 3. Check if the user is in the owner of the admin team.
+ # 4. Load bugtasks and bugs.
+ # 5. Load assignees (Person, Account, and EmailAddress).
+ # 6. Load links to specifications.
+ # 7. Load links to branches.
+ bugtask_count = 10
+ self.assert_bugtasks_query_count(
+ self.milestone, bugtask_count, query_limit=8)
+
+ def test_milestone_eager_loading(self):
+ # Verify that the number of queries does not increase with more
+ # bugs with different assignees.
+ query_limit = 32
+ self.add_bug(3)
+ self.assert_milestone_page_query_count(
+ self.milestone, query_limit=query_limit)
+ self.add_bug(10)
+ self.assert_milestone_page_query_count(
+ self.milestone, query_limit=query_limit)
=== modified file 'lib/lp/registry/interfaces/person.py'
--- lib/lp/registry/interfaces/person.py 2010-12-10 20:54:12 +0000
+++ lib/lp/registry/interfaces/person.py 2010-12-15 16:58:33 +0000
@@ -2151,6 +2151,24 @@
def cacheBrandingForPeople(people):
"""Prefetch Librarian aliases and content for personal images."""
+ def getPrecachedPersonsFromIDs(
+ person_ids, need_karma=False, need_ubuntu_coc=False,
+ need_location=False, need_archive=False,
+ need_preferred_email=False, need_validity=False):
+ """Lookup person objects from ids with optional precaching.
+
+ :param person_ids: List of person ids.
+ :param need_karma: The karma attribute will be cached.
+ :param need_ubuntu_coc: The is_ubuntu_coc_signer attribute will be
+ cached.
+ :param need_location: The location attribute will be cached.
+ :param need_archive: The archive attribute will be cached.
+ :param need_preferred_email: The preferred email attribute will be
+ cached.
+ :param need_validity: The is_valid attribute will be cached.
+ """
+
+
class IRequestPeopleMerge(Interface):
"""This schema is used only because we want a very specific vocabulary."""
=== modified file 'lib/lp/registry/model/person.py'
--- lib/lp/registry/model/person.py 2010-12-10 14:58:31 +0000
+++ lib/lp/registry/model/person.py 2010-12-15 16:58:33 +0000
@@ -1672,7 +1672,6 @@
# The difference between the two is that
# getMembersWithPreferredEmails includes self, which is arguably
# wrong, but perhaps deliberate.
- store = Store.of(self)
origin = [
Person,
Join(TeamParticipation, TeamParticipation.person == Person.id),
@@ -1682,99 +1681,12 @@
TeamParticipation.team == self.id,
# But not the team itself.
TeamParticipation.person != self.id)
- columns = [Person]
- decorators = []
- if need_karma:
- # New people have no karmatotalcache rows.
- origin.append(
- LeftJoin(KarmaTotalCache,
- KarmaTotalCache.person == Person.id))
- columns.append(KarmaTotalCache)
- if need_ubuntu_coc:
- columns.append(Alias(Exists(Select(SignedCodeOfConduct,
- And(Person._is_ubuntu_coc_signer_condition(),
- SignedCodeOfConduct.ownerID == Person.id))),
- name='is_ubuntu_coc_signer'))
- if need_location:
- # New people have no location rows
- origin.append(
- LeftJoin(PersonLocation,
- PersonLocation.person == Person.id))
- columns.append(PersonLocation)
- if need_archive:
- # Not everyone has PPA's
- # It would be nice to cleanly expose the soyuz rules for this to
- # avoid duplicating the relationships.
- origin.append(
- LeftJoin(Archive, Archive.owner == Person.id))
- columns.append(Archive)
- conditions = And(conditions,
- Or(Archive.id == None, And(
- Archive.id == Select(Min(Archive.id),
- Archive.owner == Person.id, Archive),
- Archive.purpose == ArchivePurpose.PPA)))
- # checking validity requires having a preferred email.
- if need_preferred_email and not need_validity:
- # Teams don't have email, so a left join
- origin.append(
- LeftJoin(EmailAddress, EmailAddress.person == Person.id))
- columns.append(EmailAddress)
- conditions = And(conditions,
- Or(EmailAddress.status == None,
- EmailAddress.status == EmailAddressStatus.PREFERRED))
- if need_validity:
- valid_stuff = Person._validity_queries()
- origin.extend(valid_stuff["joins"])
- columns.extend(valid_stuff["tables"])
- decorators.extend(valid_stuff["decorators"])
- if len(columns) == 1:
- columns = columns[0]
- # Return a simple ResultSet
- return store.using(*origin).find(columns, conditions)
- # Adapt the result into a cached Person.
- columns = tuple(columns)
- raw_result = store.using(*origin).find(columns, conditions)
-
- def prepopulate_person(row):
- result = row[0]
- cache = get_property_cache(result)
- index = 1
- #-- karma caching
- if need_karma:
- karma = row[index]
- index += 1
- if karma is None:
- karma_total = 0
- else:
- karma_total = karma.karma_total
- cache.karma = karma_total
- #-- ubuntu code of conduct signer status caching.
- if need_ubuntu_coc:
- signed = row[index]
- index += 1
- cache.is_ubuntu_coc_signer = signed
- #-- location caching
- if need_location:
- location = row[index]
- index += 1
- cache.location = location
- #-- archive caching
- if need_archive:
- archive = row[index]
- index += 1
- cache.archive = archive
- #-- preferred email caching
- if need_preferred_email and not need_validity:
- email = row[index]
- index += 1
- cache.preferredemail = email
- for decorator in decorators:
- column = row[index]
- index += 1
- decorator(result, column)
- return result
- return DecoratedResultSet(raw_result,
- result_decorator=prepopulate_person)
+ return PersonSet()._getPrecachedPersons(
+ origin, conditions, store=Store.of(self),
+ need_karma=need_karma, need_ubuntu_coc=need_ubuntu_coc,
+ need_location=need_location, need_archive=need_archive,
+ need_preferred_email=need_preferred_email,
+ need_validity=need_validity)
def _getMembersWithPreferredEmails(self):
"""Helper method for public getMembersWithPreferredEmails.
@@ -4129,6 +4041,137 @@
list(LibraryFileAlias.select("LibraryFileAlias.id IN %s"
% sqlvalues(aliases), prejoins=["content"]))
+ def getPrecachedPersonsFromIDs(
+ self, person_ids, need_karma=False, need_ubuntu_coc=False,
+ need_location=False, need_archive=False,
+ need_preferred_email=False, need_validity=False):
+ """See `IPersonSet`."""
+ origin = [Person]
+ conditions = [
+ Person.id.is_in(person_ids)]
+ return self._getPrecachedPersons(
+ origin, conditions,
+ need_karma=need_karma, need_ubuntu_coc=need_ubuntu_coc,
+ need_location=need_location, need_archive=need_archive,
+ need_preferred_email=need_preferred_email,
+ need_validity=need_validity)
+
+ def _getPrecachedPersons(
+ self, origin, conditions, store=None,
+ need_karma=False, need_ubuntu_coc=False,
+ need_location=False, need_archive=False, need_preferred_email=False,
+ need_validity=False):
+ """Lookup all members of the team with optional precaching.
+
+ :param store: Provide ability to specify the store.
+ :param origin: List of storm tables and joins. This list will be
+ appended to. The Person table is required.
+ :param conditions: Storm conditions for tables in origin.
+ :param need_karma: The karma attribute will be cached.
+ :param need_ubuntu_coc: The is_ubuntu_coc_signer attribute will be
+ cached.
+ :param need_location: The location attribute will be cached.
+ :param need_archive: The archive attribute will be cached.
+ :param need_preferred_email: The preferred email attribute will be
+ cached.
+ :param need_validity: The is_valid attribute will be cached.
+ """
+ if store is None:
+ store = IStore(Person)
+ columns = [Person]
+ decorators = []
+ if need_karma:
+ # New people have no karmatotalcache rows.
+ origin.append(
+ LeftJoin(KarmaTotalCache,
+ KarmaTotalCache.person == Person.id))
+ columns.append(KarmaTotalCache)
+ if need_ubuntu_coc:
+ columns.append(Alias(Exists(Select(SignedCodeOfConduct,
+ And(Person._is_ubuntu_coc_signer_condition(),
+ SignedCodeOfConduct.ownerID == Person.id))),
+ name='is_ubuntu_coc_signer'))
+ if need_location:
+ # New people have no location rows
+ origin.append(
+ LeftJoin(PersonLocation,
+ PersonLocation.person == Person.id))
+ columns.append(PersonLocation)
+ if need_archive:
+ # Not everyone has PPA's
+ # It would be nice to cleanly expose the soyuz rules for this to
+ # avoid duplicating the relationships.
+ origin.append(
+ LeftJoin(Archive, Archive.owner == Person.id))
+ columns.append(Archive)
+ conditions = And(conditions,
+ Or(Archive.id == None, And(
+ Archive.id == Select(Min(Archive.id),
+ Archive.owner == Person.id, Archive),
+ Archive.purpose == ArchivePurpose.PPA)))
+ # checking validity requires having a preferred email.
+ if need_preferred_email and not need_validity:
+ # Teams don't have email, so a left join
+ origin.append(
+ LeftJoin(EmailAddress, EmailAddress.person == Person.id))
+ columns.append(EmailAddress)
+ conditions = And(conditions,
+ Or(EmailAddress.status == None,
+ EmailAddress.status == EmailAddressStatus.PREFERRED))
+ if need_validity:
+ valid_stuff = Person._validity_queries()
+ origin.extend(valid_stuff["joins"])
+ columns.extend(valid_stuff["tables"])
+ decorators.extend(valid_stuff["decorators"])
+ if len(columns) == 1:
+ columns = columns[0]
+ # Return a simple ResultSet
+ return store.using(*origin).find(columns, conditions)
+ # Adapt the result into a cached Person.
+ columns = tuple(columns)
+ raw_result = store.using(*origin).find(columns, conditions)
+
+ def prepopulate_person(row):
+ result = row[0]
+ cache = get_property_cache(result)
+ index = 1
+ #-- karma caching
+ if need_karma:
+ karma = row[index]
+ index += 1
+ if karma is None:
+ karma_total = 0
+ else:
+ karma_total = karma.karma_total
+ cache.karma = karma_total
+ #-- ubuntu code of conduct signer status caching.
+ if need_ubuntu_coc:
+ signed = row[index]
+ index += 1
+ cache.is_ubuntu_coc_signer = signed
+ #-- location caching
+ if need_location:
+ location = row[index]
+ index += 1
+ cache.location = location
+ #-- archive caching
+ if need_archive:
+ archive = row[index]
+ index += 1
+ cache.archive = archive
+ #-- preferred email caching
+ if need_preferred_email and not need_validity:
+ email = row[index]
+ index += 1
+ cache.preferredemail = email
+ for decorator in decorators:
+ column = row[index]
+ index += 1
+ decorator(result, column)
+ return result
+ return DecoratedResultSet(raw_result,
+ result_decorator=prepopulate_person)
+
# Provide a storm alias from Person to Owner. This is useful in queries on
# objects that have more than just an owner associated with them.
=== modified file 'lib/lp/registry/tests/test_person.py'
--- lib/lp/registry/tests/test_person.py 2010-12-10 07:52:21 +0000
+++ lib/lp/registry/tests/test_person.py 2010-12-15 16:58:33 +0000
@@ -67,6 +67,7 @@
login_person,
logout,
person_logged_in,
+ StormStatementRecorder,
TestCase,
TestCaseWithFactory,
)
@@ -428,12 +429,12 @@
self.assertEqual('(\\u0170-tester)>', displayname)
-class TestPersonSet(TestCase):
+class TestPersonSet(TestCaseWithFactory):
"""Test `IPersonSet`."""
layer = DatabaseFunctionalLayer
def setUp(self):
- TestCase.setUp(self)
+ super(TestPersonSet, self).setUp()
login(ANONYMOUS)
self.addCleanup(logout)
self.person_set = getUtility(IPersonSet)
@@ -457,6 +458,31 @@
"PersonSet.getByEmail() should ignore case and whitespace.")
self.assertEqual(person1, person2)
+ def test_getPrecachedPersonsFromIDs(self):
+ # The getPrecachedPersonsFromIDs() method should only make one
+ # query to load all the extraneous data. Accessing the
+ # attributes should then cause zero queries.
+ person_ids = [
+ self.factory.makePerson().id
+ for i in range(3)]
+
+ with StormStatementRecorder() as recorder:
+ persons = list(self.person_set.getPrecachedPersonsFromIDs(
+ person_ids, need_karma=True, need_ubuntu_coc=True,
+ need_location=True, need_archive=True,
+ need_preferred_email=True, need_validity=True))
+ self.assertThat(recorder, HasQueryCount(LessThan(2)))
+
+ with StormStatementRecorder() as recorder:
+ for person in persons:
+ person.is_valid_person
+ person.karma
+ person.is_ubuntu_coc_signer
+ person.location
+ person.archive
+ person.preferredemail
+ self.assertThat(recorder, HasQueryCount(LessThan(1)))
+
class KarmaTestMixin:
"""Helper methods for setting karma."""
