Merge pull request #1440 from fabianehlert/feature/github-api-pagination

GitHub: use files API with pagination

Author: orta

CHANGELOG.md

@@ -17,6 +17,7 @@
 <!-- Your comment below this -->
 
 - 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]
 
 <!-- Your comment above this -->
 

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<GitHubFile[]> => {
     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<string> => {
+    // 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<any> => {

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<any> => {
   return Promise.resolve({
@@ -25,19 +26,6 @@ const fetchErrorJSON = (api: any, params: any): Promise<any> => {
   })
 }
 
-const fetchText = (api: any, params: any): Promise<any> => {
-  return Promise.resolve({
-    ok: true,
-    text: () =>
-      Promise.resolve(
-        JSON.stringify({
-          api,
-          ...params,
-        })
-      ),
-  })
-}
-
 const fetch = (api: any, params: any): Promise<any> => {
   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<GitHubFile[]>(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 () => {