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

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

Open
RowlandBanks opened this issue Feb 26, 2024 · 4 comments · May be fixed by #1833
Open

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

RowlandBanks opened this issue Feb 26, 2024 · 4 comments · May be fixed by #1833

Comments

@RowlandBanks
Copy link
Contributor

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 RowlandBanks linked a pull request Feb 26, 2024 that will close this issue
4 tasks
@RowlandBanks
Copy link
Contributor Author

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

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

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

/rtm

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

Successfully merging a pull request may close this issue.

1 participant