microsoft / kiota-dotnet

Abstractions library for the Kiota generated SDKs in dotnet
https://aka.ms/kiota/docs
MIT License
32 stars 29 forks source link

JsonSerializationWriter.WriteNonParsableObjectValue does not support anonymous objects #306

Closed greg-ww closed 1 month ago

greg-ww commented 1 month ago

I was passing values into the AdditionalData property on the type generated in my Kiota client and I noticed that anonymous object values were being passed as empty objects.

{ "myAdditionalDataProperty": { } }

From debugging I can see that here https://github.com/microsoft/kiota-dotnet/blob/42ea74edf45d473462dd050661b1972ce14c15e1/src/serialization/json/JsonSerializationWriter.cs#L447 T comes through as "Object" so get properties returns no results.

No error is thrown, the properties just never show up. It's possible this affects other types but I only tested with anonymous objects.

greg-ww commented 1 month ago

In trying to work around this issue, it seems the problem is worse than I thought. I made a my own class for the data and that also failed to serialize properly, then I tried to use a Dictionary<string, object> and that didn't work either.

The PR I submitted should work for custom non parseable classes but for dictionary should probably have it's own method.

I will try to make a more extensive PR.

baywet commented 1 month ago

Hi @greg-ww Thanks for using kiota and for reaching out. I have reviewed the PR before I saw this issue (orders of notifications on GitHub) I'm curious to understand in what scenario do you feel like you need to rely on anonymous types? For context, the serialization interface here was never meant to be used by humans, just to support the generated code. Code that is generated from a schema, and should give us enough context not to have to rely on anonymous types.

greg-ww commented 1 month ago

Hi @baywet thanks for getting back to me. Like I mentioned, I am setting AdditionalData on a type generated by Kiota. The original type for the property in question in our API is a dictionary of string to another model type which gets represented in the openAPI spec as

"myProperty": {
            "type": "object",
            "additionalProperties": {
              "$ref": "#/components/schemas/MyDictionaryValueType"
            },
          },

for which Kiota generates

 // <auto-generated/>
using Microsoft.Kiota.Abstractions.Extensions;
using Microsoft.Kiota.Abstractions.Serialization;
using System.Collections.Generic;
using System.IO;
using System;
namespace Civitai.Orchestration.IntegrationTests.Clients.Models
{
    /// <summary>
    /// My summary.
    /// </summary>
    [global::System.CodeDom.Compiler.GeneratedCode("Kiota", "1.16.0")]
    public partial class MainType_myProperty : IAdditionalDataHolder, IParsable
    {
        /// <summary>Stores additional data not described in the OpenAPI description found when deserializing. Can be used for serialization as well.</summary>
        public IDictionary<string, object> AdditionalData { get; set; }
        /// <summary>
        /// Instantiates a new <see cref="global::Civitai.Orchestration.IntegrationTests.Clients.Models.MainType_myProperty"/> and sets the default values.
        /// </summary>
        public MainType_myProperty()
        {
            AdditionalData = new Dictionary<string, object>();
        }
        /// <summary>
        /// Creates a new instance of the appropriate class based on discriminator value
        /// </summary>
        /// <returns>A <see cref="global::Civitai.Orchestration.IntegrationTests.Clients.Models.MainType_myProperty"/></returns>
        /// <param name="parseNode">The parse node to use to read the discriminator value and create the object</param>
        public static global::Civitai.Orchestration.IntegrationTests.Clients.Models.MainType_myProperty CreateFromDiscriminatorValue(IParseNode parseNode)
        {
            _ = parseNode ?? throw new ArgumentNullException(nameof(parseNode));
            return new global::Civitai.Orchestration.IntegrationTests.Clients.Models.MainType_myProperty();
        }
        /// <summary>
        /// The deserialization information for the current model
        /// </summary>
        /// <returns>A IDictionary&lt;string, Action&lt;IParseNode&gt;&gt;</returns>
        public virtual IDictionary<string, Action<IParseNode>> GetFieldDeserializers()
        {
            return new Dictionary<string, Action<IParseNode>>
            {
            };
        }
        /// <summary>
        /// Serializes information the current object
        /// </summary>
        /// <param name="writer">Serialization writer to use to serialize this model</param>
        public virtual void Serialize(ISerializationWriter writer)
        {
            _ = writer ?? throw new ArgumentNullException(nameof(writer));
            writer.WriteAdditionalData(AdditionalData);
        }
    }
}

I am not sure I understand why Kiota doesn't just use a dictionary of the same type as our models, given that the structure in the openAPI spec appears to be constrained enough to do so. But with things the way they are the only way to populate this type is to set AdditionalData, something the generated comments indicate is actively supported.

So we set the values in additional data which is Dictionary<string, object>, but no type is provided by Kiota for the type described in the openAPI spec as "MyDictionaryValueType". Because we treat the code in the Kiota client folder as generated and rely on that to provide us the types we use interacting with our API the best and most convenient compromise is to set the values we want as an anonymous object as this is the closest thing to setting them on a generated type and by all indications from the serializer this is perfectly valid and functional (until you look at the JSON Kiota sends).

Outside of my own personal use case, I feel like setting AdditionalData is a useful and valid way to set properties for a type that is either extremely flexible or cannot be fully defined in the openAPI spec. The options for values I tried to assign to it seemed perfectly intuitive, first an anonymous object, then I made a class to represent it, then I represented the type as a dictionary. If there are good reasons to not support these I would at least expect the serializer to return a proper error, preferably returning me some information about what types would be accepted.

baywet commented 1 month ago

Thanks for the additional context here. Yes, it's a limitation today. https://github.com/microsoft/kiota/issues/62

baywet commented 1 month ago

Have you considered making the types that you pass as values of the additional properties implement IParsable instead?

greg-ww commented 1 month ago

Have you considered making the types that you pass as values of the additional properties implement IParsable instead?

Yeah that's my next workaround, I only know I need to do that because I debugged into JsonSerializationWriter though. At minimum I'd expect an error if the types I tried to use are unsupported.

baywet commented 1 month ago

This is probably a good situation for other cases. Can you tweak this exception message as part of your PR please? https://github.com/microsoft/kiota-dotnet/pull/307/files#diff-0f94b98a554904a6aea6e2d994e76fd110b76d2c96fd51d29e30ddad644ac620R552