Merge lp:~wgrant/launchpad/sharing-prettier-sql into lp:launchpad

[William Grant]

Rework findGranteePermissionsByPolicy to be fast and to batch correctly.

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).

[Ian Booth (wallyworld)]

Looks good.

review: Approve

=== 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]))