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

Offer helper to deserialize API response to an entity #3406

Closed
waldekmastykarz opened this issue Oct 3, 2023 · 52 comments
Closed

Offer helper to deserialize API response to an entity #3406

waldekmastykarz opened this issue Oct 3, 2023 · 52 comments
Assignees
Labels
Milestone

Comments

@waldekmastykarz
Copy link
Contributor

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:

  • there's a new endpoint that's not yet in the SDK
  • 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

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 baywet added the enhancement New feature or request label Oct 3, 2023
@baywet baywet added this to the Kiota v1.9 milestone Oct 3, 2023
@baywet
Copy link
Member

baywet commented Oct 3, 2023

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:

  • which serialization instance is the static method going to use? (make a new one? use the one in the registry?)
  • what are we supposed to do for collections and scalar properties? (expose multiple methods? use overloads with complex generic type constraints that are not going to be portable to other languages?)
  • how are we supposed to get the factory method for the type? (ask it to the end user? use reflection that's not going to be portable?)
  • what about the serialization scenario? (as been asked in the past)
  • what about the difference serialization formats?

@baywet
Copy link
Member

baywet commented Oct 6, 2023

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

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

baywet commented Oct 9, 2023

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

waldekmastykarz commented Oct 10, 2023

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

baywet commented Oct 10, 2023

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

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

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

baywet commented Oct 11, 2023

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

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

baywet commented Oct 24, 2023

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:

  • discriminated downcast support: e.g. deserializing to a User even though the expected type was Directory Object because of the OData Type property in the payload. This relies on the CreateFromDiscriminator method.
  • union/intersection types wrapper support: deserializing to the right member type. This relies on the CreateFromDiscriminator method.
  • deserializing the object type: relies on the getFieldDeserializers method (instance method in most languages, static function for TypeScript)
  • serializing the object type: relies on the serialize method (instance method in most languages, static function for TypeScript).
  • deserializing a enum member: relies on the parse static method.
  • serializing a enum member: relies on the serialize static method.

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

  • 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...)
  • do we need to support discriminated downcast? @sebastienlevert (I think it's a false issue because of the other requirements)
  • do we need to support union/intersection types wrappers? (same reasoning)

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

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:

  • do we need to support discriminated downcast? Yes as it relies on the discriminator anyways.
  • do we need to support union/intersection types wrappers? Yes as the type you'd pass is a union type anyways.

@baywet
Copy link
Member

baywet commented Oct 25, 2023

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

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

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

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

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

  • "application/json" having to pass it as a string look error prone as this is gonna be user, hand-written code. I think that directly exposing a specific API would be great e.g. Kiota.JsonSerialization...

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

  • CreateFromDiscriminatorValue and Parse seems like implementation details and I would avoid asking the user to invoke them directly. What we can do is to have a Dictionary<Class, DeserializationFunction> generated and the final API can look something similar to:
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
Copy link
Member

baywet commented Oct 27, 2023

I started working on this over here

Here are a the methods for deserialization:

  • DeserializationHelpers.Deserialize() x 4 (content type, string/stream, factory?)
  • DeserializationHelpers.DeserializeCollection() x 4 (content type, string/stream, factory?)
  • JsonDeserializationHelpers.Deserialize() x 4 (string/stream, factory?)
  • JsonDeserializationHelpers.DeserializeCollection() x 4 (string/stream, factory?)

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:

  • the enum values are usually part of a larger payload, and are not the entry point.
  • when enum are the entry point, they are just a text representation at the end, and people can use the Parse/ToString methods directly.
  • if demand shows up for that scenario, we can always add it later.

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

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

baywet commented Oct 30, 2023

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

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

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

baywet commented Oct 30, 2023

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

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

baywet commented Oct 30, 2023

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

  • SerializationHelpers.SerializeAsStream x2 (contentType, single object/collection)
  • SerializationHelpers.SerializeAsString x2 (contentType, single object/collection)
  • JsonSerializationHelpers.SerializeAsStream x2 (single object/collection)
  • JsonSerializationHelpers.SerializeAsString x2 (single object/collection)

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

Those are good.

@waldekmastykarz
Copy link
Contributor Author

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

  • if they're coming from a Kiota namespace, then we're unnecessarily repeating Kiota in the full name
  • 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)

I suggest we use class names without Kiota

@pschaeflein
Copy link

pschaeflein commented Oct 31, 2023

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

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

baywet commented Oct 31, 2023

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

baywet commented Nov 1, 2023

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

baywet commented Nov 1, 2023

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

baywet commented Nov 1, 2023

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:

  • SerializeToJson
  • DeserializeFromJson

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

baywet commented Nov 2, 2023

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

baywet commented Nov 2, 2023

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

@baywet baywet closed this as completed Nov 2, 2023
@heinrich-ulbricht
Copy link

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: microsoftgraph/msgraph-sdk-dotnet#1708 (comment)

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

@baywet
Copy link
Member

baywet commented Jan 17, 2024

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

heinrich-ulbricht commented Jan 17, 2024

@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.

@daniel-palacian-haufe
Copy link

daniel-palacian-haufe commented Feb 29, 2024

I have the exact pb.
But what should happen if you have a custom Deserializer in your client? How will that work with KiotaJsonSerializer?

@baywet
Copy link
Member

baywet commented Mar 1, 2024

Assuming this custom deserializer is for JSON as well.
You effectively need to register it in the application.
This is done by default by the generated client when it's instantiated today. (so you can just new up the client before calling the static methods)

Or you can call the dedicated registration method like in this example

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Archived in project
Development

No branches or pull requests

7 participants