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

add pet store examples to the samples repo #135

Closed
4 tasks
baywet opened this issue Apr 21, 2021 · 86 comments · Fixed by microsoft/kiota-samples#993
Closed
4 tasks

add pet store examples to the samples repo #135

baywet opened this issue Apr 21, 2021 · 86 comments · Fixed by microsoft/kiota-samples#993
Assignees
Labels
documentation Improvements or additions to documentation generator Issues or improvements relater to generation capabilities. hacktoberfest help wanted Issue caused by core project dependency modules or library
Milestone

Comments

@baywet
Copy link
Member

baywet commented Apr 21, 2021

  • in the kiota-samples repo, copy the "templates" folder into a "petstore" folder.
  • for each language, generate an SDK from this description
  • add an entry to the table of content
  • update the readme in the folder that was created.

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

AB#9120

@baywet baywet added documentation Improvements or additions to documentation help wanted Issue caused by core project dependency modules or library labels Apr 21, 2021
@baywet baywet added this to the Preview milestone Apr 21, 2021
@baywet baywet modified the milestones: Preview, Alpha Apr 30, 2021
@darrelmiller
Copy link
Member

Sure, I'll get to it.

@PureKrome
Copy link
Contributor

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
Copy link
Member Author

baywet commented May 27, 2021

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

@baywet
Copy link
Member Author

baywet commented Jun 4, 2021

@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
Copy link
Contributor

PureKrome commented Jun 4, 2021

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

@baywet
Copy link
Member Author

baywet commented Jun 4, 2021

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
Copy link
Contributor

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

@baywet
Copy link
Member Author

baywet commented Jun 4, 2021

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
Copy link
Contributor

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
Copy link
Member Author

baywet commented Jun 4, 2021

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
Copy link
Member Author

baywet commented Jun 4, 2021

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
Copy link
Contributor

PureKrome commented Jun 5, 2021

Status Update:

  • Created GH PAT
  • Forked "Samples" repo
  • Duplicated then rename templates folder
    ** "alt-tabbed" over to another version of VS with Kiota upstream/master
  • Pulled (to make sure I've got the latest Kiota code)
  • "commandLineArgs": "--language csharp --openapi https://petstore.swagger.io/v2/swagger.json -o ..\\..\\..\\PetstoreClient-dotnet -c GraphClient --loglevel Information "
  • copied generated csharp client over to the new samples folder
    ** "alt-tabbed" back to "kiota-samples"
  • (TEMP STEP) updated nuget.config with my username and PAT Token
  • Installed package Microsoft.Kiota.Abstractions (with the PAT, this works 🎉)
  • ❌Compile code : fails
  • ❌SDK generation failed (see pictures) :

image

image

Thoughts:

  • Not sure what File is and why it didn't get generated?
  • username vs username. this is frustrating :( the class AND the property are the same (which can't be allowed, it seems). One of these will need to be renamed auto-magically during generation if both exist? I'm guessing the property ... with maybe a hint? This might be above my pay-grade 😊

Update

The File might be related to this node:

image

@baywet
Copy link
Member Author

baywet commented Jun 7, 2021

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
Copy link
Contributor

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
Copy link
Member Author

baywet commented Jun 7, 2021

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
Copy link
Member Author

baywet commented Jun 21, 2021

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

@baywet
Copy link
Member Author

baywet commented May 17, 2022

thanks for the update, try out x => x.QueryParameters.Status = new string[] { "pending"} instead. We've made a recent change based on community feedback to group the request configuration (parameters, headers, options) on a single object.

@PureKrome
Copy link
Contributor

PureKrome commented May 17, 2022

✅ Ok nice! things are working. Really appreciate the hand-holding, baby steps 😊


Alright, so I've had another play and .. I think this is starting to look good!

My first thing I noticed was the AdditionalData which made me get flashbacks to our previous discussion above ...

... which I then noticed, I didn't answer your last questions!! Zomg, I'm so sorry. I'm sure it's too late now because decisions have probably been made, but i'll give it a go. (if anything, you might be able to at least answer what decisions you've all made and how this can work with my thoughts)

Now, if we take a step back I think there are multiple concerns from your side:

  • You don't want the additional property to show up in the serialization result: this could be solved via changing the switch to a property + accessor method.

Correct. I don't want to see -any- AdditionalProperties in a serialized result. -ALSO- I don't want to see it in the POCO when I'm debugging or coding. So if it's an "accessor method" that would be lovely!

to clarify, it would then look like this:

public class Cat
{
   private IDictionary<string, object> _additionalData;

   public IDictionary<string, object> GetAdditionalData => return _additionalData;
   public void SetAdditionalData(string key, string value) { .. }
   
   // Do you remove or clear AdditionalData, via accessor methods.
}

This would mean:

  • We still have access to this data
  • This data is a bit less "obvious" (it's not always showing in debug dialogues or intellisence, etc)
  • It won't be serialized to json, etc. (assuming private fields aren't part of the serializer)
  • You'd like to avoid this property if you don't need it: that's already supported besides a bug Parsable interface should be splat #737, and the requirement from the spec is to set additionalProperties to false in the description. I don't think we want to go against the spec even if that means hurting the ease of use a little.

I don't really understand what this point is saying/communicating? But I do -not- want to go against a spec, at all. Spec > everything else. That's the source of truth .. even if the "truth" hurts. Now, if the spec says the default value is false, does that mean the default is to -not- copy any unmapped key/values into this dictionary? If that is correct, can we still obfuscate this property to a field + accessor method, then?

Also, where does one change this value from false to true?

Could someone post an link to an example of how this looks like, with Kiota? E.g. given 'this json result from some api' ... this key/value will end up in the AdditionalProperties dictionary ... and only if "this setting" is turned on.

  • You'd like to be able to use "native serializers" with the models: this one is a no, we've designed Kiota with an abstraction from serialization formats and libraries in mind because we've had way too many issues in the past when making an opiniated choice and tying our models to a specific library. We can however add vanity methods to make using our serializers easier.

Ah ok. This -definatley- needs to be in some bold, upper case, shouting text. If we (the developers) can't just use the common System.Text.Json or Newtonsoft nugets to serialize a POCO which was hydrated/deserialized when calling some API ... then we need to know this. Or have I misunderstood this, also?

EDIT: to clarify the last point about native serializers, I was meaning this:

image

image

see how the result is a POCO which Kiota has auto created? awesome! So now I would just like to serialize that to .. somewhere. Can I just use my standard, normal Json serializer's for this? Or is that what you are suggesting -> we should not and we should use something Kiota provides?

@baywet
Copy link
Member Author

baywet commented May 17, 2022

Additional Data

see the spec setting additionalProperties: false should remove that from the model.

About hiding it, what would this achieve? The trade-offs being more code generated and harder discovery (arguably the intent here). We've had multiple cases in Microsoft Graph of people asking "I see property X in the http snippet, but not in the model, where is it at?", IMHO the more obvious we can make this property, the better. This way if they are debugging and inspecting the object, they might discover the property they are looking for is there instead.

Default for that additionalProperties from the spec AFAIK is true (string), and the work to split it has been done since then.

Serialization of models

Yes, this is a design choice, models are not intended to be serialized via native libs directly as those native libraries have a number of different limitations we've learnt the hard way in our Microsoft Graph SDK experience.

Something like the following should be used instead

using var writer = requestAdapter.SerializationWriterFactory.GetSerializationWriter("application/json");
writer.WriteCollectionOfObjectsValues(null, result);
using var serializedStream = writer.GetSerializedContent();

@PureKrome
Copy link
Contributor

setting additionalProperties: false should remove that from the model.

setting this where in my code?

We've had multiple cases in Microsoft Graph of people asking "I see property X in the http snippet, but not in the model, where is it at?"

Why would the http result have a property, but the model doesn't? Should Kiota generate the model with the property?

Or is this because the model was generated with Kiota at .. say 1st may. and then a week later, that API was updated to return an extra property .. but of course, my model was generated a week ago and that property wasn't available then .. and still hasn't been updated. So then the developer would say: "I see property X in the http snippet/response .. but not in the model?" ... so wouldn't the developer then just re-run Kiota to generate the model(s)? I feel like I'm really missing something, here.

IMHO the more obvious we can make this property, the better.

I guess we just need to agree to disagree. I don't want to see extra fields that aren't in the api response. It's so confusing. Just imagine all the people starting to use this and have -never- seen or heard about OpenAPi specs and AdditionalData and stuff. And then they first hit F5-Debug and check out the response from their api and go .. "huh? why are there all these AdditionalData things? What is this and how can i get rid of it?".

This way if they are debugging and inspecting the object, they might discover the property they are looking for is there instead.

Again, depends on which way you come at this from. Model <-> Api is 1:1 match .. versus .. model is behind/out of date with Api.

Yes, this is a design choice, models are not intended to be serialized via native libs directly

No probs. This needs to be shouted out from the roof-tops and with an example of why. I would have thought a lot of POCO's generated from an API would be pretty .. simple/basic. Again, I've not used MS Graph. So then the common json libraries can do a fine job. But if you're saying the generated POCO's that represent the API results are complex, which break stuff .. then yeah .. would be kewl to see some examples so we go 'a-ha.. gotcha'.

@baywet
Copy link
Member Author

baywet commented May 17, 2022

setting this where in my code?

It leaves in the OpenAPI description, you'd have to work on your own copy of pet store.

Why would the http result have a property, but the model doesn't? Should Kiota generate the model with the property?

It's both to support new properties showing up in the API without having to re-generate/update the client and also to support other underlying protocols that do not necessarily describe the payload in its entirety because it varies on the context. A common example of that are OData annotations (@odata.type/nextlink/context/count...) that may or may not be here depending on headers, query parameters, the data set....

I guess we just need to agree to disagree

No worries, I just wanted to be transparent on the fact this is something that's not likely to change and that you have control over it by tweaking the OpenAPI description.

This needs to be shouted out from the roof-tops and with an example of why

Agreed, we're still figuring out where the kiota docs should go once we release. For now we have this on serialization, which leaves here. Would you mind sending up a pull request to add a paragraph about that?

@PureKrome
Copy link
Contributor

It leaves in the OpenAPI description, you'd have to work on your own copy of pet store.

AH! kewl. ok .. so is there any way I could manually say during generation (using kiota.exe, etc) to not generate AdditionalData ? Imagine all the scenario's where people wish to generate clients against other people's API's. We can't control their schema. But we can control what we generate. Is this possible or can this be possible?

@baywet
Copy link
Member Author

baywet commented May 17, 2022

Yes, that could be easily done.
The first step would be to add a command switch which defaults to true and is optional
Then, you'd need to add a configuration setting
After that, update the condition that adds the additional data manager
Lastly, don't forget to update the documentation

In case you want to send a pull request our way, we'd be delighted to get it merged :)

Note: the serialization libraries are already implemented in a way that they use the additional data manager when present or drop values that are not schematized when absent.

@PureKrome
Copy link
Contributor

Kewl! i'll create a new issue about having the ability to force AdditionalData to be excluded from model generation. Lets continue the convo over there.

@PureKrome
Copy link
Contributor

PureKrome commented Jul 9, 2022

Hi @baywet 👋🏻

I have a local branch with sme of my custom code, adding in the CLI option. ✔️

Problem is, I'm trying to compile against main and when I use that kiota.exe I get some weird generated-code errors when generating the new models:

image

To make sure it's not -my- custom code causing this, I tried just downloading the latest code (as a zip) from GitHub and then building that (dotnet build) and then using the created kiota.exe as my source file.

image

C:\Projects\Temp\kiota-main\src\kiota\bin\Debug\net6.0\kiota.exe --openapi https://petstore.swagger.io/v2/swagger.json --language csharp -o PetshopClient -c ApiClient --log-level information --clean-output
dbug: Kiota.Builder.KiotaBuilder[0]
      step 1 - reading the stream - took 00:00:01.2963934
dbug: Kiota.Builder.KiotaBuilder[0]
      step 2 - parsing the document - took 00:00:00.1427446
dbug: Kiota.Builder.KiotaBuilder[0]
      step 3 - create uri space - took 00:00:00.0012521
dbug: Kiota.Builder.KiotaBuilder[0]
      CreateRequestBuilderClass 00:00:00.0552805
dbug: Kiota.Builder.KiotaBuilder[0]
      MapTypeDefinitions 00:00:00.0091444
dbug: Kiota.Builder.KiotaBuilder[0]
      step 4 - create source model - took 00:00:00.0647277
dbug: Kiota.Builder.KiotaBuilder[0]
      41ms: Language refinement applied
dbug: Kiota.Builder.KiotaBuilder[0]
      step 5 - refine by language - took 00:00:00.0412805
dbug: Kiota.Builder.KiotaBuilder[0]
      step 6 - writing files - took 00:00:00.0648806
PS C:\Temp\__asdsaddas\Kiota-Petstore-Sample>

Are you finding that the current code in main is causing the same generation grief?

the Kiota project compiles fine .. but using that kiota.exe generates bad models .. at least against "Swagger Petstore" OpenApi.

@baywet
Copy link
Member Author

baywet commented Jul 11, 2022

Hi @PureKrome,
Thanks for reporting this, #1530 introduced a regression, addressed with #1733.

@PureKrome
Copy link
Contributor

Thanks @baywet!

@baywet
Copy link
Member Author

baywet commented Sep 8, 2022

Hey @PureKrome 👋
We just released a new version of kiota (2 in facts since we last "spoke"), which contains a lot of bug fixes. Would you mind giving it another try?
Thanks for the support from the early days!

@PureKrome
Copy link
Contributor

Absolutely! i'll get onto it this weekend 👍🏻

@PureKrome
Copy link
Contributor

PureKrome commented Sep 12, 2022

@baywet 👋🏻 G'day! Okay, so I've created this DRAFT PR over in the samples repo.

It's dotnet only :(

I've actually done none of the other langs. I started with the golang tonight and sorta got stuck on the first bit of code trying to create an instance of the anon authenticate request.

So - I have some questions and what to see your thoughts:

Do you need me to do the golang/java/all-the-langs? (i don't have a mac, though) Or can I just keep on going and try and ask some lang-specific questions here? (I think i'll get more ❤️ here than StackOverflow where I'll either get abused for 'no-homework-questions-here' or that I'll get downvoted for who-knows-what.

Also, with my current PR, I also tried to do an example where I hit this Swagger Petstore endpoint:

curl -X 'GET' 'https://petstore.swagger.io/v2/store/inventory' -H 'accept: application/json'

but I have no idea how to do this with the generated SDK? It's like ... it didn't work? (compiled, but no result).

image

(and the console.writeline output)
image

So I'm sorta confused :(


EDIT/UPDATE: Oh! I also forgot to ask this question, also: Why does the generator automatically include XXXX when the class isn't using it? This means I need to either:

  • manually edit this file (urgh yuck. we don't want to touch/change auto-generated files)
  • make sure this nuget is added to the project. Which is silly, if I'm never going to use/reference that?

image

@baywet
Copy link
Member Author

baywet commented Sep 12, 2022

Thanks for the update, and the continuous work on this.

Which languages

Dotnet and TypeScript would be enough. Go and Java better. PHP and Python great additions. I'm not expecting swift/ruby as these languages are very early in their development. I'm not expecting CLI because the content is really similar to dotnet.

Pet store inventory

The description doesn't say much besides it'll return an object which supports additional data. So I'd expect you'll find the additional data in the additional data manager. (well, if you didn't disable it during generation).

Dependencies

I'm not sure why the IDE is reporting this one as unused since, it's actually being used https://github.com/microsoft/kiota-samples/pull/993/files#diff-e7516bc6ca3b86ee17272ffd8e5cbc7ccb1ee91b8815b2c2825d02ee4e720568R43

@PureKrome
Copy link
Contributor

Thanks @baywet

Dotnet and TypeScript would be enough

I'll def start with that as a minimum.

The description doesn't say much besides it'll return an object which supports additional data. So I'd expect you'll find the additional data in the additional data manager.

oh ... the addition data thingy again :(

image

Yep - was that :( damn. but okay! it's working as intended then.

I'm not sure why the IDE is reporting this one as unused since, it's actually being used

Ah good point! It was 'hidden' so I didn't see that. Is it needed, though? Like, sure I can remove it manually but is there any reason to have it there? Can the code know, by the produces field/value?

image

@baywet
Copy link
Member Author

baywet commented Sep 13, 2022

Ah good point! It was 'hidden' so I didn't see that. Is it needed, though? Like, sure I can remove it manually but is there any reason to have it there? Can the code know, by the produces field/value?

Right now this is de-corelated to give people control through the serializer and deserializer options

The generator doesn't know that "this serializer handles Json" so it doesn't use that option to select which mime type it's looking at. And in the other way around the generator doesn't use the mime types to select which serializers it's going to include. You have to provide those manually.

Additionally, you also have an option to instruct the generator which mime types to consider "structured" (and generate models for) and which ones to just map to stream.

@PureKrome
Copy link
Contributor

So are you saying that, if I know that my OpenApi will only be returning json payloads, I can do:

kiota.exe -m application/json <.....>

?

also, the docs have the wrong example, for this?

image

@baywet
Copy link
Member Author

baywet commented Sep 14, 2022

correct, you'd have to do 3 things actually:

  1. pass the serializers argument, to only include the JSON dependency
  2. pass the deserializers argument, to only include the JSON depdency
  3. pass the structured mime types argument, to only consider JSON as structured

The source of the page is here if you want to correct it. https://github.com/microsoft/kiota/blob/main/docs/using.md

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation generator Issues or improvements relater to generation capabilities. hacktoberfest help wanted Issue caused by core project dependency modules or library
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

3 participants