Skip to content

feat: Paginate compareCommits and compareCommitsWithBasehead #678

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
Draft

feat: Paginate compareCommits and compareCommitsWithBasehead #678

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -183,6 +183,7 @@ Most of GitHub's paginating REST API endpoints return an array, but there are a
- [List check suites for a specific ref](https://developer.github.com/v3/checks/suites/#response-1) (key: `check_suites`)
- [List repositories](https://developer.github.com/v3/apps/installations/#list-repositories) for an installation (key: `repositories`)
- [List installations for a user](https://developer.github.com/v3/apps/installations/#response-1) (key `installations`)
- [Compare commits](https://docs.github.com/en/rest/commits/commits?apiVersion=2022-11-28#compare-two-commits) (key `commits`)

`octokit.paginate()` is working around these inconsistencies so you don't have to worry about it.

Expand Down
5 changes: 0 additions & 5 deletions scripts/update-endpoints/typescript.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,11 +28,6 @@ const ENDPOINTS_WITH_PER_PAGE_ATTRIBUTE_THAT_BEHAVE_DIFFERENTLY = [
// Only the `files` key inside the commit is paginated. The rest is duplicated across
// all pages. Handling this case properly requires custom code.
{ scope: "repos", id: "get-commit" },
// The [docs](https://docs.github.com/en/rest/commits/commits#compare-two-commits) make
// these ones sound like a special case too - they must be because they support pagination
// but doesn't return an array.
{ scope: "repos", id: "compare-commits" },
{ scope: "repos", id: "compare-commits-with-basehead" },
];

const hasMatchingEndpoint = (list, id, scope) =>
Expand Down
22 changes: 22 additions & 0 deletions src/generated/paginating-endpoints.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1195,6 +1195,26 @@ export interface PaginatingEndpoints {
response: Endpoints["GET /repos/{owner}/{repo}/commits/{ref}/statuses"]["response"];
};

/**
* @see https://docs.github.com/rest/commits/commits#compare-two-commits
*/
"GET /repos/{owner}/{repo}/compare/{basehead}": {
parameters: Endpoints["GET /repos/{owner}/{repo}/compare/{basehead}"]["parameters"];
response: Endpoints["GET /repos/{owner}/{repo}/compare/{basehead}"]["response"] & {
data: Endpoints["GET /repos/{owner}/{repo}/compare/{basehead}"]["response"]["data"]["commits"];
};
};

/**
* @see https://docs.github.com/rest/reference/repos#compare-two-commits
*/
"GET /repos/{owner}/{repo}/compare/{base}...{head}": {
parameters: Endpoints["GET /repos/{owner}/{repo}/compare/{base}...{head}"]["parameters"];
response: Endpoints["GET /repos/{owner}/{repo}/compare/{base}...{head}"]["response"] & {
data: Endpoints["GET /repos/{owner}/{repo}/compare/{base}...{head}"]["response"]["data"]["commits"];
};
};

/**
* @see https://docs.github.com/rest/repos/repos#list-repository-contributors
*/
Expand Down Expand Up @@ -2325,6 +2345,8 @@ export const paginatingEndpoints: (keyof PaginatingEndpoints)[] = [
"GET /repos/{owner}/{repo}/commits/{ref}/check-suites",
"GET /repos/{owner}/{repo}/commits/{ref}/status",
"GET /repos/{owner}/{repo}/commits/{ref}/statuses",
"GET /repos/{owner}/{repo}/compare/{basehead}",
"GET /repos/{owner}/{repo}/compare/{base}...{head}",
"GET /repos/{owner}/{repo}/contributors",
"GET /repos/{owner}/{repo}/dependabot/alerts",
"GET /repos/{owner}/{repo}/dependabot/secrets",
Expand Down
11 changes: 11 additions & 0 deletions src/iterator.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,17 @@ export function iterator(
/<([^<>]+)>;\s*rel="next"/,
) || [])[1];

if (!url && "total_commits" in normalizedResponse.data) {
const parsedUrl = new URL(normalizedResponse.url);
const params = parsedUrl.searchParams;
const page = parseInt(params.get('page') || '1', 10)
const per_page = parseInt(params.get('per_page') || '250', 10);
if (page * per_page < normalizedResponse.data.total_commits) {
params.set('page', String(page + 1));
url = parsedUrl.toString();
}
}

Comment on lines +43 to +53
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you explain why this is necessary?
Don't the existing methods do this already?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As far as I can tell, the other methods rely on response.headers.link, which is not present in responses to compareCommits or compareCommitsWithBasehead

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That seems like an oversight on GitHub's part, their page on pagination says to use the link header
https://docs.github.com/en/rest/using-the-rest-api/using-pagination-in-the-rest-api?apiVersion=2022-11-28#using-link-headers

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you add a test that covers this?

return { value: normalizedResponse };
} catch (error: any) {
// `GET /repos/{owner}/{repo}/commits` throws a `409 Conflict` error for empty repositories
Expand Down
5 changes: 4 additions & 1 deletion src/normalize-paginated-list-response.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,17 +28,19 @@ export function normalizePaginatedListResponse(
};
}
const responseNeedsNormalization =
"total_count" in response.data && !("url" in response.data);
("total_count" in response.data && !("url" in response.data)) || "total_commits" in response.data;
if (!responseNeedsNormalization) return response;

// keep the additional properties intact as there is currently no other way
// to retrieve the same information.
const incompleteResults = response.data.incomplete_results;
const repositorySelection = response.data.repository_selection;
const totalCount = response.data.total_count;
const totalCommits = response.data.total_commits;
delete response.data.incomplete_results;
delete response.data.repository_selection;
delete response.data.total_count;
delete response.data.total_commits;

const namespaceKey = Object.keys(response.data)[0];
const data = response.data[namespaceKey];
Expand All @@ -53,5 +55,6 @@ export function normalizePaginatedListResponse(
}

response.data.total_count = totalCount;
response.data.total_commits = totalCommits;
return response;
}
1 change: 1 addition & 0 deletions src/types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ import type { PaginatingEndpoints } from "./generated/paginating-endpoints.js";
type PaginationMetadataKeys =
| "repository_selection"
| "total_count"
| "total_commits"
| "incomplete_results";

// https://stackoverflow.com/a/58980331/206879
Expand Down
57 changes: 57 additions & 0 deletions test/paginate.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -998,6 +998,63 @@ describe("pagination", () => {
]);
});
});

it(".paginate() with results namespace (GET /repos/{owner}/{repo}/compare/{basehead})", () => {
const result1 = {
total_commits: 2,
commits: [
{
sha: "f3b573e4d60a079d154018d2e2d04aff4d26fc41",
},
],
};
const result2 = {
total_commits: 2,
commits: [
{
sha: "a740e83052aea45a4cbcdf2954a3a9e47b5d530d",
},
],
};

const mock = fetchMock
.createInstance()
.get(
'https://api.github.com/repos/octocat/hello-world/compare/1.0.0...1.0.1?per_page=1',
{
body: result1,
},
)
.get(
'https://api.github.com/repos/octocat/hello-world/compare/1.0.0...1.0.1?per_page=1&page=2',
{
body: result2,
},
);

const octokit = new TestOctokit({
request: {
fetch: mock.fetchHandler,
},
});

return octokit
.paginate({
method: "GET",
url: "/repos/{owner}/{repo}/compare/{basehead}",
owner: "octocat",
repo: "hello-world",
basehead: "1.0.0...1.0.1",
per_page: 1,
})
.then((results) => {
expect(results).toEqual([
...result1.commits,
...result2.commits,
]);
});
});

it(".paginate() with results namespace (GET /repos/{owner}/{repo}/actions/runs)", () => {
const result1 = {
total_count: 2,
Expand Down
Loading