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

Idea: Use Kiota generated models for serialization #310

Closed
svrooij opened this issue Jul 31, 2024 · 7 comments · Fixed by #311
Closed

Idea: Use Kiota generated models for serialization #310

svrooij opened this issue Jul 31, 2024 · 7 comments · Fixed by #311
Labels
area:serialization Focused on functional modules of the product enhancement New feature or request help wanted Standard GitHub label type:enhancement Enhancement request targeting an existing experience WIP

Comments

@svrooij
Copy link
Contributor

svrooij commented Jul 31, 2024

I was wondering if I could use the models together with the used JsonSerializerOptions to read/write json.

The use case is that I would want to save some results of the Graph API, to a folder. Talking conditional access policies here, but any model will do. And I don't want to do any customizations, just save the object exactly as it would be transferred to and from the api.

pseudo code

var client = new GraphServiceClient(deviceCodeCredential, scopes: ["User.Read"]);

// Get something from the Kiota client
var user = await client.Me.GetAsync();

// This does not work
Microsoft.Kiota.Serialization.Json.JsonSerializationWriter writer = new Microsoft.Kiota.Serialization.Json.JsonSerializationWriter(new Microsoft.Kiota.Serialization.Json.KiotaJsonSerializationContext());
user!.Serialize(writer);
var stream = writer.GetSerializedContent();
// maybe this would be nice!
var userStream = user!.SerializeAsJsonStream(); // extension method?

// Save the stream to a file
using var fileStream = File.Create(filename);
stream.Seek(0, SeekOrigin.Begin);
stream.CopyTo(fileStream);

// and on the other hand, I want to read it back
using var fileStream2 = File.OpenRead(filename);
Microsoft.Graph.Models.User userFromDisk = ... // How to deserialize this?

Discussion

This is not really an issue, but this repo doesn't have discussions enabled.

@github-project-automation github-project-automation bot moved this to Needs Triage 🔍 in Kiota Jul 31, 2024
@MartinM85
Copy link
Contributor

MartinM85 commented Jul 31, 2024

This should work

using var jsonSerializerWriter = new JsonSerializationWriter();
jsonSerializerWriter.WriteObjectValue(string.Empty, user);
var serializedStream = jsonSerializerWriter.GetSerializedContent();
using var reader = new StreamReader(serializedStream, Encoding.UTF8);
var serializedJsonString = reader.ReadToEnd();

There is also KiotaJsonSerializer class with SerializeAsStringAsync and DeserializeAsync<T> methods

@svrooij
Copy link
Contributor Author

svrooij commented Jul 31, 2024

@MartinM85 thanks for the quick reply.

That seems not to work. the object is still empty, I want it to work because that would be awesome.

image

And I wonder why deserializing to a stream is not async with this helper.

@baywet
Copy link
Member

baywet commented Jul 31, 2024

Hi everyone,
Thanks @svrooij for starting the conversation here.
I also love the fact that @MartinM85 jumped in!

I think the request here is effectively the same as this one.
microsoft/kiota-java#1417

Could you confirm please?

@MartinM85
Copy link
Contributor

I guess it's related to a fact that User model is using backing store and backing store serialization writer returns only changed properties.

@baywet baywet added enhancement New feature or request status:waiting-for-author-feedback Issue that we've responded but needs author feedback to close type:enhancement Enhancement request targeting an existing experience help wanted Standard GitHub label labels Jul 31, 2024
@baywet baywet moved this from Needs Triage 🔍 to In Design 🎨 in Kiota Jul 31, 2024
@svrooij
Copy link
Contributor Author

svrooij commented Jul 31, 2024

@baywet as that is a different language, it's not the same, but definitely related.

And apart from that, an extension method for IParseable in the JSON serializer namespace, that hides all the mandatory initializing would be great.

And then maybe also the counterpart where you can say, I have this json stream, parse it as T

This would really help everyone that is saving returned objects to do some sort of desired state using files in git

This code seems to work for the serializing part:

    public static Stream? SerializeAsJsonStream<T>(this T? input) where T : IParsable
    {
        if (input == null) return null;
        var writer = new Microsoft.Kiota.Serialization.Json.JsonSerializationWriter(new Microsoft.Kiota.Serialization.Json.KiotaJsonSerializationContext());
        writer.WriteObjectValue(null, input);
        return writer.GetSerializedContent();
    }

Diving deeper into this, I noticed that all the json serializing is done synchronously, what is the reason for that? Seems like it might have some room for speed improvements?

@microsoft-github-policy-service microsoft-github-policy-service bot added Needs: Attention 👋 and removed status:waiting-for-author-feedback Issue that we've responded but needs author feedback to close labels Jul 31, 2024
@svrooij
Copy link
Contributor Author

svrooij commented Jul 31, 2024

I went ahead and added:

  • way to get the full item by disabling the backing store
  • Added an extension method to IParsable to serialize as json stream (this will only show up if the developer includes using Microsoft.Kiota.Abstractions.Serialization;

@baywet
Copy link
Member

baywet commented Jul 31, 2024

Aysnc API

Diving deeper into this, I noticed that all the json serializing is done synchronously

I'm guessing you're referring to these calls:

https://github.com/microsoft/kiota-dotnet/blob/bc0404d400e977e41c0dc277a845d6bf977ef44b/src/serialization/json/JsonSerializationWriter.cs#L93C17-L93C31

This would "force us" to have the whole ISerializationWriter interface to be async (all the methods in there) which would be a source breaking change at this point.

Likewise it'd contaminate the API surface

A couple of thoughts here:

  • enabling concurrency doesn't always translate to better performances. All those tasks would have an overhead.
  • ultimately this would result in the serialize method (from IParsable) to have to become async as well.
  • a more beneficial change would be to have the caller pass the stream instead of creating it. This would allow the request adapter to directly pass the request body stream, reducing memory pressure. But making that change would require additional changes in request information, the Serialization writer interface and would be a source breaking change at this point. Probably a good cleanup for v2 though.

solution

I've provided additional context to why this method works on the PR.
We'll want to avoid any solution that involves an extension method as this would only work for dotnet.

coordination

We'll want to coordinate that change across languages that have these helper methods to avoid differences in experiences (only CSharp/Java/TypeScript/Go at the moment)

Hopefully all of that helps make progress here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:serialization Focused on functional modules of the product enhancement New feature or request help wanted Standard GitHub label type:enhancement Enhancement request targeting an existing experience WIP
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

4 participants