microsoft / kiota

OpenAPI based HTTP Client code generator
https://aka.ms/kiota/docs
MIT License
3.01k stars 210 forks source link

Customizing the Structure of Generated Client Code in Kiota #4004

Open sander1095 opened 10 months ago

sander1095 commented 10 months ago

Issue

Currently, Kiota generates client code based on the structure of the provided OpenAPI document. I have an API that looks like this:

/
 └─api
    └─talks
       └─{id}

This results in the following API client code:

var result = await kiotaClient.Api.Talks.GetAsync();

I find this a bit ugly. I'd prefer it to be kiotaClient.Talks.GetAsync(); because the Api doesn't add anything.

Is there a way to do this? Lots of API's have api/ in their path, so I don't think this is an exceptional case :)

My code can be found here: https://github.com/sander1095/openapi-code-generation-talk/tree/main/api-client-generation-demo

svrooij commented 10 months ago

Just add the /api part to the server URL (aka base uri). Then your open API docs won't even show it. That is how I always configure my docs anyway, and that is also what Microsoft itself does with their Graph API

sander1095 commented 10 months ago

Hey @svrooij, thanks for the quick reply!

I believe that you simply remove the api/ bit from the OpenAPI document this way, this feels a bit like cheating ;-). While clever, I believe this wouldn't work on a server that serves an API on api/ and other content in /client, for example. Like websites that return both a server-rendered client or an API for 3rd party clients.

This isn't the case for me right now, but I definitely believe this isn't uncommon. I'd like to see an option to tell Kiota to handle Api as a part of its internal base address so I don't need to touch this .Api. bit :)

darrelmiller commented 10 months ago

@sander1095 What @svrooij suggested definitely is not cheating. It is a core philosophy of Kiota that the projected language experience is as close a match to the HTTP API description as possible. While I understand that it is appealing to many people to want to tweak the API surface to "fix" it client libraries, we believe that introducing these differences lead to confusion in developer experiences.

Having said all of this.... Kiota is designed to allow generation of clients for just subsets of an API. It would be possible to allow a parameter that defines a new base URL for the client to use. e.g. Consider Twilio's API image

It would be possible to only include paths /2010-04-01/* and redefine the base path to include the 2010-04-01 so that it would not be a property of the client. That would be convenient. However, it means that if someone rebuilds the client with endpoints that have resources from a later version, it would be a breaking change to the client.

I do think this is worth having a conversation about for this particular conversation. It is particularly interesting for Azure APIs that have /subscriptions/{subscriptionId}/Microsoft.SomeProvider as a prefix to many of their API endpoints.

sander1095 commented 10 months ago

Hi @darrelmiller, thanks for your reply.

While I understand that it is appealing to many people to want to tweak the API surface to "fix" it client libraries, we believe that introducing these differences lead to confusion in developer experiences.

Honestly, I find the current experience to be more "confusing". I see the api/ purely as a prefix to access the real API, which is talks in my case. :) Of course this is just my opinion ;).

FYI: Whilst I do not really want to do this, another workaround would be to create a wrapper around the client like so:

class ApiClientWrapper(KiotaApiClient apiClient)
{
    public ApiRequestBuilder Talks => apiClient.Api;
}

var kiotaApiClient = GetKiotaApiClient();
var apiClientWrapper = new(kiotaApiClient);

// Now we avoid the .Api. property in actual Kiota usage.
var talks = apiClientWrapper.Talks.GetAsync();

I'm not the biggest fan of this custom approach, mostly because I'm used to using NSwag for my API Client generation, and these issues do not exist there because it simply generates a class with the API calls (and api/ path) in it.

I'd love to have this conversation that you mention. From my point of view, I think it would be worth having a flag like --internalizePrefix "api/" (or something) so it would simply call api/ in the generated Kiota code. Clients would then choose for this behavior and would not be confused. The top-level objects (In my case, Talks) could even have some XML comments that explain that this is api/Talks under the hood because of client generation options, so things are clearer for devs.

This would be similar to NSwag's behavior, where the api/ is internalized: https://github.com/sander1095/openapi-code-generation-talk/blob/9a5ae8999d32b29a3a8c73a4171640d81fd2a95a/api-client-generation-demo/ConferenceApp/Clients/NSwag/GeneratedApiClient.cs#L108

baywet commented 10 months ago

Hi @sander1095,

If we added a list of prefixes to ignore when projecting the fluent API, this brings two new problems:

As for the fluent API aspect (when comparing with NSwag), it's one of the core experience aspects of kiota, so kiota clients work great regardless of the shape of the API. Flattening clients works ok for APIs that don't have a lot of endpoints and/or a lot of nesting. But for the many cases of APIs that aren't designed this way, it becomes a nightmare (e.g. /users/id/conversations/id/members/id/profile/picture become a client.GetUsersIdConversationsIdMembersIdProfilePictureAsync if we don't want it to conflict with other APIs, which in my opinion is harder to parse than client.Users["id"].Conversations["id"].Members["id"].Profile.Picture.GetAsync() where you know a "dot" on the fluent API call roughly maps to a slash in the path segmentation.

sander1095 commented 10 months ago

@baywet

What should the list be (everyone has different opinions)? which leads to another command line parameter, which makes kiota more complex to use.

The list would not be decided by kiota. A developer could set these up when using kiota generate like this: --internalizePrefix "api/".

What happens when a sub path segment conflicts with a root path segment? (e.g. description has both api/talks and /talks for some reason)

This can be found out during code generation, so an error can be thrown.

(e.g. /users/id/conversations/id/members/id/profile/picture become a client.GetUsersIdConversationsIdMembersIdProfilePictureAsync if we don't want it to conflict with other APIs

NSwag handles this in a smart way by allowing users to decide the method name in multiple ways, like path segments, tags, operationid, etc.. I understand that the method name would be vague, but that would also be the case for the server implementation of this method :)

baywet commented 10 months ago

The additional argument solution (with no defaults) could be a valid approach. My concern with the current "design" is people will want another one for the suffix as well, and soon after one to remove segments in the middle as well. Which is a slippery slope all together.

As for throwing an exception, even assuming we format it properly with an explicit error message, I'm not sure how that's going to improve the experience.

I'll let @sebastienlevert lean on this one as the PM, but from my perspective there's already a workaround with the base URL. Although I understand how painful it can be to maintain those change in sync if you're not the API producer as well.

sebastienlevert commented 10 months ago

I share sentiment with @baywet on this scenario, but I really think this is an important one to support. Though, having yet another configuration shouldn't be how we move forward with this. This will become unmanageable for generating clients in the future and we should think broader. Where I think there is an opportunity is to leverage OpenAPI overlays to let developers simply override the pieces they need. There is a lot of things to think about with overlays but it's something we're already envisioning for the future of Kiota. If a developer was providing a super simple overlay, they could override the server url without the need to control the OpenAPI description and would be a good and standard experience.

overlay: 1.0.0
servers:
  - url: "http://localhost:4010/api"
    description: Local development server

Adding @darrelmiller to share thoughts, but I feel this is the way to go.

sander1095 commented 10 months ago

Could this issue be reopened until a conclusion is reached?

sander1095 commented 10 months ago

I see the tag "author feedback needed" is added. i don't have experience with openapi overlays. I think it could be interesting, however I also think that lots of developers generate their openapi definition instead of designing it upfront, so i dont know how many generators will start utilizing these overlays.

But, anything that makes use of the spec is a plus point for me!