Add logic to properly honor naming policy when serializing flag enums

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<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)
@@ -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;
+        }
     }
 }

src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Value/EnumConverterFactory.cs

@@ -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;

src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonStringEnumConverter.cs

@@ -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;

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<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
@@ -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,
+        }
     }
 }