launchpad-reviewers team mailing list archive

[Guruprasad <mp+413581@xxxxxxxxxxxxxxxxxx>]

Guruprasad has proposed merging ~lgp171188/launchpad:optimize-has-any-bug-role-bulk-team-membership-checks into launchpad:master.

Commit message:
Optimize _has_any_bug_role to do bulk team membership checks

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~lgp171188/launchpad/+git/launchpad/+merge/413581
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of ~lgp171188/launchpad:optimize-has-any-bug-role-bulk-team-membership-checks into launchpad:master.

diff --git a/lib/lp/bugs/security.py b/lib/lp/bugs/security.py
index ace8336..a21f10a 100644
--- a/lib/lp/bugs/security.py
+++ b/lib/lp/bugs/security.py
@@ -123,11 +123,8 @@ class AppendBug(AuthorizationBase):
 
 def _has_any_bug_role(user, tasks):
     """Return True if the user has any role on any of these bug tasks."""
-    # XXX cjwatson 2019-03-26: This is inefficient for bugs with many
-    # targets.  However, we only get here if we can't easily establish that
-    # the user seems legitimate, so it shouldn't be a big problem in
-    # practice.  We can optimise this further if it turns out to matter.
     targets = [task.pillar for task in tasks]
+    bug_target_roles = {}
     for target in targets:
         roles = []
         if IHasOwner.providedBy(target):
@@ -136,8 +133,10 @@ def _has_any_bug_role(user, tasks):
             roles.append('driver')
         if IHasBugSupervisor.providedBy(target):
             roles.append('bug_supervisor')
-        if user.isOneOf(target, roles):
-            return True
+        bug_target_roles[target] = roles
+
+    if user.hasAnyRole(bug_target_roles):
+        return True
     return False
 
 
diff --git a/lib/lp/registry/interfaces/person.py b/lib/lp/registry/interfaces/person.py
index 540e777..02b9a99 100644
--- a/lib/lp/registry/interfaces/person.py
+++ b/lib/lp/registry/interfaces/person.py
@@ -1247,6 +1247,12 @@ class IPersonViewRestricted(IHasBranches, IHasSpecifications,
             `False` is returned.
         """
 
+    # XXX: lgp171188, 2022-01-04: Unexported for the same reasons
+    # as the inTeam() method.
+    def inAnyTeam(teams):
+        """Is this person a member of any of the given `teams`?"""
+
+
     def clearInTeamCache():
         """Clears the person's inTeam cache.
 
@@ -2417,6 +2423,9 @@ class IPersonSetPublic(Interface):
         Return None if there is no person with the given name.
         """
 
+    def getByNames(self, names):
+        """Return the persons with the given names."""
+
     def getByAccount(account):
         """Return the `IPerson` with the given account, or None."""
 
diff --git a/lib/lp/registry/interfaces/role.py b/lib/lp/registry/interfaces/role.py
index caa0e1b..f78276b 100644
--- a/lib/lp/registry/interfaces/role.py
+++ b/lib/lp/registry/interfaces/role.py
@@ -146,3 +146,5 @@ class IPersonRoles(Interface):
         :param obj: The object to check the relation to.
         :param attributes: A list of attribute names to check with inTeam.
         """
+    def hasAnyRole(target_roles):
+        """Does this person have one of the roles for the given targets?"""
diff --git a/lib/lp/registry/model/person.py b/lib/lp/registry/model/person.py
index 1a0c380..1902f8a 100644
--- a/lib/lp/registry/model/person.py
+++ b/lib/lp/registry/model/person.py
@@ -1342,6 +1342,48 @@ class Person(
         self._inTeam_cache[team.id] = in_team
         return in_team
 
+    def inAnyTeam(self, teams):
+        """See `IPerson`."""
+        if not teams:
+            return False
+
+        # Translate each team name to an ITeam if we were passed
+        # a list of teams.
+        if all(isinstance(team, str) for team in teams):
+            teams = PersonSet.getByNames(teams)
+            if not teams:
+                return False
+
+        team_ids = {team.id for team in teams if team.is_team}
+        if self.id in team_ids:
+            # A team is always a member of itself.
+            return True
+
+        if self._inTeam_cache is None:
+            # Initialize cache
+            self._inTeam_cache = {}
+        else:
+            if any(self._inTeam_cache.get(team_id) for team_id in team_ids):
+                return True
+
+        team_participations = IStore(TeamParticipation).find(
+            TeamParticipation,
+            And(
+                TeamParticipation.teamID.is_in(team_ids),
+                TeamParticipation.personID == self.id
+            )
+        )
+
+        in_any_team = False
+
+        for team_participation in team_participations:
+            in_any_team = True
+            if team_participation.team.id not in self._inTeam_cache:
+                self._inTeam_cache[team_participation.team.id] = True
+
+        return in_any_team
+
+
     def hasParticipationEntryFor(self, team):
         """See `IPerson`."""
         return bool(TeamParticipation.selectOneBy(person=self, team=team))
@@ -3638,6 +3680,9 @@ class PersonSet:
             query = And(query, Person.mergedID == None)
         return Person.selectOne(query)
 
+    def getByNames(self, names):
+        return IStore(self).find(Person, Person.name.is_in(names))
+
     def getByAccount(self, account):
         """See `IPersonSet`."""
         return Person.selectOne(Person.q.accountID == account.id)
diff --git a/lib/lp/registry/model/personroles.py b/lib/lp/registry/model/personroles.py
index f8bf273..bfb631f 100644
--- a/lib/lp/registry/model/personroles.py
+++ b/lib/lp/registry/model/personroles.py
@@ -29,6 +29,7 @@ class PersonRoles:
         self._celebrities = getUtility(ILaunchpadCelebrities)
         # Use an unproxied inTeam() method for security checks.
         self.inTeam = removeSecurityProxy(self.person).inTeam
+        self.inAnyTeam = removeSecurityProxy(self.person).inAnyTeam
 
     def __getattr__(self, name):
         """Handle all in_* attributes."""
@@ -71,3 +72,15 @@ class PersonRoles:
             if self.inTeam(role):
                 return True
         return False
+
+    def hasAnyRole(self, bug_target_roles):
+        """See IPersonRoles."""
+        teams = []
+        for bug_target, roles in bug_target_roles.items():
+            for role_ in roles:
+                role = getattr(bug_target, role_)
+                if role:
+                    teams.append(role)
+        if self.inAnyTeam(teams):
+            return True
+        return False