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

JsonSerializationWriter.WriteNonParsableObjectValue does not support anonymous objects #306

Closed
greg-ww opened this issue Jul 25, 2024 · 7 comments · Fixed by #307
Closed
Labels
enhancement New feature or request Needs: Attention 👋 type:enhancement Enhancement request targeting an existing experience

Comments

@greg-ww
Copy link
Contributor

greg-ww commented Jul 25, 2024

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

foreach(var oProp in typeof(T).GetProperties())
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
Copy link
Contributor Author

greg-ww commented Jul 25, 2024

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 baywet added enhancement New feature or request type:enhancement Enhancement request targeting an existing experience status:waiting-for-author-feedback Issue that we've responded but needs author feedback to close labels Jul 25, 2024
@baywet baywet moved this from Needs Triage 🔍 to Waits for author 🔁 in Kiota Jul 25, 2024
@baywet
Copy link
Member

baywet commented Jul 25, 2024

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
Copy link
Contributor Author

greg-ww commented Jul 25, 2024

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.

@microsoft-github-policy-service microsoft-github-policy-service bot added Needs: Attention 👋 and removed status:waiting-for-author-feedback Issue that we've responded but needs author feedback to close labels Jul 25, 2024
@baywet
Copy link
Member

baywet commented Jul 25, 2024

Thanks for the additional context here.
Yes, it's a limitation today. microsoft/kiota#62

@baywet
Copy link
Member

baywet commented Jul 25, 2024

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

@greg-ww
Copy link
Contributor Author

greg-ww commented Jul 25, 2024

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
Copy link
Member

baywet commented Jul 26, 2024

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

@github-project-automation github-project-automation bot moved this from Waits for author 🔁 to Done ✔️ in Kiota Jul 31, 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 Needs: Attention 👋 type:enhancement Enhancement request targeting an existing experience
Projects
Archived in project
2 participants