Optimize peer-review team membership lookups

scripts/libs/GitHubAPIClient.ts

@@ -8,6 +8,22 @@ type InternalOctokit = InstanceType<typeof OctokitWithPlugins>;
 
 const OctokitWithPlugins = Octokit.plugin(throttling, paginateRest);
 
+type GraphQLVariable<Name extends string = string> = {
+    name: Name;
+    value: string;
+    declaration: `$${Name}: String!`;
+    reference: `$${Name}`;
+};
+
+function createGraphQLVariable<Name extends string>(name: Name, value: string): GraphQLVariable<Name> {
+    return {
+        name,
+        value,
+        declaration: `$${name}: String!`,
+        reference: `$${name}`,
+    };
+}
+
 /**
  * This GitHub API client:
  *   - Exposes utils for octokit (rest), graphql, and pagination.
@@ -67,4 +83,5 @@ class GitHubAPIClient {
 }
 
 export default GitHubAPIClient;
-export type {InternalOctokit};
+export {createGraphQLVariable};
+export type {GraphQLVariable, InternalOctokit};

scripts/libs/GitHubUtils/getTeamMemberLogins.ts

@@ -1,89 +1,116 @@
 import type GitHubAPIClient from '../GitHubAPIClient';
+import {createGraphQLVariable} from '../GitHubAPIClient';
+import type {GraphQLVariable} from '../GitHubAPIClient';
 
 const EXPENSIFY_ORG = 'Expensify';
 
-type TeamMembersResponse = {
-    organization: {
-        team: {
-            members: {
-                pageInfo: {
-                    hasNextPage: boolean;
-                    endCursor: string | null;
-                };
-                nodes: Array<{
-                    login: string;
-                }>;
-            };
-        } | null;
-    } | null;
+type TeamMemberSearch = {
+    login: string;
+    alias: string;
+    variable: GraphQLVariable;
+};
+
+type TeamSearch = {
+    slug: string;
+    alias: string;
+    variable: GraphQLVariable;
 };
 
-const teamMemberLoginsPromisesByClient = new WeakMap<GitHubAPIClient, Map<string, Promise<Set<string>>>>();
+type TeamMemberSearchResult = {
+    nodes: Array<{login: string}>;
+};
+
+type TeamMembersResponse = {
+    organization: Record<string, Record<string, TeamMemberSearchResult | null> | null> | null;
+};
 
 /**
- * Fetches and memoizes all members of a GitHub team for the lifetime of a client.
+ * Returns the candidate logins that belong to any of the teams, batching all searches into one request.
  */
-async function getTeamMemberLogins(client: GitHubAPIClient, teamSlug: string): Promise<Set<string>> {
-    let teamPromises = teamMemberLoginsPromisesByClient.get(client);
-    if (!teamPromises) {
-        teamPromises = new Map();
-        teamMemberLoginsPromisesByClient.set(client, teamPromises);
-    }
+async function getTeamMemberLogins(client: GitHubAPIClient, teamSlugs: string[], candidateLogins: string[]): Promise<Set<string>> {
+    const teamMemberLogins = new Set<string>();
+    const uniqueTeamSlugs = new Set(teamSlugs);
+    const uniqueCandidateLogins = new Set(candidateLogins);
 
-    let teamMembersPromise = teamPromises.get(teamSlug);
-    if (!teamMembersPromise) {
-        teamMembersPromise = fetchTeamMemberLogins(client, teamSlug);
-        teamPromises.set(teamSlug, teamMembersPromise);
+    if (uniqueTeamSlugs.size === 0 || uniqueCandidateLogins.size === 0) {
+        return teamMemberLogins;
     }
 
-    return teamMembersPromise;
-}
+    const organizationVariable = createGraphQLVariable('organization', EXPENSIFY_ORG);
+    const memberSearches: TeamMemberSearch[] = [...uniqueCandidateLogins].map((login, index) => {
+        const alias = `member${index}`;
+        return {
+            login,
+            alias,
+            variable: createGraphQLVariable(alias, login),
+        };
+    });
+    const teamSearches: TeamSearch[] = [...uniqueTeamSlugs].map((slug, index) => {
+        const alias = `team${index}`;
+        return {
+            slug,
+            alias,
+            variable: createGraphQLVariable(`teamSlug${index}`, slug),
+        };
+    });
 
-async function fetchTeamMemberLogins(client: GitHubAPIClient, teamSlug: string): Promise<Set<string>> {
-    const teamMemberLogins = new Set<string>();
-    let cursor: string | null = null;
-    let hasNextPage = true;
-
-    while (hasNextPage) {
-        // await-in-loop is necessary and appropriate for polling a paginated endpoint;
-        // each request is dependent upon the response of the previous.
-        // eslint-disable-next-line no-await-in-loop
-        const response: TeamMembersResponse = await client.graphql<TeamMembersResponse>(
-            `
-            query TeamMembers($organization: String!, $teamSlug: String!, $cursor: String) {
-                organization(login: $organization) {
-                    team(slug: $teamSlug) {
-                        members(first: 100, after: $cursor) {
-                            pageInfo {
-                                hasNextPage
-                                endCursor
-                            }
-                            nodes {
-                                login
-                            }
-                        }
+    const queryVariables: GraphQLVariable[] = [organizationVariable, ...memberSearches.map(({variable}) => variable), ...teamSearches.map(({variable}) => variable)];
+    const teamQueries = teamSearches.map((teamSearch) => {
+        const memberQueries = memberSearches.map(
+            (memberSearch) => `
+                ${memberSearch.alias}: members(first: 1, query: ${memberSearch.variable.reference}) {
+                    nodes {
+                        login
                     }
                 }
-            }
-        `,
-            {
-                organization: EXPENSIFY_ORG,
-                teamSlug,
-                cursor,
-            },
+            `,
         );
 
-        const members = response.organization?.team?.members;
-        if (!members) {
-            throw new Error(`${EXPENSIFY_ORG}/${teamSlug} team could not be found.`);
+        return `
+            ${teamSearch.alias}: team(slug: ${teamSearch.variable.reference}) {
+                ${memberQueries.join('\n')}
+            }
+        `;
+    });
+
+    const variables: Record<string, string> = {};
+    const variableDeclarations: Array<GraphQLVariable['declaration']> = [];
+    for (const variable of queryVariables) {
+        variables[variable.name] = variable.value;
+        variableDeclarations.push(variable.declaration);
+    }
+
+    const query = `
+        query TeamMembers(${variableDeclarations.join(', ')}) {
+            organization(login: ${organizationVariable.reference}) {
+                ${teamQueries.join('\n')}
+            }
         }
+    `;
 
-        for (const member of members.nodes) {
-            teamMemberLogins.add(member.login);
+    const response = await client.graphql<TeamMembersResponse>(query, variables);
+    const organization = response.organization;
+    if (!organization) {
+        throw new Error(`${EXPENSIFY_ORG} organization could not be found.`);
+    }
+
+    for (const teamSearch of teamSearches) {
+        const team = organization[teamSearch.alias];
+        if (!team) {
+            throw new Error(`${EXPENSIFY_ORG}/${teamSearch.slug} team could not be found.`);
         }
 
-        hasNextPage = members.pageInfo.hasNextPage;
-        cursor = members.pageInfo.endCursor;
+        for (const memberSearch of memberSearches) {
+            const members = team[memberSearch.alias];
+            if (!members) {
+                throw new Error(`Missing team membership search result for ${memberSearch.login} in ${teamSearch.slug}.`);
+            }
+
+            // GitHub's query can be fuzzy, so only an exact login match proves membership.
+            if (members.nodes.some((member) => member.login === memberSearch.login)) {
+                teamMemberLogins.add(memberSearch.login);
+            }
+        }
     }
 
     return teamMemberLogins;

scripts/libs/GitHubUtils/index.ts

@@ -20,7 +20,7 @@ function createGitHubUtils(client: GitHubAPIClient) {
         getPullRequestCommitCount: (args: {owner: string; repo: string; number: number}) => getPullRequestCommitCount(client, args),
         getRequiredApprovingReviewCount: (args: {owner: string; repo: string; baseRef: string}) => getRequiredApprovingReviewCount(client, args),
         isBotUser,
-        getTeamMemberLogins: (teamSlug: string) => getTeamMemberLogins(client, teamSlug),
+        getTeamMemberLogins: (teamSlugs: string[], candidateLogins: string[]) => getTeamMemberLogins(client, teamSlugs, candidateLogins),
         listPullRequestCommits: (args: {owner: string; repo: string; number: number}) => listPullRequestCommits(client, args),
     };
 }

scripts/verifyPeerReview.ts

@@ -91,8 +91,7 @@ async function getIndependentApprovers(gitHubUtils: GitHubUtils, approvers: stri
     const authorsSet = new Set(authors);
     const independentApprovers = approvers.filter((approver) => !authorsSet.has(approver));
     const reviewerTeams = [EXPENSIFY_EMPLOYEE_TEAM_SLUG, ...(REPOSITORY_REVIEWER_TEAMS.get(`${owner}/${repo}`) ?? [])];
-    const teamMembers = await Promise.all(reviewerTeams.map((teamSlug) => gitHubUtils.getTeamMemberLogins(teamSlug)));
-    const eligibleReviewers = new Set(teamMembers.flatMap((members) => [...members]));
+    const eligibleReviewers = await gitHubUtils.getTeamMemberLogins(reviewerTeams, independentApprovers);
 
     return independentApprovers.filter((approver) => eligibleReviewers.has(approver));
 }

tests/GitHubUtils.test.ts

@@ -392,21 +392,82 @@ describe('GitHubUtils', () => {
     });
 
     describe('getTeamMemberLogins', () => {
-        it('returns the fetched team login set', async () => {
+        it('batches unique candidate searches across teams and rejects fuzzy and differently cased matches', async () => {
+            let calls = 0;
+            const gitHubUtils = createGitHubUtils(
+                createMockClient(async (query, variables) => {
+                    calls++;
+                    assert.match(query, /team0: team\(slug: \$teamSlug0\)/);
+                    assert.match(query, /team1: team\(slug: \$teamSlug1\)/);
+                    assert.match(query, /member0: members\(first: 1, query: \$member0\)/);
+                    assert.match(query, /member1: members\(first: 1, query: \$member1\)/);
+                    assert.doesNotMatch(query, /pageInfo|after:/);
+                    assert.deepEqual(variables, {
+                        organization: 'Expensify',
+                        member0: 'AndrewGable',
+                        member1: 'outsider',
+                        teamSlug0: 'expensify-expensify',
+                        teamSlug1: 'writers',
+                    });
+                    return {
+                        organization: {
+                            team0: {
+                                member0: {nodes: [{login: 'AndrewGable'}]},
+                                member1: {nodes: [{login: 'outsider-other'}]},
+                            },
+                            team1: {
+                                member0: {nodes: []},
+                                member1: {nodes: [{login: 'Outsider'}]},
+                            },
+                        },
+                    };
+                }),
+            );
+            assert.deepEqual(
+                await gitHubUtils.getTeamMemberLogins(['expensify-expensify', 'writers', 'expensify-expensify'], ['AndrewGable', 'outsider', 'AndrewGable']),
+                new Set(['AndrewGable']),
+            );
+            assert.equal(calls, 1);
+        });
+
+        it('skips the API when there are no teams or candidates', async () => {
+            const gitHubUtils = createGitHubUtils(
+                createMockClient(async () => {
+                    throw new Error('Unexpected API call');
+                }),
+            );
+            assert.deepEqual(await gitHubUtils.getTeamMemberLogins(['team'], []), new Set());
+            assert.deepEqual(await gitHubUtils.getTeamMemberLogins([], ['alice']), new Set());
+        });
+
+        it('fails closed for missing organizations, teams, or search results', async () => {
+            for (const response of [{organization: null}, {organization: {team0: null}}, {organization: {team0: {}}}, {organization: {team0: {member0: null}}}]) {
+                const gitHubUtils = createGitHubUtils(createMockClient(async () => response));
+                await assert.rejects(() => gitHubUtils.getTeamMemberLogins(['team'], ['alice']));
+            }
+        });
+
+        it('propagates API failures', async () => {
+            const gitHubUtils = createGitHubUtils(
+                createMockClient(async () => {
+                    throw new Error('Forbidden');
+                }),
+            );
+            await assert.rejects(() => gitHubUtils.getTeamMemberLogins(['team'], ['alice']), /Forbidden/);
+        });
+
+        it('returns only exact candidate logins', async () => {
             const gitHubUtils = createGitHubUtils(
                 createMockClient(async () => ({
                     organization: {
-                        team: {
-                            members: {
-                                pageInfo: {hasNextPage: false, endCursor: null},
-                                nodes: [{login: 'AndrewGable'}],
-                            },
+                        team0: {
+                            member0: {nodes: [{login: 'AndrewGable'}]},
                         },
                     },
                 })),
             );
 
-            const logins = await gitHubUtils.getTeamMemberLogins('expensify-expensify');
+            const logins = await gitHubUtils.getTeamMemberLogins(['expensify-expensify'], ['AndrewGable']);
 
             // GitHub logins are case-sensitive, so this is a direct set lookup, not a case-insensitive match.
             assert.equal(logins.has('AndrewGable'), true);