microsoft / kiota-dotnet

Abstractions library for the Kiota generated SDKs in dotnet
https://aka.ms/kiota/docs
MIT License
41 stars 34 forks source link

Backing store when using deserialize async #379

Open svrooij opened 2 months ago

svrooij commented 2 months ago

I was trying to use the serialize / deserialize to save and load an object for the graph api.

The idea was this (graph or any other api):

  1. Get an object from the API (or create a new object in C#)
  2. Serialize it as json
  3. (optional) change the json file
  4. Deserialize the object from json
  5. Post the object to a new tenant (or create the entry if it did not exist already)

In this last step I was faced with a 400 error, and after inspecting the request I figured out that none of the properties where sent to the server.

I think that users that actually use this static method or any of the extensions that call this method don't want the backing store.

And I would thus suggest that this method automatically sets the model.BackingStore.InitializationCompleted to false. That resulted in the full object being posted.

baywet commented 2 months ago

Hi @svrooij Thank you for using kiota and for reaching out.

If I'm understanding you correctly, you effectively want to do the opposite of what you've already implemented for the serialization? i.e. have an optional boolean parameter to "set everything as changed" by default (default value false)

If so, is this something you'd like to submit a pull request for provided some guidance?

svrooij commented 2 months ago

@baywet I changed the initial description. (Was written on a tablet)

baywet commented 2 months ago

Thank you for the additional information. I think we're on the same page. Is this something you'd like to submit a pull request for provided some guidance?

svrooij commented 2 months ago

I'm willing to create a PR for this, but what do you suggest? Can I just change it to resetting the backing store if someone uses this method to deserialize? I guess it's not used by the other code, that will probably call the code directly without the KiotaSerializer.Deserialize(...)

baywet commented 2 months ago

for consistency I'd align with previous changes we've made: parameter named something like "markAllValuesAsChanged" default false. This way we don't change the current behaviour for people using the methods today. What do you think?

svrooij commented 2 months ago

How do I add an addition parameter to this method? https://github.com/microsoft/kiota-dotnet/blob/427e3ff3c7517f68d395e1591be127f29803f7c8/src/abstractions/serialization/KiotaSerializer.Deserialization.cs#L166-L168

I heard that the CancellationToken should be the last parameter. If I add it in between, it will be source breaking.

Just add a new method? and an obsolete to the old?

baywet commented 2 months ago

Just add a new method? and an obsolete to the old?

I'm afraid that's the only option here

svrooij commented 2 months ago

Shall I add this to the same PR? #396

baywet commented 2 months ago

It'd probably be easier to have separate PRs at this point.