fix reading json with naming policy

[2m0nd]

Maybe this solution resolve problem #31619?

[Dotnet-GitSync-Bot]

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

[dnfadmin]

All CLA requirements met.

[watfordgnf]

If json were not serialized by JsonSerializer with opts would this still work?

[Theory]
[InlineData(@"""none""", TestType.None)]
[InlineData(@"0", TestType.None)]
[InlineData(@"""value_one""", TestType.ValueOne)]
[InlineData(@"1", TestType.ValueOne)]
[InlineData(@"""value_two""", TestType.ValueTwo)]
[InlineData(@"2", TestType.ValueTwo)]
public void TestEnumNamingPolicy(string json, TestType expectedValue)
{
    // ...opts as above
    string json = $@"{{ ""test_type"": {json} }}";
    var deserializedObject = JsonSerializer.Deserialize<ObjectWithEnumProperty>(json, opts);
    Assert.Equal(expectedValue, deserializedObject.TestType);
}

I ask because it appears that the cache for enum names is only ever populated if you write the value first.

[2m0nd]

Cache is initialized by EnumConverter constructor
https://github.com/dotnet/runtime/pull/42302/files/2a7699d12692176a00de0d3892960d383a13e2c5#diff-1836df549fbe8f438e0cfc38f3fc07daR76-R77 with only condition that naming policy exists.
Test cases will work if opts contains a JsonStringEnumConverter with snake case naming policy
https://github.com/dotnet/runtime/pull/42302/files/2a7699d12692176a00de0d3892960d383a13e2c5#diff-212a88a736cc2af2d4071f394407acd5R71-R79

[watfordgnf]

Thanks, I was unable to get it work pass locally, looks like it may be an issue on my end!

[2m0nd]

Sory, i was wrong, cache initializer will be for first 64 enum values.
https://github.com/dotnet/runtime/pull/42302/files/2a7699d12692176a00de0d3892960d383a13e2c5#diff-1836df549fbe8f438e0cfc38f3fc07daR59-R64
#42302 (comment)

[jozkee]

Similar to DictionaryKeyPolicy I think the naming policies should be a one-way operation, I'm not sure if we want to enable this for enums but not for dictionary keys.

See related threads:
#30008 (comment)
#30142

However, on Newtonsoft.Json, the StringEnumConverter with a NamingStrategy seem to round-trip without problem; it may be worth checking what is being performed on their side and evaluate if we can do something alike, specially for the Flags enum case.

[jozkee]

Consider initializing this field only when required, i.e: namingPolicy not null and the enum contain any names.

[jozkee]

It could be better to use the encoded string version of the enum value as TKey and the enum value as TValue so when you call TryGetValue while reading you don't have to create the JsonEncodedText which IIRC is expensive.

Suggested change

	_sourceNameCache = new ConcurrentDictionary<JsonEncodedText, string>();
	_sourceNameCache = new ConcurrentDictionary<string, T>();

[2m0nd]

Thanks for the code review.

1. I replace cache type to ConcurrentDictionary<string, string>
2. Initialize only need add new value.
3. And add support Flags
4. Add test case TestFlagsEnumValuesWithNamingPolicy

and update this pull request.

I just don't know what to do in cases where the NameCacheSizeSoftLimit = 64 restriction occurs.

[2m0nd]

And I'll try to see how it is done in Newtonsoft.Json StringEnumConverter.

[2m0nd]

On Newtonsoft.Json create cache by all enum names, maybe there is nothing dangerous when once (in constructor or static code) a cache is created for a type for all its values?

Is there a danger of a cache overflow only if it is added to the cache at the user's request (for example, write json)?

Current version of type EnumConverter contains two point adding values to cache: 1) constructor and 2) method Write(...). I think that NameCacheSizeSoftLimit added when cache writes on method Write(...).

Maybe adding cache values in method's write & writequotes not actual? And check NameCacheSizeSoftLimit in constructor not actual?

runtime/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Value/EnumConverter.cs

Line 388 in b8a6677

_nameCache.TryAdd(key, formatted);

runtime/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Value/EnumConverter.cs

Line 199 in b8a6677

_nameCache.TryAdd(key, formatted);

[2m0nd]

I realized that adding to the cache in Write(...) methods is required in the case of flags when intermediate values.
However, in the constructor, it may not be worth checking the NameCacheSizeSoftLimit?

runtime/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Value/EnumConverter.cs

Line 58 in b8a6677

if (_nameCache.Count >= NameCacheSizeSoftLimit)

Although, for sure, there are no such large enums, so we can assume that size 64 is enough for everyone.

[jozkee]

This won't work for Flags enums and you also don't want to cache all the possible combinations since that would cause a security vulnerability; see #36726 (comment).

[ericstj]

@layomia @eiriktsarpalis can you please have a look? I'm not sure if all of @jozkee's feedback has been addressed but we should have a look and identify next steps here.

[ericstj]

Ping. Reviewers, please have a look and provide next steps

[layomia]

@2m0nd thanks for your work on this PR. Based on the potential implementations here, I believe that this feature is not feasible.

We are trying to map the result of a naming policy conversion back to enum property values. On the surface, this is no different from what we do when deserializing object properties using JsonSerializerOptions.PropertyNamingPolicy.

The complication here is with flag combination values. We cannot cache all possible transformed enum property name values, of which there could be several, opening the door to security vulnerabilities (#36726 (comment)). In addition, any fallback to iterate through all the properties of a given enum value, calculate their naming-policy transformed values, then map that back to the value we parsed from JSON will lead to bad performance. This issue is compounded when dealing with enum flags, esp with arbitrary order. It is easy for malicious agents to craft JSON which could trigger this bad fallback and lead to denial of service issues when the serializer tries to process the input.

One potential solution here is to cache some (a max of ~64 per NameCacheSizeSoftLimit ) of the transformed enum prop name values like you're doing in this PR. We would not include flag combination values. For flag combinations and single values not included in the cache, we would throw JsonException as those transformed values would not be recognized. I do not believe we should go down this route because it is inconsistent behavior which might not be easy to maintain or communicate to users. I believe we should stick to the simple behavior of the enum naming policy being a one-way transformation (serialization only). The workaround would be for users to write custom converters which know about a closed set of expected enum values on deserialization.

My inclination is thus for us to close this PR and provide a sample in the docs for a custom converter that can provide this behavior. FYI @steveharter @eiriktsarpalis @jozkee @GrabYourPitchforks @tdykstra, thoughts?

---

However, on Newtonsoft.Json, the StringEnumConverter with a NamingStrategy seem to round-trip without problem; it may be worth checking what is being performed on their side and evaluate if we can do something alike, specially for the Flags enum case.

Newtonsoft.Json appears (test, implementation) to compute and cache transformed enum flag combination values on the fly given the naming policy. This is susceptible to the security vulnerabilities we've highlighted above. We should not take this implementation/support as precedent.

[layomia]

Per my comments above #42302 (review), I'll close this PR. @2m0nd thanks for your work on this.