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

Setting fields to nil now causes null in resulting serialized data #142

Closed
ashmrtn opened this issue Jan 19, 2024 · 9 comments
Closed

Setting fields to nil now causes null in resulting serialized data #142

ashmrtn opened this issue Jan 19, 2024 · 9 comments
Assignees

Comments

@ashmrtn
Copy link

ashmrtn commented Jan 19, 2024

Hi there, I recently attempted to update to v1.5.5 and started getting server errors with the message were unable to deserialize when posting a calendar event

This error is consistently reproducible if you do the following:

  1. create some graph SDK model, for this example we'll use an event
  2. create a body for the event
  3. update the event body with body.SetOdataType(nil)
  4. POST the event to microsoft's graph endpoint like you're making a new event instance
  5. get above error response back

Digging into this a bit more, I found a difference in the serialized data output that's sent to the graph servers. v1.5.4 doesn't add fields set to nil to the request being made. v1.5.5 adds fields set to nil to null (in our example you end up with "odataType": null in the request for the body of the event). Setting other fields to nil in the code also results in the request having them set to null

While I realize that setting the odata type in code is probably looked down on, it does make it more difficult to manage these objects as in some cases my team and I are taking an object, copying it's data to a new object, and then modifying some of the fields in the new object. It'd be preferable if the code didn't have to differentiate between what field it's copying over (i.e. explicitly avoid copying the odata type field since just not setting the odata field at all seems to avoid the server error)

As for how we got an object with a nil odata type as the source of our copy: we were deserializing a previously serialized version of the object

Sending nulls is also a bit concerning because it requires additional data bytes to be transferred when sending a request. While not a huge issue for one or two requests, the amount of additional data required could add up over time

@baywet baywet self-assigned this Jan 19, 2024
@baywet baywet added question Further information is requested Needs: Author Feedback labels Jan 19, 2024
@baywet
Copy link
Member

baywet commented Jan 19, 2024

Hi @ashmrtn,
Thanks for using the Go SDK and for reaching out.
This is intentional so people can reset values on APIs (a commonly used pattern).
The serialization infrastructure should only be sending nulls when:

  1. the object has been previously obtained from the API.
  2. a property has explicitly been set to nil by the application.
  3. the object is being sent back to the API.

Which seems to match your case to some extent.
Can you share a snippet of code of how the copy is done today in your application?

@ashmrtn
Copy link
Author

ashmrtn commented Jan 19, 2024

We're using the graph SDK in an open source project called Corso which is meant to backup and restore M365 data for users. Our end goal with the bit of code we're working with is to get an object model that we can send to microsoft to create a new version of the object with a distinct ID from the original object. In the example, fields we're clearing are ones that will either cause server issues (found through experimentation) or are ignored by the server and don't need to be sent (except maybe the odata type, though that was never an error before)

Right now we're doing the copy by manually selecting the fields (some of which need cleared, others of which are just pulled from the original). I was sort of hoping to switch this copy to something better in the future by maybe using reflection to avoid having to pick and choose which fields are copied over as that seems error prone if the API expands. Doing a manual copy requires intimate knowledge of the API, how it evolves over time, and which fields are important and need selected

@baywet
Copy link
Member

baywet commented Jan 22, 2024

Thanks for the additional context here. This is a bit of a specific scenario compared with the typical usage.

For the properties you want to reset without sending null to the API, you could do something like that.

myModel.GetBackingStore().SetInitializationCompleted(false);
myModel.SetProperty(nil);
myModel.GetBackingStore().SetInitializationCompleted(true);

Let us know if you need anything else.

@ashmrtn
Copy link
Author

ashmrtn commented Jan 22, 2024

is this going to be the only way to handle this going forward? I'm a little concerned that it's going to make it difficult to write correct code since it requires special handling of some fields and it's not necessarily clear when special handling should be used and when it shouldn't. As there's no written documentation for how to handle this or why it needs to be handled that way, correctness would rely on the code author and/or the code reviewer having the tribal knowledge that that's the proper way to do things

If my understanding of the various SDK libraries and HTTP CRUD operations is correct, the recent update to this library would mostly be applicable to PATCH requests right? I could see some cases where needing to set a field to null could be important to clear a default value when creating a new object (POST), but it seems far more likely to be used during item updates (PATCH)

@baywet
Copy link
Member

baywet commented Jan 22, 2024

Yes this is the main scenario we've designed this for:

  1. GET an object.
  2. Reset a property by setting it to null.
  3. PUT or PATCH the object back to the service to reset the property there.

Although this is discouraged by REST guidelines, nothing prevents an API provider from using post for update purposes. This is especially true when the update requires additional ephemeral parameters. For example an update meeting endpoint that would take the meeting + a couple of parameters like "send update email notification" etc.

Finally, the serialization infrastructure doesn't have knowledge of what http method is in use, those are orthogonal.

Because these accessors are exposed by the Backed model interface, you should be able to come up with a generic solution that can be applied to all models types.

The alternative being to generate your own Microsoft graph client using kiota, without the baking store if you want.

@ashmrtn
Copy link
Author

ashmrtn commented Feb 8, 2024

I've been working on updating the code my team maintains using the workaround you suggested. However, I'm a bit confused as to when I need to use the workaround you suggested and when I don't. For example, should I only be using it if setting a field to nil? Is there some form of documentation explaining that information?

@baywet
Copy link
Member

baywet commented Feb 8, 2024

Thanks for the prompt, I have created an issue in our documentation repository

I'd say you'd need to implement the workaround (assuming we're talking about switching the flags for the backing store and not generating your own client without the backing store) any time you:

  1. get an object from the API.
  2. set one of it's properties to null.
  3. send the object back to the API BUT you don't want that property to be sent as null on the payload.

Hopefully that clarifies the scope.

@ashmrtn
Copy link
Author

ashmrtn commented Feb 8, 2024

yes, that info is super helpful! You're right that I was talking about switching the flags for the backing store and sorry for not clarifying that initially

Thanks for also creating an issue for adding documentation around this. It'll be good to be able to point other engineers on my team to it if/when needed

This issue has been automatically marked as stale because it has been marked as requiring author feedback but has not had any activity for 4 days. It will be closed if no further activity occurs within 3 days of this comment.

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

No branches or pull requests

2 participants