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.

[ghost]

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.

[davidfowl]

So this is about flowing the context through, I see.

[halter73]

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.

I want to make a slight adjustment to the approved API. Because we reject non-null EndpointBuilder.RequestDelegates in RequestDelegateFactory.Create() it becomes more necessary to construct RouteEndpontBuilders with null request delegates.

This was already possible because the RouteEndpontBuilder never null checked the argument and the RouteEndpontBuilder.RequestDelegate property was already nullable. Marking the constructor parameter as nullable is just recognizing the current state of affairs. You could argue this is just a nullability fixup instead of a real API change.

I also want to use required init for EndpointFilterFactoryContext now that we have access to it. This seems to generally be the better choice for context types. Here's the updated API proposal:

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 EndopintFilterFactoryContext();

-    public MethodInfo MethodInfo { get; }
+    public required MethodInfo MethodInfo { get; init; }
+    public required EndpointBuilder EndpointBuilder { get; init; }
}

namespace Microsoft.AspNetCore.Routing;

public sealed class RouteEndpointBuilder : EndpointBuilder
{
-    public RouteEndpointBuilder(RequestDelegate requestDelegate, RoutePattern routePattern, int order);
+    public RouteEndpointBuilder(RequestDelegate? requestDelegate, RoutePattern routePattern, int order);
}

[ghost]

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.

[BrennanConroy]

I think you meant to include init in public required EndpointBuilder EndpointBuilder { get; }

[BrennanConroy]

👍 required init, the nullable part is a bit odd, since we aren't passing null in anywhere ourselves, and it needs to be non-null by the time you call build. But to your point, it does need to be null for RequestDelegateFactory (after your changes).

[halter73]

API Review Notes:

1. Do we need mutable metadata at all for factories?
   • Maybe. Maybe not.
2. If we need mutable metadata, does it make more sense to expose the entire EndpointBuilder instead of just the IList<object> for metadata?
   • EndpointBuilder.Metadata is more characters than EndpointMetadata
   • Assuming all someone want to do is observe or mutate metadata it's less properties to learn about.
   • EndpointBuilder is more self-documenting because it is the normal way to modify endpoint metadata.
   • EndpointBuilder is more flexible because it allows reading more properties like DisplayName and RoutePattern.
3. Is exposing the entire EndpointBuilder exposing a footgun?
   • You won't be able to add a filter for example. - Same goes for IEndpointConventionBuilder.Finally(Action<EndpointBuilder>) callbacks. No matter what, we need to prevent mutating this after route-specific conventions run.
   • What about other properties like RequestDelegate?
     • RDF requires that it's null to avoid any possible confusion
   • What about the RoutePattern?
     • That's one of the things that's nice to be able to read. And changing it would still work anyway. Again the same can be said about Finally callbacks.
4. What about exposing immutable EndpointMetadataCollection instead like we did previously?
   • This could break @DamianEdwards scenario that caused us to add it.
   • It wouldn't be the complete metadata anyway. The Finally callbacks have yet to run.
   • We're not aware of any other incomplete EndpointMetadataCollections. This could be very confusing.

I personally think that it makes no sense to expose mutable metadata but not the EndpointBuilder. As soon as you have the ability to modify the metadata you can modify the behavior of the endpoint and route matching in strange and unusual ways. It's already a footgun. We might as well make it more self-documenting and useful.

[halter73]

API Review Notes

• Is it reasonable for RDFOptions to have an EndpointBuilder property?
  • Yes. It's pretty low level. And EndpointBuilder allows things like the IEndpointMetadataProvider to see the RoutePattern even though RDF doesn't have a reference to routing itself.
• Should filters participate in endpoint construction?
  • Here's the original request from @DamianEdwards asking for mutating metadata is at https://github.com/dotnet/aspnetcore/issues/41722?
    • Do we have other scenarios? We don't think this scenario is super compelling.
    • You could have your IEnpointConventionBuilder extension method add both a filter and "convention" together.
  • The other scenario is read-only endpoint metadata that we might want to cache in the filter factory.
    • The thing I'm (@halter73) scared of is exposing something that's almost like Endpoint or RouteEndpoint but it's not final.
• What if we delete the first phase completely?
  • You'd have to grab the MethodInfo from HttpRequest.GetEndpoint()
  • If we don't make filters two-phase. A two-phase RDF that infers before the call to Create() can infer all metadata before create. Otherwise, factories could add metadata in Create().

API review decision. No more EndpointFilterFactoryContext.EndpointMetadata or EndpointFilterFactoryContext.EndpointBuilder for .NET 7. You can use conventions if you want to modify endpoint metadata.

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 EndopintFilterFactoryContext();

-    public MethodInfo MethodInfo { get; }
+    public required IServiceProvider MethodInfo { get; init; }

-    public IServiceProvider ApplicationServices { get; }
+    public IServiceProvider ApplicationServices { init; }

-    public IList<object> EndpointMetadata { get; }
}

namespace Microsoft.AspNetCore.Routing;

public sealed class RouteEndpointBuilder : EndpointBuilder
{
-    public RouteEndpointBuilder(RequestDelegate requestDelegate, RoutePattern routePattern, int order);
+    public RouteEndpointBuilder(RequestDelegate? requestDelegate, RoutePattern routePattern, int order);
}

[DamianEdwards]

@halter73 seems this one should go in rc.2 as well?