launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #00628
[Merge] lp:~lifeless/launchpad/milestones into lp:launchpad/devel
Robert Collins has proposed merging lp:~lifeless/launchpad/milestones into lp:launchpad/devel.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
Hopefully fix the current instantiation of https://bugs.edge.launchpad.net/launchpad-registry/+bug/447418.
Private bug access checking is rather slow: https://bugs.edge.launchpad.net/malone/+bug/619039.
Solution - a cached attribute recording people that we've calculated should have view access, prepopulated by BugTaskSet.search().
Added tests for the low level behaviour and that it fixes the observed scaling problem in a simple milestone view.
\o/
--
https://code.launchpad.net/~lifeless/launchpad/milestones/+merge/32855
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~lifeless/launchpad/milestones into lp:launchpad/devel.
=== modified file 'lib/canonical/launchpad/security.py'
--- lib/canonical/launchpad/security.py 2010-08-13 01:43:26 +0000
+++ lib/canonical/launchpad/security.py 2010-08-17 09:41:19 +0000
@@ -920,24 +920,8 @@
usedfor = IHasBug
def checkAuthenticated(self, user):
-
- if user.in_admin:
- # Admins can always edit bugtasks, whether they're reported on a
- # private bug or not.
- return True
-
- if not self.obj.bug.private:
- # This is a public bug, so anyone can edit it.
- return True
- else:
- # This is a private bug, and we know the user isn't an admin, so
- # we'll only allow editing if the user is explicitly subscribed to
- # this bug.
- for subscription in self.obj.bug.subscriptions:
- if user.inTeam(subscription.person):
- return True
-
- return False
+ # Delegated entirely to the bug.
+ return self.obj.bug.userCanView(user)
class PublicToAllOrPrivateToExplicitSubscribersForBugTask(AuthorizationBase):
@@ -959,21 +943,10 @@
def checkAuthenticated(self, user):
"""Allow any logged in user to edit a public bug, and only
- explicit subscribers to edit private bugs.
+ explicit subscribers to edit private bugs. Any bug that can be seen can
+ be edited.
"""
- if not self.obj.private:
- # This is a public bug.
- return True
- elif user.in_admin:
- # Admins can edit all bugs.
- return True
- else:
- # This is a private bug. Only explicit subscribers may edit it.
- for subscription in self.obj.subscriptions:
- if user.inTeam(subscription.person):
- return True
-
- return False
+ return self.obj.userCanView(user)
def checkUnauthenticated(self):
"""Never allow unauthenticated users to edit a bug."""
=== modified file 'lib/canonical/launchpad/utilities/personroles.py'
--- lib/canonical/launchpad/utilities/personroles.py 2010-01-14 11:28:51 +0000
+++ lib/canonical/launchpad/utilities/personroles.py 2010-08-17 09:41:19 +0000
@@ -35,6 +35,10 @@
except AttributeError:
raise AttributeError(errortext)
+ @property
+ def id(self):
+ return self.person.id
+
def isOwner(self, obj):
"""See IPersonRoles."""
return self.person.inTeam(obj.owner)
=== modified file 'lib/lp/bugs/model/bug.py'
--- lib/lp/bugs/model/bug.py 2010-08-07 14:54:40 +0000
+++ lib/lp/bugs/model/bug.py 2010-08-17 09:41:19 +0000
@@ -42,6 +42,10 @@
ObjectCreatedEvent, ObjectDeletedEvent, ObjectModifiedEvent)
from lazr.lifecycle.snapshot import Snapshot
+from canonical.cachedproperty import (
+ cachedproperty,
+ clear_property,
+ )
from canonical.config import config
from canonical.database.constants import UTC_NOW
from canonical.database.datetimecol import UtcDateTimeCol
@@ -555,6 +559,7 @@
# disabled see the change.
store.flush()
self.updateHeat()
+ clear_property(self, '_cached_viewers')
return
def unsubscribeFromDupes(self, person, unsubscribed_by):
@@ -1547,21 +1552,32 @@
self, self.messages[comment_number])
bug_message.visible = visible
+ @cachedproperty('_cached_viewers')
+ def _known_viewers(self):
+ """A dict of of known persons able to view this bug."""
+ return set()
+
def userCanView(self, user):
- """See `IBug`."""
+ """See `IBug`.
+
+ Note that Editing is also controlled by this check,
+ because we permit editing of any bug one can see.
+ """
+ if user.id in self._known_viewers:
+ return True
admins = getUtility(ILaunchpadCelebrities).admin
if not self.private:
# This is a public bug.
return True
- elif user.inTeam(admins):
+ elif user.in_admin:
# Admins can view all bugs.
return True
else:
# This is a private bug. Only explicit subscribers may view it.
for subscription in self.subscriptions:
if user.inTeam(subscription.person):
+ self._known_viewers.add(user.id)
return True
-
return False
def linkHWSubmission(self, submission):
=== modified file 'lib/lp/bugs/model/bugtask.py'
--- lib/lp/bugs/model/bugtask.py 2010-08-03 08:49:19 +0000
+++ lib/lp/bugs/model/bugtask.py 2010-08-17 09:41:19 +0000
@@ -42,7 +42,7 @@
from lazr.enum import DBItem
from canonical.config import config
-
+from canonical.cachedproperty import cache_property
from canonical.database.sqlbase import (
SQLBase, block_implicit_flushes, convert_storm_clause_to_string, cursor,
quote, quote_like, sqlvalues)
@@ -51,6 +51,7 @@
from canonical.database.nl_search import nl_phrase_search
from canonical.database.enumcol import EnumCol
+from canonical.launchpad.components.decoratedresultset import DecoratedResultSet
from canonical.launchpad.interfaces.lpstorm import IStore
from canonical.launchpad.webapp.interfaces import ILaunchBag
@@ -1534,11 +1535,15 @@
"""Build and return an SQL query with the given parameters.
Also return the clauseTables and orderBy for the generated query.
+
+ :return: A query, the tables to query, ordering expression and a
+ decorator to call on each returned row.
"""
assert isinstance(params, BugTaskSearchParams)
extra_clauses = ['Bug.id = BugTask.bug']
clauseTables = ['BugTask', 'Bug']
+ decorators = []
# These arguments can be processed in a loop without any other
# special handling.
@@ -1814,6 +1819,11 @@
clause = get_bug_privacy_filter(params.user)
if clause:
extra_clauses.append(clause)
+ userid = params.user.id
+ def cache_user_can_view_bug(bugtask):
+ cache_property(bugtask.bug, '_cached_viewers', set([userid]))
+ return bugtask
+ decorators.append(cache_user_can_view_bug)
hw_clause = self._buildHardwareRelatedClause(params)
if hw_clause is not None:
@@ -1842,7 +1852,15 @@
orderby_arg = self._processOrderBy(params)
query = " AND ".join(extra_clauses)
- return query, clauseTables, orderby_arg
+
+ if not decorators:
+ decorator = lambda x:x
+ else:
+ def decorator(obj):
+ for decor in decorators:
+ obj = decor(obj)
+ return obj
+ return query, clauseTables, orderby_arg, decorator
def _buildUpstreamClause(self, params):
"""Return an clause for returning upstream data if the data exists.
@@ -2074,22 +2092,23 @@
"""See `IBugTaskSet`."""
store_selector = getUtility(IStoreSelector)
store = store_selector.get(MAIN_STORE, SLAVE_FLAVOR)
- query, clauseTables, orderby = self.buildQuery(params)
+ query, clauseTables, orderby, decorator = self.buildQuery(params)
if len(args) == 0:
# Do normal prejoins, if we don't have to do any UNION
# queries. Prejoins don't work well with UNION, and the way
# we prefetch objects without prejoins cause problems with
# COUNT(*) queries, which get inefficient.
- return BugTask.select(
+ resultset = BugTask.select(
query, clauseTables=clauseTables, orderBy=orderby,
prejoins=['product', 'sourcepackagename'],
prejoinClauseTables=['Bug'])
+ return DecoratedResultSet(resultset, result_decorator=decorator)
bugtask_fti = SQL('BugTask.fti')
result = store.find((BugTask, bugtask_fti), query,
AutoTables(SQL("1=1"), clauseTables))
for arg in args:
- query, clauseTables, dummy = self.buildQuery(arg)
+ query, clauseTables, dummy, decorator = self.buildQuery(arg)
result = result.union(
store.find((BugTask, bugtask_fti), query,
AutoTables(SQL("1=1"), clauseTables)))
@@ -2108,7 +2127,7 @@
(BugTask, Bug, Product, SourcePackageName))
bugtasks = SQLObjectResultSet(BugTask, orderBy=orderby,
prepared_result_set=result)
- return bugtasks
+ return DecoratedResultSet(bugtasks, result_decorator=decorator)
def getAssignedMilestonesFromSearch(self, search_results):
"""See `IBugTaskSet`."""
=== modified file 'lib/lp/bugs/tests/test_bugtask.py'
--- lib/lp/bugs/tests/test_bugtask.py 2010-07-14 14:11:15 +0000
+++ lib/lp/bugs/tests/test_bugtask.py 2010-08-17 09:41:19 +0000
@@ -20,10 +20,14 @@
from lp.bugs.interfaces.bugtarget import IBugTarget
from lp.bugs.interfaces.bugtask import (
- BugTaskImportance, BugTaskSearchParams, BugTaskStatus)
+ BugTaskImportance,
+ BugTaskSearchParams,
+ BugTaskStatus,
+ IBugTaskSet,
+ )
from lp.bugs.model.bugtask import build_tag_search_clause
from lp.registry.interfaces.distribution import IDistributionSet
-from lp.registry.interfaces.person import IPersonSet
+from lp.registry.interfaces.person import IPerson, IPersonSet
from lp.testing import (
ANONYMOUS, TestCase, TestCaseWithFactory, login, login_person, logout,
normalize_whitespace)
@@ -792,6 +796,28 @@
result = target.searchTasks(None, modified_since=date)
self.assertEqual([task2], list(result))
+ def test_private_bug_view_permissions_cached(self):
+ """private bugs from a search know the user can see the bugs."""
+ target = self.makeBugTarget()
+ person = self.login()
+ self.factory.makeBug(product=target, private=True, owner=person)
+ self.factory.makeBug(product=target, private=True, owner=person)
+ # Search style and parameters taken from the milestone index view where
+ # the issue was discovered.
+ login_person(person)
+ tasks =target.searchTasks(BugTaskSearchParams(person, omit_dupes=True,
+ orderby=['status', '-importance', 'id']))
+ # We must be finding the bugs.
+ self.assertEqual(2, tasks.count())
+ # Cache in the storm cache the account->person lookup so its not
+ # distorting what we're testing.
+ _ = IPerson(person.account, None)
+ # One query and only one should be issued to get the tasks, bugs and
+ # allow access to getConjoinedMaster attribute - an attribute that
+ # triggers a permission check (nb: id does not trigger such a check)
+ self.assertStatementCount(1,
+ lambda:[task.getConjoinedMaster for task in tasks])
+
def test_suite():
suite = unittest.TestSuite()
=== modified file 'lib/lp/registry/browser/tests/test_milestone.py'
--- lib/lp/registry/browser/tests/test_milestone.py 2010-08-16 18:39:11 +0000
+++ lib/lp/registry/browser/tests/test_milestone.py 2010-08-17 09:41:19 +0000
@@ -1,19 +1,30 @@
# Copyright 2010 Canonical Ltd. This software is licensed under the
# GNU Affero General Public License version 3 (see the file LICENSE).
+from __future__ import with_statement
+
"""Test milestone views."""
__metaclass__ = type
-import unittest
-
+from testtools.matchers import LessThan
from zope.component import getUtility
+from canonical.launchpad.webapp import canonical_url
from canonical.testing.layers import DatabaseFunctionalLayer
from lp.bugs.interfaces.bugtask import IBugTaskSet
-from lp.testing import ANONYMOUS, login_person, login, TestCaseWithFactory
+from lp.testing import (
+ ANONYMOUS,
+ login,
+ login_person,
+ logout,
+ person_logged_in,
+ TestCaseWithFactory,
+ )
+from lp.testing.matchers import HasQueryCount
+from lp.testing.memcache import MemcacheTestCase
from lp.testing.views import create_initialized_view
-from lp.testing.memcache import MemcacheTestCase
+from lp.testing._webservice import QueryCollector
class TestMilestoneMemcache(MemcacheTestCase):
@@ -105,5 +116,61 @@
self.assertEqual(0, product.development_focus.all_bugtasks.count())
-def test_suite():
- return unittest.TestLoader().loadTestsFromName(__name__)
+class TestMilestoneIndex(TestCaseWithFactory):
+
+ layer = DatabaseFunctionalLayer
+
+ def test_more_private_bugs_query_count_is_constant(self):
+ product = self.factory.makeProduct()
+ login_person(product.owner)
+ milestone = self.factory.makeMilestone(
+ productseries=product.development_focus)
+ bug1 = self.factory.makeBug(product=product, private=True,
+ owner=product.owner)
+ bug1.bugtasks[0].transitionToMilestone(milestone, product.owner)
+ # We look at the page as someone who is a member of a team and the team
+ # is subscribed to the bugs, so that we don't get trivial shortcuts
+ # avoiding queries : test the worst case.
+ subscribed_team = self.factory.makeTeam()
+ viewer = self.factory.makePerson(password="test")
+ with person_logged_in(subscribed_team.teamowner):
+ subscribed_team.addMember(viewer, subscribed_team.teamowner)
+ bug1.subscribe(subscribed_team, product.owner)
+ bug1_url = canonical_url(bug1)
+ milestone_url = canonical_url(milestone)
+ browser = self.getUserBrowser(user=viewer)
+ # Seed the cookie cache and any other cross-request state we may gain
+ # in future. See canonical.launchpad.webapp.serssion: _get_secret.
+ browser.open(milestone_url)
+ 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)
+ self.assertThat(collector, HasQueryCount(LessThan(page_query_limit)))
+ with_1_private_bug = collector.count
+ with_1_queries = ["%s: %s" % (pos, stmt[3]) for (pos, stmt) in
+ enumerate(collector.queries)]
+ login_person(product.owner)
+ bug2 = self.factory.makeBug(product=product, private=True,
+ owner=product.owner)
+ bug2.bugtasks[0].transitionToMilestone(milestone, product.owner)
+ bug2.subscribe(subscribed_team, product.owner)
+ bug2_url = canonical_url(bug2)
+ bug3 = self.factory.makeBug(product=product, private=True,
+ owner=product.owner)
+ bug3.bugtasks[0].transitionToMilestone(milestone, product.owner)
+ bug3.subscribe(subscribed_team, product.owner)
+ logout()
+ browser.open(milestone_url)
+ self.assertTrue(bug2_url in browser.contents)
+ self.assertThat(collector, HasQueryCount(LessThan(page_query_limit)))
+ with_3_private_bugs = collector.count
+ with_3_queries = ["%s: %s" % (pos, stmt[3]) for (pos, stmt) in
+ enumerate(collector.queries)]
+ 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)))
=== modified file 'lib/lp/testing/factory.py'
--- lib/lp/testing/factory.py 2010-08-16 21:51:28 +0000
+++ lib/lp/testing/factory.py 2010-08-17 09:41:19 +0000
@@ -7,7 +7,7 @@
"""Testing infrastructure for the Launchpad application.
-This module should not have any actual tests.
+This module should not contain tests (but it should be tested).
"""
__metaclass__ = type
Follow ups
