diff --git a/scripts/libs/GitHubAPIClient.ts b/scripts/libs/GitHubAPIClient.ts index caf2b85..22095e7 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; + declaration: `$${Name}: String!`; + reference: `$${Name}`; +}; + +function createGraphQLVariable(name: Name, value: string): GraphQLVariable { + 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}; diff --git a/scripts/libs/GitHubUtils/getTeamMemberLogins.ts b/scripts/libs/GitHubUtils/getTeamMemberLogins.ts index a22bb45..c5a257c 100644 --- a/scripts/libs/GitHubUtils/getTeamMemberLogins.ts +++ b/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>>>(); +type TeamMemberSearchResult = { + nodes: Array<{login: string}>; +}; + +type TeamMembersResponse = { + organization: Record | 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> { - let teamPromises = teamMemberLoginsPromisesByClient.get(client); - if (!teamPromises) { - teamPromises = new Map(); - teamMemberLoginsPromisesByClient.set(client, teamPromises); - } +async function getTeamMemberLogins(client: GitHubAPIClient, teamSlugs: string[], candidateLogins: string[]): Promise> { + const teamMemberLogins = new Set(); + 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> { - const teamMemberLogins = new Set(); - 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( - ` - 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 = {}; + 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')} + } } + `; - for (const member of members.nodes) { - teamMemberLogins.add(member.login); + const response = await client.graphql(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; diff --git a/scripts/libs/GitHubUtils/index.ts b/scripts/libs/GitHubUtils/index.ts index bf9d2ed..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) => getTeamMemberLogins(client, teamSlug), + 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 7d2b1c1..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))); - 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 f93d34e..3d5a5e7 100644 --- a/tests/GitHubUtils.test.ts +++ b/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); diff --git a/tests/verifyPeerReviewPolicy.test.ts b/tests/verifyPeerReviewPolicy.test.ts index 18333b6..365311a 100644 --- a/tests/verifyPeerReviewPolicy.test.ts +++ b/tests/verifyPeerReviewPolicy.test.ts @@ -27,6 +27,31 @@ const BASE_INPUT: PeerReviewInput = { describe('verifyPeerReview', () => { describe('getIndependentApprovers', () => { + 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 (teams, candidates) => { + callCount++; + teamSlugs = teams; + candidateLogins = candidates; + return new Set(['MonilBhavsar']); + }, + }); + const independent = await VerifyPeerReview.getIndependentApprovers( + gitHubUtils, + ['AndrewGable', 'MonilBhavsar', 'outsider'], + ['AndrewGable'], + 'Expensify', + 'react-native-live-markdown', + ); + assert.deepEqual(independent, ['MonilBhavsar']); + 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 () => { const gitHubUtils = createFakeGitHubUtils({getTeamMemberLogins: async () => new Set(['AndrewGable', 'MonilBhavsar'])}); @@ -62,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, }); } @@ -155,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'}); @@ -167,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']); }, }); @@ -187,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']); }, });