From cbff9e89b459111ea104ec71c2d3bf9cee6c9e5f Mon Sep 17 00:00:00 2001 From: Stephen Halter Date: Tue, 19 Jul 2022 13:54:47 -0700 Subject: [PATCH 01/10] Remove RequestDelegateFactory call from RouteEndpointBuilder - Remove ModelEndpointDataSource. Use RouteEndpointDataSource instead --- .../Builder/EndpointRouteBuilderExtensions.cs | 81 ++++++----- .../src/Builder/RouteHandlerBuilder.cs | 2 +- .../Routing/src/ModelEndpointDataSource.cs | 38 ------ src/Http/Routing/src/RouteEndpointBuilder.cs | 31 +---- .../Routing/src/RouteEndpointDataSource.cs | 128 ++++++++++++------ ...RoutingApplicationBuilderExtensionsTest.cs | 2 + ...egateEndpointRouteBuilderExtensionsTest.cs | 12 +- .../UnitTests/RouteEndpointBuilderTest.cs | 38 ++++++ 8 files changed, 173 insertions(+), 159 deletions(-) delete mode 100644 src/Http/Routing/src/ModelEndpointDataSource.cs diff --git a/src/Http/Routing/src/Builder/EndpointRouteBuilderExtensions.cs b/src/Http/Routing/src/Builder/EndpointRouteBuilderExtensions.cs index bcbe36c63524..4ba58059c045 100644 --- a/src/Http/Routing/src/Builder/EndpointRouteBuilderExtensions.cs +++ b/src/Http/Routing/src/Builder/EndpointRouteBuilderExtensions.cs @@ -2,8 +2,6 @@ // The .NET Foundation licenses this file to you under the MIT license. using System.Diagnostics.CodeAnalysis; -using System.Linq; -using System.Reflection; using Microsoft.AspNetCore.Http; using Microsoft.AspNetCore.Routing; using Microsoft.AspNetCore.Routing.Patterns; @@ -152,10 +150,7 @@ public static IEndpointConventionBuilder MapMethods( { ArgumentNullException.ThrowIfNull(httpMethods); - var builder = endpoints.Map(RoutePatternFactory.Parse(pattern), requestDelegate); - builder.WithDisplayName($"{pattern} HTTP: {string.Join(", ", httpMethods)}"); - builder.WithMetadata(new HttpMethodMetadata(httpMethods)); - return builder; + return endpoints.Map(RoutePatternFactory.Parse(pattern), requestDelegate, httpMethods); } /// @@ -186,41 +181,21 @@ public static IEndpointConventionBuilder Map( this IEndpointRouteBuilder endpoints, RoutePattern pattern, RequestDelegate requestDelegate) + { + return Map(endpoints, pattern, requestDelegate, httpMethods: null); + } + + private static IEndpointConventionBuilder Map( + this IEndpointRouteBuilder endpoints, + RoutePattern pattern, + RequestDelegate requestDelegate, + IEnumerable? httpMethods) { ArgumentNullException.ThrowIfNull(endpoints); ArgumentNullException.ThrowIfNull(pattern); ArgumentNullException.ThrowIfNull(requestDelegate); - const int defaultOrder = 0; - - var builder = new RouteEndpointBuilder( - requestDelegate, - pattern, - defaultOrder) - { - DisplayName = pattern.RawText ?? pattern.DebuggerToString(), - }; - - // Add delegate attributes as metadata - var attributes = requestDelegate.Method.GetCustomAttributes(); - - // This can be null if the delegate is a dynamic method or compiled from an expression tree - if (attributes != null) - { - foreach (var attribute in attributes) - { - builder.Metadata.Add(attribute); - } - } - - var dataSource = endpoints.DataSources.OfType().FirstOrDefault(); - if (dataSource == null) - { - dataSource = new ModelEndpointDataSource(); - endpoints.DataSources.Add(dataSource); - } - - return dataSource.AddEndpointBuilder(builder); + return endpoints.GetOrAddRouteEndpointDataSource().AddRequestDelegate(pattern, requestDelegate, httpMethods); } /// @@ -429,18 +404,38 @@ private static RouteHandlerBuilder Map( ArgumentNullException.ThrowIfNull(pattern); ArgumentNullException.ThrowIfNull(handler); - var dataSource = endpoints.DataSources.OfType().FirstOrDefault(); - if (dataSource is null) + return endpoints.GetOrAddRouteEndpointDataSource().AddRouteHandler(pattern, handler, httpMethods, isFallback); + } + + private static RouteEndpointDataSource GetOrAddRouteEndpointDataSource(this IEndpointRouteBuilder endpoints) + { + RouteEndpointDataSource? routeEndpointDataSource = null; + + foreach (var dataSource in endpoints.DataSources) { - var routeHandlerOptions = endpoints.ServiceProvider.GetService>(); + if (dataSource is RouteEndpointDataSource foundDataSource) + { + routeEndpointDataSource = foundDataSource; + break; + } + } + + if (routeEndpointDataSource is null) + { + // ServiceProvider isn't nullable, but it is being called by methods that historically did not access this property, so we null check anyway. + var routeHandlerOptions = endpoints.ServiceProvider?.GetService>(); var throwOnBadRequest = routeHandlerOptions?.Value.ThrowOnBadRequest ?? false; - dataSource = new RouteEndpointDataSource(endpoints.ServiceProvider, throwOnBadRequest); - endpoints.DataSources.Add(dataSource); + routeEndpointDataSource = new RouteEndpointDataSource(endpoints.ServiceProvider ?? new EmptyServiceProvider(), throwOnBadRequest); + endpoints.DataSources.Add(routeEndpointDataSource); } - var conventions = dataSource.AddEndpoint(pattern, handler, httpMethods, isFallback); + return routeEndpointDataSource; + } - return new RouteHandlerBuilder(conventions); + private sealed class EmptyServiceProvider : IServiceProvider + { + public static EmptyServiceProvider Instance { get; } = new EmptyServiceProvider(); + public object? GetService(Type serviceType) => null; } } diff --git a/src/Http/Routing/src/Builder/RouteHandlerBuilder.cs b/src/Http/Routing/src/Builder/RouteHandlerBuilder.cs index 3910e4a2bd64..3000b1ae8a46 100644 --- a/src/Http/Routing/src/Builder/RouteHandlerBuilder.cs +++ b/src/Http/Routing/src/Builder/RouteHandlerBuilder.cs @@ -15,7 +15,7 @@ public sealed class RouteHandlerBuilder : IEndpointConventionBuilder /// /// Instantiates a new given a ThrowOnAddAfterEndpointBuiltConventionCollection from - /// . + /// . /// /// The convention list returned from . internal RouteHandlerBuilder(ICollection> conventions) diff --git a/src/Http/Routing/src/ModelEndpointDataSource.cs b/src/Http/Routing/src/ModelEndpointDataSource.cs deleted file mode 100644 index df36e6756bda..000000000000 --- a/src/Http/Routing/src/ModelEndpointDataSource.cs +++ /dev/null @@ -1,38 +0,0 @@ -// Licensed to the .NET Foundation under one or more agreements. -// The .NET Foundation licenses this file to you under the MIT license. - -using System.Linq; -using Microsoft.AspNetCore.Builder; -using Microsoft.AspNetCore.Http; -using Microsoft.Extensions.FileProviders; -using Microsoft.Extensions.Primitives; - -namespace Microsoft.AspNetCore.Routing; - -internal sealed class ModelEndpointDataSource : EndpointDataSource -{ - private readonly List _endpointConventionBuilders; - - public ModelEndpointDataSource() - { - _endpointConventionBuilders = new List(); - } - - public IEndpointConventionBuilder AddEndpointBuilder(EndpointBuilder endpointBuilder) - { - var builder = new DefaultEndpointConventionBuilder(endpointBuilder); - _endpointConventionBuilders.Add(builder); - - return builder; - } - - public override IChangeToken GetChangeToken() - { - return NullChangeToken.Singleton; - } - - public override IReadOnlyList Endpoints => _endpointConventionBuilders.Select(e => e.Build()).ToArray(); - - // for testing - internal IEnumerable EndpointBuilders => _endpointConventionBuilders.Select(b => b.EndpointBuilder); -} diff --git a/src/Http/Routing/src/RouteEndpointBuilder.cs b/src/Http/Routing/src/RouteEndpointBuilder.cs index b4c3e1e03468..2660c24bb261 100644 --- a/src/Http/Routing/src/RouteEndpointBuilder.cs +++ b/src/Http/Routing/src/RouteEndpointBuilder.cs @@ -1,7 +1,6 @@ // Licensed to the .NET Foundation under one or more agreements. // The .NET Foundation licenses this file to you under the MIT license. -using System.Diagnostics.CodeAnalysis; using Microsoft.AspNetCore.Builder; using Microsoft.AspNetCore.Http; using Microsoft.AspNetCore.Routing.Patterns; @@ -44,8 +43,6 @@ public RouteEndpointBuilder( } /// - [UnconditionalSuppressMessage("Trimmer", "IL2026", - Justification = "We surface a RequireUnreferencedCode in AddEndpointFilter which is required to call unreferenced code here. The trimmer is unable to infer this.")] public override Endpoint Build() { if (RequestDelegate is null) @@ -53,35 +50,11 @@ public override Endpoint Build() throw new InvalidOperationException($"{nameof(RequestDelegate)} must be specified to construct a {nameof(RouteEndpoint)}."); } - var requestDelegate = RequestDelegate; - - // Only replace the RequestDelegate if filters have been applied to this builder and they were not already handled by RouteEndpointDataSource. - // This affects other data sources like DefaultEndpointDataSource (this is people manually newing up a data source with a list of Endpoints), - // ModelEndpointDataSource (Map(RoutePattern, RequestDelegate) and by extension MapHub, MapHealthChecks, etc...), - // ActionEndpointDataSourceBase (MapControllers, MapRazorPages, etc...) and people with custom data sources or otherwise manually building endpoints - // using this type. At the moment this class is sealed, so at the moment we do not need to concern ourselves with what derived types may be doing. - if (EndpointFilterFactories is { Count: > 0 }) - { - // Even with filters applied, RDF.Create() will return back the exact same RequestDelegate instance we pass in if filters decide not to modify the - // invocation pipeline. We're just passing in a RequestDelegate so none of the fancy options pertaining to how the Delegate parameters are handled - // do not matter. - RequestDelegateFactoryOptions rdfOptions = new() - { - EndpointFilterFactories = EndpointFilterFactories, - EndpointMetadata = Metadata, - }; - - // We ignore the returned EndpointMetadata has been already populated since we passed in non-null EndpointMetadata. - requestDelegate = RequestDelegateFactory.Create(requestDelegate, rdfOptions).RequestDelegate; - } - - var routeEndpoint = new RouteEndpoint( - requestDelegate, + return new RouteEndpoint( + RequestDelegate, RoutePattern, Order, new EndpointMetadataCollection(Metadata), DisplayName); - - return routeEndpoint; } } diff --git a/src/Http/Routing/src/RouteEndpointDataSource.cs b/src/Http/Routing/src/RouteEndpointDataSource.cs index e15e2da25943..3afff038bc7d 100644 --- a/src/Http/Routing/src/RouteEndpointDataSource.cs +++ b/src/Http/Routing/src/RouteEndpointDataSource.cs @@ -25,24 +25,50 @@ public RouteEndpointDataSource(IServiceProvider applicationServices, bool throwO _throwOnBadRequest = throwOnBadRequest; } - public ICollection> AddEndpoint( + public RouteHandlerBuilder AddRequestDelegate( + RoutePattern pattern, + RequestDelegate requestDelegate, + IEnumerable? httpMethods) + { + + var conventions = new ThrowOnAddAfterEndpointBuiltConventionCollection(); + + _routeEntries.Add(new() + { + RoutePattern = pattern, + RouteHandler = requestDelegate, + HttpMethods = httpMethods, + RouteAttributes = RouteAttributes.None, + Conventions = conventions, + }); + + return new RouteHandlerBuilder(conventions); + } + + public RouteHandlerBuilder AddRouteHandler( RoutePattern pattern, Delegate routeHandler, IEnumerable? httpMethods, bool isFallback) { - RouteEntry entry = new() + var conventions = new ThrowOnAddAfterEndpointBuiltConventionCollection(); + + var routeAttributes = RouteAttributes.RouteHandler; + if (isFallback) + { + routeAttributes |= RouteAttributes.Fallback; + } + + _routeEntries.Add(new() { RoutePattern = pattern, RouteHandler = routeHandler, HttpMethods = httpMethods, - IsFallback = isFallback, - Conventions = new ThrowOnAddAfterEndpointBuiltConventionCollection(), - }; - - _routeEntries.Add(entry); + RouteAttributes = routeAttributes, + Conventions = conventions, + }); - return entry.Conventions; + return new RouteHandlerBuilder(conventions); } public override IReadOnlyList Endpoints @@ -88,6 +114,12 @@ private RouteEndpointBuilder CreateRouteEndpointBuilder( { var pattern = RoutePatternFactory.Combine(groupPrefix, entry.RoutePattern); var handler = entry.RouteHandler; + var isRouteHandler = (entry.RouteAttributes & RouteAttributes.RouteHandler) == RouteAttributes.RouteHandler; + var isFallback = (entry.RouteAttributes & RouteAttributes.Fallback) == RouteAttributes.Fallback; + + // The Map methods don't support customizing the order apart from using int.MaxValue to give MapFallback the lowest priority. + // Otherwise, we always use the default of 0 unless a convention changes it later. + var order = isFallback ? int.MaxValue : 0; var displayName = pattern.RawText ?? pattern.DebuggerToString(); // Methods defined in a top-level program are generated as statics so the delegate target will be null. @@ -105,13 +137,13 @@ private RouteEndpointBuilder CreateRouteEndpointBuilder( displayName = $"HTTP: {string.Join(", ", entry.HttpMethods)} {displayName}"; } - if (entry.IsFallback) + if (isFallback) { displayName = $"Fallback {displayName}"; } RequestDelegate? factoryCreatedRequestDelegate = null; - RequestDelegate redirectedRequestDelegate = context => + RequestDelegate redirectRequestDelegate = context => { if (factoryCreatedRequestDelegate is null) { @@ -121,20 +153,25 @@ private RouteEndpointBuilder CreateRouteEndpointBuilder( return factoryCreatedRequestDelegate(context); }; - // The Map methods don't support customizing the order apart from using int.MaxValue to give MapFallback the lowest priority. - // Otherwise, we always use the default of 0 unless a convention changes it later. - var order = entry.IsFallback ? int.MaxValue : 0; - - RouteEndpointBuilder builder = new(redirectedRequestDelegate, pattern, order) + RouteEndpointBuilder builder = new(redirectRequestDelegate, pattern, order) { DisplayName = displayName, ApplicationServices = _applicationServices, }; - // Add MethodInfo and HttpMethodMetadata (if any) as first metadata items as they are intrinsic to the route much like - // the pattern or default display name. This gives visibility to conventions like WithOpenApi() to intrinsic route details - // (namely the MethodInfo) even when applied early as group conventions. - builder.Metadata.Add(handler.Method); + if (isRouteHandler) + { + // Add MethodInfo and HttpMethodMetadata (if any) as first metadata items as they are intrinsic to the route much like + // the pattern or default display name. This gives visibility to conventions like WithOpenApi() to intrinsic route details + // (namely the MethodInfo) even when applied early as group conventions. + builder.Metadata.Add(handler.Method); + } + else + { + // If we're not a route handler, we started with a fully realized (although unfiltered) RequestDelegate, so we can just redirect to that + // while running any conventions. We'll put the original back if it remains unfiltered right before building the endpoint. + factoryCreatedRequestDelegate = (RequestDelegate)entry.RouteHandler; + } if (entry.HttpMethods is not null) { @@ -166,29 +203,29 @@ private RouteEndpointBuilder CreateRouteEndpointBuilder( entrySpecificConvention(builder); } - var routeParamNames = new List(pattern.Parameters.Count); - foreach (var parameter in pattern.Parameters) - { - routeParamNames.Add(parameter.Name); - } - - RequestDelegateFactoryOptions factoryOptions = new() + if (isRouteHandler || builder.EndpointFilterFactories is { Count: > 0}) { - ServiceProvider = _applicationServices, - RouteParameterNames = routeParamNames, - ThrowOnBadRequest = _throwOnBadRequest, - DisableInferBodyFromParameters = ShouldDisableInferredBodyParameters(entry.HttpMethods), - EndpointMetadata = builder.Metadata, - EndpointFilterFactories = builder.EndpointFilterFactories, - }; - - // We ignore the returned EndpointMetadata has been already populated since we passed in non-null EndpointMetadata. - factoryCreatedRequestDelegate = RequestDelegateFactory.Create(entry.RouteHandler, factoryOptions).RequestDelegate; + var routeParamNames = new List(pattern.Parameters.Count); + foreach (var parameter in pattern.Parameters) + { + routeParamNames.Add(parameter.Name); + } - // Clear out any filters so they don't get rerun in Build(). We can rethink how we do this later when exposed as public API. - builder.EndpointFilterFactories = null; + RequestDelegateFactoryOptions factoryOptions = new() + { + ServiceProvider = _applicationServices, + RouteParameterNames = routeParamNames, + ThrowOnBadRequest = _throwOnBadRequest, + DisableInferBodyFromParameters = ShouldDisableInferredBodyParameters(entry.HttpMethods), + EndpointMetadata = builder.Metadata, + EndpointFilterFactories = builder.EndpointFilterFactories, + }; + + // We ignore the returned EndpointMetadata has been already populated since we passed in non-null EndpointMetadata. + factoryCreatedRequestDelegate = RequestDelegateFactory.Create(entry.RouteHandler, factoryOptions).RequestDelegate; + } - if (ReferenceEquals(builder.RequestDelegate, redirectedRequestDelegate)) + if (ReferenceEquals(builder.RequestDelegate, redirectRequestDelegate)) { // No convention has changed builder.RequestDelegate, so we can just replace it with the final version as an optimization. // We still set factoryRequestDelegate in case something is still referencing the redirected version of the RequestDelegate. @@ -233,10 +270,21 @@ private struct RouteEntry public RoutePattern RoutePattern { get; init; } public Delegate RouteHandler { get; init; } public IEnumerable? HttpMethods { get; init; } - public bool IsFallback { get; init; } + public RouteAttributes RouteAttributes { get; init; } public ThrowOnAddAfterEndpointBuiltConventionCollection Conventions { get; init; } } + [Flags] + private enum RouteAttributes + { + // The endpoint was defined by a RequestDelegate, RequestDelegateFactory.Create() should be skipped unless there are endpoint filters. + None = 0, + // This was added as Delegate route handler, so RequestDelegateFactory.Create() should always be called. + RouteHandler = 1, + // This was added by MapFallback. + Fallback = 2, + } + // This private class is only exposed to internal code via ICollection> in RouteEndpointBuilder where only Add is called. private sealed class ThrowOnAddAfterEndpointBuiltConventionCollection : List>, ICollection> { diff --git a/src/Http/Routing/test/UnitTests/Builder/EndpointRoutingApplicationBuilderExtensionsTest.cs b/src/Http/Routing/test/UnitTests/Builder/EndpointRoutingApplicationBuilderExtensionsTest.cs index 9caac8465e86..bd352464d104 100644 --- a/src/Http/Routing/test/UnitTests/Builder/EndpointRoutingApplicationBuilderExtensionsTest.cs +++ b/src/Http/Routing/test/UnitTests/Builder/EndpointRoutingApplicationBuilderExtensionsTest.cs @@ -9,6 +9,7 @@ using Microsoft.AspNetCore.Routing.Patterns; using Microsoft.AspNetCore.Routing.TestObjects; using Microsoft.Extensions.DependencyInjection; +using Microsoft.Extensions.Hosting; using Microsoft.Extensions.Options; using Moq; @@ -359,6 +360,7 @@ private IServiceProvider CreateServices(MatcherFactory matcherFactory) var listener = new DiagnosticListener("Microsoft.AspNetCore"); services.AddSingleton(listener); services.AddSingleton(listener); + services.AddSingleton(Mock.Of()); var serviceProvder = services.BuildServiceProvider(); diff --git a/src/Http/Routing/test/UnitTests/Builder/RequestDelegateEndpointRouteBuilderExtensionsTest.cs b/src/Http/Routing/test/UnitTests/Builder/RequestDelegateEndpointRouteBuilderExtensionsTest.cs index f7ab3518642b..00187ec4d5d6 100644 --- a/src/Http/Routing/test/UnitTests/Builder/RequestDelegateEndpointRouteBuilderExtensionsTest.cs +++ b/src/Http/Routing/test/UnitTests/Builder/RequestDelegateEndpointRouteBuilderExtensionsTest.cs @@ -3,18 +3,13 @@ #nullable enable -using System.IO.Pipelines; using System.Linq.Expressions; using System.Reflection; using System.Text; using Microsoft.AspNetCore.Http; -using Microsoft.AspNetCore.Http.Features; using Microsoft.AspNetCore.Http.Metadata; using Microsoft.AspNetCore.Routing; using Microsoft.AspNetCore.Routing.Patterns; -using Microsoft.Extensions.DependencyInjection; -using Microsoft.Extensions.Logging; -using Moq; namespace Microsoft.AspNetCore.Builder; @@ -27,7 +22,6 @@ private RouteEndpointBuilder GetRouteEndpointBuilder(IEndpointRouteBuilder endpo GetBuilderEndpointDataSource(endpointRouteBuilder) switch { RouteEndpointDataSource routeDataSource => routeDataSource.GetSingleRouteEndpointBuilder(), - ModelEndpointDataSource modelDataSource => Assert.IsType(Assert.Single(modelDataSource.EndpointBuilders)), _ => throw new InvalidOperationException($"Unknown EndointDataSource type!"), }; @@ -233,9 +227,11 @@ public void MapEndpoint_PrecedenceOfMetadata_BuilderMetadataReturned() var dataSource = Assert.Single(builder.DataSources); var endpoint = Assert.Single(dataSource.Endpoints); + // As with the Delegate Map method overloads for route handlers, the attributes on the RequestDelegate + // can override the HttpMethodMetadata. Extension methods could already do this. Assert.Equal(3, endpoint.Metadata.Count); - Assert.Equal("ATTRIBUTE", GetMethod(endpoint.Metadata[0])); - Assert.Equal("METHOD", GetMethod(endpoint.Metadata[1])); + Assert.Equal("METHOD", GetMethod(endpoint.Metadata[0])); + Assert.Equal("ATTRIBUTE", GetMethod(endpoint.Metadata[1])); Assert.Equal("BUILDER", GetMethod(endpoint.Metadata[2])); Assert.Equal("BUILDER", endpoint.Metadata.GetMetadata()?.HttpMethods.Single()); diff --git a/src/Http/Routing/test/UnitTests/RouteEndpointBuilderTest.cs b/src/Http/Routing/test/UnitTests/RouteEndpointBuilderTest.cs index ed6f408337cb..914ec657176a 100644 --- a/src/Http/Routing/test/UnitTests/RouteEndpointBuilderTest.cs +++ b/src/Http/Routing/test/UnitTests/RouteEndpointBuilderTest.cs @@ -28,4 +28,42 @@ public void Build_AllValuesSet_EndpointCreated() Assert.Equal("/", endpoint.RoutePattern.RawText); Assert.Equal(metadata, Assert.Single(endpoint.Metadata)); } + + [Fact] + public async void Build_DoesNot_RunFilters() + { + + var endpointFilterCallCount = 0; + var invocationFilterCallCount = 0; + var invocationCallCount = 0; + + const int defaultOrder = 0; + RequestDelegate requestDelegate = (d) => + { + invocationCallCount++; + return Task.CompletedTask; + }; + + var builder = new RouteEndpointBuilder(requestDelegate, RoutePatternFactory.Parse("/"), defaultOrder); + + builder.EndpointFilterFactories = new List>(); + builder.EndpointFilterFactories.Add((endopintContext, next) => + { + endpointFilterCallCount++; + + return invocationContext => + { + invocationFilterCallCount++; + + return next(invocationContext); + }; + }); + + var endpoint = Assert.IsType(builder.Build()); + + await endpoint.RequestDelegate(new DefaultHttpContext()); + + Assert.Equal(0, endpointFilterCallCount); + Assert.Equal(0, invocationFilterCallCount); + } } From 6594c905fe502ee6dd0b36db30a64ba998686166 Mon Sep 17 00:00:00 2001 From: Stephen Halter Date: Fri, 22 Jul 2022 14:37:28 -0700 Subject: [PATCH 02/10] Apply suggestions from code review Co-authored-by: Kahbazi Co-authored-by: David Fowler --- src/Http/Routing/src/Builder/EndpointRouteBuilderExtensions.cs | 2 +- src/Http/Routing/test/UnitTests/RouteEndpointBuilderTest.cs | 1 - 2 files changed, 1 insertion(+), 2 deletions(-) diff --git a/src/Http/Routing/src/Builder/EndpointRouteBuilderExtensions.cs b/src/Http/Routing/src/Builder/EndpointRouteBuilderExtensions.cs index 4ba58059c045..22e82eb81590 100644 --- a/src/Http/Routing/src/Builder/EndpointRouteBuilderExtensions.cs +++ b/src/Http/Routing/src/Builder/EndpointRouteBuilderExtensions.cs @@ -426,7 +426,7 @@ private static RouteEndpointDataSource GetOrAddRouteEndpointDataSource(this IEnd var routeHandlerOptions = endpoints.ServiceProvider?.GetService>(); var throwOnBadRequest = routeHandlerOptions?.Value.ThrowOnBadRequest ?? false; - routeEndpointDataSource = new RouteEndpointDataSource(endpoints.ServiceProvider ?? new EmptyServiceProvider(), throwOnBadRequest); + routeEndpointDataSource = new RouteEndpointDataSource(endpoints.ServiceProvider ?? EmptyServiceProvider.Instance, throwOnBadRequest); endpoints.DataSources.Add(routeEndpointDataSource); } diff --git a/src/Http/Routing/test/UnitTests/RouteEndpointBuilderTest.cs b/src/Http/Routing/test/UnitTests/RouteEndpointBuilderTest.cs index 914ec657176a..6ca5655e1053 100644 --- a/src/Http/Routing/test/UnitTests/RouteEndpointBuilderTest.cs +++ b/src/Http/Routing/test/UnitTests/RouteEndpointBuilderTest.cs @@ -32,7 +32,6 @@ public void Build_AllValuesSet_EndpointCreated() [Fact] public async void Build_DoesNot_RunFilters() { - var endpointFilterCallCount = 0; var invocationFilterCallCount = 0; var invocationCallCount = 0; From bb2b157c3b6db7c33543b6f8622ce9a5ef08f8e8 Mon Sep 17 00:00:00 2001 From: Stephen Halter Date: Fri, 29 Jul 2022 16:10:17 -0700 Subject: [PATCH 03/10] Renames - Also add MethodInfo to all endpoint metadata in RouteEndpointDataSource --- .../src/PublicAPI.Unshipped.txt | 4 +- .../src/RequestDelegateFactory.cs | 2 +- .../src/RequestDelegateFactoryOptions.cs | 2 +- .../test/RequestDelegateFactoryTests.cs | 38 +++++++++---------- .../src/Builder/EndpointFilterExtensions.cs | 3 +- src/Http/Routing/src/PublicAPI.Unshipped.txt | 1 + src/Http/Routing/src/RouteEndpointBuilder.cs | 10 +++-- .../Routing/src/RouteEndpointDataSource.cs | 28 ++++++-------- ...egateEndpointRouteBuilderExtensionsTest.cs | 16 ++++---- .../UnitTests/RouteEndpointBuilderTest.cs | 3 +- 10 files changed, 52 insertions(+), 55 deletions(-) diff --git a/src/Http/Http.Extensions/src/PublicAPI.Unshipped.txt b/src/Http/Http.Extensions/src/PublicAPI.Unshipped.txt index 4b8eccfa6afb..a50b6a15b6b0 100644 --- a/src/Http/Http.Extensions/src/PublicAPI.Unshipped.txt +++ b/src/Http/Http.Extensions/src/PublicAPI.Unshipped.txt @@ -38,6 +38,8 @@ Microsoft.AspNetCore.Http.ProblemDetailsOptions.ProblemDetailsOptions() -> void *REMOVED*Microsoft.AspNetCore.Mvc.ProblemDetails.Title.set -> void *REMOVED*Microsoft.AspNetCore.Mvc.ProblemDetails.Type.get -> string? *REMOVED*Microsoft.AspNetCore.Mvc.ProblemDetails.Type.set -> void +Microsoft.AspNetCore.Http.RequestDelegateFactoryOptions.RouteHandlerFilterFactories.get -> System.Collections.Generic.IList!>? +Microsoft.AspNetCore.Http.RequestDelegateFactoryOptions.RouteHandlerFilterFactories.init -> void Microsoft.AspNetCore.Mvc.ProblemDetails (forwarded, contained in Microsoft.AspNetCore.Http.Abstractions) Microsoft.AspNetCore.Mvc.ProblemDetails.Detail.get -> string? (forwarded, contained in Microsoft.AspNetCore.Http.Abstractions) Microsoft.AspNetCore.Mvc.ProblemDetails.Detail.set -> void (forwarded, contained in Microsoft.AspNetCore.Http.Abstractions) @@ -52,8 +54,6 @@ Microsoft.AspNetCore.Mvc.ProblemDetails.Title.set -> void (forwarded, contained Microsoft.AspNetCore.Mvc.ProblemDetails.Type.get -> string? (forwarded, contained in Microsoft.AspNetCore.Http.Abstractions) Microsoft.AspNetCore.Mvc.ProblemDetails.Type.set -> void (forwarded, contained in Microsoft.AspNetCore.Http.Abstractions) Microsoft.Extensions.DependencyInjection.ProblemDetailsServiceCollectionExtensions -Microsoft.AspNetCore.Http.RequestDelegateFactoryOptions.EndpointFilterFactories.get -> System.Collections.Generic.IReadOnlyList!>? -Microsoft.AspNetCore.Http.RequestDelegateFactoryOptions.EndpointFilterFactories.init -> void Microsoft.AspNetCore.Http.RequestDelegateFactoryOptions.EndpointMetadata.get -> System.Collections.Generic.IList? Microsoft.AspNetCore.Http.RequestDelegateFactoryOptions.EndpointMetadata.init -> void Microsoft.Extensions.DependencyInjection.HttpJsonServiceExtensions diff --git a/src/Http/Http.Extensions/src/RequestDelegateFactory.cs b/src/Http/Http.Extensions/src/RequestDelegateFactory.cs index d27ca2442cc0..624d430f0736 100644 --- a/src/Http/Http.Extensions/src/RequestDelegateFactory.cs +++ b/src/Http/Http.Extensions/src/RequestDelegateFactory.cs @@ -208,7 +208,7 @@ private static FactoryContext CreateFactoryContext(RequestDelegateFactoryOptions RouteParameters = options?.RouteParameterNames?.ToList(), ThrowOnBadRequest = options?.ThrowOnBadRequest ?? false, DisableInferredFromBody = options?.DisableInferBodyFromParameters ?? false, - FilterFactories = options?.EndpointFilterFactories?.ToList(), + FilterFactories = options?.RouteHandlerFilterFactories?.ToList(), Metadata = options?.EndpointMetadata ?? new List(), }; } diff --git a/src/Http/Http.Extensions/src/RequestDelegateFactoryOptions.cs b/src/Http/Http.Extensions/src/RequestDelegateFactoryOptions.cs index a3f05d9db8a3..c2f6b6e97442 100644 --- a/src/Http/Http.Extensions/src/RequestDelegateFactoryOptions.cs +++ b/src/Http/Http.Extensions/src/RequestDelegateFactoryOptions.cs @@ -36,7 +36,7 @@ public sealed class RequestDelegateFactoryOptions /// /// The list of filters that must run in the pipeline for a given route handler. /// - public IReadOnlyList>? EndpointFilterFactories { get; init; } + public IList>? RouteHandlerFilterFactories { get; init; } /// /// The mutable initial endpoint metadata to add as part of the creation of the . In most cases, diff --git a/src/Http/Http.Extensions/test/RequestDelegateFactoryTests.cs b/src/Http/Http.Extensions/test/RequestDelegateFactoryTests.cs index 8b877f6a6cd2..9556b15f493f 100644 --- a/src/Http/Http.Extensions/test/RequestDelegateFactoryTests.cs +++ b/src/Http/Http.Extensions/test/RequestDelegateFactoryTests.cs @@ -4889,7 +4889,7 @@ string HelloName(string name) // Act var factoryResult = RequestDelegateFactory.Create(HelloName, new RequestDelegateFactoryOptions() { - EndpointFilterFactories = new List>() + RouteHandlerFilterFactories = new List>() { (routeHandlerContext, next) => async (context) => { @@ -4921,7 +4921,7 @@ public async Task RequestDelegateFactory_InvokesFilters_OnDelegateWithTarget() // Act var factoryResult = RequestDelegateFactory.Create((string name) => $"Hello, {name}!", new RequestDelegateFactoryOptions() { - EndpointFilterFactories = new List>() + RouteHandlerFilterFactories = new List>() { (routeHandlerContext, next) => async (context) => { @@ -4963,7 +4963,7 @@ public async Task RequestDelegateFactory_InvokesFilters_OnMethodInfoWithNullTarg // Act var factoryResult = RequestDelegateFactory.Create(methodInfo!, null, new RequestDelegateFactoryOptions() { - EndpointFilterFactories = new List>() + RouteHandlerFilterFactories = new List>() { (routeHandlerContext, next) => async (context) => { @@ -5006,7 +5006,7 @@ public async Task RequestDelegateFactory_InvokesFilters_OnMethodInfoWithProvided }; var factoryResult = RequestDelegateFactory.Create(methodInfo!, targetFactory, new RequestDelegateFactoryOptions() { - EndpointFilterFactories = new List>() + RouteHandlerFilterFactories = new List>() { (routeHandlerContext, next) => async (context) => { @@ -5043,7 +5043,7 @@ string HelloName(string name) // Act var factoryResult = RequestDelegateFactory.Create(HelloName, new RequestDelegateFactoryOptions() { - EndpointFilterFactories = new List>() { + RouteHandlerFilterFactories = new List>() { (routeHandlerContext, next) => async (context) => { if (context.HttpContext.Response.StatusCode == 400) @@ -5089,7 +5089,7 @@ string HelloName(string name, int age) // Act var factoryResult = RequestDelegateFactory.Create(HelloName, new RequestDelegateFactoryOptions() { - EndpointFilterFactories = new List>() + RouteHandlerFilterFactories = new List>() { (routeHandlerContext, next) => async (context) => { @@ -5137,7 +5137,7 @@ string HelloName(string name) // Act var factoryResult = RequestDelegateFactory.Create(HelloName, new RequestDelegateFactoryOptions() { - EndpointFilterFactories = new List>() + RouteHandlerFilterFactories = new List>() { (routeHandlerContext, next) => { @@ -5192,7 +5192,7 @@ string HelloName(IFormFileCollection formFiles) // Act var factoryResult = RequestDelegateFactory.Create(HelloName, new RequestDelegateFactoryOptions() { - EndpointFilterFactories = new List>() + RouteHandlerFilterFactories = new List>() { (routeHandlerContext, next) => { @@ -5243,7 +5243,7 @@ string PrintTodo(Todo todo) // Act var factoryResult = RequestDelegateFactory.Create(PrintTodo, new RequestDelegateFactoryOptions() { - EndpointFilterFactories = new List>() + RouteHandlerFilterFactories = new List>() { (routeHandlerContext, next) => async (context) => { @@ -5284,7 +5284,7 @@ string HelloName(string name) // Act var factoryResult = RequestDelegateFactory.Create(HelloName, new RequestDelegateFactoryOptions() { - EndpointFilterFactories = new List>() + RouteHandlerFilterFactories = new List>() { (routeHandlerContext, next) => async (context) => { @@ -5327,7 +5327,7 @@ string HelloName(string name) // Act var factoryResult = RequestDelegateFactory.Create(HelloName, new RequestDelegateFactoryOptions() { - EndpointFilterFactories = new List>() + RouteHandlerFilterFactories = new List>() { (routeHandlerContext, next) => async (context) => { @@ -5396,7 +5396,7 @@ public async Task CanInvokeFilter_OnTaskOfTReturningHandler(Delegate @delegate) // Act var factoryResult = RequestDelegateFactory.Create(@delegate, new RequestDelegateFactoryOptions() { - EndpointFilterFactories = new List>() + RouteHandlerFilterFactories = new List>() { (routeHandlerContext, next) => async (context) => { @@ -5454,7 +5454,7 @@ public async Task CanInvokeFilter_OnValueTaskOfTReturningHandler(Delegate @deleg // Act var factoryResult = RequestDelegateFactory.Create(@delegate, new RequestDelegateFactoryOptions() { - EndpointFilterFactories = new List>() + RouteHandlerFilterFactories = new List>() { (routeHandlerContext, next) => async (context) => { @@ -5519,7 +5519,7 @@ public async Task CanInvokeFilter_OnVoidReturningHandler(Delegate @delegate) // Act var factoryResult = RequestDelegateFactory.Create(@delegate, new RequestDelegateFactoryOptions() { - EndpointFilterFactories = new List>() + RouteHandlerFilterFactories = new List>() { (routeHandlerContext, next) => async (context) => { @@ -5553,7 +5553,7 @@ async Task HandlerWithTaskAwait(HttpContext c) // Act var factoryResult = RequestDelegateFactory.Create(HandlerWithTaskAwait, new RequestDelegateFactoryOptions() { - EndpointFilterFactories = new List>() + RouteHandlerFilterFactories = new List>() { (routeHandlerContext, next) => async (context) => { @@ -5620,7 +5620,7 @@ public async Task CanInvokeFilter_OnHandlerReturningTasksOfStruct(Delegate @dele // Act var factoryResult = RequestDelegateFactory.Create(@delegate, new RequestDelegateFactoryOptions() { - EndpointFilterFactories = new List>() + RouteHandlerFilterFactories = new List>() { (routeHandlerContext, next) => async (context) => { @@ -5656,7 +5656,7 @@ string HelloName(int? one, string? two, int? three, string? four, int? five, boo // Act var factoryResult = RequestDelegateFactory.Create(HelloName, new RequestDelegateFactoryOptions() { - EndpointFilterFactories = new List>() + RouteHandlerFilterFactories = new List>() { (routeHandlerContext, next) => async (context) => { @@ -5687,7 +5687,7 @@ string HelloName() // Act var factoryResult = RequestDelegateFactory.Create(HelloName, new RequestDelegateFactoryOptions() { - EndpointFilterFactories = new List>() + RouteHandlerFilterFactories = new List>() { (routeHandlerContext, next) => async (context) => { @@ -6081,7 +6081,7 @@ public void Create_ReturnsSameRequestDelegatePassedIn_IfNotModifiedByFilters() RequestDelegateFactoryOptions options = new() { - EndpointFilterFactories = new List>() + RouteHandlerFilterFactories = new List>() { (routeHandlerContext, next) => { diff --git a/src/Http/Routing/src/Builder/EndpointFilterExtensions.cs b/src/Http/Routing/src/Builder/EndpointFilterExtensions.cs index 0fc669ec5223..0a5bf6a5fb7a 100644 --- a/src/Http/Routing/src/Builder/EndpointFilterExtensions.cs +++ b/src/Http/Routing/src/Builder/EndpointFilterExtensions.cs @@ -117,8 +117,7 @@ public static TBuilder AddEndpointFilter(this TBuilder builder, Func void +Microsoft.AspNetCore.Routing.RouteEndpointBuilder.RouteHandlerFilterFactories.get -> System.Collections.Generic.IList!>! Microsoft.AspNetCore.Routing.RouteGroupBuilder Microsoft.AspNetCore.Routing.RouteGroupContext Microsoft.AspNetCore.Routing.RouteGroupContext.ApplicationServices.get -> System.IServiceProvider! diff --git a/src/Http/Routing/src/RouteEndpointBuilder.cs b/src/Http/Routing/src/RouteEndpointBuilder.cs index 2660c24bb261..ed2d090ce763 100644 --- a/src/Http/Routing/src/RouteEndpointBuilder.cs +++ b/src/Http/Routing/src/RouteEndpointBuilder.cs @@ -12,15 +12,17 @@ namespace Microsoft.AspNetCore.Routing; /// public sealed class RouteEndpointBuilder : EndpointBuilder { - // TODO: Make this public as a gettable IReadOnlyList>. - // AddEndpointFilter will still be the only way to mutate this list. - internal List>? EndpointFilterFactories { get; set; } - /// /// Gets or sets the associated with this endpoint. /// public RoutePattern RoutePattern { get; set; } + /// + /// Gets the collection of route handler filter factories associated with this endpoint. + /// + public IList> RouteHandlerFilterFactories { get; } + = new List>(); + /// /// Gets or sets the order assigned to the endpoint. /// diff --git a/src/Http/Routing/src/RouteEndpointDataSource.cs b/src/Http/Routing/src/RouteEndpointDataSource.cs index 3afff038bc7d..9432d5e43d1a 100644 --- a/src/Http/Routing/src/RouteEndpointDataSource.cs +++ b/src/Http/Routing/src/RouteEndpointDataSource.cs @@ -142,7 +142,11 @@ private RouteEndpointBuilder CreateRouteEndpointBuilder( displayName = $"Fallback {displayName}"; } - RequestDelegate? factoryCreatedRequestDelegate = null; + // If we're not a route handler, we started with a fully realized (although unfiltered) RequestDelegate, so we can just redirect to that + // while running any conventions. We'll put the original back if it remains unfiltered right before building the endpoint. + RequestDelegate? factoryCreatedRequestDelegate = isRouteHandler ? null : (RequestDelegate)entry.RouteHandler; + + // Let existing conventions capture and call into builder.RequestDelegate as long as they do so after it has been created. RequestDelegate redirectRequestDelegate = context => { if (factoryCreatedRequestDelegate is null) @@ -153,26 +157,16 @@ private RouteEndpointBuilder CreateRouteEndpointBuilder( return factoryCreatedRequestDelegate(context); }; + // Add MethodInfo and HttpMethodMetadata (if any) as first metadata items as they are intrinsic to the route much like + // the pattern or default display name. This gives visibility to conventions like WithOpenApi() to intrinsic route details + // (namely the MethodInfo) even when applied early as group conventions. RouteEndpointBuilder builder = new(redirectRequestDelegate, pattern, order) { DisplayName = displayName, ApplicationServices = _applicationServices, + Metadata = { handler.Method }, }; - if (isRouteHandler) - { - // Add MethodInfo and HttpMethodMetadata (if any) as first metadata items as they are intrinsic to the route much like - // the pattern or default display name. This gives visibility to conventions like WithOpenApi() to intrinsic route details - // (namely the MethodInfo) even when applied early as group conventions. - builder.Metadata.Add(handler.Method); - } - else - { - // If we're not a route handler, we started with a fully realized (although unfiltered) RequestDelegate, so we can just redirect to that - // while running any conventions. We'll put the original back if it remains unfiltered right before building the endpoint. - factoryCreatedRequestDelegate = (RequestDelegate)entry.RouteHandler; - } - if (entry.HttpMethods is not null) { builder.Metadata.Add(new HttpMethodMetadata(entry.HttpMethods)); @@ -203,7 +197,7 @@ private RouteEndpointBuilder CreateRouteEndpointBuilder( entrySpecificConvention(builder); } - if (isRouteHandler || builder.EndpointFilterFactories is { Count: > 0}) + if (isRouteHandler || builder.RouteHandlerFilterFactories.Count > 0) { var routeParamNames = new List(pattern.Parameters.Count); foreach (var parameter in pattern.Parameters) @@ -218,7 +212,7 @@ private RouteEndpointBuilder CreateRouteEndpointBuilder( ThrowOnBadRequest = _throwOnBadRequest, DisableInferBodyFromParameters = ShouldDisableInferredBodyParameters(entry.HttpMethods), EndpointMetadata = builder.Metadata, - EndpointFilterFactories = builder.EndpointFilterFactories, + RouteHandlerFilterFactories = builder.RouteHandlerFilterFactories, }; // We ignore the returned EndpointMetadata has been already populated since we passed in non-null EndpointMetadata. diff --git a/src/Http/Routing/test/UnitTests/Builder/RequestDelegateEndpointRouteBuilderExtensionsTest.cs b/src/Http/Routing/test/UnitTests/Builder/RequestDelegateEndpointRouteBuilderExtensionsTest.cs index 00187ec4d5d6..b43a63604f0f 100644 --- a/src/Http/Routing/test/UnitTests/Builder/RequestDelegateEndpointRouteBuilderExtensionsTest.cs +++ b/src/Http/Routing/test/UnitTests/Builder/RequestDelegateEndpointRouteBuilderExtensionsTest.cs @@ -192,9 +192,10 @@ public void MapEndpoint_AttributesCollectedAsMetadata() // Assert var endpointBuilder1 = GetRouteEndpointBuilder(builder); Assert.Equal("/", endpointBuilder1.RoutePattern.RawText); - Assert.Equal(2, endpointBuilder1.Metadata.Count); - Assert.IsType(endpointBuilder1.Metadata[0]); - Assert.IsType(endpointBuilder1.Metadata[1]); + Assert.Equal(3, endpointBuilder1.Metadata.Count); + Assert.Equal(((RequestDelegate)Handle).Method, endpointBuilder1.Metadata[0]); + Assert.IsType(endpointBuilder1.Metadata[1]); + Assert.IsType(endpointBuilder1.Metadata[2]); } [Fact] @@ -229,10 +230,11 @@ public void MapEndpoint_PrecedenceOfMetadata_BuilderMetadataReturned() // As with the Delegate Map method overloads for route handlers, the attributes on the RequestDelegate // can override the HttpMethodMetadata. Extension methods could already do this. - Assert.Equal(3, endpoint.Metadata.Count); - Assert.Equal("METHOD", GetMethod(endpoint.Metadata[0])); - Assert.Equal("ATTRIBUTE", GetMethod(endpoint.Metadata[1])); - Assert.Equal("BUILDER", GetMethod(endpoint.Metadata[2])); + Assert.Equal(4, endpoint.Metadata.Count); + Assert.Equal(((RequestDelegate)HandleHttpMetdata).Method, endpoint.Metadata[0]); + Assert.Equal("METHOD", GetMethod(endpoint.Metadata[1])); + Assert.Equal("ATTRIBUTE", GetMethod(endpoint.Metadata[2])); + Assert.Equal("BUILDER", GetMethod(endpoint.Metadata[3])); Assert.Equal("BUILDER", endpoint.Metadata.GetMetadata()?.HttpMethods.Single()); diff --git a/src/Http/Routing/test/UnitTests/RouteEndpointBuilderTest.cs b/src/Http/Routing/test/UnitTests/RouteEndpointBuilderTest.cs index 6ca5655e1053..2593b2b8d88c 100644 --- a/src/Http/Routing/test/UnitTests/RouteEndpointBuilderTest.cs +++ b/src/Http/Routing/test/UnitTests/RouteEndpointBuilderTest.cs @@ -45,8 +45,7 @@ public async void Build_DoesNot_RunFilters() var builder = new RouteEndpointBuilder(requestDelegate, RoutePatternFactory.Parse("/"), defaultOrder); - builder.EndpointFilterFactories = new List>(); - builder.EndpointFilterFactories.Add((endopintContext, next) => + builder.RouteHandlerFilterFactories.Add((endopintContext, next) => { endpointFilterCallCount++; From bbfdc8a76cedb8b52df5d7bdd94c92f4ca9a89b3 Mon Sep 17 00:00:00 2001 From: Stephen Halter Date: Fri, 29 Jul 2022 17:40:10 -0700 Subject: [PATCH 04/10] Fix tests --- .../src/ConfigureRouteHandlerOptions.cs | 6 ++-- .../src/Microsoft.AspNetCore.Routing.csproj | 1 - .../Routing/src/RouteEndpointDataSource.cs | 13 ++++---- src/Shared/RoslynUtils/GeneratedNameParser.cs | 30 ------------------- src/Shared/RoslynUtils/TypeHelper.cs | 30 +++++++++++++++++++ 5 files changed, 39 insertions(+), 41 deletions(-) delete mode 100644 src/Shared/RoslynUtils/GeneratedNameParser.cs diff --git a/src/Http/Routing/src/ConfigureRouteHandlerOptions.cs b/src/Http/Routing/src/ConfigureRouteHandlerOptions.cs index 1341c9bb5f1a..5b38606d58e7 100644 --- a/src/Http/Routing/src/ConfigureRouteHandlerOptions.cs +++ b/src/Http/Routing/src/ConfigureRouteHandlerOptions.cs @@ -8,16 +8,16 @@ namespace Microsoft.AspNetCore.Routing; internal sealed class ConfigureRouteHandlerOptions : IConfigureOptions { - private readonly IHostEnvironment _environment; + private readonly IHostEnvironment? _environment; - public ConfigureRouteHandlerOptions(IHostEnvironment environment) + public ConfigureRouteHandlerOptions(IHostEnvironment? environment = null) { _environment = environment; } public void Configure(RouteHandlerOptions options) { - if (_environment.IsDevelopment()) + if (_environment?.IsDevelopment() ?? false) { options.ThrowOnBadRequest = true; } diff --git a/src/Http/Routing/src/Microsoft.AspNetCore.Routing.csproj b/src/Http/Routing/src/Microsoft.AspNetCore.Routing.csproj index 0e1f47d2ab67..1351c5a7cc8a 100644 --- a/src/Http/Routing/src/Microsoft.AspNetCore.Routing.csproj +++ b/src/Http/Routing/src/Microsoft.AspNetCore.Routing.csproj @@ -27,7 +27,6 @@ - diff --git a/src/Http/Routing/src/RouteEndpointDataSource.cs b/src/Http/Routing/src/RouteEndpointDataSource.cs index 9432d5e43d1a..0d67593bbc7e 100644 --- a/src/Http/Routing/src/RouteEndpointDataSource.cs +++ b/src/Http/Routing/src/RouteEndpointDataSource.cs @@ -7,7 +7,6 @@ using Microsoft.AspNetCore.Builder; using Microsoft.AspNetCore.Http; using Microsoft.AspNetCore.Routing.Patterns; -using Microsoft.CodeAnalysis.CSharp.Symbols; using Microsoft.Extensions.FileProviders; using Microsoft.Extensions.Primitives; @@ -122,13 +121,13 @@ private RouteEndpointBuilder CreateRouteEndpointBuilder( var order = isFallback ? int.MaxValue : 0; var displayName = pattern.RawText ?? pattern.DebuggerToString(); - // Methods defined in a top-level program are generated as statics so the delegate target will be null. - // Inline lambdas are compiler generated method so they be filtered that way. - if (GeneratedNameParser.TryParseLocalFunctionName(handler.Method.Name, out var endpointName) - || !TypeHelper.IsCompilerGeneratedMethod(handler.Method)) + // Don't include the method name for non-route-handlers because the name is just "Invoke" when built from + // ApplicationBuilder.Build(). This was observed in MapSignalRTests and is not very useful. Maybe if we come up + // with a better heuristic for what a useful method name is, we could use it for everything. Inline lambdas are + // compiler generated methods so they are filtered out even for route handlers. + if (isRouteHandler && TypeHelper.TryGetNonCompilerGeneratedMethodName(handler.Method, out var methodName)) { - endpointName ??= handler.Method.Name; - displayName = $"{displayName} => {endpointName}"; + displayName = $"{displayName} => {methodName}"; } if (entry.HttpMethods is not null) diff --git a/src/Shared/RoslynUtils/GeneratedNameParser.cs b/src/Shared/RoslynUtils/GeneratedNameParser.cs deleted file mode 100644 index 82d6c2da1fa5..000000000000 --- a/src/Shared/RoslynUtils/GeneratedNameParser.cs +++ /dev/null @@ -1,30 +0,0 @@ -// Licensed to the .NET Foundation under one or more agreements. -// The .NET Foundation licenses this file to you under the MIT license. - -using System.Diagnostics.CodeAnalysis; - -// This code is a stop-gap and exists to address the issues with extracting -// original method names from generated local functions. See https://github.com/dotnet/roslyn/issues/55651 -// for more info. -namespace Microsoft.CodeAnalysis.CSharp.Symbols; - -internal static class GeneratedNameParser -{ - /// - /// Parses generated local function name out of a generated method name. - /// - internal static bool TryParseLocalFunctionName(string generatedName, [NotNullWhen(true)] out string? originalName) - { - originalName = null; - - var startIndex = generatedName.LastIndexOf(">g__", StringComparison.Ordinal); - var endIndex = generatedName.LastIndexOf("|", StringComparison.Ordinal); - if (startIndex >= 0 && endIndex >= 0 && endIndex - startIndex > 4) - { - originalName = generatedName.Substring(startIndex + 4, endIndex - startIndex - 4); - return true; - } - - return false; - } -} diff --git a/src/Shared/RoslynUtils/TypeHelper.cs b/src/Shared/RoslynUtils/TypeHelper.cs index b7f8a20ab851..5ae871b5e9bb 100644 --- a/src/Shared/RoslynUtils/TypeHelper.cs +++ b/src/Shared/RoslynUtils/TypeHelper.cs @@ -1,6 +1,7 @@ // Licensed to the .NET Foundation under one or more agreements. // The .NET Foundation licenses this file to you under the MIT license. +using System.Diagnostics.CodeAnalysis; using System.Reflection; namespace System.Runtime.CompilerServices; @@ -36,4 +37,33 @@ internal static bool IsCompilerGeneratedMethod(MethodInfo method) { return Attribute.IsDefined(method, typeof(CompilerGeneratedAttribute)) || IsCompilerGeneratedType(method.DeclaringType); } + + /// + /// Tries to get non-compiler-generaged name of function. This parses generated local function names out of a generated method name if possible. + /// + internal static bool TryGetNonCompilerGeneratedMethodName(MethodInfo method, [NotNullWhen(true)] out string? originalName) + { + var methodName = method.Name; + + if (!IsCompilerGeneratedMethod(method)) + { + originalName = methodName; + return true; + } + + // This code is a stop-gap and exists to address the issues with extracting + // original method names from generated local functions. See https://github.com/dotnet/roslyn/issues/55651 + // for more info. + var startIndex = methodName.LastIndexOf(">g__", StringComparison.Ordinal); + var endIndex = methodName.LastIndexOf("|", StringComparison.Ordinal); + if (startIndex >= 0 && endIndex >= 0 && endIndex - startIndex > 4) + { + originalName = methodName.Substring(startIndex + 4, endIndex - startIndex - 4); + return true; + } + + originalName = null; + return false; + } } + From 43dd13e810bb497609252ba811df81f132e7675f Mon Sep 17 00:00:00 2001 From: Stephen Halter Date: Wed, 3 Aug 2022 12:55:55 -0700 Subject: [PATCH 05/10] Revert "Renames" This reverts commit bb2b157c3b6db7c33543b6f8622ce9a5ef08f8e8. --- .../src/PublicAPI.Unshipped.txt | 4 +- .../src/RequestDelegateFactory.cs | 2 +- .../src/RequestDelegateFactoryOptions.cs | 2 +- .../test/RequestDelegateFactoryTests.cs | 38 +++++++++---------- .../src/Builder/EndpointFilterExtensions.cs | 3 +- src/Http/Routing/src/PublicAPI.Unshipped.txt | 1 - src/Http/Routing/src/RouteEndpointBuilder.cs | 10 ++--- .../Routing/src/RouteEndpointDataSource.cs | 28 ++++++++------ ...egateEndpointRouteBuilderExtensionsTest.cs | 16 ++++---- .../UnitTests/RouteEndpointBuilderTest.cs | 3 +- 10 files changed, 55 insertions(+), 52 deletions(-) diff --git a/src/Http/Http.Extensions/src/PublicAPI.Unshipped.txt b/src/Http/Http.Extensions/src/PublicAPI.Unshipped.txt index a50b6a15b6b0..4b8eccfa6afb 100644 --- a/src/Http/Http.Extensions/src/PublicAPI.Unshipped.txt +++ b/src/Http/Http.Extensions/src/PublicAPI.Unshipped.txt @@ -38,8 +38,6 @@ Microsoft.AspNetCore.Http.ProblemDetailsOptions.ProblemDetailsOptions() -> void *REMOVED*Microsoft.AspNetCore.Mvc.ProblemDetails.Title.set -> void *REMOVED*Microsoft.AspNetCore.Mvc.ProblemDetails.Type.get -> string? *REMOVED*Microsoft.AspNetCore.Mvc.ProblemDetails.Type.set -> void -Microsoft.AspNetCore.Http.RequestDelegateFactoryOptions.RouteHandlerFilterFactories.get -> System.Collections.Generic.IList!>? -Microsoft.AspNetCore.Http.RequestDelegateFactoryOptions.RouteHandlerFilterFactories.init -> void Microsoft.AspNetCore.Mvc.ProblemDetails (forwarded, contained in Microsoft.AspNetCore.Http.Abstractions) Microsoft.AspNetCore.Mvc.ProblemDetails.Detail.get -> string? (forwarded, contained in Microsoft.AspNetCore.Http.Abstractions) Microsoft.AspNetCore.Mvc.ProblemDetails.Detail.set -> void (forwarded, contained in Microsoft.AspNetCore.Http.Abstractions) @@ -54,6 +52,8 @@ Microsoft.AspNetCore.Mvc.ProblemDetails.Title.set -> void (forwarded, contained Microsoft.AspNetCore.Mvc.ProblemDetails.Type.get -> string? (forwarded, contained in Microsoft.AspNetCore.Http.Abstractions) Microsoft.AspNetCore.Mvc.ProblemDetails.Type.set -> void (forwarded, contained in Microsoft.AspNetCore.Http.Abstractions) Microsoft.Extensions.DependencyInjection.ProblemDetailsServiceCollectionExtensions +Microsoft.AspNetCore.Http.RequestDelegateFactoryOptions.EndpointFilterFactories.get -> System.Collections.Generic.IReadOnlyList!>? +Microsoft.AspNetCore.Http.RequestDelegateFactoryOptions.EndpointFilterFactories.init -> void Microsoft.AspNetCore.Http.RequestDelegateFactoryOptions.EndpointMetadata.get -> System.Collections.Generic.IList? Microsoft.AspNetCore.Http.RequestDelegateFactoryOptions.EndpointMetadata.init -> void Microsoft.Extensions.DependencyInjection.HttpJsonServiceExtensions diff --git a/src/Http/Http.Extensions/src/RequestDelegateFactory.cs b/src/Http/Http.Extensions/src/RequestDelegateFactory.cs index 624d430f0736..d27ca2442cc0 100644 --- a/src/Http/Http.Extensions/src/RequestDelegateFactory.cs +++ b/src/Http/Http.Extensions/src/RequestDelegateFactory.cs @@ -208,7 +208,7 @@ private static FactoryContext CreateFactoryContext(RequestDelegateFactoryOptions RouteParameters = options?.RouteParameterNames?.ToList(), ThrowOnBadRequest = options?.ThrowOnBadRequest ?? false, DisableInferredFromBody = options?.DisableInferBodyFromParameters ?? false, - FilterFactories = options?.RouteHandlerFilterFactories?.ToList(), + FilterFactories = options?.EndpointFilterFactories?.ToList(), Metadata = options?.EndpointMetadata ?? new List(), }; } diff --git a/src/Http/Http.Extensions/src/RequestDelegateFactoryOptions.cs b/src/Http/Http.Extensions/src/RequestDelegateFactoryOptions.cs index c2f6b6e97442..a3f05d9db8a3 100644 --- a/src/Http/Http.Extensions/src/RequestDelegateFactoryOptions.cs +++ b/src/Http/Http.Extensions/src/RequestDelegateFactoryOptions.cs @@ -36,7 +36,7 @@ public sealed class RequestDelegateFactoryOptions /// /// The list of filters that must run in the pipeline for a given route handler. /// - public IList>? RouteHandlerFilterFactories { get; init; } + public IReadOnlyList>? EndpointFilterFactories { get; init; } /// /// The mutable initial endpoint metadata to add as part of the creation of the . In most cases, diff --git a/src/Http/Http.Extensions/test/RequestDelegateFactoryTests.cs b/src/Http/Http.Extensions/test/RequestDelegateFactoryTests.cs index 9556b15f493f..8b877f6a6cd2 100644 --- a/src/Http/Http.Extensions/test/RequestDelegateFactoryTests.cs +++ b/src/Http/Http.Extensions/test/RequestDelegateFactoryTests.cs @@ -4889,7 +4889,7 @@ string HelloName(string name) // Act var factoryResult = RequestDelegateFactory.Create(HelloName, new RequestDelegateFactoryOptions() { - RouteHandlerFilterFactories = new List>() + EndpointFilterFactories = new List>() { (routeHandlerContext, next) => async (context) => { @@ -4921,7 +4921,7 @@ public async Task RequestDelegateFactory_InvokesFilters_OnDelegateWithTarget() // Act var factoryResult = RequestDelegateFactory.Create((string name) => $"Hello, {name}!", new RequestDelegateFactoryOptions() { - RouteHandlerFilterFactories = new List>() + EndpointFilterFactories = new List>() { (routeHandlerContext, next) => async (context) => { @@ -4963,7 +4963,7 @@ public async Task RequestDelegateFactory_InvokesFilters_OnMethodInfoWithNullTarg // Act var factoryResult = RequestDelegateFactory.Create(methodInfo!, null, new RequestDelegateFactoryOptions() { - RouteHandlerFilterFactories = new List>() + EndpointFilterFactories = new List>() { (routeHandlerContext, next) => async (context) => { @@ -5006,7 +5006,7 @@ public async Task RequestDelegateFactory_InvokesFilters_OnMethodInfoWithProvided }; var factoryResult = RequestDelegateFactory.Create(methodInfo!, targetFactory, new RequestDelegateFactoryOptions() { - RouteHandlerFilterFactories = new List>() + EndpointFilterFactories = new List>() { (routeHandlerContext, next) => async (context) => { @@ -5043,7 +5043,7 @@ string HelloName(string name) // Act var factoryResult = RequestDelegateFactory.Create(HelloName, new RequestDelegateFactoryOptions() { - RouteHandlerFilterFactories = new List>() { + EndpointFilterFactories = new List>() { (routeHandlerContext, next) => async (context) => { if (context.HttpContext.Response.StatusCode == 400) @@ -5089,7 +5089,7 @@ string HelloName(string name, int age) // Act var factoryResult = RequestDelegateFactory.Create(HelloName, new RequestDelegateFactoryOptions() { - RouteHandlerFilterFactories = new List>() + EndpointFilterFactories = new List>() { (routeHandlerContext, next) => async (context) => { @@ -5137,7 +5137,7 @@ string HelloName(string name) // Act var factoryResult = RequestDelegateFactory.Create(HelloName, new RequestDelegateFactoryOptions() { - RouteHandlerFilterFactories = new List>() + EndpointFilterFactories = new List>() { (routeHandlerContext, next) => { @@ -5192,7 +5192,7 @@ string HelloName(IFormFileCollection formFiles) // Act var factoryResult = RequestDelegateFactory.Create(HelloName, new RequestDelegateFactoryOptions() { - RouteHandlerFilterFactories = new List>() + EndpointFilterFactories = new List>() { (routeHandlerContext, next) => { @@ -5243,7 +5243,7 @@ string PrintTodo(Todo todo) // Act var factoryResult = RequestDelegateFactory.Create(PrintTodo, new RequestDelegateFactoryOptions() { - RouteHandlerFilterFactories = new List>() + EndpointFilterFactories = new List>() { (routeHandlerContext, next) => async (context) => { @@ -5284,7 +5284,7 @@ string HelloName(string name) // Act var factoryResult = RequestDelegateFactory.Create(HelloName, new RequestDelegateFactoryOptions() { - RouteHandlerFilterFactories = new List>() + EndpointFilterFactories = new List>() { (routeHandlerContext, next) => async (context) => { @@ -5327,7 +5327,7 @@ string HelloName(string name) // Act var factoryResult = RequestDelegateFactory.Create(HelloName, new RequestDelegateFactoryOptions() { - RouteHandlerFilterFactories = new List>() + EndpointFilterFactories = new List>() { (routeHandlerContext, next) => async (context) => { @@ -5396,7 +5396,7 @@ public async Task CanInvokeFilter_OnTaskOfTReturningHandler(Delegate @delegate) // Act var factoryResult = RequestDelegateFactory.Create(@delegate, new RequestDelegateFactoryOptions() { - RouteHandlerFilterFactories = new List>() + EndpointFilterFactories = new List>() { (routeHandlerContext, next) => async (context) => { @@ -5454,7 +5454,7 @@ public async Task CanInvokeFilter_OnValueTaskOfTReturningHandler(Delegate @deleg // Act var factoryResult = RequestDelegateFactory.Create(@delegate, new RequestDelegateFactoryOptions() { - RouteHandlerFilterFactories = new List>() + EndpointFilterFactories = new List>() { (routeHandlerContext, next) => async (context) => { @@ -5519,7 +5519,7 @@ public async Task CanInvokeFilter_OnVoidReturningHandler(Delegate @delegate) // Act var factoryResult = RequestDelegateFactory.Create(@delegate, new RequestDelegateFactoryOptions() { - RouteHandlerFilterFactories = new List>() + EndpointFilterFactories = new List>() { (routeHandlerContext, next) => async (context) => { @@ -5553,7 +5553,7 @@ async Task HandlerWithTaskAwait(HttpContext c) // Act var factoryResult = RequestDelegateFactory.Create(HandlerWithTaskAwait, new RequestDelegateFactoryOptions() { - RouteHandlerFilterFactories = new List>() + EndpointFilterFactories = new List>() { (routeHandlerContext, next) => async (context) => { @@ -5620,7 +5620,7 @@ public async Task CanInvokeFilter_OnHandlerReturningTasksOfStruct(Delegate @dele // Act var factoryResult = RequestDelegateFactory.Create(@delegate, new RequestDelegateFactoryOptions() { - RouteHandlerFilterFactories = new List>() + EndpointFilterFactories = new List>() { (routeHandlerContext, next) => async (context) => { @@ -5656,7 +5656,7 @@ string HelloName(int? one, string? two, int? three, string? four, int? five, boo // Act var factoryResult = RequestDelegateFactory.Create(HelloName, new RequestDelegateFactoryOptions() { - RouteHandlerFilterFactories = new List>() + EndpointFilterFactories = new List>() { (routeHandlerContext, next) => async (context) => { @@ -5687,7 +5687,7 @@ string HelloName() // Act var factoryResult = RequestDelegateFactory.Create(HelloName, new RequestDelegateFactoryOptions() { - RouteHandlerFilterFactories = new List>() + EndpointFilterFactories = new List>() { (routeHandlerContext, next) => async (context) => { @@ -6081,7 +6081,7 @@ public void Create_ReturnsSameRequestDelegatePassedIn_IfNotModifiedByFilters() RequestDelegateFactoryOptions options = new() { - RouteHandlerFilterFactories = new List>() + EndpointFilterFactories = new List>() { (routeHandlerContext, next) => { diff --git a/src/Http/Routing/src/Builder/EndpointFilterExtensions.cs b/src/Http/Routing/src/Builder/EndpointFilterExtensions.cs index 0a5bf6a5fb7a..0fc669ec5223 100644 --- a/src/Http/Routing/src/Builder/EndpointFilterExtensions.cs +++ b/src/Http/Routing/src/Builder/EndpointFilterExtensions.cs @@ -117,7 +117,8 @@ public static TBuilder AddEndpointFilter(this TBuilder builder, Func void -Microsoft.AspNetCore.Routing.RouteEndpointBuilder.RouteHandlerFilterFactories.get -> System.Collections.Generic.IList!>! Microsoft.AspNetCore.Routing.RouteGroupBuilder Microsoft.AspNetCore.Routing.RouteGroupContext Microsoft.AspNetCore.Routing.RouteGroupContext.ApplicationServices.get -> System.IServiceProvider! diff --git a/src/Http/Routing/src/RouteEndpointBuilder.cs b/src/Http/Routing/src/RouteEndpointBuilder.cs index ed2d090ce763..2660c24bb261 100644 --- a/src/Http/Routing/src/RouteEndpointBuilder.cs +++ b/src/Http/Routing/src/RouteEndpointBuilder.cs @@ -12,17 +12,15 @@ namespace Microsoft.AspNetCore.Routing; /// public sealed class RouteEndpointBuilder : EndpointBuilder { + // TODO: Make this public as a gettable IReadOnlyList>. + // AddEndpointFilter will still be the only way to mutate this list. + internal List>? EndpointFilterFactories { get; set; } + /// /// Gets or sets the associated with this endpoint. /// public RoutePattern RoutePattern { get; set; } - /// - /// Gets the collection of route handler filter factories associated with this endpoint. - /// - public IList> RouteHandlerFilterFactories { get; } - = new List>(); - /// /// Gets or sets the order assigned to the endpoint. /// diff --git a/src/Http/Routing/src/RouteEndpointDataSource.cs b/src/Http/Routing/src/RouteEndpointDataSource.cs index 0d67593bbc7e..e1da21ee77cd 100644 --- a/src/Http/Routing/src/RouteEndpointDataSource.cs +++ b/src/Http/Routing/src/RouteEndpointDataSource.cs @@ -141,11 +141,7 @@ private RouteEndpointBuilder CreateRouteEndpointBuilder( displayName = $"Fallback {displayName}"; } - // If we're not a route handler, we started with a fully realized (although unfiltered) RequestDelegate, so we can just redirect to that - // while running any conventions. We'll put the original back if it remains unfiltered right before building the endpoint. - RequestDelegate? factoryCreatedRequestDelegate = isRouteHandler ? null : (RequestDelegate)entry.RouteHandler; - - // Let existing conventions capture and call into builder.RequestDelegate as long as they do so after it has been created. + RequestDelegate? factoryCreatedRequestDelegate = null; RequestDelegate redirectRequestDelegate = context => { if (factoryCreatedRequestDelegate is null) @@ -156,16 +152,26 @@ private RouteEndpointBuilder CreateRouteEndpointBuilder( return factoryCreatedRequestDelegate(context); }; - // Add MethodInfo and HttpMethodMetadata (if any) as first metadata items as they are intrinsic to the route much like - // the pattern or default display name. This gives visibility to conventions like WithOpenApi() to intrinsic route details - // (namely the MethodInfo) even when applied early as group conventions. RouteEndpointBuilder builder = new(redirectRequestDelegate, pattern, order) { DisplayName = displayName, ApplicationServices = _applicationServices, - Metadata = { handler.Method }, }; + if (isRouteHandler) + { + // Add MethodInfo and HttpMethodMetadata (if any) as first metadata items as they are intrinsic to the route much like + // the pattern or default display name. This gives visibility to conventions like WithOpenApi() to intrinsic route details + // (namely the MethodInfo) even when applied early as group conventions. + builder.Metadata.Add(handler.Method); + } + else + { + // If we're not a route handler, we started with a fully realized (although unfiltered) RequestDelegate, so we can just redirect to that + // while running any conventions. We'll put the original back if it remains unfiltered right before building the endpoint. + factoryCreatedRequestDelegate = (RequestDelegate)entry.RouteHandler; + } + if (entry.HttpMethods is not null) { builder.Metadata.Add(new HttpMethodMetadata(entry.HttpMethods)); @@ -196,7 +202,7 @@ private RouteEndpointBuilder CreateRouteEndpointBuilder( entrySpecificConvention(builder); } - if (isRouteHandler || builder.RouteHandlerFilterFactories.Count > 0) + if (isRouteHandler || builder.EndpointFilterFactories is { Count: > 0}) { var routeParamNames = new List(pattern.Parameters.Count); foreach (var parameter in pattern.Parameters) @@ -211,7 +217,7 @@ private RouteEndpointBuilder CreateRouteEndpointBuilder( ThrowOnBadRequest = _throwOnBadRequest, DisableInferBodyFromParameters = ShouldDisableInferredBodyParameters(entry.HttpMethods), EndpointMetadata = builder.Metadata, - RouteHandlerFilterFactories = builder.RouteHandlerFilterFactories, + EndpointFilterFactories = builder.EndpointFilterFactories, }; // We ignore the returned EndpointMetadata has been already populated since we passed in non-null EndpointMetadata. diff --git a/src/Http/Routing/test/UnitTests/Builder/RequestDelegateEndpointRouteBuilderExtensionsTest.cs b/src/Http/Routing/test/UnitTests/Builder/RequestDelegateEndpointRouteBuilderExtensionsTest.cs index b43a63604f0f..00187ec4d5d6 100644 --- a/src/Http/Routing/test/UnitTests/Builder/RequestDelegateEndpointRouteBuilderExtensionsTest.cs +++ b/src/Http/Routing/test/UnitTests/Builder/RequestDelegateEndpointRouteBuilderExtensionsTest.cs @@ -192,10 +192,9 @@ public void MapEndpoint_AttributesCollectedAsMetadata() // Assert var endpointBuilder1 = GetRouteEndpointBuilder(builder); Assert.Equal("/", endpointBuilder1.RoutePattern.RawText); - Assert.Equal(3, endpointBuilder1.Metadata.Count); - Assert.Equal(((RequestDelegate)Handle).Method, endpointBuilder1.Metadata[0]); - Assert.IsType(endpointBuilder1.Metadata[1]); - Assert.IsType(endpointBuilder1.Metadata[2]); + Assert.Equal(2, endpointBuilder1.Metadata.Count); + Assert.IsType(endpointBuilder1.Metadata[0]); + Assert.IsType(endpointBuilder1.Metadata[1]); } [Fact] @@ -230,11 +229,10 @@ public void MapEndpoint_PrecedenceOfMetadata_BuilderMetadataReturned() // As with the Delegate Map method overloads for route handlers, the attributes on the RequestDelegate // can override the HttpMethodMetadata. Extension methods could already do this. - Assert.Equal(4, endpoint.Metadata.Count); - Assert.Equal(((RequestDelegate)HandleHttpMetdata).Method, endpoint.Metadata[0]); - Assert.Equal("METHOD", GetMethod(endpoint.Metadata[1])); - Assert.Equal("ATTRIBUTE", GetMethod(endpoint.Metadata[2])); - Assert.Equal("BUILDER", GetMethod(endpoint.Metadata[3])); + Assert.Equal(3, endpoint.Metadata.Count); + Assert.Equal("METHOD", GetMethod(endpoint.Metadata[0])); + Assert.Equal("ATTRIBUTE", GetMethod(endpoint.Metadata[1])); + Assert.Equal("BUILDER", GetMethod(endpoint.Metadata[2])); Assert.Equal("BUILDER", endpoint.Metadata.GetMetadata()?.HttpMethods.Single()); diff --git a/src/Http/Routing/test/UnitTests/RouteEndpointBuilderTest.cs b/src/Http/Routing/test/UnitTests/RouteEndpointBuilderTest.cs index 2593b2b8d88c..6ca5655e1053 100644 --- a/src/Http/Routing/test/UnitTests/RouteEndpointBuilderTest.cs +++ b/src/Http/Routing/test/UnitTests/RouteEndpointBuilderTest.cs @@ -45,7 +45,8 @@ public async void Build_DoesNot_RunFilters() var builder = new RouteEndpointBuilder(requestDelegate, RoutePatternFactory.Parse("/"), defaultOrder); - builder.RouteHandlerFilterFactories.Add((endopintContext, next) => + builder.EndpointFilterFactories = new List>(); + builder.EndpointFilterFactories.Add((endopintContext, next) => { endpointFilterCallCount++; From 2cf906641dc0bcc03b5abba8ff178e4e513fc67e Mon Sep 17 00:00:00 2001 From: Stephen Halter Date: Wed, 3 Aug 2022 12:57:45 -0700 Subject: [PATCH 06/10] Add MethodInfo to all endpoint metadata in RouteEndpointDataSource --- .../Routing/src/RouteEndpointDataSource.cs | 24 +++++++------------ ...egateEndpointRouteBuilderExtensionsTest.cs | 16 +++++++------ 2 files changed, 18 insertions(+), 22 deletions(-) diff --git a/src/Http/Routing/src/RouteEndpointDataSource.cs b/src/Http/Routing/src/RouteEndpointDataSource.cs index e1da21ee77cd..476b032fbf2d 100644 --- a/src/Http/Routing/src/RouteEndpointDataSource.cs +++ b/src/Http/Routing/src/RouteEndpointDataSource.cs @@ -141,7 +141,11 @@ private RouteEndpointBuilder CreateRouteEndpointBuilder( displayName = $"Fallback {displayName}"; } - RequestDelegate? factoryCreatedRequestDelegate = null; + // If we're not a route handler, we started with a fully realized (although unfiltered) RequestDelegate, so we can just redirect to that + // while running any conventions. We'll put the original back if it remains unfiltered right before building the endpoint. + RequestDelegate? factoryCreatedRequestDelegate = isRouteHandler ? null : (RequestDelegate)entry.RouteHandler; + + // Let existing conventions capture and call into builder.RequestDelegate as long as they do so after it has been created. RequestDelegate redirectRequestDelegate = context => { if (factoryCreatedRequestDelegate is null) @@ -152,26 +156,16 @@ private RouteEndpointBuilder CreateRouteEndpointBuilder( return factoryCreatedRequestDelegate(context); }; + // Add MethodInfo and HttpMethodMetadata (if any) as first metadata items as they are intrinsic to the route much like + // the pattern or default display name. This gives visibility to conventions like WithOpenApi() to intrinsic route details + // (namely the MethodInfo) even when applied early as group conventions. RouteEndpointBuilder builder = new(redirectRequestDelegate, pattern, order) { DisplayName = displayName, ApplicationServices = _applicationServices, + Metadata = { handler.Method }, }; - if (isRouteHandler) - { - // Add MethodInfo and HttpMethodMetadata (if any) as first metadata items as they are intrinsic to the route much like - // the pattern or default display name. This gives visibility to conventions like WithOpenApi() to intrinsic route details - // (namely the MethodInfo) even when applied early as group conventions. - builder.Metadata.Add(handler.Method); - } - else - { - // If we're not a route handler, we started with a fully realized (although unfiltered) RequestDelegate, so we can just redirect to that - // while running any conventions. We'll put the original back if it remains unfiltered right before building the endpoint. - factoryCreatedRequestDelegate = (RequestDelegate)entry.RouteHandler; - } - if (entry.HttpMethods is not null) { builder.Metadata.Add(new HttpMethodMetadata(entry.HttpMethods)); diff --git a/src/Http/Routing/test/UnitTests/Builder/RequestDelegateEndpointRouteBuilderExtensionsTest.cs b/src/Http/Routing/test/UnitTests/Builder/RequestDelegateEndpointRouteBuilderExtensionsTest.cs index 00187ec4d5d6..b43a63604f0f 100644 --- a/src/Http/Routing/test/UnitTests/Builder/RequestDelegateEndpointRouteBuilderExtensionsTest.cs +++ b/src/Http/Routing/test/UnitTests/Builder/RequestDelegateEndpointRouteBuilderExtensionsTest.cs @@ -192,9 +192,10 @@ public void MapEndpoint_AttributesCollectedAsMetadata() // Assert var endpointBuilder1 = GetRouteEndpointBuilder(builder); Assert.Equal("/", endpointBuilder1.RoutePattern.RawText); - Assert.Equal(2, endpointBuilder1.Metadata.Count); - Assert.IsType(endpointBuilder1.Metadata[0]); - Assert.IsType(endpointBuilder1.Metadata[1]); + Assert.Equal(3, endpointBuilder1.Metadata.Count); + Assert.Equal(((RequestDelegate)Handle).Method, endpointBuilder1.Metadata[0]); + Assert.IsType(endpointBuilder1.Metadata[1]); + Assert.IsType(endpointBuilder1.Metadata[2]); } [Fact] @@ -229,10 +230,11 @@ public void MapEndpoint_PrecedenceOfMetadata_BuilderMetadataReturned() // As with the Delegate Map method overloads for route handlers, the attributes on the RequestDelegate // can override the HttpMethodMetadata. Extension methods could already do this. - Assert.Equal(3, endpoint.Metadata.Count); - Assert.Equal("METHOD", GetMethod(endpoint.Metadata[0])); - Assert.Equal("ATTRIBUTE", GetMethod(endpoint.Metadata[1])); - Assert.Equal("BUILDER", GetMethod(endpoint.Metadata[2])); + Assert.Equal(4, endpoint.Metadata.Count); + Assert.Equal(((RequestDelegate)HandleHttpMetdata).Method, endpoint.Metadata[0]); + Assert.Equal("METHOD", GetMethod(endpoint.Metadata[1])); + Assert.Equal("ATTRIBUTE", GetMethod(endpoint.Metadata[2])); + Assert.Equal("BUILDER", GetMethod(endpoint.Metadata[3])); Assert.Equal("BUILDER", endpoint.Metadata.GetMetadata()?.HttpMethods.Single()); From 8426c40648e2a96c0aab11e10f61370c068aac47 Mon Sep 17 00:00:00 2001 From: Stephen Halter Date: Wed, 3 Aug 2022 12:14:14 -0700 Subject: [PATCH 07/10] Fix tests and address PR feedback - No. I do not think normalizing the display names is a breaking change. --- src/Http/Routing/test/UnitTests/RouteHandlerOptionsTests.cs | 5 +++-- src/Middleware/Rewrite/test/MiddlewareTests.cs | 2 +- src/Shared/RoslynUtils/TypeHelper.cs | 4 ++-- 3 files changed, 6 insertions(+), 5 deletions(-) diff --git a/src/Http/Routing/test/UnitTests/RouteHandlerOptionsTests.cs b/src/Http/Routing/test/UnitTests/RouteHandlerOptionsTests.cs index 247fc36674f8..96025ce57446 100644 --- a/src/Http/Routing/test/UnitTests/RouteHandlerOptionsTests.cs +++ b/src/Http/Routing/test/UnitTests/RouteHandlerOptionsTests.cs @@ -55,14 +55,15 @@ public void ThrowOnBadRequestIsNotOverwrittenIfNotInDevelopmentEnvironment() } [Fact] - public void RouteHandlerOptionsFailsToResolveWithoutHostEnvironment() + public void RouteHandlerOptionsCanResolveWithoutHostEnvironment() { var services = new ServiceCollection(); services.AddOptions(); services.AddRouting(); var serviceProvider = services.BuildServiceProvider(); - Assert.Throws(() => serviceProvider.GetRequiredService>()); + var options = serviceProvider.GetRequiredService>(); + Assert.False(options.Value.ThrowOnBadRequest); } private class HostEnvironment : IHostEnvironment diff --git a/src/Middleware/Rewrite/test/MiddlewareTests.cs b/src/Middleware/Rewrite/test/MiddlewareTests.cs index eedf71436506..b6078a1c5aef 100644 --- a/src/Middleware/Rewrite/test/MiddlewareTests.cs +++ b/src/Middleware/Rewrite/test/MiddlewareTests.cs @@ -668,7 +668,7 @@ public async Task RewriteAfterUseRoutingHitsOriginalEndpoint() var response = await server.CreateClient().GetStringAsync("foo"); - Assert.Equal("/foo HTTP: GET from /foos", response); + Assert.Equal("HTTP: GET /foo from /foos", response); } [Fact] diff --git a/src/Shared/RoslynUtils/TypeHelper.cs b/src/Shared/RoslynUtils/TypeHelper.cs index 5ae871b5e9bb..72b27fab86e8 100644 --- a/src/Shared/RoslynUtils/TypeHelper.cs +++ b/src/Shared/RoslynUtils/TypeHelper.cs @@ -33,13 +33,13 @@ internal static bool IsCompilerGeneratedType(Type? type = null) /// /// The method to evaluate. /// if is compiler generated. - internal static bool IsCompilerGeneratedMethod(MethodInfo method) + private static bool IsCompilerGeneratedMethod(MethodInfo method) { return Attribute.IsDefined(method, typeof(CompilerGeneratedAttribute)) || IsCompilerGeneratedType(method.DeclaringType); } /// - /// Tries to get non-compiler-generaged name of function. This parses generated local function names out of a generated method name if possible. + /// Tries to get non-compiler-generated name of function. This parses generated local function names out of a generated method name if possible. /// internal static bool TryGetNonCompilerGeneratedMethodName(MethodInfo method, [NotNullWhen(true)] out string? originalName) { From df764e84e0737e1a292dd0900728e89a5ed3c71a Mon Sep 17 00:00:00 2001 From: Stephen Halter Date: Wed, 3 Aug 2022 13:29:35 -0700 Subject: [PATCH 08/10] Add back TryParseLocalFunctionName --- src/Shared/RoslynUtils/TypeHelper.cs | 33 +++++++++++++++++----------- 1 file changed, 20 insertions(+), 13 deletions(-) diff --git a/src/Shared/RoslynUtils/TypeHelper.cs b/src/Shared/RoslynUtils/TypeHelper.cs index 72b27fab86e8..ad84c696cf3d 100644 --- a/src/Shared/RoslynUtils/TypeHelper.cs +++ b/src/Shared/RoslynUtils/TypeHelper.cs @@ -38,6 +38,25 @@ private static bool IsCompilerGeneratedMethod(MethodInfo method) return Attribute.IsDefined(method, typeof(CompilerGeneratedAttribute)) || IsCompilerGeneratedType(method.DeclaringType); } + /// + /// Parses generated local function name out of a generated method name. This code is a stop-gap and exists to address the issues with extracting + /// original method names from generated local functions. See https://github.com/dotnet/roslyn/issues/55651 for more info. + /// + internal static bool TryParseLocalFunctionName(string generatedName, [NotNullWhen(true)] out string? originalName) + { + originalName = null; + + var startIndex = generatedName.LastIndexOf(">g__", StringComparison.Ordinal); + var endIndex = generatedName.LastIndexOf("|", StringComparison.Ordinal); + if (startIndex >= 0 && endIndex >= 0 && endIndex - startIndex > 4) + { + originalName = generatedName.Substring(startIndex + 4, endIndex - startIndex - 4); + return true; + } + + return false; + } + /// /// Tries to get non-compiler-generated name of function. This parses generated local function names out of a generated method name if possible. /// @@ -51,19 +70,7 @@ internal static bool TryGetNonCompilerGeneratedMethodName(MethodInfo method, [No return true; } - // This code is a stop-gap and exists to address the issues with extracting - // original method names from generated local functions. See https://github.com/dotnet/roslyn/issues/55651 - // for more info. - var startIndex = methodName.LastIndexOf(">g__", StringComparison.Ordinal); - var endIndex = methodName.LastIndexOf("|", StringComparison.Ordinal); - if (startIndex >= 0 && endIndex >= 0 && endIndex - startIndex > 4) - { - originalName = methodName.Substring(startIndex + 4, endIndex - startIndex - 4); - return true; - } - - originalName = null; - return false; + return TryParseLocalFunctionName(methodName, out originalName); } } From 8916cc16030ee2f0bae6c4c24f644c2d74194f66 Mon Sep 17 00:00:00 2001 From: Stephen Halter Date: Wed, 3 Aug 2022 13:31:03 -0700 Subject: [PATCH 09/10] internal -> private --- src/Shared/RoslynUtils/TypeHelper.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Shared/RoslynUtils/TypeHelper.cs b/src/Shared/RoslynUtils/TypeHelper.cs index ad84c696cf3d..51ca29d9c925 100644 --- a/src/Shared/RoslynUtils/TypeHelper.cs +++ b/src/Shared/RoslynUtils/TypeHelper.cs @@ -42,7 +42,7 @@ private static bool IsCompilerGeneratedMethod(MethodInfo method) /// Parses generated local function name out of a generated method name. This code is a stop-gap and exists to address the issues with extracting /// original method names from generated local functions. See https://github.com/dotnet/roslyn/issues/55651 for more info. /// - internal static bool TryParseLocalFunctionName(string generatedName, [NotNullWhen(true)] out string? originalName) + private static bool TryParseLocalFunctionName(string generatedName, [NotNullWhen(true)] out string? originalName) { originalName = null; From 6b4d80ff19196cf9cc6a3a1a9cd0447539b40bf6 Mon Sep 17 00:00:00 2001 From: Stephen Halter Date: Fri, 5 Aug 2022 17:17:38 -0700 Subject: [PATCH 10/10] invocationCallCount is 1 in Build_DoesNot_RunFilters test --- src/Http/Routing/test/UnitTests/RouteEndpointBuilderTest.cs | 1 + 1 file changed, 1 insertion(+) diff --git a/src/Http/Routing/test/UnitTests/RouteEndpointBuilderTest.cs b/src/Http/Routing/test/UnitTests/RouteEndpointBuilderTest.cs index 6ca5655e1053..1bfc5ecc724c 100644 --- a/src/Http/Routing/test/UnitTests/RouteEndpointBuilderTest.cs +++ b/src/Http/Routing/test/UnitTests/RouteEndpointBuilderTest.cs @@ -64,5 +64,6 @@ public async void Build_DoesNot_RunFilters() Assert.Equal(0, endpointFilterCallCount); Assert.Equal(0, invocationFilterCallCount); + Assert.Equal(1, invocationCallCount); } }