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

Feature: Improve generated BaseCollectionPagination(Count)Response (DRY) + paging out of the box #3879

Closed
dabla opened this issue Dec 8, 2023 · 5 comments

Comments

@dabla
Copy link

dabla commented Dec 8, 2023

As already discussed in the pull request I made in the msgraph_sdk, would it be possible the adapt the generated code of BaseCollectionPaginationResponse so it would become more generic (which means fewer lines of code needs to be generated) AND also supports auto paging out of the box (using the Iterable desing pattern) so that the user of the client doesn't have to care anymore how the odata paging works which would make it even more user friendly to use? I think this would be a nice improvement, before we were using a custom python client for the ms graph api which resembles a bit of what's done here (ofc not auto generated) and there we had that functionality which makes it very easy with less (custom) code to do paging on large datasets coming from the MS Graph API. Maybe we could define a base class in the python implementation of the core packages and reference it where needed, but there as I don't know the generated part I hope you guys can come with a solution.

Here is the link to the pull request where the discussion started:

microsoftgraph/msgraph-sdk-python#506

I'm willing to contribute if necessary.

@github-project-automation github-project-automation bot moved this to Todo in Kiota Dec 8, 2023
@baywet baywet self-assigned this Dec 8, 2023
@baywet baywet added this to the Backlog milestone Dec 8, 2023
@baywet
Copy link
Member

baywet commented Dec 8, 2023

Hi @dabla,
Thank you for your interest and your contribution.
Kiota doesn't know about OData semantics, it relies on OpenAPI to describe the API surface and delegates the OData to OpenAPI conversion to a library called OpenAPI.net.OData.

Long story short: we had a history of building OData based SDKs, it brought a lot of complexity, and since Microsoft Graph is not a true OData service (lot of conventions not consistently implemented), it'd break down in a lot of cases. The previous generators failed to achieve adoption in parts because they relied on OData. So when we started working on kiota, we made the separation of concerns one of the principles.

With that in mind, kiota doesn't "know" about the paging structure from the service, it just "sees" a BaseCollectionPaginationResponse schema in the description and projects it as a type. Unfortunately OpenAPI doesn't have a construct to support generic schemas (schemas with a type parameter), this would be a great addition but it doesn't exist today.
This is why the conversion library projects every collection as an inherited type from Base collection (or base count collection), which arguably is wasteful, but there doesn't seem to be a better alternative to represent this scenario today in the standard.

Now that you have a bit more context, are there still avenues to improve this scenario from your perspective?

@dabla
Copy link
Author

dabla commented Dec 8, 2023

Hi @dabla, Thank you for your interest and your contribution. Kiota doesn't know about OData semantics, it relies on OpenAPI to describe the API surface and delegates the OData to OpenAPI conversion to a library called OpenAPI.net.OData.

Long story short: we had a history of building OData based SDKs, it brought a lot of complexity, and since Microsoft Graph is not a true OData service (lot of conventions not consistently implemented), it'd break down in a lot of cases. The previous generators failed to achieve adoption in parts because they relied on OData. So when we started working on kiota, we made the separation of concerns one of the principles.

With that in mind, kiota doesn't "know" about the paging structure from the service, it just "sees" a BaseCollectionPaginationResponse schema in the description and projects it as a type. Unfortunately OpenAPI doesn't have a construct to support generic schemas (schemas with a type parameter), this would be a great addition but it doesn't exist today. This is why the conversion library projects every collection as an inherited type from Base collection (or base count collection), which arguably is wasteful, but there doesn't seem to be a better alternative to represent this scenario today in the standard.

Now that you have a bit more context, are there still avenues to improve this scenario from your perspective?

Hello @baywet,

Thank you for your elaborate explanation, this makes indeed sense and I understand now why it's not possible from a generational perpective (if the spec doesn't provide anything to achieve this). Wouldn't it be an idea to add some kind of interception during the code generation and when particular code is generated (for example SiteCollectionResponse which has BaseCollectionPagination(Count)Response as base) this would be detected and we apply the auto paging stuff. Or is this just the kind of think you want to avoid/not want to do?

@baywet
Copy link
Member

baywet commented Dec 8, 2023

Slight correction on my previous statement, OpenAPI 3.1 has dynamicRef which would allow us to describe generics today.
However 3.1 is not implemented by OpenAPI.net yet (which both kiota and the conversion process rely on today to parse/emit the descriptions), this library also belongs to our team and we've had people working on implementing the support for a while now. I have created an issue in the conversion library to capture the idea so we don't forget about it.

As for the extensibility of the generation logic part, another pillar of kiota is to keep it as simple as possible to use. No language specific switches, a single generation experience per language, no customized templating mechanism... in an effort to bolster adoption. So far the assumption behind this pillar seems to be holding true. So while this would be possible, this is not the direction we want to take. As an implementation of 3.1 would have broader benefits for everyone.

@dabla
Copy link
Author

dabla commented Dec 8, 2023

Let's wait for OpenAPI 3.1 implementation then :)

@baywet
Copy link
Member

baywet commented Dec 8, 2023

perfect, closing this issue since things are being tracked upstream. Thanks for bringing this up!

@baywet baywet closed this as not planned Won't fix, can't repro, duplicate, stale Dec 8, 2023
@github-project-automation github-project-automation bot moved this from Todo to Done in Kiota Dec 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Archived in project
Development

No branches or pull requests

2 participants