JsonStringEnumConverter parses numbers when the token is a String even when allowIntegerValues is false

[DonFrazier-zz]

It would be nice if the allowIntegerValues parameter recognized strings that are numbers and failed the conversion. Parsing any numeric into an enum can be hard to track down as a data problem and closing this loophole would help identify data errors closer to the their source.

[Fact]
public void ParseIntegerWhenItIsAString()
{
    var options = new JsonSerializerOptions();
    options.Converters.Add(new JsonStringEnumConverter(namingPolicy: null, allowIntegerValues: false));

    var A = JsonSerializer.Deserialize<Switch>(@"""1""", options);
    A.Should().Be(Switch.On);

    // an invalid integer can still derail an enum when it is enclosed in quotes.
    // allowIntegerValues logic should detect that the string is also a valid Number and disallow parsing
    var B = JsonSerializer.Deserialize<Switch>(@"""100""", options);
    B.Should().Be((Switch)100);

    try
    {
        JsonSerializer.Deserialize<Switch>("100", options);
    }
    catch(JsonException e)
    {
        // this message should appear when B is deserialized
        e.Message.Should().Be("The JSON value could not be converted to IssueExample+Switch. Path: $ | LineNumber: 0 | BytePositionInLine: 3.");
    }
}

[danmoseley]

Could you please update into a standalone example, such as can be pasted into a console app? The above code is incomplete.

[DonFrazier-zz]

public enum Switch { Off = 1, On = 2 };

public void ParseIntegerWhenItIsAString()
{
    var options = new JsonSerializerOptions();
    options.Converters.Add(new JsonStringEnumConverter(namingPolicy: null, allowIntegerValues: false));

    var A = JsonSerializer.Deserialize<Switch>(@"""1""", options);
    Console.WriteLine(A.ToString());

    // an invalid integer can still derail an enum when it is enclosed in quotes.
    // allowIntegerValues logic should detect that the string is also a valid Number and disallow parsing
    var B = JsonSerializer.Deserialize<Switch>(@"""100""", options);
    Console.WriteLine(B.ToString());

    try
    {
        JsonSerializer.Deserialize<Switch>("100", options);
    }
    catch(JsonException e)
    {
        // this message should appear when B is deserialized
        Console.WriteLine(e.Message);
//        e.Message.Should().Be("The JSON value could not be converted to IssueExample+Switch. Path: $ | LineNumber: 0 | BytePositionInLine: 3.");
    }
}

Update: I see I missed pasting in the Switch enum.

[DonFrazier-zz]

The gist is that I expected the deserialization into B to fail like it does in the try/catch block because 100 is not a defined enum value but because the Enum.Parse() method can convert it to an integer (I'm guessing that's what's going on anyway) the value is deserialized and results in an undefined enum value.

[layomia]

cc @JeremyKuhne

[JeremyKuhne]

allowIntegerValues is about whether JSON value number types are supported in reading/writing. What this option means is it will write a number value if it isn't defined (instead of a string value of the number). When reading a number value instead of a string value is supported. Naming should have probably stuck to JSON specification terms. :/

That said, I didn't really internalize that Enum.Parse will allow any number, and as such it probably should have just dumped a string representation of the underlying number rather than falling through to a different value type.

If I could do it again I would have this mean:

1. It dumps the numeric string for undefined enum values on write.
2. It prevents strings that start with a digit or sign and number values on the way in.

I don't know how tenable that sort of change (particularly bullet 2) is now given that anyone parsing enums as string numeric values would now be broken.

[DonFrazier-zz]

Personally I think the ENum parsing is incorrect and have had an extension method since extension methods were introduced. That shipped sailed long ago.

If you wanted to introduce this behavior you would need (sigh) another setting like "TreatIntegerStringsAsIntegers" with a default value of false. Anyone could opt-in to the new behavior if they wanted it.

I'd like to see an option that just says "allowDefinedEnumsOnly" and apply the parsing behavior to ENum type fields only. I realize you have to consider a bunch more things.

[JeremyKuhne]

@DonFrazier allowOnlyDefinedValues is an entirely rational thing to consider adding. It effectively is what I laid out above I think. If we kick strings that start with a digit or sign before we call TryParse it would be pretty efficient.

Is there anything else you think is missing?

[DonFrazier-zz]

For enums with the [Flags] Attribute you must allow parsing any combination of defined flags.

[Flags]
public enum WorkflowType
{
None = 0x00,
Offer = 0x01,
MetaDataOnly = 0x02,
DirectPublish = 0x04,
}
Here's a real one that I have to be able to parse any combo of flags.

[eiriktsarpalis]

Closing as duplicate of #58247.