Repository navigation
Use keyset pagination instead of offset pagination #961
Description
Description of the problem, including code/CLI snippet
This package currently does pagination through page and per_page arguments. This can be problematic because some results could be skipped (for example, if some older result gets deleted in meantime).
See more information here:
Expected Behavior
Recently, GitLab (and already available on gitlab.com) supports also id_after and id_before. So this package could support iterating over many results by: ordering by ID and then using id_after to get the next set of results.
Activity
max-wittig commentedon Dec 7, 2019
Sounds interesting. Thanks for raising the issue. But as long as the Projects list API doesn't change the defaults, I wouldn't set it by default. Maybe one could offer an alternative project listing method.
Or what do you think?
mitar commentedon Dec 7, 2019
What do you mean by defaults?
Yes, I think for now it could be opt-in. I would name it
gl.projects.list(use_keyset_pagination=True), which would then prevent one to configure ordering. You could still configureper_page. One should still be able to useid_afterandid_before, which would serve as limits on overall span over which to paginate.max-wittig commentedon Dec 7, 2019
@mitar Thanks for your input. I will take a look soon!
mitar commentedon Dec 7, 2019
Awesome, thanks!
max-wittig commentedon Jan 23, 2020
I guess we don't need to tackle this anymore, since GitLab is doing it now on the server side.
We just need to add an option to the GitLab object now: https://docs.gitlab.com/ee/api/#keyset-based-pagination
mitar commentedon Jan 23, 2020
So this just makes "next page link" use
id_afterinstead of pagination? So we do not have to modify it ourselves? That is nice.My main motivation behind this issue was that the package should promote keyset pagination a bit more (maybe through an example how to use it), but also that the default pagination when iterating over all results (
all=True) should be keyset pagination. Maybe also that if you setuse_keyset_pagination=True, if you use incompatibleorder_byit should raise an exception.max-wittig commentedon Jan 26, 2020
@mitar Seems like GitLab already raises an exception itself, I've noticed. So I will just add the feature to add it globally and that's it. But we can't make it the default anytime soon, we would need to change the default sort-order
created_atand I think we shouldn't change this if GitLab doesn'tmitar commentedon Jan 26, 2020
But when fetching all results, it does not matter in which order we fetch? So we should use keyset pagination to assure that we fetch really all results (other modes allow results to be missed).
max-wittig commentedon Jan 26, 2020
We're just a wrapper around the Gitlab API. I don't think we should pro-actively change ordering to id ordering now, but it can be done pretty easily, with 2 arguments to the global Gitlab object.
mitar commentedon Jan 26, 2020
Sorry to nitpick here, feel free to start ignoring me. :-) But, see here:
The
allparameter is not something GitLab supports, no? This is addition of this wrapper. So, I am saying that when somebody makesall=True, internally, we should use keyset pagination. That is the only way to assure that "get all the items when using listing methods". Without it,all=Truecan still missing some items.max-wittig commentedon Jan 26, 2020
I'm not gonna ignore you 😄
You're raising valid points.
I think the quote just means that it only returns 20 items per page by default. But we could make all=true use keyset generation by default, that's true.
mitar commentedon Jan 26, 2020
That is all I am asking. Plus some documentation pointing out the importance of keyset pagination, so that people can make informed decision what arguments to use.
max-wittig commentedon Jan 27, 2020
@mitar I've just implemented the feature locally, but I'm worried that this is gonna break scripts for some reason. People could just use
If we start to modify all for pagination, then could also start to change the pagination per page for optimal performance. But I'm not sure that we should do things like that as a wrapper.
What do you think?
max-wittig commentedon Jan 27, 2020
@mitar I've also prepared a draft, so feel free to take a look: #1003
mitar commentedon Jan 27, 2020
I think having common patterns exposed easily in the wrapper is sensible. If you worry about backwards compatibility though, then or we add a special argument, or just document this well. I think that given it is just two more arguments, we could just go with documenting this well in various places.
max-wittig commentedon Mar 19, 2020
@mitar I've reverted this change as it was causing issues with other MR listing, as there is no id available then. So we could either have a whitelist of cases as if statement or just leave it out and rely on the user to set
pagination="keyset", order_by="id"?mitar commentedon Apr 14, 2020
Sorry for late reply. Where are no IDs? For which endpoint?
I think it is beneficial if can provide for as many endpoints keyset pagination by default when it is possible. In my view the purpose of such package is to expose best practices to the developer so that developer doe not have to learn about internals and special cases of GitLab API.
So not sure if we should maintain a central list of cases or it should be made in a more extendable way with each handler maybe having a property to tell the
listlogic to use keyset or not by default. In any case, I would do something that it is used if possible. And documentation explained that keyset is used when possible.It would be useful to understand why GitLab does not provide IDs in all cases though?
rkpatel7 commentedon May 11, 2020
I would like to raise awareness of the following notice that may affect the discussion in this issue:
I don't have an opinion on the implementation details, but I think it would be valuable to enable keyset pagination through this library given the changes coming in Gitlab 13.0.
mitar commentedon Oct 25, 2021
You are right that some GitLab API endpoints do not support keyset pagination and that was a surprise for me. Example is list merge requests. I opened a GitLab issue for this.
There is also this epic where they track adding keyset to various APIs.