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

Backing store when using deserialize async #379

Open
svrooij opened this issue Sep 5, 2024 · 9 comments
Open

Backing store when using deserialize async #379

svrooij opened this issue Sep 5, 2024 · 9 comments
Labels
enhancement New feature or request Needs: Attention 👋 type:enhancement Enhancement request targeting an existing experience

Comments

@svrooij
Copy link
Contributor

svrooij commented Sep 5, 2024

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

baywet commented Sep 6, 2024

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?

@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 labels Sep 6, 2024
@svrooij
Copy link
Contributor Author

svrooij commented Sep 7, 2024

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

@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 Sep 7, 2024
@baywet
Copy link
Member

baywet commented Sep 9, 2024

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?

@baywet baywet added status:waiting-for-author-feedback Issue that we've responded but needs author feedback to close and removed Needs: Attention 👋 labels Sep 9, 2024
@svrooij
Copy link
Contributor Author

svrooij commented Sep 24, 2024

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(...)

  • Just reset the backing store to see everything deserialized this way is seen as changed
  • Make a parameter, which defaults to yes reset
  • Make a parameter which defaults to the old behavior

@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 Status: No Recent Activity labels Sep 24, 2024
@baywet
Copy link
Member

baywet commented Sep 24, 2024

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?

@baywet baywet added status:waiting-for-author-feedback Issue that we've responded but needs author feedback to close and removed Needs: Attention 👋 labels Sep 24, 2024
@svrooij
Copy link
Contributor Author

svrooij commented Sep 24, 2024

How do I add an addition parameter to this method?

public static async Task<T?> DeserializeAsync<T>(string contentType, string serializedRepresentation, ParsableFactory<T> parsableFactory,
CancellationToken cancellationToken = default) where T : IParsable
{

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?

@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 Sep 24, 2024
@baywet
Copy link
Member

baywet commented Sep 24, 2024

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

I'm afraid that's the only option here

@baywet baywet added status:waiting-for-author-feedback Issue that we've responded but needs author feedback to close and removed Needs: Attention 👋 labels Sep 24, 2024
@svrooij
Copy link
Contributor Author

svrooij commented Sep 24, 2024

Shall I add this to the same PR? #396

@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 Sep 24, 2024
@baywet
Copy link
Member

baywet commented Sep 24, 2024

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request Needs: Attention 👋 type:enhancement Enhancement request targeting an existing experience
Projects
Status: Waits for author 🔁
Development

No branches or pull requests

2 participants