fix: updated unit-tests to test round-trip serialization.

[RowlandBanks]

Description

• Adds a new unit-test proving a model can be round-tripped ✔️
• Fixes any bugs encountered ✔️

Related Issue

Fixes #1832

Checklist

• The code follows the project's coding standards and is properly linted (npm run lint).
• Tests have been added or updated to cover the changes.
• Documentation has been updated to reflect the changes.
• All tests pass successfully locally.(npm run test).

Additional Notes

[netlify]

✅ Deploy Preview for modelina canceled.

Name	Link
🔨 Latest commit	3816e7c
🔍 Latest deploy log	https://app.netlify.com/sites/modelina/deploys/65ddfb07d396db000834f1c2

[coveralls]

Pull Request Test Coverage Report for Build 8066939011

Details

• -1 of 8 (87.5%) changed or added relevant lines in 1 file are covered.
• No unchanged relevant lines lost coverage.
• Overall coverage decreased (-0.02%) to 92.283%

---

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
src/generators/csharp/presets/JsonSerializerPreset.ts	7	8	87.5%

Totals
Change from base Build 8062206205:	-0.02%
Covered Lines:	5998
Relevant Lines:	6330

---

💛 - Coveralls

[jonaslagoni]

Gonna take a look after some lunch ✌️

[jonaslagoni]

What happens when the enum value can both be a number and string? 🤔

[RowlandBanks]

I think I get what you're asking but to avoid confusion, can you kindly post an example schema? Thanks :)

[jonaslagoni]

{  "enum": ["string", 2]}

It's actually part of the current enum, that can both be boolean, string, number and JSON encoded string:

modelina/test/generators/csharp/presets/__snapshots__/JsonSerializerPreset.spec.ts.snap

Line 177 in 4bf0b56

case EnumTest.NUMBER_2: return 2;

modelina/test/generators/csharp/presets/__snapshots__/JsonSerializerPreset.spec.ts.snap

Line 189 in 4bf0b56

case 2: return EnumTest.NUMBER_2;

[jonaslagoni]

Happy to accept a PR that partially fix it, as long as it is improved compared to the current approach, and we create a followup issue with the restriction 🙂

[RowlandBanks]

I've dug into this a little bit and the code we would end up generating is a little bit ugly.

Crucially, System.Text.Json will return a dynamic value as a JsonElement, so we would need to look for JsonElement's and handle all the possible scenarios. The code ends up looking like:

if (JsonSerializer.Deserialize<dynamic>(ref reader, options) is JsonElement jsonElement)
{
    if (jsonElement.ValueKind == JsonValueKind.Null)
    {
        instance.EnumProp = null;
        continue;
    }
    else if (jsonElement.ValueKind == JsonValueKind.String)
    {
        instance.EnumProp = DynamicEnumExtensions.ToDynamicEnum(jsonElement.GetString());
        continue;
    }
    else if (jsonElement.ValueKind == JsonValueKind.Number)
    {
        instance.EnumProp = DynamicEnumExtensions.ToDynamicEnum(jsonElement.GetInt32());
        continue;
    }
    else if (jsonElement.ValueKind == JsonValueKind.Object ||
                jsonElement.ValueKind == JsonValueKind.Array)
    {
        instance.EnumProp = DynamicEnumExtensions.ToDynamicEnum(jsonElement.ToString());
        continue;
    }
    else if (jsonElement.ValueKind == JsonValueKind.True || jsonElement.ValueKind == JsonValueKind.False)
    {
        instance.EnumProp = DynamicEnumExtensions.ToDynamicEnum(jsonElement.GetBoolean());
        continue;
    }
    else
    {
        continue;
    }
}
else
{
    var value = EnumTestExtensions.ToEnumTest(JsonSerializer.Deserialize<string>(ref reader, options));
    instance.EnumTest = value;
    continue;
}

Let me know if:

1. You want me to implement the above,
2. You are happy to merge the code as-is, or
3. You can think of a better way 😄

I'm happy with any of the above options, so just let me know 🙂.

[jonaslagoni]

Since EnumTestExtensions.ToEnumTest takes a dynamic type, is it not possible to mix the two? And in which scenario will the first if statement fail? Or can we keep just that one 🤔

(or something similar, just skeleton syntax)

if (JsonSerializer.Deserialize<dynamic>(ref reader, options) is JsonElement jsonElement)
{
    var enumValue;
    if (jsonElement.ValueKind == JsonValueKind.Null)
    {
        enumValue = null;
    }
    else if (jsonElement.ValueKind == JsonValueKind.String)
    {
        enumValue = jsonElement.GetString()
    }
    else if (jsonElement.ValueKind == JsonValueKind.Number)
    {
        enumValue = jsonElement.GetInt32()
    }
    else if (jsonElement.ValueKind == JsonValueKind.Object ||
                jsonElement.ValueKind == JsonValueKind.Array)
    {
        enumValue = jsonElement.GetString()
    }
    else if (jsonElement.ValueKind == JsonValueKind.True || jsonElement.ValueKind == JsonValueKind.False)
    {
        enumValue = jsonElement.GetBoolean()
    }
    else
    {
        continue;
    }
    var value = EnumTestExtensions.ToEnumTest(enumValue);
    instance.EnumTest = value;
    continue;
}
else
{
    var value = EnumTestExtensions.ToEnumTest(JsonSerializer.Deserialize<string>(ref reader, options));
    instance.EnumTest = value;
    continue;
}

[RowlandBanks]

Sorry, yes - my posted code is pseudo-code as well. The key point I was trying to highlight is that we will need to inspect JsonElement, and so will have that big if (jsonElement.TokenType== xxx) block.

In practice, I don't think it will ever fall through to the else - I was planning to try out a few scenarios as part of finishing the implementation though to be sure.

Since EnumTestExtensions.ToEnumTest takes a dynamic type, is it not possible to mix the two?

Do you mean "Add code into ToEnumTest to do the JsonElement check in there"? If so, we don't want to do that I think - we don't want serializer-specific code in the ToEnumTest method I think?

So, just to confirm we want to do option 1 (implement the code), right?

[jonaslagoni]

Ahh okay, yea, go for it 👍 If the current code with var value = EnumTestExtensions.ToEnumTest(JsonSerializer.Deserialize<dynamic>(ref reader, options)); does not work, this is definitely better 👍

[sonarcloud]

Quality Gate failed

Failed conditions
24.9% Duplication on New Code (required ≤ 3%)

See analysis details on SonarCloud