microsoftgraph / msgraph-sdk-dotnet

Microsoft Graph Client Library for .NET!
https://graph.microsoft.com
Other
688 stars 246 forks source link

Issue with type changes on Graph Sdk model classes #2386

Open jsweiler opened 5 months ago

jsweiler commented 5 months ago

In one of the recent Graph sdk updates (Microsoft.Graph.Communications.Calls package) many of the models were moved from the Microsoft.Graph namespace to Microsoft.Graph.Models namespace.

However some of the notification updates that our app gets do not have the .models namespace in the json. Depending on how you deserialize the json you will get a null result. One example is when an audio prompt is completed playing currently with the latest sdk (version 1.2.0.10115) you get a notification like this

[ { "@odata.type": "#microsoft.graph.mediaPrompt", "mediaInfo": { "@odata.type": "#microsoft.graph.mediaInfo", "uri": "uriStrippedFor Privacy", "resourceId": "resourceId stripped for privacy" } } ]

(This is the prompts array which shows prompts which just completed and is inside the PlayPromptOperation type). But notice both of the odata.types do not match up with the current .models namespace.

Another example is when handling the CallsOnIncoming event. In the incoming call it contains this info about the resource id of the call.

{ "@odata.type": "#microsoft.graph.identity", "id": "369ebe19-93ba-46da-aa2b-c473034c9a64", "tenantId": "1aad987e-533b-4c16-83c1-13359661d26d", "identityProvider": "AAD" }

Again notice that the type is microsoft.graph.identity and the actual type in the sdk is now microsoft.graph.models.identity. I'm wondering if this was intended or just overlooked?

I realize there are ways to deserialize that can handler either way, but also depending on how the application handles it this breaks it.

andrueastman commented 5 months ago

Thanks for raising this @jsweiler

Any chance you can share more details on the deserialization mechanism used that uses the naming in the "@odata.type with the naming in the namespace?

jsweiler commented 5 months ago

@andrueastman using an older sdk this kind of code worked to get an applicationInstance

var appInstanceData = call.Resource.Targets.FirstOrDefault(c => c.Identity.AdditionalData != null);
if (appInstanceData.Identity.AdditionalData.TryGetValue("applicationInstance", out object value))
{
    var appInstance = value as Microsoft.Graph.Identity;
    if (appInstance != null)
    {
        // do something with appInstance
    }
}

That does not anymore.

Now using Newtonsoft.Json I am able to correctly deserialize it.

My bigger concern would be that the api doesn't change beneath the sdk before we are able to get our newer deserialization code into prod.

andrueastman commented 5 months ago

Taking a look at the mapping values in the Identity model type for example, you'll see the mapping values do follow the correct naming to enable polymorphic deserialization. Therefore, the existence of the source in the Microsoft.Graph.Models shouldn't really be an issue given the discriminator values.

https://github.com/microsoftgraph/msgraph-sdk-dotnet/blob/890e0dfa1aa04c9e9f1222edaf4f4ad8e44686c5/src/Microsoft.Graph/Generated/Models/Identity.cs#L79

@jsweiler Any chance you can confirm what the type of the call property is in your sample? I suspect it's not a model in the SDK package. We may have to follow up with the Microsoft.Graph.Communications.Calls package if the model is owned there to understand this better.

I also notice the pulling of the applicationInstance property from the additionalData when it is defined in the CommunicationsIdentitySet type meaning this should be directly accessible with something like this https://github.com/microsoftgraph/msgraph-sdk-dotnet/blob/890e0dfa1aa04c9e9f1222edaf4f4ad8e44686c5/src/Microsoft.Graph/Generated/Models/CommunicationsIdentitySet.cs#L13

if (appInstanceData.Identity is CommunicationsIdentitySet communicationsIdentitySet)
{
    var appInstance = communicationsIdentitySet.ApplicationInstance;
}

In general, models generated by Kiota are meant to be serialized using the kiota serialization libraries as do not use serialization library annotations/mechanisms for System.Text.Json/Newtonsoft and should be deserialized with the KiotaJsonSerializer

var call = KiotaJsonSerializer.Deserialize<Call>(responseStream);
jsweiler commented 5 months ago

@andrueastman The type of the call object in my code is ICall in the Microsoft.Graph.Communications.Calls namespace. The call object has an AdditionalData property which is of type IDictionary<string, object> and that is where I get the Identity object.