From bfe5d06372ed9944eb96060a70805582ccd0c88b Mon Sep 17 00:00:00 2001 From: rory Date: Thu, 17 Sep 2026 17:20:38 -0700 Subject: [PATCH 1/3] perf: target peer-review team membership lookups --- .../libs/GitHubUtils/getTeamMemberLogins.ts | 118 ++++++++---------- scripts/libs/GitHubUtils/index.ts | 2 +- scripts/verifyPeerReview.ts | 2 +- tests/GitHubUtils.test.ts | 93 +++++++++++++- tests/verifyPeerReviewPolicy.test.ts | 20 +++ 5 files changed, 165 insertions(+), 70 deletions(-) diff --git a/scripts/libs/GitHubUtils/getTeamMemberLogins.ts b/scripts/libs/GitHubUtils/getTeamMemberLogins.ts index a22bb45..412e069 100644 --- a/scripts/libs/GitHubUtils/getTeamMemberLogins.ts +++ b/scripts/libs/GitHubUtils/getTeamMemberLogins.ts @@ -4,86 +4,72 @@ const EXPENSIFY_ORG = 'Expensify'; type TeamMembersResponse = { organization: { - team: { - members: { - pageInfo: { - hasNextPage: boolean; - endCursor: string | null; - }; - nodes: Array<{ - login: string; - }>; - }; - } | null; + team: Record< + string, + { + pageInfo: {hasNextPage: boolean; endCursor: string | null}; + nodes: Array<{login: string}>; + } + > | null; } | null; }; -const teamMemberLoginsPromisesByClient = new WeakMap>>>(); - /** - * Fetches and memoizes all members of a GitHub team for the lifetime of a client. + * Returns the candidate logins that belong to a team, batching their searches into one request per page. */ -async function getTeamMemberLogins(client: GitHubAPIClient, teamSlug: string): Promise> { - let teamPromises = teamMemberLoginsPromisesByClient.get(client); - if (!teamPromises) { - teamPromises = new Map(); - teamMemberLoginsPromisesByClient.set(client, teamPromises); - } - - let teamMembersPromise = teamPromises.get(teamSlug); - if (!teamMembersPromise) { - teamMembersPromise = fetchTeamMemberLogins(client, teamSlug); - teamPromises.set(teamSlug, teamMembersPromise); - } - - return teamMembersPromise; -} - -async function fetchTeamMemberLogins(client: GitHubAPIClient, teamSlug: string): Promise> { +async function getTeamMemberLogins(client: GitHubAPIClient, teamSlug: string, candidateLogins: string[]): Promise> { const teamMemberLogins = new Set(); - let cursor: string | null = null; - let hasNextPage = true; + let searches = [...new Set(candidateLogins)].map((login, index) => ({login, alias: `member${index}`, cursor: null as string | null})); - 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( - ` - query TeamMembers($organization: String!, $teamSlug: String!, $cursor: String) { - organization(login: $organization) { - team(slug: $teamSlug) { - members(first: 100, after: $cursor) { - pageInfo { - hasNextPage - endCursor - } - nodes { - login - } - } - } - } + while (searches.length > 0) { + const variables: Record = {organization: EXPENSIFY_ORG, teamSlug}; + const declarations = searches.map(({alias, login, cursor}) => { + variables[alias] = login; + variables[`${alias}Cursor`] = cursor; + return `$${alias}: String!, $${alias}Cursor: String`; + }); + const fields = searches.map( + ({alias}) => ` + ${alias}: members(first: 100, query: $${alias}, after: $${alias}Cursor) { + pageInfo { hasNextPage endCursor } + nodes { login } } `, - { - organization: EXPENSIFY_ORG, - teamSlug, - cursor, - }, ); - const members = response.organization?.team?.members; - if (!members) { + // Each subsequent request depends on the cursors returned by the previous page. + // eslint-disable-next-line no-await-in-loop + const response = await client.graphql( + `query TeamMembers($organization: String!, $teamSlug: String!, ${declarations.join(', ')}) { + organization(login: $organization) { + team(slug: $teamSlug) { ${fields.join('\n')} } + } + }`, + variables, + ); + const team = response.organization?.team; + if (!team) { throw new Error(`${EXPENSIFY_ORG}/${teamSlug} team could not be found.`); } - for (const member of members.nodes) { - teamMemberLogins.add(member.login); - } - - hasNextPage = members.pageInfo.hasNextPage; - cursor = members.pageInfo.endCursor; + searches = searches.flatMap((search) => { + const members = team[search.alias]; + if (!members) { + throw new Error(`Missing team membership search result for ${search.login}.`); + } + // GitHub's query is a fuzzy search, so only an exact login match proves membership. + if (members.nodes.some(({login}) => login === search.login)) { + teamMemberLogins.add(search.login); + return []; + } + if (!members.pageInfo.hasNextPage) { + return []; + } + if (!members.pageInfo.endCursor || members.pageInfo.endCursor === search.cursor) { + throw new Error(`Missing or repeated team membership cursor for ${search.login}.`); + } + return [{...search, cursor: members.pageInfo.endCursor}]; + }); } return teamMemberLogins; diff --git a/scripts/libs/GitHubUtils/index.ts b/scripts/libs/GitHubUtils/index.ts index bf9d2ed..4544e3f 100644 --- a/scripts/libs/GitHubUtils/index.ts +++ b/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: (teamSlug: string, candidateLogins: string[]) => getTeamMemberLogins(client, teamSlug, candidateLogins), listPullRequestCommits: (args: {owner: string; repo: string; number: number}) => listPullRequestCommits(client, args), }; } diff --git a/scripts/verifyPeerReview.ts b/scripts/verifyPeerReview.ts index 7d2b1c1..ee49d5e 100644 --- a/scripts/verifyPeerReview.ts +++ b/scripts/verifyPeerReview.ts @@ -91,7 +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 teamMembers = await Promise.all(reviewerTeams.map((teamSlug) => gitHubUtils.getTeamMemberLogins(teamSlug, independentApprovers))); const eligibleReviewers = new Set(teamMembers.flatMap((members) => [...members])); return independentApprovers.filter((approver) => eligibleReviewers.has(approver)); diff --git a/tests/GitHubUtils.test.ts b/tests/GitHubUtils.test.ts index f93d34e..02995ac 100644 --- a/tests/GitHubUtils.test.ts +++ b/tests/GitHubUtils.test.ts @@ -392,12 +392,101 @@ describe('GitHubUtils', () => { }); describe('getTeamMemberLogins', () => { + it('batches unique candidates and rejects fuzzy and differently cased matches', async () => { + let calls = 0; + const gitHubUtils = createGitHubUtils( + createMockClient(async (query, variables) => { + calls++; + assert.match(query, /member0: members\(first: 100, query: \$member0/); + assert.match(query, /member1: members\(first: 100, query: \$member1/); + assert.deepEqual(variables, { + organization: 'Expensify', + teamSlug: 'expensify-expensify', + member0: 'AndrewGable', + member0Cursor: null, + member1: 'outsider', + member1Cursor: null, + }); + return { + organization: { + team: { + member0: {nodes: [{login: 'AndrewGable'}], pageInfo: {hasNextPage: false, endCursor: null}}, + member1: {nodes: [{login: 'outsider-other'}, {login: 'Outsider'}], pageInfo: {hasNextPage: false, endCursor: null}}, + }, + }, + }; + }), + ); + assert.deepEqual(await gitHubUtils.getTeamMemberLogins('expensify-expensify', ['AndrewGable', 'outsider', 'AndrewGable']), new Set(['AndrewGable'])); + assert.equal(calls, 1); + }); + + it('paginates only unresolved searches so fuzzy matches cannot hide an exact match', async () => { + let calls = 0; + const gitHubUtils = createGitHubUtils( + createMockClient(async (query, variables) => { + calls++; + if (calls === 1) { + return { + organization: { + team: { + member0: {nodes: [{login: 'alice'}], pageInfo: {hasNextPage: true, endCursor: 'unused'}}, + member1: {nodes: [{login: 'bob-other'}], pageInfo: {hasNextPage: true, endCursor: 'next'}}, + }, + }, + }; + } + assert.doesNotMatch(query, /member0/); + assert.equal(variables?.member1Cursor, 'next'); + return { + organization: { + team: { + member1: {nodes: [{login: 'bob'}], pageInfo: {hasNextPage: false, endCursor: null}}, + }, + }, + }; + }), + ); + assert.deepEqual(await gitHubUtils.getTeamMemberLogins('team', ['alice', 'bob']), new Set(['alice', 'bob'])); + assert.equal(calls, 2); + }); + + it('skips the API when there are no candidates', async () => { + const gitHubUtils = createGitHubUtils( + createMockClient(async () => { + throw new Error('Unexpected API call'); + }), + ); + assert.deepEqual(await gitHubUtils.getTeamMemberLogins('team', []), new Set()); + }); + + it('fails closed for missing organizations, teams, results, and cursors', async () => { + for (const response of [ + {organization: null}, + {organization: {team: null}}, + {organization: {team: {}}}, + {organization: {team: {member0: {nodes: [], pageInfo: {hasNextPage: true, endCursor: 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 the fetched team login set', async () => { const gitHubUtils = createGitHubUtils( createMockClient(async () => ({ organization: { team: { - members: { + member0: { pageInfo: {hasNextPage: false, endCursor: null}, nodes: [{login: 'AndrewGable'}], }, @@ -406,7 +495,7 @@ describe('GitHubUtils', () => { })), ); - 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); diff --git a/tests/verifyPeerReviewPolicy.test.ts b/tests/verifyPeerReviewPolicy.test.ts index 18333b6..39cc7c1 100644 --- a/tests/verifyPeerReviewPolicy.test.ts +++ b/tests/verifyPeerReviewPolicy.test.ts @@ -27,6 +27,26 @@ const BASE_INPUT: PeerReviewInput = { describe('verifyPeerReview', () => { describe('getIndependentApprovers', () => { + it('looks up only independent approvers in each eligible team', async () => { + const teams: string[] = []; + const gitHubUtils = createFakeGitHubUtils({ + getTeamMemberLogins: async (teamSlug, candidates) => { + teams.push(teamSlug); + assert.deepEqual(candidates, ['MonilBhavsar', 'outsider']); + return new Set(['MonilBhavsar']); + }, + }); + const independent = await VerifyPeerReview.getIndependentApprovers( + gitHubUtils, + ['AndrewGable', 'MonilBhavsar', 'outsider'], + ['AndrewGable'], + 'Expensify', + 'react-native-live-markdown', + ); + assert.deepEqual(independent, ['MonilBhavsar']); + assert.deepEqual(teams, ['expensify-expensify', 'react-native-live-markdown-writers', 'react-native-live-markdown-maintainers']); + }); + it('excludes commit authors and non-employees', async () => { const gitHubUtils = createFakeGitHubUtils({getTeamMemberLogins: async () => new Set(['AndrewGable', 'MonilBhavsar'])}); From 0b5a4f9eceb3a4f8328733819cedc50d65f25530 Mon Sep 17 00:00:00 2001 From: rory Date: Thu, 17 Sep 2026 18:13:25 -0700 Subject: [PATCH 2/3] refactor: clarify team membership queries --- scripts/libs/GitHubAPIClient.ts | 19 ++- .../libs/GitHubUtils/getTeamMemberLogins.ts | 129 +++++++++++++----- 2 files changed, 111 insertions(+), 37 deletions(-) diff --git a/scripts/libs/GitHubAPIClient.ts b/scripts/libs/GitHubAPIClient.ts index caf2b85..abdee41 100644 --- a/scripts/libs/GitHubAPIClient.ts +++ b/scripts/libs/GitHubAPIClient.ts @@ -8,6 +8,22 @@ type InternalOctokit = InstanceType; const OctokitWithPlugins = Octokit.plugin(throttling, paginateRest); +type GraphQLVariable = { + name: Name; + value: string | null; + declaration: `$${Name}: String${'' | '!'}`; + reference: `$${Name}`; +}; + +function createGraphQLVariable(name: Name, value: string | null, type: 'String' | 'String!'): GraphQLVariable { + return { + name, + value, + declaration: `$${name}: ${type}`, + 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}; diff --git a/scripts/libs/GitHubUtils/getTeamMemberLogins.ts b/scripts/libs/GitHubUtils/getTeamMemberLogins.ts index 412e069..eebe581 100644 --- a/scripts/libs/GitHubUtils/getTeamMemberLogins.ts +++ b/scripts/libs/GitHubUtils/getTeamMemberLogins.ts @@ -1,16 +1,26 @@ import type GitHubAPIClient from '../GitHubAPIClient'; +import {createGraphQLVariable} from '../GitHubAPIClient'; +import type {GraphQLVariable} from '../GitHubAPIClient'; const EXPENSIFY_ORG = 'Expensify'; +type MembershipSearch = { + login: string; + alias: string; + cursor: string | null; +}; + +type TeamMemberPage = { + pageInfo: { + hasNextPage: boolean; + endCursor: string | null; + }; + nodes: Array<{login: string}>; +}; + type TeamMembersResponse = { organization: { - team: Record< - string, - { - pageInfo: {hasNextPage: boolean; endCursor: string | null}; - nodes: Array<{login: string}>; - } - > | null; + team: Record | null; } | null; }; @@ -19,57 +29,104 @@ type TeamMembersResponse = { */ async function getTeamMemberLogins(client: GitHubAPIClient, teamSlug: string, candidateLogins: string[]): Promise> { const teamMemberLogins = new Set(); - let searches = [...new Set(candidateLogins)].map((login, index) => ({login, alias: `member${index}`, cursor: null as string | null})); - - while (searches.length > 0) { - const variables: Record = {organization: EXPENSIFY_ORG, teamSlug}; - const declarations = searches.map(({alias, login, cursor}) => { - variables[alias] = login; - variables[`${alias}Cursor`] = cursor; - return `$${alias}: String!, $${alias}Cursor: String`; + let pendingSearches: MembershipSearch[] = []; + + // Each candidate gets a stable GraphQL alias so we can identify their results in the response. + // Logins can't be used as GraphQL aliases because they may contain hyphens + const uniqueLogins = new Set(candidateLogins); + for (const login of uniqueLogins) { + pendingSearches.push({ + login, + alias: `member${pendingSearches.length}`, + cursor: null, }); - const fields = searches.map( - ({alias}) => ` - ${alias}: members(first: 100, query: $${alias}, after: $${alias}Cursor) { - pageInfo { hasNextPage endCursor } - nodes { login } + } + + while (pendingSearches.length > 0) { + const organizationVariable = createGraphQLVariable('organization', EXPENSIFY_ORG, 'String!'); + const teamSlugVariable = createGraphQLVariable('teamSlug', teamSlug, 'String!'); + const queryVariables: GraphQLVariable[] = [organizationVariable, teamSlugVariable]; + const memberQueries: string[] = []; + + // Build one request containing a separate team-member search for each pending login. + for (const search of pendingSearches) { + const loginVariable = createGraphQLVariable(search.alias, search.login, 'String!'); + const cursorVariable = createGraphQLVariable(`${search.alias}Cursor`, search.cursor, 'String'); + queryVariables.push(loginVariable, cursorVariable); + + memberQueries.push(` + ${search.alias}: members(first: 100, query: ${loginVariable.reference}, after: ${cursorVariable.reference}) { + pageInfo { + hasNextPage + endCursor + } + nodes { + login + } + } + `); + } + + const variables: Record = {}; + const variableDeclarations: Array = []; + for (const variable of queryVariables) { + variables[variable.name] = variable.value; + variableDeclarations.push(variable.declaration); + } + + const query = ` + query TeamMembers(${variableDeclarations.join(', ')}) { + organization(login: ${organizationVariable.reference}) { + team(slug: ${teamSlugVariable.reference}) { + ${memberQueries.join('\n')} + } + } } - `, - ); + `; // Each subsequent request depends on the cursors returned by the previous page. // eslint-disable-next-line no-await-in-loop - const response = await client.graphql( - `query TeamMembers($organization: String!, $teamSlug: String!, ${declarations.join(', ')}) { - organization(login: $organization) { - team(slug: $teamSlug) { ${fields.join('\n')} } - } - }`, - variables, - ); + const response = await client.graphql(query, variables); const team = response.organization?.team; if (!team) { throw new Error(`${EXPENSIFY_ORG}/${teamSlug} team could not be found.`); } - searches = searches.flatMap((search) => { + const nextPageSearches: MembershipSearch[] = []; + for (const search of pendingSearches) { const members = team[search.alias]; if (!members) { throw new Error(`Missing team membership search result for ${search.login}.`); } + // GitHub's query is a fuzzy search, so only an exact login match proves membership. - if (members.nodes.some(({login}) => login === search.login)) { + let foundExactMatch = false; + for (const member of members.nodes) { + if (member.login === search.login) { + foundExactMatch = true; + break; + } + } + + if (foundExactMatch) { teamMemberLogins.add(search.login); - return []; + continue; } if (!members.pageInfo.hasNextPage) { - return []; + continue; } if (!members.pageInfo.endCursor || members.pageInfo.endCursor === search.cursor) { throw new Error(`Missing or repeated team membership cursor for ${search.login}.`); } - return [{...search, cursor: members.pageInfo.endCursor}]; - }); + // We haven't found this login yet, but there are more results to check. + nextPageSearches.push({ + login: search.login, + alias: search.alias, + cursor: members.pageInfo.endCursor, + }); + } + + pendingSearches = nextPageSearches; } return teamMemberLogins; From 944a4519ae465d1fa439fe83007b68179993f028 Mon Sep 17 00:00:00 2001 From: rory Date: Thu, 24 Sep 2026 16:09:30 +0000 Subject: [PATCH 3/3] perf: batch peer-review team queries --- scripts/libs/GitHubAPIClient.ts | 8 +- .../libs/GitHubUtils/getTeamMemberLogins.ts | 166 ++++++++---------- scripts/libs/GitHubUtils/index.ts | 2 +- scripts/verifyPeerReview.ts | 3 +- tests/GitHubUtils.test.ts | 88 ++++------ tests/verifyPeerReviewPolicy.test.ts | 43 ++--- 6 files changed, 133 insertions(+), 177 deletions(-) diff --git a/scripts/libs/GitHubAPIClient.ts b/scripts/libs/GitHubAPIClient.ts index abdee41..22095e7 100644 --- a/scripts/libs/GitHubAPIClient.ts +++ b/scripts/libs/GitHubAPIClient.ts @@ -10,16 +10,16 @@ const OctokitWithPlugins = Octokit.plugin(throttling, paginateRest); type GraphQLVariable = { name: Name; - value: string | null; - declaration: `$${Name}: String${'' | '!'}`; + value: string; + declaration: `$${Name}: String!`; reference: `$${Name}`; }; -function createGraphQLVariable(name: Name, value: string | null, type: 'String' | 'String!'): GraphQLVariable { +function createGraphQLVariable(name: Name, value: string): GraphQLVariable { return { name, value, - declaration: `$${name}: ${type}`, + declaration: `$${name}: String!`, reference: `$${name}`, }; } diff --git a/scripts/libs/GitHubUtils/getTeamMemberLogins.ts b/scripts/libs/GitHubUtils/getTeamMemberLogins.ts index eebe581..c5a257c 100644 --- a/scripts/libs/GitHubUtils/getTeamMemberLogins.ts +++ b/scripts/libs/GitHubUtils/getTeamMemberLogins.ts @@ -4,129 +4,113 @@ import type {GraphQLVariable} from '../GitHubAPIClient'; const EXPENSIFY_ORG = 'Expensify'; -type MembershipSearch = { +type TeamMemberSearch = { login: string; alias: string; - cursor: string | null; + variable: GraphQLVariable; }; -type TeamMemberPage = { - pageInfo: { - hasNextPage: boolean; - endCursor: string | null; - }; +type TeamSearch = { + slug: string; + alias: string; + variable: GraphQLVariable; +}; + +type TeamMemberSearchResult = { nodes: Array<{login: string}>; }; type TeamMembersResponse = { - organization: { - team: Record | null; - } | null; + organization: Record | null> | null; }; /** - * Returns the candidate logins that belong to a team, batching their searches into one request per page. + * Returns the candidate logins that belong to any of the teams, batching all searches into one request. */ -async function getTeamMemberLogins(client: GitHubAPIClient, teamSlug: string, candidateLogins: string[]): Promise> { +async function getTeamMemberLogins(client: GitHubAPIClient, teamSlugs: string[], candidateLogins: string[]): Promise> { const teamMemberLogins = new Set(); - let pendingSearches: MembershipSearch[] = []; + const uniqueTeamSlugs = new Set(teamSlugs); + const uniqueCandidateLogins = new Set(candidateLogins); - // Each candidate gets a stable GraphQL alias so we can identify their results in the response. - // Logins can't be used as GraphQL aliases because they may contain hyphens - const uniqueLogins = new Set(candidateLogins); - for (const login of uniqueLogins) { - pendingSearches.push({ - login, - alias: `member${pendingSearches.length}`, - cursor: null, - }); + if (uniqueTeamSlugs.size === 0 || uniqueCandidateLogins.size === 0) { + return teamMemberLogins; } - while (pendingSearches.length > 0) { - const organizationVariable = createGraphQLVariable('organization', EXPENSIFY_ORG, 'String!'); - const teamSlugVariable = createGraphQLVariable('teamSlug', teamSlug, 'String!'); - const queryVariables: GraphQLVariable[] = [organizationVariable, teamSlugVariable]; - const memberQueries: string[] = []; - - // Build one request containing a separate team-member search for each pending login. - for (const search of pendingSearches) { - const loginVariable = createGraphQLVariable(search.alias, search.login, 'String!'); - const cursorVariable = createGraphQLVariable(`${search.alias}Cursor`, search.cursor, 'String'); - queryVariables.push(loginVariable, cursorVariable); - - memberQueries.push(` - ${search.alias}: members(first: 100, query: ${loginVariable.reference}, after: ${cursorVariable.reference}) { - pageInfo { - hasNextPage - endCursor - } + 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), + }; + }); + + 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 } } - `); - } - - const variables: Record = {}; - const variableDeclarations: Array = []; - for (const variable of queryVariables) { - variables[variable.name] = variable.value; - variableDeclarations.push(variable.declaration); - } + `, + ); - const query = ` - query TeamMembers(${variableDeclarations.join(', ')}) { - organization(login: ${organizationVariable.reference}) { - team(slug: ${teamSlugVariable.reference}) { - ${memberQueries.join('\n')} - } - } + return ` + ${teamSearch.alias}: team(slug: ${teamSearch.variable.reference}) { + ${memberQueries.join('\n')} } `; + }); + + const variables: Record = {}; + const variableDeclarations: Array = []; + 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')} + } + } + `; + + const response = await client.graphql(query, variables); + const organization = response.organization; + if (!organization) { + throw new Error(`${EXPENSIFY_ORG} organization could not be found.`); + } - // Each subsequent request depends on the cursors returned by the previous page. - // eslint-disable-next-line no-await-in-loop - const response = await client.graphql(query, variables); - const team = response.organization?.team; + for (const teamSearch of teamSearches) { + const team = organization[teamSearch.alias]; if (!team) { - throw new Error(`${EXPENSIFY_ORG}/${teamSlug} team could not be found.`); + throw new Error(`${EXPENSIFY_ORG}/${teamSearch.slug} team could not be found.`); } - const nextPageSearches: MembershipSearch[] = []; - for (const search of pendingSearches) { - const members = team[search.alias]; + for (const memberSearch of memberSearches) { + const members = team[memberSearch.alias]; if (!members) { - throw new Error(`Missing team membership search result for ${search.login}.`); + throw new Error(`Missing team membership search result for ${memberSearch.login} in ${teamSearch.slug}.`); } - // GitHub's query is a fuzzy search, so only an exact login match proves membership. - let foundExactMatch = false; - for (const member of members.nodes) { - if (member.login === search.login) { - foundExactMatch = true; - break; - } - } - - if (foundExactMatch) { - teamMemberLogins.add(search.login); - continue; + // 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); } - if (!members.pageInfo.hasNextPage) { - continue; - } - if (!members.pageInfo.endCursor || members.pageInfo.endCursor === search.cursor) { - throw new Error(`Missing or repeated team membership cursor for ${search.login}.`); - } - // We haven't found this login yet, but there are more results to check. - nextPageSearches.push({ - login: search.login, - alias: search.alias, - cursor: members.pageInfo.endCursor, - }); } - - pendingSearches = nextPageSearches; } return teamMemberLogins; diff --git a/scripts/libs/GitHubUtils/index.ts b/scripts/libs/GitHubUtils/index.ts index 4544e3f..d1f3d77 100644 --- a/scripts/libs/GitHubUtils/index.ts +++ b/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, candidateLogins: string[]) => getTeamMemberLogins(client, teamSlug, candidateLogins), + getTeamMemberLogins: (teamSlugs: string[], candidateLogins: string[]) => getTeamMemberLogins(client, teamSlugs, candidateLogins), listPullRequestCommits: (args: {owner: string; repo: string; number: number}) => listPullRequestCommits(client, args), }; } diff --git a/scripts/verifyPeerReview.ts b/scripts/verifyPeerReview.ts index ee49d5e..ddbdc85 100644 --- a/scripts/verifyPeerReview.ts +++ b/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, independentApprovers))); - const eligibleReviewers = new Set(teamMembers.flatMap((members) => [...members])); + const eligibleReviewers = await gitHubUtils.getTeamMemberLogins(reviewerTeams, independentApprovers); return independentApprovers.filter((approver) => eligibleReviewers.has(approver)); } diff --git a/tests/GitHubUtils.test.ts b/tests/GitHubUtils.test.ts index 02995ac..3d5a5e7 100644 --- a/tests/GitHubUtils.test.ts +++ b/tests/GitHubUtils.test.ts @@ -392,83 +392,58 @@ describe('GitHubUtils', () => { }); describe('getTeamMemberLogins', () => { - it('batches unique candidates and rejects fuzzy and differently cased matches', 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, /member0: members\(first: 100, query: \$member0/); - assert.match(query, /member1: members\(first: 100, query: \$member1/); + 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', - teamSlug: 'expensify-expensify', member0: 'AndrewGable', - member0Cursor: null, member1: 'outsider', - member1Cursor: null, + teamSlug0: 'expensify-expensify', + teamSlug1: 'writers', }); return { organization: { - team: { - member0: {nodes: [{login: 'AndrewGable'}], pageInfo: {hasNextPage: false, endCursor: null}}, - member1: {nodes: [{login: 'outsider-other'}, {login: 'Outsider'}], pageInfo: {hasNextPage: false, endCursor: null}}, + team0: { + member0: {nodes: [{login: 'AndrewGable'}]}, + member1: {nodes: [{login: 'outsider-other'}]}, }, - }, - }; - }), - ); - assert.deepEqual(await gitHubUtils.getTeamMemberLogins('expensify-expensify', ['AndrewGable', 'outsider', 'AndrewGable']), new Set(['AndrewGable'])); - assert.equal(calls, 1); - }); - - it('paginates only unresolved searches so fuzzy matches cannot hide an exact match', async () => { - let calls = 0; - const gitHubUtils = createGitHubUtils( - createMockClient(async (query, variables) => { - calls++; - if (calls === 1) { - return { - organization: { - team: { - member0: {nodes: [{login: 'alice'}], pageInfo: {hasNextPage: true, endCursor: 'unused'}}, - member1: {nodes: [{login: 'bob-other'}], pageInfo: {hasNextPage: true, endCursor: 'next'}}, - }, - }, - }; - } - assert.doesNotMatch(query, /member0/); - assert.equal(variables?.member1Cursor, 'next'); - return { - organization: { - team: { - member1: {nodes: [{login: 'bob'}], pageInfo: {hasNextPage: false, endCursor: null}}, + team1: { + member0: {nodes: []}, + member1: {nodes: [{login: 'Outsider'}]}, }, }, }; }), ); - assert.deepEqual(await gitHubUtils.getTeamMemberLogins('team', ['alice', 'bob']), new Set(['alice', 'bob'])); - assert.equal(calls, 2); + 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 candidates', async () => { + 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(['team'], []), new Set()); + assert.deepEqual(await gitHubUtils.getTeamMemberLogins([], ['alice']), new Set()); }); - it('fails closed for missing organizations, teams, results, and cursors', async () => { - for (const response of [ - {organization: null}, - {organization: {team: null}}, - {organization: {team: {}}}, - {organization: {team: {member0: {nodes: [], pageInfo: {hasNextPage: true, endCursor: null}}}}}, - ]) { + 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'])); + await assert.rejects(() => gitHubUtils.getTeamMemberLogins(['team'], ['alice'])); } }); @@ -478,24 +453,21 @@ describe('GitHubUtils', () => { throw new Error('Forbidden'); }), ); - await assert.rejects(() => gitHubUtils.getTeamMemberLogins('team', ['alice']), /Forbidden/); + await assert.rejects(() => gitHubUtils.getTeamMemberLogins(['team'], ['alice']), /Forbidden/); }); - it('returns the fetched team login set', async () => { + it('returns only exact candidate logins', async () => { const gitHubUtils = createGitHubUtils( createMockClient(async () => ({ organization: { - team: { - member0: { - pageInfo: {hasNextPage: false, endCursor: null}, - nodes: [{login: 'AndrewGable'}], - }, + team0: { + member0: {nodes: [{login: 'AndrewGable'}]}, }, }, })), ); - const logins = await gitHubUtils.getTeamMemberLogins('expensify-expensify', ['AndrewGable']); + 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); diff --git a/tests/verifyPeerReviewPolicy.test.ts b/tests/verifyPeerReviewPolicy.test.ts index 39cc7c1..365311a 100644 --- a/tests/verifyPeerReviewPolicy.test.ts +++ b/tests/verifyPeerReviewPolicy.test.ts @@ -27,12 +27,15 @@ const BASE_INPUT: PeerReviewInput = { describe('verifyPeerReview', () => { describe('getIndependentApprovers', () => { - it('looks up only independent approvers in each eligible team', async () => { - const teams: string[] = []; + it('looks up only independent approvers across all eligible teams in one call', async () => { + let callCount = 0; + let teamSlugs: string[] = []; + let candidateLogins: string[] = []; const gitHubUtils = createFakeGitHubUtils({ - getTeamMemberLogins: async (teamSlug, candidates) => { - teams.push(teamSlug); - assert.deepEqual(candidates, ['MonilBhavsar', 'outsider']); + getTeamMemberLogins: async (teams, candidates) => { + callCount++; + teamSlugs = teams; + candidateLogins = candidates; return new Set(['MonilBhavsar']); }, }); @@ -44,7 +47,9 @@ describe('verifyPeerReview', () => { 'react-native-live-markdown', ); assert.deepEqual(independent, ['MonilBhavsar']); - assert.deepEqual(teams, ['expensify-expensify', 'react-native-live-markdown-writers', 'react-native-live-markdown-maintainers']); + assert.equal(callCount, 1); + assert.deepEqual(teamSlugs, ['expensify-expensify', 'react-native-live-markdown-writers', 'react-native-live-markdown-maintainers']); + assert.deepEqual(candidateLogins, ['MonilBhavsar', 'outsider']); }); it('excludes commit authors and non-employees', async () => { @@ -82,7 +87,7 @@ describe('verifyPeerReview', () => { getRequiredApprovingReviewCount: async () => 1, getLatestApprovers: async () => [], listPullRequestCommits: async () => [], - getTeamMemberLogins: async (teamSlug) => (teamSlug === 'expensify-expensify' ? new Set(['MonilBhavsar', 'AndrewGable', 'rafecolton']) : new Set()), + getTeamMemberLogins: async () => new Set(['MonilBhavsar', 'AndrewGable', 'rafecolton']), ...overrides, }); } @@ -175,7 +180,10 @@ describe('verifyPeerReview', () => { const gitHubUtils = createBaseFakeGitHubUtils({ getLatestApprovers: async () => ['JakubKorytko'], listPullRequestCommits: mockCommits([makeCommit('AndrewGable')]), - getTeamMemberLogins: async (teamSlug) => (teamSlug === 'react-native-wallet-writers' ? new Set(['JakubKorytko']) : new Set()), + getTeamMemberLogins: async (teamSlugs) => { + assert.deepEqual(teamSlugs, ['expensify-expensify', 'react-native-wallet-writers']); + return new Set(['JakubKorytko']); + }, }); const result = await VerifyPeerReview.evaluatePeerReview(gitHubUtils, {...BASE_INPUT, repo: 'react-native-wallet'}); @@ -187,14 +195,9 @@ describe('verifyPeerReview', () => { const gitHubUtils = createBaseFakeGitHubUtils({ getLatestApprovers: async () => ['MonilBhavsar'], listPullRequestCommits: mockCommits([makeCommit('AndrewGable')]), - getTeamMemberLogins: async (teamSlug) => { - if (teamSlug === 'expensify-expensify') { - return new Set(['MonilBhavsar']); - } - if (teamSlug === 'react-native-wallet-writers') { - return new Set(['JakubKorytko']); - } - return new Set(); + getTeamMemberLogins: async (teamSlugs) => { + assert.deepEqual(teamSlugs, ['expensify-expensify', 'react-native-wallet-writers']); + return new Set(['MonilBhavsar']); }, }); @@ -207,11 +210,9 @@ describe('verifyPeerReview', () => { const gitHubUtils = createBaseFakeGitHubUtils({ getLatestApprovers: async () => ['JakubKorytko'], listPullRequestCommits: mockCommits([makeCommit('AndrewGable')]), - getTeamMemberLogins: async (teamSlug) => { - if (teamSlug === 'expensify-expensify') { - return new Set(['MonilBhavsar', 'AndrewGable', 'rafecolton']); - } - throw new Error(`Unexpected team lookup: ${teamSlug}`); + getTeamMemberLogins: async (teamSlugs) => { + assert.deepEqual(teamSlugs, ['expensify-expensify']); + return new Set(['MonilBhavsar', 'AndrewGable', 'rafecolton']); }, });