ApiOptions and PageCount

[patricknolan]

trafficstars

Hi,

I'm just after some clarification regarding the ApiOptions PageCount.

I have a Repository which contains 12 labels. Could you explain why "call 1" would fail (just seems to hangs) and yet "call 2" would succeed and return back 12 labels as expected?

It's a little confusing what PageCount is meant to do when calling GetAllForRepository as it returns a list of Labels and not a list of pages containing labels.

call 1 var result = await this.Client.Issue.Labels.GetAllForRepository(repositoryId, new ApiOptions() { PageCount = 2, PageSize = 11, StartPage = 1 });

call 2 var result = await this.Client.Issue.Labels.GetAllForRepository(repositoryId, new ApiOptions() { PageCount = 2, PageSize = 12, StartPage = 1 });

[M-Zuber]

Do the examples here help anything? https://github.com/octokit/octokit.net/blob/master/docs/extensibility.md#pagination

It does not really explain why the first call hangs though

[patricknolan]

Hi @M-Zuber I had read through that doco but I don't understand why call 1 hangs.

Also, the doco isn't overly clear how PageCount is handled. The return value for GetAllForRepository is List of Labels but it's not paged. So does octokit retrieve x number of pages (where x = PageCount ) via the GitHub API and then merge these pages into a single list of Labels which is returned?

In most APIs PageCount refers to the number of pages that is returned. Usually there is a page object which contains a number of values (in this case it would be Labels).

[M-Zuber]

yeah, the way Octokit works is that the actually returned result is concatenated into a single list of results. I do not see anything in the git history that discuss why though.

I started writing a test for this case, but the second call hangs for me as well. Is the repository open source?

[ryangribble]

The GitHub API itself only supports start_page and page_size settings for pagination, but the way we implemented pagination support in Octokit.net is that we also have a PageCount property where the library will fetch that many pages for you, and yes the results are always concatenated into a single list. Also note if you provide no ApiOptions then the library will fetch ALL pages for you.

The reason for the results being a list and not actually exposing pagination in the responses is due to a combination of factors such as "history", trying to remain backwards compatible (pagination was implemented after the library was well established) and trying to be idiomatic to typical usage/expectations of .NET/c# which was (at the time) deemed to be returning List types and having consumers use Linq to manipulate the lists. Eg if you wanted the first 100 then use .Take(100) rather than exposing "pages" to the consumer.

Personally Im pretty happy about not having to deal with "pages" of results if you dont want to - if you do want to then you can use PageCount=1 and loop through results... but if you don't want to you can do say PageSize=100, PageCount=5 and know you will get up to 500 results without any mucking around with loops etc.

That said, hindsight is wonderful and I do think our implementation could have been better if it was some sort of wrapped response class as that would have allowed us to expose things that are in response headers, such as the first and last page numbers... eg something like OctokitResponse<ReadOnlyList<Repository>>

But in terms of the actual problem raised in this issue, it totally SHOULD work in that case, so we need to look at how/why that call is hanging!

[shiftkey]

I put together this test to see the behaviour for myself:

for (int i = 0; i < 12; i++)
{
    int k = i + 1;
    var newLabel = new NewLabel("test label " + k, "FF0000");
    var label = await _issuesLabelsClient.Create(_context.Repository.Id, newLabel);
}

var firstCall = new ApiOptions
{
    StartPage = 1,
    PageSize = 11,
    PageCount = 2,
};

var firstAPICall = await _issuesLabelsClient.GetAllForRepository(_context.Repository.Id, firstCall);
        
Assert.Equal(11, firstAPICall.Count);

I also put Fiddler in the middle to see for myself what was happening, and this looks like why it is deadlocking - it continually re-requests the same URL:

Looking at that first request, this is the URL that octokit sends:

GET /repositories/179678946/labels?per_page=11&page=1

And we get this Link header back:

Link: <https://api.github.com/repositories/179678946/labels?per_page=11&page=2>; rel="next", <https://api.github.com/repositories/179678946/labels?per_page=11&page=2>; rel="last"

But the client doesn't seem to use this updated repositories/179678946/labels?per_page=11&page=2 - that's the part I'm still trying to investigate here.

[ryangribble]

I spent some time debugging this and what is happening is inside the pagination loop. we are taking the "next" Url from the link header https://api.github.com/repositories/179678946/labels?per_page=11&page=2 and passing it into our extension method Uri.ApplyParameters(nextUrl, parameters) to "re-apply" any relevant parameters from the original request, to our subsequent page request (not sure if this is even necessary as wouldnt the link header from the upstream API echo back any required query parameters in the link header???). So what we end up with is nextUri contains per_page=11&page=2 and the original parameters we are re-applying has per_page=11&page=1 and the result of the ApplyParameters is our desired page=2 gets trumped by the original page=1.

It seems most likely to have been introduced as a side effect by another pagination fix in #1649 (yes, almost 2 years ago!)

This is a bit of a brain twister 🤔 but I think we haven't run into this in all this time is that I believe this will only happen if you specified a StartPage value. If you don't specify StartPage then the original Uri has no page=x parameter that will then clobber the desired page number for subsequent calls. I can only conclude that most people retrieving multiple pages are probably not specifying a start page as they are happy with the default (being page 1) but arent explicitly setting it?

[patricknolan]

Good find @ryangribble. It looks like that is the case. I was also just using the default until recently. Will be nice getting a fix for this one.

[bobbytreed]

I am trying to retrieve all Issue events for a repository. I worked around another bug with Int32 MaxValues, but I get back 40000 results now. I'm suspicious of round numbers when there are 22,383 issues (open and closed). So you are saying that pagination is handled internally without me having to do anything?

I did notice that my first call hit some type of pagination limit xception : System.AggregateException: One or more errors occurred. ---> Octokit.ApiValidationException: In order to keep the API fast for everyone, pagination is limited for this resource. Check the rel=last link relation in the Link response header to see how far back you can traverse. at Octokit.Connection.HandleErrors(IResponse response)

When I specified ApiOption.PageSize of 100 the command completed successfully. Is there anything I need to do to get any additional results, if there are any?

[haacked]

If I had a time machine I'd have written these APIs to return a custom IPaginatedList<T> interface. The current implementation makes it impossible to build a client with pagination without requesting ALL issues on every request. I honestly don't remember what I was thinking making it work the way it does. Most likely I was only focused on the needs of GitHub for Windows and GitHub for Visual Studio.

[github-actions[bot]]

👋 Hey Friends, this issue has been automatically marked as stale because it has no recent activity. It will be closed if no further activity occurs. Please add the Status: Pinned label if you feel that this issue needs to remain open/active. Thank you for your contributions and help in keeping things tidy!