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

[API Proposal]: EnumConverter support for numeric/string in but always numeric out #61726

Closed
benmccallum opened this issue Nov 17, 2021 · 8 comments
Labels
api-suggestion Early API idea and discussion, it is NOT ready for implementation area-System.Text.Json untriaged New issue has not been triaged by the area owner

Comments

@benmccallum
Copy link

benmccallum commented Nov 17, 2021

Background and motivation

Right now with EnumConverterOptions it's easy enough to say "allow strings" and have "0" or "MemberName" be deserialized, but by opting in to that setting it also affects all the serialization and everything is serialized as the member name string representation.

i.e. Allow strings is checked first in serialization method and outputs string before falling back to numeric.

public override void Write(Utf8JsonWriter writer, T value, JsonSerializerOptions options)
{
// If strings are allowed, attempt to write it out as a string value
if (_converterOptions.HasFlag(EnumConverterOptions.AllowStrings))
{

In our transition from Newtonsoft.Json, we found a couple of places where our clients were passing in a string, in one case even the numeric representation as a string (straight from a radio button value) so we want to accept that still, but we always want to serialize and send out the numeric representation as we were before.

Right now I need to copy-paste-modify the standard factory and converter to achieve this, but I'd love it if this was support natively via some form of options.

API Proposal

I imagine this would take shape in some modification to System.Text.Json.Serialization.Converters.EnumConverterOptions, but exactly what I'm not sure.

API Usage

Replace the standard converter with a custom one as usual.

Alternative Designs

No response

Risks

Can't think of any. Perf would be same as current string in support.

@benmccallum benmccallum added the api-suggestion Early API idea and discussion, it is NOT ready for implementation label Nov 17, 2021
@dotnet-issue-labeler dotnet-issue-labeler bot added the untriaged New issue has not been triaged by the area owner label Nov 17, 2021
@dotnet-issue-labeler
Copy link

I couldn't figure out the best area label to add to this issue. If you have write-permissions please help me learn by adding exactly one area label.

@benmccallum
Copy link
Author

Tagging owners as I wasn't specific enough for the bot to pull the area out, sorry.
@eiriktsarpalis @layomia @krwq @steveharter

@eiriktsarpalis
Copy link
Member

Have you considered using JsonStringEnumConverter? Note that the converter doesn't support reading quoted numeric values, this is already tracked #58247. At this point you'd need a custom converter to cover your use case I'm afraid.

@ghost
Copy link

ghost commented Nov 17, 2021

Tagging subscribers to this area: @dotnet/area-system-text-json
See info in area-owners.md if you want to be subscribed.

Issue Details

Background and motivation

Right now with EnumConverterOptions it's easy enough to say "allow strings" and have "0" or "MemberName" be deserialized, but by opting in to that setting it also affects all the serialization and everything is serialized as the member name string representation.

i.e. Allow strings is checked first in serialization method and outputs string before falling back to numeric.

public override void Write(Utf8JsonWriter writer, T value, JsonSerializerOptions options)
{
// If strings are allowed, attempt to write it out as a string value
if (_converterOptions.HasFlag(EnumConverterOptions.AllowStrings))
{

In our transition from Newtonsoft.Json, we found a couple of places where our clients were passing in a string, in one case even the numeric representation as a string (straight from a radio button value) so we want to accept that still, but we always want to serialize and send out the numeric representation as we were before.

Right now I need to copy-paste-modify the standard factory and converter to achieve this, but I'd love it if this was support natively via some form of options.

API Proposal

I imagine this would take shape in some modification to System.Text.Json.Serialization.Converters.EnumConverterOptions, but exactly what I'm not sure.

API Usage

Replace the standard converter with a custom one as usual.

Alternative Designs

No response

Risks

Can't think of any. Perf would be same as current string in support.

Author: benmccallum
Assignees: -
Labels:

api-suggestion, area-System.Text.Json, untriaged

Milestone: -

@benmccallum
Copy link
Author

benmccallum commented Nov 18, 2021

Hi @eiriktsarpalis , I looked at JsonStringEnumConverter but I think it's just a factory which instantiates the core enum converter and passes the flags to support string:

public JsonStringEnumConverter(JsonNamingPolicy? namingPolicy = null, bool allowIntegerValues = true)
{
_namingPolicy = namingPolicy;
_converterOptions = allowIntegerValues
? EnumConverterOptions.AllowNumbers | EnumConverterOptions.AllowStrings
: EnumConverterOptions.AllowStrings;
}

The bug you mentioned would be a blocker for us, but I believe even if that's solved the result of JsonStringEnumConverter opting in to string conversion would still mean it'd force serialization out as the string repesentation (member name) as that flag is checked first.

public override void Write(Utf8JsonWriter writer, T value, JsonSerializerOptions options)
{
// If strings are allowed, attempt to write it out as a string value
if (_converterOptions.HasFlag(EnumConverterOptions.AllowStrings))
{

Workaround
I've solved my issue for now with a custom converter and factory based on the standard one, just working around some internals and always writing out the numeric value. Here's a gist if anyone has the same need.

I'll leave it in your court as to whether there's need for what I'm asking as an option flag. I think it'd be useful, but sure you've all got more important things to get on with :)

@eiriktsarpalis
Copy link
Member

In that case, I'm going to close this in favor of #58247.

@benmccallum
Copy link
Author

To be clear, #58247 isn't a fix for this. My custom converter is the only way.

@eiriktsarpalis
Copy link
Member

eiriktsarpalis commented Nov 26, 2021

IIRC the primary blocker even if #58247 were fixed is that you'd need to serialize as numbers? Yeah, there are all sorts of permutations wrt serialization vs deserialization behavior that the default converter won't be supporting. Recommend using a custom converter to solve your use case.

@ghost ghost locked as resolved and limited conversation to collaborators Dec 26, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api-suggestion Early API idea and discussion, it is NOT ready for implementation area-System.Text.Json untriaged New issue has not been triaged by the area owner
Projects
None yet
Development

No branches or pull requests

3 participants