PR feedback

Author: JamesNK

src/Grpc/JsonTranscoding/src/Microsoft.AspNetCore.Grpc.JsonTranscoding/GrpcJsonSettings.cs

@@ -50,8 +50,8 @@ public sealed class GrpcJsonSettings
     /// }
     /// </code>
     /// <para>
-    /// When <see cref="RemoveEnumPrefix"/> is set to <see langword="true"/>, the enum values above
-    /// will be read and written as <c>UNKNOWN</c> and <c>OK</c> instead of <c>STATUS_UNKNOWN</c>
+    /// <c>STATUS</c> prefix is removed from enum values when <see cref="RemoveEnumPrefix"/> is set to <see langword="true"/>.
+    /// The enum values above will be read and written as <c>UNKNOWN</c> and <c>OK</c> instead of <c>STATUS_UNKNOWN</c>
     /// and <c>STATUS_OK</c>.
     /// </para>
     /// </remarks>

src/Grpc/JsonTranscoding/src/Microsoft.AspNetCore.Grpc.JsonTranscoding/Internal/Json/EnumConverter.cs

@@ -20,18 +20,12 @@ public EnumConverter(JsonContext context) : base(context)
         switch (reader.TokenType)
         {
             case JsonTokenType.String:
-                var enumDescriptor = (EnumDescriptor?)Context.DescriptorRegistry.FindDescriptorByType(typeToConvert);
-                if (enumDescriptor == null)
-                {
-                    throw new InvalidOperationException($"Unable to resolve descriptor for {typeToConvert}.");
-                }
+                var enumDescriptor = (EnumDescriptor?)Context.DescriptorRegistry.FindDescriptorByType(typeToConvert)
+                    ?? throw new InvalidOperationException($"Unable to resolve descriptor for {typeToConvert}.");
 
                 var value = reader.GetString()!;
-                var valueDescriptor = JsonNamingHelpers.GetEnumFieldReadValue(enumDescriptor, value, Context.Settings);
-                if (valueDescriptor == null)
-                {
-                    throw new InvalidOperationException(@$"Error converting value ""{value}"" to enum type {typeToConvert}.");
-                }
+                var valueDescriptor = JsonNamingHelpers.GetEnumFieldReadValue(enumDescriptor, value, Context.Settings)
+                    ?? throw new InvalidOperationException(@$"Error converting value ""{value}"" to enum type {typeToConvert}.");
 
                 return ConvertFromInteger(valueDescriptor.Number);
             case JsonTokenType.Number:
@@ -51,11 +45,8 @@ public override void Write(Utf8JsonWriter writer, TEnum value, JsonSerializerOpt
         }
         else
         {
-            var enumDescriptor = (EnumDescriptor?)Context.DescriptorRegistry.FindDescriptorByType(value.GetType());
-            if (enumDescriptor == null)
-            {
-                throw new InvalidOperationException($"Unable to resolve descriptor for {value.GetType()}.");
-            }
+            var enumDescriptor = (EnumDescriptor?)Context.DescriptorRegistry.FindDescriptorByType(value.GetType())
+                ?? throw new InvalidOperationException($"Unable to resolve descriptor for {value.GetType()}.");
 
             var name = JsonNamingHelpers.GetEnumFieldWriteName(enumDescriptor, value, Context.Settings);
             if (name != null)

src/Grpc/JsonTranscoding/src/Microsoft.AspNetCore.Grpc.JsonTranscoding/Internal/Json/EnumNameHelpers.cs

@@ -48,44 +48,50 @@ internal static class JsonNamingHelpers
 
     private static EnumMapping GetEnumMapping(EnumDescriptor enumDescriptor)
     {
-        var enumType = enumDescriptor.ClrType;
-
-        EnumMapping? enumMapping;
-        lock (_enumMappings)
-        {
-            if (!_enumMappings.TryGetValue(enumType, out enumMapping))
-            {
-                _enumMappings[enumType] = enumMapping = GetEnumMapping(enumDescriptor.Name, enumType);
-            }
-        }
-
-        return enumMapping;
+        return _enumMappings.GetOrAdd(
+            enumDescriptor.ClrType,
+            static (t, descriptor) => GetEnumMapping(descriptor.Name, t),
+            enumDescriptor);
     }
 
     private static EnumMapping GetEnumMapping(string enumName, Type enumType)
     {
-        var enumFields = enumType.GetTypeInfo().DeclaredFields
+        var nameMappings = enumType.GetTypeInfo().DeclaredFields
             .Where(f => f.IsStatic)
             .Where(f => f.GetCustomAttributes<OriginalNameAttribute>().FirstOrDefault()?.PreferredAlias ?? true)
-            .ToList();
-
-        var writeMapping = enumFields.ToDictionary(
-            f => f.GetValue(null)!,
-            f =>
+            .Select(f =>
             {
                 // If the attribute hasn't been applied, fall back to the name of the field.
                 var fieldName = f.GetCustomAttributes<OriginalNameAttribute>().FirstOrDefault()?.Name ?? f.Name;
 
                 return new NameMapping
                 {
+                    Value = f.GetValue(null)!,
                     OriginalName = fieldName,
                     RemoveEnumPrefixName = GetEnumValueName(enumName, fieldName)
                 };
-            });
+            })
+            .ToList();
 
-        var removeEnumPrefixMapping = writeMapping.Values.ToDictionary(
-            m => m.RemoveEnumPrefixName,
-            m => m.OriginalName);
+        var writeMapping = nameMappings.ToDictionary(m => m.Value, m => m);
+
+        // Protobuf codegen prevents collision of enum names when the prefix is removed.
+        // For example, the following enum will fail to build because both fields would resolve to "OK":
+        //
+        // enum Status {
+        //     STATUS_OK = 0;
+        //     OK = 1;
+        // }
+        //
+        // Tooling error message:
+        // ----------------------
+        // Enum name OK has the same name as STATUS_OK if you ignore case and strip out the enum name prefix (if any).
+        // (If you are using allow_alias, please assign the same number to each enum value name.)
+        //
+        // Just in case it does happen, map to the first value rather than error.
+        var removeEnumPrefixMapping = nameMappings
+            .GroupBy(m => m.RemoveEnumPrefixName)
+            .ToDictionary(g => g.Key, g => g.First().OriginalName, StringComparer.Ordinal);
 
         return new EnumMapping { WriteMapping = writeMapping, RemoveEnumPrefixMapping = removeEnumPrefixMapping };
     }
@@ -146,6 +152,7 @@ private sealed class EnumMapping
 
     private sealed class NameMapping
     {
+        public required object Value { get; init; }
         public required string OriginalName { get; init; }
         public required string RemoveEnumPrefixName { get; init; }
     }