asyncapi / modelina

A library for generating typed models based on inputs such as AsyncAPI, OpenAPI, and JSON Schema documents with high customization
https://modelina.org
Apache License 2.0
292 stars 169 forks source link

Exception occurs when using System.Text.Json deserializer to deserialize enum. #1832

Open RowlandBanks opened 6 months ago

RowlandBanks commented 6 months ago

The existing runtime-csharp unit-tests do not currently test deserialization. When adding such a test, a bug is exposed.

Steps to reproduce

Add the following unit-test in the runtime-csharp test solution under \test\models\json_serializer.

    [Test]
    public void TestRoundTrip()
    {
        const string expectedJsonString = "{\"house_number\":1,\"marriage\":true,\"members\":2,\"array_type\":[1,\"test\"],\"nestedObject\":{\"test\":\"test\"},\"enumTest\":\"test\",\"houseType\":\"flat\",\"roofType\":\"straw\",\"test_not_used\":2}";
        Address address = JsonSerializer.Deserialize<Address>(expectedJsonString) ?? throw new Exception("Failed to deserialize address.");
        string actualJsonString = JsonSerializer.Serialize(address);
        Assert.That(actualJsonString, Is.EqualTo(expectedJsonString));
    }

Run the test.

Expected outcome

The JSON is correctly deserialized to the relevant model and the test passes.

Actual outcome

An exception occurs:

Microsoft.CSharp.RuntimeBinder.RuntimeBinderException : The best overloaded method match for 'com.mycompany.app.json_serializer.EnumTestExtensions.ToEnumTest(string)' has some invalid arguments

Technical detail

This is caused by the following generated code in the generated JsonConverter<T>:

if (propertyName == "houseType")
{
    var value = HousingTypeExtensions.ToHousingType(JsonSerializer.Deserialize<dynamic>(ref reader, options));
    instance.HouseType = value;
    continue;
}

Note the call to Deserialize<dynamic> - this should be Deserialize<string>.

RowlandBanks commented 6 months ago

I've fixed the enum bug, but the unit-test has identified a second bug. The generator generates the following code:

if (instance.AdditionalProperties == null)
{
    instance.AdditionalProperties = new Dictionary<string, dynamic>();
}

var deserializedValue = JsonSerializer.Deserialize<Dictionary<string, dynamic>?>(ref reader, options);
instance.AdditionalProperties.Add(propertyName, deserializedValue);
continue;

Which fails on the call to Deserialize with the exception:

System.Text.Json.JsonException : The JSON value could not be converted to System.Collections.Generic.Dictionary`2[System.String,System.Object]. Path: $ | LineNumber: 0 | BytePositionInLine: 1.

However, the generated code looks very wrong - I don't think it should be deserializing to a dictionary, but to a simple dynamic value that gets added to the AdditionalProperties dictionary.

RowlandBanks commented 6 months ago

A third bug is that oneOf types are deserialized to a JsonElement, not to the actual type when being deserialized.

Therefore, when attempting to serialize a deserialized model, one gets the following error:

Microsoft.CSharp.RuntimeBinder.RuntimeBinderException : Operator '!=' cannot be applied to operands of type 'System.Text.Json.JsonElement' and '<null>'

For example,

"members": {
  "oneOf": [
    {
      "type": "string"
    },
    {
      "type": "number"
    },
    {
      "type": "boolean"
    }
  ]
},

will cause the error on this serailization code:

if (value.Members != null)
{
    // write property name and let the serializer serialize the value itself
    writer.WritePropertyName("members");
    JsonSerializer.Serialize(writer, value.Members, options);
}
RowlandBanks commented 6 months ago

I've fixed the bugs I've mentioned above, but I have a concern that deserializing to JsonElement will be confusing for consumers.

Perhaps this can be looked into in a future issue? However, without adding type-annotations to the output JSON, it's likely that there is no sensible way around this, so šŸ¤· .

RowlandBanks commented 6 months ago

/rtm

github-actions[bot] commented 2 months ago

This issue has been automatically marked as stale because it has not had recent activity :sleeping:

It will be closed in 120 days if no further activity occurs. To unstale this issue, add a comment with a detailed explanation.

There can be many reasons why some specific issue has no activity. The most probable cause is lack of time, not lack of interest. AsyncAPI Initiative is a Linux Foundation project not owned by a single for-profit company. It is a community-driven initiative ruled under open governance model.

Let us figure out together how to push this issue forward. Connect with us through one of many communication channels we established here.

Thank you for your patience :heart: