launchpad-reviewers team mailing list archive

[Colin Watson <mp+413581@xxxxxxxxxxxxxxxxxx>]

Diff comments:

> 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

No - for purposes of returning early, we're only interested in positive entries in the cache.

> +
> +        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

That isn't quite right.  The purpose of the condition at line 97 is to check whether we already know that the user is in at least one of the requested teams, in which case we indeed don't need to do any further work and can return early.  However, it's entirely possible that `_inTeam_cache` has cached some *negative* results.  For example, let's imagine that you query whether I'm in any of (~admins, ~canonical, ~launchpad).  We might know from a previous `inTeam` or `inAnyTeam` call that I'm not in ~admins: in that case `_inTeam_cache[<id of ~admins>]` will be False.  Since we have no positive cache entries, we can't return early.  But there's no point in querying for my membership in ~admins again, since we already know the answer.  In this case, the `TeamParticipation` query should be limited to just ~canonical and ~launchpad.

> +
> +        return in_any_team
> +
> +
>      def hasParticipationEntryFor(self, team):
>          """See `IPerson`."""
>          return bool(TeamParticipation.selectOneBy(person=self, team=team))


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