Skip to content

Commit

Permalink
Add logic to properly honor naming policy when serializing flag enums (…
Browse files Browse the repository at this point in the history 
…#36726)

* Add logic to properly honor naming policy when serializing flag enums

* Cache JsonEncodedText, add optimization to-do, and use .Contains

* Fix CI failure

* Apply feedback, use enum value as lookup key, cache result even when naming policy is not used

* Use ulong as name cache key and perform caching at warm up

* Address feedback - set cap for name cache, compute cache key properly, and add more tests
  • Loading branch information
@layomia
layomia committed Jun 3, 2020
1 parent 5bdb598 commit f511ec4
Show file tree
Hide file tree
Showing 4 changed files with 244 additions and 38 deletions.
173 changes: 139 additions & 34 deletions ...ies/System.Text.Json/src/System/Text/Json/Serialization/Converters/Value/EnumConverter.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3,8 +3,10 @@
// See the LICENSE file in the project root for more information.

using System.Collections.Concurrent;
using System.Diagnostics;
using System.Globalization;
using System.Runtime.CompilerServices;
using System.Text.Encodings.Web;

namespace System.Text.Json.Serialization.Converters
{
Expand All @@ -16,32 +18,57 @@ internal class EnumConverter<T> : JsonConverter<T>
// Odd type codes are conveniently signed types (for enum backing types).
private static readonly string? s_negativeSign = ((int)s_enumTypeCode % 2) == 0 ? null : NumberFormatInfo.CurrentInfo.NegativeSign;

private const string ValueSeparator = ", ";

private readonly EnumConverterOptions _converterOptions;
private readonly JsonNamingPolicy _namingPolicy;
private readonly ConcurrentDictionary<string, string>? _nameCache;

private readonly JsonNamingPolicy? _namingPolicy;

private readonly ConcurrentDictionary<ulong, JsonEncodedText> _nameCache;

// 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.
private const int NameCacheSizeSoftLimit = 64;

public override bool CanConvert(Type type)
{
return type.IsEnum;
}

public EnumConverter(EnumConverterOptions options)
: this(options, namingPolicy: null)
public EnumConverter(EnumConverterOptions converterOptions, JsonSerializerOptions serializerOptions)
: this(converterOptions, namingPolicy: null, serializerOptions)
{
}

public EnumConverter(EnumConverterOptions options, JsonNamingPolicy? namingPolicy)
public EnumConverter(EnumConverterOptions converterOptions, JsonNamingPolicy? namingPolicy, JsonSerializerOptions serializerOptions)
{
_converterOptions = options;
if (namingPolicy != null)
{
_nameCache = new ConcurrentDictionary<string, string>();
}
else
_converterOptions = converterOptions;
_namingPolicy = namingPolicy;
_nameCache = new ConcurrentDictionary<ulong, JsonEncodedText>();

string[] names = Enum.GetNames(TypeToConvert);
Array values = Enum.GetValues(TypeToConvert);
Debug.Assert(names.Length == values.Length);

JavaScriptEncoder? encoder = serializerOptions.Encoder;

for (int i = 0; i < names.Length; i++)
{
namingPolicy = JsonNamingPolicy.Default;
if (_nameCache.Count >= NameCacheSizeSoftLimit)
{
break;
}

T value = (T)values.GetValue(i)!;
ulong key = ConvertToUInt64(value);
string name = names[i];

_nameCache.TryAdd(
key,
namingPolicy == null
? JsonEncodedText.Encode(name, encoder)
: FormatEnumValue(name, encoder));
}
_namingPolicy = namingPolicy;
}

public override T Read(ref Utf8JsonReader reader, Type typeToConvert, JsonSerializerOptions options)
Expand Down Expand Up @@ -138,40 +165,46 @@ public override T Read(ref Utf8JsonReader reader, Type typeToConvert, JsonSerial
return default;
}

private static bool IsValidIdentifier(string value)
{
// Trying to do this check efficiently. When an enum is converted to
// string the underlying value is given if it can't find a matching
// identifier (or identifiers in the case of flags).
//
// The underlying value will be given back with a digit (e.g. 0-9) possibly
// preceded by a negative sign. Identifiers have to start with a letter
// so we'll just pick the first valid one and check for a negative sign
// if needed.
return (value[0] >= 'A' &&
(s_negativeSign == null || !value.StartsWith(s_negativeSign)));
}

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))
{
string original = value.ToString();
if (_nameCache != null && _nameCache.TryGetValue(original, out string? transformed))
ulong key = ConvertToUInt64(value);

if (_nameCache.TryGetValue(key, out JsonEncodedText formatted))
{
writer.WriteStringValue(transformed);
writer.WriteStringValue(formatted);
return;
}

string original = value.ToString();
if (IsValidIdentifier(original))
{
transformed = _namingPolicy.ConvertName(original);
writer.WriteStringValue(transformed);
if (_nameCache != null)
// We are dealing with a combination of flag constants since
// all constant values were cached during warm-up.
JavaScriptEncoder? encoder = options.Encoder;

if (_nameCache.Count < NameCacheSizeSoftLimit)
{
_nameCache.TryAdd(original, transformed);
formatted = _namingPolicy == null
? JsonEncodedText.Encode(original, encoder)
: FormatEnumValue(original, encoder);

writer.WriteStringValue(formatted);

_nameCache.TryAdd(key, formatted);
}
else
{
// We also do not create a JsonEncodedText instance here because passing the string
// directly to the writer is cheaper than creating one and not caching it for reuse.
writer.WriteStringValue(
_namingPolicy == null
? original
: FormatEnumValueToString(original, encoder));
}

return;
}
}
Expand Down Expand Up @@ -212,5 +245,77 @@ public override void Write(Utf8JsonWriter writer, T value, JsonSerializerOptions
break;
}
}

// This method is adapted from Enum.ToUInt64 (an internal method):
// https://github.com/dotnet/runtime/blob/bd6cbe3642f51d70839912a6a666e5de747ad581/src/libraries/System.Private.CoreLib/src/System/Enum.cs#L240-L260
private static ulong ConvertToUInt64(object value)
{
Debug.Assert(value is T);
ulong result = s_enumTypeCode switch
{
TypeCode.Int32 => (ulong)(int)value,
TypeCode.UInt32 => (uint)value,
TypeCode.UInt64 => (ulong)value,
TypeCode.Int64 => (ulong)(long)value,
TypeCode.SByte => (ulong)(sbyte)value,
TypeCode.Byte => (byte)value,
TypeCode.Int16 => (ulong)(short)value,
TypeCode.UInt16 => (ushort)value,
_ => throw new InvalidOperationException(),
};
return result;
}

private static bool IsValidIdentifier(string value)
{
// Trying to do this check efficiently. When an enum is converted to
// string the underlying value is given if it can't find a matching
// identifier (or identifiers in the case of flags).
//
// The underlying value will be given back with a digit (e.g. 0-9) possibly
// preceded by a negative sign. Identifiers have to start with a letter
// so we'll just pick the first valid one and check for a negative sign
// if needed.
return (value[0] >= 'A' &&
(s_negativeSign == null || !value.StartsWith(s_negativeSign)));
}

private JsonEncodedText FormatEnumValue(string value, JavaScriptEncoder? encoder)
{
Debug.Assert(_namingPolicy != null);
string formatted = FormatEnumValueToString(value, encoder);
return JsonEncodedText.Encode(formatted, encoder);
}

private string FormatEnumValueToString(string value, JavaScriptEncoder? encoder)
{
Debug.Assert(_namingPolicy != null);

string converted;
if (!value.Contains(ValueSeparator))
{
converted = _namingPolicy.ConvertName(value);
}
else
{
// todo: optimize implementation here by leveraging https://github.com/dotnet/runtime/issues/934.
string[] enumValues = value.Split(
#if BUILDING_INBOX_LIBRARY
ValueSeparator
#else
new string[] { ValueSeparator }, StringSplitOptions.None
#endif
);

for (int i = 0; i < enumValues.Length; i++)
{
enumValues[i] = _namingPolicy.ConvertName(enumValues[i]);
}

converted = string.Join(ValueSeparator, enumValues);
}

return converted;
}
}
}
4 changes: 2 additions & 2 deletions ...tem.Text.Json/src/System/Text/Json/Serialization/Converters/Value/EnumConverterFactory.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -19,15 +19,15 @@ public override bool CanConvert(Type type)
}

[PreserveDependency(
".ctor(System.Text.Json.Serialization.Converters.EnumConverterOptions)",
".ctor(System.Text.Json.Serialization.Converters.EnumConverterOptions, System.Text.Json.JsonSerializerOptions)",
"System.Text.Json.Serialization.Converters.EnumConverter`1")]
public override JsonConverter CreateConverter(Type type, JsonSerializerOptions options)
{
JsonConverter converter = (JsonConverter)Activator.CreateInstance(
typeof(EnumConverter<>).MakeGenericType(type),
BindingFlags.Instance | BindingFlags.Public,
binder: null,
new object[] { EnumConverterOptions.AllowNumbers },
new object[] { EnumConverterOptions.AllowNumbers, options },
culture: null)!;

return converter;
Expand Down
4 changes: 2 additions & 2 deletions src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonStringEnumConverter.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -55,15 +55,15 @@ public override bool CanConvert(Type typeToConvert)

/// <inheritdoc />
[PreserveDependency(
".ctor(System.Text.Json.Serialization.Converters.EnumConverterOptions, System.Text.Json.JsonNamingPolicy)",
".ctor(System.Text.Json.Serialization.Converters.EnumConverterOptions, System.Text.Json.JsonNamingPolicy, System.Text.Json.JsonSerializerOptions)",
"System.Text.Json.Serialization.Converters.EnumConverter`1")]
public override JsonConverter CreateConverter(Type typeToConvert, JsonSerializerOptions options)
{
JsonConverter converter = (JsonConverter)Activator.CreateInstance(
typeof(EnumConverter<>).MakeGenericType(typeToConvert),
BindingFlags.Instance | BindingFlags.Public,
binder: null,
new object?[] { _converterOptions, _namingPolicy },
new object?[] { _converterOptions, _namingPolicy, options },
culture: null)!;

return converter;
Expand Down
101 changes: 101 additions & 0 deletions src/libraries/System.Text.Json/tests/Serialization/EnumConverterTests.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
// See the LICENSE file in the project root for more information.

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

namespace System.Text.Json.Serialization.Tests
Expand Down Expand Up @@ -107,6 +108,21 @@ public void ConvertFileAttributes()
options = new JsonSerializerOptions();
options.Converters.Add(new JsonStringEnumConverter(allowIntegerValues: false));
Assert.Throws<JsonException>(() => JsonSerializer.Serialize((FileAttributes)(-1), options));

// Flag values honor naming policy correctly
options = new JsonSerializerOptions();
options.Converters.Add(new JsonStringEnumConverter(new SimpleSnakeCasePolicy()));

json = JsonSerializer.Serialize(
FileAttributes.Directory | FileAttributes.Compressed | FileAttributes.IntegrityStream,
options);
Assert.Equal(@"""directory, compressed, integrity_stream""", json);

json = JsonSerializer.Serialize(FileAttributes.Compressed & FileAttributes.Device, options);
Assert.Equal(@"0", json);

json = JsonSerializer.Serialize(FileAttributes.Directory & FileAttributes.Compressed | FileAttributes.IntegrityStream, options);
Assert.Equal(@"""integrity_stream""", json);
}

public class FileState
Expand Down Expand Up @@ -185,5 +201,90 @@ public void EnumWithConverterAttribute()
obj = JsonSerializer.Deserialize<MyCustomEnum>("2");
Assert.Equal(MyCustomEnum.Second, obj);
}

[Fact]
public static void EnumWithNoValues()
{
var options = new JsonSerializerOptions
{
Converters = { new JsonStringEnumConverter() }
};

Assert.Equal("-1", JsonSerializer.Serialize((EmptyEnum)(-1), options));
Assert.Equal("1", JsonSerializer.Serialize((EmptyEnum)(1), options));
}

public enum EmptyEnum { };

[Fact]
public static void MoreThan64EnumValuesToSerialize()
{
var options = new JsonSerializerOptions
{
Converters = { new JsonStringEnumConverter() }
};

for (int i = 0; i < 128; i++)
{
MyEnum value = (MyEnum)i;
string asStr = value.ToString();
string expected = char.IsLetter(asStr[0]) ? $@"""{asStr}""" : asStr;
Assert.Equal(expected, JsonSerializer.Serialize(value, options));
}
}

[Fact, OuterLoop]
public static void VeryLargeAmountOfEnumsToSerialize()
{
// Ensure we don't throw OutOfMemoryException.
// Multiple threads are used to ensure the approximate size limit
// for the name cache(a concurrent dictionary) is honored.

var options = new JsonSerializerOptions
{
Converters = { new JsonStringEnumConverter() }
};

const int MaxValue = 33554432; // Value for MyEnum.Z
Task[] tasks = new Task[MaxValue];

for (int i = 0; i < tasks.Length; i++)
{
tasks[i] = Task.Run(() => JsonSerializer.Serialize((MyEnum)i, options));
}

Task.WaitAll(tasks);
}

[Flags]
public enum MyEnum
{
A = 1 << 0,
B = 1 << 1,
C = 1 << 2,
D = 1 << 3,
E = 1 << 4,
F = 1 << 5,
G = 1 << 6,
H = 1 << 7,
I = 1 << 8,
J = 1 << 9,
K = 1 << 10,
L = 1 << 11,
M = 1 << 12,
N = 1 << 13,
O = 1 << 14,
P = 1 << 15,
Q = 1 << 16,
R = 1 << 17,
S = 1 << 18,
T = 1 << 19,
U = 1 << 20,
V = 1 << 21,
W = 1 << 22,
X = 1 << 23,
Y = 1 << 24,
Z = 1 << 25,
}
}
}

0 comments on commit f511ec4

Please sign in to comment.