launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #06863
[Merge] lp:~wgrant/launchpad/sharing-prettier-sql into lp:launchpad
William Grant has proposed merging lp:~wgrant/launchpad/sharing-prettier-sql into lp:launchpad.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
Related bugs:
Bug #962751 in Launchpad itself: "findGranteePermissionsByPolicy's primary query is slow"
https://bugs.launchpad.net/launchpad/+bug/962751
For more details, see:
https://code.launchpad.net/~wgrant/launchpad/sharing-prettier-sql/+merge/98967
This branch reworks findGranteePermissionsByPolicy to be fast and to batch correctly.
Currently the primary query sorts the people then eliminates duplicates. But people often have tens or even thousands of rows, so we save a lot of time by doing uniqueness first. I moved the distinct into a subquery to ensure this, and for Ubuntu it's roughly 4x faster on DF than the old query on qastaging.
Batches derived from the method's output usually have less than the desired number of rows. This is because the UI slices by person, but the method slices by (person, policy), so people with more than one policy will cause fewer people to be returned. I reworked findGranteePermissionsByPolicy and jsonShareeData to handle one result row per person containing a dict of {policy: permission, [...]}, instead of multiple rows each containing a single (policy, permission).
--
https://code.launchpad.net/~wgrant/launchpad/sharing-prettier-sql/+merge/98967
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~wgrant/launchpad/sharing-prettier-sql into lp:launchpad.
=== modified file 'lib/lp/registry/model/accesspolicy.py'
--- lib/lp/registry/model/accesspolicy.py 2012-03-22 13:01:27 +0000
+++ lib/lp/registry/model/accesspolicy.py 2012-03-23 04:21:26 +0000
@@ -11,10 +11,14 @@
'AccessPolicyGrant',
]
+from collections import defaultdict
+
import pytz
from storm.expr import (
And,
+ In,
Or,
+ Select,
SQL,
)
from storm.properties import (
@@ -353,51 +357,48 @@
@classmethod
def findGranteePermissionsByPolicy(cls, policies, grantees=None):
"""See `IAccessPolicyGrantFlatSource`."""
- ids = [policy.id for policy in policies]
-
- # A cache for the sharing permissions, keyed on (grantee.id, policy.id)
- permissions_cache = {}
-
- def set_permission(row):
- # row contains (grantee.id, policy.id, permission_placeholder)
- # Lookup the permission from the previously loaded cache.
- return (
- row[0], row[1], permissions_cache.get((row[0].id, row[1].id)))
-
- def load_permissions(rows):
+ policies_by_id = dict((policy.id, policy) for policy in policies)
+
+ # A cache for the sharing permissions, keyed on grantee
+ permissions_cache = defaultdict(dict)
+
+ def set_permission(person):
+ # Lookup the permissions from the previously loaded cache.
+ return (person[0], permissions_cache[person[0]])
+
+ def load_permissions(people):
# We now have the grantees and policies we want in the result so
# load any corresponding permissions and cache them.
- person_ids = set(row[0].id for row in rows)
- policy_ids = set(row[1].id for row in rows)
- sharing_permission_term = SQL("""
- CASE(
- MIN(COALESCE(artifact, 0)))
- WHEN 0 THEN '%s'
- ELSE '%s'
- END
- """% (SharingPermission.ALL.name, SharingPermission.SOME.name))
+ people_by_id = dict(
+ (person[0].id, person[0]) for person in people)
+ sharing_permission_term = SQL(
+ "CASE MIN(COALESCE(artifact, 0)) WHEN 0 THEN ? ELSE ? END",
+ (SharingPermission.ALL.name, SharingPermission.SOME.name))
constraints = [
- cls.grantee_id.is_in(person_ids),
- cls.policy_id.is_in(policy_ids)]
+ cls.grantee_id.is_in(people_by_id.keys()),
+ cls.policy_id.is_in(policies_by_id.keys())]
result_set = IStore(cls).find(
(cls.grantee_id, cls.policy_id, sharing_permission_term),
*constraints).group_by(cls.grantee_id, cls.policy_id)
for (person_id, policy_id, permission) in result_set:
- permissions_cache[(person_id, policy_id)] = (
- SharingPermission.items[permission])
+ person = people_by_id[person_id]
+ policy = policies_by_id[policy_id]
+ permissions_cache[person][policy] = (
+ SharingPermission.items[str(permission)])
- # The main result set has a placeholder for permission.
- constraints = [
- Person.id == cls.grantee_id,
- AccessPolicy.id == cls.policy_id,
- cls.policy_id.is_in(ids)]
+ constraints = [cls.policy_id.is_in(policies_by_id.keys())]
if grantees:
grantee_ids = [grantee.id for grantee in grantees]
constraints.append(cls.grantee_id.is_in(grantee_ids))
+ # Since the sort time dominates this query, we do the DISTINCT
+ # in a subquery to ensure it's performed first.
result_set = IStore(cls).find(
- (Person, AccessPolicy,
- SQL("'%s' as permission" % SharingPermission.NOTHING.name)),
- *constraints).config(distinct=True)
+ (Person,),
+ In(
+ Person.id,
+ Select(
+ (cls.grantee_id,), where=And(*constraints),
+ distinct=True)))
return DecoratedResultSet(
result_set,
result_decorator=set_permission, pre_iter_hook=load_permissions)
=== modified file 'lib/lp/registry/services/sharingservice.py'
--- lib/lp/registry/services/sharingservice.py 2012-03-22 09:00:36 +0000
+++ lib/lp/registry/services/sharingservice.py 2012-03-23 04:21:26 +0000
@@ -117,23 +117,18 @@
def jsonShareeData(self, grant_permissions):
"""See `ISharingService`."""
result = []
- person_by_id = {}
request = get_current_web_service_request()
browser_request = IWebBrowserOriginatingRequest(request)
- for (grantee, policy, sharing_permission) in grant_permissions:
- if not grantee.id in person_by_id:
- person_data = {
- 'name': grantee.name,
- 'meta': 'team' if grantee.is_team else 'person',
- 'display_name': grantee.displayname,
- 'self_link': absoluteURL(grantee, request),
- 'permissions': {}}
- person_data['web_link'] = absoluteURL(grantee, browser_request)
- person_by_id[grantee.id] = person_data
- result.append(person_data)
- person_data = person_by_id[grantee.id]
- person_data['permissions'][policy.type.name] = (
- sharing_permission.name)
+ for (grantee, permissions) in grant_permissions:
+ result.append({
+ 'name': grantee.name,
+ 'meta': 'team' if grantee.is_team else 'person',
+ 'display_name': grantee.displayname,
+ 'self_link': absoluteURL(grantee, request),
+ 'web_link': absoluteURL(grantee, browser_request),
+ 'permissions': dict(
+ (policy.type.name, permission.name)
+ for (policy, permission) in permissions.iteritems())})
return result
@available_with_permission('launchpad.Edit', 'pillar')
=== modified file 'lib/lp/registry/services/tests/test_sharingservice.py'
--- lib/lp/registry/services/tests/test_sharingservice.py 2012-03-22 12:11:34 +0000
+++ lib/lp/registry/services/tests/test_sharingservice.py 2012-03-23 04:21:26 +0000
@@ -125,15 +125,18 @@
def test_jsonShareeData(self):
# jsonShareeData returns the expected data.
- access_policy = self.factory.makeAccessPolicy()
+ product = self.factory.makeProduct()
+ [policy1, policy2] = getUtility(IAccessPolicySource).findByPillar(
+ [product])
grantee = self.factory.makePerson()
sharees = self.service.jsonShareeData(
- [(grantee, access_policy, SharingPermission.ALL),
- (grantee, access_policy, SharingPermission.SOME)])
+ [(grantee, {
+ policy1: SharingPermission.ALL,
+ policy2: SharingPermission.SOME})])
expected_data = self._makeShareeData(
grantee,
- [(access_policy.type, SharingPermission.ALL),
- (access_policy.type, SharingPermission.SOME)])
+ [(policy1.type, SharingPermission.ALL),
+ (policy2.type, SharingPermission.SOME)])
self.assertContentEqual([expected_data], sharees)
def _assert_getPillarShareeData(self, pillar):
@@ -256,8 +259,8 @@
sharees = self.service.getPillarSharees(pillar)
expected_sharees = [
- (grantee, access_policy, SharingPermission.ALL),
- (artifact_grant.grantee, access_policy, SharingPermission.SOME)]
+ (grantee, {access_policy: SharingPermission.ALL}),
+ (artifact_grant.grantee, {access_policy: SharingPermission.SOME})]
self.assertContentEqual(expected_sharees, sharees)
def test_getProductSharees(self):
@@ -355,10 +358,9 @@
self.assertEqual(expected_sharee_data, sharee_data)
# Check that getPillarSharees returns what we expect.
expected_sharee_grants = [
- (sharee, policy, permission)
- for policy, permission in [
- (es_policy, SharingPermission.ALL),
- (ud_policy, SharingPermission.SOME)]]
+ (sharee, {
+ es_policy: SharingPermission.ALL,
+ ud_policy: SharingPermission.SOME})]
sharee_grants = list(self.service.getPillarSharees(pillar))
self.assertContentEqual(expected_sharee_grants, sharee_grants)
@@ -470,11 +472,11 @@
access_policy for access_policy in access_policies
if access_policy.type in expected_information_types]
expected_data = [
- (grantee, policy, SharingPermission.ALL)
+ (grantee, {policy: SharingPermission.ALL})
for policy in expected_policies]
# Add the expected data for the other sharee.
another_person_data = (
- another, access_policies[0], SharingPermission.ALL)
+ another, {access_policies[0]: SharingPermission.ALL})
expected_data.append(another_person_data)
self.assertContentEqual(
expected_data, self.service.getPillarSharees(pillar))
=== modified file 'lib/lp/registry/tests/test_accesspolicy.py'
--- lib/lp/registry/tests/test_accesspolicy.py 2012-03-22 09:00:36 +0000
+++ lib/lp/registry/tests/test_accesspolicy.py 2012-03-23 04:21:26 +0000
@@ -24,6 +24,7 @@
IAccessPolicyGrantSource,
IAccessPolicySource,
)
+from lp.registry.model.person import Person
from lp.services.database.lpstorm import IStore
from lp.testing import TestCaseWithFactory
from lp.testing.layers import DatabaseFunctionalLayer
@@ -480,14 +481,18 @@
policy = self.factory.makeAccessPolicy()
policy_grant = self.factory.makeAccessPolicyGrant(policy=policy)
self.assertContentEqual(
- [(policy_grant.grantee, policy, SharingPermission.ALL)],
+ [(policy_grant.grantee, {policy: SharingPermission.ALL})],
apgfs.findGranteePermissionsByPolicy(
[policy, policy_with_no_grantees]))
# But not people with grants on artifacts.
- artifact_grant = self.factory.makeAccessArtifactGrant()
+ artifact = self.factory.makeAccessArtifact()
+ artifact_grant = self.factory.makeAccessArtifactGrant(
+ artifact=artifact, grantee=policy_grant.grantee)
+ other_artifact_grant = self.factory.makeAccessArtifactGrant(
+ artifact=artifact)
self.assertContentEqual(
- [(policy_grant.grantee, policy, SharingPermission.ALL)],
+ [(policy_grant.grantee, {policy: SharingPermission.ALL})],
apgfs.findGranteePermissionsByPolicy(
[policy, policy_with_no_grantees]))
@@ -496,11 +501,23 @@
self.factory.makeAccessPolicyArtifact(
artifact=artifact_grant.abstract_artifact, policy=another_policy)
self.assertContentEqual(
- [(policy_grant.grantee, policy, SharingPermission.ALL),
- (artifact_grant.grantee, another_policy, SharingPermission.SOME)],
+ [(policy_grant.grantee, {
+ policy: SharingPermission.ALL,
+ another_policy: SharingPermission.SOME}),
+ (other_artifact_grant.grantee, {
+ another_policy: SharingPermission.SOME})],
apgfs.findGranteePermissionsByPolicy([
policy, another_policy, policy_with_no_grantees]))
+ # Slicing works by person, not by (person, policy).
+ self.assertContentEqual(
+ [(policy_grant.grantee, {
+ policy: SharingPermission.ALL,
+ another_policy: SharingPermission.SOME})],
+ apgfs.findGranteePermissionsByPolicy([
+ policy, another_policy, policy_with_no_grantees]).order_by(
+ Person.id)[:1])
+
def test_findGranteePermissionsByPolicy_filter_grantees(self):
# findGranteePermissionsByPolicy() returns anyone with a grant for any
# of the policies or the policies' artifacts so long as the grantee is
@@ -516,7 +533,7 @@
self.factory.makeAccessPolicyGrant(
policy=policy, grantee=grantee_not_in_result)
self.assertContentEqual(
- [(policy_grant.grantee, policy, SharingPermission.ALL)],
+ [(policy_grant.grantee, {policy: SharingPermission.ALL})],
apgfs.findGranteePermissionsByPolicy(
[policy], [grantee_in_result]))
