Consider not throwing exceptions when deserializing unknown enum values

[martincostello]

Describe the feature

There have been a number of times where drift between the schema definitions and/or this library and the actual runtime behaviour of github.com/GitHub Enterprise Server cause runtime failures in applications consuming the library to process webhooks. Examples include #52, #76 and #89.

To make consuming applications more resilient to natural evolution of the service over time, it would be preferable if enum deserialization is more tolerant to undefined values (this could be opt-in).

This is particularly impactful with github.com as the payloads could change at any time without advance notice to the owner of an integration due to a new deployment/feature on the server. In contrast, upgrades to GitHub Enterprise Server are more predictable and can be planned for in advance as the appliance is typically upgraded through a specific decision on the part of its operators.

[JamieMagee]

I'd split those issues into two categories:

1. GitHub has started sending a new event
   • fix: add waiting deployment status state #52
   • fix: Add merge_group app event #76
2. I've tried to tighten up the types and got it wrong
   • fix: Add App to HookType #89
   • feat: add PullRequestReviewState enum #91 Thanks @wolfy1339!

Do you know of a way to handle this more gracefully in System.Text.Json? I know in Newtonsoft.Json there the OnErrorAttribute¹. There's currently an open issue² for better error handling in System.Text.Json. Is the best thing to do just wrap deserialization in a try/catch?

Footnotes

1. https://www.newtonsoft.com/json/help/html/serializationerrorhandling.htm#OnErrorAttribute ↩

2. https://github.com/dotnet/runtime/issues/38049 ↩

[martincostello]

Having a dig around, it looks like the JsonStringEnumMemberConverter converter that's brought in from Macross.Json.Extensions has some support for this built in a few different ways.

You can specify a fallback value on each enum using [JsonStringEnumMemberConverterOptions].

[PublicAPI]
[JsonConverter(typeof(JsonStringEnumMemberConverter))]
[JsonStringEnumMemberConverterOptions(deserializationFailureFallbackValue: -1)]
public enum HookType
{
    [EnumMember(Value = "Repository")]
    Repository,
    [EnumMember(Value = "Organization")]
    Organization,
    [EnumMember(Value = "App")]
    App,
}

Another way is to create a derived JsonStringEnumMemberConverter that specifies the fallback in code .

[PublicAPI]
[JsonConverter(typeof(JsonStringEnumMemberConverterWithFallback))]
public enum HookType
{
    [EnumMember(Value = "Repository")]
    Repository,
    [EnumMember(Value = "Organization")]
    Organization,
    [EnumMember(Value = "App")]
    App,
}

public sealed class JsonStringEnumMemberConverterWithFallback : JsonStringEnumMemberConverter
{
    private static readonly JsonStringEnumMemberConverterOptions Options = new()
    {
        DeserializationFailureFallbackValue = -1,
    };

    public JsonStringEnumMemberConverterWithFallback()
        : base(Options)
    {
    }
}

Given that, one approach could be to use a custom implementation of it that uses -1 to signify an invalid value, then unknown values would only affect code that tried to use something that was affected. It would stop the serializer failing, but anything reading the specific webhook event property that received that invalid value would do whatever the user code does with it. The upside to that is the user's code doesn't use a given event property, the changes remain invisible to them. The downside is that the user's code might not handle an undefined value in a nice way if it's started to run once it's been deserialized.

Another approach entirely (but a big breaking change) could be to type enums like octokit.net does by wrapping them with the StringEnum<TEnum> struct. That would preserve the raw string values so the serializer wouldn't complain but would throw an exception when the value was read if parsing it fails. This approach would still break apps if they get new values in the payload if they access .Value, but an app would have the option of using .StringValue or TryParse() to make a conscious decision per usage on what to do if an unknown value is encountered.

Option 1 seems more light-touch, but isn't really customisable. Option 2 is more flexible for users, but would likely need code fix-ups to adopt in an existing application due to the binary-breaking changes.

[JamieMagee]

I think that option 1 would be good for a short-term fix. We can do that in a 1.x release. I agree that option 2 is the better long term solution. We can do that in a 2.x release. It might be a good place to release the Azure Functions package as well.

What do you think?

[martincostello]

That sounds reasonable to me.

I'll have a test of approach 1 and see how it handles things. If it looks viable I'll push up a draft PR for you to take a look at.

[JamieMagee]

I've started working on option 2 in the feat/string-enum branch