launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #07860
[Merge] lp:~wallyworld/launchpad/sharing-details-timeout-997343 into lp:launchpad
Ian Booth has proposed merging lp:~wallyworld/launchpad/sharing-details-timeout-997343 into lp:launchpad.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
Related bugs:
Bug #997343 in Launchpad itself: "Timeout trying to see sharing details page for ~launchpad-security"
https://bugs.launchpad.net/launchpad/+bug/997343
For more details, see:
https://code.launchpad.net/~wallyworld/launchpad/sharing-details-timeout-997343/+merge/105774
== Implementation ==
The sharing details view was dying a death of 1000 queries. Each bug or branch to be displayed was fetched individually. To find which artifacts to display, the sharing service getSharedArtifacts() method was invoked. This returned AccessArtifact objects, which contain a reference to the bug/branch. Each reference was loaded in a separate query instead of being bulk loaded.
Another issue was that the getSharedArtifacts() API was implemnented to return all shared bugs/branches, regardless of whether the caller could see them or not. The view itself then had to do the filtering. And it only did it for bugs.
The core of the implementation is for getSharedArtifacts() to return a (bugtasks, branches) tuple where the artifacts are bulk loaded by the service. In order for this to work, the service needed to get the ids of the shared bugs and branches and use this with the bugtask and branch query infrastructure to do the bulk loading. So the bug_id and branch_id attributes were added to the IAccessArtifact interface to allow access.
Also, the view really required bugtasks to work with rather than bugs. The underlying implementation uses the bugtask search API which filters the bugtasks on the pillar. The view uses the information from the bugtask directly. So the implementation returns bugtasks not bugs. The original view code itself in places used to refer to bugs when the data was bugtasks so now it's all consistent.
The BranchCollection infrastructure needed a little love. It required a new filter method, withIds(), to allow only the branches for the shared ids to be loaded.
== Demo and QA ==
To quote the bug report:
"The page to see the details shared with a given team or user (e.g. https://qastaging.launchpad.net/launchpad/+sharing/launchpad-security) constantly times out when that user is ~launchpad-security"
== Tests ==
Add new test for BranchCollection.withIds()
There was no test for the sharing service getSharedArtifacts() so a new one was added.
A test was added to PillarSharingDetailsMixin to ensure the query count is limited to a reasonable value.
The existing pillar sharing details view tests check the view content is correctly loaded still.
== Lint ==
Checking for conflicts and issues in changed files.
Linting changed files:
lib/lp/code/interfaces/branchcollection.py
lib/lp/code/model/branchcollection.py
lib/lp/code/model/tests/test_branchcollection.py
lib/lp/registry/browser/pillar.py
lib/lp/registry/browser/tests/test_pillar_sharing.py
lib/lp/registry/interfaces/accesspolicy.py
lib/lp/registry/interfaces/sharingservice.py
lib/lp/registry/services/sharingservice.py
lib/lp/registry/services/tests/test_sharingservice.py
--
https://code.launchpad.net/~wallyworld/launchpad/sharing-details-timeout-997343/+merge/105774
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~wallyworld/launchpad/sharing-details-timeout-997343 into lp:launchpad.
=== modified file 'lib/lp/code/interfaces/branchcollection.py'
--- lib/lp/code/interfaces/branchcollection.py 2011-11-01 08:17:57 +0000
+++ lib/lp/code/interfaces/branchcollection.py 2012-05-15 08:40:23 +0000
@@ -212,6 +212,9 @@
:param since: If supplied, ignore merge proposals before this date.
"""
+ def withIds(*branch_ids):
+ """Restrict the collection to branches with the specified ids."""
+
class IAllBranches(IBranchCollection):
"""A `IBranchCollection` representing all branches in Launchpad."""
=== modified file 'lib/lp/code/model/branchcollection.py'
--- lib/lp/code/model/branchcollection.py 2011-12-30 06:14:56 +0000
+++ lib/lp/code/model/branchcollection.py 2012-05-15 08:40:23 +0000
@@ -740,6 +740,10 @@
"""See `IBranchCollection`."""
return self._filterBy([Branch.last_scanned > epoch], symmetric=False)
+ def withIds(self, *branch_ids):
+ """See `IBranchCollection`."""
+ return self._filterBy([Branch.id.is_in(branch_ids)], symmetric=False)
+
class AnonymousBranchCollection(GenericBranchCollection):
"""Branch collection that only shows public branches."""
=== modified file 'lib/lp/code/model/tests/test_branchcollection.py'
--- lib/lp/code/model/tests/test_branchcollection.py 2012-05-03 00:08:11 +0000
+++ lib/lp/code/model/tests/test_branchcollection.py 2012-05-15 08:40:23 +0000
@@ -430,6 +430,18 @@
sorted([branch1, branch3, branch4]),
sorted(collection.getBranches()))
+ def test_withIds(self):
+ # 'withIds' returns a new collection that only has branches with the
+ # given ids.
+ branch1 = self.factory.makeAnyBranch()
+ branch2 = self.factory.makeAnyBranch()
+ self.factory.makeAnyBranch()
+ ids = [branch1.id, branch2.id]
+ collection = self.all_branches.withIds(*ids)
+ self.assertEqual(
+ sorted([branch1, branch2]),
+ sorted(collection.getBranches()))
+
def test_registeredBy(self):
# 'registeredBy' returns a new collection that only has branches that
# were registered by the given user.
=== modified file 'lib/lp/registry/browser/pillar.py'
--- lib/lp/registry/browser/pillar.py 2012-04-12 09:22:03 +0000
+++ lib/lp/registry/browser/pillar.py 2012-05-15 08:40:23 +0000
@@ -42,21 +42,13 @@
from lp.bugs.browser.structuralsubscription import (
StructuralSubscriptionMenuMixin,
)
-from lp.bugs.interfaces.bug import IBug
-from lp.bugs.interfaces.bugtask import (
- BugTaskSearchParams,
- IBugTaskSet,
- )
-from lp.code.interfaces.branch import IBranch
from lp.registry.enums import InformationType
-from lp.registry.interfaces.distribution import IDistribution
from lp.registry.interfaces.distributionsourcepackage import (
IDistributionSourcePackage,
)
from lp.registry.interfaces.distroseries import IDistroSeries
from lp.registry.interfaces.person import IPersonSet
from lp.registry.interfaces.pillar import IPillar
-from lp.registry.interfaces.product import IProduct
from lp.registry.interfaces.projectgroup import IProjectGroup
from lp.registry.model.pillar import PillarPerson
from lp.services.config import config
@@ -391,7 +383,7 @@
cache = IJSONRequestCache(self.request)
request = get_current_web_service_request()
branch_data = self._build_branch_template_data(self.branches, request)
- bug_data = self._build_bug_template_data(self.bugs, request)
+ bug_data = self._build_bug_template_data(self.bugtasks, request)
sharee_data = {
'displayname': self.person.displayname,
'self_link': absoluteURL(self.person, request)
@@ -409,42 +401,14 @@
cache.objects['sharing_write_enabled'] = (write_flag_enabled
and check_permission('launchpad.Edit', self.pillar))
- def _getSafeBugs(self, bugs):
- """Uses the bugsearch tools to safely get the list of bugs the user is
- allowed to see."""
- if bugs == set([]):
- return []
- params = []
- for b in bugs:
- param = BugTaskSearchParams(user=self.user, bug=b)
- if IProduct.providedBy(self.pillar):
- param.setProduct(self.pillar)
- elif IDistribution.providedBy(self.pillar):
- param.setDistribution(self.pillar)
- params.append(param)
-
- safe_bugs = getUtility(IBugTaskSet).search(params[0], *params[1:])
- return list(safe_bugs)
-
def _loadSharedArtifacts(self):
# As a concrete can by linked via more than one policy, we use sets to
# filter out dupes.
- bugs = set()
- branches = set()
- for artifact in self.sharing_service.getSharedArtifacts(
- self.pillar, self.person):
- concrete = artifact.concrete_artifact
- if IBug.providedBy(concrete):
- bugs.add(concrete)
- elif IBranch.providedBy(concrete):
- branches.add(concrete)
-
- # For security reasons, the bugs have to be refetched by ID through
- # the normal querying mechanism. This prevents bugs the user shouldn't
- # be able to see from being displayed.
- self.bugs = self._getSafeBugs(bugs)
- self.branches = branches
- self.shared_bugs_count = len(self.bugs)
+ self.bugtasks, self.branches = (
+ self.sharing_service.getSharedArtifacts(
+ self.pillar, self.person, self.user))
+ bug_ids = set([bugtask.bug.id for bugtask in self.bugtasks])
+ self.shared_bugs_count = len(bug_ids)
self.shared_branches_count = len(self.branches)
def _build_branch_template_data(self, branches, request):
=== modified file 'lib/lp/registry/browser/tests/test_pillar_sharing.py'
--- lib/lp/registry/browser/tests/test_pillar_sharing.py 2012-05-13 23:59:55 +0000
+++ lib/lp/registry/browser/tests/test_pillar_sharing.py 2012-05-15 08:40:23 +0000
@@ -24,6 +24,7 @@
from lp.registry.interfaces.accesspolicy import IAccessPolicyGrantFlatSource
from lp.registry.model.pillar import PillarPerson
from lp.services.config import config
+from lp.services.database.lpstorm import IStore
from lp.services.features.testing import FeatureFixture
from lp.services.webapp.interfaces import StormRangeFactoryError
from lp.services.webapp.publisher import canonical_url
@@ -193,7 +194,7 @@
with FeatureFixture(DETAILS_ENABLED_FLAG):
pillarperson = self.getPillarPerson()
view = create_initialized_view(pillarperson, '+index')
- bugtask = list(view.bugs)[0]
+ bugtask = list(view.bugtasks)[0]
bug = bugtask.bug
cache = IJSONRequestCache(view.request)
request = get_current_web_service_request()
@@ -224,6 +225,20 @@
'self_link': absoluteURL(branch, request),
}, cache.objects.get('branches')[0])
+ def test_view_query_count(self):
+ # Test that the view bulk loads artifacts.
+ with FeatureFixture(DETAILS_ENABLED_FLAG):
+ person = self.factory.makePerson()
+ for x in range(0, 15):
+ self.makeArtifactGrantee(person, True, True, False)
+ pillarperson = PillarPerson(self.pillar, person)
+
+ # Invalidate the Storm cache and check the query count.
+ IStore(self.pillar).invalidate()
+ with StormStatementRecorder() as recorder:
+ create_initialized_view(pillarperson, '+index')
+ self.assertThat(recorder, HasQueryCount(LessThan(12)))
+
def test_view_write_enabled_without_feature_flag(self):
# Test that sharing_write_enabled is not set without the feature flag.
with FeatureFixture(DETAILS_ENABLED_FLAG):
=== modified file 'lib/lp/registry/interfaces/accesspolicy.py'
--- lib/lp/registry/interfaces/accesspolicy.py 2012-04-26 08:12:23 +0000
+++ lib/lp/registry/interfaces/accesspolicy.py 2012-05-15 08:40:23 +0000
@@ -33,6 +33,8 @@
id = Attribute("ID")
concrete_artifact = Attribute("Concrete artifact")
+ bug_id = Attribute("bug_id")
+ branch_id = Attribute("branch_id")
class IAccessArtifactGrant(Interface):
=== modified file 'lib/lp/registry/interfaces/sharingservice.py'
--- lib/lp/registry/interfaces/sharingservice.py 2012-04-13 01:06:53 +0000
+++ lib/lp/registry/interfaces/sharingservice.py 2012-05-15 08:40:23 +0000
@@ -45,8 +45,18 @@
# version 'devel'
export_as_webservice_entry(publish_web_link=False, as_of='beta')
- def getSharedArtifacts(pillar, person):
- """Return the artifacts shared between the pillar and person."""
+ def getSharedArtifacts(pillar, person, user):
+ """Return the artifacts shared between the pillar and person.
+
+ The result includes bugtasks rather than bugs since this is what the
+ pillar filtering is applied to and is what the calling code uses.
+ The shared bug can be obtained simply by reading the bugtask.bug
+ attribute.
+
+ :param user: the user making the request. Only artifacts visible to the
+ user will be included in the result.
+ :return: a (bugtasks, branches) tuple
+ """
def getInformationTypes(pillar):
"""Return the allowed information types for the given pillar."""
=== modified file 'lib/lp/registry/services/sharingservice.py'
--- lib/lp/registry/services/sharingservice.py 2012-04-26 07:54:34 +0000
+++ lib/lp/registry/services/sharingservice.py 2012-05-15 08:40:23 +0000
@@ -15,6 +15,11 @@
from zope.security.interfaces import Unauthorized
from zope.traversing.browser.absoluteurl import absoluteURL
+from lp.bugs.interfaces.bugtask import (
+ BugTaskSearchParams,
+ IBugTaskSet,
+ )
+from lp.code.interfaces.branchcollection import IAllBranches
from lp.registry.enums import (
InformationType,
SharingPermission,
@@ -26,11 +31,13 @@
IAccessPolicyGrantSource,
IAccessPolicySource,
)
+from lp.registry.interfaces.distribution import IDistribution
from lp.registry.interfaces.product import IProduct
from lp.registry.interfaces.projectgroup import IProjectGroup
from lp.registry.interfaces.sharingservice import ISharingService
from lp.registry.model.person import Person
from lp.services.features import getFeatureFlag
+from lp.services.searchbuilder import any
from lp.services.webapp.authorization import available_with_permission
@@ -53,11 +60,36 @@
return bool(getFeatureFlag(
'disclosure.enhanced_sharing.writable'))
- def getSharedArtifacts(self, pillar, person):
+ def getSharedArtifacts(self, pillar, person, user):
+ """See `ISharingService`."""
policies = getUtility(IAccessPolicySource).findByPillar([pillar])
flat_source = getUtility(IAccessPolicyGrantFlatSource)
- return [a for a in
- flat_source.findArtifactsByGrantee(person, policies)]
+ bug_ids = set()
+ branch_ids = set()
+ for artifact in flat_source.findArtifactsByGrantee(person, policies):
+ if artifact.bug_id:
+ bug_ids.add(artifact.bug_id)
+ elif artifact.branch_id:
+ branch_ids.add(artifact.branch_id)
+
+ # Load the bugs.
+ bugtasks = []
+ if bug_ids:
+ param = BugTaskSearchParams(user=user, bug=any(*bug_ids))
+ if IProduct.providedBy(pillar):
+ param.setProduct(pillar)
+ elif IDistribution.providedBy(pillar):
+ param.setDistribution(pillar)
+ bugtasks = list(getUtility(IBugTaskSet).search(param))
+ # Load the branches.
+ branches = []
+ if branch_ids:
+ all_branches = getUtility(IAllBranches).visibleByUser(user)
+ wanted_branches = all_branches.visibleByUser(user).withIds(
+ *branch_ids)
+ branches = list(wanted_branches.getBranches())
+
+ return bugtasks, branches
def getInformationTypes(self, pillar):
"""See `ISharingService`."""
=== modified file 'lib/lp/registry/services/tests/test_sharingservice.py'
--- lib/lp/registry/services/tests/test_sharingservice.py 2012-05-02 05:25:11 +0000
+++ lib/lp/registry/services/tests/test_sharingservice.py 2012-05-15 08:40:23 +0000
@@ -13,6 +13,11 @@
from zope.traversing.browser.absoluteurl import absoluteURL
from lp.app.interfaces.services import IService
+from lp.code.enums import (
+ BranchSubscriptionDiffSize,
+ BranchSubscriptionNotificationLevel,
+ CodeReviewNotificationLevel,
+ )
from lp.registry.enums import (
InformationType,
SharingPermission,
@@ -695,6 +700,68 @@
Unauthorized, self.service.revokeAccessGrants,
product, sharee, bugs=[bug])
+ def test_getSharedArtifacts(self):
+ # Test the getSharedArtifacts method.
+ owner = self.factory.makePerson()
+ product = self.factory.makeProduct(owner=owner)
+ login_person(owner)
+
+ bugs = []
+ bug_tasks = []
+ for x in range(0, 10):
+ bug = self.factory.makeBug(
+ product=product, owner=owner,
+ information_type=InformationType.USERDATA)
+ bugs.append(bug)
+ bug_tasks.append(bug.default_bugtask)
+ branches = []
+ for x in range(0, 10):
+ branch = self.factory.makeBranch(
+ product=product, owner=owner, private=True)
+ branches.append(branch)
+
+ # Grant access to grantee as well as the person who will be doing the
+ # query. The person who will be doing the query is not granted access
+ # to the last bug/branch so those won't be in the result.
+ grantee = self.factory.makePerson()
+ user = self.factory.makePerson()
+
+ def grant_access(artifact, grantee_only):
+ access_artifact = self.factory.makeAccessArtifact(
+ concrete=artifact)
+ self.factory.makeAccessArtifactGrant(
+ artifact=access_artifact, grantee=grantee, grantor=owner)
+ if not grantee_only:
+ self.factory.makeAccessArtifactGrant(
+ artifact=access_artifact, grantee=user, grantor=owner)
+ return access_artifact
+
+ for i, bug in enumerate(bugs):
+ grant_access(bug, i == 9)
+ # For branches we also need to call makeAccessPolicyArtifact.
+ [policy] = getUtility(IAccessPolicySource).find(
+ [(product, InformationType.USERDATA)])
+ for i, branch in enumerate(branches):
+ artifact = grant_access(branch, i == 9)
+ # XXX for now we need to subscribe users to the branch in order
+ # for the underlying BranchCollection to allow access. This will
+ # no longer be the case when BranchCollection supports the new
+ # access policy framework.
+ if i < 9:
+ branch.subscribe(
+ user, BranchSubscriptionNotificationLevel.NOEMAIL,
+ BranchSubscriptionDiffSize.NODIFF,
+ CodeReviewNotificationLevel.NOEMAIL,
+ owner)
+ self.factory.makeAccessPolicyArtifact(
+ artifact=artifact, policy=policy)
+
+ # Check the results.
+ shared_bugtasks, shared_branches = self.service.getSharedArtifacts(
+ product, grantee, user)
+ self.assertContentEqual(bug_tasks[:9], shared_bugtasks)
+ self.assertContentEqual(branches[:9], shared_branches)
+
class ApiTestMixin:
"""Common tests for launchpadlib and webservice."""
Follow ups
