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

Serialization methods overloads to disable the "only changed properties behaviour" #1131

Closed
baywet opened this issue Mar 20, 2024 · 5 comments · Fixed by #1608
Closed

Serialization methods overloads to disable the "only changed properties behaviour" #1131

baywet opened this issue Mar 20, 2024 · 5 comments · Fixed by #1608
Assignees
Labels
enhancement New feature or request Standard GitHub label Issue caused by core project dependency modules or library WIP

Comments

@baywet
Copy link
Member

baywet commented Mar 20, 2024

We recently got very similar questions:
microsoftgraph/msgraph-sdk-java#1886
microsoftgraph/msgraph-sdk-java#1879

People generally want to:

  1. get something from the service
  2. serialize it to store it locally, in a document db, etc...

While our serialization helpers are helpful, they fall short of one aspect: they don't allow the caller to specify "serialize everything" (as opposed to serializing only the changed properties).

My suggestion here is to:

  1. add an overload that'd accept a boolean parameter "serializeOnlyCHangedValues" default false
  2. have the existing implementations call into that overload, with default false (the behaviour will change)
  3. add a routine that crawls the object tree whenever the value is false, and whenever it finds a backed model, flips the serialize only changed values from the backing store.
  4. once the serialization is done, crawl the tree again to restore things.

@sebastienlevert to provide input on this one before we start any work, and if we get an agreement here, I'll replicate the issue to the other repos.

@baywet baywet added enhancement New feature or request Standard GitHub label Issue caused by core project dependency modules or library labels Mar 20, 2024
@baywet baywet added this to Kiota Mar 20, 2024
@github-project-automation github-project-automation bot moved this to Todo in Kiota Mar 20, 2024
@sebastienlevert
Copy link

Just so I understand. When serializing models (across languages) we only serialize what has changed when the backing store is enabled, right? If this is the case, I agree we should have options to "deep" serialize vs only what is in the backing store.

If it's the case without the backing store, then I think this is a bug, though...

@baywet
Copy link
Member Author

baywet commented Mar 22, 2024

this is only present with the backing store. And we don't want to give people the option, but rather have a consistent behaviour.
When they change a collection (set the collection again), we should consider all items within the collection changed, regardless of where they came from. (this is broken)
If they only insert/remove items to the collection, then only individual items should be considered changed. (that aspect works today)

@sebastienlevert
Copy link

Just to make it clear, can you provide a tiny example of before / after of a collection that needs to be serialized?

@baywet
Copy link
Member Author

baywet commented Apr 15, 2024

fixing the broken serialization of collections

var joeUser = client.users.byId("[email protected]").get();
var userResponse = client.users.get();
userResponse.SetValue(new User[] { joeUser });
KiotaJsonSerialization.serializeAsString(userReponse);

results in

{
   "value": []
}

when it should result in

{
   "value": [{"displayName": "Joe Smith", "email": "[email protected]"}]
}

(that illustrates the broken part)

Option to serialize everything when things didn't change

var userResponse = client.users.get();
KiotaJsonSerialization.serializeAsString(userReponse);

results in

{}

when it should result in

{
   "value": [{"displayName": "Joe Smith", "email": "[email protected]"}]
}

@baywet
Copy link
Member Author

baywet commented Sep 10, 2024

For everyone's context, we recently went through this in dotnet and should replicate the changes here.
microsoft/kiota-dotnet#367
microsoft/kiota-dotnet#311

@Ndiritu Ndiritu self-assigned this Sep 23, 2024
@Ndiritu Ndiritu moved this from Todo 📃 to In Progress 🚧 in Kiota Oct 2, 2024
@Ndiritu Ndiritu moved this from In Progress 🚧 to In Review 💭 in Kiota Oct 4, 2024
@github-project-automation github-project-automation bot moved this from In Review 💭 to Done ✔️ in Kiota Oct 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request Standard GitHub label Issue caused by core project dependency modules or library WIP
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

3 participants