Skip to content
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.

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

[BUG]: Pagination not working with compareCommitsWithBasehead() #647

Open
1 task done
LykinsN opened this issue Jan 8, 2025 · 7 comments
Open
1 task done

[BUG]: Pagination not working with compareCommitsWithBasehead() #647

LykinsN opened this issue Jan 8, 2025 · 7 comments
Labels
Type: Bug Something isn't working as documented

Comments

@LykinsN
Copy link

LykinsN commented Jan 8, 2025

What happened?

Our team is using the @octokit/rest library to handle several different calls to our private GHE platform and we are using the paginate wrapper to handle larger datasets. This functionality is working on several other API calls (i.e. listPullRequestsAssociatedWithCommit, listBranches, etc) but is not working correctly with the compareCommitsWithBasehead call. For example, if I reference two branches in the following snippet:

import { Octokit } from '@octokit/rest';
...
const octokit = new Octokit({ auth: githubToken, baseUrl: githubBaseUrl });
...
const data = await octokit.paginate(
   octokit.rest.repos.compareCommitsWithBasehead,
  {
    owner: githubOwner,
    repo: githubRepo,
    basehead: `${branchOne}...${branchTwo}`,
  },
);

If this results in more than 250 records, I'd expect to get the full set back, i.e. 500. But instead, the call only seems to return a single page, and only returns the maximum allowed number from the compareCommitsWithBasehead API call. I've been able to write a custom pagination handler to do the work for me in this case, but I'd expect the paginate wrapper should handle it as well.

Versions

@octokit/rest: v20.1.0, Node: v18.20.4

Relevant log output

No response

Code of Conduct

  • I agree to follow this project's Code of Conduct
@LykinsN LykinsN added Status: Triage This is being looked at and prioritized Type: Bug Something isn't working as documented labels Jan 8, 2025
Copy link
Contributor

github-actions bot commented Jan 8, 2025

👋 Hi! Thank you for this contribution! Just to let you know, our GitHub SDK team does a round of issue and PR reviews twice a week, every Monday and Friday! We have a process in place for prioritizing and responding to your input. Because you are a part of this community please feel free to comment, add to, or pick up any issues/PRs that are labeled with Status: Up for grabs. You & others like you are the reason all of this works! So thank you & happy coding! 🚀

@wolfy1339
Copy link
Member

I recommend upgrading to @octokit/rest v21, do note that it is ESM only now.

Can you try again with that version?

@LykinsN
Copy link
Author

LykinsN commented Jan 8, 2025

I tried reverting back to my original code and upgraded @octokit/rest to 21.0.2, but I'm seeing the same behavior, unfortunately. The paginate response is still limiting the dataset to 250 items.

@wolfy1339 wolfy1339 removed the Status: Triage This is being looked at and prioritized label Jan 8, 2025
@wolfy1339
Copy link
Member

can you share your custom pagination code? it could help us debug the issue further

@LykinsN
Copy link
Author

LykinsN commented Jan 8, 2025

Sure thing, I based it off a sample I found elsewhere in the GitHub docs, where an example was provided. I just tuned it a bit to match my use-case. I'm not sure it's relevant but we're running in Typescript rather than just native Javascript. I'll include the code sections that are relevant to the pagination process.

import { Octokit as OctokitBasePackage } from 'octokit';

const octokitBase = new OctokitBasePackage({ auth: githubToken, baseUrl: githubBaseUrl });

export const parseData = (data) => {
  // If the data is an array, return that
  if (Array.isArray(data)) {
    return data;
  }

  // Some endpoints respond with 204 No Content instead of empty array
  //   when there is no data. In that case, return an empty array.
  if (!data) {
    return [];
  }

  // Pull out the array of items
  const namespaceKey = Object.keys(data)[0];
  return data[namespaceKey];
};

export const getDiffedPullRequests = async (
  octokitBase: OctokitBasePackage,
  githubClient: Octokit,
  githubOwner,
  githubRepo,
  releaseBranch: string,
  previousBranch: string,
) => {
  const nextPattern = /(?<=<)([\S]*)(?=>; rel="Next")/i;
  let pagesRemaining = true;
  let data = [];
  const pullRequestList = [];

  let url = 'GET /repos/{owner}/{repo}/compare/{basehead}';

  while (pagesRemaining) {
    const response = await octokitBase.request(url, {
      owner: githubOwner,
      repo: githubRepo,
      basehead: `${previousBranch}...${releaseBranch}`,
      per_page: 100,
      headers: {
        'X-GitHub-Api-Version': '2022-11-28',
      },
    });

    const parsedData = parseData(response.data.commits);

    data = [...data, ...(Array.isArray(parsedData) ? parsedData : [parsedData])];

    const linkHeader = response.headers.link;

    pagesRemaining = linkHeader && linkHeader.includes('rel="next"');

    if (pagesRemaining) {
      [url] = linkHeader.match(nextPattern);
    }
  }
  return data;
};

const data = await getDiffedPullRequests(octokitBase, githubClient, githubOwner, githubRepo, releaseBranch, previousBranch);

@wolfy1339 wolfy1339 transferred this issue from octokit/octokit.js Jan 9, 2025

This comment was marked as spam.

@wolfy1339
Copy link
Member

Looking through documentation, and the code, it seems that it is a special case.

// 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" },

It is explicitly filtered out from the types.

const responseNeedsNormalization =
"total_count" in response.data && !("url" in response.data);

As seen there, the pagination plugin doesn't currently handle other special cases. In this case there is no total_count but total_commits

@wolfy1339 wolfy1339 changed the title [BUG]: Octokit rest library - paginate wrapper not working with compareCommitsWithBasehead API call [BUG]: Pagination not working with compareCommitsWithBasehead() Jan 9, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Bug Something isn't working as documented
Projects
Status: 🆕 Triage
Development

No branches or pull requests

2 participants