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

[BUG][CSHARP] DeserializeObject putting everything in inherited Dictionary #16767

Closed
5 of 6 tasks
KRSogaard opened this issue Oct 9, 2023 · 5 comments · Fixed by #17140
Closed
5 of 6 tasks

[BUG][CSHARP] DeserializeObject putting everything in inherited Dictionary #16767

KRSogaard opened this issue Oct 9, 2023 · 5 comments · Fixed by #17140

Comments

@KRSogaard
Copy link
Contributor

KRSogaard commented Oct 9, 2023

Bug Report Checklist

  • Have you provided a full/minimal spec to reproduce the issue?
  • Have you validated the input using an OpenAPI validator (example)?
  • Have you tested with the latest master to confirm the issue still exists?
  • Have you searched for related issues/PRs?
  • What's the actual output vs expected output?
  • [Optional] Sponsorship to speed up the bug fix or feature request (example)
Description

I am using the openAPI-generator to generate a Client. I am using the httpclient, but i have notice the same problem restsharp and generichost.
When the API serialized the response it get back, it adds all of the value to the inhered dictionary instead of the correct properties. I have written a unit test below that the reproduce the problem.

openapi-generator version

7.0.1, don't know first time using it

OpenAPI declaration file content or url

https://raw.githubusercontent.com/plaid/plaid-openapi/master/2020-09-14.yml

Steps to reproduce

See unit test below

Related issues/PRs

Could not find any mentions of this

Suggest a fix

If inheritance of Dictionary<string, object> and the this.AdditionalProperties is remove the object is serilized correctly. However i can not find a way to do this via the settings even though there is a isAdditionalPropertiesTrue in the mustache template.

Test to reproduce

The Deserialize code is lifted from the generated code, to test it in isolation

[Fact]
    public async Task TestDeserializeObject()
    {
        var json =
            "{\n  \"expiration\": \"2023-10-09T20:00:58Z\",\n  \"link_token\": \"link-sandbox-ac4a8c0f-b865-43e9-b2b9-83085588ac63\",\n  \"request_id\": \"pFIyYxXkrrIHsgEc\"\n}";

        var _serializerSettings = new JsonSerializerSettings
        {
            // OpenAPI generated types generally hide default constructors.
            ConstructorHandling = ConstructorHandling.AllowNonPublicDefaultConstructor,
            ContractResolver = new DefaultContractResolver
            {
                NamingStrategy = new CamelCaseNamingStrategy
                {
                    OverrideSpecifiedNames = false,
                },
            },
        };
        var output = (LinkTokenCreateResponse)JsonConvert.DeserializeObject(json, typeof(LinkTokenCreateResponse), _serializerSettings);
        Assert.NotNull(output.LinkToken);
        Assert.NotNull(output.Expiration);
        Assert.NotNull(output.RequestId);
    }

The model generated:

[DataContract(Name = "LinkTokenCreateResponse")]
public class LinkTokenCreateResponse : Dictionary<string, object>, IEquatable<LinkTokenCreateResponse>, IValidatableObject
{
   /// <summary>
   ///     Initializes a new instance of the <see cref="LinkTokenCreateResponse" /> class.
   /// </summary>
   [JsonConstructorAttribute]
   protected LinkTokenCreateResponse()
   {
       this.AdditionalProperties = new Dictionary<string, object>();
   }

   /// <summary>
   ///     Initializes a new instance of the <see cref="LinkTokenCreateResponse" /> class.
   /// </summary>
   /// <param name="linkToken">
   ///     A &#x60;link_token&#x60;, which can be supplied to Link in order to initialize it and receive a &#x60;public_token&#x60;, which can be
   ///     exchanged for an &#x60;access_token&#x60;. (required).
   /// </param>
   /// <param name="expiration">
   ///     The expiration date for the &#x60;link_token&#x60;, in [ISO 8601](https://wikipedia.org/wiki/ISO_8601) format. A &#x60;link_token
   ///     &#x60; created to generate a &#x60;public_token&#x60; that will be exchanged for a new &#x60;access_token&#x60; expires after 4 hours. A &#x60;link_token
   ///     &#x60; created for an existing Item (such as when updating an existing &#x60;access_token&#x60; by launching Link in update mode) expires after 30 minutes.
   ///     (required).
   /// </param>
   /// <param name="requestId">
   ///     A unique identifier for the request, which can be used for troubleshooting. This identifier, like all Plaid identifiers, is case
   ///     sensitive. (required).
   /// </param>
   /// <param name="hostedLinkUrl">A URL of a Plaid-hosted Link flow that will use the Link token returned by this request.</param>
   public LinkTokenCreateResponse(string linkToken = default(string), DateTimeOffset expiration = default(DateTimeOffset), string requestId = default(string),
       string hostedLinkUrl = default(string))
   {
       // to ensure "linkToken" is required (not null)
       if (linkToken == null)
       {
           throw new ArgumentNullException("linkToken is a required property for LinkTokenCreateResponse and cannot be null");
       }

       this.LinkToken = linkToken;
       this.Expiration = expiration;
       // to ensure "requestId" is required (not null)
       if (requestId == null)
       {
           throw new ArgumentNullException("requestId is a required property for LinkTokenCreateResponse and cannot be null");
       }

       this.RequestId = requestId;
       this.HostedLinkUrl = hostedLinkUrl;
       this.AdditionalProperties = new Dictionary<string, object>();
   }

   /// <summary>
   ///     A &#x60;link_token&#x60;, which can be supplied to Link in order to initialize it and receive a &#x60;public_token&#x60;, which can be exchanged for an
   ///     &#x60;access_token&#x60;.
   /// </summary>
   /// <value>
   ///     A &#x60;link_token&#x60;, which can be supplied to Link in order to initialize it and receive a &#x60;public_token&#x60;, which can be exchanged for an
   ///     &#x60;access_token&#x60;.
   /// </value>
   [DataMember(Name = "link_token", IsRequired = true, EmitDefaultValue = true)]
   public string LinkToken { get; set; }

   /// <summary>
   ///     The expiration date for the &#x60;link_token&#x60;, in [ISO 8601](https://wikipedia.org/wiki/ISO_8601) format. A &#x60;link_token&#x60; created to generate
   ///     a &#x60;public_token&#x60; that will be exchanged for a new &#x60;access_token&#x60; expires after 4 hours. A &#x60;link_token&#x60; created for an
   ///     existing Item (such as when updating an existing &#x60;access_token&#x60; by launching Link in update mode) expires after 30 minutes.
   /// </summary>
   /// <value>
   ///     The expiration date for the &#x60;link_token&#x60;, in [ISO 8601](https://wikipedia.org/wiki/ISO_8601) format. A &#x60;link_token&#x60; created to
   ///     generate a &#x60;public_token&#x60; that will be exchanged for a new &#x60;access_token&#x60; expires after 4 hours. A &#x60;link_token&#x60; created for
   ///     an existing Item (such as when updating an existing &#x60;access_token&#x60; by launching Link in update mode) expires after 30 minutes.
   /// </value>
   [DataMember(Name = "expiration", IsRequired = true, EmitDefaultValue = true)]
   public DateTimeOffset Expiration { get; set; }

   /// <summary>
   ///     A unique identifier for the request, which can be used for troubleshooting. This identifier, like all Plaid identifiers, is case sensitive.
   /// </summary>
   /// <value>A unique identifier for the request, which can be used for troubleshooting. This identifier, like all Plaid identifiers, is case sensitive.</value>
   [DataMember(Name = "request_id", IsRequired = true, EmitDefaultValue = true)]
   public string RequestId { get; set; }

   /// <summary>
   ///     A URL of a Plaid-hosted Link flow that will use the Link token returned by this request
   /// </summary>
   /// <value>A URL of a Plaid-hosted Link flow that will use the Link token returned by this request</value>
   [DataMember(Name = "hosted_link_url", EmitDefaultValue = false)]
   public string HostedLinkUrl { get; set; }

   /// <summary>
   ///     Gets or Sets additional properties
   /// </summary>
   [JsonExtensionData]
   public IDictionary<string, object> AdditionalProperties { get; set; }

   /// <summary>
   ///     Returns true if LinkTokenCreateResponse instances are equal
   /// </summary>
   /// <param name="input">Instance of LinkTokenCreateResponse to be compared</param>
   /// <returns>Boolean</returns>
   public bool Equals(LinkTokenCreateResponse input)
   {
       if (input == null)
       {
           return false;
       }

       return base.Equals(input) &&
              (
                  this.LinkToken == input.LinkToken ||
                  (this.LinkToken != null &&
                   this.LinkToken.Equals(input.LinkToken))
              ) && base.Equals(input) &&
              (
                  this.Expiration == input.Expiration ||
                  (this.Expiration != null &&
                   this.Expiration.Equals(input.Expiration))
              ) && base.Equals(input) &&
              (
                  this.RequestId == input.RequestId ||
                  (this.RequestId != null &&
                   this.RequestId.Equals(input.RequestId))
              ) && base.Equals(input) &&
              (
                  this.HostedLinkUrl == input.HostedLinkUrl ||
                  (this.HostedLinkUrl != null &&
                   this.HostedLinkUrl.Equals(input.HostedLinkUrl))
              )
              && this.AdditionalProperties.Count == input.AdditionalProperties.Count && !this.AdditionalProperties.Except(input.AdditionalProperties).Any();
   }

   /// <summary>
   ///     To validate all properties of the instance
   /// </summary>
   /// <param name="validationContext">Validation context</param>
   /// <returns>Validation Result</returns>
   IEnumerable<ValidationResult> IValidatableObject.Validate(ValidationContext validationContext)
   {
       return this.BaseValidate(validationContext);
   }

   /// <summary>
   ///     Returns the string presentation of the object
   /// </summary>
   /// <returns>String presentation of the object</returns>
   public override string ToString()
   {
       var sb = new StringBuilder();
       sb.Append("class LinkTokenCreateResponse {\n");
       sb.Append("  ").Append(base.ToString().Replace("\n", "\n  ")).Append("\n");
       sb.Append("  LinkToken: ").Append(this.LinkToken).Append("\n");
       sb.Append("  Expiration: ").Append(this.Expiration).Append("\n");
       sb.Append("  RequestId: ").Append(this.RequestId).Append("\n");
       sb.Append("  HostedLinkUrl: ").Append(this.HostedLinkUrl).Append("\n");
       sb.Append("  AdditionalProperties: ").Append(this.AdditionalProperties).Append("\n");
       sb.Append("}\n");
       return sb.ToString();
   }

   /// <summary>
   ///     Returns the JSON string presentation of the object
   /// </summary>
   /// <returns>JSON string presentation of the object</returns>
   public string ToJson()
   {
       return JsonConvert.SerializeObject(this, Formatting.Indented);
   }

   /// <summary>
   ///     Returns true if objects are equal
   /// </summary>
   /// <param name="input">Object to be compared</param>
   /// <returns>Boolean</returns>
   public override bool Equals(object input)
   {
       return this.Equals(input as LinkTokenCreateResponse);
   }

   /// <summary>
   ///     Gets the hash code
   /// </summary>
   /// <returns>Hash code</returns>
   public override int GetHashCode()
   {
       unchecked // Overflow is fine, just wrap
       {
           var hashCode = base.GetHashCode();
           if (this.LinkToken != null)
           {
               hashCode = (hashCode * 59) + this.LinkToken.GetHashCode();
           }

           if (this.Expiration != null)
           {
               hashCode = (hashCode * 59) + this.Expiration.GetHashCode();
           }

           if (this.RequestId != null)
           {
               hashCode = (hashCode * 59) + this.RequestId.GetHashCode();
           }

           if (this.HostedLinkUrl != null)
           {
               hashCode = (hashCode * 59) + this.HostedLinkUrl.GetHashCode();
           }

           if (this.AdditionalProperties != null)
           {
               hashCode = (hashCode * 59) + this.AdditionalProperties.GetHashCode();
           }

           return hashCode;
       }
   }

   /// <summary>
   ///     To validate all properties of the instance
   /// </summary>
   /// <param name="validationContext">Validation context</param>
   /// <returns>Validation Result</returns>
   protected IEnumerable<ValidationResult> BaseValidate(ValidationContext validationContext)
   {
       yield break;
   }
}

Upon investigation, it appears that if an object is of type Dictionary, both Newtonsoft.Json and System.Text.Json will treat the object solely as a Dictionary, disregarding any other properties on the object. Moreover, the test suite seems to lack any tests that verify whether a JSON string can actually be deserialized. This raises the question: has the C# code generation ever been functional?

@KRSogaard KRSogaard changed the title [BUG][CSHARP] [BUG][CSHARP] DeserializeObject putting everything in AdditionalProperties Oct 9, 2023
@KRSogaard KRSogaard changed the title [BUG][CSHARP] DeserializeObject putting everything in AdditionalProperties [BUG][CSHARP] DeserializeObject putting everything in inherited Dictionary Oct 9, 2023
@devhl-labs
Copy link
Contributor

The generichost library had a sample called something like manualtest which verifies round trip de/serialization, so yes it works. It covers inheritance too, but maybe not inheriting a dictionary. Though im not sure what would be expected with a dictionary of objects.

@KRSogaard
Copy link
Contributor Author

This does seem like a blocker for any C# usage. I agree that inheriting from a Directory is unnecessary, especially at the cost of breaking the serialization of normal objects. However, I was unable to locate where this inheritance is implemented in the code.

@devhl-labs
Copy link
Contributor

devhl-labs commented Oct 10, 2023

This spec is causing an error:
Illegal schema found with non-object type combined with properties, no properties should be defined:

Seems to be coming from dozens of string properties which also have their own properties such as this status property
image

I ran the spec through a validator and it says it is good, but this library seems to disagree. @wing328 Can you clear this up?
image

@KRSogaard
Copy link
Contributor Author

I believe that the issue identified by @devhl-labs is distinct from the one I've encountered. To address my specific issue, I created a custom version of the Plaid specification where I modified all instances of additionalProperties: true to additionalProperties: false. This modification eliminated the inheritance from Directory<string, object> and allowed the models to be successfully deserialized.

I'm curious about the reasoning behind inheriting from Dictionary. If the intent is to enable data access as if it were a dictionary or map, this functionality doesn't appear to be fully implemented. To achieve that, the following logic could be added:

public class MyModel : Dictionary<string, object>
{
    public string LinkToken { get; set; }
    public IDictionary<string, object> AdditionalProperties { get; set; }
    
    public new object this[string key]
    {
        get
        {
            if (key == nameof(LinkToken))
            {
                return LinkToken;
            }
            return AdditionalProperties[key];
        }
        set
        {
            if (key == nameof(LinkToken))
            {
                LinkToken = value as string;
            }
            AdditionalProperties.Add(key, value);
        }
    }
}

Alternatively, a cleaner approach might be to implement the IDictionary<string, object> interface rather than inheriting from Dictionary.

@devhl-labs
Copy link
Contributor

devhl-labs commented Oct 12, 2023

Interesting, we see this in the samples too. Zebra only inherits dictionary with additionalProperties enabled. I don't think it is supposed to.

image
image

The additionalProperties should just go in here instead of inheriting dictionary. @wing328 Is this intended? Do I need to patch it in the abstract csharp? Seems like this must be a bug in the default generator, because how could it inherit correctly if additionalProperties forces it to inherit dictionary? C# does not have multinheritence so that can't work.
image

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

Successfully merging a pull request may close this issue.

2 participants