-
-
Notifications
You must be signed in to change notification settings - Fork 178
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
fix: updated unit-tests to test round-trip serialization. #1833
base: master
Are you sure you want to change the base?
fix: updated unit-tests to test round-trip serialization. #1833
Conversation
✅ Deploy Preview for modelina canceled.
|
efa4810
to
3cef088
Compare
Pull Request Test Coverage Report for Build 8066939011Details
💛 - Coveralls |
Gonna take a look after some lunch ✌️ |
@@ -85,7 +85,7 @@ internal class TestConverter : JsonConverter<Test> | |||
} | |||
if (propertyName == \\"enumProp\\") | |||
{ | |||
var value = EnumTestExtensions.ToEnumTest(JsonSerializer.Deserialize<dynamic>(ref reader, options)); | |||
var value = EnumTestExtensions.ToEnumTest(JsonSerializer.Deserialize<string>(ref reader, options)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What happens when the enum value can both be a number and string? 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I get what you're asking but to avoid confusion, can you kindly post an example schema? Thanks :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
{ "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; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 🙂
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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:
- You want me to implement the above,
- You are happy to merge the code as-is, or
- You can think of a better way 😄
I'm happy with any of the above options, so just let me know 🙂.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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;
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 👍
Quality Gate failedFailed conditions |
This pull request has been automatically marked as stale because it has not had recent activity 😴 It will be closed in 120 days if no further activity occurs. To unstale this pull request, add a comment with detailed explanation. There can be many reasons why some specific pull request 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 pull request forward. Connect with us through one of many communication channels we established here. Thank you for your patience ❤️ |
Description
Related Issue
Fixes #1832
Checklist
npm run lint
).npm run test
).Additional Notes