Expose entire EndpointBuilder to filter factories

[halter73]

Background and Motivation

In reaction to #42592, I think we should expose the entire EndopintBuilder via RequestDelegateFactoryOptions. This would allow downcasting to RouteEndpointBuilder and seeing the RoutePattern too from filters.

Proposed API

namespace Microsoft.AspNetCore.Http;

public sealed class RequestDelegateFactoryOptions
{
-    public IReadOnlyList<Func<EndpointFilterFactoryContext, EndpointFilterDelegate, EndpointFilterDelegate>>? EndpointFilterFactories { get; init; }
-    public IList<object>? EndpointMetadata { get; init; }
+    public EndpointBuilder? EndpointBuilder { get; init; }
}

namespace Microsoft.AspNetCore.Http;

public sealed class EndpointFilterFactoryContext
{
-    public EndpointFilterFactoryContext(MethodInfo methodInfo, IList<object> endpointMetadata, IServiceProvider applicationServices);
+    public EndpointFilterFactoryContext(MethodInfo methodInfo, EndpointBuilder endopintBuilder);

-    public IList<object> EndpointMetadata { get; }
-    public IServiceProvider ApplicationServices { get; }
+    public EndpointBuilder EndpointBuilder { get; }
}

Usage Examples

Before:

RequestDelegateFactoryOptions factoryOptions = new()
{
    ServiceProvider = _applicationServices,
    RouteParameterNames = routeParamNames,
    ThrowOnBadRequest = _throwOnBadRequest,
    DisableInferBodyFromParameters = ShouldDisableInferredBodyParameters(entry.HttpMethods),
    EndpointMetadata = builder.Metadata,
    // builder.EndopintFilterFactories is not settable so is always a List.
    EndpointFilterFactories = (IReadOnlyList<Func<EndpointFilterFactoryContext, EndpointFilterDelegate, EndpointFilterDelegate>>)builder.EndpointFilterFactories,
};

After:

RequestDelegateFactoryOptions factoryOptions = new()
{
    ServiceProvider = _applicationServices,
    RouteParameterNames = routeParamNames,
    ThrowOnBadRequest = _throwOnBadRequest,
    DisableInferBodyFromParameters = ShouldDisableInferredBodyParameters(entry.HttpMethods),
    EndpointBuilder = builder,
};

Alternative Designs

We could keep the filter factories directly on RequestDelegateFactoryOptions but rename EndpointFilterFactories to FilterFactories to match the name in EndpointBuilder and make it an IList instead of IReadOnlyList which aligns with RequestDelegateFactoryOptions.Metadata.

namespace Microsoft.AspNetCore.Http;

public sealed class RequestDelegateFactoryOptions
{
-    public IReadOnlyList<Func<EndpointFilterFactoryContext, EndpointFilterDelegate, EndpointFilterDelegate>>? EndpointFilterFactories { get; init; }
+    public IList<Func<EndpointFilterFactoryContext, EndpointFilterDelegate, EndpointFilterDelegate>>? FilterFactories { get; init; }
}

We could also keep it as an IReadOnlyList. Arguably this communicates better that the RequestDelegateFactory won't mutate it. RDF does mutate EndpointMetadata which is an IList so that might add to the confusion.

Risks

Low. This affects API that has only been released in previews and few people depend on.

Thank you for submitting this for API review. This will be reviewed by @dotnet/aspnet-api-review at the next meeting of the ASP.NET Core API Review group. Please ensure you take a look at the API review process documentation and ensure that:

• The PR contains changes to the reference-assembly that describe the API change. Or, you have included a snippet of reference-assembly-style code that illustrates the API change.
• The PR describes the impact to users, both positive (useful new APIs) and negative (breaking changes).
• Someone is assigned to "champion" this change in the meeting, and they understand the impact and design of the change.

[halter73]

API Review Notes:

• Exposing the entire EndpointBuilder to filters seems cleaner. It's less properties on RequestDelegateFactoryOptions and EndpointFilterFactoryContext and everything that is required on EndpointBuilderis required for filters.
• Will callers expect EndpointBuilder.RequestDelegate to be set upon completion?
  • Metadata is filled in upon completion, so why not the RequestDelegate?
  • We think we should set EndpointBuilder.RequestDelegate if RDF succeeds. At the very least if it is null going in.

API Approved!

namespace Microsoft.AspNetCore.Http;

public sealed class RequestDelegateFactoryOptions
{
-    public IReadOnlyList<Func<EndpointFilterFactoryContext, EndpointFilterDelegate, EndpointFilterDelegate>>? EndpointFilterFactories { get; init; }
-    public IList<object>? EndpointMetadata { get; init; }
+    public EndpointBuilder? EndpointBuilder { get; init; }
}

public sealed class EndpointFilterFactoryContext
{
-    public EndpointFilterFactoryContext(MethodInfo methodInfo, IList<object> endpointMetadata, IServiceProvider applicationServices);
+    public EndpointFilterFactoryContext(MethodInfo methodInfo, EndpointBuilder endopintBuilder);

-    public IList<object> EndpointMetadata { get; }
-    public IServiceProvider ApplicationServices { get; }
+    public EndpointBuilder EndpointBuilder { get; }
}

[davidfowl]

Isn't this a little weird? What does it mean if the EndpointBuilder has a RequestDelegate set?

[halter73]

I'm not sure it ever really makes sense to pass in an EndointBuilder with a RequestDelegate already set considering the implication of calling the factory is that it could at the very least be changed. Even in the RequestDelegate case, I plan to only pass that in via the Delegate to RequestDelegateFactory.Create() and not via RequestDelegateFactoryOptions.EndpointBuilder.RequestDelegate.

Since this is new API, we can just throw if it's not null. I don't see a problem with that. There's no real point to setting it that early.

[davidfowl]

I think the point is that the coupling here seems unnecessary. Why is the endpoint builder being passed into the request delegate factory?

[halter73]

Because it can include the display name, route pattern, order, and potentially different things in the future. Anything on the builder is of interest to filters. It doesn't make sense to copy all the properties of EndpointBuilder except the RequestDelegate (which is already nullable) and not give filters access to anything in the derived type.

Also, it's not required. You can still use RequestDelegateFactory just fine without it although you're giving filters less information than they might have otherwise had. Once filters started mutating the real EndpointBuilder.Metadata instance rather than working on a copy so that they could inspect the endpoints actual metadata so far, this was the natural conclusion.

I think EndpointMetadataContext and EndpointParameterMetadataContext should get the same treatment next.