wivuu / Wivuu.JsonPolymorphism

Adds System.Text.Json serializer support for polymorphism with a discriminator attribute
MIT License
25 stars 3 forks source link

Deserialization Case Sensitivity #6

Closed Aragas closed 2 years ago

Aragas commented 2 years ago

In it's current state, when passing a custom enum converter that sets the casing to camelCase, deserialization is still done in PascalCase, because Enum.TryParse is used instead of the converter's Read() method. This pull request checks if there is a custom Enum converter and uses it instead, switching to the old behaviour if no converter is found

onionhammer commented 2 years ago

@Aragas thanks for the contribution - are you able to determine whether there is a converter at compile-time instead of run-time?

Are you saying the root issue is that this library doesn't handle the the discriminator if the case doesnt match the enum? i.e. "turtle" doesnt match "Animal.Turtle" ?

Aragas commented 2 years ago

@Aragas thanks for the contribution - are you able to determine whether there is a converter at compile-time instead of run-time?

I don't think we'll be able to determine the converter at compile time, since the CanConvert() method might return different result at the whole applications lifetime.

Are you saying the root issue is that this library doesn't handle the the discriminator if the case doesnt match the enum? i.e. "turtle" doesnt match "Animal.Turtle" ?

Correct, this is the case. You just need to add something like new JsonStringEnumConverter(JsonNamingPolicy.CamelCase) to the converters and then serialization will respect the custom converter, but not deserialization

onionhammer commented 2 years ago

I think we may be able to switch to using the new 'JsonElement.Deserialize()' method introduced in system.text.json version 6 that may fix your issue. Can you create another PR with a failing test case demonstrating this issue?

onionhammer commented 2 years ago

Here's sample generated code using 'JsonElement.Deserialize', one issue with this would be consumers would need to upgrade System.Text.Json, but it simplifies the logic quite a bit

image

onionhammer commented 2 years ago

Try the latest version and see if this fixes your issue

Aragas commented 2 years ago

Yep, it works! I agree, this is a way more elegant solution. Thanks for the quick fix and release!