From bf158b8075435c2758fa42b01850771a93e0223d Mon Sep 17 00:00:00 2001 From: Fabian Ehlert Date: Sat, 27 Apr 2024 00:14:19 +0200 Subject: [PATCH] GitHub: use files API with pagination --- CHANGELOG.md | 1 + source/platforms/github/GitHubAPI.ts | 63 +++++++++++++++++-- .../github/_tests/_github_api.test.ts | 57 ++++++++++------- 3 files changed, 93 insertions(+), 28 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 0481cfe86..2f5c8dea8 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -17,6 +17,7 @@ - Adds a log when you run on GitHub Actions without being a pull_request - [@orta] +- GitHub: Move to 'List pull request files' API with pagination support [@fabianehlert] diff --git a/source/platforms/github/GitHubAPI.ts b/source/platforms/github/GitHubAPI.ts index 0c8ab70d2..c02ff29f6 100644 --- a/source/platforms/github/GitHubAPI.ts +++ b/source/platforms/github/GitHubAPI.ts @@ -17,6 +17,12 @@ export type APIToken = string const limit = pLimit(25) +// Structure of files returned by the 'List pull request files' API +export interface GitHubFile { + filename: string + patch: string +} + // Note that there are parts of this class which don't seem to be // used by Danger, they are exposed for Peril support. @@ -329,13 +335,60 @@ export class GitHubAPI { }) } - getPullRequestDiff = async () => { + getPullRequestFiles = async (page: number = 1): Promise => { const repo = this.repoMetadata.repoSlug const prID = this.repoMetadata.pullRequestID - const res = await this.get(`repos/${repo}/pulls/${prID}`, { - Accept: "application/vnd.github.v3.diff", - }) - return res.ok ? res.text() : "" + const perPage = 100 + const url = `repos/${repo}/pulls/${prID}/files?page=${page}&per_page=${perPage}` + + try { + const response = await this.get(url, { + Accept: "application/vnd.github.v3.diff", + }) + const data = await response.json() + + if (!response.ok) { + throw new Error(`GitHub 'List pull request files' API returned an error: ${data.message}`) + } + + // Check for pagination using the Link header + const linkHeader = response.headers.get("Link") + const hasNextPage = linkHeader && linkHeader.includes('rel="next"') + + // If there's a next page, recursively fetch it and concatenate the results + if (hasNextPage) { + const nextPageNumber = page + 1 + const nextPageFiles = await this.getPullRequestFiles(nextPageNumber) + return [...data, ...nextPageFiles] + } + + return data as GitHubFile[] + } catch (error) { + console.error("Failed to fetch GitHub pull request files:", error) + throw error + } + } + + getPullRequestDiff = async (): Promise => { + // This is a hack to get the file patch into a format that parse-diff accepts + // as the GitHub API for listing pull request files is missing file names in the patch. + function prefixedPatch(file: GitHubFile): string { + return ` +diff --git a/${file.filename} b/${file.filename} +--- a/${file.filename} ++++ b/${file.filename} +${file.patch} +` + } + + try { + const files = await this.getPullRequestFiles() + const diff = files.map(prefixedPatch).join("\n") + return diff + } catch (error) { + console.error("Failed to fetch pull request diff:", error) + return "" + } } getFileContents = async (path: string, repoSlug: string, ref: string): Promise => { diff --git a/source/platforms/github/_tests/_github_api.test.ts b/source/platforms/github/_tests/_github_api.test.ts index 6c3782705..f9eb74a4a 100644 --- a/source/platforms/github/_tests/_github_api.test.ts +++ b/source/platforms/github/_tests/_github_api.test.ts @@ -2,6 +2,7 @@ import { FakeCI } from "../../../ci_source/providers/Fake" import { GitHubAPI } from "../GitHubAPI" import { requestWithFixturedJSON } from "../../_tests/_github.test" import { GitHubUser } from "../../../dsl/GitHubDSL" +import { GitHubFile } from "../GitHubAPI" const fetchJSON = (api: any, params: any): Promise => { return Promise.resolve({ @@ -25,19 +26,6 @@ const fetchErrorJSON = (api: any, params: any): Promise => { }) } -const fetchText = (api: any, params: any): Promise => { - return Promise.resolve({ - ok: true, - text: () => - Promise.resolve( - JSON.stringify({ - api, - ...params, - }) - ), - }) -} - const fetch = (api: any, params: any): Promise => { return Promise.resolve({ api, @@ -86,17 +74,40 @@ describe("API testing", () => { }) it("getPullRequestDiff", async () => { - api.getPullRequestInfo = await requestWithFixturedJSON("github_pr.json") - api.fetch = fetchText - let text = await api.getPullRequestDiff() - expect(JSON.parse(text)).toMatchObject({ - api: "https://api.github.com/repos/artsy/emission/pulls/1", - headers: { - Authorization: "token ABCDE", - Accept: "application/vnd.github.v3.diff", - "Content-Type": "application/json", - }, + api.fetch = jest.fn().mockReturnValue({ + ok: true, + headers: new Headers({}), + json: jest + .fn() + .mockImplementation(() => + Promise.resolve(JSON.parse('[{"filename": "file.txt", "patch": "+ hello"}]')) + ), }) + + let diff = await api.getPullRequestDiff() + + let expected = ` +diff --git a/file.txt b/file.txt +--- a/file.txt ++++ b/file.txt ++ hello +` + + expect(diff).toEqual(expected) + + expect(api.fetch).toHaveBeenCalledWith( + "https://api.github.com/repos/artsy/emission/pulls/1/files?page=1&per_page=100", + { + body: null, + headers: { + Accept: "application/vnd.github.v3.diff", + Authorization: "token ABCDE", + "Content-Type": "application/json", + }, + method: "GET", + }, + undefined + ) }) it("getDangerCommentIDs ignores comments not marked as generated", async () => {