msgraph-sdk-dotnet icon indicating copy to clipboard operation
msgraph-sdk-dotnet copied to clipboard

AdditionalData can break my code when I update the graph package

Open christianarg opened this issue 4 years ago • 8 comments

Describe the bug If I use AdditionalData as the data is not bound to any property and then update to a version that is bound to a propery, the value is not anyomore in AdditionalData and my code brekas

To Reproduce I use AdditionalData. for example

if(entity.AdditionalData.Any(x => x.Key == "SomeProperty" && x.Value?.ToString() == "SomeValue"){}

Then I update to a version that entity has SomeProperty so it is removed from AdditionalData and my code breaks.

I'm forced to check every usage of AdditionalData everytime I update the graph package

Expected behavior I suppose that you can expose a dictionary entity.AllData where all the values are present. That way I'm not forced to check every usage of AdditionalData everytime I update the graph package.

We should be able to use the typed entity without this risk

AB#7811

christianarg avatar Jan 27 '21 10:01 christianarg

My code faced some logical errors due to changing the data in AdditionalData. ODataType used to be preceded by a '#', and it was removed after updating packages.

Danialmz96 avatar Apr 28 '21 15:04 Danialmz96

@baywet Since we will have the backing store in Kiota, can we make it so that scenarios like this can be handled gracefully? So for example, a property is not schematized at time0 and therefore the property is exposed through AdditionalData, and then at timeNext it gets schematized and the property no longer is exposed through AdditionalData. This is a breaking change for the customer even if the schema omission was a bug itself.

MIchaelMainer avatar Jun 23 '21 14:06 MIchaelMainer

@MIchaelMainer I don't think this is an issue where the backing store feature would help. Whenever the SDK gets re-generated based on the metadata, the property will show up in the type. And when that happens, the deserializer will try to fit the data it finds on the payload to a property first (which will go to the backing store arguably). I don't think we should encourage consumers to read directly from the backing store. It's more of an infrastructure piece to handle the dirty tracking problem and/or interop with other layers. If we wanted to do that, we'd need to keep tracking/diff two schemas/api documentations which feels like a lot of work for such an edge case.

baywet avatar Jun 23 '21 16:06 baywet

This certainly is an edge case as most properties are schematized when they are made public. We should consider OpenType scenarios where a property is present without schema and then gets added at some point in the future to the schema. Unfortunately the customer can't discover that this is a schema bug and that they shouldn't take a dependency on the object in AdditionalData. I was thinking more along the lines of AdditionalData.TryGet() abstracting the backing store. I don't recall if in the Kiota design, whether AdditionalData uses the backing store. If it does, AdditionalData.TryGet could be used to access the backing store.

MIchaelMainer avatar Jun 23 '21 19:06 MIchaelMainer

when the backing store is enabled, the additional data dictionary itself is stored in it. Maybe we should break it apart instead of putting a dictionary into a dictionary?

baywet avatar Jun 23 '21 20:06 baywet

Any update on this?

christianarg avatar Dec 03 '21 14:12 christianarg

@baywet did we ever address this as you described above for Kiota?

ddyett avatar Jun 28 '23 02:06 ddyett

We have not, currently the additional data is stored as a single entry in the backing store, this is probably something we should address across the board to get rid of edge cases like there.

baywet avatar Jun 28 '23 11:06 baywet