fix: correctness fixes and performance improvements to custom json converters

Author: JamieMagee

src/Octokit.Webhooks/Converter/DateTimeOffsetConverter.cs

@@ -22,7 +22,13 @@ private static DateTimeOffset HandleString(Utf8JsonReader reader)
     {
         var stringValue = reader.GetString() ?? throw new InvalidOperationException("Cannot parse a String JsonToken with a null value");
 
-        return DateTimeOffset.Parse(stringValue, CultureInfo.InvariantCulture);
+        var span = stringValue.AsSpan();
+        if (!DateTimeOffset.TryParse(span, CultureInfo.InvariantCulture, DateTimeStyles.None, out var result))
+        {
+            throw new JsonException($"Unable to parse '{stringValue}' as DateTimeOffset");
+        }
+
+        return result;
     }
 
     private static DateTimeOffset HandleNumber(Utf8JsonReader reader) => DateTimeOffset.FromUnixTimeSeconds(reader.GetInt64());

src/Octokit.Webhooks/Converter/StringEnumConverter.cs

@@ -11,7 +11,12 @@ public sealed class StringEnumConverter<TEnum> : JsonConverter<StringEnum<TEnum>
     public override StringEnum<TEnum> Read(
         ref Utf8JsonReader reader,
         Type typeToConvert,
-        JsonSerializerOptions options) => new(reader.GetString()!);
+        JsonSerializerOptions options)
+    {
+        var stringValue = reader.GetString() ?? throw new JsonException("Unexpected null value when deserializing StringEnum.");
+
+        return new(stringValue);
+    }
 
     public override void Write(Utf8JsonWriter writer, StringEnum<TEnum> value, JsonSerializerOptions options) =>
         JsonSerializer.Serialize(writer, value.StringValue, options);

src/Octokit.Webhooks/Converter/StringEnumEnumerableConverter.cs

@@ -27,16 +27,16 @@ private static List<StringEnum<TEnum>> ReadInternal(ref Utf8JsonReader reader)
             throw new JsonException("Unexpected null value.");
         }
 
+        if (reader.TokenType != JsonTokenType.StartArray)
+        {
+            throw new JsonException("Expected JSON array.");
+        }
+
         var returnValue = new List<StringEnum<TEnum>>();
 
-        while (reader.TokenType != JsonTokenType.EndArray)
+        while (reader.Read() && reader.TokenType != JsonTokenType.EndArray)
         {
-            if (reader.TokenType != JsonTokenType.StartArray)
-            {
-                returnValue.Add(JsonSerializer.Deserialize<StringEnum<TEnum>>(ref reader, Options)!);
-            }
-
-            _ = reader.Read();
+            returnValue.Add(JsonSerializer.Deserialize<StringEnum<TEnum>>(ref reader, Options)!);
         }
 
         return returnValue;

src/Octokit.Webhooks/Converter/WebhookConverter.cs

@@ -1,56 +1,64 @@
 namespace Octokit.Webhooks.Converter;
 
+using System.Collections.Concurrent;
 using System.Linq;
 using System.Reflection;
 
 [PublicAPI]
 public class WebhookConverter<T> : JsonConverter<T>
     where T : WebhookEvent
 {
+    private static readonly ConcurrentDictionary<Type, Dictionary<string, Type>> TypeCache = new();
     private readonly Dictionary<string, Type> types;
 
     public WebhookConverter()
     {
         var type = typeof(T);
-        this.types = this.GetType().Assembly.GetTypes()
-            .Where(x => type.IsAssignableFrom(x) && x is { IsClass: true, IsAbstract: false } &&
-                        x.GetCustomAttribute<WebhookActionTypeAttribute>() is not null)
-            .ToDictionary(
-                y => y.GetCustomAttribute<WebhookActionTypeAttribute>()!.ActionType,
-                y => y);
+        this.types = TypeCache.GetOrAdd(type, static t =>
+            t.Assembly.GetTypes()
+                .Where(x => t.IsAssignableFrom(x) && x is { IsClass: true, IsAbstract: false } &&
+                            x.GetCustomAttribute<WebhookActionTypeAttribute>() is not null)
+                .ToDictionary(
+                    y => y.GetCustomAttribute<WebhookActionTypeAttribute>()!.ActionType,
+                    y => y));
     }
 
     public override T Read(ref Utf8JsonReader reader, Type typeToConvert, JsonSerializerOptions options)
     {
         if (reader.TokenType != JsonTokenType.StartObject)
         {
-            throw new JsonException();
+            throw new JsonException($"Expected JSON object but found {reader.TokenType}");
         }
 
         using var jsonDocument = JsonDocument.ParseValue(ref reader);
         if (!jsonDocument.RootElement.TryGetProperty("action", out var action))
         {
-            throw new JsonException();
+            throw new JsonException("Webhook payload is missing required 'action' property");
         }
 
         Type? type = null;
-
         var actionValue = action.GetString();
 
         if (actionValue is not null && this.types.TryGetValue(actionValue, out var payloadType))
         {
             type = payloadType;
         }
 
-        // repository_dispatch events can have custom user-defined actions values
+        // Special case: repository_dispatch events can have custom user-defined action values
         if (type == null && typeToConvert == typeof(Events.RepositoryDispatchEvent))
         {
+            if (string.IsNullOrEmpty(actionValue))
+            {
+                throw new JsonException("Repository dispatch event is missing action value");
+            }
+
             type = typeof(Events.RepositoryDispatch.RepositoryDispatchCustomEvent);
         }
 
         if (type is null)
         {
-            throw new JsonException();
+            var availableActions = string.Join(", ", this.types.Keys.OrderBy(k => k));
+            throw new JsonException($"Unknown action '{actionValue}' for webhook event type '{typeof(T).Name}'. Available actions: {availableActions}");
         }
 
         var jsonObject = jsonDocument.RootElement.GetRawText();

src/Octokit.Webhooks/Extensions/StringEnum.cs

@@ -1,6 +1,7 @@
 namespace Octokit.Webhooks.Extensions;
 
 using System;
+using System.Collections.Concurrent;
 using System.Globalization;
 using System.Linq;
 using System.Runtime.Serialization;
@@ -10,6 +11,8 @@ namespace Octokit.Webhooks.Extensions;
 public sealed record StringEnum<TEnum>
     where TEnum : struct
 {
+    private static readonly ConcurrentDictionary<TEnum, string> EnumToStringCache = new();
+    private static readonly ConcurrentDictionary<string, TEnum> StringToEnumCache = new();
     private TEnum? parsedValue;
 
     public StringEnum(string stringValue)
@@ -74,31 +77,44 @@ private TEnum ParseValue()
         throw GetArgumentException(this.StringValue);
     }
 
-    private static string ToEnumString(TEnum type)
-    {
-        var enumType = typeof(TEnum);
-        var name = Enum.GetName(enumType, type);
-        if (name is not null)
+    private static string ToEnumString(TEnum type) =>
+        EnumToStringCache.GetOrAdd(type, static enumValue =>
         {
-            var enumMemberAttribute = ((EnumMemberAttribute[])enumType.GetField(name)!.GetCustomAttributes(typeof(EnumMemberAttribute), true)).Single();
-            return enumMemberAttribute.Value!;
-        }
+            var enumType = typeof(TEnum);
+            var name = Enum.GetName(enumType, enumValue);
+            if (name is not null)
+            {
+                var field = enumType.GetField(name)!;
+                var enumMemberAttribute = field.GetCustomAttributes(typeof(EnumMemberAttribute), true)
+                    .Cast<EnumMemberAttribute>()
+                    .FirstOrDefault();
+
+                if (enumMemberAttribute?.Value is not null)
+                {
+                    return enumMemberAttribute.Value;
+                }
+            }
 
-        throw new ArgumentException(type.ToString());
-    }
+            throw new ArgumentException($"Enum value '{enumValue}' does not have a valid EnumMemberAttribute");
+        });
 
-    private static TEnum ToEnum(string str)
-    {
-        var enumType = typeof(TEnum);
-        foreach (var name in Enum.GetNames(enumType))
+    private static TEnum ToEnum(string str) =>
+        StringToEnumCache.GetOrAdd(str, static stringValue =>
         {
-            var enumMemberAttribute = ((EnumMemberAttribute[])enumType.GetField(name)!.GetCustomAttributes(typeof(EnumMemberAttribute), true)).Single();
-            if (enumMemberAttribute.Value == str)
+            var enumType = typeof(TEnum);
+            foreach (var name in Enum.GetNames(enumType))
             {
-                return (TEnum)Enum.Parse(enumType, name);
+                var field = enumType.GetField(name)!;
+                var enumMemberAttribute = field.GetCustomAttributes(typeof(EnumMemberAttribute), true)
+                    .Cast<EnumMemberAttribute>()
+                    .FirstOrDefault();
+
+                if (enumMemberAttribute?.Value == stringValue)
+                {
+                    return (TEnum)Enum.Parse(enumType, name);
+                }
             }
-        }
 
-        throw new ArgumentException(str);
-    }
+            throw new ArgumentException($"String value '{stringValue}' is not a valid '{enumType.Name}' enum value");
+        });
 }