From a21ac5fb153e3bf418ccd2a4abef2787c241813c Mon Sep 17 00:00:00 2001 From: Julius Marminge Date: Wed, 7 Oct 2026 17:26:25 -0700 Subject: [PATCH] refactor(server): GitHub GraphQL batches use variables and share one pager (#16960) Co-authored-by: Claude Opus 5.5 (1M context) --- .../pullRequest/GitHubPullRequestApi.test.ts | 107 +++++-- .../src/pullRequest/GitHubPullRequestApi.ts | 268 +++++++++--------- .../pullRequest/gitHubPullRequestJson.test.ts | 53 ++-- .../src/pullRequest/gitHubPullRequestJson.ts | 165 ++++++----- .../src/pullRequest/githubStackActions.ts | 20 +- .../src/sourceControl/githubGraphQl.test.ts | 105 +++++++ .../server/src/sourceControl/githubGraphQl.ts | 100 +++++++ 7 files changed, 564 insertions(+), 254 deletions(-) create mode 100644 apps/server/src/sourceControl/githubGraphQl.test.ts create mode 100644 apps/server/src/sourceControl/githubGraphQl.ts diff --git a/apps/server/src/pullRequest/GitHubPullRequestApi.test.ts b/apps/server/src/pullRequest/GitHubPullRequestApi.test.ts index c42bf6d147bfc76d361c995e76e18801155cd9f9..64259e337e922dd71a4eb56e2008f0371572f415 100644 --- a/apps/server/src/pullRequest/GitHubPullRequestApi.test.ts +++ b/apps/server/src/pullRequest/GitHubPullRequestApi.test.ts @@ -767,11 +767,15 @@ layer("GitHubPullRequestApi.layer", (it) => { ); assert.strictEqual(eight?.headBranch, "feat/8"); expect(mockedExecute).toHaveBeenCalledOnce(); - const document = queryAt(0); - expect(document).toContain( - 's0: repository(owner: "acme", name: "web") { pullRequest(number: 7)', + expect(queryAt(0)).toContain( + "s0: repository(owner: $s0_owner, name: $s0_name) { pullRequest(number: $s0_number)", ); - expect(document).toContain("pullRequest(number: 8)"); + expect(varsAt(0)).toMatchObject({ + s0_owner: "acme", + s0_name: "web", + s0_number: 7, + s1_number: 8, + }); }), ); @@ -815,10 +819,10 @@ layer("GitHubPullRequestApi.layer", (it) => { // GitHub had no answer for #8, so its watch reads it in full. expect(eight).toBeNull(); expect(mockedExecute).toHaveBeenCalledOnce(); - const call = callAt(0); - expect(call.kind === "graphql" ? call.query : "").toContain( - 'w1: repository(owner: "acme", name: "web") { pullRequest(number: 8)', + expect(queryAt(0)).toContain( + "w1: repository(owner: $w1_owner, name: $w1_name) { pullRequest(number: $w1_number)", ); + expect(varsAt(0)).toMatchObject({ w1_owner: "acme", w1_name: "web", w1_number: 8 }); }), ); @@ -1108,6 +1112,37 @@ layer("GitHubPullRequestApi.layer", (it) => { }), ); + it.effect("reports truncation when GitHub repeats a cursor before the page is full", () => + Effect.gen(function* () { + // Two pages of 100 that hand back the same cursor, for a page of 250. + const repeating = output( + encodeJson({ + data: { + search: { + pageInfo: { hasNextPage: true, endCursor: "same" }, + nodes: rows(100, 1), + }, + }, + }), + ); + mockedExecute.mockReturnValue(Effect.succeed(repeating)); + const cli = yield* GitHubPullRequestApi.GitHubPullRequestApi; + + const batch = yield* cli.listPullRequests({ + cwd: "/w", + repository: "acme/web", + host: "github.com", + state: "open", + involvement: "all", + viewer: "bilal", + limit: 250, + }); + + assert.strictEqual(mockedExecute.mock.calls.length, 2); + assert.isTrue(batch.truncated); + }), + ); + it.effect("excludes merged pull requests from the Closed tab", () => Effect.gen(function* () { mockedExecute.mockImplementation((call) => @@ -1408,11 +1443,12 @@ layer("GitHubPullRequestApi.layer", (it) => { expect(batch.continues).toBe(false); expect(mockedStackMemberships).toHaveBeenCalledTimes(1); const membership = mockedStackMemberships.mock.calls[0]?.[0]; - const query = membership?.kind === "graphql" ? membership.query : ""; - expect(query).toContain("pullRequest(number: 4)"); - expect(query).toContain("pullRequest(number: 5)"); - expect(query).not.toContain("pullRequest(number: 1)"); - expect(query).not.toContain("pullRequest(number: 6)"); + const numbers = Object.entries( + membership?.kind === "graphql" ? (membership.variables ?? {}) : {}, + ) + .filter(([name]) => name.endsWith("_number")) + .map(([, number]) => number); + expect(numbers).toEqual([4, 5]); }), ); @@ -1420,7 +1456,7 @@ layer("GitHubPullRequestApi.layer", (it) => { Effect.gen(function* () { mockedExecute.mockReturnValueOnce(Effect.succeed(output(pullRequests(27, 1)))); mockedStackMemberships.mockImplementation((input) => - input.kind === "graphql" && input.query.includes("pullRequest(number: 26)") + input.kind === "graphql" && Object.values(input.variables ?? {}).includes(26) ? Effect.fail( new GitHubApi.GitHubApiResponseError({ host: "github.com", @@ -1513,9 +1549,8 @@ layer("GitHubPullRequestApi.layer", (it) => { { repository: "acme/web", number: 1, additions: 4, deletions: 1 }, { repository: "acme/web", number: 26, additions: 4, deletions: 1 }, ]); - const document = queryAt(0); - expect(document).toContain('s0: repository(owner: "acme", name: "web")'); - expect(document).toContain("pullRequest(number: 25)"); + expect(queryAt(0)).toContain("s0: repository(owner: $s0_owner, name: $s0_name)"); + expect(varsAt(0)).toMatchObject({ s0_owner: "acme", s0_name: "web", s24_number: 25 }); }), ); @@ -2614,6 +2649,38 @@ layer("GitHubPullRequestApi.layer", (it) => { }), ); + it.effect("refuses workflow approval when the head list stops before its end", () => + Effect.gen(function* () { + // GitHub hands the same cursor back, so the second page is never read past. #7 looks + // unique on what was read, and an unread page could still hold another head like it. + const repeating = heads([7]); + repeating.data.repository.pullRequests.pageInfo = { + hasNextPage: true, + endCursor: "same", + } as never; + workflowApprovalRoutes(() => crossRepositoryDetail(), repeating, workflowRuns([])); + const cli = yield* GitHubPullRequestApi.GitHubPullRequestApi; + + const error = yield* Effect.flip( + cli.listWorkflowRunsRequiringApproval({ + cwd: "/w", + repository: "acme/web", + host: "github.com", + number: 7, + headSha: "abc123", + headBranch: "feat/page", + headRepositoryOwner: "octocat", + isCrossRepository: true, + }), + ); + + expect(error).toMatchObject({ + _tag: "GitHubWorkflowApprovalRefusedError", + reason: "head-list-truncated", + }); + }), + ); + it.effect("refuses workflow approval when GitHub omits the head repository", () => Effect.gen(function* () { mockedExecute.mockReturnValueOnce( @@ -4386,9 +4453,9 @@ layer("GitHubPullRequestApi.layer", (it) => { number: 7, }); - assert.strictEqual(mockedExecute.mock.calls.length, 5); + // The same cursor twice is a page GitHub already gave, so reading stops there. + assert.strictEqual(mockedExecute.mock.calls.length, 2); assert.isTrue(viewed.truncated); - assert.strictEqual(viewed.files.length, 5); }), ); @@ -4420,8 +4487,8 @@ layer("GitHubPullRequestApi.layer", (it) => { expect(queryAt(1)).toContain("f1: unmarkFileAsViewed"); expect(varsAt(1)).toEqual({ pullRequestId: "PR_1", - path0: "src/a.ts", - path1: "src/b.ts", + f0_path: "src/a.ts", + f1_path: "src/b.ts", }); }), ); diff --git a/apps/server/src/pullRequest/GitHubPullRequestApi.ts b/apps/server/src/pullRequest/GitHubPullRequestApi.ts index 116a8a742d1a7c0a0c61810ba59e30f076c60f37..59281964df4a6097b226e0dcafd775e6cc76e199 100644 --- a/apps/server/src/pullRequest/GitHubPullRequestApi.ts +++ b/apps/server/src/pullRequest/GitHubPullRequestApi.ts @@ -44,6 +44,7 @@ import { import { AllowGitHubReserve } from "../sourceControl/GitHubRepositoryApi.ts"; import * as GitHubApi from "../sourceControl/GitHubApi.ts"; +import { readGraphQlPages } from "../sourceControl/githubGraphQl.ts"; import * as VcsProcess from "../vcs/VcsProcess.ts"; import * as SourceControlRateLimit from "../sourceControl/SourceControlRateLimit.ts"; import { @@ -64,7 +65,6 @@ import { PULL_REQUEST_ACTIVITY_GRAPHQL_QUERY, PULL_REQUEST_HEADS_GRAPHQL_QUERY, decodeWorkflowRunsJson, - type GitHubCheckContext, pullRequestCoreGraphQlQuery, type GitHubPullRequestCore, type GitHubPullRequestSummary, @@ -1481,42 +1481,45 @@ export const make = Effect.gen(function* () { * its first hundred. Each page names the head it read, so a push mid-walk fails rather than * mixing two revisions' checks. */ + type CheckContextsPage = Result.Result.Success< + ReturnType + >; const readAllCheckContexts = ( input: Parameters[0], allowReserve: boolean, ) => Effect.gen(function* () { const { owner, name } = parseRepositorySelector(input.repository); - const contexts: GitHubCheckContext[] = []; - let headSha: string | null = null; - let after: string | null = null; - for (let page = 0; page < CHECK_CONTEXT_PAGES; page++) { - const read: { - readonly headSha: string; - readonly contexts: ReadonlyArray; - readonly nextCursor: string | null; - } = yield* graphqlRead({ - cwd: input.cwd, - host: input.host, - operation: "getPullRequestDetail", - allowReserve, - variables: { owner, name, number: input.number, after }, - query: pullRequestCheckContextsGraphQlQuery(input.host), - decode: decodePullRequestCheckContextsJson, - }); - if (headSha !== null && read.headSha !== headSha) { - return yield* readError( - input.cwd, - "getPullRequestDetail", - new Error("Pull request head changed while reading checks."), - ); - } - headSha = read.headSha; - contexts.push(...read.contexts); - after = read.nextCursor; - if (after === null) break; - } - return { headSha, contexts, truncated: after !== null }; + const { pages, truncated } = yield* readGraphQlPages( + (after, previous: ReadonlyArray) => + graphqlRead({ + cwd: input.cwd, + host: input.host, + operation: "getPullRequestDetail", + allowReserve, + variables: { owner, name, number: input.number, after }, + query: pullRequestCheckContextsGraphQlQuery(input.host), + decode: decodePullRequestCheckContextsJson, + }).pipe( + // Each page names the head it read, so a push mid-walk fails rather than mixing + // two revisions' checks. + Effect.filterOrFail( + (page) => previous.length === 0 || page.headSha === previous[0]!.headSha, + () => + readError( + input.cwd, + "getPullRequestDetail", + new Error("Pull request head changed while reading checks."), + ), + ), + ), + { maxPages: CHECK_CONTEXT_PAGES, nextCursor: (page) => page.nextCursor }, + ); + return { + headSha: pages[0]?.headSha ?? null, + contexts: pages.flatMap((page) => page.contexts), + truncated, + }; }); const getPullRequestDetail: GitHubPullRequestApi["Service"]["getPullRequestDetail"] = (input) => { @@ -1568,7 +1571,10 @@ export const make = Effect.gen(function* () { const workflowApprovalReadError = (cwd: string, cause: unknown) => readError(cwd, "listWorkflowRunsRequiringApproval", cause); - /** Every open pull request whose head branch carries this name, up to one past the limit. */ + /** + * Every open pull request whose head branch carries this name, up to one past the limit. + * `truncated` says some were never read, which is never enough to call a head unique. + */ const listHeadsByBranch = (input: { readonly cwd: string; readonly repository: string; @@ -1577,25 +1583,25 @@ export const make = Effect.gen(function* () { }) => Effect.gen(function* () { const { owner, name } = parseRepositorySelector(input.repository); - const heads: GitHubPullRequestHead[] = []; - let after: string | null = null; - do { - const page: { - readonly heads: ReadonlyArray; - readonly nextCursor: string | null; - } = yield* graphqlRead({ - cwd: input.cwd, - host: input.host, - operation: "listWorkflowRunsRequiringApproval", - allowReserve: true, - variables: { owner, name, head: input.headBranch, after }, - query: PULL_REQUEST_HEADS_GRAPHQL_QUERY, - decode: decodePullRequestHeadsJson, - }); - heads.push(...page.heads); - after = page.nextCursor; - } while (after !== null && heads.length <= workflowApprovalLimit); - return heads; + const countHeads = (pages: ReadonlyArray<{ readonly heads: ReadonlyArray }>) => + pages.reduce((total, page) => total + page.heads.length, 0); + const { pages, truncated } = yield* readGraphQlPages( + (after) => + graphqlRead({ + cwd: input.cwd, + host: input.host, + operation: "listWorkflowRunsRequiringApproval", + allowReserve: true, + variables: { owner, name, head: input.headBranch, after }, + query: PULL_REQUEST_HEADS_GRAPHQL_QUERY, + decode: decodePullRequestHeadsJson, + }), + { + nextCursor: (page) => page.nextCursor, + until: (pages) => countHeads(pages) > workflowApprovalLimit, + }, + ); + return { heads: pages.flatMap((page) => page.heads), truncated }; }); /** @@ -1650,9 +1656,10 @@ export const make = Effect.gen(function* () { [ listHeadsByBranch(input).pipe( Effect.flatMap( - ( + ({ heads, - ): Effect.Effect< + truncated, + }): Effect.Effect< GitHubPullRequestHead, GitHubPullRequestReadError | GitHubWorkflowApprovalRefusedError > => { @@ -1663,7 +1670,7 @@ export const make = Effect.gen(function* () { pullRequest.headRepositoryOwner?.toLowerCase() === input.headRepositoryOwner.toLowerCase(), ); - if (heads.length > workflowApprovalLimit) { + if (truncated || heads.length > workflowApprovalLimit) { return Effect.fail( new GitHubWorkflowApprovalRefusedError({ cwd: input.cwd, @@ -1755,18 +1762,18 @@ export const make = Effect.gen(function* () { ); // Stack membership rides along where GitHub serves stacks, so the background sync can skip // the REST stack read for pull requests that are in none. - const query = buildPullRequestSummariesGraphQlQuery( + const document = buildPullRequestSummariesGraphQlQuery( batchable.map((entry) => entry.request), first.request.host === "github.com", ); const batched = - query === null + document === null ? Effect.succeed(new Map()) : graphqlRead({ cwd: first.request.cwd, host: first.request.host, operation: "getPullRequestSummary", - query, + ...document, decode: decodePullRequestSummariesJson, }); return batched.pipe( @@ -1833,17 +1840,17 @@ export const make = Effect.gen(function* () { const batchable = entries.filter( (entry) => buildPullRequestWatchFingerprintsGraphQlQuery([entry.request]) !== null, ); - const query = buildPullRequestWatchFingerprintsGraphQlQuery( + const document = buildPullRequestWatchFingerprintsGraphQlQuery( batchable.map((entry) => entry.request), ); const read = - query === null + document === null ? Effect.succeed(new Map()) : graphqlRead({ cwd: first.request.cwd, host: first.request.host, operation: "getPullRequestWatchFingerprint", - query, + ...document, decode: decodePullRequestWatchFingerprintsJson, }); return read.pipe( @@ -1897,16 +1904,22 @@ export const make = Effect.gen(function* () { ) => Effect.Effect, ) => Effect.gen(function* () { - const items: GitHubPullRequestListItem[] = []; - let rawCount = 0; - let after: string | null = null; - do { - const read: GitHubPullRequestListPage = yield* page(after, rows - rawCount); - items.push(...read.items); - rawCount += read.rawCount; - after = read.endCursor; - } while (after !== null && rawCount < rows); - return { items, rawCount }; + const countRows = (pages: ReadonlyArray) => + pages.reduce((total, read) => total + read.rawCount, 0); + const { pages, truncated } = yield* readGraphQlPages( + (after, pages: ReadonlyArray) => + page(after, rows - countRows(pages)), + { + nextCursor: (read) => read.endCursor, + until: (pages) => countRows(pages) >= rows, + }, + ); + // `truncated` also covers a cursor GitHub repeated before `rows` arrived. + return { + items: pages.flatMap((read) => read.items), + rawCount: countRows(pages), + truncated, + }; }); const read = ( continues: boolean, @@ -1932,7 +1945,7 @@ export const make = Effect.gen(function* () { decode: decodePullRequestListJson, }), ).pipe( - Effect.flatMap(({ items: rawItems, rawCount }) => { + Effect.flatMap(({ items: rawItems, rawCount, truncated: cutShort }) => { const items = continues ? rawItems : rawItems.filter((item) => matchesUnsortedListing(item, input)); @@ -1949,9 +1962,11 @@ export const make = Effect.gen(function* () { items: items.slice(0, input.limit), // One row over the page size is the probe for a next page, and it is // counted before decoding: a skipped malformed row must not end paging. - truncated: continues - ? rawCount > input.limit - : items.length > input.limit || rawCount >= requestedRows, + truncated: + cutShort || + (continues + ? rawCount > input.limit + : items.length > input.limit || rawCount >= requestedRows), continues, }); }), @@ -1989,16 +2004,16 @@ export const make = Effect.gen(function* () { return Effect.forEach( chunks, (chunk) => { - const query = buildPullRequestStackMembershipsGraphQlQuery( + const document = buildPullRequestStackMembershipsGraphQlQuery( input.repository, chunk.map((item) => item.number), ); - if (query === null) return Effect.succeed(chunk); + if (document === null) return Effect.succeed(chunk); return graphqlRead({ cwd: input.cwd, host: input.host, operation: "listPullRequestStackMemberships", - query, + ...document, decode: decodePullRequestStackMembershipsJson, }).pipe( Effect.map((memberships) => @@ -2061,8 +2076,8 @@ export const make = Effect.gen(function* () { return Effect.forEach( chunks, (chunk) => { - const query = buildPullRequestStatsGraphQlQuery(chunk); - if (query === null) { + const document = buildPullRequestStatsGraphQlQuery(chunk); + if (document === null) { return Effect.fail( new GitHubRepositorySelectorError({ cwd: input.cwd, @@ -2074,7 +2089,7 @@ export const make = Effect.gen(function* () { cwd: input.cwd, host: input.host, operation: "listPullRequestStats", - query, + ...document, decode: decodePullRequestStatsJson, }).pipe( Effect.map((stats) => @@ -2328,23 +2343,26 @@ export const make = Effect.gen(function* () { // Almost never entered: the embedded page already holds every dismissal a pull request // ordinarily accrues. Followed so a review whose event fell past that page still finds // its reason. - let dismissalPage = 0; - while (dismissalCursor !== null && dismissalPage < REVIEW_THREAD_PAGES) { - const read: { - readonly dismissalsByReviewId: ReadonlyMap; - readonly nextCursor: string | null; - } = yield* graphqlRead({ - cwd: input.cwd, - host: input.host, - operation: "listReviewThreadComments", - variables: { owner, name, number: input.number, cursor: dismissalCursor }, - query: REVIEW_DISMISSALS_GRAPHQL_QUERY, - decode: decodeReviewDismissalsJson, - }); - for (const [id, message] of read.dismissalsByReviewId) - dismissalsByReviewId.set(id, message); - dismissalCursor = read.nextCursor; - dismissalPage += 1; + if (dismissalCursor !== null) { + const { pages } = yield* readGraphQlPages( + (cursor) => + graphqlRead({ + cwd: input.cwd, + host: input.host, + operation: "listReviewThreadComments", + variables: { owner, name, number: input.number, cursor }, + query: REVIEW_DISMISSALS_GRAPHQL_QUERY, + decode: decodeReviewDismissalsJson, + }), + { + from: dismissalCursor, + maxPages: REVIEW_THREAD_PAGES, + nextCursor: (read) => read.nextCursor, + }, + ); + for (const read of pages) + for (const [id, message] of read.dismissalsByReviewId) + dismissalsByReviewId.set(id, message); } const reviewThreads = entries.map((entry) => ({ @@ -2708,44 +2726,34 @@ export const make = Effect.gen(function* () { getPullRequestFilesViewed: (input) => { const { owner, name } = parseRepositorySelector(input.repository); - const read = ( - after: string | null, - collected: ReadonlyArray, - pagesLeft: number, - ): Effect.Effect => - graphqlRead({ - cwd: input.cwd, - host: input.host, - operation: "getPullRequestFilesViewed", - variables: { owner, name, number: input.number, after }, - query: PULL_REQUEST_FILES_VIEWED_GRAPHQL_QUERY, - decode: decodePullRequestFilesViewedJson, - }).pipe( - Effect.flatMap((page) => { - const files = [...collected, ...page.files]; - if (page.nextCursor === null) { - return Effect.succeed({ files, truncated: false }); - } - return pagesLeft <= 1 - ? Effect.succeed({ files, truncated: true }) - : read(page.nextCursor, files, pagesLeft - 1); + return readGraphQlPages( + (after) => + graphqlRead({ + cwd: input.cwd, + host: input.host, + operation: "getPullRequestFilesViewed", + variables: { owner, name, number: input.number, after }, + query: PULL_REQUEST_FILES_VIEWED_GRAPHQL_QUERY, + decode: decodePullRequestFilesViewedJson, }), - ); - return read(null, [], FILES_VIEWED_MAX_PAGES); + { maxPages: FILES_VIEWED_MAX_PAGES, nextCursor: (page) => page.nextCursor }, + ).pipe( + Effect.map(({ pages, truncated }) => ({ + files: pages.flatMap((page) => page.files), + truncated, + })), + ); }, setPullRequestFilesViewed: (input) => { - const mutation = buildSetFilesViewedGraphQlMutation(input.files); - if (mutation === null) return Effect.void; + if (input.files.length === 0) return Effect.void; return pullRequestNodeId({ ...input, operation: "setPullRequestFilesViewed" }).pipe( - Effect.flatMap((pullRequestId) => - graphql({ - host: input.host, - operation: "setPullRequestFilesViewed", - query: mutation.query, - variables: { pullRequestId, ...mutation.variables }, - }), - ), + Effect.flatMap((pullRequestId) => { + const mutation = buildSetFilesViewedGraphQlMutation(pullRequestId, input.files); + return mutation === null + ? Effect.void + : graphql({ host: input.host, operation: "setPullRequestFilesViewed", ...mutation }); + }), ); }, diff --git a/apps/server/src/pullRequest/gitHubPullRequestJson.test.ts b/apps/server/src/pullRequest/gitHubPullRequestJson.test.ts index 3105634dc0875deb78ef6d2ba9a5ce74bfee60c3..06e25c6e4709a90c6238fce99546b176cfa04d1b 100644 --- a/apps/server/src/pullRequest/gitHubPullRequestJson.test.ts +++ b/apps/server/src/pullRequest/gitHubPullRequestJson.test.ts @@ -1754,36 +1754,40 @@ describe("decodePullRequestFilesViewedJson", () => { describe("buildSetFilesViewedGraphQlMutation", () => { it("asks for nothing when nothing was pressed", () => { - expect(buildSetFilesViewedGraphQlMutation([])).toBeNull(); + expect(buildSetFilesViewedGraphQlMutation("PR_1", [])).toBeNull(); }); it("clears and restores in one document, each file under its own alias", () => { - const mutation = buildSetFilesViewedGraphQlMutation([ + const mutation = buildSetFilesViewedGraphQlMutation("PR_1", [ { path: "src/a.ts", viewed: true }, { path: "src/b.ts", viewed: false }, ]); expect(mutation).not.toBeNull(); if (mutation === null) return; expect(mutation.query).toContain( - "mutation($pullRequestId: ID!, $path0: String!, $path1: String!)", + "mutation($pullRequestId: ID!, $f0_path: String!, $f1_path: String!)", ); expect(mutation.query).toContain( - "f0: markFileAsViewed(input: { pullRequestId: $pullRequestId, path: $path0 })", + "f0: markFileAsViewed(input: { pullRequestId: $pullRequestId, path: $f0_path })", ); expect(mutation.query).toContain( - "f1: unmarkFileAsViewed(input: { pullRequestId: $pullRequestId, path: $path1 })", + "f1: unmarkFileAsViewed(input: { pullRequestId: $pullRequestId, path: $f1_path })", ); - expect(mutation.variables).toEqual({ path0: "src/a.ts", path1: "src/b.ts" }); + expect(mutation.variables).toEqual({ + pullRequestId: "PR_1", + f0_path: "src/a.ts", + f1_path: "src/b.ts", + }); }); it("keeps a path out of the document, so one cannot be read as part of it", () => { - const mutation = buildSetFilesViewedGraphQlMutation([ + const mutation = buildSetFilesViewedGraphQlMutation("PR_1", [ { path: '") { __typename } evil: markFileAsViewed(input: { path: "x', viewed: true }, ]); expect(mutation).not.toBeNull(); if (mutation === null) return; expect(mutation.query).not.toContain("evil"); - expect(mutation.variables.path0).toBe( + expect(mutation.variables.f0_path).toBe( '") { __typename } evil: markFileAsViewed(input: { path: "x', ); }); @@ -1922,9 +1926,14 @@ describe("pull request stack membership batches", () => { expect(buildPullRequestStackMembershipsGraphQlQuery("acme/web", [0])).toBeNull(); expect(buildPullRequestStackMembershipsGraphQlQuery("acme/web", [1.5])).toBeNull(); expect(buildPullRequestStackMembershipsGraphQlQuery("acme/web", [])).toBeNull(); - expect(buildPullRequestStackMembershipsGraphQlQuery("acme/web", [7, 8])).toContain( - "pullRequest(number: 8)", - ); + expect(buildPullRequestStackMembershipsGraphQlQuery("acme/web", [7, 8])?.variables).toEqual({ + s0_owner: "acme", + s0_name: "web", + s0_number: 7, + s1_owner: "acme", + s1_name: "web", + s1_number: 8, + }); }); }); @@ -1982,10 +1991,10 @@ describe("batched pull request summaries", () => { it("reads stack membership only where the document asked for it", () => { expect( - buildPullRequestSummariesGraphQlQuery([{ repository: "acme/web", number: 7 }], true), + buildPullRequestSummariesGraphQlQuery([{ repository: "acme/web", number: 7 }], true)?.query, ).toContain("stack { number size baseRefName } stackEntry { position }"); expect( - buildPullRequestSummariesGraphQlQuery([{ repository: "acme/web", number: 7 }]), + buildPullRequestSummariesGraphQlQuery([{ repository: "acme/web", number: 7 }])?.query, ).not.toContain("stack {"); const pullRequest = (number: number, stack: Record) => ({ pullRequest: { @@ -2019,16 +2028,22 @@ describe("batched pull request summaries", () => { describe("pull request watch fingerprints", () => { it("asks for every pull request in one aliased read, and refuses an unsafe selector", () => { - const query = buildPullRequestWatchFingerprintsGraphQlQuery([ + const document = buildPullRequestWatchFingerprintsGraphQlQuery([ { repository: "pingdotgg/t3code", number: 7 }, { repository: "pingdotgg/lakebed", number: 8 }, ]); - expect(query).toContain( - 'w0: repository(owner: "pingdotgg", name: "t3code") { pullRequest(number: 7)', - ); - expect(query).toContain( - 'w1: repository(owner: "pingdotgg", name: "lakebed") { pullRequest(number: 8)', + expect(document?.query).toContain( + "w0: repository(owner: $w0_owner, name: $w0_name) { pullRequest(number: $w0_number)", ); + expect(document?.query).toContain("w1: repository(owner: $w1_owner, name: $w1_name)"); + expect(document?.variables).toEqual({ + w0_owner: "pingdotgg", + w0_name: "t3code", + w0_number: 7, + w1_owner: "pingdotgg", + w1_name: "lakebed", + w1_number: 8, + }); expect( buildPullRequestWatchFingerprintsGraphQlQuery([{ repository: 'evil") { x', number: 1 }]), ).toBeNull(); diff --git a/apps/server/src/pullRequest/gitHubPullRequestJson.ts b/apps/server/src/pullRequest/gitHubPullRequestJson.ts index ce89cf6fe44a27842b4ec0263dae1baf7c1e8b8a..028231c513f24219c7e86447e00a7b851e10be5d 100644 --- a/apps/server/src/pullRequest/gitHubPullRequestJson.ts +++ b/apps/server/src/pullRequest/gitHubPullRequestJson.ts @@ -35,6 +35,7 @@ import type { import { quoteGitPatchPath } from "@t3tools/shared/gitPatchPath"; import { decodeJsonResult } from "@t3tools/shared/schemaJson"; +import { aliasedGraphQlDocument, type GraphQlDocument } from "../sourceControl/githubGraphQl.ts"; import { dedupeChecks } from "./pullRequestChecks.ts"; /** @@ -1917,53 +1918,81 @@ function toStackMembership( : undefined; } -/** What a repository selector may hold before it is written into a GraphQL document unquoted. */ +/** What GitHub allows in an owner or repository name. */ const REPOSITORY_PART = /^[A-Za-z0-9._-]+$/; +/** + * The owner and name of an `owner/name` selector, or null for one GitHub cannot name. A batch is + * one document, and a single unanswerable alias fails all of it, so these are refused up front. + */ +function repositoryParts( + repository: string, +): { readonly owner: string; readonly name: string } | null { + const [owner, name, ...rest] = repository.trim().split("/"); + if (rest.length > 0 || owner === undefined || name === undefined) return null; + return REPOSITORY_PART.test(owner) && REPOSITORY_PART.test(name) ? { owner, name } : null; +} + +const isPullRequestNumber = (number: number) => Number.isSafeInteger(number) && number > 0; + +/** One aliased `repository { pullRequest { selection } }` per change request, on one host. */ +function aliasedPullRequestsDocument( + name: string, + alias: string, + changeRequests: ReadonlyArray<{ readonly repository: string; readonly number: number }>, + selection: string, +): GraphQlDocument | null { + const addressed = changeRequests.map((changeRequest) => ({ + parts: repositoryParts(changeRequest.repository), + number: changeRequest.number, + })); + if (addressed.some(({ parts, number }) => parts === null || !isPullRequestNumber(number))) { + return null; + } + return aliasedGraphQlDocument({ + operation: "query", + name, + alias, + items: addressed, + variables: ({ parts, number }) => ({ + owner: ["String!", parts!.owner], + name: ["String!", parts!.name], + number: ["Int!", number], + }), + field: ({ owner, name, number }) => + `repository(owner: ${owner}, name: ${name}) { pullRequest(number: ${number}) { ${selection} } }`, + }); +} + /** * The line counts for rows a listing already handed over, as one aliased lookup each. * * Aliases rather than `nodes(ids:)` because the caller asks in the terms the page holds — a - * repository and a number — and never sees a node id. Owner, name and number are written into - * the document, so each is checked against what GitHub can actually name first: null for anything - * else, which the caller reports rather than sends. - * - * Null too for an empty request, since a GraphQL document with no selection is not a document. + * repository and a number — and never sees a node id. Null for a selector GitHub cannot name, + * which the caller reports rather than sends, and for an empty request. */ export function buildPullRequestStatsGraphQlQuery( changeRequests: ReadonlyArray<{ readonly repository: string; readonly number: number }>, -): string | null { - if (changeRequests.length === 0) return null; - const selections: string[] = []; - for (const [index, changeRequest] of changeRequests.entries()) { - const [owner, name, ...rest] = changeRequest.repository.trim().split("/"); - if (rest.length > 0 || owner === undefined || name === undefined) return null; - if (!REPOSITORY_PART.test(owner) || !REPOSITORY_PART.test(name)) return null; - if (!Number.isSafeInteger(changeRequest.number) || changeRequest.number <= 0) return null; - selections.push( - ` s${index}: repository(owner: "${owner}", name: "${name}") { pullRequest(number: ${changeRequest.number}) { additions deletions } }`, - ); - } - return `query {\n${selections.join("\n")}\n}`; +): GraphQlDocument | null { + return aliasedPullRequestsDocument( + "PullRequestStats", + "s", + changeRequests, + "additions deletions", + ); } /** Stack membership for the visible rows of a per-repository listing. */ export function buildPullRequestStackMembershipsGraphQlQuery( repository: string, numbers: ReadonlyArray, -): string | null { - if (numbers.length === 0) return null; - const [owner, name, ...rest] = repository.trim().split("/"); - if (rest.length > 0 || owner === undefined || name === undefined) return null; - if (!REPOSITORY_PART.test(owner) || !REPOSITORY_PART.test(name)) return null; - const selections: string[] = []; - for (const [index, number] of numbers.entries()) { - if (!Number.isSafeInteger(number) || number <= 0) return null; - selections.push( - ` s${index}: repository(owner: "${owner}", name: "${name}") { pullRequest(number: ${number}) { stack { number size baseRefName } stackEntry { position } } }`, - ); - } - return `query PullRequestStackMemberships {\n${selections.join("\n")}\n}`; +): GraphQlDocument | null { + return aliasedPullRequestsDocument( + "PullRequestStackMemberships", + "s", + numbers.map((number) => ({ repository, number })), + STACK_MEMBERSHIP_SELECTION, + ); } const decodeStackMemberships = decodeJsonResult(RawStackMembershipsSchema); @@ -2024,26 +2053,17 @@ const PULL_REQUEST_SUMMARY_SELECTION = "latestReviews(first: 20) { nodes { state author { login } } } " + "commits(last: 1) { nodes { commit { statusCheckRollup { state } } } }"; -/** - * Summaries for pull requests anywhere on one host, one aliased lookup each. Checked and written - * into the document the way `buildPullRequestStatsGraphQlQuery` does, so null means "do not send". - */ +/** Summaries for pull requests anywhere on one host, one aliased lookup each; null when unsafe. */ export function buildPullRequestSummariesGraphQlQuery( changeRequests: ReadonlyArray<{ readonly repository: string; readonly number: number }>, includeStacks = false, -): string | null { - if (changeRequests.length === 0) return null; - const selections: string[] = []; - for (const [index, changeRequest] of changeRequests.entries()) { - const [owner, name, ...rest] = changeRequest.repository.trim().split("/"); - if (rest.length > 0 || owner === undefined || name === undefined) return null; - if (!REPOSITORY_PART.test(owner) || !REPOSITORY_PART.test(name)) return null; - if (!Number.isSafeInteger(changeRequest.number) || changeRequest.number <= 0) return null; - selections.push( - ` s${index}: repository(owner: "${owner}", name: "${name}") { pullRequest(number: ${changeRequest.number}) { ${PULL_REQUEST_SUMMARY_SELECTION}${includeStacks ? ` ${STACK_MEMBERSHIP_SELECTION}` : ""} } }`, - ); - } - return `query PullRequestSummaries {\n${selections.join("\n")}\n}`; +): GraphQlDocument | null { + return aliasedPullRequestsDocument( + "PullRequestSummaries", + "s", + changeRequests, + `${PULL_REQUEST_SUMMARY_SELECTION}${includeStacks ? ` ${STACK_MEMBERSHIP_SELECTION}` : ""}`, + ); } /** One pull request's summary, aliased the way the batch is so the same decoder reads it. */ @@ -2170,19 +2190,13 @@ const PULL_REQUEST_WATCH_FINGERPRINT_SELECTION = /** Watch fingerprints for pull requests on one host, one aliased lookup each; null when unsafe. */ export function buildPullRequestWatchFingerprintsGraphQlQuery( changeRequests: ReadonlyArray<{ readonly repository: string; readonly number: number }>, -): string | null { - if (changeRequests.length === 0) return null; - const selections: string[] = []; - for (const [index, changeRequest] of changeRequests.entries()) { - const [owner, name, ...rest] = changeRequest.repository.trim().split("/"); - if (rest.length > 0 || owner === undefined || name === undefined) return null; - if (!REPOSITORY_PART.test(owner) || !REPOSITORY_PART.test(name)) return null; - if (!Number.isSafeInteger(changeRequest.number) || changeRequest.number <= 0) return null; - selections.push( - ` w${index}: repository(owner: "${owner}", name: "${name}") { pullRequest(number: ${changeRequest.number}) { ${PULL_REQUEST_WATCH_FINGERPRINT_SELECTION} } }`, - ); - } - return `query PullRequestWatchFingerprints {\n${selections.join("\n")}\n}`; +): GraphQlDocument | null { + return aliasedPullRequestsDocument( + "PullRequestWatchFingerprints", + "w", + changeRequests, + PULL_REQUEST_WATCH_FINGERPRINT_SELECTION, + ); } /** @@ -3357,25 +3371,20 @@ export function decodePullRequestFilesViewedJson( * request each. GitHub has no bulk form of `markFileAsViewed`/`unmarkFileAsViewed`, which each * take a single path, so the batching is done with aliases; top-level mutation fields run in * write order, so the last word about a path is the one that sticks. - * - * Paths travel as variables rather than interpolated into the document, since a path is data - * and a document is not. */ export function buildSetFilesViewedGraphQlMutation( + pullRequestId: string, files: ReadonlyArray<{ readonly path: string; readonly viewed: boolean }>, -): { readonly query: string; readonly variables: Readonly> } | null { - if (files.length === 0) return null; - const parameters = files.map((_, index) => `$path${index}: String!`).join(", "); - const fields = files - .map( - (file, index) => - ` f${index}: ${file.viewed ? "markFileAsViewed" : "unmarkFileAsViewed"}(input: { pullRequestId: $pullRequestId, path: $path${index} }) { clientMutationId }`, - ) - .join("\n"); - return { - query: `mutation($pullRequestId: ID!, ${parameters}) {\n${fields}\n}`, - variables: Object.fromEntries(files.map((file, index) => [`path${index}`, file.path])), - }; +): GraphQlDocument | null { + return aliasedGraphQlDocument({ + operation: "mutation", + alias: "f", + items: files, + shared: { pullRequestId: ["ID!", pullRequestId] }, + variables: (file) => ({ path: ["String!", file.path] }), + field: ({ path }, file) => + `${file.viewed ? "markFileAsViewed" : "unmarkFileAsViewed"}(input: { pullRequestId: $pullRequestId, path: ${path} }) { clientMutationId }`, + }); } /** One pull request as the stacks API lists it: a number, a head, and whether it is done. */ diff --git a/apps/server/src/pullRequest/githubStackActions.ts b/apps/server/src/pullRequest/githubStackActions.ts index 59d29627b81ba335b350e49329aa1078212afcdf..c2e8fadb751dc4dc3c63aa03f606c6ca33e4e8b9 100644 --- a/apps/server/src/pullRequest/githubStackActions.ts +++ b/apps/server/src/pullRequest/githubStackActions.ts @@ -9,6 +9,7 @@ import * as Result from "effect/Result"; import * as Schema from "effect/Schema"; import * as GitHubApi from "../sourceControl/GitHubApi.ts"; +import { aliasedGraphQlDocument } from "../sourceControl/githubGraphQl.ts"; import { decodePullRequestStacksJson } from "./gitHubPullRequestJson.ts"; import { cascadeRebaseStack } from "./githubStackRebase.ts"; @@ -192,13 +193,18 @@ export const runGitHubStackAction = Effect.fn("runGitHubStackAction")(function* host: input.host, operation: "runGitHubStackAction", allowReserve: true, - variables: { owner, name }, - query: `query($owner:String!,$name:String!){repository(owner:$owner,name:$name){${open - .map( - (layer) => - `pr${layer.number}:pullRequest(number:${layer.number}){headRepository{viewerPermission} maintainerCanModify}`, - ) - .join(" ")}}}`, + // `open` is non-empty here, so there is always a document. + ...aliasedGraphQlDocument({ + operation: "query", + alias: "pr", + key: (layer) => layer.number, + items: open, + shared: { owner: ["String!", owner], name: ["String!", name] }, + variables: (layer) => ({ number: ["Int!", layer.number] }), + field: ({ number }) => + `pullRequest(number: ${number}) { headRepository { viewerPermission } maintainerCanModify }`, + within: (fields) => `repository(owner: $owner, name: $name) {\n${fields}\n}`, + })!, }); const access = yield* decodeBranchAccess(permissions).pipe( Effect.mapError((cause) => new GitHubStackResponseInvalidError({ ...identity, cause })), diff --git a/apps/server/src/sourceControl/githubGraphQl.test.ts b/apps/server/src/sourceControl/githubGraphQl.test.ts new file mode 100644 index 0000000000000000000000000000000000000000..9ab8f93544933c9a86c147d7069b8a10cd8b7409 --- /dev/null +++ b/apps/server/src/sourceControl/githubGraphQl.test.ts @@ -0,0 +1,105 @@ +import { assert, describe, expect, it } from "@effect/vitest"; +import * as Effect from "effect/Effect"; + +import { aliasedGraphQlDocument, readGraphQlPages } from "./githubGraphQl.ts"; + +describe("aliasedGraphQlDocument", () => { + it("keeps every value out of the document, each under its alias's own variable", () => { + const document = aliasedGraphQlDocument({ + operation: "query", + name: "Lookups", + alias: "s", + items: ['acme") { x } #', "web"], + shared: { owner: ["String!", "acme"] }, + variables: (repo) => ({ name: ["String!", repo] }), + field: ({ name }) => `repository(owner: $owner, name: ${name}) { id }`, + }); + assert.isNotNull(document); + expect(document!.query).toBe( + "query Lookups($owner: String!, $s0_name: String!, $s1_name: String!) {\n" + + " s0: repository(owner: $owner, name: $s0_name) { id }\n" + + " s1: repository(owner: $owner, name: $s1_name) { id }\n}", + ); + expect(document!.variables).toEqual({ + owner: "acme", + s0_name: 'acme") { x } #', + s1_name: "web", + }); + }); + + it("aliases by key and nests inside a shared parent when asked", () => { + const document = aliasedGraphQlDocument({ + operation: "query", + alias: "pr", + key: (number) => number, + items: [2, 3], + variables: (number) => ({ number: ["Int!", number] }), + field: ({ number }) => `pullRequest(number: ${number}) { id }`, + within: (fields) => `repository(owner: "a", name: "b") {\n${fields}\n}`, + }); + expect(document?.query).toContain(" pr2: pullRequest(number: $pr2_number) { id }"); + expect(document?.query).toContain(" pr3: pullRequest(number: $pr3_number) { id }"); + expect(document?.variables).toEqual({ pr2_number: 2, pr3_number: 3 }); + }); + + it("asks for nothing when there is nothing to ask", () => { + expect( + aliasedGraphQlDocument({ + operation: "mutation", + alias: "f", + items: [], + variables: () => ({}), + field: () => "x", + }), + ).toBeNull(); + }); +}); + +describe("readGraphQlPages", () => { + const pages = (cursors: ReadonlyArray) => { + const asked: Array = []; + const read = (after: string | null) => + Effect.sync(() => { + asked.push(after); + return { next: cursors[asked.length - 1] ?? null }; + }); + return { asked, read }; + }; + + it.effect("follows the cursor until GitHub has no more", () => + Effect.gen(function* () { + const { asked, read } = pages(["a", "b", null]); + const result = yield* readGraphQlPages(read, { nextCursor: (page) => page.next }); + expect(asked).toEqual([null, "a", "b"]); + expect(result.truncated).toBe(false); + }), + ); + + it.effect("stops as truncated at the page limit, at `until`, or on a repeated cursor", () => + Effect.gen(function* () { + const limited = pages(["a", "b", "c"]); + const atLimit = yield* readGraphQlPages(limited.read, { + nextCursor: (page) => page.next, + maxPages: 2, + }); + expect(limited.asked).toEqual([null, "a"]); + expect(atLimit.truncated).toBe(true); + + const enough = pages(["a", "b", "c"]); + const satisfied = yield* readGraphQlPages(enough.read, { + nextCursor: (page) => page.next, + until: (read) => read.length === 1, + }); + expect(enough.asked).toEqual([null]); + expect(satisfied.truncated).toBe(true); + + const stuck = pages(["a", "a", "a"]); + const repeated = yield* readGraphQlPages(stuck.read, { + nextCursor: (page) => page.next, + from: "start", + }); + expect(stuck.asked).toEqual(["start", "a"]); + expect(repeated.truncated).toBe(true); + }), + ); +}); diff --git a/apps/server/src/sourceControl/githubGraphQl.ts b/apps/server/src/sourceControl/githubGraphQl.ts new file mode 100644 index 0000000000000000000000000000000000000000..0b1e3ca65844c5c8656a153f0c07e71e583cdc95 --- /dev/null +++ b/apps/server/src/sourceControl/githubGraphQl.ts @@ -0,0 +1,100 @@ +import * as Effect from "effect/Effect"; + +/** GraphQL variables by name, each with the type it is declared as and the value it carries. */ +export type GraphQlVariables = Readonly>; + +export interface GraphQlDocument { + readonly query: string; + readonly variables: Readonly>; +} + +/** + * One document asking the same field for many items, each under its own alias: the prefix and + * the item's position (`s0`, `s1`, …), or its `key` when the answer is easier read by one. GitHub + * has no bulk form of most lookups, so aliases are how a batch travels in one request. + * + * Every value travels as a variable renamed per alias (`$s0_owner`), and `field` is handed those + * placeholders rather than the values: nothing an item holds is written into the document. + * `shared` variables are declared once and referenced by their own name; `within` nests the aliased + * fields inside one parent selection, such as a single repository. + * + * Null for no items, since a document with no selection is not a document. + */ +export function aliasedGraphQlDocument(input: { + readonly operation: "query" | "mutation"; + /** The operation's name, which tells one batch from another in logs and recorded traffic. */ + readonly name?: string; + readonly alias: string; + readonly key?: (item: Item, index: number) => number; + readonly items: ReadonlyArray; + readonly variables: (item: Item) => Variables; + readonly field: ( + placeholders: { readonly [Name in keyof Variables]: string }, + item: Item, + ) => string; + readonly shared?: GraphQlVariables; + readonly within?: (fields: string) => string; +}): GraphQlDocument | null { + if (input.items.length === 0) return null; + const declarations: string[] = []; + const variables: Record = {}; + for (const [name, [type, value]] of Object.entries(input.shared ?? {})) { + declarations.push(`$${name}: ${type}`); + variables[name] = value; + } + const fields = input.items.map((item, index) => { + const alias = `${input.alias}${input.key?.(item, index) ?? index}`; + const placeholders: Record = {}; + for (const [name, [type, value]] of Object.entries(input.variables(item))) { + const variable = `${alias}_${name}`; + declarations.push(`$${variable}: ${type}`); + variables[variable] = value; + placeholders[name] = `$${variable}`; + } + return ` ${alias}: ${input.field(placeholders as { readonly [Name in keyof Variables]: string }, item)}`; + }); + const selection = fields.join("\n"); + const parameters = declarations.length === 0 ? "" : `(${declarations.join(", ")})`; + return { + query: `${input.operation}${input.name === undefined ? "" : ` ${input.name}`}${parameters} {\n${input.within?.(selection) ?? selection}\n}`, + variables, + }; +} + +/** + * The pages of a GraphQL connection, read one after another from `from` (the first page by + * default) until GitHub has no more, `until` is satisfied, or `maxPages` have been read. `read` is + * handed the pages so far, for reads that size or check the next page by what has already arrived. + * + * `truncated` says pages remained when reading stopped. A cursor GitHub hands back a second time + * would page forever, so it stops the read as truncated too. + */ +export const readGraphQlPages = ( + read: (after: string | null, pages: ReadonlyArray) => Effect.Effect, + options: { + readonly nextCursor: (page: NoInfer) => string | null; + readonly from?: string | null; + readonly maxPages?: number; + readonly until?: (pages: ReadonlyArray>) => boolean; + }, +): Effect.Effect<{ readonly pages: ReadonlyArray; readonly truncated: boolean }, E, R> => + Effect.gen(function* () { + const pages: Page[] = []; + const seen = new Set(); + let after = options.from ?? null; + while (true) { + const page: Page = yield* read(after, pages); + pages.push(page); + const next = options.nextCursor(page); + if (next === null) return { pages, truncated: false }; + if ( + seen.has(next) || + pages.length >= (options.maxPages ?? Infinity) || + options.until?.(pages) === true + ) { + return { pages, truncated: true }; + } + seen.add(next); + after = next; + } + }); -- 2.54.0