launchpad-reviewers team mailing list archive

[William Grant <me@xxxxxxxxxxxxxxxxxx>]

William Grant has proposed merging lp:~wgrant/launchpad/expose-constant-queries into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~wgrant/launchpad/expose-constant-queries/+merge/98083

This branch fixes expose_user_administered_teams_to_js to use a constant number of queries, and hopefully not take 250ms. It previously failed to preload the preferredemails and permissions of the teams it considered, causing lots of late evaluation for people owning or admining many teams.

I had to change precache_permission_for_objects to optionally use the default interaction and participation, since the FakeRequest used by test_expose isn't itself a participation. I'm not sure why this isn't the default behaviour, seeing as all the other permission operations use the default participation.
-- 
https://code.launchpad.net/~wgrant/launchpad/expose-constant-queries/+merge/98083
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~wgrant/launchpad/expose-constant-queries into lp:launchpad.

=== modified file 'lib/lp/bugs/browser/structuralsubscription.py'
--- lib/lp/bugs/browser/structuralsubscription.py	2012-02-17 23:00:56 +0000
+++ lib/lp/bugs/browser/structuralsubscription.py	2012-03-18 01:36:21 +0000
@@ -57,9 +57,16 @@
     IDistributionSourcePackage,
     )
 from lp.registry.interfaces.milestone import IProjectGroupMilestone
-from lp.registry.interfaces.person import IPersonSet
+from lp.registry.interfaces.person import (
+    IPerson,
+    IPersonSet,
+    )
 from lp.services.propertycache import cachedproperty
-from lp.services.webapp.authorization import check_permission
+from lp.services.webapp.authorization import (
+    check_permission,
+    precache_permission_for_objects,
+    )
+from lp.services.webapp.interaction import get_current_principal
 from lp.services.webapp.interfaces import NoCanonicalUrl
 from lp.services.webapp.menu import (
     enabled_with_permission,
@@ -459,6 +466,19 @@
             # filters can only be edited by the subscriber.
             # This can happen if the user is an owner but not a member.
             administers_and_in = membership.intersection(administrated_teams)
+            list(getUtility(IPersonSet).getPrecachedPersonsFromIDs(
+                [team.id for team in administers_and_in],
+                need_preferred_email=True))
+
+            # If the requester is the user, they're at least an admin in
+            # all of these teams. Precache launchpad.(Limited)View so we
+            # can see the necessary attributes.
+            current_user = IPerson(get_current_principal(), None)
+            if current_user is not None and user == current_user:
+                for perm in ('launchpad.View', 'launchpad.LimitedView'):
+                    precache_permission_for_objects(
+                        None, perm, administers_and_in)
+
             for team in administers_and_in:
                 if (bug_supervisor is not None and
                     not team.inTeam(bug_supervisor)):

=== modified file 'lib/lp/bugs/browser/tests/test_expose.py'
--- lib/lp/bugs/browser/tests/test_expose.py	2012-02-22 23:03:55 +0000
+++ lib/lp/bugs/browser/tests/test_expose.py	2012-03-18 01:36:21 +0000
@@ -25,18 +25,26 @@
     expose_user_administered_teams_to_js,
     expose_user_subscriptions_to_js,
     )
+from lp.registry.interfaces.person import PersonVisibility
 from lp.registry.interfaces.teammembership import TeamMembershipStatus
+from lp.registry.model.person import Person
+from lp.services.database.lpstorm import IStore
 from lp.services.identity.interfaces.emailaddress import EmailAddressStatus
+from lp.services.webapp.authorization import clear_cache
 from lp.services.webapp.publisher import canonical_url
 from lp.services.webapp.servers import LaunchpadTestRequest
 from lp.testing import (
+    login_person,
     person_logged_in,
     StormStatementRecorder,
     TestCase,
     TestCaseWithFactory,
     )
 from lp.testing.layers import DatabaseFunctionalLayer
-from lp.testing.matchers import Contains
+from lp.testing.matchers import (
+    Contains,
+    HasQueryCount,
+    )
 
 
 class FakeRequest:
@@ -176,6 +184,40 @@
         self.assertThat(team_info[0]['link'],
             Equals(u'http://example.com/\u201cBugSupervisorSubTeam\u201dteam'))
 
+    def test_query_count(self):
+        # The function issues a constant number of queries regardless of
+        # team count.
+        login_person(self.user)
+        context = self.factory.makeProduct(owner=self.user)
+        self._setup_teams(self.user)
+
+        IStore(Person).invalidate()
+        clear_cache()
+        with StormStatementRecorder() as recorder:
+            expose_user_administered_teams_to_js(
+                self.request, self.user, context,
+                absoluteURL=fake_absoluteURL)
+        self.assertThat(recorder, HasQueryCount(Equals(4)))
+
+        # Create some new public teams owned by the user, and a private
+        # team administered by the user.
+        for i in range(3):
+            self.factory.makeTeam(owner=self.user)
+        pt = self.factory.makeTeam(
+            visibility=PersonVisibility.PRIVATE, members=[self.user])
+        with person_logged_in(pt.teamowner):
+            pt.addMember(
+                self.user, pt.teamowner, status=TeamMembershipStatus.ADMIN)
+
+        IStore(Person).invalidate()
+        clear_cache()
+        del IJSONRequestCache(self.request).objects['administratedTeams']
+        with StormStatementRecorder() as recorder:
+            expose_user_administered_teams_to_js(
+                self.request, self.user, context,
+                absoluteURL=fake_absoluteURL)
+        self.assertThat(recorder, HasQueryCount(Equals(4)))
+
     def test_expose_user_administered_teams_to_js__uses_cached_teams(self):
         # The function expose_user_administered_teams_to_js uses a
         # cached list of administrated teams.

=== modified file 'lib/lp/services/webapp/authorization.py'
--- lib/lp/services/webapp/authorization.py	2012-03-02 07:19:51 +0000
+++ lib/lp/services/webapp/authorization.py	2012-03-18 01:36:21 +0000
@@ -313,6 +313,8 @@
 
 def precache_permission_for_objects(participation, permission_name, objects):
     """Precaches the permission for the objects into the policy cache."""
+    if participation is None:
+        participation = getInteraction().participations[0]
     permission_cache = participation.annotations.setdefault(
         LAUNCHPAD_SECURITY_POLICY_CACHE_KEY,
         weakref.WeakKeyDictionary())

=== modified file 'lib/lp/services/webapp/tests/test_authorization.py'
--- lib/lp/services/webapp/tests/test_authorization.py	2012-03-02 07:19:51 +0000
+++ lib/lp/services/webapp/tests/test_authorization.py	2012-03-18 01:36:21 +0000
@@ -461,6 +461,16 @@
         self.assertTrue(check_permission('launchpad.View', objects[0]))
         self.assertTrue(check_permission('launchpad.View', objects[1]))
 
+    def test_default_request(self):
+        # If no request is provided, the current interaction is used.
+        class Boring(object):
+            """A boring, but weakref-able object."""
+        obj = Boring()
+        request = LaunchpadTestRequest()
+        login(ANONYMOUS, request)
+        precache_permission_for_objects(None, 'launchpad.View', [obj])
+        self.assertTrue(check_permission('launchpad.View', obj))
+
 
 class TestIterAuthorization(TestCase):
     """Tests for `iter_authorization`.