search

microsoft / kiota

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

add pet store examples to the samples repo #135

Closed baywet closed 1 year ago

baywet commented 3 years ago

Note: the projects won't build until you add references to kiota abstractions.

AB#9120

darrelmiller commented 3 years ago

Sure, I'll get to it.

PureKrome commented 3 years ago

Further to this issue, here's the cli command I've been trying to use (but with no luck / errors out):

>kiota.exe --openapi https://petstore.swagger.io/v2/swagger.json -o OpenApiClient\Generated -c PetShopGraphClient --loglevel Information

and this throws an error:

image

(one of the two schema instances is a null instance)

baywet commented 3 years ago

@PureKrome Thanks for reporting this and trying to generate the pet store samples. Fixed it with #185

baywet commented 3 years ago

@pureKrome, are you still working on this or should we let @wcontayon take a stab at this?

Note: it'll probably be better to wait for #204 to be merged before taking this on.

PureKrome commented 3 years ago

Happy to wait and/or let @wcontayon take it - i've still tried and haven't had too much luck yet.

baywet commented 3 years ago

Thanks for the lightspeed answer! Did you run into any additional issue? If so, can you report them as new issues so we can tackle them please?

PureKrome commented 3 years ago

I thought the latest code pull .. didn't work? i think the generated code was requiring nuget packages that didn't exist?

baywet commented 3 years ago

You should be able to get the packages by configuring the package manager to use the github package feed. Which language are you targeting at the moment?

PureKrome commented 3 years ago

C# / .NET Core.

Yep - I just haven't done the Github PAT yet. Feels like a frustrating (temp) barrier to entry. But I'll give it a go!

baywet commented 3 years ago

the very summarized instructions are here. If this is not clear enough, don't hesitate to complain/PR the instructions. We'll be removing this barrier once we go to public preview as mentioned in #122 and #128 but it's too early for now.

baywet commented 3 years ago

I'm going to assign this to you for now, to avoid having multiple people working on the same thing. If you feel like you don't have time or or don't want to do it anymore, just let me know. Thanks for the help!

PureKrome commented 3 years ago

Status Update:

image

image

Thoughts:

Update

The File might be related to this node:

image

baywet commented 3 years ago

Hey @PureKrome Thanks for the additional feedback here! Pet Store is full of surprises 😲 ! I've logged a few issues to track future work to fully support all those cases. We won't be fixing #219, #220 and #221 for now. I however authored #222 which works around these issues and fixes #218. Give it a try and let me know what you think! Thanks again for all the work.

PureKrome commented 3 years ago

Thanks again @baywet for looking into this.

I don't understand #219 and #220 (that just me struggling to grok stuff).

but with #221 - until that is done, File will always be generated, preventing the GeneratedClient from compiling, right?

baywet commented 3 years ago

no because this file type is only included because of the schema of the "multipart/form-data" request schema. And I have excluded that content type for requests with my latest PR as even if we fix the generation issue, we'll still need to add a new serializer we don't have today. If you delete the previous generation result and do it again, you should end up with everything for pet store except that operation.

baywet commented 3 years ago

Hey @PureKrome, How are things going on this front?

PureKrome commented 3 years ago

Heya - i've not forgotton this. I've just been slammed. I still want to do this (i have started on it. I got stuff done/generated in docker images, actually :P )

PureKrome commented 3 years ago

Heya again @baywet - trying this again.

(just ignore that the folder should be called something like PetStoreClient .. not Pet -SHOP- Client 😊

image

image

image

i'm guessing i've done some incorrect/missing step.

baywet commented 3 years ago

Hey @PureKrome You've made great progress! No, it's not you doing anything wrong, it's me forgetting to update the template project and probably the fact the released version is a bit behind.

Project file updates

Not only these references only work relatively to cloning kiota and kiota samples as a submodule, it reference to the older kiota core single package that was splat a while ago.

Here is how to fix it:

  1. remove all the project references.
  2. Keep the abstractions reference as it is.

  3. Add the following package references (you don't need those to compile, but you'll need those to run)

     <PackageReference Include="Microsoft.Kiota.Authentication.Azure" Version="1.0.2" />
 <PackageReference Include="Microsoft.Kiota.Serialization.Json" Version="1.0.2" />
 <PackageReference Include="Microsoft.Kiota.Http.HttpClient" Version="1.0.2" />

Released version behind

Can you generate from a kiota built from the latest of the main branch please? If it does fix the issue, I'll create a release. If it doesn't, I'll investigate further.

PureKrome commented 3 years ago

Thanks @baywet ! I'll give all this another go :) this is great info!

PureKrome commented 3 years ago

Heya @baywet.

had another attempt at this. I feel bad for keeping posting questions/problems. Sincerely, so sorry.

So I've tried again. This time, I used kiota (latest code as of 30 mins ago) to generate the -client-.

Here's some WARNING output:

image

Ignoring these warnings, I continued on:

image

For simplicity, I have:

Does any of this info, help?

baywet commented 3 years ago

@PureKrome No need to apologize at all. This effort is super valuable and helps us identify and fix the rough edged corners of Kiota before it reaches prime time. From what you describe, you did everything right. I'm not sure why you have that bug though since I already fixed with with #218 (you had already reported it earlier in the conversation). I reopened the bug and will take another look at it. Thanks for your patience on the matter and for all the super valuable feedback!

baywet commented 3 years ago

also put that together in case it was a confusion between forks/remotes etc... https://github.com/PureKrome/kiota/pull/1

baywet commented 3 years ago

@PureKrome I can confirm that was an issue with the generator. A casing issue 🤦‍♂️ Put a PR to fix it #378. Can you give it another try using that branch (or after it gets merged) please? Thanks a lot again!

PureKrome commented 3 years ago

Woot! Sure will! i'll give it a go, tonight! Exciting .. hopefully getting closer ... :)

PureKrome commented 3 years ago

I've had a few more attempts in the last three weeks and still not working. I've had different errors over this period .. with today's being different again.

This is the latest error(s): (sorry the images are so wide)

image

image

Note that I've just pulled 1.0.4 for one of the packages.

dotnet.csproj

<Project Sdk="Microsoft.NET.Sdk">

  <ItemGroup>
      <PackageReference Include="Microsoft.Kiota.Authentication.Azure" Version="1.0.3" />
      <PackageReference Include="Microsoft.Kiota.Serialization.Json" Version="1.0.3" />
      <PackageReference Include="Microsoft.Kiota.Http.HttpClient" Version="1.0.4" />
  </ItemGroup>

  <PropertyGroup>
    <OutputType>Exe</OutputType>
    <TargetFramework>net5.0</TargetFramework>
  </PropertyGroup>

</Project>

and the client was generated with latest code from main branch, which I just pulled down about 15 mins ago.

Thoughts? I'm wondering if this is my fault, now?

baywet commented 3 years ago

@PureKrome Thanks for the feedback once again! No you didn't do anything wrong, this is another limitation that wasn't anticipated. I've logged #467 , the note at the bottom provides a little more context on the issue. I'll try to get going on that bug as soon as I have some time. :)

PureKrome commented 3 years ago

Thanks mate :) really appreciate it, as always!

PureKrome commented 3 years ago

@baywet - good news tonight!

so my next question -> how to use the generated ApiClient ?

It says it requires an IHttpCore. So i've been digging around various folders in the root directory and did find an HttpCore. this though, requires an IAuthenticationProvider which .. after further searching, looks like there is only -one- implimentation of this? An AzureIdentityAuthenticationProvider only? Which wouldn't apply to me, here because I'm not hitting an Azure auth-secured endpoint.

HttpCore requires an IAuthenticationProvider. Was this an oversight?

Next, I did find some docs on this ... and the docs have no requires parameters?

image

any ideas?

baywet commented 3 years ago

That's great news! Thank you for all the feedback on this, it definitively help mature Kiota. As a general disclaimer, we know the documentation is far behind at this point. We do have a few issues to capture that see #129, #130 & #132. Help on those is welcomed, and I'll try to see with the PMs (@roinochieng @maisarissi) whether we can slow down on feature work and tackle this as the lack of up to date documentation is hurting both our community adoption and the ramp-up of new internal developers on the project. Nonetheless I put together #495 to at least address your comments.

Now, here is a question for you: we made the assumption that most clients generated with Kiota would talk to APIs using some form of authN/authZ. This is why we made the choice of requiring an auth provider, this guides the consumer into "doing the right thing". How does that assumption hold from your point of view?

Another question for you: did you try looking at the doc comments/navigating the symbols to discover where this authentication provider could come from? Did it help at all?

At this time a workaround for you would be to implement a "no-op authentication provider" from the IAuthenticationProvider interface and return non-empty string. Hopefully the target API will ignore the bearer. But I think a few things could be done to make it easier for other authentication schemes:

  1. Update the interface to accept the full request info, so implementations can set any header they want to, or nothing.
  2. Provide a default "AnonymousAuthenticationHandler" in the abstractions library.
PureKrome commented 3 years ago

hi @baywet - I thought I replied to the questions above, but it looks like I didn't :( sowwy.

Now, here is a question for you: we made the assumption that most clients generated with Kiota would talk to APIs using some form of authN/authZ. This is why we made the choice of requiring an auth provider, this guides the consumer into "doing the right thing". How does that assumption hold from your point of view?

Ah - interesting. I was wondering what was going on there. It's funny because at first I was finding this decision was hindering my spike. All the places I want to experiment with are public with optional auth. For example, I'm playing with the Swagger Petstore but also will want to play with Twitter API (they have an OpenApi schema) and Stripe (they also have an OpenApi schema but it's a bit confusing, to me).

For our personal use case, I wish to use it for our microservices, so that will be using typical JWT auth.

So I feel like providing an Auth Provider should be optional? make it a nullable ctor parameter maybe? which under the hood just uses a static instance (why keep creating allocations of the same thing) of the no-op AnonymousAuthenticationHandler ?

Another question for you: did you try looking at the doc comments/navigating the symbols to discover where this authentication provider could come from? Did it help at all?

Nope :( I actually didn't really know they existed? I've been working off the readme file, but even then I didn't realize that some of the links off that page were actually documentation examples (or lead to doc examples). I've been literally trying to look at some tests or code examples deep in the repo.

What really helped me was this code example. But I only just found it last night after clicking through a few links. See this:

image

to me, this was a bit misleading. Required tools (to me) was potentially (because I've never clicked on it) some steps/tools that would tell me how to run Kiota. Because I already know this, i've never clicked on it. I didn't think it would have some code examples etc.

Also, the other links don't explain to me why I would want to click on them? I thought they would just be links to the actual code (which they sorta are, like the AnonymousAuth link) ... but no indication as to why I would need this info? Most of time time, I don't want to know how things are implemented but how to use them. So maybe some clarification in the readme might help me/others discover things easier/more efficiently?

baywet commented 3 years ago

Thanks again, this is all super valuable feedback!

Authentication: We've taken the path of requiring an authentication provider to guide people to make a conscious choice about that. While I understand it might not be super intuitive to pass the anonymous provider, I do believe that anonymous APIs are a minority and we do have strong requirements for the Microsoft Graph client SDKs we'll be generating using Kiota on that aspect. I suggest to see if we get more feedback on that aspect from different sources.

Required tools: @darrelmiller put most of that documentation together yesterday. We do know we have a gap in terms of documentation we're trying really hard to address, and Darrel made some good progress over the last week on that. The fact is our PM for Kiota is busy on other priorities at the moment and we're a bit short-handed here, thanks for your patience on that :)

In your opinion, how could we improve that table to make you want to click the link? Maybe renaming the tooling to Host project configuration? or something along those lines? Something different?

baywet commented 3 years ago

Did you see this section ?

PureKrome commented 3 years ago

We've taken the path of requiring an authentication provider to guide people to make a conscious choice about that.

No probs. As you also suggested, if there's blowback then it's not a hard thing to add extra overloads, etc. The main item here is the communication about this (versus the actual code a developer has to write). Docs and readme's and summaries can be leveraged here to promote this concept, IMO. Nice!

Did you see this section ?

Yep. But this info was all about -generating- an SDK. Which was pretty easy to learn/find/use, when I first started out.

In your opinion, how could we improve that table to make you want to click the link? Maybe renaming the tooling to Host project configuration? or something along those lines? Something different?

IMO, I feel like all the communication in the chart is miscommunicating.

I'm not going to suggest what to do, but say what my experience was that made me all get confused.

image

Supported Languages

to me, this is talking about what languages the SDK is available for. Then later on I also realised it was both SDK -and- nugets (yes, nugets are C# only, but you understand the analogy).

So this is fine. It's technical and gives me an overall quick answer: Can I generate some code from a OpenApi endpoing in {insert my language}? Yep. nice.

Required Tools

This was the tricky one. I thought this was telling me what tools I would need to have installed on my developer machine if I wished to help out with the code base and contribute back to the project. Not once did I think: these are the tools required to generate a client class -AND- here's some example code to leverage that.

If I was going to try and do a TL;DR; in the readme for not-so-smart peeps like me, I would try and do a high level overview about the steps required to get an awesome result:

Then, after this high level summary - we can get down and dirty with

Problem with my brain dump above: which language to use in the steps above? It's an overkill to do all. so maybe 2 with some text MORE LANGS BELOW or links to the same example for other langs or something.

so, does this help?

baywet commented 3 years ago

this is super valuable feedback as always! I've drafted #580 in an effort to improve the story, let me know what you think! We're trying to get a technical writer to review and improve all of our existing documentation.

Back to the sample generation, were you able to call the petstore API? is anything else blocking you from putting together the PR to add it to the samples repo at this point?

PureKrome commented 3 years ago

I've added some feedback to #580 - I'm trying to brain dump my initial thoughts, so please don't be offended if it's come off sounding rude - 💯 wasn't/isn't ment to be, at all ❤️

Now, back to the petstore API. I got further but still no results.

Here is my current branch: https://github.com/purekrome/kiota-samples/tree/petstore

So .. getting there.

Also, it took me a bit of time to figure out how to pass Query Params. So this will defiantly need some documentation on this one.

baywet commented 3 years ago

Global feedback

No, don't worry about that, it is all very valuable feedback to have in the early days so we take a step back and look at basics. And thank you again for taking the time to provide it.

Fluent API style

This

var statusQueryParameters= new Dictionary<string, object> { { "status", "available,sold" } };
var result = await client.Pet.FindByStatus.GetAsync(x => x.AddQueryParameters(statusQueryParameters));

Was intended to be that

var result = await client.Pet.FindByStatus.GetAsync(x => x.Status = new string[] {"available", "sold"});

But maybe this approach is too "javascripty" for dotnet developers? This is something we could easily change to

var qParams = new GetQueryParameters {
    Status = new string[] {"available", "sold"}
};
var result = await client.Pet.FindByStatus.GetAsync(qParams);

The disadvantages of doing that are:

Arrays serialization

The reason it's serializing like that is because we've made another assumption that comes from OData. In OData the select parameter is an array, and it is used the following way select=id,displayName,lastName. Now we could add an option object for the http core, and a property of that option class would be serializeQueryStringArraysAsMutipleParameters defaulted to false. to switch behaviour. But I'd like to get @andrueastman and @darrelmiller 's point of view on this one before committing to anything, and maybe we have a better alternative here.

Authentication

That's weird since you're passing a auth provider (the anonymous one), what's the message you're getting exactly?

PureKrome commented 3 years ago

Fluent API Style.

I actually like this:

var result = await client.Pet.FindByStatus.GetAsync(x => x.Status = new string[] {"available", "sold"});

this works well with a lot of how dotnet web sites are setup in places like ConfigureServices, etc.

So this might be more of a documentation issue, than anything.

Arrays serialization

Just my luck that I stumble across edge cases, early on 😄 Or maybe they aren't edge cases? Very interested to hear the thoughts of @andrueastman and @darrelmiller

Auth.

image

It's weird. It's like it's saying: "this endpoint is restricted .. but the swagger says it's not restricted?"

baywet commented 3 years ago

Arrays

😂 no, it's not bad luck, don't worry about that. We've been building Kiota for Microsoft Graph SDKs first. And Microsoft Graph is an OData flavoured API. This brings some consistency on a lot of aspects which masks other use cases. Having someone keeping us in check and busting some assumptions we've made this early on the design is super valuable. Thanks again!

Auth

Here is your culprit 🤦‍♂️ I'm really sorry for missing that aspect, I logged #622 to solve this.

PureKrome commented 3 years ago

Awesome pickup @baywet ! I would have never guessed it was a hardcoded issue. I've sub'd to that issue so I can keep an eye out.

But yes! i never thought of using Fiddler to see the request.

So ... trying this once more ...

var result = await client.Pet.FindByStatus.GetAsync(x => x.Status = new[] { "available" }); ends up failing with:

image

It's not converting the dictionary correctly ?

while var result = await client.Store.Inventory.GetAsync(); now works!! (as it's going to the correct end point 🎉 )

getting closer ....

baywet commented 3 years ago

Thanks, just logged #625 to get this addressed, so at least we get status=available to be consistent with our current assumption.

baywet commented 3 years ago

@PureKrome Thanks for your patience, FYI the work on #628 has started and is available in #683 for dotnet in case you want to preview it. It should fully solve URL building challenges including solving for exploded query parameters (q=option1&q=option2).

PureKrome commented 3 years ago

Thanks @baywet! I was checking out commits and issues a few hours ago (like playing with the new docker image etc) :) so i'll give more 👀 on this, this weekend!

looking forward to it :)

baywet commented 3 years ago

@PureKrome fyi #683 has been merged a couple of days ago which should resolve the problem you were facing

PureKrome commented 2 years ago

Hi @baywet 👋🏻

So i've given this another run this morning and it's looking good 🎉

Now, I was checking out the result data to compare it to the upstream-petstore-source and noticed that the generator is adding some exta meta-data property in various places?

This is a json representation of a single item, in the list of results:

image

while when I look at a similar result via the online Swagger UI, it doesn't have this AdditionalData properties ?

image

think of this scenario:

baywet commented 2 years ago

Hi @PureKrome, Long time, thanks for the continuous efforts on that!

How did you get this Json representation? Did you just feed the object to System.Text.Json? Or did you use the provided serialization writer? When you call the methods from the doc db provider, do you pass it the object right away or a string result?

The presence of additional data is dictated by the OpenAPI description actually, but we currently have a conceptual shortcoming in the interfaces if you disable it, see #737 for the details.

PureKrome commented 2 years ago

👋🏻 G'Day @baywet 😸

How did you get this Json representation

And here's the code how I did it:

image

Did you just feed the object to System.Text.Json

Yep.

Or did you use the provided serialization writer

Um ... the wa???

So ... maybe I'll need to wait to set some property to -not- generate AdditionData which sounds like it's a very specific OData thingy?

baywet commented 2 years ago

We do actually have some conceptual documentation since a couple of days ago, I'd love for you to go over it and provide feedback https://microsoft.github.io/kiota/extending/serialization.html#serialization-writer

No, AdditionalData is not an OData concept. Even if it's not a best practice, the API could start returning properties that are not included in the description and for which the generated client doesn't have properties to "store them". Without this additional data property, the only alternative would be to ditch the values, with this, at least they have a home.

Try 

using var serializationWriter = requestAdapter.SerializationWriterFactory.GetSerializationWriter("application/json");
serializationWriter.WriteObject(result);
using var content = serializationWriter.GetSerializedContent();
using var reader = new StreamReader(content, Encoding.UTF8);
var serializedJsonString = reader.ReadToEnd();
PureKrome commented 2 years ago

I'm trying to understand what's going on here, but I'm confused.

I just don't understand why the json model is now including these AdditionalProperties when they are not part of the upstream/source OpenAPI schema?

Now, it's there because the generated models include them. e.g.

    public class FindByStatus : IParsable {
        /// <summary>Stores additional data not described in the OpenAPI description found when deserializing. Can be used for serialization as well.</summary>
        public IDictionary<string, object> AdditionalData { get; set; }
        public Petstore.Models.Category.Category Category { get; set; }
 ....
}

So what I don't understand is why these are added to the model -by default- ? I appreciate the extension point they are offering ... but could this be an OPT-IN setting?

I feel like I shouldn't be needing to have these properties AND I shouldn't have to be playing around with serializationWriters, etc...

All thinking about the least barrier to entry, to generating + calling an API.

baywet commented 2 years ago

This is super valuable feedback on the usage part!

The reason why it's included by default is because the OpenAPI.NET lib sets the value to true by default and it does so because it's following the JSON schema spec (on which the OpenAPI spec depends).

Here we're "following the spec" in terms of behavior. Ping @darrelmiller to provide additional input.