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

Add keyset pagination options #1827

Merged
merged 2 commits into from Nov 13, 2023
Merged

Add keyset pagination options #1827

merged 2 commits into from Nov 13, 2023

Conversation

pwlandoll
Copy link
Contributor

This is one possible implementation of keyset-based pagination, as suggested in #815.

The basic flow of offset-based pagination is that the client requests the first page, and the server returns information about the page in headers. The flow for keyset-based pagination differs in that the information for building a request for the next page is not returned in pieces, but in a pre-formatted URL in the Link header. The documentation recommends using that link to make the request, which doesn't seem like it will work cleanly with the existing code here. In particular, the link includes a number of query parameters that differ among endpoints that support keyset-based pagination.

So, the changes here provide a way to directly use all of the query parameters returned in the header when building the next request. ListOptions now includes fields that are shared among all keyset-based paginated endpoints. Other required parameters for paginated endpoints can be added by means of a RequestOptionFunc, for which there is a builder function included.

An alternative would be to have ListOptions enumerate each possible query parameter that could be included in a "next Link" header, and return them in the Response struct in the same way as existing header values. This would mean that the end-user code for each endpoint using this pagination might look different, i.e. there would be no single way to do all keyset-based paginated endpoints. But, it would make the code less opaque, and avoid using a "magic black box function" to add RequestOptionFuncs.

This PR is mostly a suggestion/WIP to get the conversation moving about how this package might support keyset-based paginated endpoints, so please let me know how it looks!

gitlab.go Outdated
// The "next" link in the Link header includes query parameters for the
// next paginated request, but the relevant parameters vary among API
// endpoints.
NextLinkParameters url.Values
Copy link

@stanhu stanhu Nov 2, 2023

Choose a reason for hiding this comment

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

Minor question: I wonder if this field should be omitted, so that BuildKeysetPaginatedRequestOptionFunc can just take in the NextLink and lazily parse it. That might clean up the interface a little so that clients don't need to think about how to distinguish NextLink and NextLinkParameters.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think that's a good idea. It seems like the goal of the library right now is more "provide a clean interface without being too opinionated" rather than "provide as little as possible" or "all batteries included." I'll see if I can streamline that a little better.

@stanhu
Copy link

stanhu commented Nov 2, 2023

Thanks for doing this!

This looks like a pretty good approach to me. I just had one minor point for consideration.

@stanhu
Copy link

stanhu commented Nov 2, 2023

@svanharmelen Would you mind looking at this? We noticed that on GitLab.com there are clients such as gitlab-ci-pipelines-exporter that are iterating through a large number of CI jobs with offset pagination, when keyset pagination should be used.

@pwlandoll pwlandoll marked this pull request as ready for review November 7, 2023 20:05
@pwlandoll pwlandoll changed the title Draft: add keyset pagination options Add keyset pagination options Nov 8, 2023
@stanhu
Copy link

stanhu commented Nov 9, 2023

Thanks, this looks good to me!

@svanharmelen Could you review/merge?

@svanharmelen
Copy link
Member

I'll have a look at this one tomorrow or Sunday... Looks like something we can get merged 👍🏻

Copy link
Member

@svanharmelen svanharmelen left a comment

Choose a reason for hiding this comment

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

I'll take it... Thanks @pwlandoll 👍🏻

@svanharmelen svanharmelen merged commit 5a3d963 into xanzy:main Nov 13, 2023
3 checks passed
stanhu added a commit to stanhu/gitlab-ci-pipelines-exporter that referenced this pull request Nov 13, 2023
With xanzy/go-gitlab#1827, go-gitlab
now supports keyset pagination, which is much more efficient
for iterating through many pages of data. This commit
switches the GitLab CI jobs API to use keyset pagination.
stanhu added a commit to stanhu/gitlab-ci-pipelines-exporter that referenced this pull request Nov 13, 2023
With xanzy/go-gitlab#1827, go-gitlab now
supports keyset pagination, which is much more efficient for iterating
through many pages of data.  This commit switches the GitLab CI
`/api/v4/projects/:id/jobs` API to use keyset pagination.

The other pipeline API endpoints need keyset pagination support:
https://gitlab.com/gitlab-org/gitlab/-/issues/431632
@pwlandoll
Copy link
Contributor Author

Great, thanks! I was able to pull the latest version and run code like the included example.

mvisonneau pushed a commit to mvisonneau/gitlab-ci-pipelines-exporter that referenced this pull request Nov 29, 2023
* build(deps): bump github.com/xanzy/go-gitlab from 0.92.3 to 0.94.0

Bumps [github.com/xanzy/go-gitlab](https://github.com/xanzy/go-gitlab) from 0.92.3 to 0.94.0.
- [Changelog](https://github.com/xanzy/go-gitlab/blob/main/releases_test.go)
- [Commits](xanzy/go-gitlab@v0.92.3...v0.94.0)

---
updated-dependencies:
- dependency-name: github.com/xanzy/go-gitlab
  dependency-type: direct:production
  update-type: version-update:semver-minor
...

Signed-off-by: dependabot[bot] <support@github.com>

* feat: use keyset pagination for retrieving project CI jobs

With xanzy/go-gitlab#1827, go-gitlab now
supports keyset pagination, which is much more efficient for iterating
through many pages of data.  This commit switches the GitLab CI
`/api/v4/projects/:id/jobs` API to use keyset pagination.

The other pipeline API endpoints need keyset pagination support:
https://gitlab.com/gitlab-org/gitlab/-/issues/431632

---------

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants