microsoft / kiota

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

Offer helper to deserialize API response to an entity #3406

Closed waldekmastykarz closed 1 year ago

waldekmastykarz commented 1 year ago

We should offer a helper method to help developers deserialize API response to an entity.

While in most cases we handle deserialization in the SDK, there are some exceptions to it:

While the first issue is exceptional, it's still blocking developers who need to wait for us to address the issue and release a new client.

Right now, deserializing API responses to entities require the following code:

var bodyBytes = System.Text.Encoding.UTF8.GetBytes(body);
using var ms = new MemoryStream(bodyBytes);
var parseNode = ParseNodeFactoryRegistry.DefaultInstance.GetRootParseNode("application/json", ms);
var operation = parseNode.GetObjectValue(ConnectionOperation.CreateFromDiscriminatorValue);

This code isn't intuitive and requires quite some Kiota knowledge. We should aim to make it as simple as:

var operation = JsonSerializer.Deserialize<ConnectionOperation>(body)>
baywet commented 1 year ago

Hi @waldekmastykarz , Thanks for suggesting this.

Assigning to @sebastienlevert to refine the developer experience here.

While a static method on the JsonSerializationWriter is easy, we need to think about a couple of things:

baywet commented 1 year ago

Just capturing notes from our meeting now

An alternative could be to do binding instead of requesting the factory:

var fruit = new Fruit(); // user creates the instance
KiotaMagicMapper.Bind(fruit, "application/json", "{\"color\":\"red\",\"name\":\"apple\"}"); // standard deserialization code is being used
// would call ===>>> JsonParseNode.GetObjectValue(_ => instance); we don't need to ask for the factory since we already have the instance

This works well for objects, but starts breaking down for collections

waldekmastykarz commented 1 year ago

What if we made it event easier? Each request builder is tied to an entity, right? What if we'd let developers pass either an entity or an object and if it's just an object (like what you get when you deserialize a JSON string), we create a new instance of the entity and bind the object to it? That way, developers don't need to learn any special concepts to use Kiota.

baywet commented 1 year ago

In most languages the serialization and deserialization information is tied to the object itself (class implements an interface). We won't know what to do with the instance if it doesn't carry its serialization information.

That's because kiota leverages auto serialization to avoid relying on reflection. It's a bit less flexible but it's faster.

waldekmastykarz commented 1 year ago

Right, but if we pass the entity type as part of the serialization then we could create a new instance of it and then overlay the regular object on top of it, right, say:

Kiota.Magic.Deserialize<Fruit>("application/json", "{{json string goes here}}")

and then inside Deserialize we create a new instance of Fruit because we've got a guard that it needs to be new()

Thinking of it, since Graph is all about JSON, we might skip the content type to simplify it further. I know that in that sense Kiota is generic and works with any API and content type, but specifically for Graph, we could layer abstraction over it, to make it easier for devs.

baywet commented 1 year ago

and then inside Deserialize we create a new instance of Fruit because we've got a guard that it needs to be new()

This only works for dotnet, we've generally avoiding designing solutions that only work for a single language.

sebastienlevert commented 1 year ago

This is the most challenging (and most important) aspect of these designs. It needs to support existing (and even future) languages. This would not be a language-specific implementation.

I understand the concern and this might be something to be simplified on every language we support Graph (building another wrapper simplifying the experience). I still feel we should fix it upstream and make it as developer-friendly as possible before adapting the approach to Graph, for instance.

Also in the initial proposal from @baywet we are missing collection support. What would be missing to make this support scalar and collections?

waldekmastykarz commented 1 year ago

This only works for dotnet, we've generally avoiding designing solutions that only work for a single language.

I understand that and appreciate that it's hard. Still, it shouldn't come at the cost of developer experience.

This is the most challenging (and most important) aspect of these designs. It needs to support existing (and even future) languages. This would not be a language-specific implementation.

The problem with this approach is settling for the lowest common denominator leading to a subpar experience for all developers, rather than making use of language-specific features that enrich developer experience. I wonder how many developers use several different languages and seek consistency across all of them vs. use one language and would like to fully benefit of its features.

baywet commented 1 year ago

To clarify the point: I don't think we're saying not to take advantage of language specific feature when we can so we can deliver the best of class experience. We already do this (e.g. indexers in dotnet). But we need to design with solutions that work for every language while delivering an acceptable experience. Taking this scenario as an example, let's say we solved the issue for dotnet (which we haven't entirely) by using the new generic type constraint. What would the equivalent look like in Java/TypeScript/etc...? Is it radically different? If so should we align dotnet to that design even though dotnet has a better solution for consistency?

waldekmastykarz commented 1 year ago

My gut feeling tells me, that we should make the best use of language features and native patterns at the cost of inconsistency across languages. If we chose this route, then we'd need to investigate each language and its capabilities, which means more work and deviation but ideally a significantly better developer experience.

baywet commented 1 year ago

Part of the reason why we're not able to come up with a simple solution here is because we're not scoping/framing the problem well enough. I'm going to try to attempt that.

We want people to be able to easily (de)serialize models to/from a persistent storage. One of the keywords here is models ([collection of]? enums or classes/interfaces), which excludes [collections of]? primitives, for which they can use their favorite library to do so.

If we focus on object models for a second, the auto-serialization infrastructure does multiple things for us:

Now that we have a better understanding of what the infrastructure does for us today, let's add a couple of requirements:

With all that in mind, here is my suggestion: binding is now going to work for TypeScript specifically.

// Deserializing an object
Kiota.Magic.Deserialize("application/json", "{{json string goes here}}", Fruit.CreateFromDiscriminatorValue);
// Deserializing a collection of object
Kiota.Magic.DeserializeCollection("application/json", "{{json string goes here}}", Fruit.CreateFromDiscriminatorValue);
// Deserializing an enum value
Kiota.Magic.DeserializeEnum("application/json", "{{json string goes here}}", FruitKind.Parse);
// Deserializing a collection of enum values
Kiota.Magic.DeserializeEnumCollection("application/json", "{{json string goes here}}", FruitKind.Parse);
//Serializing an object
Kiota.Magic.Serialize("application/json", streamReference, myFruit);
//Serializing an object collection
Kiota.Magic.SerializeCollection("application/json", streamReference, myFruits);
//Serializing a enum value
Kiota.Magic.SerializeEnum("application/json", streamReference, myFruitKind);
//Serializing a collection of enum values
Kiota.Magic.SerializeEnumValues("application/json", streamReference, myFruitKinds);

Note: I'm using different method names for serialize/deserialize as in some languages we'll need different method names (no overloads), but in dotnet, I believe we could sensibly have the same method name.

Thoughts?

sebastienlevert commented 1 year ago

This is a great investigation @baywet!

I think Kiota.Serialization.* should be part of our abstractions and would rely on the downstream serializers. So I would absolutely see something like this:

// Deserializing an object
Kiota.Serialization.Deserialize("application/json", "{{json string goes here}}", Fruit.CreateFromDiscriminatorValue);
// Deserializing a collection of object
Kiota.Serialization.DeserializeCollection("application/json", "{{json string goes here}}", Fruit.CreateFromDiscriminatorValue);
// Deserializing an enum value
Kiota.Serialization.DeserializeEnum("application/json", "{{json string goes here}}", FruitKind.Parse);
// Deserializing a collection of enum values
Kiota.Serialization.DeserializeEnumCollection("application/json", "{{json string goes here}}", FruitKind.Parse);
//Serializing an object
Kiota.Serialization.Serialize("application/json", streamReference, myFruit);
//Serializing an object collection
Kiota.Serialization.SerializeCollection("application/json", streamReference, myFruits);
//Serializing a enum value
Kiota.Serialization.SerializeEnum("application/json", streamReference, myFruitKind);
//Serializing a collection of enum values
Kiota.Serialization.SerializeEnumValues("application/json", streamReference, myFruitKinds);

Also to confirm with your reasonning, where you put {{ json string goes here }} am I right to think that it should be {{ string of the body goes here }} in scenarios where this is not JSON? We're pushing json in all examples, but I feel this could be supported the exact same way for other serializers / deserializers.

To your questions:

baywet commented 1 year ago

To the other formats: yes, this is why we ask for the mime type as well. This way we could rely on the infrastructure to parse or serialize to any format.

Great, I think we have a plan. I'd like @waldekmastykarz 's opinion on the experience here as well in case we missed anything.

sebastienlevert commented 1 year ago

I feel we're convenient enough in this setup to offer a good developer experience (one-liner + the right imports) while offering the right set of rail guards. Thanks for thinking deeply in this space @baywet! Would also love to have @andreaTP thoughts on this topic!

andreaTP commented 1 year ago

Being able to serialize/deserialize using the Kiota infrastructure, at least from/to Json in a convenient way is truly desirable.

I'll try to have a look at the details of this proposal tomorrow.

waldekmastykarz commented 1 year ago

I'd like us to simplify it further and avoid exposing Fruit.CreateFromDiscriminatorValue, Fruit.Parse etc. to client consumers. These are new concepts that developers likely aren't familiar with, that require them to understand the difference and when to use which and steepen the learning curve. Is there a way for us to infer those methods from the types they pass, so that we can use something like:

Kiota.Magic.Deserialize<Fruit>("application/json", "{{json string goes here}}")

which is closer to the standard serializations folks are already familiar with?

andreaTP commented 1 year ago

Looking at the proposal I think that it's still including a bunch of details that can be hidden for the user:

Please note that I'd expect Json to be the only "required" serialization format, at least in this first interaction.

Kiota.JsonSerialization.Deserialize(inputStream, Fruit.class)
Kiota.JsonSerialization.Serialize(myFruit)

This is the kind of API I would aim for, I'm sure that this proposal doesn't cover all the edge cases yet, but is the best I can put together on my phone :-) and I'm convinced that it captures the gist of it.

Overall more opinionated and less control for the user in favor of a minimal and convenient API.

baywet commented 1 year ago

the challenge with those proposals is that they'll require reflection.

And I already had shared this requirement:

we'd like to avoid reflection, due to the performance impacts and due to the reliability issues (no compile time validation, needed members might be trimmed, etc...)

Besides the performance hit, it might not even be possible to do this in some languages (the dynamically typed ones are more at risk here).

But we could try to provide both overloads (the one using reflection and the one that doesn't) when possible, it's going to add a lot of API surface (examples bellow), but should be doable.

As for not asking for the content type, we could add local "overloads" in each serialization library that just happen to set the constant for people.

Here is the example for the object scenario in dotnet:

// Deserializing an object (this lives in abstractions)
Kiota.Serialization.Deserialize("application/json", "{{json string goes here}}", Fruit.CreateFromDiscriminatorValue);
// Deserializing an object (also lives in abstractions, relies on the previous one, uses reflection
Kiota.Serialization.Deserialize<Fruit>("application/json", "{{json string goes here}}");
// Deserializing an object (this lives in the Json serialization package, relies on the one in abstractions, no reflection)
Kiota.JSonSerialization.Deserialize("{{json string goes here}}", Fruit.CreateFromDiscriminatorValue);
// Deserializing an object (this lives in the Json serialization package, relies on the second one in abstractions, uses reflection)
Kiota.JsonSerialization.Deserialize("{{json string goes here}}");

And we'll get all of the above for each scenario in my previous answer, doubled up to support stream as well. => 32 (deserialize) + 8 (serialize) new methods in the API surface. (x 7 languages). Hence my initial suggestion to start with only the ones that do not require reflection and see what feedback we get.

andreaTP commented 1 year ago

Thanks for iterating on the proposal Vincent! Much appreciated!

the challenge with those proposals is that they'll require reflection.

I think this can be solved without reflection, but let's doublecheck:

The proposed signature,

Kiota.JSonSerialization.Deserialize("{{json string goes here}}", Fruit.GetType());

can be implemented as an overload similar to:

public T Deserialize(string jsonString, Type classType) {
   var mappings = new Dictionary<Type, Function<.., ...>> { { Fruit.GetType,  Fruit.CreateFromDiscriminatorValue } }
  Deserialize(jsnoString, mappings.TryGetValue(classType))
}

would you consider adding this as an "optional" feature, like the BackingStore?

baywet commented 1 year ago

Thanks for the suggestion. The challenge with this approach is that it'll effectively make the models non trimmable (dotnet, typescript, etc...) as ALL models will be referenced in this method, as soon as this method is referenced by the client code. This can have serious impacts on the resulting "binary" size. Plus GetType makes use of some level of reflection as well, though we could circumvent that by using the type name in the mapping to some extent.

andreaTP commented 1 year ago

Thanks for elaborating! What do you think about making this entirely separate from the "main" generation, e.g. a different CLI command kiota generate-serde-helpers ?

pschaeflein commented 1 year ago

Let me address to two scenarios differently:

  • there's a new endpoint that's not yet in the SDK

If "the SDK" means Graph, then I believe it is safe to assume the developer has an understanding of the Graph response (by using Graph Explorer). They would likely know the Model type (or be creating one if it is new).

What they likely do not know is the SDK inner workings. Here is how I would approach it:

  1. I can see there are methods to get a request information object. So I'm going to create one, passing in the new endpoint. (Using C# since that is what I know.)

    var endpoint = $"{graphClient.RequestAdapter.BaseUrl}/somenewthing/{thing1}";
    var ri = new RequestInformation() { URI = new Uri(endpoint), HttpMethod=Method.GET };
  2. The GraphServiceClient object that I have already knows how to authenticate and send the request. So I look at the definition of GraphServiceClient, and the only "thing" I see that might help is the RequestAdapter. The SendXXX methods of that class require a parseable factory. Huh?

  3. I reviewed the docs for the Graph SDK with no help.

  4. Since I see Kiota in the namespace for this factory, I can look at its docs. The page on Request builders looks promising, but since this is the new endpoint scenario I have to build one myself. The BaseRequestBuilder is abstract so I can't use that. Now I need to go learn how to build one of those. Not likely to happen.

4a. I just realized I have a RequestInformation object, not a RequestBuilder. Never mind...

  1. I reviewed the Kiota Serialization and Models pages, and I am a bit overwhelmed. I just want to send a request. Poking around, I see a JsonParseNodeFactory

    var factory = new Microsoft.Kiota.Serialization.Json.JsonParseNodeFactory();
    var t = await graphClient.RequestAdapter.SendAsync<SomeNewThing>(ri, factory);

    This won't compile because: There is no implicit reference conversion from 'SomeNewThing' to 'Microsoft.Kiota.Abstractions.Serialization.IParsable'.

I now understand why this scenario was in the original post. But, even if you give me a way to deserialize the response, how do I send the request? I need an IParseable object to use the RequestAdapter.

The v4 SDK (again, .Net) had providers exposed off the GraphServiceClient, so I could just use them:

var hrm = new baseRequest(endpoint,graphClient).GetHttpRequestMessage();
await graphClient.AuthenticationProvider.AuthenticateRequestAsync(hrm);
var operationResponse = await graphClient.HttpProvider.SendAsync(hrm);

I'm struggling to see how I can avoid just making the HttpRequest myself (authenticating it "manually") and deserializing the response to a custom Model. But for devs who just use the ICredential class instead of understanding Entra token acquisition, they would be stuck. Either call for help or spend a lot of time figuring it out.

In summary, I believe this scenario needs to solve more than just serialization.

  • developers are building a new middleware/handler where they work with raw requests and responses and need to parse the response body to an entity

In this scenario, I would expect something similar to the way we deserialize everywhere else: JSON.parse() or JsonConvert.DeserializeObject(stream).

My preference would be something close to Andrea's suggestion: Kiota.JsonSerialization.Deserialize(inputStream, Fruit.class). I understand you need to figure out the reflection/trimming bits, but that signature is closer to what I would expect to find.

baywet commented 1 year ago

Thanks for setting the context here @pschaeflein

I don't think the "there's an endpoint that's not in the SDK yet" is a valid scenario here because (and yes, I didn't catch that in the initial message):

As for making arbitrary requests, I'll reiterate: the RequestInformation/Request Adapter API surface was never meant to be used by humans, it only supports the generated code. People should be using the native client for those scenarios, augmented with the set of middleware handlers we provide. What's missing here is an optional middleware that'd take an access token provider to handle the authentication automatically for people. Maybe we can queue that separately @sebastienlevert .

With this, I think we should be able to scope this issue to "I have/want a JSON representation of a model, without having to send a request, how do I do that more easily than done today?" (e.g. persisting things to storage, etc...) Hence the proposals.

I think your feedback confirms the fact that consumers don't want to figure out the factory pattern if we can avoid them having to do so. Some might for performance reasons, so we should not lock them out, but most don't.

Lastly, to @andreaTP 's last comment, I think adding a separate command adds to the complexity and doesn't make things easier. If people are struggling to find the factory method, how are they going to know that they need to generate additional assets?

andreaTP commented 1 year ago

With this, I think we should be able to scope this issue to "I have/want a JSON representation of a model, without having to send a request, how do I do that more easily than done today?" (e.g. persisting things to storage, etc...) Hence the proposals.

I agree that this is the end goal of this effort, I have been there, loading Json representations from disk to feed the http calls and felt the gap.

how are they going to know that they need to generate additional assets?

Again here you are right, but, as anything in software, this is gonna be a tradeoff. In this case we are talking about a convenience for developers, and, I believe that having to balance this requirement with code size is not going to fly...

Have we considered the effort of building it as an(almost?) independent, stand alone, library(even using reflection and stuffs)?

baywet commented 1 year ago

A separate library just for this functionality feels like a lot of ceremony, both for us as maintainers but also for the consumers from a discovery standpoint.

waldekmastykarz commented 1 year ago

I don't think the "there's an endpoint that's not in the SDK yet" is a valid scenario here because (and yes, I didn't catch that in the initial message):

  • Kiota's value is to be able to refresh clients on the spot.
  • Microsoft Graph customers get a refresh weekly, which is as often as the service gets updated. (2 days lag)

Looking at it from our developers' point of view: I'm working on a project and came across something that's not in the SDK (we just found one the other day where creating external connection was described using an incorrect verb in API description). Now I'm blocked for 2 days (not to mention the time to create an issue in the repo to report the problem and have it triaged and sorted out by the right person which will take longer if the issue stems from a mistake in the API description that needs to be fixed first) with no way out. Our developers won't wait for two days and will look for other ways to get the job done, including not using our SDKs (that's always an option they have).

I fully agree that something missing in our SDKs is highly exceptional and we aim to always reflect what's on the API. That said, when it happens, and we offer developers no way to solve it, we risk losing them and never coming back. I'd rather have an escape catch that they can use in such cases, so that they can keep using our tooling.

baywet commented 1 year ago

@waldekmastykarz, I understand the pain here, and I think this is largely outside of this discussion. As you're aware, we have a number of efforts to drive better metadata quality and close those gaps across the boards (SDKs, snippets, etc...): TypeSpec to produce better metadata to begin with (long term), XOD in the metadata repo (short term patches), funneling feedback to service teams (arguably, we had mitigated success with this approach), raptor to catch miss alignments between snippets-metadata-sdks....

Please make sure you create a new issue in the metadata repository when you come across those gaps so we can address them one way or another.

But if we make the assumption that there'll always be metadata gaps in the case of Microsoft Graph (again, I believe this is mostly a Graph concern), sure, we need to enable arbitrary queries from the native client, while reusing most of the infrastructure we built up, and reusing models when available for the scenario (To Paul's point). This takes:

  1. A way to get a native http client with our middleware infrastructure ✔️
  2. A way to authenticate the requests (possible today, but complex, maybe @sebastienlevert can drive an additional work item)
  3. A way to (de)serialize generated models into/from a payload (what we're enabling here).

With those parts in place, I believe we'll enable most if not all scenarios we've talked about here.

pschaeflein commented 1 year ago

You also need:

2.5. A (simpler) way to send the request thru the configured pipeline.

In the v4 .Net client, it is graphClient.HttpProvider.SendAsync(). The v5 equivalent is way more complex. (Is that the "native http client" bit? If so, I couldn't find that in the docs.)

baywet commented 1 year ago

We have a huge gap in the documentation on the kiota side, that's for sure.

But effectively for dotnet getting a native client augmented with the middleware looks something like that

using var client = KiotaClientFactory.Create(); // when talking to graph, use GraphClientFactory instead from graph core to get additional handlers
// we're probably missing a middleware for authentication as stated in 2
using var response = await client.GetAsync("url");
//... do something with the response
baywet commented 1 year ago

I started working on this over here

Here are a the methods for deserialization:

Also this is all in abstractions since all of them are just helper methods, and it'll make it so people can also use the Json shorthand with any implementation this way.

I don't think we actually need to do the enum as:

So here are a couple of questions for you fine folks:

  1. What do you think about the naming of static classes?
  2. What do you think about the naming of of the methods?
  3. What do you think about not including enums as the entry point for now?
  4. Should we group all the Serialize/Deserialize methods under the same static class or keep them separate?
sebastienlevert commented 1 year ago
  1. I'd prefer them without helpers as I don't think it provided any value. So JsonDeserialization.Deserialize(). The namespace could have the Helpers in it.
  2. Serialize + Deserialize are natural for developers. I'd keep them like that.
  3. Good question. I think we can deal without them for now, but I'd love to learn more about their use in the wild for the entry point. I feel this could be a use case (/getAnimalType) and return an enum value. What's the cost of this and how much can we infer like we do here with the Reflection we are implementing?
  4. I think they should be part of the same namespace, different classes.
baywet commented 1 year ago
  1. the challenge with not having the helpers (or something else) in the class name is the fact that we have namespace names with "Serialization" segments. This is going to create headaches for people when they try to use the class as the compiler won't know whether they are trying to fully qualify a class, or making a reference to the class we're building. We need to have a prefix, or a suffix to avoid these issues.
  2. ok
  3. cost is not super high, it's just they don't add value. (except maybe for the version with reflection)
  4. ok
sebastienlevert commented 1 year ago
  1. Why not using the same naming conventions from JsonSerializer in .NET? JsonSerializer.Deserialize(). This would not conflict, right?
  2. I assume the biggest advantage is to use the same mechanism compared to a "totally" other way. Similar to handling collections right. Maybe we should know that it's either an object, a collection or an enum? What are the downsides of having a single approach (and set of methods) for all of these scenarios?
pschaeflein commented 1 year ago
  1. I would prefer "KiotaConvert.Deserialize", but as long as it is documented I could live with it.
  2. These are fine
  3. As long as the properties of Entities are set to the correct Enum value, that's fine.
  4. One class. JsonConvert has both Serialize and Deserialize. I would expect it to match that.
baywet commented 1 year ago
  1. (seb) Using the same name as known static classes from other libraries/the runtime is a bad idea. It'd require people to alias their usings or to fully qualify things when both are in scope. This would be a poor experience. (paul) yep, I'm happy with that name, or maybe KiotaSerializer to blend with what Seb suggested?
  2. Seb, you messed up the numbering.
  3. (paul) yes that's going to be the case as everything after the root type will rely on auto-serialization. (seb with the right number) overloads could work in dotnet, will not in a number of languages.
  4. ok (but contradicts Seb's initial opinion)
sebastienlevert commented 1 year ago

I numbered them well, but them markedown decided that a list with only 1 and 3 was not going to make it!

  1. I meant a "similar" approach, but definitely not reusing the .NET one. My bad. I'm happy with KiotaSerializer to mimic .NET.
  2. Yes, I messed up.
  3. Then, native seems to be reasonable.
  4. If we use a single KiotaSerializer then I'm with Paul.
baywet commented 1 year ago

Thanks everyone for the great feedback. I haven't had time to incorporate it just yet, but I worked on the serialization scenarios:

Granted that SerializationHelpers will become KiotaSerialization and JsonSerializationHelpers will become KiotaJsonSerialization, what do you think about the method names?

I had to add the AsStream and AsString since they have the same arguments. And I was able to make it work with the same name for collections and single objects (which arguably is not consistent with the deserialization from out last discussions)

pschaeflein commented 1 year ago

Those are good.

waldekmastykarz commented 1 year ago

I suggest that we don't use Kiota in the class name. Not sure how the helpers will be exposed to the user but:

I suggest we use class names without Kiota

pschaeflein commented 1 year ago

if they're coming from a Kiota namespace, then we're unnecessarily repeating Kiota in the full name

Is that really a big deal? I can only speak to C# and Typescript, but I cannot remember the last time I compared the using/import statements with the code in the body of a method. In fact, I only look at using/import when I first use a namespace. Having a repeated name in the namespace is way better than having to figure out the alias syntax of using. (Which, in fact, I had to look up right now to know what the name of the thing was.) I often use the fully-qualified name in the method to avoid this situation.

if they're coming from the client namespace, then we're leaking client implementation details to client consumers (I doubt folks who use our clients care how we build them internally)

If I come across this scenario, I WANT to know the implementation details. I'm only here because the client does not do what I need.

Just my thoughts...

sebastienlevert commented 1 year ago

I doubt folks who use our clients care how we build them internally

This is where I think we need to leak some of it. A library that is built with Kiota needs to be configured and customized with Kiota concepts anyways. In this case, I feel KiotaSerialization is the right place to start.

baywet commented 1 year ago

Thanks everyone for the great input here! I think we have everything we need, I'll move on the replicating to other languages now.

baywet commented 1 year ago

For go we won't have the reflection methods as it's impossible due to the way things are compiled as far as I understand more information

baywet commented 1 year ago

I'm also not going to add the string overloads as this is a trivial operation in Go that most developers are familiars with (string to byte[] and vice vera)

baywet commented 1 year ago

Additionally, because Go (and TypeScript) has a notion of static functions (not attached to a class/struct), the naming I went with will be the following:

The alternative being to use the same name as the functions that require passing the content type, but being in a sub-package. This would work well for Go as you need to prefix symbols with their package import name (like json.Serialize(...)) but breaks down a little for TypeScript as if somebody wants to use both methods in the same scope, they'll have to declare aliases to avoid collisions.

baywet commented 1 year ago

I just submitted the PR for TypeScript. An additional note is that for both serialization and deserialization we need to as for the serializer and the deserializer function for the model. This is because interfaces get erased at runtime and there's no good way to get these methods by reflection.

baywet commented 1 year ago

closing since it's been implemented in 4 canonical languages and related issues have been created for other languages.

heinrich-ulbricht commented 10 months ago

For anybody finding this issue while searching for a way to deserialize JSON to Kiota object model instance.

Waldek's approach did NOT work for me:

var bodyBytes = System.Text.Encoding.UTF8.GetBytes(body);
using var ms = new MemoryStream(bodyBytes);
var parseNode = ParseNodeFactoryRegistry.DefaultInstance.GetRootParseNode("application/json", ms);
var sitePage = parseNode.GetObjectValue(SitePage.CreateFromDiscriminatorValue);

It throws:

An exception of type 'System.InvalidOperationException' occurred in Microsoft.Kiota.Abstractions.dll but was not handled in user code: 'Content type application/json does not have a factory registered to be parsed'

Further googling lead me to this, which works:

var jsonParseNode = new JsonParseNode(JsonDocument.Parse(body).RootElement);
var sitePage = jsonParseNode.GetObjectValue<SitePage>(SitePage.CreateFromDiscriminatorValue);

Source was over here, at a similar issue: https://github.com/microsoftgraph/msgraph-sdk-dotnet/issues/1708#issuecomment-1467658543

Developers gonna develop. As long as I can google something and it works :stuck_out_tongue:

baywet commented 10 months ago

if you new up a client before calling ParseNodeFactoryRegistry.DefaultInstance.GetRootParseNode("application/json", ms) waldek's approach will work. And now we have facilitator methods, so you can directly call KiotaJsonSerializer.Deserialize<SitePage>(body) instead of the four lines.

heinrich-ulbricht commented 10 months ago

@baywet Ah! Was not sure if that arrived in C#, yet. Looks good.

But first-use experience for me also is this:

Exception has occurred: CLR/System.InvalidOperationException
An exception of type 'System.InvalidOperationException' occurred in Microsoft.Kiota.Abstractions.dll but was not handled in user code: 'Content type application/json does not have a factory registered to be parsed'
   at Microsoft.Kiota.Abstractions.Serialization.ParseNodeFactoryRegistry.GetRootParseNode(String contentType, Stream content)
   at Microsoft.Kiota.Abstractions.Serialization.KiotaSerializer.Deserialize[T](String contentType, Stream stream, ParsableFactory`1 parsableFactory)
   at Microsoft.Kiota.Abstractions.Serialization.KiotaSerializer.Deserialize[T](String contentType, String serializedRepresentation, ParsableFactory`1 parsableFactory)
   at Microsoft.Kiota.Abstractions.Serialization.KiotaSerializer.Deserialize[T](String contentType, String serializedRepresentation)
   at Microsoft.Kiota.Abstractions.Serialization.KiotaJsonSerializer.Deserialize[T](String serializedRepresentation)
   at WikiTraccs.Tests.ConversionTests.TestOptionalTransformations.JsonToSitePage(String body)

This is in unit tests, so there is no client yet.

Running this before makes it work:

_ = new GraphServiceClient(new Mock<IAuthenticationProvider>().Object, null);

No one liner anymore, but nevertheless nice, that KiotaJsonSerializer. Will put the client init somewhere static. Thanks for the heads-up.