Support `[GeneralWebHook]` actions without a specified `WebHookBodyType`

[dougbu]

ASP.NET Core WebHooks includes a filter (WebHookVerifyBodyTypeFilter) that rejects requests with unexpected request content. For example, the Dropbox receiver can handle only JSON-formatted request bodies; WebHookVerifyBodyTypeFilter rejects anything else.

However the filter is too constrained for General and GitHub actions i.e. those actions with an associated [GeneralWebHook] or [GitHubWebHook] attribute. Those actions can choose a supported request body type using properties of those attributes. (General WebHook actions actually support only the subset of configured receivers matching their data parameters. GitHub is the only receiver that supports multiple body types.)

Working around this limitation quickly becomes cumbersome e.g.

[Consumes("application/json", "text/json")]
[GeneralWebHook(BodyType = WebHookBodyType.Json)]
public async Task<IActionResult> Execute(string receiverName, string id, string[] events, JObject data)
{
    // ...
    return Ok();
}

[Consumes("application/x-www-form-urlencoded", "multipart/form-data")]
[GeneralWebHook(BodyType = WebHookBodyType.Form)]
public async Task<IActionResult> Execute(string receiverName, string id, string[] events, IFormCollection data)
{
    // ...
    return Ok();
}

[Consumes("application/xml", "text/xml")]
[GeneralWebHook(BodyType = WebHookBodyType.Xml)]
public async Task<IActionResult> Execute(string receiverName, string id, string[] events, XElement data)
{
    // ...
    return Ok();
}

or, equivalently (if MailChimp is the only configured receiver accepting form data)

[GeneralWebHook] // WebHookBodyType.Json is the default BodyType
public async Task<IActionResult> Execute(string receiverName, string id, string[] events, JObject data)
{
    // ...
    return Ok();
}

[MailChimpWebHook]
public async Task<IActionResult> Execute(string receiverName, string id, string[] events, IFormCollection data)
{
    // ...
    return Ok();
}

[SalesforceWebHook]
public async Task<IActionResult> Execute(string receiverName, string id, string[] events, XElement data)
{
    // ...
    return Ok();
}

Suggest we make this less complicated for users.

[dougbu]

Have a number of options here. For example,

1. Stick with the current properties for [GeneralWebHook] and [GitHubWebHook] but add a routing constraint that is aware of these properties. This constraint would be added to the application model in the same way WebHookVerifyBodyTypeFilter is today. Can probably get rid of WebHookVerifyBodyTypeFilter since all 40x responses are the same to most senders.
2. Remove the AcceptFormData from [GitHubWebHook] i.e. support just JSON-formatted request bodies in a GitHub action. Then do (1) but only for [GeneralWebHook].
3. Make specifying a BodyType in [GeneralWebHook] optional i.e. change the property's type to WebHookBodyType?. Users can then leave out the data parameter and late-bind the data using the IWebHookRequestReader service I'll need to add when centralizing support for filters that read the request body.

A few non-suggestions:

1. Don't support object data parameters because that gets messy fast and forces users to dispatch on the runtime type of data.
2. Don't attempt to infer BodyType from the data parameter's type because that implies more fine-grained action selection than is sensible. That is, actions with AzureAlertNotification and PusherNotifications parameters should be associated with [AzureAlertWebHook] and [PusherWebHook] attributes. MyGet actions with BuildFinishedPayload and PackageAddedPayload parameters should be distinguished using event names, just as they would be if both actions had JObject parameters.
3. Don't add errors when BodyType and a data parameter's type appear to be mismatched. For one thing, parameter types aren't that rigid e.g. Newtonsoft.Json can bind XElement parameters from a JSON request body. For another, if the MVC model binding system's errors aren't already clear enough, that's an MVC bug.

[danroth27]

@rynowak Thoughts on this? I believe the motivating scenario here is handling web hooks in a generic fashion (like Azure Functions does).

[rynowak]

This is just [Consumes] right?

[dougbu]

One side of the issue (lack of action selection based on the already-specified content type) is just [Consumes]. But, note the [Consumes] appears redundant given the BodyType already in the [GeneralWebHook].

The other side of the issue is that it's not possible to define a WebHook action that can receive requests for any receiver. Specifying a BodyType is currently required and that controls a filter that blocks everything else.

[dougbu]

My thanks to @rynowak for a good discussion of the requirements here.

We decided not to change WebHooks action selection at all. The restriction that users may create unreachable actions if they mix [Consumes] with event name action selection is too extreme of a corner case to change the WebHooks framework. (At most, may suggest users avoid [Consumes] with WebHooks actions in documentation.)

1. Change WebHookVerifyBodyTypeFilter to use registered IWebHookBodyTypeMetadataService implementations and register it globally. Do nothing if metadata for request's receiver doesn't exist.
2. Change WebHookBodyType to be a [Flags] enum. This allows every receiver (even GitHub) to completely specify its content type requirements.
3. Change the type of the IWebHookBodyTypeMetadata.BodyType property to Nullable<WebHookBodyType>. This allows frameworks to use the [GeneralWebHook] attribute without choosing a specific body type. Action code can use IWebHookRequestReader after it knows the request content type.
   • Note IWebHookBodyTypeMetadataService doesn't really need to inherit from IWebHookBodyTypeMetadata. Can keep the concept count down just as well by using WebHookBodyType in both.

[dougbu]

Renamed issue because we decided to make changes here but not to support action selection based on the WebHookBodyType.

[dougbu]

@mkArtakMSFT cost takes into account work already done

[dougbu]

730adf3128