Allow to easily use GitHub Pagination.

[andreaTP]

Is your feature request related to a problem? Please describe the problem.

Trying to use pagination with GitHub API as described here:
https://docs.github.com/en/rest/using-the-rest-api/using-pagination-in-the-rest-api

I have only 2 ways:

• ignore the headers, just request pages once at the time until I get an error or an incomplete page
• implement a RequestHandler to extract headers from the responses, but it's going to be challenging to use this mechanism in a concurrent system

Client library/SDK language

None

Describe the solution you'd like

It would be great to have a generic mechanism(maybe a callback?) to manually extract information from the raw Response.

Additional context

Do you already have a design in mind to solve this use-case?

I can implement something custom in my RequestAdapter but it would be nicer if we rely on some common interfaces.

cc. @maxandersen

[baywet]

Hi @andreaTP
Have you tried to use HeadersInpectionOption to do that?

[sebastienlevert]

This specific scenario is (GitHub paging) is what triggered the HeaderInspectionOptions feature. @andreaTP please confirm this would work for you!

[andreaTP]

Have you tried to use HeadersInpectionOption to do that?

No, I haven't, but an interceptor needs some kind of synchronization in addition to the request and it's inherently not safe in case of concurrency/parallelism.

e.g. I need strong guarantees of single threaded executions to be able to use it:

- create a(isolated?) client with the interceptor
- make a request
- before anything else happens I should extract the header etc. etc.

HeaderInspectionOptions lives in the dotnet-http adapter repo, so it's a dotnet specific implementation, making it impossible to re-use. We can probably do something similar with Java using an Interceptor but I would need to re-implement the feature for VertX. I was looking for a more "streamlined" and supported pattern for a common enough operation.

All in all, I'd consider HeaderInspectionOptions as the current workaround, but I'm looking forward on how this can be solved at the abstractions boundary making it consistent and supported across languages/libraries.

[andreaTP]

I confirm: https://github.com/microsoft/kiota-http-dotnet/blob/0212e818931b31a2d9e0ab4d768b31f23fe5f386/Microsoft.Kiota.Http.HttpClientLibrary.Tests/Middleware/HeadersInspectionHandlerTests.cs#L43-L55

This API has some shortcomings:

• it's bound to the http client implementation
• it's extremely low level
• requires merging information coming from 2 different sources
• it couples the RequestAdapter instance to the request business logic

Considering the user's point of view:

I want to use GitHub pagination

I would say that there is room for improvement here 🙂

[baywet]

Thanks for the additional context here.

dotnet specific implementation

Here is the java equivalent (yes it's tied to OkHttp implementation)

thread safety

This should be safe since the option is tied to the request information and doesn't cross any thread boundaries (unless the request execution is deferred to a thread pool by the application explicitly, in which case the simplest is to defer the whole block and not just the request execution).

I want to use GitHub pagination

There's no standard for pagination across REST APIs, or even standard ways to describe how the pagination is done.
This is why in the case of Microsoft Graph the page iterator lives in the Microsoft Graph Core layer, and not in kiota.

We considered adding support for generic pagination in kiota in the past, but this is something that we decided not to do in the end. #1499

I'd expect GitHub's pagination logic to be implemented in GitHub's core library for the time being. If we get enough demand for generic pagination support over time, we might want to consider it a new feature for kiota, but you're the first to ask for it at this time.

[andreaTP]

Thanks for the feedback!

I'd expect GitHub's pagination logic to be implemented in GitHub's core library for the time being.

The problem of having the implementation tied to the request adapter implementation means that it needs to be ported to all the official and integrators libraries.
This leaves room for partial/incomplete/diverging experiences across different languages and http-client implementations.

I keep thinking that, at minimum, we should lift the burden to extract the headers(body in case of errors would be nice too) from a request, having a higher level API defined in abstractions(hence implemented by all the http-client implementation with the same interfaces).
Something like this should be possible:

Pair<Foo, Header> resp = client.foo.getWithHeader();

Is it reasonable?

[baywet]

Fair enough, if we hoisted the option in the abstractions, multiple http implementations could reuse it, making the implementation for the consumer a detail.

This getWithHeaders additional method has multiple drawbacks: complexifies the API surface, SDK size, might be a slippery slope when people want other things (trailers, etc...). I'd prefer avoiding it.

[andreaTP]

An alternative would be to add methods to the RequestAdapter, something like:

WithHeaders<Foo> resp = adapter.getWithHeader(() -> client.foo.get());
var headers = resp.headers();
var foo = resp.value();
...

thoughts?

[baywet]

I'm going to share dotnet examples if you don't mind.

regular scenario

var user = await client.Users["jane"].GetAsync();

proxy scenario

myAdapter.ReturnResponses = true;
var userResponse = await client.Users["jane"].GetAsync();
var headers = userResponse.Headers;
var user = userResponse as User;

As I'm writing this, I'm realizing that it's most likely the only language which would allow us to do that without having to generate a proxy class for each model would be typescript thanks to the proxy object. I don't think there are equivalent in other languages, especially the statically typed ones, which would allow us to "augment" any type with the additional properties we care about.

[andreaTP]

In Java there is Dynamic Proxy, but I would refrain to use it if possible.

But we can try to iterate here(in C# no problem!):

myAdapter.ReturnResponses = true;
var user = await client.Users["jane"].GetAsync();
var headers = user.Response.Headers;

this design requires:

• a single field: Response only on model classes that are actually returned from an endpoint
• it can be null and not set by default

[baywet]

yes, I guess that'd be the only way to make my alternative suggestion work.
My main concern here is that we're mixing concerns between the serialized model and the response "metadata".
In the super early days of kiota @darrelmiller absolutely didn't want to abstract the response (which is really what we'd need here, it'd also help us clean up the request adapter interface).

[andreaTP]

didn't want to abstract the response

I agree as a "general sentiment", but, in practice, there are several real world use-cases that would take advantage of such functionality.
The other way around it is to provide specific implementations/customizations to enable each and every use-case.

My main concern here is that we're mixing concerns between the serialized model and the response "metadata".

I agree, this option won't be my favorite one, but, I was following along to try to get to an acceptable design ...
The other options mentioned earlier have a cleaner distinction but different tradeoffs.

Adding a GetWithResponse ( XXXWithResponse) method to the generated code is probably the cleanest design, but I hear the argument about code size; should we start considering a CLI flag?

[baywet]

I know I'm annoying with that but I'd really like to avoid a design that requires introducing a CLI flag. It makes the experience more complicated and hostile to new comers, adds maintenance burden, etc...
Could static helper methods help orchestrate the option and do the trick, assuming we hoist the option? (a little like the native response handler

[andreaTP]

I see this option pretty close to this, but, keeping the static methods away from the adapter is going to make it hard to discover/test/etc.
In addition, we already assessed that we need a concrete http-client implementation to provide the functionality, hence, I'm not sure how to plug everything together, an example of "user code" would be helpful.
Even if possible, I would say that is better than nothing, but far from the Kiota experience we aim for.

[baywet]

For the example, I guess it'd look something like that

var userResponse = await ResponseReader.ExecuteAsync(() => client.Users["jane"].GetAsync());
var headers = userResponse.Headers;
var user = userResponse.Value;
var statusCode = userResponse.StatusCode;

[andreaTP]

Sorry for the delay here.
Well, your proposal works and is slightly better than the current status, if this is the best we can agree on, I'll take it.
How to proceed?
Should we do a strawman to verify that we are not missing invariants in one of the languages?

[baywet]

The only languages that worry me are the ones that don't have a great generic support (Go) or that are dynamically typed.
And sure, starting by defining the helper, response wrapper, and options in all abstractions should help us remove uncertainty here.

[andreaTP]

@darrelmiller I think we agreed on having your proposal using callbacks here, do you have a link or a write-up?

[Javatar81]

I have a related use case. I need to read the location response header. My approach in Java is:

NativeResponseHandler responseHandlerOption = new NativeResponseHandler();
ResponseHandlerOption opt = new ResponseHandlerOption();
opt.setResponseHandler(responseHandlerOption);
        
apiClient.admin().realms().byRealm(primary.getSpec().getRealm()).clients().post(createOption, r -> {r.options.add(opt);});

The problem is that I can't add to the options attribute because the collection is immutable.

[baywet]

@Javatar81 which version of the abstractions are you using? this is a bug we fixed see microsoft/kiota-java#1240

[Javatar81]

I am using microsoft-kiota-abstractions 1.1.6 currently. BTW the client that I am implementing based on Kiota is for the Keycloak API.

[baywet]

@Javatar81 try with latest 1.1.14 and let us know on a new issue in the kiota-java repo if this is still an issue (I don't want to side-track this thread)

[NetzwergX]

I would like to stress that it is often quite important to read the reponse headers. For example, when caching the responses from a 3rd party API, the "Last-Modified" and "Expires" headers are of utmost importance. Pagination has also been mentioned as important use case. Many APIs are rate limited and return information about available quotes in their headers back to the caller, e.g. remaining quota and time until quota resets. I currently interface with a 3rd party API where I have to take care of all of those, I have to cache the responses (Last-Modifed & Expires), some endpoints are paginated, and almost all of them return some rat-limiting metadata.

I am currently required to wrap every single call into code like this:

    var _headers = new HeadersInspectionOption();
    _headers.setInspectResponseHeaders(true);

    var  data = client.items().byItem_id(12345).get(it -> it.options.add(_headers));
    Objects.requireNonNull(data).getAdditionalData().put("headers", _headers.getResponseHeaders());

I like Kiota because the generated API is much nicer than what e.g. openapis swagger generator delivers. But not being able to have an easy way to both get the value and the headers is very frustrating. And the above is still a cruft because I put stuff into additionalData....

(I am using Java).

The headers are very relevant more often than not. I haven't ever interfaced with another REST endpoint and not needed them. Caching, Pagination and Rate limiting are only three areas where that is important, and they come up often. Other use cases also require the headers.

If anyone has an easier way to get the response headers, I am open for suggestions. Otherwise please consider exposing the response headers in the API far more easily.

[baywet]

Thank you for the additional information.
I believe that caching is an orthogonal concern which should be implemented by a middleware handler. And as such, we probably shouldn't mix the discussion with that additional concern.
This is something we've been talking about implementing for a while, but never got the funding/time to get to it.
If you want to start a separate conversation about caching, please go ahead