microsoft / kiota

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

Generated Code Structure #1722

Closed softworkz closed 2 years ago

softworkz commented 2 years ago

From my OpenApi spec, I see generated code like this:

image

And the selected class has the following namespace:

namespace ApiSdk.Items.Item.Images.Item.Item.Item.Item.Item.Item.Item

..which in turn means that you would need the same kind of using statements everywhere you use the code.

That does not seem feasible to me. An API user cannot be expected to work with that.

Design or Bug?

baywet commented 2 years ago

Design https://microsoft.github.io/kiota/extending/requestbuilders.html#pathparameters

You rarely (if at all) have to reference the request builders themselves, you might have to reference the request body if you're sending a POST/PATCH and the request body is defined inline which will be placed next to request builder.

I'm guessing you ended up with this result because you have multiple single path parameters in the API description like: /Items/{id1}/Images/{id2}/{id3}/{id4}/{id5}/{id6}/{id7}/{id8}

Which means you'll be able to build a request like this:

client.Items["id1"].Images["id2"]["id3"]["id4"]["id5"]["id6"]["id7"]["id8"].GetAsync()
softworkz commented 2 years ago

I'm guessing you ended up with this result because you have multiple single path parameters in the API description like: /Items/{id1}/Images/{id2}/{id3}/{id4}/{id5}/{id6}/{id7}/{id8}

Yes, something like:

    [Route("/Items/{Id}/Images/{Type}/{Index}/{Tag}/{Format}/{MaxWidth}/{MaxHeight}/{PercentPlayed}/{UnplayedCount}", "HEAD")]

Those paths exist because when using images in browser (html) clients, you need GET requests for specifying the image source of DOM elements.

Which means you'll be able to build a request like this:

client.Items["id1"].Images["id2"]["id3"]["id4"]["id5"]["id6"]["id7"]["id8"].GetAsync()

Oh....what is this? This is how I'm supposed to do a request?

So, there's no type checking, it's all strings? That syntax is horrible. How do I know which param is which and which type?

The document you referenced says something like "The matching function will be chosen automatically". Does it mean that it chooses even the REST API endpoints based on the supplied parameters?

baywet commented 2 years ago

So, there's no type checking, it's all strings? That syntax is horrible. How do I know which param is which and which type?

To be fair, if someone so that API call, with no OpenAPI description, or no additional context, how would the guess which is what?

GET /Items/abdec/Images/wide/30/football/png/1920/1080/10/3

They might be able to deduct some of the parameters based on common knowledge (png, the first id, 1920, 1080) but some will be really hard to understand (wide, football, 10, 3). Inserting additional segments would have made for a more self-explanatory API experience.

GET /Items/abdec/Images/Type/wide/30/Category/football/Format/png/Width/1920/Height/1080/PctViewed/10/UnplayedCount3

Parameter types described as integer might translate to an integer in the dotnet indexers, I don't believe we have tested for that but this should be easily fixable if not the case today.

The document you referenced says something like "The matching function will be chosen automatically". Does it mean that it chooses even the REST API endpoints based on the supplied parameters?

No, what that meant is depending on how many path parameter you have per any given segment, it'll be translated in the following symbol on the chained API:

softworkz commented 2 years ago

To be fair, if someone so that API call, with no OpenAPI description, or no additional context, how would the guess which is what? They might be able to deduct some of the parameters based on common knowledge (png, the first id, 1920, 1080)

Why are we talking about guessing the placement of path parameters? We are here to make sure that nobody needs to do that. That's the whole point of generating API clients with strong typing. Finally, ending up with some simple functions to call like:

image

But again - what is this - how should this be any good?

client.Items["id1"].Images["id2"]["id3"]["id4"]["id5"]["id6"]["id7"]["id8"].GetAsync()

This breaks half of the achievements of modern software development.

A simple function is superior to this in all ways. Even constructing the path manually is still better:

image

baywet commented 2 years ago

Why are we talking about guessing the placement of path parameters?

Because the generated client can only get as good as the API was designed to begin with. Instead of appending all the parameters in the route template this API could have:

In this API design two concerns are mixed:

Building the path string doesn't provide any validation whatsoever, and makes a moot point of using a generated client. Using a single function flattens the whole paths tree which brings a whole other set of problems (mostly name conflicts and discoverability) for: overloads, additional path structure (e.g. what if there is a Videos root path? etc...)

Finally, indexers are only used in CSharp, because it's the only language they exist in, other languages generate methods instead. Like

client.itemsById("{Id}").ImagesByType("{Type}").ByIndex("{Index}").ByTag("{Tag}")...
softworkz commented 2 years ago

Both of these ways:

client.Items["id1"].Images["id2"]["id3"]["id4"]["id5"]["id6"]["id7"]["id8"].GetAsync()
client.itemsById("{Id}").ImagesByType("{Type}").ByIndex("{Index}").ByTag("{Tag}")...

..are the very last ones for how I would want to access a REST API from a client.

From my personal point of view, those patterns are misdirected. Essentially, huge effort is taken here in order to make C# (or another .NET language) behave like much inferior languages, in parts even like scripting languages. Even the "Fluent" approach itself has limited value IMO and some severe downsides as well - but that's not the point here.

Even with most fluent APIs, a function call is a function call - a single (logical) function call! But the idea of splitting a logical (REST API) call in a way that you call function-by-function-by-function.. for every single parameter before you actually execute it, is an extraordinarily bad idea IMO. While you do that, you don't even know which function you will call eventually. It's one of the worst concepts I have heard and seen in a long time.

I don't know who created that concept - please don't take it personal in case. You have been very very friendly and helpful and patient and I had never expected to come to that conclusion about Kiota. I really hadn't seen it coming, probably because none of the "Getting Started" docs had any code snippets showing the usage of the generated API. In that case I would have just silently turned away 😞

Thanks for all your time and your friendly support! softworkz

baywet commented 2 years ago

No worries, the feedback about the documentation being incomplete is something we already have in the backlog and our documentation writer will work on (hopefully) soon.

As per the generated API shape, I understand you're not a fan of it. Just for context it doesn't come out of nowhere, the builder pattern and the fluent API design pattern being fairly popular (even in dotnet, if you look at all the asp.net core middleware configuration aspects).

As per knowing which method to call, well the chained API maps to the actual API, an example with Microsoft Graph would be:

GET /groups/{id}/members/microsoft.graph.user/

would map to

client.Groups["id"].Members.User.GetAsync()

Same number of segments and the names are really close.

baywet commented 2 years ago

I do appreciate the open conversation as well and the feedback, thanks for your (short) time spent on Kiota!

softworkz commented 2 years ago

the fluent API design pattern being fairly popular (even in dotnet, if you look at all the asp.net core middleware configuration aspects).

Really? I beg to differ. Because when it would be popular, it would have been quickly adopted in many other places. There were attempts here and there - but no clear success stories.

The reason why it is used with ASP.Net is because they designed it in a way that you are more or less forced to use it because the regular way would just be much worse. While it has some nice aspects, my major point of criticism is that it "hides" everything you might need. As the architecture is heavily relying on extension methods, those methods don't easily appear in IntelliSense. You always need to 1. import the right nuget package and 2. use appropriate using statements to import the namespaces. Even ReSharper doesn't always get it right. This has a gigantic impact on discoverability. Normally, when you are new to an API, you can scan through classes and other types, look at functions and their parameters, and you often quickly find what you need. With the new ASP.Net, I had to spend endless time on reading documentation and doing research on how to do specific things, because you can't find those things from the object model. Even for very basic things, you need nuget packages and reference them, otherwise you don't see them. And you are often not sure whether an approach you read about is the best/latest/most-suitable approach.

Yes, you are right. I'm not a fan of it. But I would be when it would improve my work. Which it doesn't :-)