Fixing failing tests and address feedback

Author: captainsafia

src/Http/Http.Extensions/src/PublicAPI.Unshipped.txt

@@ -1,5 +1,5 @@
 #nullable enable
 Microsoft.Extensions.DependencyInjection.RouteHandlerJsonServiceExtensions
 static Microsoft.Extensions.DependencyInjection.RouteHandlerJsonServiceExtensions.ConfigureRouteHandlerJsonOptions(this Microsoft.Extensions.DependencyInjection.IServiceCollection! services, System.Action<Microsoft.AspNetCore.Http.Json.JsonOptions!>! configureOptions) -> Microsoft.Extensions.DependencyInjection.IServiceCollection!
-Microsoft.AspNetCore.Http.RequestDelegateFactoryOptions.RouteHandlerFilters.get -> System.Collections.Generic.IEnumerable<Microsoft.AspNetCore.Http.IRouteHandlerFilter!>?
+Microsoft.AspNetCore.Http.RequestDelegateFactoryOptions.RouteHandlerFilters.get -> System.Collections.Generic.IReadOnlyList<Microsoft.AspNetCore.Http.IRouteHandlerFilter!>?
 Microsoft.AspNetCore.Http.RequestDelegateFactoryOptions.RouteHandlerFilters.init -> void

src/Http/Http.Extensions/src/RequestDelegateFactory.cs

@@ -79,7 +79,7 @@ public static partial class RequestDelegateFactory
     private static readonly BinaryExpression TempSourceStringNullExpr = Expression.Equal(TempSourceStringExpr, Expression.Constant(null));
     private static readonly UnaryExpression TempSourceStringIsNotNullOrEmptyExpr = Expression.Not(Expression.Call(StringIsNullOrEmptyMethod, TempSourceStringExpr));
 
-    private static readonly ConstructorInfo RouteHandlerFilterContextConstructor = typeof(RouteHandlerFilterContext).GetConstructors().Single();
+    private static readonly ConstructorInfo RouteHandlerFilterContextConstructor = typeof(RouteHandlerFilterContext).GetConstructor(new[] { typeof(HttpContext), typeof(object[]) })!;
     private static readonly ParameterExpression FilterContextExpr = Expression.Parameter(typeof(RouteHandlerFilterContext), "context");
     private static readonly MemberExpression FilterContextParametersExpr = Expression.Property(FilterContextExpr, typeof(RouteHandlerFilterContext).GetProperty(nameof(RouteHandlerFilterContext.Parameters))!);
     private static readonly MemberExpression FilterContextHttpContextExpr = Expression.Property(FilterContextExpr, typeof(RouteHandlerFilterContext).GetProperty(nameof(RouteHandlerFilterContext.HttpContext))!);
@@ -224,6 +224,7 @@ private static FactoryContext CreateFactoryContext(RequestDelegateFactoryOptions
 
     private static Func<RouteHandlerFilterContext, ValueTask<object?>> CreateFilterPipeline(MethodInfo methodInfo, Expression? target, FactoryContext factoryContext)
     {
+        Debug.Assert(factoryContext.Filters is not null);
         // httpContext.Response.StatusCode == 400
         // ? Task.CompletedTask
         // : handler((string)context.Parameters[0], (int)context.Parameters[1])
@@ -240,7 +241,7 @@ target is null
                 )),
             FilterContextExpr).Compile();
 
-        for (int i = factoryContext.Filters!.Count - 1; i >= 0; i--)
+        for (var i = factoryContext.Filters.Count - 1; i >= 0; i--)
         {
             var currentFilter = factoryContext.Filters![i];
             var nextFilter = filteredInvocation;

src/Http/Http.Extensions/src/RequestDelegateFactoryOptions.cs

@@ -35,5 +35,5 @@ public sealed class RequestDelegateFactoryOptions
     /// <summary>
     /// The list of filters that must run in the pipeline for a given route handler.
     /// </summary>
-    public IEnumerable<IRouteHandlerFilter>? RouteHandlerFilters { get; init; }
+    public IReadOnlyList<IRouteHandlerFilter>? RouteHandlerFilters { get; init; }
 }

src/Http/Routing/src/Builder/EndpointRouteBuilderExtensions.cs

@@ -507,18 +507,6 @@ private static RouteHandlerBuilder Map(
             builder.DisplayName = $"{builder.DisplayName} => {endpointName}";
         }
 
-        // Add delegate attributes as metadata
-        var attributes = handler.Method.GetCustomAttributes();
-
-        // This can be null if the delegate is a dynamic method or compiled from an expression tree
-        if (attributes is not null)
-        {
-            foreach (var attribute in attributes)
-            {
-                builder.Metadata.Add(attribute);
-            }
-        }
-
         var dataSource = endpoints.DataSources.OfType<ModelEndpointDataSource>().FirstOrDefault();
         if (dataSource is null)
         {
@@ -538,11 +526,24 @@ private static RouteHandlerBuilder Map(
                 RouteHandlerFilters = routeHandlerBuilder.RouteHandlerFilters
             };
             var filteredRequestDelegateResult = RequestDelegateFactory.Create(handler, options);
-            // Add add request delegate metadata
+            // Add request delegate metadata
             foreach (var metadata in filteredRequestDelegateResult.EndpointMetadata)
             {
                 endpointBuilder.Metadata.Add(metadata);
             }
+
+            // We add attributes on the handler after those automatically generated by the
+            // RDF since they have a higher specificity.
+            var attributes = handler.Method.GetCustomAttributes();
+
+            // This can be null if the delegate is a dynamic method or compiled from an expression tree
+            if (attributes is not null)
+            {
+                foreach (var attribute in attributes)
+                {
+                    endpointBuilder.Metadata.Add(attribute);
+                }
+            }
             endpointBuilder.RequestDelegate = filteredRequestDelegateResult.RequestDelegate;
         });
 

src/Http/Routing/test/UnitTests/Builder/RouteHandlerEndpointRouteBuilderExtensionsTest.cs

@@ -208,7 +208,9 @@ public async Task MapGetWithoutRouteParameter_BuildsEndpointWithQuerySpecificBin
     public void MapGet_ThrowsWithImplicitFromBody()
     {
         var builder = new DefaultEndpointRouteBuilder(new ApplicationBuilder(new EmptyServiceProvider()));
-        var ex = Assert.Throws<InvalidOperationException>(() => builder.MapGet("/", (Todo todo) => { }));
+        _ = builder.MapGet("/", (Todo todo) => { });
+        var dataSource = GetBuilderEndpointDataSource(builder);
+        var ex = Assert.Throws<InvalidOperationException>(() => dataSource.Endpoints);
         Assert.Contains("Body was inferred but the method does not allow inferred body parameters.", ex.Message);
         Assert.Contains("Did you mean to register the \"Body (Inferred)\" parameter(s) as a Service or apply the [FromServices] or [FromBody] attribute?", ex.Message);
     }
@@ -217,7 +219,9 @@ public void MapGet_ThrowsWithImplicitFromBody()
     public void MapDelete_ThrowsWithImplicitFromBody()
     {
         var builder = new DefaultEndpointRouteBuilder(new ApplicationBuilder(new EmptyServiceProvider()));
-        var ex = Assert.Throws<InvalidOperationException>(() => builder.MapDelete("/", (Todo todo) => { }));
+        _ = builder.MapDelete("/", (Todo todo) => { });
+        var dataSource = GetBuilderEndpointDataSource(builder);
+        var ex = Assert.Throws<InvalidOperationException>(() => dataSource.Endpoints);
         Assert.Contains("Body was inferred but the method does not allow inferred body parameters.", ex.Message);
         Assert.Contains("Did you mean to register the \"Body (Inferred)\" parameter(s) as a Service or apply the [FromServices] or [FromBody] attribute?", ex.Message);
     }
@@ -243,7 +247,9 @@ public static object[][] NonImplicitFromBodyMethods
     public void MapVerb_ThrowsWithImplicitFromBody(string method)
     {
         var builder = new DefaultEndpointRouteBuilder(new ApplicationBuilder(new EmptyServiceProvider()));
-        var ex = Assert.Throws<InvalidOperationException>(() => builder.MapMethods("/", new[] { method }, (Todo todo) => { }));
+        _ = builder.MapMethods("/", new[] { method }, (Todo todo) => { });
+        var dataSource = GetBuilderEndpointDataSource(builder);
+        var ex = Assert.Throws<InvalidOperationException>(() => dataSource.Endpoints);
         Assert.Contains("Body was inferred but the method does not allow inferred body parameters.", ex.Message);
         Assert.Contains("Did you mean to register the \"Body (Inferred)\" parameter(s) as a Service or apply the [FromServices] or [FromBody] attribute?", ex.Message);
     }
@@ -581,7 +587,9 @@ public async Task MapVerbWithRouteParameterDoesNotFallbackToQuery(Func<IEndpoint
     public void MapGetWithRouteParameter_ThrowsIfRouteParameterDoesNotExist()
     {
         var builder = new DefaultEndpointRouteBuilder(new ApplicationBuilder(new EmptyServiceProvider()));
-        var ex = Assert.Throws<InvalidOperationException>(() => builder.MapGet("/", ([FromRoute] int id) => { }));
+        _ = builder.MapGet("/", ([FromRoute] int id) => { });
+        var dataSource = GetBuilderEndpointDataSource(builder);
+        var ex = Assert.Throws<InvalidOperationException>(() => dataSource.Endpoints);
         Assert.Equal("'id' is not a route parameter.", ex.Message);
     }
 
@@ -637,7 +645,9 @@ public async Task MapGetWithNamedFromRouteParameter_FailsForParameterName()
     public void MapGetWithNamedFromRouteParameter_ThrowsForMismatchedPattern()
     {
         var builder = new DefaultEndpointRouteBuilder(new ApplicationBuilder(new EmptyServiceProvider()));
-        var ex = Assert.Throws<InvalidOperationException>(() => builder.MapGet("/{id}", ([FromRoute(Name = "value")] int id, HttpContext httpContext) => { }));
+        _ = builder.MapGet("/{id}", ([FromRoute(Name = "value")] int id, HttpContext httpContext) => { });
+        var dataSource = GetBuilderEndpointDataSource(builder);
+        var ex = Assert.Throws<InvalidOperationException>(() => dataSource.Endpoints);
         Assert.Equal("'value' is not a route parameter.", ex.Message);
     }
 
@@ -677,7 +687,6 @@ public void MapPost_BuildsEndpointWithCorrectEndpointMetadata()
         Assert.False(endpointMetadata!.IsOptional);
         Assert.Equal(typeof(Todo), endpointMetadata.RequestType);
         Assert.Equal(new[] { "application/xml" }, endpointMetadata.ContentTypes);
-
     }
 
     [Fact]