launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

Colin Watson has proposed merging ~cjwatson/launchpad:faster-teams-vocab into launchpad:master.

Commit message:
Optimise UserTeamsParticipationVocabulary

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~cjwatson/launchpad/+git/launchpad/+merge/389909

UserTeamsParticipationVocabulary (or, more specifically, AllUserTeamsParticipationPlusSelfVocabulary) is very slow for people who are members of many private teams, doing one TeamParticipation query for each such team.

Optimise this in two ways: firstly, we can fetch fewer teams from the database to begin with; secondly, we can precache the permission check, since a user can always view teams they belong to.
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of ~cjwatson/launchpad:faster-teams-vocab into launchpad:master.

diff --git a/lib/lp/registry/doc/vocabularies.txt b/lib/lp/registry/doc/vocabularies.txt
index 307eaa1..7e4d4d1 100644
--- a/lib/lp/registry/doc/vocabularies.txt
+++ b/lib/lp/registry/doc/vocabularies.txt
@@ -327,33 +327,6 @@ membership is public.
     LookupError:...
 
 
-PersonTeamParticipations
-........................
-
-This vocabulary contains all the teams a person participates in. Either
-through direct or indirect participations.
-
-    >>> sample_person = person_set.getByName('name12')
-    >>> [membership.team.name
-    ...  for membership in sample_person.team_memberships]
-    [u'hwdb-team', u'landscape-developers', u'launchpad-users', u'name20']
-
-    >>> [team.name for team in sample_person.teams_participated_in]
-    [u'hwdb-team', u'landscape-developers', u'launchpad-users', u'name18',
-     u'name20']
-
-    >>> sample_person_teams_vocabulary = get_naked_vocab(
-    ...     sample_person, 'PersonTeamParticipations')
-
-    >>> for term in sample_person_teams_vocabulary:
-    ...     print "%s: %s (%s)" % (term.token, term.title, term.value.name)
-    hwdb-team: HWDB Team (hwdb-team)
-    landscape-developers: Landscape Developers (landscape-developers)
-    launchpad-users: Launchpad Users (launchpad-users)
-    name18: Ubuntu Gnome Team (name18)
-    name20: Warty Security Team (name20)
-
-
 Milestone
 .........
 
diff --git a/lib/lp/registry/tests/test_user_vocabularies.py b/lib/lp/registry/tests/test_user_vocabularies.py
index a35ff21..77a4a25 100644
--- a/lib/lp/registry/tests/test_user_vocabularies.py
+++ b/lib/lp/registry/tests/test_user_vocabularies.py
@@ -1,4 +1,4 @@
-# Copyright 2009 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2020 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Test the user vocabularies."""
@@ -19,9 +19,11 @@ from lp.testing import (
     ANONYMOUS,
     login,
     login_person,
+    record_two_runs,
     TestCaseWithFactory,
     )
 from lp.testing.layers import DatabaseFunctionalLayer
+from lp.testing.matchers import HasQueryCount
 
 
 class TestUserTeamsParticipationPlusSelfVocabulary(TestCaseWithFactory):
@@ -67,6 +69,26 @@ class TestUserTeamsParticipationPlusSelfVocabulary(TestCaseWithFactory):
         login_person(user)
         self.assertEqual([user], self._vocabTermValues())
 
+    def test_user_no_private_teams_query_count(self):
+        # Checking membership of private teams doesn't inflate the
+        # vocabulary's query count.
+        user = self.factory.makePerson()
+        team_owner = self.factory.makePerson()
+
+        def make_private_team():
+            team = self.factory.makeTeam(
+                owner=team_owner, visibility=PersonVisibility.PRIVATE)
+            team.addMember(person=user, reviewer=team_owner)
+
+        def expand_vocabulary():
+            login_person(user)
+            self.assertEqual([user], self._vocabTermValues())
+
+        recorder1, recorder2 = record_two_runs(
+            expand_vocabulary, make_private_team, 5,
+            login_method=lambda: login_person(team_owner))
+        self.assertThat(recorder2, HasQueryCount.byEquality(recorder1))
+
     def test_indirect_team_membership(self):
         # Indirect team membership is shown.
         user = self.factory.makePerson()
@@ -101,6 +123,28 @@ class TestAllUserTeamsParticipationPlusSelfVocabulary(TestCaseWithFactory):
         login_person(team_owner)
         self.assertEqual([team_owner, team], self._vocabTermValues())
 
+    def test_user_no_private_teams_query_count(self):
+        # Checking membership of private teams doesn't inflate the
+        # vocabulary's query count.
+        user = self.factory.makePerson()
+        team_owner = self.factory.makePerson()
+        teams = []
+
+        def make_private_team():
+            team = self.factory.makeTeam(
+                owner=team_owner, visibility=PersonVisibility.PRIVATE)
+            team.addMember(person=user, reviewer=team_owner)
+            teams.append(team)
+
+        def expand_vocabulary():
+            login_person(user)
+            self.assertContentEqual([user] + teams, self._vocabTermValues())
+
+        recorder1, recorder2 = record_two_runs(
+            expand_vocabulary, make_private_team, 5,
+            login_method=lambda: login_person(team_owner))
+        self.assertThat(recorder2, HasQueryCount.byEquality(recorder1))
+
     def test_only_exclusive_teams_for_series_branches(self):
         # For series branches, only exclusive teams are permitted in the vocab.
         branch = self.factory.makeBranch()
diff --git a/lib/lp/registry/vocabularies.py b/lib/lp/registry/vocabularies.py
index d7ef98d..78780a4 100644
--- a/lib/lp/registry/vocabularies.py
+++ b/lib/lp/registry/vocabularies.py
@@ -1,4 +1,4 @@
-# Copyright 2009-2016 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2020 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Vocabularies for content objects.
@@ -44,7 +44,6 @@ __all__ = [
     'MilestoneWithDateExpectedVocabulary',
     'NewPillarGranteeVocabulary',
     'NonMergedPeopleAndTeamsVocabulary',
-    'person_team_participations_vocabulary_factory',
     'PersonAccountToMergeVocabulary',
     'PersonActiveMembershipVocabulary',
     'ProductReleaseVocabulary',
@@ -199,7 +198,10 @@ from lp.services.propertycache import (
     cachedproperty,
     get_property_cache,
     )
-from lp.services.webapp.authorization import check_permission
+from lp.services.webapp.authorization import (
+    check_permission,
+    precache_permission_for_objects,
+    )
 from lp.services.webapp.interfaces import ILaunchBag
 from lp.services.webapp.publisher import nearest
 from lp.services.webapp.vocabulary import (
@@ -401,25 +403,38 @@ class UserTeamsParticipationVocabulary(SQLObjectVocabularyBase):
         return SimpleTerm(obj, obj.name, obj.unique_displayname)
 
     def __iter__(self):
-        kw = {}
-        if self._orderBy:
-            kw['orderBy'] = self._orderBy
         launchbag = getUtility(ILaunchBag)
         if launchbag.user:
             user = launchbag.user
-            for team in user.teams_participated_in:
-                if ((team.visibility == PersonVisibility.PUBLIC
-                    or self.INCLUDE_PRIVATE_TEAM) and
-                    (team.membership_policy in EXCLUSIVE_TEAM_POLICY
-                    or not self.EXCLUSIVE_TEAMS_ONLY)):
-                    yield self.toTerm(team)
+            clauses = [
+                Person.id == TeamParticipation.teamID,
+                TeamParticipation.person == user,
+                Person.teamowner != None,
+                ]
+            if not self.INCLUDE_PRIVATE_TEAM:
+                clauses.append(Person.visibility == PersonVisibility.PUBLIC)
+            if self.EXCLUSIVE_TEAMS_ONLY:
+                clauses.append(
+                    Person.membership_policy.is_in(EXCLUSIVE_TEAM_POLICY))
+            teams = list(
+                IStore(Person).find(Person, *clauses).order_by(
+                    Person._storm_sortingColumns))
+            # Users can view all the teams they belong to.
+            precache_permission_for_objects(
+                None, 'launchpad.LimitedView', teams)
+            for team in teams:
+                yield self.toTerm(team)
 
     def getTermByToken(self, token):
         """See `IVocabularyTokenized`."""
         launchbag = getUtility(ILaunchBag)
         if launchbag.user:
             user = launchbag.user
-            for team in user.teams_participated_in:
+            teams = list(user.teams_participated_in)
+            # Users can view all the teams they belong to.
+            precache_permission_for_objects(
+                None, 'launchpad.LimitedView', teams)
+            for team in teams:
                 if team.name == token:
                     return self.getTerm(team)
         raise LookupError(token)
@@ -1056,21 +1071,6 @@ class ActiveMailingListVocabulary(FilteredVocabularyBase):
         return CountableIterator(results.count(), results, self.toTerm)
 
 
-def person_term(person):
-    """Return a SimpleTerm for the `Person`."""
-    return SimpleTerm(person, person.name, title=person.displayname)
-
-
-def person_team_participations_vocabulary_factory(context):
-    """Return a SimpleVocabulary containing the teams a person
-    participate in.
-    """
-    assert context is not None
-    person = IPerson(context)
-    return SimpleVocabulary([
-        person_term(team) for team in person.teams_participated_in])
-
-
 class UserTeamsParticipationPlusSelfVocabulary(
     UserTeamsParticipationVocabulary):
     """A vocabulary containing the public teams that the logged
diff --git a/lib/lp/registry/vocabularies.zcml b/lib/lp/registry/vocabularies.zcml
index 057cad4..4e2e488 100644
--- a/lib/lp/registry/vocabularies.zcml
+++ b/lib/lp/registry/vocabularies.zcml
@@ -184,15 +184,6 @@
 
 
   <securedutility
-    name="PersonTeamParticipations"
-    component="lp.registry.vocabularies.person_team_participations_vocabulary_factory"
-    provides="zope.schema.interfaces.IVocabularyFactory"
-    >
-    <allow interface="zope.schema.interfaces.IVocabularyFactory"/>
-  </securedutility>
-
-
-  <securedutility
     name="Product"
     component="lp.registry.vocabularies.ProductVocabulary"
     provides="zope.schema.interfaces.IVocabularyFactory"