launchpad-reviewers team mailing list archive

[Ian Booth <ian.booth@xxxxxxxxxxxxx>]

Ian Booth has proposed merging lp:~wallyworld/launchpad/bugtask-index-queries-more into lp:launchpad with lp:~wallyworld/launchpad/bugtask-index-queries-726370 as a prerequisite.

Commit message:
Provide bulk checkPillarAccess interface to reduce query count on bugtask page.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #1064813 in Launchpad itself: "repeated pillar access queries on bugtask index page"
  https://bugs.launchpad.net/launchpad/+bug/1064813

For more details, see:
https://code.launchpad.net/~wallyworld/launchpad/bugtask-index-queries-more/+merge/128852

== Implementation ==

Alter the sharing service checkPillarAccess to take a collection of pillars to check so it can do a single query instead of multiple.

== Tests ==

Reduce the allowed query count in some bugtask view tests.

== Lint ==

Checking for conflicts and issues in changed files.

Linting changed files:
  lib/lp/bugs/browser/tests/test_bugtask.py
  lib/lp/bugs/model/bug.py
  lib/lp/code/model/branchnamespace.py
  lib/lp/registry/interfaces/sharingservice.py
  lib/lp/registry/services/sharingservice.py
  lib/lp/registry/services/tests/test_sharingservice.py
  lib/lp/registry/tests/test_product.py


-- 
https://code.launchpad.net/~wallyworld/launchpad/bugtask-index-queries-more/+merge/128852
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~wallyworld/launchpad/bugtask-index-queries-more into lp:launchpad.

=== modified file 'lib/lp/bugs/browser/tests/test_bugtask.py'
--- lib/lp/bugs/browser/tests/test_bugtask.py	2012-10-10 03:02:20 +0000
+++ lib/lp/bugs/browser/tests/test_bugtask.py	2012-10-10 03:02:20 +0000
@@ -156,7 +156,7 @@
     def test_rendered_query_counts_constant_with_attachments(self):
         with celebrity_logged_in('admin'):
             browses_under_limit = BrowsesWithQueryLimit(
-                97, self.factory.makePerson())
+                95, self.factory.makePerson())
 
             # First test with a single attachment.
             task = self.factory.makeBugTask()
@@ -250,7 +250,7 @@
         # Render the view with one activity.
         with celebrity_logged_in('admin'):
             browses_under_limit = BrowsesWithQueryLimit(
-                98, self.factory.makePerson())
+                93, self.factory.makePerson())
             person = self.factory.makePerson()
             add_activity("description", person)
 

=== modified file 'lib/lp/bugs/model/bug.py'
--- lib/lp/bugs/model/bug.py	2012-10-04 14:22:59 +0000
+++ lib/lp/bugs/model/bug.py	2012-10-10 03:02:20 +0000
@@ -2017,13 +2017,8 @@
         roles = IPersonRoles(user)
         if roles.in_admin or roles.in_registry_experts:
             return True
-        pillars = list(self.affected_pillars)
-        service = getUtility(IService, 'sharing')
-        for pillar in pillars:
-            if service.checkPillarAccess(
-                    pillar, InformationType.USERDATA, user):
-                return True
-        return False
+        return getUtility(IService, 'sharing').checkPillarAccess(
+            self.affected_pillars, InformationType.USERDATA, user)
 
     def linkHWSubmission(self, submission):
         """See `IBug`."""

=== modified file 'lib/lp/code/model/branchnamespace.py'
--- lib/lp/code/model/branchnamespace.py	2012-09-28 06:25:44 +0000
+++ lib/lp/code/model/branchnamespace.py	2012-10-10 03:02:20 +0000
@@ -431,10 +431,10 @@
                 self.product.branch_sharing_policy]
             if (required_grant is not None
                 and not getUtility(IService, 'sharing').checkPillarAccess(
-                    self.product, required_grant, self.owner)
+                    [self.product], required_grant, self.owner)
                 and (who is None
                     or not getUtility(IService, 'sharing').checkPillarAccess(
-                        self.product, required_grant, who))):
+                        [self.product], required_grant, who))):
                 return []
 
             return BRANCH_POLICY_ALLOWED_TYPES[

=== modified file 'lib/lp/registry/interfaces/sharingservice.py'
--- lib/lp/registry/interfaces/sharingservice.py	2012-09-28 06:15:58 +0000
+++ lib/lp/registry/interfaces/sharingservice.py	2012-10-10 03:02:20 +0000
@@ -52,11 +52,11 @@
     # version 'devel'
     export_as_webservice_entry(publish_web_link=False, as_of='beta')
 
-    def checkPillarAccess(pillar, information_type, person):
-        """Check the person's access to the given pillar and information type.
+    def checkPillarAccess(pillars, information_type, person):
+        """Check the person's access to the given pillars and information type.
 
-        :return: True if the user has access to all the pillar's information
-            of that type, False otherwise
+        :return: True if the user has sharing permission 'ALL' on any of the
+            specified pillars.
         """
 
     def getAccessPolicyGrantCounts(pillar):

=== modified file 'lib/lp/registry/services/sharingservice.py'
--- lib/lp/registry/services/sharingservice.py	2012-09-28 06:15:58 +0000
+++ lib/lp/registry/services/sharingservice.py	2012-10-10 03:02:20 +0000
@@ -94,11 +94,12 @@
         """See `IService`."""
         return 'sharing'
 
-    def checkPillarAccess(self, pillar, information_type, person):
+    def checkPillarAccess(self, pillars, information_type, person):
         """See `ISharingService`."""
-        policy = getUtility(IAccessPolicySource).find(
-            [(pillar, information_type)]).one()
-        if policy is None:
+        policies = getUtility(IAccessPolicySource).find(
+            [(pillar, information_type) for pillar in pillars])
+        policy_ids = [policy.id for policy in policies]
+        if not policy_ids:
             return False
         store = IStore(AccessPolicyGrant)
         tables = [
@@ -109,7 +110,7 @@
             ]
         result = store.using(*tables).find(
             AccessPolicyGrant,
-            AccessPolicyGrant.policy_id == policy.id,
+            AccessPolicyGrant.policy_id.is_in(policy_ids),
             TeamParticipation.personID == person.id)
         return not result.is_empty()
 

=== modified file 'lib/lp/registry/services/tests/test_sharingservice.py'
--- lib/lp/registry/services/tests/test_sharingservice.py	2012-10-08 10:07:11 +0000
+++ lib/lp/registry/services/tests/test_sharingservice.py	2012-10-10 03:02:20 +0000
@@ -1722,18 +1722,18 @@
         self.assertEqual(
             False,
             self.service.checkPillarAccess(
-                product, InformationType.USERDATA, wrong_person))
+                [product], InformationType.USERDATA, wrong_person))
         self.assertEqual(
             True,
             self.service.checkPillarAccess(
-                product, InformationType.USERDATA, right_person))
+                [product], InformationType.USERDATA, right_person))
 
     def test_checkPillarAccess_no_policy(self):
         # checkPillarAccess returns False if there's no policy.
         self.assertEqual(
             False,
             self.service.checkPillarAccess(
-                self.factory.makeProduct(), InformationType.PUBLIC,
+                [self.factory.makeProduct()], InformationType.PUBLIC,
                 self.factory.makePerson()))
 
     def test_getAccessPolicyGrantCounts(self):

=== modified file 'lib/lp/registry/tests/test_product.py'
--- lib/lp/registry/tests/test_product.py	2012-10-08 10:07:11 +0000
+++ lib/lp/registry/tests/test_product.py	2012-10-10 03:02:20 +0000
@@ -1089,7 +1089,8 @@
              getUtility(IAccessPolicySource).findByPillar([self.product])])
         self.assertTrue(
             getUtility(IService, 'sharing').checkPillarAccess(
-                self.product, InformationType.PROPRIETARY, self.product.owner))
+                [self.product], InformationType.PROPRIETARY,
+                self.product.owner))
 
     def test_unused_policies_are_pruned(self):
         # When a sharing policy is changed, the allowed information types may
@@ -1193,10 +1194,12 @@
              getUtility(IAccessPolicySource).findByPillar([self.product])])
         self.assertTrue(
             getUtility(IService, 'sharing').checkPillarAccess(
-                self.product, InformationType.PROPRIETARY, self.product.owner))
+                [self.product], InformationType.PROPRIETARY,
+                self.product.owner))
         self.assertTrue(
             getUtility(IService, 'sharing').checkPillarAccess(
-                self.product, InformationType.EMBARGOED, self.product.owner))
+                [self.product], InformationType.EMBARGOED,
+                self.product.owner))
 
 
 class ProductSnapshotTestCase(TestCaseWithFactory):