From f511ec456f18f5ffa10aca88b29f4d8d644ba1b5 Mon Sep 17 00:00:00 2001 From: Layomi Akinrinade Date: Wed, 3 Jun 2020 13:36:55 -0400 Subject: [PATCH] Add logic to properly honor naming policy when serializing flag enums (#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 --- .../Converters/Value/EnumConverter.cs | 173 ++++++++++++++---- .../Converters/Value/EnumConverterFactory.cs | 4 +- .../Serialization/JsonStringEnumConverter.cs | 4 +- .../tests/Serialization/EnumConverterTests.cs | 101 ++++++++++ 4 files changed, 244 insertions(+), 38 deletions(-) diff --git a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Value/EnumConverter.cs b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Value/EnumConverter.cs index 5d0fb439c04f4..41f7302f7db55 100644 --- a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Value/EnumConverter.cs +++ b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Value/EnumConverter.cs @@ -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 { @@ -16,32 +18,57 @@ internal class EnumConverter : JsonConverter // 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? _nameCache; + + private readonly JsonNamingPolicy? _namingPolicy; + + private readonly ConcurrentDictionary _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(); - } - else + _converterOptions = converterOptions; + _namingPolicy = namingPolicy; + _nameCache = new ConcurrentDictionary(); + + 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) @@ -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; } } @@ -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; + } } } diff --git a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Value/EnumConverterFactory.cs b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Value/EnumConverterFactory.cs index 38ad9315e41d9..9faa76caec737 100644 --- a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Value/EnumConverterFactory.cs +++ b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Value/EnumConverterFactory.cs @@ -19,7 +19,7 @@ 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) { @@ -27,7 +27,7 @@ public override JsonConverter CreateConverter(Type type, JsonSerializerOptions o typeof(EnumConverter<>).MakeGenericType(type), BindingFlags.Instance | BindingFlags.Public, binder: null, - new object[] { EnumConverterOptions.AllowNumbers }, + new object[] { EnumConverterOptions.AllowNumbers, options }, culture: null)!; return converter; diff --git a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonStringEnumConverter.cs b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonStringEnumConverter.cs index ca363da7be16b..a5def112ae948 100644 --- a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonStringEnumConverter.cs +++ b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonStringEnumConverter.cs @@ -55,7 +55,7 @@ public override bool CanConvert(Type typeToConvert) /// [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) { @@ -63,7 +63,7 @@ public override JsonConverter CreateConverter(Type typeToConvert, JsonSerializer typeof(EnumConverter<>).MakeGenericType(typeToConvert), BindingFlags.Instance | BindingFlags.Public, binder: null, - new object?[] { _converterOptions, _namingPolicy }, + new object?[] { _converterOptions, _namingPolicy, options }, culture: null)!; return converter; diff --git a/src/libraries/System.Text.Json/tests/Serialization/EnumConverterTests.cs b/src/libraries/System.Text.Json/tests/Serialization/EnumConverterTests.cs index 3c15d9d0869dd..753aeaf2494d9 100644 --- a/src/libraries/System.Text.Json/tests/Serialization/EnumConverterTests.cs +++ b/src/libraries/System.Text.Json/tests/Serialization/EnumConverterTests.cs @@ -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 @@ -107,6 +108,21 @@ public void ConvertFileAttributes() options = new JsonSerializerOptions(); options.Converters.Add(new JsonStringEnumConverter(allowIntegerValues: false)); Assert.Throws(() => 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 @@ -185,5 +201,90 @@ public void EnumWithConverterAttribute() obj = JsonSerializer.Deserialize("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, + } } }