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

fix reading json with naming policy #42302

Closed
wants to merge 5 commits into from
Closed
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,9 @@ internal class EnumConverter<T> : JsonConverter<T>
private readonly JsonNamingPolicy? _namingPolicy;

private readonly ConcurrentDictionary<ulong, JsonEncodedText> _nameCache;
private readonly ConcurrentDictionary<JsonEncodedText, string> _sourceNameCache;
private readonly bool _needRestoreSourceName;
private readonly JsonSerializerOptions _serializerOptions;

// This is used to prevent flooding the cache due to exponential bitwise combinations of flags.
// Since multiple threads can add to the cache, a few more values might be added.
Expand All @@ -43,7 +46,9 @@ public EnumConverter(EnumConverterOptions converterOptions, JsonNamingPolicy? na
{
_converterOptions = converterOptions;
_namingPolicy = namingPolicy;
_serializerOptions = serializerOptions;
_nameCache = new ConcurrentDictionary<ulong, JsonEncodedText>();
_sourceNameCache = new ConcurrentDictionary<JsonEncodedText, string>();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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>();

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Author

@2m0nd 2m0nd Sep 17, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?


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


string[] names = Enum.GetNames(TypeToConvert);
Array values = Enum.GetValues(TypeToConvert);
Expand All @@ -67,7 +72,12 @@ public EnumConverter(EnumConverterOptions converterOptions, JsonNamingPolicy? na
namingPolicy == null
? JsonEncodedText.Encode(name, encoder)
: FormatEnumValue(name, encoder));

if (namingPolicy != null)
_sourceNameCache.TryAdd(FormatEnumValue(name, encoder), name);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

}

_needRestoreSourceName = namingPolicy != null;
}

public override T Read(ref Utf8JsonReader reader, Type typeToConvert, JsonSerializerOptions options)
Expand Down Expand Up @@ -313,6 +323,9 @@ internal override T ReadWithQuotes(ref Utf8JsonReader reader)
{
string? enumString = reader.GetString();

if (_needRestoreSourceName && enumString != null)
_sourceNameCache.TryGetValue(FormatEnumValue(enumString, _serializerOptions.Encoder), out enumString);

// Try parsing case sensitive first
if (!Enum.TryParse(enumString, out T value)
&& !Enum.TryParse(enumString, ignoreCase: true, out value))
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,96 @@
// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.

using System.IO;
using System.Linq;
using System.Threading.Tasks;
using Xunit;
using Xunit.Abstractions;

namespace System.Text.Json.Serialization.Tests
{
public class EnumConverterWithNamingPolicyTests
{
private readonly ITestOutputHelper _outputHelper;

public EnumConverterWithNamingPolicyTests(ITestOutputHelper outputHelper)
{
_outputHelper = outputHelper;
}

public class SnakeCaseNamingPolicy : JsonNamingPolicy
{
public override string ConvertName(string name)
{
if (name == null)
{
throw new ArgumentNullException(nameof(name));
}
var result = new StringBuilder();
for (var i = 0; i < name.Length; i++)
{
var c = name[i];
if (i == 0)
{
result.Append(char.ToLower(c));
}
else
{
if (char.IsUpper(c))
{
result.Append('_');
result.Append(char.ToLower(c));
}
else
{
result.Append(c);
}
}
}
return result.ToString();
}

}
public enum TestType
{
None,
ValueOne,
ValueTwo,
}

public class ObjectWithEnumProperty
{
public TestType TestType { get; set; }
}

[Fact]
public void TestEnumCase()
{
var namingPolicy = new SnakeCaseNamingPolicy();

var opts = new JsonSerializerOptions()
{
PropertyNamingPolicy = namingPolicy,
DictionaryKeyPolicy = namingPolicy,
Converters =
{
new JsonStringEnumConverter(namingPolicy)
}
};

var enumValues = Enum.GetValues(typeof(TestType)).Cast<TestType>();
foreach (var v in enumValues)
{
var sourceObject = new ObjectWithEnumProperty()
{
TestType = v
};

var json = JsonSerializer.Serialize(sourceObject, opts);
_outputHelper.WriteLine(json);
var deserializedObject = JsonSerializer.Deserialize<ObjectWithEnumProperty>(json, opts);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

@2m0nd 2m0nd Sep 17, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Assert.Equal(sourceObject.TestType, deserializedObject.TestType);
}
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -88,6 +88,7 @@
<Compile Include="Serialization\CyclicTests.cs" />
<Compile Include="Serialization\DeserializationWrapper.cs" />
<Compile Include="Serialization\EnumConverterTests.cs" />
<Compile Include="Serialization\EnumConverterWithNamingPolicyTests.cs" />
<Compile Include="Serialization\EnumTests.cs" />
<Compile Include="Serialization\ExceptionTests.cs" />
<Compile Include="Serialization\ExtensionDataTests.cs" />
Expand Down