From 79803dddb647c673c40280b104a20361791e3204 Mon Sep 17 00:00:00 2001 From: Stephen Halter Date: Fri, 10 Jun 2022 14:57:51 -0700 Subject: [PATCH 01/21] Add EndpointDataSource.GetGroupedEndpoints --- .../src/PublicAPI.Unshipped.txt | 5 +- .../src/RequestDelegateResult.cs | 3 +- .../src/RouteHandlerContext.cs | 14 +- .../src/EndpointMetadataContext.cs | 12 +- .../src/EndpointParameterMetadataContext.cs | 10 +- .../src/PublicAPI.Unshipped.txt | 8 +- .../src/RequestDelegateFactory.cs | 26 +- .../test/RequestDelegateFactoryTests.cs | 22 +- .../Builder/EndpointRouteBuilderExtensions.cs | 72 +----- .../src/Builder/RouteHandlerBuilder.cs | 16 +- .../Builder/RouteHandlerFilterExtensions.cs | 12 +- .../src/CompositeEndpointDataSource.cs | 232 +++++++++--------- .../Routing/src/DefaultEndpointDataSource.cs | 6 +- src/Http/Routing/src/EndpointDataSource.cs | 149 ++++++++++- .../Routing/src/EndpointRoutingMiddleware.cs | 4 + .../src/Patterns/RoutePatternFactory.cs | 98 ++++---- src/Http/Routing/src/PublicAPI.Unshipped.txt | 9 +- src/Http/Routing/src/RouteEndpointBuilder.cs | 20 +- src/Http/Routing/src/RouteGroupBuilder.cs | 159 +++++------- src/Http/Routing/src/RouteGroupContext.cs | 46 ++++ .../src/RouteHandlerEndpointDataSource.cs | 194 +++++++++++++++ .../test/FunctionalTests/RouteHandlerTest.cs | 65 +++++ .../test/UnitTests/Builder/GroupTest.cs | 65 +++-- ...egateEndpointRouteBuilderExtensionsTest.cs | 4 +- ...ndlerEndpointRouteBuilderExtensionsTest.cs | 13 +- .../CompositeEndpointDataSourceTest.cs | 157 +++++++++++- .../EndpointRoutingMiddlewareTest.cs | 1 + 27 files changed, 975 insertions(+), 447 deletions(-) create mode 100644 src/Http/Routing/src/RouteGroupContext.cs create mode 100644 src/Http/Routing/src/RouteHandlerEndpointDataSource.cs diff --git a/src/Http/Http.Abstractions/src/PublicAPI.Unshipped.txt b/src/Http/Http.Abstractions/src/PublicAPI.Unshipped.txt index b8d0ab94d187..2f5fcd95247f 100644 --- a/src/Http/Http.Abstractions/src/PublicAPI.Unshipped.txt +++ b/src/Http/Http.Abstractions/src/PublicAPI.Unshipped.txt @@ -16,9 +16,10 @@ Microsoft.AspNetCore.Http.Metadata.IRequestSizeLimitMetadata Microsoft.AspNetCore.Http.Metadata.IRequestSizeLimitMetadata.MaxRequestBodySize.get -> long? Microsoft.AspNetCore.Http.RouteHandlerContext Microsoft.AspNetCore.Http.RouteHandlerContext.ApplicationServices.get -> System.IServiceProvider! -Microsoft.AspNetCore.Http.RouteHandlerContext.EndpointMetadata.get -> Microsoft.AspNetCore.Http.EndpointMetadataCollection! +Microsoft.AspNetCore.Http.RouteHandlerContext.EndpointMetadata.get -> System.Collections.Generic.IReadOnlyList! +Microsoft.AspNetCore.Http.RouteHandlerContext.InferredMetadata.get -> System.Collections.Generic.IList! Microsoft.AspNetCore.Http.RouteHandlerContext.MethodInfo.get -> System.Reflection.MethodInfo! -Microsoft.AspNetCore.Http.RouteHandlerContext.RouteHandlerContext(System.Reflection.MethodInfo! methodInfo, Microsoft.AspNetCore.Http.EndpointMetadataCollection! endpointMetadata, System.IServiceProvider! applicationServices) -> void +Microsoft.AspNetCore.Http.RouteHandlerContext.RouteHandlerContext(System.Reflection.MethodInfo! methodInfo, System.Collections.Generic.IReadOnlyList! endpointMetadata, System.Collections.Generic.IList! inferredMetadata, System.IServiceProvider! applicationServices) -> void Microsoft.AspNetCore.Http.RouteHandlerFilterDelegate Microsoft.AspNetCore.Http.RouteHandlerInvocationContext Microsoft.AspNetCore.Http.RouteHandlerInvocationContext.RouteHandlerInvocationContext() -> void diff --git a/src/Http/Http.Abstractions/src/RequestDelegateResult.cs b/src/Http/Http.Abstractions/src/RequestDelegateResult.cs index 55f033fb8ce8..70d2eb956176 100644 --- a/src/Http/Http.Abstractions/src/RequestDelegateResult.cs +++ b/src/Http/Http.Abstractions/src/RequestDelegateResult.cs @@ -23,7 +23,8 @@ public RequestDelegateResult(RequestDelegate requestDelegate, IReadOnlyList - /// Gets endpoint metadata inferred from creating the + /// Gets endpoint metadata inferred from creating the . This does not contain + /// anything from RequestDelegateFactoryOptions.InitialEndpointMetadata. /// public IReadOnlyList EndpointMetadata { get; } } diff --git a/src/Http/Http.Abstractions/src/RouteHandlerContext.cs b/src/Http/Http.Abstractions/src/RouteHandlerContext.cs index 93c398f1e432..417b02a57130 100644 --- a/src/Http/Http.Abstractions/src/RouteHandlerContext.cs +++ b/src/Http/Http.Abstractions/src/RouteHandlerContext.cs @@ -16,15 +16,18 @@ public sealed class RouteHandlerContext /// /// The associated with the route handler of the current request. /// The associated with the endpoint the filter is targeting. + /// The mutable metadata inferred about current endpoint. This will come before in . /// The instance used to access the application services. - public RouteHandlerContext(MethodInfo methodInfo, EndpointMetadataCollection endpointMetadata, IServiceProvider applicationServices) + public RouteHandlerContext(MethodInfo methodInfo, IReadOnlyList endpointMetadata, IList inferredMetadata, IServiceProvider applicationServices) { ArgumentNullException.ThrowIfNull(methodInfo); ArgumentNullException.ThrowIfNull(endpointMetadata); + ArgumentNullException.ThrowIfNull(inferredMetadata); ArgumentNullException.ThrowIfNull(applicationServices); MethodInfo = methodInfo; EndpointMetadata = endpointMetadata; + InferredMetadata = inferredMetadata; ApplicationServices = applicationServices; } @@ -34,9 +37,14 @@ public RouteHandlerContext(MethodInfo methodInfo, EndpointMetadataCollection end public MethodInfo MethodInfo { get; } /// - /// The associated with the current endpoint. + /// The read-only metadata already applied to the current endpoint. /// - public EndpointMetadataCollection EndpointMetadata { get; } + public IReadOnlyList EndpointMetadata { get; } + + /// + /// The mutable metadata inferred about current endpoint. This will come before in . + /// + public IList InferredMetadata { get; } /// /// Gets the instance used to access application services. diff --git a/src/Http/Http.Extensions/src/EndpointMetadataContext.cs b/src/Http/Http.Extensions/src/EndpointMetadataContext.cs index caf96455684c..1cf021800b56 100644 --- a/src/Http/Http.Extensions/src/EndpointMetadataContext.cs +++ b/src/Http/Http.Extensions/src/EndpointMetadataContext.cs @@ -15,15 +15,18 @@ public sealed class EndpointMetadataContext /// /// The of the route handler delegate of the endpoint being created. /// The list of objects that will be added to the metadata of the endpoint. + /// The mutable metadata inferred about current endpoint. This will come before in . /// The instance used to access application services. - public EndpointMetadataContext(MethodInfo method, IList endpointMetadata, IServiceProvider applicationServices) + public EndpointMetadataContext(MethodInfo method, IReadOnlyList endpointMetadata, IList inferredMetadata, IServiceProvider applicationServices) { ArgumentNullException.ThrowIfNull(method); ArgumentNullException.ThrowIfNull(endpointMetadata); + ArgumentNullException.ThrowIfNull(inferredMetadata); ArgumentNullException.ThrowIfNull(applicationServices); Method = method; EndpointMetadata = endpointMetadata; + InferredMetadata = inferredMetadata; ApplicationServices = applicationServices; } @@ -35,7 +38,12 @@ public EndpointMetadataContext(MethodInfo method, IList endpointMetadata /// /// Gets the list of objects that will be added to the metadata of the endpoint. /// - public IList EndpointMetadata { get; } + public IReadOnlyList EndpointMetadata { get; } + + /// + /// The mutable metadata inferred about current endpoint. This will come before in . + /// + public IList InferredMetadata { get; } /// /// Gets the instance used to access application services. diff --git a/src/Http/Http.Extensions/src/EndpointParameterMetadataContext.cs b/src/Http/Http.Extensions/src/EndpointParameterMetadataContext.cs index 202839cc8080..e58c2c85ac2d 100644 --- a/src/Http/Http.Extensions/src/EndpointParameterMetadataContext.cs +++ b/src/Http/Http.Extensions/src/EndpointParameterMetadataContext.cs @@ -15,15 +15,18 @@ public sealed class EndpointParameterMetadataContext /// /// The parameter of the route handler delegate of the endpoint being created. /// The list of objects that will be added to the metadata of the endpoint. + /// The mutable metadata inferred about current endpoint. This will come before in . /// The instance used to access application services. - public EndpointParameterMetadataContext(ParameterInfo parameter, IList endpointMetadata, IServiceProvider applicationServices) + public EndpointParameterMetadataContext(ParameterInfo parameter, IList endpointMetadata, IList inferredMetadata, IServiceProvider applicationServices) { ArgumentNullException.ThrowIfNull(parameter); ArgumentNullException.ThrowIfNull(endpointMetadata); + ArgumentNullException.ThrowIfNull(inferredMetadata); ArgumentNullException.ThrowIfNull(applicationServices); Parameter = parameter; EndpointMetadata = endpointMetadata; + InferredMetadata = inferredMetadata; ApplicationServices = applicationServices; } @@ -37,6 +40,11 @@ public EndpointParameterMetadataContext(ParameterInfo parameter, IList e /// public IList EndpointMetadata { get; } + /// + /// The mutable metadata inferred about current endpoint. This will come before in . + /// + public IList InferredMetadata { get; } + /// /// Gets the instance used to access application services. /// diff --git a/src/Http/Http.Extensions/src/PublicAPI.Unshipped.txt b/src/Http/Http.Extensions/src/PublicAPI.Unshipped.txt index f2913822e181..2e0280c58d5e 100644 --- a/src/Http/Http.Extensions/src/PublicAPI.Unshipped.txt +++ b/src/Http/Http.Extensions/src/PublicAPI.Unshipped.txt @@ -1,13 +1,15 @@ #nullable enable Microsoft.AspNetCore.Http.Metadata.EndpointMetadataContext Microsoft.AspNetCore.Http.Metadata.EndpointMetadataContext.ApplicationServices.get -> System.IServiceProvider! -Microsoft.AspNetCore.Http.Metadata.EndpointMetadataContext.EndpointMetadataContext(System.Reflection.MethodInfo! method, System.Collections.Generic.IList! endpointMetadata, System.IServiceProvider! applicationServices) -> void -Microsoft.AspNetCore.Http.Metadata.EndpointMetadataContext.EndpointMetadata.get -> System.Collections.Generic.IList! +Microsoft.AspNetCore.Http.Metadata.EndpointMetadataContext.EndpointMetadata.get -> System.Collections.Generic.IReadOnlyList! +Microsoft.AspNetCore.Http.Metadata.EndpointMetadataContext.EndpointMetadataContext(System.Reflection.MethodInfo! method, System.Collections.Generic.IReadOnlyList! endpointMetadata, System.Collections.Generic.IList! inferredMetadata, System.IServiceProvider! applicationServices) -> void +Microsoft.AspNetCore.Http.Metadata.EndpointMetadataContext.InferredMetadata.get -> System.Collections.Generic.IList! Microsoft.AspNetCore.Http.Metadata.EndpointMetadataContext.Method.get -> System.Reflection.MethodInfo! Microsoft.AspNetCore.Http.Metadata.EndpointParameterMetadataContext Microsoft.AspNetCore.Http.Metadata.EndpointParameterMetadataContext.ApplicationServices.get -> System.IServiceProvider! -Microsoft.AspNetCore.Http.Metadata.EndpointParameterMetadataContext.EndpointParameterMetadataContext(System.Reflection.ParameterInfo! parameter, System.Collections.Generic.IList! endpointMetadata, System.IServiceProvider! applicationServices) -> void +Microsoft.AspNetCore.Http.Metadata.EndpointParameterMetadataContext.EndpointParameterMetadataContext(System.Reflection.ParameterInfo! parameter, System.Collections.Generic.IList! endpointMetadata, System.Collections.Generic.IList! inferredMetadata, System.IServiceProvider! applicationServices) -> void Microsoft.AspNetCore.Http.Metadata.EndpointParameterMetadataContext.EndpointMetadata.get -> System.Collections.Generic.IList! +Microsoft.AspNetCore.Http.Metadata.EndpointParameterMetadataContext.InferredMetadata.get -> System.Collections.Generic.IList! Microsoft.AspNetCore.Http.Metadata.EndpointParameterMetadataContext.Parameter.get -> System.Reflection.ParameterInfo! Microsoft.AspNetCore.Http.Metadata.IEndpointMetadataProvider Microsoft.AspNetCore.Http.Metadata.IEndpointMetadataProvider.PopulateMetadata(Microsoft.AspNetCore.Http.Metadata.EndpointMetadataContext! context) -> void diff --git a/src/Http/Http.Extensions/src/RequestDelegateFactory.cs b/src/Http/Http.Extensions/src/RequestDelegateFactory.cs index 265351805ef1..f595e4bb4412 100644 --- a/src/Http/Http.Extensions/src/RequestDelegateFactory.cs +++ b/src/Http/Http.Extensions/src/RequestDelegateFactory.cs @@ -179,7 +179,7 @@ public static RequestDelegateResult Create(MethodInfo methodInfo, Func(), @@ -188,13 +188,6 @@ private static FactoryContext CreateFactoryContext(RequestDelegateFactoryOptions DisableInferredFromBody = options?.DisableInferBodyFromParameters ?? false, Filters = options?.RouteHandlerFilterFactories?.ToList() }; - - if (options?.InitialEndpointMetadata is not null) - { - context.Metadata.AddRange(options.InitialEndpointMetadata); - } - - return context; } private static Func CreateTargetableRequestDelegate(MethodInfo methodInfo, Expression? targetExpression, FactoryContext factoryContext, Expression>? targetFactory = null) @@ -215,9 +208,6 @@ private static FactoryContext CreateFactoryContext(RequestDelegateFactoryOptions // return default; // } - // Add MethodInfo as first metadata item - factoryContext.Metadata.Insert(0, methodInfo); - // CreateArguments will add metadata inferred from parameter details var arguments = CreateArguments(methodInfo.GetParameters(), factoryContext); var returnType = methodInfo.ReturnType; @@ -229,9 +219,6 @@ private static FactoryContext CreateFactoryContext(RequestDelegateFactoryOptions factoryContext.ServiceProvider, CollectionsMarshal.AsSpan(factoryContext.Parameters)); - // Add method attributes as metadata *after* any inferred metadata so that the attributes hava a higher specificity - AddMethodAttributesAsMetadata(methodInfo, factoryContext.Metadata); - // If there are filters registered on the route handler, then we update the method call and // return type associated with the request to allow for the filter invocation pipeline. if (factoryContext.Filters is { Count: > 0 }) @@ -488,17 +475,6 @@ private static void PopulateMetadataForEndpoint(EndpointMetadataContext conte T.PopulateMetadata(context); } - private static void AddMethodAttributesAsMetadata(MethodInfo methodInfo, List metadata) - { - var attributes = methodInfo.GetCustomAttributes(); - - // This can be null if the delegate is a dynamic method or compiled from an expression tree - if (attributes is not null) - { - metadata.AddRange(attributes); - } - } - private static Expression[] CreateArguments(ParameterInfo[]? parameters, FactoryContext factoryContext) { if (parameters is null || parameters.Length == 0) diff --git a/src/Http/Http.Extensions/test/RequestDelegateFactoryTests.cs b/src/Http/Http.Extensions/test/RequestDelegateFactoryTests.cs index 445e3c2adbf1..b5812ced546f 100644 --- a/src/Http/Http.Extensions/test/RequestDelegateFactoryTests.cs +++ b/src/Http/Http.Extensions/test/RequestDelegateFactoryTests.cs @@ -5837,7 +5837,7 @@ public void Create_CombinesDefaultMetadata_AndMetadataFromTaskWrappedReturnTypes } [Fact] - public void Create_CombinesDefaultMetadata_AndMetadataFromValueTaskWrappedReturnTypesImplementingIEndpointMetadataProvider() + public void Create_DoesNotCombineDefaultMetadata_AndMetadataFromValueTaskWrappedReturnTypesImplementingIEndpointMetadataProvider() { // Arrange var @delegate = () => ValueTask.FromResult(new CountsDefaultEndpointMetadataResult()); @@ -5853,7 +5853,7 @@ public void Create_CombinesDefaultMetadata_AndMetadataFromValueTaskWrappedReturn var result = RequestDelegateFactory.Create(@delegate, options); // Assert - Assert.Contains(result.EndpointMetadata, m => m is CustomEndpointMetadata { Source: MetadataSource.Caller }); + Assert.DoesNotContain(result.EndpointMetadata, m => m is CustomEndpointMetadata { Source: MetadataSource.Caller }); // Expecting '2' as only MethodInfo and initial metadata will be in the metadata list when this metadata item is added Assert.Contains(result.EndpointMetadata, m => m is DefaultMetadataCountMetadata { Count: 2 }); } @@ -6079,7 +6079,7 @@ public static void PopulateMetadata(EndpointMetadataContext context) { if (context.ApplicationServices?.GetRequiredService() is { } metadataService) { - context.EndpointMetadata.Add(metadataService); + context.InferredMetadata.Add(metadataService); } } @@ -6095,7 +6095,7 @@ public static void PopulateMetadata(EndpointMetadataContext context) { if (context.ApplicationServices?.GetRequiredService() is { } metadataService) { - context.EndpointMetadata.Add(metadataService); + context.InferredMetadata.Add(metadataService); } } } @@ -6112,7 +6112,7 @@ private class AddsCustomEndpointMetadataResult : IEndpointMetadataProvider, IRes { public static void PopulateMetadata(EndpointMetadataContext context) { - context.EndpointMetadata?.Add(new CustomEndpointMetadata { Source = MetadataSource.ReturnType }); + context.InferredMetadata.Add(new CustomEndpointMetadata { Source = MetadataSource.ReturnType }); } public Task ExecuteAsync(HttpContext httpContext) => throw new NotImplementedException(); @@ -6133,7 +6133,7 @@ private class CountsDefaultEndpointMetadataResult : IEndpointMetadataProvider, I public static void PopulateMetadata(EndpointMetadataContext context) { var defaultMetadataCount = context.EndpointMetadata?.Count; - context.EndpointMetadata?.Add(new DefaultMetadataCountMetadata { Count = defaultMetadataCount ?? 0 }); + context.InferredMetadata.Add(new DefaultMetadataCountMetadata { Count = defaultMetadataCount ?? 0 }); } public Task ExecuteAsync(HttpContext httpContext) => throw new NotImplementedException(); @@ -6168,7 +6168,7 @@ public static void PopulateMetadata(EndpointMetadataContext parameterContext) var metadata = parameterContext.EndpointMetadata[i]; if (metadata is IAcceptsMetadata) { - parameterContext.EndpointMetadata.RemoveAt(i); + parameterContext.InferredMetadata.RemoveAt(i); } } } @@ -6186,7 +6186,7 @@ public static void PopulateMetadata(EndpointMetadataContext context) var metadata = context.EndpointMetadata[i]; if (metadata is IAcceptsMetadata) { - context.EndpointMetadata.RemoveAt(i); + context.InferredMetadata.RemoveAt(i); } } } @@ -6204,7 +6204,7 @@ public static void PopulateMetadata(EndpointParameterMetadataContext parameterCo public static void PopulateMetadata(EndpointMetadataContext context) { - context.EndpointMetadata?.Add(new CustomEndpointMetadata { Source = MetadataSource.Property }); + context.InferredMetadata.Add(new CustomEndpointMetadata { Source = MetadataSource.Property }); } } @@ -6219,7 +6219,7 @@ public static void PopulateMetadata(EndpointParameterMetadataContext parameterCo public static void PopulateMetadata(EndpointMetadataContext context) { - context.EndpointMetadata?.Add(new CustomEndpointMetadata { Source = MetadataSource.Parameter }); + context.InferredMetadata.Add(new CustomEndpointMetadata { Source = MetadataSource.Parameter }); } } @@ -6234,7 +6234,7 @@ public static void PopulateMetadata(EndpointParameterMetadataContext parameterCo public static void PopulateMetadata(EndpointMetadataContext context) { - context.EndpointMetadata?.Add(new CustomEndpointMetadata { Source = MetadataSource.Parameter }); + context.InferredMetadata.Add(new CustomEndpointMetadata { Source = MetadataSource.Parameter }); } } diff --git a/src/Http/Routing/src/Builder/EndpointRouteBuilderExtensions.cs b/src/Http/Routing/src/Builder/EndpointRouteBuilderExtensions.cs index 78d1595ece2d..d2a41a446fbf 100644 --- a/src/Http/Routing/src/Builder/EndpointRouteBuilderExtensions.cs +++ b/src/Http/Routing/src/Builder/EndpointRouteBuilderExtensions.cs @@ -4,11 +4,9 @@ using System.Diagnostics.CodeAnalysis; using System.Linq; using System.Reflection; -using System.Runtime.CompilerServices; using Microsoft.AspNetCore.Http; using Microsoft.AspNetCore.Routing; using Microsoft.AspNetCore.Routing.Patterns; -using Microsoft.CodeAnalysis.CSharp.Symbols; using Microsoft.Extensions.DependencyInjection; using Microsoft.Extensions.Options; @@ -475,74 +473,18 @@ private static RouteHandlerBuilder Map( ArgumentNullException.ThrowIfNull(pattern); ArgumentNullException.ThrowIfNull(handler); - const int defaultOrder = 0; - - var fullPattern = pattern; - - if (endpoints is RouteGroupBuilder group) - { - fullPattern = RoutePatternFactory.Combine(group.GroupPrefix, pattern); - } - - var builder = new RouteEndpointBuilder( - pattern, - defaultOrder) - { - DisplayName = fullPattern.RawText ?? fullPattern.DebuggerToString(), - ServiceProvider = endpoints.ServiceProvider, - }; - - // 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 can - // be filtered that way. - if (GeneratedNameParser.TryParseLocalFunctionName(handler.Method.Name, out var endpointName) - || !TypeHelper.IsCompilerGeneratedMethod(handler.Method)) - { - endpointName ??= handler.Method.Name; - builder.DisplayName = $"{builder.DisplayName} => {endpointName}"; - } - - var dataSource = endpoints.DataSources.OfType().FirstOrDefault(); + var dataSource = endpoints.DataSources.OfType().FirstOrDefault(); if (dataSource is null) { - dataSource = new ModelEndpointDataSource(); + var routeHandlerOptions = endpoints.ServiceProvider.GetService>(); + var throwOnBadRequest = routeHandlerOptions?.Value.ThrowOnBadRequest ?? false; + + dataSource = new RouteHandlerEndpointDataSource(endpoints.ServiceProvider, throwOnBadRequest); endpoints.DataSources.Add(dataSource); } - var routeHandlerBuilder = new RouteHandlerBuilder(dataSource.AddEndpointBuilder(builder)); - routeHandlerBuilder.Add(RouteHandlerBuilderConvention); - - [UnconditionalSuppressMessage("Trimmer", "IL2026", Justification = "We surface a RequireUnreferencedCode in the call to enclosing Map method. " + - "The trimmer is unable to infer this on the nested lambda.")] - void RouteHandlerBuilderConvention(EndpointBuilder endpointBuilder) - { - var routeParams = new List(fullPattern.Parameters.Count); - foreach (var part in fullPattern.Parameters) - { - routeParams.Add(part.Name); - } - - var routeHandlerOptions = endpoints.ServiceProvider?.GetService>(); - var options = new RequestDelegateFactoryOptions - { - ServiceProvider = endpoints.ServiceProvider, - RouteParameterNames = routeParams, - ThrowOnBadRequest = routeHandlerOptions?.Value.ThrowOnBadRequest ?? false, - DisableInferBodyFromParameters = disableInferBodyFromParameters, - RouteHandlerFilterFactories = routeHandlerBuilder.RouteHandlerFilterFactories, - InitialEndpointMetadata = initialEndpointMetadata - }; - var filteredRequestDelegateResult = RequestDelegateFactory.Create(handler, options); - - // Add request delegate metadata - foreach (var metadata in filteredRequestDelegateResult.EndpointMetadata) - { - endpointBuilder.Metadata.Add(metadata); - } - - endpointBuilder.RequestDelegate = filteredRequestDelegateResult.RequestDelegate; - } + var conventions = dataSource.AddEndpoint(pattern, handler, initialEndpointMetadata, disableInferBodyFromParameters); - return routeHandlerBuilder; + return new RouteHandlerBuilder(conventions); } } diff --git a/src/Http/Routing/src/Builder/RouteHandlerBuilder.cs b/src/Http/Routing/src/Builder/RouteHandlerBuilder.cs index 735178ff4214..07df57bfaedd 100644 --- a/src/Http/Routing/src/Builder/RouteHandlerBuilder.cs +++ b/src/Http/Routing/src/Builder/RouteHandlerBuilder.cs @@ -1,7 +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 Microsoft.AspNetCore.Http; +using Microsoft.AspNetCore.Routing; namespace Microsoft.AspNetCore.Builder; @@ -11,18 +11,16 @@ namespace Microsoft.AspNetCore.Builder; public sealed class RouteHandlerBuilder : IEndpointConventionBuilder { private readonly IEnumerable? _endpointConventionBuilders; - private readonly IEndpointConventionBuilder? _endpointConventionBuilder; - - internal List> RouteHandlerFilterFactories { get; } = new(); + private readonly List>? _conventions; /// /// Instantiates a new given a single /// . /// - /// The to instantiate with. - internal RouteHandlerBuilder(IEndpointConventionBuilder endpointConventionBuilder) + /// The convention list returned from . + internal RouteHandlerBuilder(List> conventions) { - _endpointConventionBuilder = endpointConventionBuilder; + _conventions = conventions; } /// @@ -41,9 +39,9 @@ public RouteHandlerBuilder(IEnumerable endpointConve /// The convention to add to the builder. public void Add(Action convention) { - if (_endpointConventionBuilder != null) + if (_conventions is not null) { - _endpointConventionBuilder.Add(convention); + _conventions.Add(convention); } else { diff --git a/src/Http/Routing/src/Builder/RouteHandlerFilterExtensions.cs b/src/Http/Routing/src/Builder/RouteHandlerFilterExtensions.cs index f0032cc2bf73..b4cb78b7a1df 100644 --- a/src/Http/Routing/src/Builder/RouteHandlerFilterExtensions.cs +++ b/src/Http/Routing/src/Builder/RouteHandlerFilterExtensions.cs @@ -20,7 +20,7 @@ public static class RouteHandlerFilterExtensions /// A that can be used to further customize the route handler. public static RouteHandlerBuilder AddFilter(this RouteHandlerBuilder builder, IRouteHandlerFilter filter) { - builder.RouteHandlerFilterFactories.Add((routeHandlerContext, next) => (context) => filter.InvokeAsync(context, next)); + builder.AddFilter((routeHandlerContext, next) => (context) => filter.InvokeAsync(context, next)); return builder; } @@ -44,7 +44,7 @@ public static RouteHandlerBuilder AddFilter(this RouteHandlerBuilder builder, IR filterFactory = ActivatorUtilities.CreateFactory(typeof(TFilterType), Type.EmptyTypes); } - builder.RouteHandlerFilterFactories.Add((routeHandlerContext, next) => + builder.AddFilter((routeHandlerContext, next) => { var invokeArguments = new[] { routeHandlerContext }; return (context) => @@ -60,11 +60,11 @@ public static RouteHandlerBuilder AddFilter(this RouteHandlerBuilder builder, IR /// Registers a filter given a delegate onto the route handler. /// /// The . - /// A representing the core logic of the filter. + /// A representing the core logic of the filter. /// A that can be used to further customize the route handler. public static RouteHandlerBuilder AddFilter(this RouteHandlerBuilder builder, Func> routeHandlerFilter) { - builder.RouteHandlerFilterFactories.Add((routeHandlerContext, next) => (context) => routeHandlerFilter(context, next)); + builder.AddFilter((routeHandlerContext, next) => (context) => routeHandlerFilter(context, next)); return builder; } @@ -72,11 +72,11 @@ public static RouteHandlerBuilder AddFilter(this RouteHandlerBuilder builder, Fu /// Register a filter given a delegate representing the filter factory. /// /// The . - /// A representing the logic for constructing the filter. + /// A representing the logic for constructing the filter. /// A that can be used to further customize the route handler. public static RouteHandlerBuilder AddFilter(this RouteHandlerBuilder builder, Func filterFactory) { - builder.RouteHandlerFilterFactories.Add(filterFactory); + builder.Add(endpointBuilder => endpointBuilder.Metadata.Add(filterFactory)); return builder; } } diff --git a/src/Http/Routing/src/CompositeEndpointDataSource.cs b/src/Http/Routing/src/CompositeEndpointDataSource.cs index 96a4121f150f..c4d8d0204d08 100644 --- a/src/Http/Routing/src/CompositeEndpointDataSource.cs +++ b/src/Http/Routing/src/CompositeEndpointDataSource.cs @@ -6,7 +6,6 @@ using System.Diagnostics; using System.Diagnostics.CodeAnalysis; using System.Linq; -using System.Text; using Microsoft.AspNetCore.Http; using Microsoft.Extensions.Primitives; @@ -16,24 +15,20 @@ namespace Microsoft.AspNetCore.Routing; /// Represents an whose values come from a collection of instances. /// [DebuggerDisplay("{DebuggerDisplayString,nq}")] -public sealed class CompositeEndpointDataSource : EndpointDataSource +public sealed class CompositeEndpointDataSource : EndpointDataSource, IDisposable { - private readonly object _lock; - private readonly ICollection _dataSources = default!; - private IReadOnlyList _endpoints = default!; - private IChangeToken _consumerChangeToken; - private CancellationTokenSource _cts; + private readonly object _lock = new(); + private readonly ICollection _dataSources; - private CompositeEndpointDataSource() - { - CreateChangeToken(); - _lock = new object(); - } + private IReadOnlyList? _endpoints; + private IChangeToken? _consumerChangeToken; + private CancellationTokenSource? _cts; + private List? _changeTokenRegistrations; + private bool _disposed; - internal CompositeEndpointDataSource(ObservableCollection dataSources) : this() + internal CompositeEndpointDataSource(ObservableCollection dataSources) { dataSources.CollectionChanged += OnDataSourcesChanged; - _dataSources = dataSources; } @@ -41,8 +36,8 @@ internal CompositeEndpointDataSource(ObservableCollection da /// Instantiates a object from . /// /// An collection of objects. - /// A - public CompositeEndpointDataSource(IEnumerable endpointDataSources) : this() + /// A . + public CompositeEndpointDataSource(IEnumerable endpointDataSources) { _dataSources = new List(); @@ -52,17 +47,7 @@ public CompositeEndpointDataSource(IEnumerable endpointDataS } } - private void OnDataSourcesChanged(object? sender, NotifyCollectionChangedEventArgs e) - { - lock (_lock) - { - // Only trigger changes if composite data source has already initialized endpoints - if (_endpoints != null) - { - HandleChange(); - } - } - } + private void OnDataSourcesChanged(object? sender, NotifyCollectionChangedEventArgs e) => HandleChange(collectionChanged: true); /// /// Returns the collection of instances associated with the object. @@ -70,13 +55,12 @@ private void OnDataSourcesChanged(object? sender, NotifyCollectionChangedEventAr public IEnumerable DataSources => _dataSources; /// - /// Gets a used to signal invalidation of cached - /// instances. + /// Gets a used to signal invalidation of cached instances. /// /// The . public override IChangeToken GetChangeToken() { - EnsureInitialized(); + EnsureChangeTokenInitialized(); return _consumerChangeToken; } @@ -87,46 +71,110 @@ public override IReadOnlyList Endpoints { get { - EnsureInitialized(); + EnsureEndpointsInitialized(); return _endpoints; } } - // Defer initialization to avoid doing lots of reflection on startup. - private void EnsureInitialized() + /// + public override IReadOnlyList GetGroupedEndpoints(RouteGroupContext context) { - if (_endpoints == null) + if (_dataSources.Count is 0) + { + return Array.Empty(); + } + + // We could try to optimize the single data source case by returning its result directly like GroupDataSource does, + // but the CompositeEndpointDataSourceTest class was picky about the Endpoints property creating a shallow copy, + // so we'll shallow copy here for consistency. + var groupedEndpoints = new List(); + + foreach (var dataSource in _dataSources) { - Initialize(); + groupedEndpoints.AddRange(dataSource.GetGroupedEndpoints(context)); } + + // There's no need to cache these the way we do with _endpoints. This is only ever used to get intermediate results. + // Anything using the DataSourceDependentCache like the DfaMatcher will resolve the cached Endpoints property. + return groupedEndpoints; } - // Note: we can't use DataSourceDependentCache here because we also need to handle a list of change - // tokens, which is a complication most of our code doesn't have. - private void Initialize() + /// + public void Dispose() { + // CompositeDataSource is registered as a singleton by default by AddRouting(). + // UseEndpoints() adds all root data sources to this singleton. lock (_lock) { - if (_endpoints == null) - { - _endpoints = _dataSources.SelectMany(d => d.Endpoints).ToArray(); + _disposed = true; - foreach (var dataSource in _dataSources) + if (_changeTokenRegistrations is not null) + { + foreach (var registration in _changeTokenRegistrations) { - ChangeToken.OnChange( - dataSource.GetChangeToken, - HandleChange); + registration.Dispose(); } } + + if (_dataSources is ObservableCollection observableDataSources) + { + observableDataSources.CollectionChanged -= OnDataSourcesChanged; + } + + foreach (var dataSource in _dataSources) + { + (dataSource as IDisposable)?.Dispose(); + } + } + } + + // Defer initialization to avoid doing lots of reflection on startup. + [MemberNotNull(nameof(_endpoints))] + private void EnsureEndpointsInitialized() + { + lock (_lock) + { + if (_endpoints is null) + { + // Now that we're caching the _enpoints, we're responsible for keeping them up-to-date even if the caller + // hasn't started listening for changes themselves yet. + EnsureChangeTokenInitialized(); + + // Note: we can't use DataSourceDependentCache here because we also need to handle a list of change + // tokens, which is a complication most of our code doesn't have. + _endpoints = _dataSources.SelectMany(d => d.Endpoints).ToArray(); + } } } - private void HandleChange() + [MemberNotNull(nameof(_consumerChangeToken))] + private void EnsureChangeTokenInitialized() { lock (_lock) { - // Refresh the endpoints from datasource so that callbacks can get the latest endpoints - _endpoints = _dataSources.SelectMany(d => d.Endpoints).ToArray(); + if (_consumerChangeToken is null) + { + // This is our first time initializing the change token, so the collection has "changed" from nothing. + CreateChangeTokenUnsynchronized(collectionChanged: true); + } + } + } + + private void HandleChange(bool collectionChanged) + { + lock (_lock) + { + if (_disposed) + { + return; + } + + // Don't update endpoints if no one has read them yet. + if (_endpoints is not null) + { + // Refresh the endpoints from datasource so that callbacks can get the latest endpoints + _endpoints = _dataSources.SelectMany(d => d.Endpoints).ToArray(); + } // Prevent consumers from re-registering callback to inflight events as that can // cause a stackoverflow @@ -136,88 +184,48 @@ private void HandleChange() // 3. B executes some code in its callback, but needs to re-register callback // in the same callback var oldTokenSource = _cts; - var oldToken = _consumerChangeToken; - - CreateChangeToken(); - // Raise consumer callbacks. Any new callback registration would happen on the new token - // created in earlier step. - oldTokenSource.Cancel(); + // Don't create a new change token if no one is listening. + if (oldTokenSource is not null) + { + CreateChangeTokenUnsynchronized(collectionChanged); + // Raise consumer callbacks. Any new callback registration would happen on the new token + // created in earlier step. + oldTokenSource.Cancel(); + } } } - [MemberNotNull(nameof(_cts), nameof(_consumerChangeToken))] - private void CreateChangeToken() + [MemberNotNull(nameof(_consumerChangeToken))] + private void CreateChangeTokenUnsynchronized(bool collectionChanged) { _cts = new CancellationTokenSource(); _consumerChangeToken = new CancellationChangeToken(_cts.Token); - } - private string DebuggerDisplayString - { - get + if (collectionChanged) { - // Try using private variable '_endpoints' to avoid initialization - if (_endpoints == null) + if (_changeTokenRegistrations is null) { - return "No endpoints"; + _changeTokenRegistrations = new(); } - - var sb = new StringBuilder(); - foreach (var endpoint in _endpoints) + else { - if (endpoint is RouteEndpoint routeEndpoint) + foreach (var registration in _changeTokenRegistrations) { - var template = routeEndpoint.RoutePattern.RawText; - template = string.IsNullOrEmpty(template) ? "\"\"" : template; - sb.Append(template); - sb.Append(", Defaults: new { "); - sb.AppendJoin(", ", FormatValues(routeEndpoint.RoutePattern.Defaults)); - sb.Append(" }"); - var routeNameMetadata = routeEndpoint.Metadata.GetMetadata(); - sb.Append(", Route Name: "); - sb.Append(routeNameMetadata?.RouteName); - var routeValues = routeEndpoint.RoutePattern.RequiredValues; - if (routeValues.Count > 0) - { - sb.Append(", Required Values: new { "); - sb.AppendJoin(", ", FormatValues(routeValues)); - sb.Append(" }"); - } - sb.Append(", Order: "); - sb.Append(routeEndpoint.Order); - - var httpMethodMetadata = routeEndpoint.Metadata.GetMetadata(); - if (httpMethodMetadata != null) - { - sb.Append(", Http Methods: "); - sb.AppendJoin(", ", httpMethodMetadata.HttpMethods); - } - sb.Append(", Display Name: "); - sb.Append(routeEndpoint.DisplayName); - sb.AppendLine(); - } - else - { - sb.Append("Non-RouteEndpoint. DisplayName:"); - sb.AppendLine(endpoint.DisplayName); + registration.Dispose(); } + _changeTokenRegistrations.Clear(); } - return sb.ToString(); - static IEnumerable FormatValues(IEnumerable> values) + foreach (var dataSource in _dataSources) { - return values.Select( - kvp => - { - var value = "null"; - if (kvp.Value != null) - { - value = "\"" + kvp.Value.ToString() + "\""; - } - return kvp.Key + " = " + value; - }); + _changeTokenRegistrations.Add(ChangeToken.OnChange( + dataSource.GetChangeToken, + () => HandleChange(collectionChanged: false))); } } } + + // Use private variable '_endpoints' to avoid initialization + private string DebuggerDisplayString => GetDebuggerDisplayStringForEndpoints(_endpoints); } diff --git a/src/Http/Routing/src/DefaultEndpointDataSource.cs b/src/Http/Routing/src/DefaultEndpointDataSource.cs index 1d5e09039d14..940471d91fc0 100644 --- a/src/Http/Routing/src/DefaultEndpointDataSource.cs +++ b/src/Http/Routing/src/DefaultEndpointDataSource.cs @@ -1,6 +1,7 @@ -// Licensed to the .NET Foundation under one or more agreements. +// 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; using Microsoft.AspNetCore.Http; using Microsoft.Extensions.FileProviders; using Microsoft.Extensions.Primitives; @@ -10,6 +11,7 @@ namespace Microsoft.AspNetCore.Routing; /// /// Provides a collection of instances. /// +[DebuggerDisplay("{DebuggerDisplayString,nq}")] public sealed class DefaultEndpointDataSource : EndpointDataSource { private readonly IReadOnlyList _endpoints; @@ -53,4 +55,6 @@ public DefaultEndpointDataSource(IEnumerable endpoints) /// Returns a read-only collection of instances. /// public override IReadOnlyList Endpoints => _endpoints; + + private string DebuggerDisplayString => GetDebuggerDisplayStringForEndpoints(_endpoints); } diff --git a/src/Http/Routing/src/EndpointDataSource.cs b/src/Http/Routing/src/EndpointDataSource.cs index 71f0f4dd4148..c38f8fdfeb15 100644 --- a/src/Http/Routing/src/EndpointDataSource.cs +++ b/src/Http/Routing/src/EndpointDataSource.cs @@ -1,7 +1,9 @@ -// Licensed to the .NET Foundation under one or more agreements. +// Licensed to the .NET Foundation under one or more agreements. // The .NET Foundation licenses this file to you under the MIT license. +using System.Text; using Microsoft.AspNetCore.Http; +using Microsoft.AspNetCore.Routing.Patterns; using Microsoft.Extensions.Primitives; namespace Microsoft.AspNetCore.Routing; @@ -22,4 +24,149 @@ public abstract class EndpointDataSource /// Returns a read-only collection of instances. /// public abstract IReadOnlyList Endpoints { get; } + + /// + /// Get the instances for this given the specified . + /// + /// Details about how the returned instances should be grouped and a reference to application services. + /// + /// Returns a read-only collection of instances given the specified group and . + /// + public virtual IReadOnlyList GetGroupedEndpoints(RouteGroupContext context) + { + // Only evaluate Endpoints once per call. + var endpoints = Endpoints; + var wrappedEndpoints = new RouteEndpoint[endpoints.Count]; + + for (int i = 0; i < endpoints.Count; i++) + { + var endpoint = endpoints[i]; + + // Endpoint does not provide a RoutePattern but RouteEndpoint does. So it's impossible to apply a prefix for custom Endpoints. + // Supporting arbitrary Endpoints just to add group metadata would require changing the Endpoint type breaking any real scenario. + if (endpoint is not RouteEndpoint routeEndpoint) + { + throw new NotSupportedException(Resources.FormatMapGroup_CustomEndpointUnsupported(endpoint.GetType())); + } + + // Make the full route pattern visible to IEndpointConventionBuilder extension methods called on the group. + // This includes patterns from any parent groups. + var fullRoutePattern = RoutePatternFactory.Combine(context.Prefix, routeEndpoint.RoutePattern); + + // RequestDelegate can never be null on a RouteEndpoint. The nullability carries over from Endpoint. + var routeEndpointBuilder = new RouteEndpointBuilder(routeEndpoint.RequestDelegate!, fullRoutePattern, routeEndpoint.Order) + { + DisplayName = routeEndpoint.DisplayName, + ServiceProvider = context.ApplicationServices, + }; + + // Apply group conventions to each endpoint in the group at a lower precedent than metadata already on the endpoint. + foreach (var convention in context.Conventions) + { + convention(routeEndpointBuilder); + } + + // Any metadata already on the RouteEndpoint must have been applied directly to the endpoint or to a nested group. + // This makes the metadata more specific than what's being applied to this group. So add it after this group's conventions. + foreach (var metadata in routeEndpoint.Metadata) + { + routeEndpointBuilder.Metadata.Add(metadata); + } + + // The RoutePattern, RequestDelegate, Order and DisplayName can all be overridden by non-group-aware conventions. + // Unlike with metadata, if a convention is applied to a group that changes any of these, I would expect these + // to be overridden as there's reasonable way to merge these properties. + wrappedEndpoints[i] = (RouteEndpoint)routeEndpointBuilder.Build(); + } + + return wrappedEndpoints; + } + + // We don't implement DebuggerDisplay directly on the EndpointDataSource base type because this could have side effects. + // REVIEW: Should we just make this public/protected? Derived types that can provide endpoints without side effects might find this useful. + internal static string GetDebuggerDisplayStringForEndpoints(IReadOnlyList? endpoints) + { + if (endpoints is null) + { + return "No endpoints"; + } + + var sb = new StringBuilder(); + + foreach (var endpoint in endpoints) + { + if (endpoint is RouteEndpoint routeEndpoint) + { + var template = routeEndpoint.RoutePattern.RawText; + template = string.IsNullOrEmpty(template) ? "\"\"" : template; + sb.Append(template); + sb.Append(", Defaults: new { "); + FormatValues(sb, routeEndpoint.RoutePattern.Defaults); + sb.Append(" }"); + var routeNameMetadata = routeEndpoint.Metadata.GetMetadata(); + sb.Append(", Route Name: "); + sb.Append(routeNameMetadata?.RouteName); + var routeValues = routeEndpoint.RoutePattern.RequiredValues; + + if (routeValues.Count > 0) + { + sb.Append(", Required Values: new { "); + FormatValues(sb, routeValues); + sb.Append(" }"); + } + + sb.Append(", Order: "); + sb.Append(routeEndpoint.Order); + + var httpMethodMetadata = routeEndpoint.Metadata.GetMetadata(); + + if (httpMethodMetadata is not null) + { + sb.Append(", Http Methods: "); + sb.AppendJoin(", ", httpMethodMetadata.HttpMethods); + } + + sb.Append(", Display Name: "); + } + else + { + sb.Append("Non-RouteEndpoint. DisplayName: "); + } + + sb.AppendLine(endpoint.DisplayName); + } + + return sb.ToString(); + + static void FormatValues(StringBuilder sb, IEnumerable> values) + { + var isFirst = true; + + foreach (var (key, value) in values) + { + if (isFirst) + { + isFirst = false; + } + else + { + sb.Append(", "); + } + + sb.Append(key); + sb.Append('='); + + if (value is null) + { + sb.Append("null"); + } + else + { + sb.Append('\"'); + sb.Append(value); + sb.Append('\"'); + } + } + } + } } diff --git a/src/Http/Routing/src/EndpointRoutingMiddleware.cs b/src/Http/Routing/src/EndpointRoutingMiddleware.cs index ff2b382fdc85..fe336e869c95 100644 --- a/src/Http/Routing/src/EndpointRoutingMiddleware.cs +++ b/src/Http/Routing/src/EndpointRoutingMiddleware.cs @@ -26,6 +26,7 @@ public EndpointRoutingMiddleware( MatcherFactory matcherFactory, ILogger logger, IEndpointRouteBuilder endpointRouteBuilder, + EndpointDataSource rootCompositeEndpointDataSource, DiagnosticListener diagnosticListener, RequestDelegate next) { @@ -39,6 +40,9 @@ public EndpointRoutingMiddleware( _diagnosticListener = diagnosticListener ?? throw new ArgumentNullException(nameof(diagnosticListener)); _next = next ?? throw new ArgumentNullException(nameof(next)); + // rootCompositeEndpointDataSource is a constructor parameter only so it always gets disposed by DI. This ensures that any + // disposable EndpointDataSources also get disposed. _endpointDataSource is a component of rootCompositeEndpointDataSource. + _ = rootCompositeEndpointDataSource; _endpointDataSource = new CompositeEndpointDataSource(endpointRouteBuilder.DataSources); } diff --git a/src/Http/Routing/src/Patterns/RoutePatternFactory.cs b/src/Http/Routing/src/Patterns/RoutePatternFactory.cs index 1d1e8b13da56..608f732ac149 100644 --- a/src/Http/Routing/src/Patterns/RoutePatternFactory.cs +++ b/src/Http/Routing/src/Patterns/RoutePatternFactory.cs @@ -1084,46 +1084,8 @@ public static RoutePatternParameterPolicyReference ParameterPolicy(string parame return ParameterPolicyCore(parameterPolicy); } - internal static RoutePattern Combine(RoutePattern left, RoutePattern right) + internal static RoutePattern Combine(RoutePattern? left, RoutePattern right) { - static IReadOnlyList CombineLists( - IReadOnlyList leftList, - IReadOnlyList rightList, - Func>? checkDuplicates = null, - string? rawText = null) - { - if (leftList.Count is 0) - { - return rightList; - } - if (rightList.Count is 0) - { - return leftList; - } - - var combinedCount = leftList.Count + rightList.Count; - var combinedList = new List(combinedCount); - // If checkDuplicates is set, so is rawText so the right exception can be thrown from check. - var check = checkDuplicates?.Invoke(combinedCount, rawText!); - foreach (var item in leftList) - { - check?.Invoke(item); - combinedList.Add(item); - } - foreach (var item in rightList) - { - check?.Invoke(item); - combinedList.Add(item); - } - return combinedList; - } - - // Technically, the ParameterPolicies could probably be merged because it's a list, but it makes little sense to add policy - // for the same parameter in both the left and right part of the combined pattern. Defaults and Required values cannot be - // merged because the `TValue` is `object?`, but over-setting a Default or RequiredValue (which may not be in the parameter list) - // seems okay as long as the values are the same for a given key in both the left and right pattern. There's already similar logic - // in PatternCore for when defaults come from both the `defaults` and `segments` param. `requiredValues` cannot be defined in - // `segments` so there's no equivalent to merging these until now. static IReadOnlyDictionary CombineDictionaries( IReadOnlyDictionary leftDictionary, IReadOnlyDictionary rightDictionary, @@ -1146,16 +1108,15 @@ static IReadOnlyDictionary CombineDictionaries( } foreach (var (key, value) in rightDictionary) { - if (combinedDictionary.TryGetValue(key, out var leftValue)) - { - if (!Equals(leftValue, value)) - { - throw new InvalidOperationException(Resources.FormatMapGroup_RepeatedDictionaryEntry(rawText, dictionaryName, key)); - } - } - else + if (!combinedDictionary.TryAdd(key, value) && !Equals(combinedDictionary[key], value)) { - combinedDictionary.Add(key, value); + // Technically, the ParameterPolicies could probably be merged because it's a list, but it makes little sense to add policy + // for the same parameter in both the left and right part of the combined pattern. Defaults and Required values cannot be + // merged because the `TValue` is `object?`, but over-setting a Default or RequiredValue (which may not be in the parameter list) + // seems okay as long as the values are the same for a given key in both the left and right pattern. There's already similar logic + // in PatternCore for when defaults come from both the `defaults` and `segments` param. `requiredValues` cannot be defined in + // `segments` so there's no equivalent to merging these until now. + throw new InvalidOperationException(Resources.FormatMapGroup_RepeatedDictionaryEntry(rawText, dictionaryName, key)); } } return combinedDictionary; @@ -1174,6 +1135,11 @@ static Action CheckDuplicateParameters(int parameterC }; } + if (left is null) + { + return right; + } + var rawText = $"{left.RawText?.TrimEnd('/')}/{right.RawText?.TrimStart('/')}"; var parameters = CombineLists(left.Parameters, right.Parameters, CheckDuplicateParameters, rawText); @@ -1186,6 +1152,42 @@ static Action CheckDuplicateParameters(int parameterC return new RoutePattern(rawText, defaults, parameterPolicies, requiredValues, parameters, pathSegments); } + internal static IReadOnlyList CombineLists( + IReadOnlyList leftList, + IReadOnlyList rightList, + Func>? checkDuplicates = null, + string? rawText = null) + { + if (leftList.Count is 0) + { + return rightList; + } + if (rightList.Count is 0) + { + return leftList; + } + + var leftCount = leftList.Count; + var rightCount = rightList.Count; + var combinedList = new T[leftCount + rightCount]; + var check = checkDuplicates?.Invoke(combinedList.Length, rawText!); + + for (int i = 0; i < leftCount; i++) + { + var item = leftList[i]; + check?.Invoke(item); + combinedList[i] = item; + } + for (int i = 0; i < rightCount; i++) + { + var item = rightList[i]; + check?.Invoke(item); + combinedList[leftCount + i] = rightList[i]; + } + + return combinedList; + } + private static RoutePatternParameterPolicyReference ParameterPolicyCore(string parameterPolicy) { return new RoutePatternParameterPolicyReference(parameterPolicy); diff --git a/src/Http/Routing/src/PublicAPI.Unshipped.txt b/src/Http/Routing/src/PublicAPI.Unshipped.txt index fd64df4b9c77..5b7f48f1ce77 100644 --- a/src/Http/Routing/src/PublicAPI.Unshipped.txt +++ b/src/Http/Routing/src/PublicAPI.Unshipped.txt @@ -1,9 +1,15 @@ #nullable enable Microsoft.AspNetCore.Http.RouteHandlerFilterExtensions +Microsoft.AspNetCore.Routing.CompositeEndpointDataSource.Dispose() -> void Microsoft.AspNetCore.Routing.RouteGroupBuilder -Microsoft.AspNetCore.Routing.RouteGroupBuilder.GroupPrefix.get -> Microsoft.AspNetCore.Routing.Patterns.RoutePattern! +Microsoft.AspNetCore.Routing.RouteGroupContext +Microsoft.AspNetCore.Routing.RouteGroupContext.ApplicationServices.get -> System.IServiceProvider! +Microsoft.AspNetCore.Routing.RouteGroupContext.Conventions.get -> System.Collections.Generic.IReadOnlyList!>! +Microsoft.AspNetCore.Routing.RouteGroupContext.Prefix.get -> Microsoft.AspNetCore.Routing.Patterns.RoutePattern! +Microsoft.AspNetCore.Routing.RouteGroupContext.RouteGroupContext(Microsoft.AspNetCore.Routing.Patterns.RoutePattern! prefix, System.Collections.Generic.IReadOnlyList!>! conventions, System.IServiceProvider! applicationServices) -> void Microsoft.AspNetCore.Routing.RouteOptions.SetParameterPolicy(string! token, System.Type! type) -> void Microsoft.AspNetCore.Routing.RouteOptions.SetParameterPolicy(string! token) -> void +override Microsoft.AspNetCore.Routing.CompositeEndpointDataSource.GetGroupedEndpoints(Microsoft.AspNetCore.Routing.RouteGroupContext! context) -> System.Collections.Generic.IReadOnlyList! static Microsoft.AspNetCore.Builder.EndpointRouteBuilderExtensions.MapGroup(this Microsoft.AspNetCore.Routing.IEndpointRouteBuilder! endpoints, Microsoft.AspNetCore.Routing.Patterns.RoutePattern! prefix) -> Microsoft.AspNetCore.Routing.RouteGroupBuilder! static Microsoft.AspNetCore.Builder.EndpointRouteBuilderExtensions.MapGroup(this Microsoft.AspNetCore.Routing.IEndpointRouteBuilder! endpoints, string! prefix) -> Microsoft.AspNetCore.Routing.RouteGroupBuilder! static Microsoft.AspNetCore.Builder.EndpointRouteBuilderExtensions.MapPatch(this Microsoft.AspNetCore.Routing.IEndpointRouteBuilder! endpoints, string! pattern, System.Delegate! handler) -> Microsoft.AspNetCore.Builder.RouteHandlerBuilder! @@ -31,4 +37,5 @@ static Microsoft.AspNetCore.Routing.Patterns.RoutePatternFactory.Pattern(Microso static Microsoft.AspNetCore.Routing.Patterns.RoutePatternFactory.Pattern(Microsoft.AspNetCore.Routing.RouteValueDictionary? defaults, Microsoft.AspNetCore.Routing.RouteValueDictionary? parameterPolicies, params Microsoft.AspNetCore.Routing.Patterns.RoutePatternPathSegment![]! segments) -> Microsoft.AspNetCore.Routing.Patterns.RoutePattern! static Microsoft.AspNetCore.Routing.Patterns.RoutePatternFactory.Pattern(string? rawText, Microsoft.AspNetCore.Routing.RouteValueDictionary? defaults, Microsoft.AspNetCore.Routing.RouteValueDictionary? parameterPolicies, System.Collections.Generic.IEnumerable! segments) -> Microsoft.AspNetCore.Routing.Patterns.RoutePattern! static Microsoft.AspNetCore.Routing.Patterns.RoutePatternFactory.Pattern(string? rawText, Microsoft.AspNetCore.Routing.RouteValueDictionary? defaults, Microsoft.AspNetCore.Routing.RouteValueDictionary? parameterPolicies, params Microsoft.AspNetCore.Routing.Patterns.RoutePatternPathSegment![]! segments) -> Microsoft.AspNetCore.Routing.Patterns.RoutePattern! +virtual Microsoft.AspNetCore.Routing.EndpointDataSource.GetGroupedEndpoints(Microsoft.AspNetCore.Routing.RouteGroupContext! context) -> System.Collections.Generic.IReadOnlyList! virtual Microsoft.AspNetCore.Routing.Patterns.RoutePatternTransformer.SubstituteRequiredValues(Microsoft.AspNetCore.Routing.Patterns.RoutePattern! original, Microsoft.AspNetCore.Routing.RouteValueDictionary! requiredValues) -> Microsoft.AspNetCore.Routing.Patterns.RoutePattern? diff --git a/src/Http/Routing/src/RouteEndpointBuilder.cs b/src/Http/Routing/src/RouteEndpointBuilder.cs index add1f849a4a4..bec1185ce51a 100644 --- a/src/Http/Routing/src/RouteEndpointBuilder.cs +++ b/src/Http/Routing/src/RouteEndpointBuilder.cs @@ -18,7 +18,7 @@ public sealed class RouteEndpointBuilder : EndpointBuilder public RoutePattern RoutePattern { get; set; } /// - /// Gets or sets the order assigned to the endpoint. + /// Gets or sets the order assigned to the endpoint. /// public int Order { get; set; } @@ -38,24 +38,6 @@ public RouteEndpointBuilder( Order = order; } - /// - /// Constructs a new instance. - /// - /// The to use in URL matching. - /// The order assigned to the endpoint. - /// - /// This constructor allows the to be added to the - /// after construction but before - /// is invoked. - /// - internal RouteEndpointBuilder( - RoutePattern routePattern, - int order) - { - RoutePattern = routePattern; - Order = order; - } - /// public override Endpoint Build() { diff --git a/src/Http/Routing/src/RouteGroupBuilder.cs b/src/Http/Routing/src/RouteGroupBuilder.cs index 8ac12cafd18f..383dba67f46d 100644 --- a/src/Http/Routing/src/RouteGroupBuilder.cs +++ b/src/Http/Routing/src/RouteGroupBuilder.cs @@ -10,131 +10,96 @@ namespace Microsoft.AspNetCore.Routing; /// /// A builder for defining groups of endpoints with a common prefix that implements both the -/// and interfaces. This can be used to add endpoints with the given , +/// and interfaces. This can be used to add endpoints with the prefix defined by +/// /// and to customize those endpoints using conventions. /// public sealed class RouteGroupBuilder : IEndpointRouteBuilder, IEndpointConventionBuilder { private readonly IEndpointRouteBuilder _outerEndpointRouteBuilder; - private readonly RoutePattern _pattern; + private readonly RoutePattern _partialPrefix; private readonly List _dataSources = new(); private readonly List> _conventions = new(); - internal RouteGroupBuilder(IEndpointRouteBuilder outerEndpointRouteBuilder, RoutePattern pattern) + internal RouteGroupBuilder(IEndpointRouteBuilder outerEndpointRouteBuilder, RoutePattern partialPrefix) { _outerEndpointRouteBuilder = outerEndpointRouteBuilder; - _pattern = pattern; - - if (outerEndpointRouteBuilder is RouteGroupBuilder outerGroup) - { - GroupPrefix = RoutePatternFactory.Combine(outerGroup.GroupPrefix, pattern); - } - else - { - GroupPrefix = pattern; - } - - _outerEndpointRouteBuilder.DataSources.Add(new GroupDataSource(this)); + _partialPrefix = partialPrefix; + _outerEndpointRouteBuilder.DataSources.Add(new GroupEndpointDataSource(this)); } - /// - /// The prefixing all endpoints defined using this . - /// This accounts for nested groups and gives the full group prefix, not just the prefix supplied to the last call to - /// . - /// - public RoutePattern GroupPrefix { get; } - IServiceProvider IEndpointRouteBuilder.ServiceProvider => _outerEndpointRouteBuilder.ServiceProvider; IApplicationBuilder IEndpointRouteBuilder.CreateApplicationBuilder() => _outerEndpointRouteBuilder.CreateApplicationBuilder(); ICollection IEndpointRouteBuilder.DataSources => _dataSources; void IEndpointConventionBuilder.Add(Action convention) => _conventions.Add(convention); - private bool IsRoot => ReferenceEquals(GroupPrefix, _pattern); + // For testing + // This accounts for nested groups and gives the full group prefix, not just the prefix supplied to the last call to MapGroup + // If the _outerEndpointRouteBuilder is not a RouteGroupBuilder (like a wrapper around RouteGroupBuilder) this will not have + // the final prefix used in GroupDataSource.GetGroupedEndpoints() which is why this is not public even though it seems useful. + internal RoutePattern GroupPrefix => RoutePatternFactory.Combine((_outerEndpointRouteBuilder as RouteGroupBuilder)?.GroupPrefix, _partialPrefix); - private sealed class GroupDataSource : EndpointDataSource + private sealed class GroupEndpointDataSource : EndpointDataSource, IDisposable { - private readonly RouteGroupBuilder _groupRouteBuilder; + private readonly RouteGroupBuilder _routeGroupBuilder; + private CompositeEndpointDataSource? _compositeDataSource; - public GroupDataSource(RouteGroupBuilder groupRouteBuilder) + public GroupEndpointDataSource(RouteGroupBuilder groupRouteBuilder) { - _groupRouteBuilder = groupRouteBuilder; + _routeGroupBuilder = groupRouteBuilder; } - public override IReadOnlyList Endpoints + public override IReadOnlyList Endpoints => + GetGroupedEndpointsWithNullablePrefix(null, Array.Empty>(), _routeGroupBuilder._outerEndpointRouteBuilder.ServiceProvider); + + public override IReadOnlyList GetGroupedEndpoints(RouteGroupContext context) => + GetGroupedEndpointsWithNullablePrefix(context.Prefix, context.Conventions, context.ApplicationServices); + + public IReadOnlyList GetGroupedEndpointsWithNullablePrefix(RoutePattern? prefix, IReadOnlyList> conventions, IServiceProvider applicationServices) { - get + if (_routeGroupBuilder._dataSources.Count is 0) { - var list = new List(); - - foreach (var dataSource in _groupRouteBuilder._dataSources) - { - foreach (var endpoint in dataSource.Endpoints) - { - // Endpoint does not provide a RoutePattern but RouteEndpoint does. So it's impossible to apply a prefix for custom Endpoints. - // Supporting arbitrary Endpoints just to add group metadata would require changing the Endpoint type breaking any real scenario. - if (endpoint is not RouteEndpoint routeEndpoint) - { - throw new NotSupportedException(Resources.FormatMapGroup_CustomEndpointUnsupported(endpoint.GetType())); - } - - // Make the full route pattern visible to IEndpointConventionBuilder extension methods called on the group. - // This includes patterns from any parent groups. - var fullRoutePattern = RoutePatternFactory.Combine(_groupRouteBuilder.GroupPrefix, routeEndpoint.RoutePattern); - - // RequestDelegate can never be null on a RouteEndpoint. The nullability carries over from Endpoint. - var routeEndpointBuilder = new RouteEndpointBuilder(routeEndpoint.RequestDelegate!, fullRoutePattern, routeEndpoint.Order) - { - DisplayName = routeEndpoint.DisplayName, - ServiceProvider = _groupRouteBuilder._outerEndpointRouteBuilder.ServiceProvider, - }; - - // Apply group conventions to each endpoint in the group at a lower precedent than metadata already on the endpoint. - foreach (var convention in _groupRouteBuilder._conventions) - { - convention(routeEndpointBuilder); - } - - // If we supported mutating the route pattern via a group convention, RouteEndpointBuilder.RoutePattern would have - // to be the partialRoutePattern (below) instead of the fullRoutePattern (above) since that's all we can control. We cannot - // change a parent prefix. In order to allow to conventions to read the fullRoutePattern, we do not support mutation. - if (!ReferenceEquals(fullRoutePattern, routeEndpointBuilder.RoutePattern)) - { - throw new NotSupportedException(Resources.FormatMapGroup_ChangingRoutePatternUnsupported( - fullRoutePattern.RawText, routeEndpointBuilder.RoutePattern.RawText)); - } - - // Any metadata already on the RouteEndpoint must have been applied directly to the endpoint or to a nested group. - // This makes the metadata more specific than what's being applied to this group. So add it after this group's conventions. - // - // REVIEW: This means group conventions don't get visibility into endpoint-specific metadata nor the ability to override it. - // We should consider allowing group-aware conventions the ability to read and mutate this metadata in future releases. - foreach (var metadata in routeEndpoint.Metadata) - { - routeEndpointBuilder.Metadata.Add(metadata); - } - - // Use _pattern instead of GroupPrefix when we're calculating an intermediate RouteEndpoint. - var partialRoutePattern = _groupRouteBuilder.IsRoot - ? fullRoutePattern : RoutePatternFactory.Combine(_groupRouteBuilder._pattern, routeEndpoint.RoutePattern); - - // The RequestDelegate, Order and DisplayName can all be overridden by non-group-aware conventions. Unlike with metadata, - // if a convention is applied to a group that changes any of these, I would expect these to be overridden as there's no - // reasonable way to merge these properties. - list.Add(new RouteEndpoint( - // Again, RequestDelegate can never be null given a RouteEndpoint. - routeEndpointBuilder.RequestDelegate!, - partialRoutePattern, - routeEndpointBuilder.Order, - new(routeEndpointBuilder.Metadata), - routeEndpointBuilder.DisplayName)); - } - } - - return list; + return Array.Empty(); } + + var fullPrefix = RoutePatternFactory.Combine(prefix, _routeGroupBuilder._partialPrefix); + // Apply conventions passed in from the outer group first so their metadata is added earlier in the list at a lower precedent. + var combinedConventions = RoutePatternFactory.CombineLists(conventions, _routeGroupBuilder._conventions); + + if (_routeGroupBuilder._dataSources.Count is 1) + { + return _routeGroupBuilder._dataSources[0].GetGroupedEndpoints(new RouteGroupContext(fullPrefix, combinedConventions, applicationServices)); + } + + var groupedEndpoints = new List(); + + foreach (var dataSource in _routeGroupBuilder._dataSources) + { + groupedEndpoints.AddRange(dataSource.GetGroupedEndpoints(new RouteGroupContext(fullPrefix, combinedConventions, applicationServices))); + } + + return groupedEndpoints; + } + + public override IChangeToken GetChangeToken() + { + lock (_routeGroupBuilder._dataSources) + { + _compositeDataSource ??= new CompositeEndpointDataSource(_routeGroupBuilder._dataSources); + } + + return _compositeDataSource.GetChangeToken(); } - public override IChangeToken GetChangeToken() => new CompositeEndpointDataSource(_groupRouteBuilder._dataSources).GetChangeToken(); + public void Dispose() + { + _compositeDataSource?.Dispose(); + + foreach (var dataSource in _routeGroupBuilder._dataSources) + { + (dataSource as IDisposable)?.Dispose(); + } + } } } diff --git a/src/Http/Routing/src/RouteGroupContext.cs b/src/Http/Routing/src/RouteGroupContext.cs new file mode 100644 index 000000000000..0afbc08bcbb1 --- /dev/null +++ b/src/Http/Routing/src/RouteGroupContext.cs @@ -0,0 +1,46 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. + +using Microsoft.AspNetCore.Builder; +using Microsoft.AspNetCore.Routing.Patterns; + +namespace Microsoft.AspNetCore.Routing; + +/// +/// Represents the information accessible to . +/// +public sealed class RouteGroupContext +{ + /// + /// Constructs a new instance. + /// + /// The full group prefix. See . + /// All conventions added to a parent group. See . + /// Application services. See . + public RouteGroupContext(RoutePattern prefix, IReadOnlyList> conventions, IServiceProvider applicationServices) + { + ArgumentNullException.ThrowIfNull(prefix); + ArgumentNullException.ThrowIfNull(conventions); + ArgumentNullException.ThrowIfNull(applicationServices); + + Prefix = prefix; + Conventions = conventions; + ApplicationServices = applicationServices; + } + + /// + /// Gets the prefix for all the on all instances returned by the call to . + /// This accounts for nested groups and gives the full group prefix, not just the prefix supplied to the innermost call to . + /// + public RoutePattern Prefix { get; } + + /// + /// Gets all conventions added to a parent via . + /// + public IReadOnlyList> Conventions { get; } + + /// + /// Gets the instance used to access application services. + /// + public IServiceProvider ApplicationServices { get; } +} diff --git a/src/Http/Routing/src/RouteHandlerEndpointDataSource.cs b/src/Http/Routing/src/RouteHandlerEndpointDataSource.cs new file mode 100644 index 000000000000..c3c8296d6d1e --- /dev/null +++ b/src/Http/Routing/src/RouteHandlerEndpointDataSource.cs @@ -0,0 +1,194 @@ +// 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; +using System.Runtime.CompilerServices; +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; + +namespace Microsoft.AspNetCore.Routing; + +internal sealed class RouteHandlerEndpointDataSource : EndpointDataSource +{ + private readonly List _routeEntries = new(); + private readonly IServiceProvider _applicationServices; + private readonly bool _throwOnBadRequest; + + public RouteHandlerEndpointDataSource(IServiceProvider applicationServices, bool throwOnBadRequest) + { + _applicationServices = applicationServices; + _throwOnBadRequest = throwOnBadRequest; + } + + public List> AddEndpoint( + RoutePattern pattern, + Delegate routeHandler, + IEnumerable? initialEndpointMetadata, + bool disableInferFromBodyParameters) + { + RouteEntry entry = new() + { + RoutePattern = pattern, + RouteHandler = routeHandler, + InitialEndpointMetadata = initialEndpointMetadata, + DisableInferFromBodyParameters = disableInferFromBodyParameters, + Conventions = new() + }; + + _routeEntries.Add(entry); + + return entry.Conventions; + } + + public override IReadOnlyList Endpoints + { + get + { + var endpoints = new List(_routeEntries.Count); + foreach (var entry in _routeEntries) + { + endpoints.Add((RouteEndpoint)CreateRouteEndpointBuilder(entry).Build()); + } + return endpoints; + } + } + + public override IReadOnlyList GetGroupedEndpoints(RouteGroupContext context) + { + var endpoints = new List(_routeEntries.Count); + foreach (var entry in _routeEntries) + { + endpoints.Add((RouteEndpoint)CreateRouteEndpointBuilder(entry, context.Prefix, context.Conventions).Build()); + } + return endpoints; + } + + public override IChangeToken GetChangeToken() => NullChangeToken.Singleton; + + [UnconditionalSuppressMessage("Trimmer", "IL2026", + Justification = "We surface a RequireUnreferencedCode in the call to Map method adding this EndpointDataSource. " + + "The trimmer is unable to infer this.")] + private RouteEndpointBuilder CreateRouteEndpointBuilder(RouteEntry entry, RoutePattern? groupPrefix = null, IReadOnlyList>? groupConventions = null) + { + var pattern = RoutePatternFactory.Combine(groupPrefix, entry.RoutePattern); + var handler = entry.RouteHandler; + 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)) + { + endpointName ??= handler.Method.Name; + displayName = $"{displayName} => {endpointName}"; + } + + RequestDelegate? factoryCreatedRequestDelegate = null; + RequestDelegate redirectedRequestDelegate = context => + { + if (factoryCreatedRequestDelegate is null) + { + throw new InvalidOperationException($"{nameof(RequestDelegateFactory)} has not created the final {nameof(RequestDelegate)} yet."); + } + + return factoryCreatedRequestDelegate(context); + }; + + // The Map methods don't support customizing the order, so we always use the default of 0 unless a convention changes it later. + var builder = new RouteEndpointBuilder(redirectedRequestDelegate, pattern, order: 0) + { + DisplayName = displayName, + ServiceProvider = _applicationServices, + }; + + // We own EndpointBuilder.Metadata (in another assembly), so we know it's just a List. + var metadata = (List)builder.Metadata; + + // Add MethodInfo as first metadata item + builder.Metadata.Add(handler.Method); + + if (entry.InitialEndpointMetadata is not null) + { + metadata.AddRange(entry.InitialEndpointMetadata); + } + + // Apply group conventions before entry-specific conventions added to the RouteHandlerBuilder. + if (groupConventions is not null) + { + foreach (var groupConvention in groupConventions) + { + groupConvention(builder); + } + } + + // Add delegate attributes as metadata before programmatic conventions. + var attributes = handler.Method.GetCustomAttributes(); + if (attributes is not null) + { + metadata.AddRange(attributes); + } + + foreach (var entrySpecificConvention in entry.Conventions) + { + entrySpecificConvention(builder); + } + + // Let's see if any of the conventions added a filter before creating the RequestDelegate. + List>? routeHandlerFilterFactories = null; + + foreach (var item in builder.Metadata) + { + if (item is Func filter) + { + routeHandlerFilterFactories ??= new(); + routeHandlerFilterFactories.Add(filter); + } + } + + var routeParams = pattern.Parameters; + var routeParamNames = new List(routeParams.Count); + foreach (var parameter in routeParams) + { + routeParamNames.Add(parameter.Name); + } + + var factoryOptions = new RequestDelegateFactoryOptions + { + ServiceProvider = _applicationServices, + RouteParameterNames = routeParamNames, + ThrowOnBadRequest = _throwOnBadRequest, + DisableInferBodyFromParameters = entry.DisableInferFromBodyParameters, + InitialEndpointMetadata = metadata, + RouteHandlerFilterFactories = routeHandlerFilterFactories, + }; + + var requestDelegateResult = RequestDelegateFactory.Create(entry.RouteHandler, factoryOptions); + + // Give inferred metadata the lowest precedent. + metadata.InsertRange(0, requestDelegateResult.EndpointMetadata); + factoryCreatedRequestDelegate = requestDelegateResult.RequestDelegate; + + if (ReferenceEquals(requestDelegateResult.RequestDelegate, redirectedRequestDelegate)) + { + // 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. + builder.RequestDelegate = factoryCreatedRequestDelegate; + } + + return builder; + } + + private struct RouteEntry + { + public RoutePattern RoutePattern { get; init; } + public List> Conventions { get; init; } + public Delegate RouteHandler { get; init; } + public IEnumerable? InitialEndpointMetadata { get; init; } + public bool DisableInferFromBodyParameters { get; init; } + } +} diff --git a/src/Http/Routing/test/FunctionalTests/RouteHandlerTest.cs b/src/Http/Routing/test/FunctionalTests/RouteHandlerTest.cs index f3eff1389c3a..74e5568ffd9d 100644 --- a/src/Http/Routing/test/FunctionalTests/RouteHandlerTest.cs +++ b/src/Http/Routing/test/FunctionalTests/RouteHandlerTest.cs @@ -3,13 +3,16 @@ #nullable enable +using System.Net; using System.Net.Http.Json; using Microsoft.AspNetCore.Builder; using Microsoft.AspNetCore.Hosting; using Microsoft.AspNetCore.Http; using Microsoft.AspNetCore.TestHost; using Microsoft.Extensions.DependencyInjection; +using Microsoft.Extensions.FileProviders; using Microsoft.Extensions.Hosting; +using Microsoft.Extensions.Primitives; namespace Microsoft.AspNetCore.Routing.FunctionalTests; @@ -56,10 +59,72 @@ public async Task MapPost_FromBodyWorksWithJsonPayload() Assert.Equal(42, echoedTodo?.Id); } + [Fact] + public async Task CustomEndpointDataSource_IsDisposedIfResolved() + { + var testDisposeDataSource = new TestDisposeEndpointDataSource(); + var testGroupDisposeDataSource = new TestDisposeEndpointDataSource(); + + using var host = new HostBuilder() + .ConfigureWebHost(webHostBuilder => + { + webHostBuilder + .Configure(app => + { + app.UseRouting(); + app.UseEndpoints(b => + { + b.DataSources.Add(testDisposeDataSource); + + var group = b.MapGroup(""); + ((IEndpointRouteBuilder)group).DataSources.Add(testGroupDisposeDataSource); + }); + }) + .UseTestServer(); + }) + .ConfigureServices(services => + { + services.AddRouting(); + }) + .Build(); + + using var server = host.GetTestServer(); + await host.StartAsync(); + + // Make a request to ensure data sources are resolved. + var client = server.CreateClient(); + var response = await client.GetAsync("/"); + // We didn't define any endpoints. + Assert.Equal(HttpStatusCode.NotFound, response.StatusCode); + + Assert.False(testDisposeDataSource.IsDisposed); + Assert.False(testGroupDisposeDataSource.IsDisposed); + + await host.StopAsync(); + host.Dispose(); + + Assert.True(testDisposeDataSource.IsDisposed); + Assert.True(testGroupDisposeDataSource.IsDisposed); + } + private record Todo { public int Id { get; set; } public string Name { get; set; } = "Todo"; public bool IsComplete { get; set; } } + + private class TestDisposeEndpointDataSource : EndpointDataSource, IDisposable + { + public bool IsDisposed { get; private set; } + + public override IReadOnlyList Endpoints => Array.Empty(); + + public override IChangeToken GetChangeToken() => NullChangeToken.Singleton; + + public void Dispose() + { + IsDisposed = true; + } + } } diff --git a/src/Http/Routing/test/UnitTests/Builder/GroupTest.cs b/src/Http/Routing/test/UnitTests/Builder/GroupTest.cs index 1d77e8d47896..83830346d263 100644 --- a/src/Http/Routing/test/UnitTests/Builder/GroupTest.cs +++ b/src/Http/Routing/test/UnitTests/Builder/GroupTest.cs @@ -22,7 +22,7 @@ private EndpointDataSource GetEndpointDataSource(IEndpointRouteBuilder endpointR [Fact] public async Task Prefix_CanBeEmpty() { - var builder = new DefaultEndpointRouteBuilder(new ApplicationBuilder(serviceProvider: null!)); + var builder = new DefaultEndpointRouteBuilder(new ApplicationBuilder(EmptyServiceProvider.Instance)); var group = builder.MapGroup(""); Assert.Equal("", group.GroupPrefix.RawText); @@ -33,8 +33,6 @@ public async Task Prefix_CanBeEmpty() }); var dataSource = GetEndpointDataSource(builder); - - // Trigger Endpoint build by calling getter. var endpoint = Assert.Single(dataSource.Endpoints); var routeEndpoint = Assert.IsType(endpoint); @@ -57,7 +55,7 @@ public async Task Prefix_CanBeEmpty() [Fact] public async Task PrefixWithRouteParameter_CanBeUsed() { - var builder = new DefaultEndpointRouteBuilder(new ApplicationBuilder(serviceProvider: null!)); + var builder = new DefaultEndpointRouteBuilder(new ApplicationBuilder(EmptyServiceProvider.Instance)); var group = builder.MapGroup("/{org}"); Assert.Equal("/{org}", group.GroupPrefix.RawText); @@ -69,8 +67,6 @@ public async Task PrefixWithRouteParameter_CanBeUsed() }); var dataSource = GetEndpointDataSource(builder); - - // Trigger Endpoint build by calling getter. var endpoint = Assert.Single(dataSource.Endpoints); var routeEndpoint = Assert.IsType(endpoint); @@ -95,7 +91,7 @@ public async Task PrefixWithRouteParameter_CanBeUsed() [Fact] public async Task NestedPrefixWithRouteParameters_CanBeUsed() { - var builder = new DefaultEndpointRouteBuilder(new ApplicationBuilder(serviceProvider: null!)); + var builder = new DefaultEndpointRouteBuilder(new ApplicationBuilder(EmptyServiceProvider.Instance)); var group = builder.MapGroup("/{org}").MapGroup("/{id}"); Assert.Equal("/{org}/{id}", group.GroupPrefix.RawText); @@ -107,8 +103,6 @@ public async Task NestedPrefixWithRouteParameters_CanBeUsed() }); var dataSource = GetEndpointDataSource(builder); - - // Trigger Endpoint build by calling getter. var endpoint = Assert.Single(dataSource.Endpoints); var routeEndpoint = Assert.IsType(endpoint); @@ -133,9 +127,10 @@ public async Task NestedPrefixWithRouteParameters_CanBeUsed() [Fact] public void RepeatedRouteParameter_ThrowsRoutePatternException() { - var builder = new DefaultEndpointRouteBuilder(new ApplicationBuilder(serviceProvider: null!)); + var builder = new DefaultEndpointRouteBuilder(new ApplicationBuilder(EmptyServiceProvider.Instance)); + builder.MapGroup("/{ID}").MapGroup("/{id}").MapGet("/", () => { }); - var ex = Assert.Throws(() => builder.MapGroup("/{ID}").MapGroup("/{id}")); + var ex = Assert.Throws(() => builder.DataSources.Single().Endpoints); Assert.Equal("/{ID}/{id}", ex.Pattern); Assert.Equal("The route parameter name 'id' appears more than one time in the route template.", ex.Message); @@ -144,7 +139,7 @@ public void RepeatedRouteParameter_ThrowsRoutePatternException() [Fact] public void NullParameters_ThrowsArgumentNullException() { - var builder = new DefaultEndpointRouteBuilder(new ApplicationBuilder(serviceProvider: null!)); + var builder = new DefaultEndpointRouteBuilder(new ApplicationBuilder(EmptyServiceProvider.Instance)); var ex = Assert.Throws(() => builder.MapGroup((string)null!)); Assert.Equal("prefix", ex.ParamName); @@ -162,7 +157,7 @@ public void NullParameters_ThrowsArgumentNullException() [Fact] public void RoutePatternInConvention_IncludesFullGroupPrefix() { - var builder = new DefaultEndpointRouteBuilder(new ApplicationBuilder(serviceProvider: null!)); + var builder = new DefaultEndpointRouteBuilder(new ApplicationBuilder(EmptyServiceProvider.Instance)); var outer = builder.MapGroup("/outer"); var inner = outer.MapGroup("/inner"); @@ -212,7 +207,7 @@ public void ServiceProviderInConvention_IsSet() [Fact] public async Task BuildingEndpointInConvention_Works() { - var builder = new DefaultEndpointRouteBuilder(new ApplicationBuilder(serviceProvider: null!)); + var builder = new DefaultEndpointRouteBuilder(new ApplicationBuilder(EmptyServiceProvider.Instance)); var group = builder.MapGroup("/group"); var mapGetCalled = false; @@ -230,8 +225,6 @@ public async Task BuildingEndpointInConvention_Works() }); var dataSource = GetEndpointDataSource(builder); - - // Trigger Endpoint build by calling getter. var endpoint = Assert.Single(dataSource.Endpoints); var httpContext = new DefaultHttpContext(); @@ -247,9 +240,9 @@ public async Task BuildingEndpointInConvention_Works() } [Fact] - public void ModifyingRoutePatternInConvention_ThrowsNotSupportedException() + public void ModifyingRoutePatternInConvention_Works() { - var builder = new DefaultEndpointRouteBuilder(new ApplicationBuilder(serviceProvider: null!)); + var builder = new DefaultEndpointRouteBuilder(new ApplicationBuilder(EmptyServiceProvider.Instance)); var group = builder.MapGroup("/group"); group.MapGet("/foo", () => "Hello World!"); @@ -260,14 +253,16 @@ public void ModifyingRoutePatternInConvention_ThrowsNotSupportedException() }); var dataSource = GetEndpointDataSource(builder); - var ex = Assert.Throws(() => dataSource.Endpoints); - Assert.Equal("MapGroup does not support mutating RouteEndpointBuilder.RoutePattern from '/group/foo' to '/bar' via conventions.", ex.Message); + var endpoint = Assert.Single(dataSource.Endpoints); + var routeEndpoint = Assert.IsType(endpoint); + + Assert.Equal("/bar", routeEndpoint.RoutePattern.RawText); } [Fact] public async Task ChangingMostEndpointBuilderPropertiesInConvention_Works() { - var builder = new DefaultEndpointRouteBuilder(new ApplicationBuilder(serviceProvider: null!)); + var builder = new DefaultEndpointRouteBuilder(new ApplicationBuilder(EmptyServiceProvider.Instance)); var group = builder.MapGroup("/group"); var mapGetCalled = false; @@ -291,8 +286,6 @@ public async Task ChangingMostEndpointBuilderPropertiesInConvention_Works() }); var dataSource = GetEndpointDataSource(builder); - - // Trigger Endpoint build by calling getter. var endpoint = Assert.Single(dataSource.Endpoints); var httpContext = new DefaultHttpContext(); @@ -301,7 +294,7 @@ public async Task ChangingMostEndpointBuilderPropertiesInConvention_Works() Assert.False(mapGetCalled); Assert.True(replacementCalled); - Assert.Equal("Replaced!", endpoint.DisplayName); + Assert.Equal("HTTP: GET Replaced!", endpoint.DisplayName); var routeEndpoint = Assert.IsType(endpoint); Assert.Equal(42, routeEndpoint.Order); @@ -310,7 +303,7 @@ public async Task ChangingMostEndpointBuilderPropertiesInConvention_Works() [Fact] public void GivenNonRouteEndpoint_ThrowsNotSupportedException() { - var builder = new DefaultEndpointRouteBuilder(new ApplicationBuilder(serviceProvider: null!)); + var builder = new DefaultEndpointRouteBuilder(new ApplicationBuilder(EmptyServiceProvider.Instance)); var group = builder.MapGroup("/group"); ((IEndpointRouteBuilder)group).DataSources.Add(new TestCustomEndpintDataSource()); @@ -326,7 +319,7 @@ public void GivenNonRouteEndpoint_ThrowsNotSupportedException() [Fact] public void OuterGroupMetadata_AddedFirst() { - var builder = new DefaultEndpointRouteBuilder(new ApplicationBuilder(serviceProvider: null!)); + var builder = new DefaultEndpointRouteBuilder(new ApplicationBuilder(EmptyServiceProvider.Instance)); var outer = builder.MapGroup("/outer"); var inner = outer.MapGroup("/inner"); @@ -338,16 +331,13 @@ public void OuterGroupMetadata_AddedFirst() var dataSource = GetEndpointDataSource(builder); var endpoint = Assert.Single(dataSource.Endpoints); - Assert.True(endpoint.Metadata.Count >= 3); - Assert.Equal("/outer", endpoint.Metadata[0]); - Assert.Equal("/inner", endpoint.Metadata[1]); - Assert.Equal("/foo", endpoint.Metadata[^1]); + Assert.Equal(new[] { "/outer", "/inner", "/foo" }, endpoint.Metadata.GetOrderedMetadata()); } [Fact] public void MultipleEndpoints_AreSupported() { - var builder = new DefaultEndpointRouteBuilder(new ApplicationBuilder(serviceProvider: null!)); + var builder = new DefaultEndpointRouteBuilder(new ApplicationBuilder(EmptyServiceProvider.Instance)); var group = builder.MapGroup("/group"); group.MapGet("/foo", () => "foo"); @@ -361,20 +351,20 @@ public void MultipleEndpoints_AreSupported() { Assert.Equal("/group/foo", routeEndpoint.RoutePattern.RawText); Assert.True(routeEndpoint.Metadata.Count >= 1); - Assert.Equal("/group", routeEndpoint.Metadata[0]); + Assert.Equal("/group", routeEndpoint.Metadata.GetMetadata()); }, routeEndpoint => { Assert.Equal("/group/bar", routeEndpoint.RoutePattern.RawText); Assert.True(routeEndpoint.Metadata.Count >= 1); - Assert.Equal("/group", routeEndpoint.Metadata[0]); + Assert.Equal("/group", routeEndpoint.Metadata.GetMetadata()); }); } [Fact] public void DataSourceFiresChangeToken_WhenInnerDataSourceFiresChangeToken() { - var builder = new DefaultEndpointRouteBuilder(new ApplicationBuilder(serviceProvider: null!)); + var builder = new DefaultEndpointRouteBuilder(new ApplicationBuilder(EmptyServiceProvider.Instance)); var dynamicDataSource = new DynamicEndpointDataSource(); var group = builder.MapGroup("/group"); @@ -406,4 +396,11 @@ private sealed class TestCustomEndpintDataSource : EndpointDataSource public override IReadOnlyList Endpoints => new[] { new TestCustomEndpoint() }; public override IChangeToken GetChangeToken() => throw new NotImplementedException(); } + + private sealed class EmptyServiceProvider : IServiceProvider + { + public static EmptyServiceProvider Instance { get; } = new EmptyServiceProvider(); + + public object? GetService(Type serviceType) => null; + } } diff --git a/src/Http/Routing/test/UnitTests/Builder/RequestDelegateEndpointRouteBuilderExtensionsTest.cs b/src/Http/Routing/test/UnitTests/Builder/RequestDelegateEndpointRouteBuilderExtensionsTest.cs index 54f4b313ecc1..2420e424e3c4 100644 --- a/src/Http/Routing/test/UnitTests/Builder/RequestDelegateEndpointRouteBuilderExtensionsTest.cs +++ b/src/Http/Routing/test/UnitTests/Builder/RequestDelegateEndpointRouteBuilderExtensionsTest.cs @@ -286,7 +286,7 @@ private class AddsCustomEndpointMetadataResult : IEndpointMetadataProvider, IRes { public static void PopulateMetadata(EndpointMetadataContext context) { - context.EndpointMetadata.Add(new CustomEndpointMetadata { Source = MetadataSource.ReturnType }); + context.InferredMetadata.Add(new CustomEndpointMetadata { Source = MetadataSource.ReturnType }); } public Task ExecuteAsync(HttpContext httpContext) => throw new NotImplementedException(); @@ -303,7 +303,7 @@ public static void PopulateMetadata(EndpointParameterMetadataContext parameterCo public static void PopulateMetadata(EndpointMetadataContext context) { - context.EndpointMetadata.Add(new CustomEndpointMetadata { Source = MetadataSource.Parameter }); + context.InferredMetadata.Add(new CustomEndpointMetadata { Source = MetadataSource.Parameter }); } } diff --git a/src/Http/Routing/test/UnitTests/Builder/RouteHandlerEndpointRouteBuilderExtensionsTest.cs b/src/Http/Routing/test/UnitTests/Builder/RouteHandlerEndpointRouteBuilderExtensionsTest.cs index 65db53c1e756..d213399a90bc 100644 --- a/src/Http/Routing/test/UnitTests/Builder/RouteHandlerEndpointRouteBuilderExtensionsTest.cs +++ b/src/Http/Routing/test/UnitTests/Builder/RouteHandlerEndpointRouteBuilderExtensionsTest.cs @@ -1002,21 +1002,28 @@ public void RequestDelegateFactory_ProvidesAppServiceProvider_ToFilterFactory() [Fact] public void RouteHandlerContext_ThrowsArgumentNullException_ForMethodInfo() { - Assert.Throws("methodInfo", () => new RouteHandlerContext(null!, new(), new ServiceCollection().BuildServiceProvider())); + Assert.Throws("methodInfo", () => new RouteHandlerContext(null!, new List(), new List(), new ServiceCollection().BuildServiceProvider())); } [Fact] public void RouteHandlerContext_ThrowsArgumentNullException_ForEndpointMetadata() { var handler = () => { }; - Assert.Throws("endpointMetadata", () => new RouteHandlerContext(handler.Method, null!, new ServiceCollection().BuildServiceProvider())); + Assert.Throws("endpointMetadata", () => new RouteHandlerContext(handler.Method, null!, new List(), new ServiceCollection().BuildServiceProvider())); + } + + [Fact] + public void RouteHandlerContext_ThrowsArgumentNullException_ForInferredMetadata() + { + var handler = () => { }; + Assert.Throws("inferedMetadata", () => new RouteHandlerContext(handler.Method, new List(), null!, new ServiceCollection().BuildServiceProvider())); } [Fact] public void RouteHandlerContext_ThrowsArgumentNullException_ForApplicationServices() { var handler = () => { }; - Assert.Throws("applicationServices", () => new RouteHandlerContext(handler.Method, new(), null!)); + Assert.Throws("applicationServices", () => new RouteHandlerContext(handler.Method, new List(), new List(), null!)); } class MyService { } diff --git a/src/Http/Routing/test/UnitTests/CompositeEndpointDataSourceTest.cs b/src/Http/Routing/test/UnitTests/CompositeEndpointDataSourceTest.cs index 7bf5401dee70..6e71dc9ef319 100644 --- a/src/Http/Routing/test/UnitTests/CompositeEndpointDataSourceTest.cs +++ b/src/Http/Routing/test/UnitTests/CompositeEndpointDataSourceTest.cs @@ -1,9 +1,13 @@ // Licensed to the .NET Foundation under one or more agreements. // The .NET Foundation licenses this file to you under the MIT license. +using System.Collections.ObjectModel; +using Microsoft.AspNetCore.Builder; using Microsoft.AspNetCore.Http; using Microsoft.AspNetCore.Routing.Patterns; using Microsoft.AspNetCore.Routing.TestObjects; +using Microsoft.Extensions.DependencyInjection; +using Microsoft.Extensions.FileProviders; using Microsoft.Extensions.Primitives; namespace Microsoft.AspNetCore.Routing; @@ -27,6 +31,25 @@ public void CreatesShallowCopyOf_ListOfEndpoints() Assert.Equal(endpoints, dataSource.Endpoints); } + [Fact] + public void CreatesShallowCopyOf_ListOfGroupedEndpoints() + { + var endpoint1 = CreateEndpoint("/a"); + var endpoint2 = CreateEndpoint("/b"); + var dataSource = new TestGroupDataSource(new RouteEndpoint[] { endpoint1, endpoint2 }); + var compositeDataSource = new CompositeEndpointDataSource(new[] { dataSource }); + + var prefix = RoutePatternFactory.Parse("/"); + var conventions = Array.Empty>(); + var applicationServices = new ServiceCollection().BuildServiceProvider(); + + var groupedEndpoints = compositeDataSource.GetGroupedEndpoints(new RouteGroupContext(prefix, conventions, applicationServices)); + + var resolvedGroupEndpoints = Assert.Single(dataSource.ResolvedGroupedEndpoints); + Assert.NotSame(groupedEndpoints, resolvedGroupEndpoints); + Assert.Equal(groupedEndpoints, resolvedGroupEndpoints); + } + [Fact] public void Endpoints_ReturnsAllEndpoints_FromMultipleDataSources() { @@ -41,7 +64,7 @@ public void Endpoints_ReturnsAllEndpoints_FromMultipleDataSources() new DefaultEndpointDataSource(new Endpoint[] { endpoint1, endpoint2 }), new DefaultEndpointDataSource(new Endpoint[] { endpoint3, endpoint4 }), new DefaultEndpointDataSource(new Endpoint[] { endpoint5 }), - }); + }); // Act var endpoints = compositeDataSource.Endpoints; @@ -141,6 +164,117 @@ public void ConsumerChangeToken_IsRefreshed_WhenDataSourceCallbackFires() Assert.False(token.HasChanged); } + [Fact] + public void ConsumerChangeToken_IsRefreshed_WhenNewDataSourceCallbackFires() + { + var endpoint1 = CreateEndpoint("/a"); + var dataSource1 = new DynamicEndpointDataSource(endpoint1); + var observableCollection = new ObservableCollection { dataSource1 }; + var compositeDataSource = new CompositeEndpointDataSource(observableCollection); + + var changeToken1 = compositeDataSource.GetChangeToken(); + var token = Assert.IsType(changeToken1); + Assert.False(token.HasChanged); + + var endpoint2 = CreateEndpoint("/b"); + + // Update ObservableCollection with a new DynamicEndpointDataSource + var dataSource2 = new DynamicEndpointDataSource(endpoint2); + observableCollection.Add(dataSource2); + + Assert.True(changeToken1.HasChanged); + var changeToken2 = compositeDataSource.GetChangeToken(); + Assert.NotSame(changeToken2, changeToken1); + token = Assert.IsType(changeToken2); + Assert.False(token.HasChanged); + + // Update the newly added DynamicEndpointDataSource + var endpoint3 = CreateEndpoint("/c"); + dataSource2.AddEndpoint(endpoint3); + + Assert.True(changeToken2.HasChanged); + var changeToken3 = compositeDataSource.GetChangeToken(); + Assert.NotSame(changeToken3, changeToken2); + Assert.NotSame(changeToken3, changeToken1); + token = Assert.IsType(changeToken3); + Assert.False(token.HasChanged); + } + + [Fact] + public void ConsumerChangeToken_IsNotRefreshed_AfterDisposal() + { + var endpoint1 = CreateEndpoint("/a"); + var dataSource1 = new DynamicEndpointDataSource(endpoint1); + var observableCollection = new ObservableCollection { dataSource1 }; + var compositeDataSource = new CompositeEndpointDataSource(observableCollection); + + var changeToken1 = compositeDataSource.GetChangeToken(); + var token = Assert.IsType(changeToken1); + Assert.False(token.HasChanged); + + var endpoint2 = CreateEndpoint("/b"); + + // Update DynamicEndpointDatasource + dataSource1.AddEndpoint(endpoint2); + + Assert.True(changeToken1.HasChanged); + var changeToken2 = compositeDataSource.GetChangeToken(); + Assert.NotSame(changeToken2, changeToken1); + token = Assert.IsType(changeToken2); + Assert.False(token.HasChanged); + + // Update ObservableCollection + var endpoint3 = CreateEndpoint("/c"); + var datasource2 = new DynamicEndpointDataSource(endpoint3); + observableCollection.Add(datasource2); + + Assert.True(changeToken2.HasChanged); + var changeToken3 = compositeDataSource.GetChangeToken(); + Assert.NotSame(changeToken3, changeToken2); + Assert.NotSame(changeToken3, changeToken1); + token = Assert.IsType(changeToken3); + Assert.False(token.HasChanged); + + compositeDataSource.Dispose(); + + // Update DynamicEndpointDatasource and ObservableCollection after disposing CompositeEndpointDataSource. + var endpoint4 = CreateEndpoint("/d"); + dataSource1.AddEndpoint(endpoint4); + var endpoint5 = CreateEndpoint("/d"); + var datasource3 = new DynamicEndpointDataSource(endpoint5); + observableCollection.Add(datasource3); + + // Token is not changed since the CompositeEndpointDataSource was disposed prior to the last endpoint being added. + Assert.False(changeToken3.HasChanged); + } + + [Fact] + public void GetGroupedEndpoints_ForwardedToChildDataSources() + { + var endpoint = CreateEndpoint("/a"); + var dataSource = new TestGroupDataSource(new RouteEndpoint[] { endpoint }); + var compositeDataSource = new CompositeEndpointDataSource(new[] { dataSource }); + + var prefix = RoutePatternFactory.Parse("/prefix"); + var applicationServices = new ServiceCollection().BuildServiceProvider(); + var metadata = new EndpointNameMetadata("name"); + var conventions = new Action[] + { + b => b.Metadata.Add(metadata), + }; + + var context = new RouteGroupContext(prefix, conventions, applicationServices); + var groupedEndpoints = compositeDataSource.GetGroupedEndpoints(context); + + var receivedContext = Assert.Single(dataSource.ReceivedRouteGroupContexts); + Assert.Same(context, receivedContext); + + var resolvedEndpoint = Assert.Single(groupedEndpoints); + Assert.Equal("/prefix/a", resolvedEndpoint.RoutePattern.RawText); + var resolvedMetadata = Assert.Single(resolvedEndpoint.Metadata); + Assert.Same(metadata, resolvedMetadata); + } + private RouteEndpoint CreateEndpoint( string template, object defaults = null, @@ -154,4 +288,25 @@ private RouteEndpoint CreateEndpoint( EndpointMetadataCollection.Empty, null); } + + private class TestGroupDataSource : EndpointDataSource + { + public TestGroupDataSource(params RouteEndpoint[] endpoints) => Endpoints = endpoints; + + public override IReadOnlyList Endpoints { get; } + + public List ReceivedRouteGroupContexts { get; } = new(); + + public List> ResolvedGroupedEndpoints { get; } = new(); + + public override IReadOnlyList GetGroupedEndpoints(RouteGroupContext context) + { + ReceivedRouteGroupContexts.Add(context); + var resolved = base.GetGroupedEndpoints(context); + ResolvedGroupedEndpoints.Add(resolved); + return resolved; + } + + public override IChangeToken GetChangeToken() => NullChangeToken.Singleton; + } } diff --git a/src/Http/Routing/test/UnitTests/EndpointRoutingMiddlewareTest.cs b/src/Http/Routing/test/UnitTests/EndpointRoutingMiddlewareTest.cs index 6f6bc9b45f89..ba1506102c63 100644 --- a/src/Http/Routing/test/UnitTests/EndpointRoutingMiddlewareTest.cs +++ b/src/Http/Routing/test/UnitTests/EndpointRoutingMiddlewareTest.cs @@ -180,6 +180,7 @@ private EndpointRoutingMiddleware CreateMiddleware( matcherFactory, logger, new DefaultEndpointRouteBuilder(Mock.Of()), + new DefaultEndpointDataSource(), listener, next); From d3f0bfd7a8653efdc50e451a0fb539299822a840 Mon Sep 17 00:00:00 2001 From: Stephen Halter Date: Wed, 15 Jun 2022 13:45:40 -0700 Subject: [PATCH 02/21] TBuilderify AddFilter and WithOpenApi --- .../src/Extensions/EndpointBuilder.cs | 8 +- .../src/PublicAPI.Unshipped.txt | 9 +- .../src/RouteHandlerContext.cs | 17 +-- .../src/EndpointMetadataContext.cs | 12 +- .../src/EndpointParameterMetadataContext.cs | 10 +- .../src/PublicAPI.Unshipped.txt | 12 +- .../src/RequestDelegateFactory.cs | 39 +++-- .../src/RequestDelegateFactoryOptions.cs | 2 +- .../test/RequestDelegateFactoryTests.cs | 100 ++++++------- .../Builder/EndpointRouteBuilderExtensions.cs | 4 +- .../src/Builder/RouteHandlerBuilder.cs | 6 +- .../Builder/RouteHandlerFilterExtensions.cs | 75 ++++++++-- .../src/CompositeEndpointDataSource.cs | 6 +- .../src/DefaultEndpointConventionBuilder.cs | 2 +- src/Http/Routing/src/EndpointDataSource.cs | 4 +- src/Http/Routing/src/PublicAPI.Unshipped.txt | 14 +- src/Http/Routing/src/Resources.resx | 6 + ...taSource.cs => RouteEndpointDataSource.cs} | 55 +++++-- src/Http/Routing/src/RouteFilterMetadata.cs | 11 ++ src/Http/Routing/src/RouteGroupBuilder.cs | 8 +- .../test/UnitTests/Builder/GroupTest.cs | 3 +- ...egateEndpointRouteBuilderExtensionsTest.cs | 70 +++++---- ...ndlerEndpointRouteBuilderExtensionsTest.cs | 134 +++++++++--------- .../CompositeEndpointDataSourceTest.cs | 8 +- .../OpenApiRouteHandlerBuilderExtensions.cs | 77 ++++++---- src/OpenApi/src/PublicAPI.Unshipped.txt | 4 +- ...penApiRouteHandlerBuilderExtensionTests.cs | 90 +++++++++++- 27 files changed, 493 insertions(+), 293 deletions(-) rename src/Http/Routing/src/{RouteHandlerEndpointDataSource.cs => RouteEndpointDataSource.cs} (76%) create mode 100644 src/Http/Routing/src/RouteFilterMetadata.cs diff --git a/src/Http/Http.Abstractions/src/Extensions/EndpointBuilder.cs b/src/Http/Http.Abstractions/src/Extensions/EndpointBuilder.cs index 873a1dbe8bae..e9c00fce40f0 100644 --- a/src/Http/Http.Abstractions/src/Extensions/EndpointBuilder.cs +++ b/src/Http/Http.Abstractions/src/Extensions/EndpointBuilder.cs @@ -28,11 +28,17 @@ public abstract class EndpointBuilder /// /// Gets the associated with the endpoint. /// - public IServiceProvider? ServiceProvider { get; set; } + public IServiceProvider ApplicationServices { get; set; } = EmptyServiceProvicer.Instance; /// /// Creates an instance of from the . /// /// The created . public abstract Endpoint Build(); + + private sealed class EmptyServiceProvicer : IServiceProvider + { + public static EmptyServiceProvicer Instance { get; } = new EmptyServiceProvicer(); + public object? GetService(Type serviceType) => null; + } } diff --git a/src/Http/Http.Abstractions/src/PublicAPI.Unshipped.txt b/src/Http/Http.Abstractions/src/PublicAPI.Unshipped.txt index 2f5fcd95247f..bccb33497006 100644 --- a/src/Http/Http.Abstractions/src/PublicAPI.Unshipped.txt +++ b/src/Http/Http.Abstractions/src/PublicAPI.Unshipped.txt @@ -1,8 +1,8 @@ #nullable enable *REMOVED*abstract Microsoft.AspNetCore.Http.HttpResponse.ContentType.get -> string! *REMOVED*Microsoft.AspNetCore.Http.EndpointMetadataCollection.Enumerator.Current.get -> object? -Microsoft.AspNetCore.Builder.EndpointBuilder.ServiceProvider.get -> System.IServiceProvider? -Microsoft.AspNetCore.Builder.EndpointBuilder.ServiceProvider.set -> void +Microsoft.AspNetCore.Builder.EndpointBuilder.ApplicationServices.get -> System.IServiceProvider! +Microsoft.AspNetCore.Builder.EndpointBuilder.ApplicationServices.set -> void Microsoft.AspNetCore.Http.AsParametersAttribute Microsoft.AspNetCore.Http.AsParametersAttribute.AsParametersAttribute() -> void Microsoft.AspNetCore.Http.DefaultRouteHandlerInvocationContext @@ -16,10 +16,9 @@ Microsoft.AspNetCore.Http.Metadata.IRequestSizeLimitMetadata Microsoft.AspNetCore.Http.Metadata.IRequestSizeLimitMetadata.MaxRequestBodySize.get -> long? Microsoft.AspNetCore.Http.RouteHandlerContext Microsoft.AspNetCore.Http.RouteHandlerContext.ApplicationServices.get -> System.IServiceProvider! -Microsoft.AspNetCore.Http.RouteHandlerContext.EndpointMetadata.get -> System.Collections.Generic.IReadOnlyList! -Microsoft.AspNetCore.Http.RouteHandlerContext.InferredMetadata.get -> System.Collections.Generic.IList! +Microsoft.AspNetCore.Http.RouteHandlerContext.EndpointMetadata.get -> System.Collections.Generic.IList! Microsoft.AspNetCore.Http.RouteHandlerContext.MethodInfo.get -> System.Reflection.MethodInfo! -Microsoft.AspNetCore.Http.RouteHandlerContext.RouteHandlerContext(System.Reflection.MethodInfo! methodInfo, System.Collections.Generic.IReadOnlyList! endpointMetadata, System.Collections.Generic.IList! inferredMetadata, System.IServiceProvider! applicationServices) -> void +Microsoft.AspNetCore.Http.RouteHandlerContext.RouteHandlerContext(System.Reflection.MethodInfo! methodInfo, System.Collections.Generic.IList! endpointMetadata, System.IServiceProvider! applicationServices) -> void Microsoft.AspNetCore.Http.RouteHandlerFilterDelegate Microsoft.AspNetCore.Http.RouteHandlerInvocationContext Microsoft.AspNetCore.Http.RouteHandlerInvocationContext.RouteHandlerInvocationContext() -> void diff --git a/src/Http/Http.Abstractions/src/RouteHandlerContext.cs b/src/Http/Http.Abstractions/src/RouteHandlerContext.cs index 417b02a57130..f5c1f044955f 100644 --- a/src/Http/Http.Abstractions/src/RouteHandlerContext.cs +++ b/src/Http/Http.Abstractions/src/RouteHandlerContext.cs @@ -2,6 +2,7 @@ // The .NET Foundation licenses this file to you under the MIT license. using System.Reflection; +using Microsoft.AspNetCore.Builder; namespace Microsoft.AspNetCore.Http; @@ -15,19 +16,16 @@ public sealed class RouteHandlerContext /// Creates a new instance of the . /// /// The associated with the route handler of the current request. - /// The associated with the endpoint the filter is targeting. - /// The mutable metadata inferred about current endpoint. This will come before in . + /// The associated with the endpoint the filter is targeting. /// The instance used to access the application services. - public RouteHandlerContext(MethodInfo methodInfo, IReadOnlyList endpointMetadata, IList inferredMetadata, IServiceProvider applicationServices) + public RouteHandlerContext(MethodInfo methodInfo, IList endpointMetadata, IServiceProvider applicationServices) { ArgumentNullException.ThrowIfNull(methodInfo); ArgumentNullException.ThrowIfNull(endpointMetadata); - ArgumentNullException.ThrowIfNull(inferredMetadata); ArgumentNullException.ThrowIfNull(applicationServices); MethodInfo = methodInfo; EndpointMetadata = endpointMetadata; - InferredMetadata = inferredMetadata; ApplicationServices = applicationServices; } @@ -37,14 +35,9 @@ public RouteHandlerContext(MethodInfo methodInfo, IReadOnlyList endpoint public MethodInfo MethodInfo { get; } /// - /// The read-only metadata already applied to the current endpoint. + /// The associated with the current endpoint. /// - public IReadOnlyList EndpointMetadata { get; } - - /// - /// The mutable metadata inferred about current endpoint. This will come before in . - /// - public IList InferredMetadata { get; } + public IList EndpointMetadata { get; } /// /// Gets the instance used to access application services. diff --git a/src/Http/Http.Extensions/src/EndpointMetadataContext.cs b/src/Http/Http.Extensions/src/EndpointMetadataContext.cs index 1cf021800b56..caf96455684c 100644 --- a/src/Http/Http.Extensions/src/EndpointMetadataContext.cs +++ b/src/Http/Http.Extensions/src/EndpointMetadataContext.cs @@ -15,18 +15,15 @@ public sealed class EndpointMetadataContext /// /// The of the route handler delegate of the endpoint being created. /// The list of objects that will be added to the metadata of the endpoint. - /// The mutable metadata inferred about current endpoint. This will come before in . /// The instance used to access application services. - public EndpointMetadataContext(MethodInfo method, IReadOnlyList endpointMetadata, IList inferredMetadata, IServiceProvider applicationServices) + public EndpointMetadataContext(MethodInfo method, IList endpointMetadata, IServiceProvider applicationServices) { ArgumentNullException.ThrowIfNull(method); ArgumentNullException.ThrowIfNull(endpointMetadata); - ArgumentNullException.ThrowIfNull(inferredMetadata); ArgumentNullException.ThrowIfNull(applicationServices); Method = method; EndpointMetadata = endpointMetadata; - InferredMetadata = inferredMetadata; ApplicationServices = applicationServices; } @@ -38,12 +35,7 @@ public EndpointMetadataContext(MethodInfo method, IReadOnlyList endpoint /// /// Gets the list of objects that will be added to the metadata of the endpoint. /// - public IReadOnlyList EndpointMetadata { get; } - - /// - /// The mutable metadata inferred about current endpoint. This will come before in . - /// - public IList InferredMetadata { get; } + public IList EndpointMetadata { get; } /// /// Gets the instance used to access application services. diff --git a/src/Http/Http.Extensions/src/EndpointParameterMetadataContext.cs b/src/Http/Http.Extensions/src/EndpointParameterMetadataContext.cs index e58c2c85ac2d..202839cc8080 100644 --- a/src/Http/Http.Extensions/src/EndpointParameterMetadataContext.cs +++ b/src/Http/Http.Extensions/src/EndpointParameterMetadataContext.cs @@ -15,18 +15,15 @@ public sealed class EndpointParameterMetadataContext /// /// The parameter of the route handler delegate of the endpoint being created. /// The list of objects that will be added to the metadata of the endpoint. - /// The mutable metadata inferred about current endpoint. This will come before in . /// The instance used to access application services. - public EndpointParameterMetadataContext(ParameterInfo parameter, IList endpointMetadata, IList inferredMetadata, IServiceProvider applicationServices) + public EndpointParameterMetadataContext(ParameterInfo parameter, IList endpointMetadata, IServiceProvider applicationServices) { ArgumentNullException.ThrowIfNull(parameter); ArgumentNullException.ThrowIfNull(endpointMetadata); - ArgumentNullException.ThrowIfNull(inferredMetadata); ArgumentNullException.ThrowIfNull(applicationServices); Parameter = parameter; EndpointMetadata = endpointMetadata; - InferredMetadata = inferredMetadata; ApplicationServices = applicationServices; } @@ -40,11 +37,6 @@ public EndpointParameterMetadataContext(ParameterInfo parameter, IList e /// public IList EndpointMetadata { get; } - /// - /// The mutable metadata inferred about current endpoint. This will come before in . - /// - public IList InferredMetadata { get; } - /// /// Gets the instance used to access application services. /// diff --git a/src/Http/Http.Extensions/src/PublicAPI.Unshipped.txt b/src/Http/Http.Extensions/src/PublicAPI.Unshipped.txt index 2e0280c58d5e..5b917e83875b 100644 --- a/src/Http/Http.Extensions/src/PublicAPI.Unshipped.txt +++ b/src/Http/Http.Extensions/src/PublicAPI.Unshipped.txt @@ -1,22 +1,20 @@ #nullable enable Microsoft.AspNetCore.Http.Metadata.EndpointMetadataContext Microsoft.AspNetCore.Http.Metadata.EndpointMetadataContext.ApplicationServices.get -> System.IServiceProvider! -Microsoft.AspNetCore.Http.Metadata.EndpointMetadataContext.EndpointMetadata.get -> System.Collections.Generic.IReadOnlyList! -Microsoft.AspNetCore.Http.Metadata.EndpointMetadataContext.EndpointMetadataContext(System.Reflection.MethodInfo! method, System.Collections.Generic.IReadOnlyList! endpointMetadata, System.Collections.Generic.IList! inferredMetadata, System.IServiceProvider! applicationServices) -> void -Microsoft.AspNetCore.Http.Metadata.EndpointMetadataContext.InferredMetadata.get -> System.Collections.Generic.IList! +Microsoft.AspNetCore.Http.Metadata.EndpointMetadataContext.EndpointMetadataContext(System.Reflection.MethodInfo! method, System.Collections.Generic.IList! endpointMetadata, System.IServiceProvider! applicationServices) -> void +Microsoft.AspNetCore.Http.Metadata.EndpointMetadataContext.EndpointMetadata.get -> System.Collections.Generic.IList! Microsoft.AspNetCore.Http.Metadata.EndpointMetadataContext.Method.get -> System.Reflection.MethodInfo! Microsoft.AspNetCore.Http.Metadata.EndpointParameterMetadataContext Microsoft.AspNetCore.Http.Metadata.EndpointParameterMetadataContext.ApplicationServices.get -> System.IServiceProvider! -Microsoft.AspNetCore.Http.Metadata.EndpointParameterMetadataContext.EndpointParameterMetadataContext(System.Reflection.ParameterInfo! parameter, System.Collections.Generic.IList! endpointMetadata, System.Collections.Generic.IList! inferredMetadata, System.IServiceProvider! applicationServices) -> void +Microsoft.AspNetCore.Http.Metadata.EndpointParameterMetadataContext.EndpointParameterMetadataContext(System.Reflection.ParameterInfo! parameter, System.Collections.Generic.IList! endpointMetadata, System.IServiceProvider! applicationServices) -> void Microsoft.AspNetCore.Http.Metadata.EndpointParameterMetadataContext.EndpointMetadata.get -> System.Collections.Generic.IList! -Microsoft.AspNetCore.Http.Metadata.EndpointParameterMetadataContext.InferredMetadata.get -> System.Collections.Generic.IList! Microsoft.AspNetCore.Http.Metadata.EndpointParameterMetadataContext.Parameter.get -> System.Reflection.ParameterInfo! Microsoft.AspNetCore.Http.Metadata.IEndpointMetadataProvider Microsoft.AspNetCore.Http.Metadata.IEndpointMetadataProvider.PopulateMetadata(Microsoft.AspNetCore.Http.Metadata.EndpointMetadataContext! context) -> void Microsoft.AspNetCore.Http.Metadata.IEndpointParameterMetadataProvider Microsoft.AspNetCore.Http.Metadata.IEndpointParameterMetadataProvider.PopulateMetadata(Microsoft.AspNetCore.Http.Metadata.EndpointParameterMetadataContext! parameterContext) -> void -Microsoft.AspNetCore.Http.RequestDelegateFactoryOptions.InitialEndpointMetadata.get -> System.Collections.Generic.IEnumerable? -Microsoft.AspNetCore.Http.RequestDelegateFactoryOptions.InitialEndpointMetadata.init -> void +Microsoft.AspNetCore.Http.RequestDelegateFactoryOptions.EndpointMetadata.get -> System.Collections.Generic.IList? +Microsoft.AspNetCore.Http.RequestDelegateFactoryOptions.EndpointMetadata.init -> void Microsoft.AspNetCore.Http.RequestDelegateFactoryOptions.RouteHandlerFilterFactories.get -> System.Collections.Generic.IReadOnlyList!>? Microsoft.AspNetCore.Http.RequestDelegateFactoryOptions.RouteHandlerFilterFactories.init -> void Microsoft.Extensions.DependencyInjection.RouteHandlerJsonServiceExtensions diff --git a/src/Http/Http.Extensions/src/RequestDelegateFactory.cs b/src/Http/Http.Extensions/src/RequestDelegateFactory.cs index f595e4bb4412..cd52459f203b 100644 --- a/src/Http/Http.Extensions/src/RequestDelegateFactory.cs +++ b/src/Http/Http.Extensions/src/RequestDelegateFactory.cs @@ -133,7 +133,7 @@ public static RequestDelegateResult Create(Delegate handler, RequestDelegateFact var targetableRequestDelegate = CreateTargetableRequestDelegate(handler.Method, targetExpression, factoryContext, targetFactory); - return new RequestDelegateResult(httpContext => targetableRequestDelegate(handler.Target, httpContext), factoryContext.Metadata); + return new RequestDelegateResult(httpContext => targetableRequestDelegate(handler.Target, httpContext), GetMetadataList(factoryContext.Metadata)); } /// @@ -165,7 +165,7 @@ public static RequestDelegateResult Create(MethodInfo methodInfo, Func untargetableRequestDelegate(null, httpContext), factoryContext.Metadata); + return new RequestDelegateResult(httpContext => untargetableRequestDelegate(null, httpContext), GetMetadataList(factoryContext.Metadata)); } targetFactory = context => Activator.CreateInstance(methodInfo.DeclaringType)!; @@ -174,7 +174,7 @@ public static RequestDelegateResult Create(MethodInfo methodInfo, Func targetFactory(context)); - return new RequestDelegateResult(httpContext => targetableRequestDelegate(targetFactory(httpContext), httpContext), factoryContext.Metadata); + return new RequestDelegateResult(httpContext => targetableRequestDelegate(targetFactory(httpContext), httpContext), GetMetadataList(factoryContext.Metadata)); } private static FactoryContext CreateFactoryContext(RequestDelegateFactoryOptions? options) @@ -186,10 +186,21 @@ private static FactoryContext CreateFactoryContext(RequestDelegateFactoryOptions RouteParameters = options?.RouteParameterNames?.ToList(), ThrowOnBadRequest = options?.ThrowOnBadRequest ?? false, DisableInferredFromBody = options?.DisableInferBodyFromParameters ?? false, - Filters = options?.RouteHandlerFilterFactories?.ToList() + Filters = options?.RouteHandlerFilterFactories?.ToList(), + Metadata = options?.EndpointMetadata ?? new List(), }; } + private static List GetMetadataList(IList metadata) + { + if (metadata is List normalList) + { + return normalList; + } + + return new List(metadata); + } + private static Func CreateTargetableRequestDelegate(MethodInfo methodInfo, Expression? targetExpression, FactoryContext factoryContext, Expression>? targetFactory = null) { // Non void return type @@ -292,7 +303,7 @@ targetExpression is null FilterContextExpr).Compile(); var routeHandlerContext = new RouteHandlerContext( methodInfo, - new EndpointMetadataCollection(factoryContext.Metadata), + factoryContext.Metadata, factoryContext.ServiceProvider ?? EmptyServiceProvider.Instance); for (var i = factoryContext.Filters.Count - 1; i >= 0; i--) @@ -420,7 +431,7 @@ private static Expression CreateRouteHandlerInvocationContextBase(FactoryContext return fallbackConstruction; } - private static void AddTypeProvidedMetadata(MethodInfo methodInfo, List metadata, IServiceProvider? services, ReadOnlySpan parameters) + private static void AddTypeProvidedMetadata(MethodInfo methodInfo, IList metadata, IServiceProvider? services, ReadOnlySpan parameters) { object?[]? invokeArgs = null; @@ -1635,6 +1646,14 @@ private static Expression BindParameterFromBindAsync(ParameterInfo parameter, Fa return Expression.Convert(boundValueExpr, parameter.ParameterType); } + private static void InsertInferredAcceptsMetadata(FactoryContext factoryContext, Type type, string[] contentTypes) + { + // Unlike most metadata that will probably to be added by filters or metadata providers, we insert the automatically-inferred AcceptsMetadata + // to the beginning of the metadata to give it the lowest precedence. It really doesn't makes sense for this metadata to be overridden but + // we're preserving the old behavior here out of an abundance of caution. + factoryContext.Metadata.Insert(0, new AcceptsMetadata(type, factoryContext.AllowEmptyRequestBody, contentTypes)); + } + private static Expression BindParameterFromFormFiles( ParameterInfo parameter, FactoryContext factoryContext) @@ -1649,7 +1668,7 @@ private static Expression BindParameterFromFormFiles( // Do not duplicate the metadata if there are multiple form parameters if (!factoryContext.ReadForm) { - factoryContext.Metadata.Add(new AcceptsMetadata(parameter.ParameterType, factoryContext.AllowEmptyRequestBody, FormFileContentType)); + InsertInferredAcceptsMetadata(factoryContext, parameter.ParameterType, FormFileContentType); } factoryContext.ReadForm = true; @@ -1673,7 +1692,7 @@ private static Expression BindParameterFromFormFile( // Do not duplicate the metadata if there are multiple form parameters if (!factoryContext.ReadForm) { - factoryContext.Metadata.Add(new AcceptsMetadata(parameter.ParameterType, factoryContext.AllowEmptyRequestBody, FormFileContentType)); + InsertInferredAcceptsMetadata(factoryContext, parameter.ParameterType, FormFileContentType); } factoryContext.ReadForm = true; @@ -1703,7 +1722,7 @@ private static Expression BindParameterFromBody(ParameterInfo parameter, bool al factoryContext.JsonRequestBodyParameter = parameter; factoryContext.AllowEmptyRequestBody = allowEmpty || isOptional; - factoryContext.Metadata.Add(new AcceptsMetadata(parameter.ParameterType, factoryContext.AllowEmptyRequestBody, DefaultAcceptsContentType)); + InsertInferredAcceptsMetadata(factoryContext, parameter.ParameterType, DefaultAcceptsContentType); if (!factoryContext.AllowEmptyRequestBody) { @@ -2053,7 +2072,7 @@ private sealed class FactoryContext public bool HasMultipleBodyParameters { get; set; } public bool HasInferredBody { get; set; } - public List Metadata { get; internal set; } = new(); + public IList Metadata { get; init; } = default!; public NullabilityInfoContext NullabilityContext { get; } = new(); diff --git a/src/Http/Http.Extensions/src/RequestDelegateFactoryOptions.cs b/src/Http/Http.Extensions/src/RequestDelegateFactoryOptions.cs index 9b367dfcfcf0..5e3cd4fd571d 100644 --- a/src/Http/Http.Extensions/src/RequestDelegateFactoryOptions.cs +++ b/src/Http/Http.Extensions/src/RequestDelegateFactoryOptions.cs @@ -46,5 +46,5 @@ public sealed class RequestDelegateFactoryOptions /// or , i.e. this metadata will be less specific than any /// inferred by the call to . /// - public IEnumerable? InitialEndpointMetadata { get; init; } + public IList? EndpointMetadata { get; init; } } diff --git a/src/Http/Http.Extensions/test/RequestDelegateFactoryTests.cs b/src/Http/Http.Extensions/test/RequestDelegateFactoryTests.cs index b5812ced546f..9539c722266b 100644 --- a/src/Http/Http.Extensions/test/RequestDelegateFactoryTests.cs +++ b/src/Http/Http.Extensions/test/RequestDelegateFactoryTests.cs @@ -5669,7 +5669,7 @@ string HelloName() } [Fact] - public void Create_AddsDelegateMethodInfo_AsMetadata() + public void Create_DoesNotAddDelegateMethodInfo_AsMetadata() { // Arrange var @delegate = () => "Hello"; @@ -5678,27 +5678,32 @@ public void Create_AddsDelegateMethodInfo_AsMetadata() var result = RequestDelegateFactory.Create(@delegate); // Assert - Assert.Contains(result.EndpointMetadata, m => m is MethodInfo); + // RouteHandlerEndpointDataSource adds the MethodInfo as the first item in RouteHandlerOptions.EndointMetadata + Assert.Empty(result.EndpointMetadata); } [Fact] - public void Create_AddsDelegateMethodInfo_AsFirstMetadata() + public void Create_DoesNotAddAnythingBefore_ThePassedInEndpointMetadata() { // Arrange - var @delegate = (AddsCustomParameterMetadata param1) => "Hello"; + var @delegate = (AddsCustomParameterMetadataBindable param1) => { }; var customMetadata = new CustomEndpointMetadata(); - var options = new RequestDelegateFactoryOptions { InitialEndpointMetadata = new[] { customMetadata } }; + var options = new RequestDelegateFactoryOptions { EndpointMetadata = new List { customMetadata } }; // Act var result = RequestDelegateFactory.Create(@delegate, options); // Assert - var firstMetadata = result.EndpointMetadata[0]; - Assert.IsAssignableFrom(firstMetadata); + // RouteHandlerEndpointDataSource adds things like the MethodInfo, HttpMethodMetadata and attributes to RouteHandlerOptions.EndointMetadata, + // but we just specified our CustomEndpointMetadata in this test. + Assert.Collection(result.EndpointMetadata, + m => Assert.Same(customMetadata, m), + m => Assert.True(m is ParameterNameMetadata { Name: "param1" }), + m => Assert.True(m is CustomEndpointMetadata { Source: MetadataSource.Parameter })); } [Fact] - public void Create_AddsDelegateAttributes_AsMetadata() + public void Create_DoesNotAddDelegateAttributes_AsMetadata() { // Arrange var @delegate = [Attribute1, Attribute2] () => { }; @@ -5707,23 +5712,8 @@ public void Create_AddsDelegateAttributes_AsMetadata() var result = RequestDelegateFactory.Create(@delegate); // Assert - Assert.Contains(result.EndpointMetadata, m => m is Attribute1); - Assert.Contains(result.EndpointMetadata, m => m is Attribute2); - } - - [Fact] - public void Create_AddsDelegateAttributes_AsLastMetadata() - { - // Arrange - var @delegate = [Attribute1] (AddsCustomParameterMetadata param1) => { }; - var options = new RequestDelegateFactoryOptions { InitialEndpointMetadata = new[] { new CustomEndpointMetadata() } }; - - // Act - var result = RequestDelegateFactory.Create(@delegate, options); - - // Assert - var lastMetadata = result.EndpointMetadata.Last(); - Assert.IsAssignableFrom(lastMetadata); + // RouteHandlerEndpointDataSource adds the attributes to RouteHandlerOptions.EndointMetadata + Assert.Empty(result.EndpointMetadata); } [Fact] @@ -5799,7 +5789,7 @@ public void Create_CombinesDefaultMetadata_AndMetadataFromReturnTypesImplementin var @delegate = () => new CountsDefaultEndpointMetadataResult(); var options = new RequestDelegateFactoryOptions { - InitialEndpointMetadata = new List + EndpointMetadata = new List { new CustomEndpointMetadata { Source = MetadataSource.Caller } } @@ -5810,8 +5800,8 @@ public void Create_CombinesDefaultMetadata_AndMetadataFromReturnTypesImplementin // Assert Assert.Contains(result.EndpointMetadata, m => m is CustomEndpointMetadata { Source: MetadataSource.Caller }); - // Expecting '2' as only MethodInfo and initial metadata will be in the metadata list when this metadata item is added - Assert.Contains(result.EndpointMetadata, m => m is DefaultMetadataCountMetadata { Count: 2 }); + // Expecting '1' because only initial metadata will be in the metadata list when this metadata item is added + Assert.Contains(result.EndpointMetadata, m => m is DefaultMetadataCountMetadata { Count: 1 }); } [Fact] @@ -5821,7 +5811,7 @@ public void Create_CombinesDefaultMetadata_AndMetadataFromTaskWrappedReturnTypes var @delegate = () => Task.FromResult(new CountsDefaultEndpointMetadataResult()); var options = new RequestDelegateFactoryOptions { - InitialEndpointMetadata = new List + EndpointMetadata = new List { new CustomEndpointMetadata { Source = MetadataSource.Caller } } @@ -5832,18 +5822,18 @@ public void Create_CombinesDefaultMetadata_AndMetadataFromTaskWrappedReturnTypes // Assert Assert.Contains(result.EndpointMetadata, m => m is CustomEndpointMetadata { Source: MetadataSource.Caller }); - // Expecting '2' as only MethodInfo and initial metadata will be in the metadata list when this metadata item is added - Assert.Contains(result.EndpointMetadata, m => m is DefaultMetadataCountMetadata { Count: 2 }); + // Expecting '1' because only initial metadata will be in the metadata list when this metadata item is added + Assert.Contains(result.EndpointMetadata, m => m is DefaultMetadataCountMetadata { Count: 1 }); } [Fact] - public void Create_DoesNotCombineDefaultMetadata_AndMetadataFromValueTaskWrappedReturnTypesImplementingIEndpointMetadataProvider() + public void Create_CombinesDefaultMetadata_AndMetadataFromValueTaskWrappedReturnTypesImplementingIEndpointMetadataProvider() { // Arrange var @delegate = () => ValueTask.FromResult(new CountsDefaultEndpointMetadataResult()); var options = new RequestDelegateFactoryOptions { - InitialEndpointMetadata = new List + EndpointMetadata = new List { new CustomEndpointMetadata { Source = MetadataSource.Caller } } @@ -5853,9 +5843,9 @@ public void Create_DoesNotCombineDefaultMetadata_AndMetadataFromValueTaskWrapped var result = RequestDelegateFactory.Create(@delegate, options); // Assert - Assert.DoesNotContain(result.EndpointMetadata, m => m is CustomEndpointMetadata { Source: MetadataSource.Caller }); - // Expecting '2' as only MethodInfo and initial metadata will be in the metadata list when this metadata item is added - Assert.Contains(result.EndpointMetadata, m => m is DefaultMetadataCountMetadata { Count: 2 }); + Assert.Contains(result.EndpointMetadata, m => m is CustomEndpointMetadata { Source: MetadataSource.Caller }); + // Expecting '1' because only initial metadata will be in the metadata list when this metadata item is added + Assert.Contains(result.EndpointMetadata, m => m is DefaultMetadataCountMetadata { Count: 1 }); } [Fact] @@ -5865,7 +5855,7 @@ public void Create_CombinesDefaultMetadata_AndMetadataFromParameterTypesImplemen var @delegate = (AddsCustomParameterMetadata param1) => "Hello"; var options = new RequestDelegateFactoryOptions { - InitialEndpointMetadata = new List + EndpointMetadata = new List { new CustomEndpointMetadata { Source = MetadataSource.Caller } } @@ -5886,7 +5876,7 @@ public void Create_CombinesDefaultMetadata_AndMetadataFromParameterTypesImplemen var @delegate = (AddsCustomParameterMetadata param1) => "Hello"; var options = new RequestDelegateFactoryOptions { - InitialEndpointMetadata = new List + EndpointMetadata = new List { new CustomEndpointMetadata { Source = MetadataSource.Caller } } @@ -5907,7 +5897,7 @@ public void Create_CombinesPropertiesAsParameterMetadata_AndTopLevelParameter() var @delegate = ([AsParameters] AddsCustomParameterMetadata param1) => new CountsDefaultEndpointMetadataResult(); var options = new RequestDelegateFactoryOptions { - InitialEndpointMetadata = new List + EndpointMetadata = new List { new CustomEndpointMetadata { Source = MetadataSource.Caller } } @@ -5930,7 +5920,7 @@ public void Create_CombinesAllMetadata_InCorrectOrder() var @delegate = [Attribute1, Attribute2] (AddsCustomParameterMetadata param1) => new CountsDefaultEndpointMetadataResult(); var options = new RequestDelegateFactoryOptions { - InitialEndpointMetadata = new List + EndpointMetadata = new List { new CustomEndpointMetadata { Source = MetadataSource.Caller } } @@ -5941,22 +5931,16 @@ public void Create_CombinesAllMetadata_InCorrectOrder() // Assert Assert.Collection(result.EndpointMetadata, - // MethodInfo - m => Assert.IsAssignableFrom(m), - // Initial metadata from RequestDelegateFactoryOptions.InitialEndpointMetadata - m => Assert.True(m is CustomEndpointMetadata { Source: MetadataSource.Caller }), // Inferred AcceptsMetadata from RDF for complex type m => Assert.True(m is AcceptsMetadata am && am.RequestType == typeof(AddsCustomParameterMetadata)), + // Initial metadata from RequestDelegateFactoryOptions.InitialEndpointMetadata + m => Assert.True(m is CustomEndpointMetadata { Source: MetadataSource.Caller }), // Metadata provided by parameters implementing IEndpointParameterMetadataProvider m => Assert.True(m is ParameterNameMetadata { Name: "param1" }), // Metadata provided by parameters implementing IEndpointMetadataProvider m => Assert.True(m is CustomEndpointMetadata { Source: MetadataSource.Parameter }), // Metadata provided by return type implementing IEndpointMetadataProvider - m => Assert.True(m is DefaultMetadataCountMetadata { Count: 5 }), - // Handler delegate attributes - m => Assert.IsAssignableFrom(m), // NullableContextAttribute - m => Assert.IsType(m), - m => Assert.IsType(m)); + m => Assert.True(m is DefaultMetadataCountMetadata { Count: 4 })); } [Fact] @@ -6079,7 +6063,7 @@ public static void PopulateMetadata(EndpointMetadataContext context) { if (context.ApplicationServices?.GetRequiredService() is { } metadataService) { - context.InferredMetadata.Add(metadataService); + context.EndpointMetadata.Add(metadataService); } } @@ -6095,7 +6079,7 @@ public static void PopulateMetadata(EndpointMetadataContext context) { if (context.ApplicationServices?.GetRequiredService() is { } metadataService) { - context.InferredMetadata.Add(metadataService); + context.EndpointMetadata.Add(metadataService); } } } @@ -6112,7 +6096,7 @@ private class AddsCustomEndpointMetadataResult : IEndpointMetadataProvider, IRes { public static void PopulateMetadata(EndpointMetadataContext context) { - context.InferredMetadata.Add(new CustomEndpointMetadata { Source = MetadataSource.ReturnType }); + context.EndpointMetadata?.Add(new CustomEndpointMetadata { Source = MetadataSource.ReturnType }); } public Task ExecuteAsync(HttpContext httpContext) => throw new NotImplementedException(); @@ -6133,7 +6117,7 @@ private class CountsDefaultEndpointMetadataResult : IEndpointMetadataProvider, I public static void PopulateMetadata(EndpointMetadataContext context) { var defaultMetadataCount = context.EndpointMetadata?.Count; - context.InferredMetadata.Add(new DefaultMetadataCountMetadata { Count = defaultMetadataCount ?? 0 }); + context.EndpointMetadata?.Add(new DefaultMetadataCountMetadata { Count = defaultMetadataCount ?? 0 }); } public Task ExecuteAsync(HttpContext httpContext) => throw new NotImplementedException(); @@ -6168,7 +6152,7 @@ public static void PopulateMetadata(EndpointMetadataContext parameterContext) var metadata = parameterContext.EndpointMetadata[i]; if (metadata is IAcceptsMetadata) { - parameterContext.InferredMetadata.RemoveAt(i); + parameterContext.EndpointMetadata.RemoveAt(i); } } } @@ -6186,7 +6170,7 @@ public static void PopulateMetadata(EndpointMetadataContext context) var metadata = context.EndpointMetadata[i]; if (metadata is IAcceptsMetadata) { - context.InferredMetadata.RemoveAt(i); + context.EndpointMetadata.RemoveAt(i); } } } @@ -6204,7 +6188,7 @@ public static void PopulateMetadata(EndpointParameterMetadataContext parameterCo public static void PopulateMetadata(EndpointMetadataContext context) { - context.InferredMetadata.Add(new CustomEndpointMetadata { Source = MetadataSource.Property }); + context.EndpointMetadata?.Add(new CustomEndpointMetadata { Source = MetadataSource.Property }); } } @@ -6219,7 +6203,7 @@ public static void PopulateMetadata(EndpointParameterMetadataContext parameterCo public static void PopulateMetadata(EndpointMetadataContext context) { - context.InferredMetadata.Add(new CustomEndpointMetadata { Source = MetadataSource.Parameter }); + context.EndpointMetadata?.Add(new CustomEndpointMetadata { Source = MetadataSource.Parameter }); } } @@ -6234,7 +6218,7 @@ public static void PopulateMetadata(EndpointParameterMetadataContext parameterCo public static void PopulateMetadata(EndpointMetadataContext context) { - context.InferredMetadata.Add(new CustomEndpointMetadata { Source = MetadataSource.Parameter }); + context.EndpointMetadata?.Add(new CustomEndpointMetadata { Source = MetadataSource.Parameter }); } } diff --git a/src/Http/Routing/src/Builder/EndpointRouteBuilderExtensions.cs b/src/Http/Routing/src/Builder/EndpointRouteBuilderExtensions.cs index d2a41a446fbf..f4c56b7b7391 100644 --- a/src/Http/Routing/src/Builder/EndpointRouteBuilderExtensions.cs +++ b/src/Http/Routing/src/Builder/EndpointRouteBuilderExtensions.cs @@ -473,13 +473,13 @@ private static RouteHandlerBuilder Map( ArgumentNullException.ThrowIfNull(pattern); ArgumentNullException.ThrowIfNull(handler); - var dataSource = endpoints.DataSources.OfType().FirstOrDefault(); + var dataSource = endpoints.DataSources.OfType().FirstOrDefault(); if (dataSource is null) { var routeHandlerOptions = endpoints.ServiceProvider.GetService>(); var throwOnBadRequest = routeHandlerOptions?.Value.ThrowOnBadRequest ?? false; - dataSource = new RouteHandlerEndpointDataSource(endpoints.ServiceProvider, throwOnBadRequest); + dataSource = new RouteEndpointDataSource(endpoints.ServiceProvider, throwOnBadRequest); endpoints.DataSources.Add(dataSource); } diff --git a/src/Http/Routing/src/Builder/RouteHandlerBuilder.cs b/src/Http/Routing/src/Builder/RouteHandlerBuilder.cs index 07df57bfaedd..261189b7dd77 100644 --- a/src/Http/Routing/src/Builder/RouteHandlerBuilder.cs +++ b/src/Http/Routing/src/Builder/RouteHandlerBuilder.cs @@ -11,14 +11,14 @@ namespace Microsoft.AspNetCore.Builder; public sealed class RouteHandlerBuilder : IEndpointConventionBuilder { private readonly IEnumerable? _endpointConventionBuilders; - private readonly List>? _conventions; + private readonly ICollection>? _conventions; /// /// Instantiates a new given a single /// . /// - /// The convention list returned from . - internal RouteHandlerBuilder(List> conventions) + /// The convention list returned from . + internal RouteHandlerBuilder(ICollection> conventions) { _conventions = conventions; } diff --git a/src/Http/Routing/src/Builder/RouteHandlerFilterExtensions.cs b/src/Http/Routing/src/Builder/RouteHandlerFilterExtensions.cs index b4cb78b7a1df..8c03051e828b 100644 --- a/src/Http/Routing/src/Builder/RouteHandlerFilterExtensions.cs +++ b/src/Http/Routing/src/Builder/RouteHandlerFilterExtensions.cs @@ -3,6 +3,7 @@ using System.Diagnostics.CodeAnalysis; using Microsoft.AspNetCore.Builder; +using Microsoft.AspNetCore.Routing; using Microsoft.Extensions.DependencyInjection; namespace Microsoft.AspNetCore.Http; @@ -18,19 +19,19 @@ public static class RouteHandlerFilterExtensions /// The . /// The to register. /// A that can be used to further customize the route handler. - public static RouteHandlerBuilder AddFilter(this RouteHandlerBuilder builder, IRouteHandlerFilter filter) - { - builder.AddFilter((routeHandlerContext, next) => (context) => filter.InvokeAsync(context, next)); - return builder; - } + public static TBuilder AddRouteHandlerFilter(this TBuilder builder, IRouteHandlerFilter filter) where TBuilder : IEndpointConventionBuilder => + builder.AddRouteHandlerFilter((routeHandlerContext, next) => (context) => filter.InvokeAsync(context, next)); /// /// Registers a filter of type onto the route handler. /// + /// The type of the to configure. /// The type of the to register. /// The . /// A that can be used to further customize the route handler. - public static RouteHandlerBuilder AddFilter<[DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.PublicConstructors)] TFilterType>(this RouteHandlerBuilder builder) where TFilterType : IRouteHandlerFilter + public static TBuilder AddRouteHandlerFilter(this TBuilder builder) + where TBuilder : IEndpointConventionBuilder + where TFilterType : IRouteHandlerFilter { // We call `CreateFactory` twice here since the `CreateFactory` API does not support optional arguments. // See https://github.com/dotnet/runtime/issues/67309 for more info. @@ -44,7 +45,7 @@ public static RouteHandlerBuilder AddFilter(this RouteHandlerBuilder builder, IR filterFactory = ActivatorUtilities.CreateFactory(typeof(TFilterType), Type.EmptyTypes); } - builder.AddFilter((routeHandlerContext, next) => + builder.AddRouteHandlerFilter((routeHandlerContext, next) => { var invokeArguments = new[] { routeHandlerContext }; return (context) => @@ -56,16 +57,42 @@ public static RouteHandlerBuilder AddFilter(this RouteHandlerBuilder builder, IR return builder; } + /// + /// Registers a filter of type onto the route handler. + /// + /// The type of the to register. + /// The . + /// A that can be used to further customize the route handler. + public static RouteHandlerBuilder AddRouteHandlerFilter<[DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.PublicConstructors)] TFilterType>(this RouteHandlerBuilder builder) + where TFilterType : IRouteHandlerFilter + { + // We have a RouteHandlerBuiler and GroupRouteBuilder-specific AddFilter methods for convenience so you don't have to specify both arguments most the time. + return builder.AddRouteHandlerFilter(); + } + + /// + /// Registers a filter of type onto the route handler. + /// + /// The type of the to register. + /// The . + /// A that can be used to further customize the route handler. + public static RouteGroupBuilder AddRouteHandlerFilter<[DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.PublicConstructors)] TFilterType>(this RouteGroupBuilder builder) + where TFilterType : IRouteHandlerFilter + { + // We have a RouteHandlerBuiler and GroupRouteBuilder-specific AddFilter methods for convenience so you don't have to specify both arguments most the time. + return builder.AddRouteHandlerFilter(); + } + /// /// Registers a filter given a delegate onto the route handler. /// /// The . /// A representing the core logic of the filter. /// A that can be used to further customize the route handler. - public static RouteHandlerBuilder AddFilter(this RouteHandlerBuilder builder, Func> routeHandlerFilter) + public static TBuilder AddRouteHandlerFilter(this TBuilder builder, Func> routeHandlerFilter) + where TBuilder : IEndpointConventionBuilder { - builder.AddFilter((routeHandlerContext, next) => (context) => routeHandlerFilter(context, next)); - return builder; + return builder.AddRouteHandlerFilter((routeHandlerContext, next) => (context) => routeHandlerFilter(context, next)); } /// @@ -74,9 +101,33 @@ public static RouteHandlerBuilder AddFilter(this RouteHandlerBuilder builder, Fu /// The . /// A representing the logic for constructing the filter. /// A that can be used to further customize the route handler. - public static RouteHandlerBuilder AddFilter(this RouteHandlerBuilder builder, Func filterFactory) + public static TBuilder AddRouteHandlerFilter(this TBuilder builder, Func filterFactory) + where TBuilder : IEndpointConventionBuilder { - builder.Add(endpointBuilder => endpointBuilder.Metadata.Add(filterFactory)); + builder.Add(endpointBuilder => + { + // Even if we split RouteFilterMetadata across multiple metadata objects, RouteEndointDataSource will still use all of them. + // We're just trying to be good citizens and keep the Endpoint.Metadata as small as possible. + RouteFilterMetadata? filterMetadata = null; + + foreach (var item in endpointBuilder.Metadata) + { + if (item is RouteFilterMetadata metadata) + { + filterMetadata = metadata; + break; + } + } + + if (filterMetadata is null) + { + filterMetadata = new(); + endpointBuilder.Metadata.Add(filterMetadata); + } + + filterMetadata.FilterFactories.Add(filterFactory); + }); + return builder; } } diff --git a/src/Http/Routing/src/CompositeEndpointDataSource.cs b/src/Http/Routing/src/CompositeEndpointDataSource.cs index c4d8d0204d08..4573ba092a74 100644 --- a/src/Http/Routing/src/CompositeEndpointDataSource.cs +++ b/src/Http/Routing/src/CompositeEndpointDataSource.cs @@ -77,17 +77,17 @@ public override IReadOnlyList Endpoints } /// - public override IReadOnlyList GetGroupedEndpoints(RouteGroupContext context) + public override IReadOnlyList GetGroupedEndpoints(RouteGroupContext context) { if (_dataSources.Count is 0) { - return Array.Empty(); + return Array.Empty(); } // We could try to optimize the single data source case by returning its result directly like GroupDataSource does, // but the CompositeEndpointDataSourceTest class was picky about the Endpoints property creating a shallow copy, // so we'll shallow copy here for consistency. - var groupedEndpoints = new List(); + var groupedEndpoints = new List(); foreach (var dataSource in _dataSources) { diff --git a/src/Http/Routing/src/DefaultEndpointConventionBuilder.cs b/src/Http/Routing/src/DefaultEndpointConventionBuilder.cs index 49731fef37e1..c864c5453029 100644 --- a/src/Http/Routing/src/DefaultEndpointConventionBuilder.cs +++ b/src/Http/Routing/src/DefaultEndpointConventionBuilder.cs @@ -24,7 +24,7 @@ public void Add(Action convention) if (conventions is null) { - throw new InvalidOperationException("Conventions cannot be added after building the endpoint"); + throw new InvalidOperationException(Resources.RouteEndpointDataSource_ConventionsCannotBeModifiedAfterBuild); } conventions.Add(convention); diff --git a/src/Http/Routing/src/EndpointDataSource.cs b/src/Http/Routing/src/EndpointDataSource.cs index c38f8fdfeb15..71cc91a4a281 100644 --- a/src/Http/Routing/src/EndpointDataSource.cs +++ b/src/Http/Routing/src/EndpointDataSource.cs @@ -32,7 +32,7 @@ public abstract class EndpointDataSource /// /// Returns a read-only collection of instances given the specified group and . /// - public virtual IReadOnlyList GetGroupedEndpoints(RouteGroupContext context) + public virtual IReadOnlyList GetGroupedEndpoints(RouteGroupContext context) { // Only evaluate Endpoints once per call. var endpoints = Endpoints; @@ -57,7 +57,7 @@ public virtual IReadOnlyList GetGroupedEndpoints(RouteGroupContex var routeEndpointBuilder = new RouteEndpointBuilder(routeEndpoint.RequestDelegate!, fullRoutePattern, routeEndpoint.Order) { DisplayName = routeEndpoint.DisplayName, - ServiceProvider = context.ApplicationServices, + ApplicationServices = context.ApplicationServices, }; // Apply group conventions to each endpoint in the group at a lower precedent than metadata already on the endpoint. diff --git a/src/Http/Routing/src/PublicAPI.Unshipped.txt b/src/Http/Routing/src/PublicAPI.Unshipped.txt index 5b7f48f1ce77..042aa1893f5d 100644 --- a/src/Http/Routing/src/PublicAPI.Unshipped.txt +++ b/src/Http/Routing/src/PublicAPI.Unshipped.txt @@ -9,7 +9,7 @@ Microsoft.AspNetCore.Routing.RouteGroupContext.Prefix.get -> Microsoft.AspNetCor Microsoft.AspNetCore.Routing.RouteGroupContext.RouteGroupContext(Microsoft.AspNetCore.Routing.Patterns.RoutePattern! prefix, System.Collections.Generic.IReadOnlyList!>! conventions, System.IServiceProvider! applicationServices) -> void Microsoft.AspNetCore.Routing.RouteOptions.SetParameterPolicy(string! token, System.Type! type) -> void Microsoft.AspNetCore.Routing.RouteOptions.SetParameterPolicy(string! token) -> void -override Microsoft.AspNetCore.Routing.CompositeEndpointDataSource.GetGroupedEndpoints(Microsoft.AspNetCore.Routing.RouteGroupContext! context) -> System.Collections.Generic.IReadOnlyList! +override Microsoft.AspNetCore.Routing.CompositeEndpointDataSource.GetGroupedEndpoints(Microsoft.AspNetCore.Routing.RouteGroupContext! context) -> System.Collections.Generic.IReadOnlyList! static Microsoft.AspNetCore.Builder.EndpointRouteBuilderExtensions.MapGroup(this Microsoft.AspNetCore.Routing.IEndpointRouteBuilder! endpoints, Microsoft.AspNetCore.Routing.Patterns.RoutePattern! prefix) -> Microsoft.AspNetCore.Routing.RouteGroupBuilder! static Microsoft.AspNetCore.Builder.EndpointRouteBuilderExtensions.MapGroup(this Microsoft.AspNetCore.Routing.IEndpointRouteBuilder! endpoints, string! prefix) -> Microsoft.AspNetCore.Routing.RouteGroupBuilder! static Microsoft.AspNetCore.Builder.EndpointRouteBuilderExtensions.MapPatch(this Microsoft.AspNetCore.Routing.IEndpointRouteBuilder! endpoints, string! pattern, System.Delegate! handler) -> Microsoft.AspNetCore.Builder.RouteHandlerBuilder! @@ -19,10 +19,12 @@ static Microsoft.AspNetCore.Http.OpenApiRouteHandlerBuilderExtensions.ExcludeFro static Microsoft.AspNetCore.Http.OpenApiRouteHandlerBuilderExtensions.WithDescription(this TBuilder builder, string! description) -> TBuilder static Microsoft.AspNetCore.Http.OpenApiRouteHandlerBuilderExtensions.WithSummary(this TBuilder builder, string! summary) -> TBuilder static Microsoft.AspNetCore.Http.OpenApiRouteHandlerBuilderExtensions.WithTags(this TBuilder builder, params string![]! tags) -> TBuilder -static Microsoft.AspNetCore.Http.RouteHandlerFilterExtensions.AddFilter(this Microsoft.AspNetCore.Builder.RouteHandlerBuilder! builder, Microsoft.AspNetCore.Http.IRouteHandlerFilter! filter) -> Microsoft.AspNetCore.Builder.RouteHandlerBuilder! -static Microsoft.AspNetCore.Http.RouteHandlerFilterExtensions.AddFilter(this Microsoft.AspNetCore.Builder.RouteHandlerBuilder! builder, System.Func! filterFactory) -> Microsoft.AspNetCore.Builder.RouteHandlerBuilder! -static Microsoft.AspNetCore.Http.RouteHandlerFilterExtensions.AddFilter(this Microsoft.AspNetCore.Builder.RouteHandlerBuilder! builder, System.Func>! routeHandlerFilter) -> Microsoft.AspNetCore.Builder.RouteHandlerBuilder! -static Microsoft.AspNetCore.Http.RouteHandlerFilterExtensions.AddFilter(this Microsoft.AspNetCore.Builder.RouteHandlerBuilder! builder) -> Microsoft.AspNetCore.Builder.RouteHandlerBuilder! +static Microsoft.AspNetCore.Http.RouteHandlerFilterExtensions.AddRouteHandlerFilter(this TBuilder builder) -> TBuilder +static Microsoft.AspNetCore.Http.RouteHandlerFilterExtensions.AddRouteHandlerFilter(this TBuilder builder, Microsoft.AspNetCore.Http.IRouteHandlerFilter! filter) -> TBuilder +static Microsoft.AspNetCore.Http.RouteHandlerFilterExtensions.AddRouteHandlerFilter(this TBuilder builder, System.Func! filterFactory) -> TBuilder +static Microsoft.AspNetCore.Http.RouteHandlerFilterExtensions.AddRouteHandlerFilter(this TBuilder builder, System.Func>! routeHandlerFilter) -> TBuilder +static Microsoft.AspNetCore.Http.RouteHandlerFilterExtensions.AddRouteHandlerFilter(this Microsoft.AspNetCore.Builder.RouteHandlerBuilder! builder) -> Microsoft.AspNetCore.Builder.RouteHandlerBuilder! +static Microsoft.AspNetCore.Http.RouteHandlerFilterExtensions.AddRouteHandlerFilter(this Microsoft.AspNetCore.Routing.RouteGroupBuilder! builder) -> Microsoft.AspNetCore.Routing.RouteGroupBuilder! static Microsoft.AspNetCore.Routing.LinkGeneratorEndpointNameAddressExtensions.GetPathByName(this Microsoft.AspNetCore.Routing.LinkGenerator! generator, Microsoft.AspNetCore.Http.HttpContext! httpContext, string! endpointName, Microsoft.AspNetCore.Routing.RouteValueDictionary? values = null, Microsoft.AspNetCore.Http.PathString? pathBase = null, Microsoft.AspNetCore.Http.FragmentString fragment = default(Microsoft.AspNetCore.Http.FragmentString), Microsoft.AspNetCore.Routing.LinkOptions? options = null) -> string? static Microsoft.AspNetCore.Routing.LinkGeneratorEndpointNameAddressExtensions.GetPathByName(this Microsoft.AspNetCore.Routing.LinkGenerator! generator, string! endpointName, Microsoft.AspNetCore.Routing.RouteValueDictionary? values = null, Microsoft.AspNetCore.Http.PathString pathBase = default(Microsoft.AspNetCore.Http.PathString), Microsoft.AspNetCore.Http.FragmentString fragment = default(Microsoft.AspNetCore.Http.FragmentString), Microsoft.AspNetCore.Routing.LinkOptions? options = null) -> string? static Microsoft.AspNetCore.Routing.LinkGeneratorEndpointNameAddressExtensions.GetUriByName(this Microsoft.AspNetCore.Routing.LinkGenerator! generator, Microsoft.AspNetCore.Http.HttpContext! httpContext, string! endpointName, Microsoft.AspNetCore.Routing.RouteValueDictionary? values = null, string? scheme = null, Microsoft.AspNetCore.Http.HostString? host = null, Microsoft.AspNetCore.Http.PathString? pathBase = null, Microsoft.AspNetCore.Http.FragmentString fragment = default(Microsoft.AspNetCore.Http.FragmentString), Microsoft.AspNetCore.Routing.LinkOptions? options = null) -> string? @@ -37,5 +39,5 @@ static Microsoft.AspNetCore.Routing.Patterns.RoutePatternFactory.Pattern(Microso static Microsoft.AspNetCore.Routing.Patterns.RoutePatternFactory.Pattern(Microsoft.AspNetCore.Routing.RouteValueDictionary? defaults, Microsoft.AspNetCore.Routing.RouteValueDictionary? parameterPolicies, params Microsoft.AspNetCore.Routing.Patterns.RoutePatternPathSegment![]! segments) -> Microsoft.AspNetCore.Routing.Patterns.RoutePattern! static Microsoft.AspNetCore.Routing.Patterns.RoutePatternFactory.Pattern(string? rawText, Microsoft.AspNetCore.Routing.RouteValueDictionary? defaults, Microsoft.AspNetCore.Routing.RouteValueDictionary? parameterPolicies, System.Collections.Generic.IEnumerable! segments) -> Microsoft.AspNetCore.Routing.Patterns.RoutePattern! static Microsoft.AspNetCore.Routing.Patterns.RoutePatternFactory.Pattern(string? rawText, Microsoft.AspNetCore.Routing.RouteValueDictionary? defaults, Microsoft.AspNetCore.Routing.RouteValueDictionary? parameterPolicies, params Microsoft.AspNetCore.Routing.Patterns.RoutePatternPathSegment![]! segments) -> Microsoft.AspNetCore.Routing.Patterns.RoutePattern! -virtual Microsoft.AspNetCore.Routing.EndpointDataSource.GetGroupedEndpoints(Microsoft.AspNetCore.Routing.RouteGroupContext! context) -> System.Collections.Generic.IReadOnlyList! +virtual Microsoft.AspNetCore.Routing.EndpointDataSource.GetGroupedEndpoints(Microsoft.AspNetCore.Routing.RouteGroupContext! context) -> System.Collections.Generic.IReadOnlyList! virtual Microsoft.AspNetCore.Routing.Patterns.RoutePatternTransformer.SubstituteRequiredValues(Microsoft.AspNetCore.Routing.Patterns.RoutePattern! original, Microsoft.AspNetCore.Routing.RouteValueDictionary! requiredValues) -> Microsoft.AspNetCore.Routing.Patterns.RoutePattern? diff --git a/src/Http/Routing/src/Resources.resx b/src/Http/Routing/src/Resources.resx index 763fc974d0d0..9b3d90ce5b04 100644 --- a/src/Http/Routing/src/Resources.resx +++ b/src/Http/Routing/src/Resources.resx @@ -243,4 +243,10 @@ MapGroup cannot build a pattern for '{0}' because the 'RoutePattern.{1}' dictionary key '{2}' has multiple values. + + Conventions cannot be added after building the endpoint. + + + This RequestDelegate cannot be called before the final endpoint is built. + \ No newline at end of file diff --git a/src/Http/Routing/src/RouteHandlerEndpointDataSource.cs b/src/Http/Routing/src/RouteEndpointDataSource.cs similarity index 76% rename from src/Http/Routing/src/RouteHandlerEndpointDataSource.cs rename to src/Http/Routing/src/RouteEndpointDataSource.cs index c3c8296d6d1e..e2a0c0699f3d 100644 --- a/src/Http/Routing/src/RouteHandlerEndpointDataSource.cs +++ b/src/Http/Routing/src/RouteEndpointDataSource.cs @@ -13,19 +13,19 @@ namespace Microsoft.AspNetCore.Routing; -internal sealed class RouteHandlerEndpointDataSource : EndpointDataSource +internal sealed class RouteEndpointDataSource : EndpointDataSource { private readonly List _routeEntries = new(); private readonly IServiceProvider _applicationServices; private readonly bool _throwOnBadRequest; - public RouteHandlerEndpointDataSource(IServiceProvider applicationServices, bool throwOnBadRequest) + public RouteEndpointDataSource(IServiceProvider applicationServices, bool throwOnBadRequest) { _applicationServices = applicationServices; _throwOnBadRequest = throwOnBadRequest; } - public List> AddEndpoint( + public ICollection> AddEndpoint( RoutePattern pattern, Delegate routeHandler, IEnumerable? initialEndpointMetadata, @@ -70,6 +70,17 @@ public override IReadOnlyList GetGroupedEndpoints(RouteGroupConte public override IChangeToken GetChangeToken() => NullChangeToken.Singleton; + // For testing + internal RouteEndpointBuilder GetSingleRouteEndpointBuilder() + { + if (_routeEntries.Count != 1) + { + throw new InvalidOperationException($"There are {_routeEntries.Count} endpoints defined! This can only be called for a single endpoint."); + } + + return CreateRouteEndpointBuilder(_routeEntries[0]); + } + [UnconditionalSuppressMessage("Trimmer", "IL2026", Justification = "We surface a RequireUnreferencedCode in the call to Map method adding this EndpointDataSource. " + "The trimmer is unable to infer this.")] @@ -93,7 +104,7 @@ private RouteEndpointBuilder CreateRouteEndpointBuilder(RouteEntry entry, RouteP { if (factoryCreatedRequestDelegate is null) { - throw new InvalidOperationException($"{nameof(RequestDelegateFactory)} has not created the final {nameof(RequestDelegate)} yet."); + throw new InvalidOperationException(Resources.RouteEndpointDataSource_RequestDelegateCannotBeCalledBeforeBuild); } return factoryCreatedRequestDelegate(context); @@ -103,7 +114,7 @@ private RouteEndpointBuilder CreateRouteEndpointBuilder(RouteEntry entry, RouteP var builder = new RouteEndpointBuilder(redirectedRequestDelegate, pattern, order: 0) { DisplayName = displayName, - ServiceProvider = _applicationServices, + ApplicationServices = _applicationServices, }; // We own EndpointBuilder.Metadata (in another assembly), so we know it's just a List. @@ -133,6 +144,7 @@ private RouteEndpointBuilder CreateRouteEndpointBuilder(RouteEntry entry, RouteP metadata.AddRange(attributes); } + entry.Conventions.HasBeenBuilt = true; foreach (var entrySpecificConvention in entry.Conventions) { entrySpecificConvention(builder); @@ -143,10 +155,10 @@ private RouteEndpointBuilder CreateRouteEndpointBuilder(RouteEntry entry, RouteP foreach (var item in builder.Metadata) { - if (item is Func filter) + if (item is RouteFilterMetadata filterMetadata) { routeHandlerFilterFactories ??= new(); - routeHandlerFilterFactories.Add(filter); + routeHandlerFilterFactories.AddRange(filterMetadata.FilterFactories); } } @@ -163,17 +175,14 @@ private RouteEndpointBuilder CreateRouteEndpointBuilder(RouteEntry entry, RouteP RouteParameterNames = routeParamNames, ThrowOnBadRequest = _throwOnBadRequest, DisableInferBodyFromParameters = entry.DisableInferFromBodyParameters, - InitialEndpointMetadata = metadata, + EndpointMetadata = metadata, RouteHandlerFilterFactories = routeHandlerFilterFactories, }; - var requestDelegateResult = RequestDelegateFactory.Create(entry.RouteHandler, factoryOptions); + // We ignore the returned EndpointMetadata has been already populated since we passed in non-null EndpointMetadata. + factoryCreatedRequestDelegate = RequestDelegateFactory.Create(entry.RouteHandler, factoryOptions).RequestDelegate; - // Give inferred metadata the lowest precedent. - metadata.InsertRange(0, requestDelegateResult.EndpointMetadata); - factoryCreatedRequestDelegate = requestDelegateResult.RequestDelegate; - - if (ReferenceEquals(requestDelegateResult.RequestDelegate, redirectedRequestDelegate)) + if (ReferenceEquals(builder.RequestDelegate, redirectedRequestDelegate)) { // 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. @@ -186,9 +195,25 @@ private RouteEndpointBuilder CreateRouteEndpointBuilder(RouteEntry entry, RouteP private struct RouteEntry { public RoutePattern RoutePattern { get; init; } - public List> Conventions { get; init; } public Delegate RouteHandler { get; init; } public IEnumerable? InitialEndpointMetadata { get; init; } public bool DisableInferFromBodyParameters { get; init; } + public ThrowOnAddAfterBuildCollection Conventions { get; init; } + } + + // This private class is only exposed to internal code via ICollection> in RouteEndpointBuilder where only Add is called. + private class ThrowOnAddAfterBuildCollection : List>, ICollection> + { + public bool HasBeenBuilt { get; set; } + + void ICollection>.Add(Action convention) + { + if (HasBeenBuilt) + { + throw new InvalidOperationException(Resources.RouteEndpointDataSource_ConventionsCannotBeModifiedAfterBuild); + } + + Add(convention); + } } } diff --git a/src/Http/Routing/src/RouteFilterMetadata.cs b/src/Http/Routing/src/RouteFilterMetadata.cs new file mode 100644 index 000000000000..215c09b77668 --- /dev/null +++ b/src/Http/Routing/src/RouteFilterMetadata.cs @@ -0,0 +1,11 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. + +using Microsoft.AspNetCore.Http; + +namespace Microsoft.AspNetCore.Routing; + +internal class RouteFilterMetadata +{ + public List> FilterFactories { get; } = new(); +} diff --git a/src/Http/Routing/src/RouteGroupBuilder.cs b/src/Http/Routing/src/RouteGroupBuilder.cs index 383dba67f46d..b72af091c792 100644 --- a/src/Http/Routing/src/RouteGroupBuilder.cs +++ b/src/Http/Routing/src/RouteGroupBuilder.cs @@ -53,14 +53,14 @@ public GroupEndpointDataSource(RouteGroupBuilder groupRouteBuilder) public override IReadOnlyList Endpoints => GetGroupedEndpointsWithNullablePrefix(null, Array.Empty>(), _routeGroupBuilder._outerEndpointRouteBuilder.ServiceProvider); - public override IReadOnlyList GetGroupedEndpoints(RouteGroupContext context) => + public override IReadOnlyList GetGroupedEndpoints(RouteGroupContext context) => GetGroupedEndpointsWithNullablePrefix(context.Prefix, context.Conventions, context.ApplicationServices); - public IReadOnlyList GetGroupedEndpointsWithNullablePrefix(RoutePattern? prefix, IReadOnlyList> conventions, IServiceProvider applicationServices) + public IReadOnlyList GetGroupedEndpointsWithNullablePrefix(RoutePattern? prefix, IReadOnlyList> conventions, IServiceProvider applicationServices) { if (_routeGroupBuilder._dataSources.Count is 0) { - return Array.Empty(); + return Array.Empty(); } var fullPrefix = RoutePatternFactory.Combine(prefix, _routeGroupBuilder._partialPrefix); @@ -72,7 +72,7 @@ public IReadOnlyList GetGroupedEndpointsWithNullablePrefix(RouteP return _routeGroupBuilder._dataSources[0].GetGroupedEndpoints(new RouteGroupContext(fullPrefix, combinedConventions, applicationServices)); } - var groupedEndpoints = new List(); + var groupedEndpoints = new List(); foreach (var dataSource in _routeGroupBuilder._dataSources) { diff --git a/src/Http/Routing/test/UnitTests/Builder/GroupTest.cs b/src/Http/Routing/test/UnitTests/Builder/GroupTest.cs index 83830346d263..3030dd8749a0 100644 --- a/src/Http/Routing/test/UnitTests/Builder/GroupTest.cs +++ b/src/Http/Routing/test/UnitTests/Builder/GroupTest.cs @@ -195,7 +195,7 @@ public void ServiceProviderInConvention_IsSet() ((IEndpointConventionBuilder)group).Add(builder => { - endpointBuilderServiceProvider = builder.ServiceProvider; + endpointBuilderServiceProvider = builder.ApplicationServices; }); var dataSource = GetEndpointDataSource(builder); @@ -400,7 +400,6 @@ private sealed class TestCustomEndpintDataSource : EndpointDataSource private sealed class EmptyServiceProvider : IServiceProvider { public static EmptyServiceProvider Instance { get; } = new EmptyServiceProvider(); - public object? GetService(Type serviceType) => null; } } diff --git a/src/Http/Routing/test/UnitTests/Builder/RequestDelegateEndpointRouteBuilderExtensionsTest.cs b/src/Http/Routing/test/UnitTests/Builder/RequestDelegateEndpointRouteBuilderExtensionsTest.cs index 2420e424e3c4..58c3e7ca1ab3 100644 --- a/src/Http/Routing/test/UnitTests/Builder/RequestDelegateEndpointRouteBuilderExtensionsTest.cs +++ b/src/Http/Routing/test/UnitTests/Builder/RequestDelegateEndpointRouteBuilderExtensionsTest.cs @@ -1,6 +1,8 @@ // Licensed to the .NET Foundation under one or more agreements. // The .NET Foundation licenses this file to you under the MIT license. +#nullable enable + using System.IO.Pipelines; using System.Linq.Expressions; using System.Reflection; @@ -18,15 +20,16 @@ namespace Microsoft.AspNetCore.Builder; public class RequestDelegateEndpointRouteBuilderExtensionsTest { - private ModelEndpointDataSource GetBuilderEndpointDataSource(IEndpointRouteBuilder endpointRouteBuilder) - { - return Assert.IsType(Assert.Single(endpointRouteBuilder.DataSources)); - } + private EndpointDataSource GetBuilderEndpointDataSource(IEndpointRouteBuilder endpointRouteBuilder) => + Assert.Single(endpointRouteBuilder.DataSources); - private RouteEndpointBuilder GetRouteEndpointBuilder(IEndpointRouteBuilder endpointRouteBuilder) - { - return Assert.IsType(Assert.Single(GetBuilderEndpointDataSource(endpointRouteBuilder).EndpointBuilders)); - } + private RouteEndpointBuilder GetRouteEndpointBuilder(IEndpointRouteBuilder endpointRouteBuilder) => + GetBuilderEndpointDataSource(endpointRouteBuilder) switch + { + RouteEndpointDataSource routeDataSource => routeDataSource.GetSingleRouteEndpointBuilder(), + ModelEndpointDataSource modelDataSource => Assert.IsType(Assert.Single(modelDataSource.EndpointBuilders)), + _ => throw new InvalidOperationException($"Unknown EndointDataSource type!"), + }; public static object[][] MapMethods { @@ -62,8 +65,8 @@ IEndpointConventionBuilder Map(IEndpointRouteBuilder routes, string template, Re public void MapEndpoint_StringPattern_BuildsEndpoint() { // Arrange - var builder = new DefaultEndpointRouteBuilder(Mock.Of()); - RequestDelegate requestDelegate = (d) => null; + var builder = new DefaultEndpointRouteBuilder(new ApplicationBuilder(EmptyServiceProvider.Instance)); + RequestDelegate requestDelegate = (d) => Task.CompletedTask; // Act var endpointBuilder = builder.Map("/", requestDelegate); @@ -84,7 +87,7 @@ public async Task MapEndpoint_ReturnGenericTypeTask_GeneratedDelegate() httpContext.Response.Body = responseBodyStream; // Arrange - var builder = new DefaultEndpointRouteBuilder(Mock.Of()); + var builder = new DefaultEndpointRouteBuilder(new ApplicationBuilder(EmptyServiceProvider.Instance)); static async Task GenericTypeTaskDelegate(HttpContext context) => await Task.FromResult("String Test"); // Act @@ -93,7 +96,9 @@ public async Task MapEndpoint_ReturnGenericTypeTask_GeneratedDelegate() // Assert var dataSource = GetBuilderEndpointDataSource(builder); var endpoint = Assert.Single(dataSource.Endpoints); // Triggers build and construction of delegate - var requestDelegate = endpoint.RequestDelegate; + + Assert.NotNull(endpoint.RequestDelegate); + var requestDelegate = endpoint.RequestDelegate!; await requestDelegate(httpContext); var responseBody = Encoding.UTF8.GetString(responseBodyStream.ToArray()); @@ -105,8 +110,8 @@ public async Task MapEndpoint_ReturnGenericTypeTask_GeneratedDelegate() public void MapEndpoint_TypedPattern_BuildsEndpoint() { // Arrange - var builder = new DefaultEndpointRouteBuilder(Mock.Of()); - RequestDelegate requestDelegate = (d) => null; + var builder = new DefaultEndpointRouteBuilder(new ApplicationBuilder(EmptyServiceProvider.Instance)); + RequestDelegate requestDelegate = (d) => Task.CompletedTask; // Act var endpointBuilder = builder.Map(RoutePatternFactory.Parse("/"), requestDelegate); @@ -123,7 +128,7 @@ public void MapEndpoint_TypedPattern_BuildsEndpoint() public void MapEndpoint_AttributesCollectedAsMetadata() { // Arrange - var builder = new DefaultEndpointRouteBuilder(Mock.Of()); + var builder = new DefaultEndpointRouteBuilder(new ApplicationBuilder(EmptyServiceProvider.Instance)); // Act var endpointBuilder = builder.Map(RoutePatternFactory.Parse("/"), Handle); @@ -140,7 +145,7 @@ public void MapEndpoint_AttributesCollectedAsMetadata() public void MapEndpoint_GeneratedDelegateWorks() { // Arrange - var builder = new DefaultEndpointRouteBuilder(Mock.Of()); + var builder = new DefaultEndpointRouteBuilder(new ApplicationBuilder(EmptyServiceProvider.Instance)); Expression handler = context => Task.CompletedTask; @@ -156,7 +161,7 @@ public void MapEndpoint_GeneratedDelegateWorks() public void MapEndpoint_PrecedenceOfMetadata_BuilderMetadataReturned() { // Arrange - var builder = new DefaultEndpointRouteBuilder(Mock.Of()); + var builder = new DefaultEndpointRouteBuilder(new ApplicationBuilder(EmptyServiceProvider.Instance)); // Act var endpointBuilder = builder.MapMethods("/", new[] { "METHOD" }, HandleHttpMetdata); @@ -171,7 +176,7 @@ public void MapEndpoint_PrecedenceOfMetadata_BuilderMetadataReturned() Assert.Equal("METHOD", GetMethod(endpoint.Metadata[1])); Assert.Equal("BUILDER", GetMethod(endpoint.Metadata[2])); - Assert.Equal("BUILDER", endpoint.Metadata.GetMetadata().HttpMethods.Single()); + Assert.Equal("BUILDER", endpoint.Metadata.GetMetadata()?.HttpMethods.Single()); string GetMethod(object metadata) { @@ -185,7 +190,7 @@ string GetMethod(object metadata) public void Map_EndpointMetadataNotDuplicated(Func map) { // Arrange - var builder = new DefaultEndpointRouteBuilder(Mock.Of()); + var builder = new DefaultEndpointRouteBuilder(new ApplicationBuilder(EmptyServiceProvider.Instance)); // Act var endpointBuilder = map(builder, "/", context => Task.CompletedTask).WithMetadata(new EndpointNameMetadata("MapMe")); @@ -208,7 +213,7 @@ public void Map_EndpointMetadataNotDuplicated(Func map) { // Arrange - var builder = new DefaultEndpointRouteBuilder(Mock.Of()); + var builder = new DefaultEndpointRouteBuilder(new ApplicationBuilder(EmptyServiceProvider.Instance)); // Act var endpointBuilder = map(builder, "/", context => Task.CompletedTask).WithMetadata(new EndpointNameMetadata("MapMe")); @@ -227,7 +232,7 @@ public void AddingMetadataAfterBuildingEndpointThrows(Func()); + var builder = new DefaultEndpointRouteBuilder(new ApplicationBuilder(EmptyServiceProvider.Instance)); var @delegate = [Attribute1, Attribute2] (AddsCustomParameterMetadata param1) => new AddsCustomEndpointMetadataResult(); // Act @@ -240,6 +245,9 @@ public void Map_AddsMetadata_InCorrectOrder() Assert.Collection(metadata, m => Assert.IsAssignableFrom(m), + m => Assert.Equal("System.Runtime.CompilerServices.NullableContextAttribute", m.ToString()), + m => Assert.IsAssignableFrom(m), + m => Assert.IsAssignableFrom(m), m => Assert.IsAssignableFrom(m), m => { @@ -250,9 +258,7 @@ public void Map_AddsMetadata_InCorrectOrder() { Assert.IsAssignableFrom(m); Assert.Equal(MetadataSource.ReturnType, ((CustomEndpointMetadata)m).Source); - }, - m => Assert.IsAssignableFrom(m), - m => Assert.IsAssignableFrom(m)); + }); } [Attribute1] @@ -286,7 +292,7 @@ private class AddsCustomEndpointMetadataResult : IEndpointMetadataProvider, IRes { public static void PopulateMetadata(EndpointMetadataContext context) { - context.InferredMetadata.Add(new CustomEndpointMetadata { Source = MetadataSource.ReturnType }); + context.EndpointMetadata.Add(new CustomEndpointMetadata { Source = MetadataSource.ReturnType }); } public Task ExecuteAsync(HttpContext httpContext) => throw new NotImplementedException(); @@ -298,23 +304,23 @@ private class AddsCustomParameterMetadata : IEndpointParameterMetadataProvider, public static void PopulateMetadata(EndpointParameterMetadataContext parameterContext) { - parameterContext.EndpointMetadata.Add(new ParameterNameMetadata { Name = parameterContext.Parameter.Name }); + parameterContext.EndpointMetadata.Add(new ParameterNameMetadata { Name = parameterContext.Parameter.Name ?? string.Empty }); } public static void PopulateMetadata(EndpointMetadataContext context) { - context.InferredMetadata.Add(new CustomEndpointMetadata { Source = MetadataSource.Parameter }); + context.EndpointMetadata.Add(new CustomEndpointMetadata { Source = MetadataSource.Parameter }); } } private class ParameterNameMetadata { - public string Name { get; init; } + public string Name { get; init; } = string.Empty; } private class CustomEndpointMetadata { - public string Data { get; init; } + public string Data { get; init; } = string.Empty; public MetadataSource Source { get; init; } } @@ -324,4 +330,10 @@ private enum MetadataSource Parameter, ReturnType } + + private sealed class EmptyServiceProvider : IServiceProvider + { + public static EmptyServiceProvider Instance { get; } = new EmptyServiceProvider(); + public object? GetService(Type serviceType) => null; + } } diff --git a/src/Http/Routing/test/UnitTests/Builder/RouteHandlerEndpointRouteBuilderExtensionsTest.cs b/src/Http/Routing/test/UnitTests/Builder/RouteHandlerEndpointRouteBuilderExtensionsTest.cs index d213399a90bc..ee5d2cc552b9 100644 --- a/src/Http/Routing/test/UnitTests/Builder/RouteHandlerEndpointRouteBuilderExtensionsTest.cs +++ b/src/Http/Routing/test/UnitTests/Builder/RouteHandlerEndpointRouteBuilderExtensionsTest.cs @@ -16,14 +16,14 @@ namespace Microsoft.AspNetCore.Builder; public class RouteHandlerEndpointRouteBuilderExtensionsTest : LoggedTest { - private ModelEndpointDataSource GetBuilderEndpointDataSource(IEndpointRouteBuilder endpointRouteBuilder) + private RouteEndpointDataSource GetBuilderEndpointDataSource(IEndpointRouteBuilder endpointRouteBuilder) { - return Assert.IsType(Assert.Single(endpointRouteBuilder.DataSources)); + return Assert.IsType(Assert.Single(endpointRouteBuilder.DataSources)); } private RouteEndpointBuilder GetRouteEndpointBuilder(IEndpointRouteBuilder endpointRouteBuilder) { - return Assert.IsType(Assert.Single(GetBuilderEndpointDataSource(endpointRouteBuilder).EndpointBuilders)); + return GetBuilderEndpointDataSource(endpointRouteBuilder).GetSingleRouteEndpointBuilder(); } public static object?[]?[] MapMethods @@ -851,53 +851,23 @@ public async Task MapMethod_DefaultsToNotThrowOnBadHttpRequestIfItCannotResolveR public static object[][] AddFiltersByClassData = { - new object[] { (Action)((RouteHandlerBuilder builder) => builder.AddFilter(new IncrementArgFilter())) }, - new object[] { (Action)((RouteHandlerBuilder builder) => builder.AddFilter()) } + new object[] { (Action)((IEndpointConventionBuilder builder) => builder.AddRouteHandlerFilter(new IncrementArgFilter())) }, + new object[] { (Action)((IEndpointConventionBuilder builder) => builder.AddRouteHandlerFilter()) } }; - [Theory] - [MemberData(nameof(AddFiltersByClassData))] - public async Task AddFilterMethods_CanRegisterFilterWithClassImplementation(Action addFilter) - { - var builder = new DefaultEndpointRouteBuilder(new ApplicationBuilder(new ServiceCollection().BuildServiceProvider())); - - string PrintId(int id) => $"ID: {id}"; - var routeHandlerBuilder = builder.Map("/{id}", PrintId); - addFilter(routeHandlerBuilder); - - var dataSource = GetBuilderEndpointDataSource(builder); - // Trigger Endpoint build by calling getter. - var endpoint = Assert.Single(dataSource.Endpoints); - - var httpContext = new DefaultHttpContext(); - httpContext.Request.RouteValues["id"] = "2"; - var outStream = new MemoryStream(); - httpContext.Response.Body = outStream; - - await endpoint.RequestDelegate!(httpContext); - - // Assert; - var httpResponse = httpContext.Response; - httpResponse.Body.Seek(0, SeekOrigin.Begin); - var streamReader = new StreamReader(httpResponse.Body); - var body = streamReader.ReadToEndAsync().Result; - Assert.Equal(200, httpContext.Response.StatusCode); - Assert.Equal("ID: 3", body); - } - public static object[][] AddFiltersByDelegateData { get { - void WithFilter(RouteHandlerBuilder builder) => - builder.AddFilter(async (context, next) => + void WithFilter(IEndpointConventionBuilder builder) => + builder.AddRouteHandlerFilter(async (context, next) => { context.Arguments[0] = ((int)context.Arguments[0]!) + 1; return await next(context); }); - void WithFilterFactory(RouteHandlerBuilder builder) => - builder.AddFilter((routeHandlerContext, next) => async (context) => + void WithFilterFactory(IEndpointConventionBuilder builder) => + builder.AddRouteHandlerFilter((routeHandlerContext, next) => async (context) => { Assert.NotNull(routeHandlerContext.MethodInfo); Assert.NotNull(routeHandlerContext.MethodInfo.DeclaringType); @@ -908,32 +878,23 @@ void WithFilterFactory(RouteHandlerBuilder builder) => }); return new object[][] { - new object[] { (Action)WithFilter }, - new object[] { (Action)WithFilterFactory } + new object[] { (Action)WithFilter }, + new object[] { (Action)WithFilterFactory } }; } } - [Theory] - [MemberData(nameof(AddFiltersByDelegateData))] - public async Task AddFilterMethods_CanRegisterFilterWithDelegateImplementation(Action addFilter) + private static async Task AssertIdAsync(Endpoint endpoint, string expectedPattern, int expectedId) { - var builder = new DefaultEndpointRouteBuilder(new ApplicationBuilder(new ServiceCollection().BuildServiceProvider())); - - string PrintId(int id) => $"ID: {id}"; - var routeHandlerBuilder = builder.Map("/{id}", PrintId); - addFilter(routeHandlerBuilder); - - var dataSource = GetBuilderEndpointDataSource(builder); - // Trigger Endpoint build by calling getter. - var endpoint = Assert.Single(dataSource.Endpoints); + var routeEndpoint = Assert.IsType(endpoint); + Assert.Equal(expectedPattern, routeEndpoint.RoutePattern.RawText); var httpContext = new DefaultHttpContext(); httpContext.Request.RouteValues["id"] = "2"; var outStream = new MemoryStream(); httpContext.Response.Body = outStream; - await endpoint.RequestDelegate!(httpContext); + await routeEndpoint.RequestDelegate!(httpContext); // Assert; var httpResponse = httpContext.Response; @@ -941,7 +902,53 @@ public async Task AddFilterMethods_CanRegisterFilterWithDelegateImplementation(A var streamReader = new StreamReader(httpResponse.Body); var body = streamReader.ReadToEndAsync().Result; Assert.Equal(200, httpContext.Response.StatusCode); - Assert.Equal("ID: 3", body); + Assert.Equal($"ID: {expectedId}", body); + } + + [Theory] + [MemberData(nameof(AddFiltersByClassData))] + [MemberData(nameof(AddFiltersByDelegateData))] + public async Task AddRouteHandlerFilterMethods_CanRegisterFilterWithClassAndDelegateImplementations(Action addFilter) + { + var builder = new DefaultEndpointRouteBuilder(new ApplicationBuilder(new ServiceCollection().BuildServiceProvider())); + + string PrintId(int id) => $"ID: {id}"; + addFilter(builder.Map("/{id}", PrintId)); + + var dataSource = GetBuilderEndpointDataSource(builder); + // Trigger Endpoint build by calling getter. + var endpoint = Assert.Single(dataSource.Endpoints); + await AssertIdAsync(endpoint, "/{id}", 3); + } + + [Theory] + [MemberData(nameof(AddFiltersByClassData))] + [MemberData(nameof(AddFiltersByDelegateData))] + public async Task AddRouteHandlerFilterMethods_WorkWithMapGroup(Action addFilter) + { + var builder = new DefaultEndpointRouteBuilder(new ApplicationBuilder(new ServiceCollection().BuildServiceProvider())); + + string PrintId(int id) => $"ID: {id}"; + addFilter(builder.Map("/{id}", PrintId)); + + var outerGroup = builder.MapGroup("/outer"); + addFilter(outerGroup); + addFilter(outerGroup.Map("/{id}", PrintId)); + + var innerGroup = outerGroup.MapGroup("/inner"); + addFilter(innerGroup); + addFilter(innerGroup.Map("/{id}", PrintId)); + + var endpoints = builder.DataSources + .SelectMany(ds => ds.Endpoints) + .ToDictionary(e => ((RouteEndpoint)e).RoutePattern.RawText!); + + Assert.Equal(3, endpoints.Count); + + // For each layer of grouping, another filter is applies which increments the expectedId by 1 each time. + await AssertIdAsync(endpoints["/{id}"], expectedPattern: "/{id}", expectedId: 3); + await AssertIdAsync(endpoints["/outer/{id}"], expectedPattern: "/outer/{id}", expectedId: 4); + await AssertIdAsync(endpoints["/outer/inner/{id}"], expectedPattern: "/outer/inner/{id}", expectedId: 5); } [Fact] @@ -951,7 +958,7 @@ public async Task RequestDelegateFactory_CanInvokeEndpointFilter_ThatAccessesSer string? PrintLogger(HttpContext context) => $"loggerErrorIsEnabled: {context.Items["loggerErrorIsEnabled"]}, parentName: {context.Items["parentName"]}"; var routeHandlerBuilder = builder.Map("/", PrintLogger); - routeHandlerBuilder.AddFilter(); + routeHandlerBuilder.AddRouteHandlerFilter(); var dataSource = GetBuilderEndpointDataSource(builder); // Trigger Endpoint build by calling getter. @@ -984,7 +991,7 @@ public void RequestDelegateFactory_ProvidesAppServiceProvider_ToFilterFactory() string? PrintLogger(HttpContext context) => $"loggerErrorIsEnabled: {context.Items["loggerErrorIsEnabled"]}, parentName: {context.Items["parentName"]}"; var routeHandlerBuilder = builder.Map("/", PrintLogger); - routeHandlerBuilder.AddFilter((rhc, next) => + routeHandlerBuilder.AddRouteHandlerFilter((rhc, next) => { Assert.NotNull(rhc.ApplicationServices); var myService = rhc.ApplicationServices.GetRequiredService(); @@ -1002,28 +1009,21 @@ public void RequestDelegateFactory_ProvidesAppServiceProvider_ToFilterFactory() [Fact] public void RouteHandlerContext_ThrowsArgumentNullException_ForMethodInfo() { - Assert.Throws("methodInfo", () => new RouteHandlerContext(null!, new List(), new List(), new ServiceCollection().BuildServiceProvider())); + Assert.Throws("methodInfo", () => new RouteHandlerContext(null!, new List(), new ServiceCollection().BuildServiceProvider())); } [Fact] public void RouteHandlerContext_ThrowsArgumentNullException_ForEndpointMetadata() { var handler = () => { }; - Assert.Throws("endpointMetadata", () => new RouteHandlerContext(handler.Method, null!, new List(), new ServiceCollection().BuildServiceProvider())); - } - - [Fact] - public void RouteHandlerContext_ThrowsArgumentNullException_ForInferredMetadata() - { - var handler = () => { }; - Assert.Throws("inferedMetadata", () => new RouteHandlerContext(handler.Method, new List(), null!, new ServiceCollection().BuildServiceProvider())); + Assert.Throws("endpointMetadata", () => new RouteHandlerContext(handler.Method, null!, new ServiceCollection().BuildServiceProvider())); } [Fact] public void RouteHandlerContext_ThrowsArgumentNullException_ForApplicationServices() { var handler = () => { }; - Assert.Throws("applicationServices", () => new RouteHandlerContext(handler.Method, new List(), new List(), null!)); + Assert.Throws("applicationServices", () => new RouteHandlerContext(handler.Method, new List(), null!)); } class MyService { } diff --git a/src/Http/Routing/test/UnitTests/CompositeEndpointDataSourceTest.cs b/src/Http/Routing/test/UnitTests/CompositeEndpointDataSourceTest.cs index 6e71dc9ef319..c296a7d2e541 100644 --- a/src/Http/Routing/test/UnitTests/CompositeEndpointDataSourceTest.cs +++ b/src/Http/Routing/test/UnitTests/CompositeEndpointDataSourceTest.cs @@ -269,7 +269,7 @@ public void GetGroupedEndpoints_ForwardedToChildDataSources() var receivedContext = Assert.Single(dataSource.ReceivedRouteGroupContexts); Assert.Same(context, receivedContext); - var resolvedEndpoint = Assert.Single(groupedEndpoints); + var resolvedEndpoint = Assert.IsType(Assert.Single(groupedEndpoints)); Assert.Equal("/prefix/a", resolvedEndpoint.RoutePattern.RawText); var resolvedMetadata = Assert.Single(resolvedEndpoint.Metadata); Assert.Same(metadata, resolvedMetadata); @@ -291,15 +291,15 @@ private RouteEndpoint CreateEndpoint( private class TestGroupDataSource : EndpointDataSource { - public TestGroupDataSource(params RouteEndpoint[] endpoints) => Endpoints = endpoints; + public TestGroupDataSource(params Endpoint[] endpoints) => Endpoints = endpoints; public override IReadOnlyList Endpoints { get; } public List ReceivedRouteGroupContexts { get; } = new(); - public List> ResolvedGroupedEndpoints { get; } = new(); + public List> ResolvedGroupedEndpoints { get; } = new(); - public override IReadOnlyList GetGroupedEndpoints(RouteGroupContext context) + public override IReadOnlyList GetGroupedEndpoints(RouteGroupContext context) { ReceivedRouteGroupContexts.Add(context); var resolved = base.GetGroupedEndpoints(context); diff --git a/src/OpenApi/src/OpenApiRouteHandlerBuilderExtensions.cs b/src/OpenApi/src/OpenApiRouteHandlerBuilderExtensions.cs index 878ecc2bdfe0..d014640cfec4 100644 --- a/src/OpenApi/src/OpenApiRouteHandlerBuilderExtensions.cs +++ b/src/OpenApi/src/OpenApiRouteHandlerBuilderExtensions.cs @@ -23,19 +23,9 @@ public static class OpenApiRouteHandlerBuilderExtensions /// /// The . /// A that can be used to further customize the endpoint. - public static RouteHandlerBuilder WithOpenApi(this RouteHandlerBuilder builder) + public static TBuilder WithOpenApi(this TBuilder builder) where TBuilder : IEndpointConventionBuilder { - builder.Add(endpointBuilder => - { - if (endpointBuilder is RouteEndpointBuilder routeEndpointBuilder) - { - var openApiOperation = GetOperationForEndpoint(routeEndpointBuilder); - if (openApiOperation != null) - { - routeEndpointBuilder.Metadata.Add(openApiOperation); - } - }; - }); + builder.Add(builder => AddAndConfigureOperationForEndpoint(builder)); return builder; } @@ -46,24 +36,46 @@ public static RouteHandlerBuilder WithOpenApi(this RouteHandlerBuilder builder) /// The . /// An that returns a new OpenAPI annotation given a generated operation. /// A that can be used to further customize the endpoint. - public static RouteHandlerBuilder WithOpenApi(this RouteHandlerBuilder builder, Func configureOperation) + public static TBuilder WithOpenApi(this TBuilder builder, Func configureOperation) + where TBuilder : IEndpointConventionBuilder + { + builder.Add(endpointBuilder => AddAndConfigureOperationForEndpoint(endpointBuilder, configureOperation)); + return builder; + } + + private static void AddAndConfigureOperationForEndpoint(EndpointBuilder endpointBuilder, Func? configure = null) { - builder.Add(endpointBuilder => + foreach (var item in endpointBuilder.Metadata) { - if (endpointBuilder is RouteEndpointBuilder routeEndpointBuilder) + if (item is OpenApiOperation existingOperation) { - var openApiOperation = GetOperationForEndpoint(routeEndpointBuilder); - if (openApiOperation != null) + if (configure is not null) { - routeEndpointBuilder.Metadata.Add(configureOperation(openApiOperation)); + var configuredOperation = configure(existingOperation); + + if (!ReferenceEquals(configuredOperation, existingOperation)) + { + endpointBuilder.Metadata.Remove(existingOperation); + + // The only way configureOperation could be null here is if configureOperation violated it's signature and returned null. + // We could throw or something, removing the previous metadata seems fine. + if (configuredOperation is not null) + { + endpointBuilder.Metadata.Add(configuredOperation); + } + } } - }; - }); - return builder; - } - private static OpenApiOperation? GetOperationForEndpoint(RouteEndpointBuilder routeEndpointBuilder) - { + return; + } + } + + // We cannot generate an OpenApiOperation without routeEndpointBuilder.RoutePattern. + if (endpointBuilder is not RouteEndpointBuilder routeEndpointBuilder) + { + return; + } + var pattern = routeEndpointBuilder.RoutePattern; var metadata = new EndpointMetadataCollection(routeEndpointBuilder.Metadata); var methodInfo = metadata.OfType().SingleOrDefault(); @@ -71,12 +83,25 @@ public static RouteHandlerBuilder WithOpenApi(this RouteHandlerBuilder builder, if (methodInfo == null || serviceProvider == null) { - return null; + return; } var hostEnvironment = serviceProvider.GetService(); var serviceProviderIsService = serviceProvider.GetService(); var generator = new OpenApiGenerator(hostEnvironment, serviceProviderIsService); - return generator.GetOpenApiOperation(methodInfo, metadata, pattern); + var newOperation = generator.GetOpenApiOperation(methodInfo, metadata, pattern); + + if (newOperation is not null) + { + if (configure is not null) + { + newOperation = configure(newOperation); + } + + if (newOperation is not null) + { + routeEndpointBuilder.Metadata.Add(newOperation); + } + } } } diff --git a/src/OpenApi/src/PublicAPI.Unshipped.txt b/src/OpenApi/src/PublicAPI.Unshipped.txt index d83debd1c5d6..a6362d7b206d 100644 --- a/src/OpenApi/src/PublicAPI.Unshipped.txt +++ b/src/OpenApi/src/PublicAPI.Unshipped.txt @@ -1,4 +1,4 @@ #nullable enable Microsoft.AspNetCore.OpenApi.OpenApiRouteHandlerBuilderExtensions -static Microsoft.AspNetCore.OpenApi.OpenApiRouteHandlerBuilderExtensions.WithOpenApi(this Microsoft.AspNetCore.Builder.RouteHandlerBuilder! builder) -> Microsoft.AspNetCore.Builder.RouteHandlerBuilder! -static Microsoft.AspNetCore.OpenApi.OpenApiRouteHandlerBuilderExtensions.WithOpenApi(this Microsoft.AspNetCore.Builder.RouteHandlerBuilder! builder, System.Func! configureOperation) -> Microsoft.AspNetCore.Builder.RouteHandlerBuilder! +static Microsoft.AspNetCore.OpenApi.OpenApiRouteHandlerBuilderExtensions.WithOpenApi(this TBuilder builder) -> TBuilder +static Microsoft.AspNetCore.OpenApi.OpenApiRouteHandlerBuilderExtensions.WithOpenApi(this TBuilder builder, System.Func! configureOperation) -> TBuilder diff --git a/src/OpenApi/test/OpenApiRouteHandlerBuilderExtensionTests.cs b/src/OpenApi/test/OpenApiRouteHandlerBuilderExtensionTests.cs index 97be66d20a46..eae0866fc3fb 100644 --- a/src/OpenApi/test/OpenApiRouteHandlerBuilderExtensionTests.cs +++ b/src/OpenApi/test/OpenApiRouteHandlerBuilderExtensionTests.cs @@ -58,8 +58,94 @@ public void WithOpenApi_CanSetOperationInMetadataWithOverride() Assert.Empty(operation.Responses); } - private ModelEndpointDataSource GetBuilderEndpointDataSource(IEndpointRouteBuilder endpointRouteBuilder) + [Fact] + public void WithOpenApi_WorksWithMapGroup() + { + var hostEnvironment = new HostEnvironment() { ApplicationName = nameof(OpenApiOperationGeneratorTests) }; + var serviceProviderIsService = new ServiceProviderIsService(); + var serviceProvider = new ServiceCollection() + .AddSingleton(serviceProviderIsService) + .AddSingleton(hostEnvironment) + .BuildServiceProvider(); + + var builder = new DefaultEndpointRouteBuilder(new ApplicationBuilder(serviceProvider)); + string GetString() => "Foo"; + var myGroup = builder.MapGroup("/group"); + + myGroup.MapDelete("/a", GetString); + + // The order WithOpenApi() is relative to the MapDelete() methods does not matter. + myGroup.WithOpenApi(); + + myGroup.MapDelete("/b", GetString); + + // The RotueGroupBuilder adds a single EndpointDataSource. + var groupDataSource = Assert.Single(builder.DataSources); + + Assert.Collection(groupDataSource.Endpoints, + e => Assert.NotNull(e.Metadata.GetMetadata()), + e => Assert.NotNull(e.Metadata.GetMetadata())); + } + + [Fact] + public void WithOpenApi_GroupMetadataCanBeSeenByAndOverriddenByMoreLocalMetadata() + { + var hostEnvironment = new HostEnvironment() { ApplicationName = nameof(OpenApiOperationGeneratorTests) }; + var serviceProviderIsService = new ServiceProviderIsService(); + var serviceProvider = new ServiceCollection() + .AddSingleton(serviceProviderIsService) + .AddSingleton(hostEnvironment) + .BuildServiceProvider(); + + var builder = new DefaultEndpointRouteBuilder(new ApplicationBuilder(serviceProvider)); + string GetString() => "Foo"; + + static void WithLocalSummary(RouteHandlerBuilder builder) + { + builder.WithOpenApi(operation => + { + operation.Summary += $" | Local Summary | 200 Status Response Content-Type: {operation.Responses["200"].Content.Keys.Single()}"; + return operation; + }); + } + + WithLocalSummary(builder.MapDelete("/root", GetString)); + + var outerGroup = builder.MapGroup("/outer"); + var innerGroup = outerGroup.MapGroup("/inner"); + + WithLocalSummary(outerGroup.MapDelete("/outer-a", GetString)); + + // The order WithOpenApi() is relative to the MapDelete() methods does not matter. + outerGroup.WithOpenApi(operation => + { + operation.Summary = "Outer Group Summary"; + return operation; + }); + + WithLocalSummary(outerGroup.MapDelete("/outer-b", GetString)); + WithLocalSummary(innerGroup.MapDelete("/inner-a", GetString)); + + innerGroup.WithOpenApi(operation => + { + operation.Summary += " | Inner Group Summary"; + return operation; + }); + + WithLocalSummary(innerGroup.MapDelete("/inner-b", GetString)); + + var summaries = builder.DataSources.SelectMany(ds => ds.Endpoints).Select(e => e.Metadata.GetMetadata().Summary).ToArray(); + + Assert.Equal(5, summaries.Length); + Assert.Contains(" | Local Summary | 200 Status Response Content-Type: text/plain", summaries); + Assert.Contains("Outer Group Summary | Local Summary | 200 Status Response Content-Type: text/plain", summaries); + Assert.Contains("Outer Group Summary | Local Summary | 200 Status Response Content-Type: text/plain", summaries); + Assert.Contains("Outer Group Summary | Inner Group Summary | Local Summary | 200 Status Response Content-Type: text/plain", summaries); + Assert.Contains("Outer Group Summary | Inner Group Summary | Local Summary | 200 Status Response Content-Type: text/plain", summaries); + } + + private RouteEndpointDataSource GetBuilderEndpointDataSource(IEndpointRouteBuilder endpointRouteBuilder) { - return Assert.IsType(Assert.Single(endpointRouteBuilder.DataSources)); + return Assert.IsType(Assert.Single(endpointRouteBuilder.DataSources)); } } From e1c8b912471642ded96565be6c2c05c0b606bfd7 Mon Sep 17 00:00:00 2001 From: Stephen Halter Date: Wed, 15 Jun 2022 14:16:25 -0700 Subject: [PATCH 03/21] Fix missed ServiceProvider -> ApplicationServices --- src/OpenApi/src/OpenApiRouteHandlerBuilderExtensions.cs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/OpenApi/src/OpenApiRouteHandlerBuilderExtensions.cs b/src/OpenApi/src/OpenApiRouteHandlerBuilderExtensions.cs index d014640cfec4..83ce59fb6aa6 100644 --- a/src/OpenApi/src/OpenApiRouteHandlerBuilderExtensions.cs +++ b/src/OpenApi/src/OpenApiRouteHandlerBuilderExtensions.cs @@ -79,15 +79,15 @@ private static void AddAndConfigureOperationForEndpoint(EndpointBuilder endpoint var pattern = routeEndpointBuilder.RoutePattern; var metadata = new EndpointMetadataCollection(routeEndpointBuilder.Metadata); var methodInfo = metadata.OfType().SingleOrDefault(); - var serviceProvider = routeEndpointBuilder.ServiceProvider; + var applicationServices = routeEndpointBuilder.ApplicationServices; - if (methodInfo == null || serviceProvider == null) + if (methodInfo is null || applicationServices is null) { return; } - var hostEnvironment = serviceProvider.GetService(); - var serviceProviderIsService = serviceProvider.GetService(); + var hostEnvironment = applicationServices.GetService(); + var serviceProviderIsService = applicationServices.GetService(); var generator = new OpenApiGenerator(hostEnvironment, serviceProviderIsService); var newOperation = generator.GetOpenApiOperation(methodInfo, metadata, pattern); From 97a5ada5d7c6bb547eaa58203e052ecc29f99c76 Mon Sep 17 00:00:00 2001 From: Stephen Halter Date: Wed, 15 Jun 2022 18:36:46 -0700 Subject: [PATCH 04/21] Fix some tests and doc comments --- .../WebApplicationTests.cs | 13 +-- .../src/RequestDelegateFactoryOptions.cs | 8 +- ...pointMetadataApiDescriptionProviderTest.cs | 80 +++++++------------ .../OpenApiRouteHandlerBuilderExtensions.cs | 4 +- ...penApiRouteHandlerBuilderExtensionTests.cs | 28 +++++-- 5 files changed, 62 insertions(+), 71 deletions(-) diff --git a/src/DefaultBuilder/test/Microsoft.AspNetCore.Tests/WebApplicationTests.cs b/src/DefaultBuilder/test/Microsoft.AspNetCore.Tests/WebApplicationTests.cs index 52c757f10f70..1ae68e9aa770 100644 --- a/src/DefaultBuilder/test/Microsoft.AspNetCore.Tests/WebApplicationTests.cs +++ b/src/DefaultBuilder/test/Microsoft.AspNetCore.Tests/WebApplicationTests.cs @@ -1572,12 +1572,13 @@ public async Task BranchingPipelineHasOwnRoutes() app.Start(); var ds = app.Services.GetRequiredService(); - Assert.Equal(5, ds.Endpoints.Count); - Assert.Equal("One", ds.Endpoints[0].DisplayName); - Assert.Equal("Two", ds.Endpoints[1].DisplayName); - Assert.Equal("Three", ds.Endpoints[2].DisplayName); - Assert.Equal("Four", ds.Endpoints[3].DisplayName); - Assert.Equal("Five", ds.Endpoints[4].DisplayName); + var displayNames = ds.Endpoints.Select(e => e.DisplayName).ToArray(); + Assert.Equal(5, displayNames.Length); + Assert.Contains("One", displayNames); + Assert.Contains("Two", displayNames); + Assert.Contains("Three", displayNames); + Assert.Contains("Four", displayNames); + Assert.Contains("Five", displayNames); var client = app.GetTestClient(); diff --git a/src/Http/Http.Extensions/src/RequestDelegateFactoryOptions.cs b/src/Http/Http.Extensions/src/RequestDelegateFactoryOptions.cs index 5e3cd4fd571d..03c02a8a81cf 100644 --- a/src/Http/Http.Extensions/src/RequestDelegateFactoryOptions.cs +++ b/src/Http/Http.Extensions/src/RequestDelegateFactoryOptions.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 Microsoft.AspNetCore.Builder; using Microsoft.AspNetCore.Http.Metadata; using Microsoft.Extensions.Logging; @@ -38,13 +39,10 @@ public sealed class RequestDelegateFactoryOptions public IReadOnlyList>? RouteHandlerFilterFactories { get; init; } /// - /// The initial endpoint metadata to add as part of the creation of the . + /// The to use for the creation of the . /// /// - /// This metadata will be included in before any metadata inferred during creation of the - /// and before any metadata provided by types in the delegate signature that implement - /// or , i.e. this metadata will be less specific than any - /// inferred by the call to . + /// This metadata will be returned as if it is non-null. /// public IList? EndpointMetadata { get; init; } } diff --git a/src/Mvc/Mvc.ApiExplorer/test/EndpointMetadataApiDescriptionProviderTest.cs b/src/Mvc/Mvc.ApiExplorer/test/EndpointMetadataApiDescriptionProviderTest.cs index d3a8fc96eab1..deda7c9f03d5 100644 --- a/src/Mvc/Mvc.ApiExplorer/test/EndpointMetadataApiDescriptionProviderTest.cs +++ b/src/Mvc/Mvc.ApiExplorer/test/EndpointMetadataApiDescriptionProviderTest.cs @@ -1,6 +1,8 @@ // Licensed to the .NET Foundation under one or more agreements. // The .NET Foundation licenses this file to you under the MIT license. +#nullable enable + using System.ComponentModel; using System.Reflection; using System.Security.Claims; @@ -111,8 +113,6 @@ public void AddsMultipleRequestFormatsFromMetadataWithRequestTypeAndOptionalBody Assert.False(apiParameterDescription.IsRequired); } -#nullable enable - [Fact] public void AddsMultipleRequestFormatsFromMetadataWithRequiredBodyParameter() { @@ -128,8 +128,6 @@ public void AddsMultipleRequestFormatsFromMetadataWithRequiredBodyParameter() Assert.True(apiParameterDescription.IsRequired); } -#nullable disable - [Fact] public void AddsJsonResponseFormatWhenFromBodyInferred() { @@ -415,8 +413,6 @@ public void AddsDefaultValueFromParameters() Assert.Equal(42, param.DefaultValue); } -#nullable enable - [Fact] public void AddsMultipleParameters() { @@ -446,7 +442,6 @@ public void AddsMultipleParameters() } #nullable disable - [Fact] public void AddsMultipleParametersFromParametersAttribute() { @@ -488,6 +483,7 @@ static void AssertParameters(ApiDescription apiDescription, string capturedName AssertParameters(GetApiDescription(([AsParameters] ArgumentListRecordWithoutAttributes req) => { }, "/{foo}"), "foo"); AssertParameters(GetApiDescription(([AsParameters] ArgumentListRecordWithoutAttributes req) => { }, "/{Foo}")); } +#nullable enable [Fact] public void TestParameterIsRequired() @@ -531,6 +527,7 @@ public void AddsMetadataFromRouteEndpoint() Assert.True(apiExplorerSettings.IgnoreApi); } +#nullable disable [Fact] public void TestParameterIsRequiredForObliviousNullabilityContext() { @@ -577,6 +574,7 @@ public void TestParameterAttributesCanBeInspected() Assert.NotNull(description); Assert.Equal("The name.", description.Description); } +#nullable enable [Fact] public void RespectsProducesProblemExtensionMethod() @@ -773,11 +771,12 @@ public void HandleAcceptsMetadata() }); } +#nullable disable [Fact] public void HandleAcceptsMetadataWithTypeParameter() { // Arrange - var builder = new TestEndpointRouteBuilder(new ApplicationBuilder(null)); + var builder = new TestEndpointRouteBuilder(new ApplicationBuilder(TestServiceProvider.Instance)); builder.MapPost("/api/todos", (InferredJsonClass inferredJsonClass) => "") .Accepts(typeof(InferredJsonClass), "application/json"); var context = new ApiDescriptionProviderContext(Array.Empty()); @@ -800,6 +799,7 @@ public void HandleAcceptsMetadataWithTypeParameter() Assert.Equal("inferredJsonClass", bodyParameterDescription.Name); Assert.False(bodyParameterDescription.IsRequired); } +#nullable enable [Fact] public void FavorsProducesMetadataOverAttribute() @@ -832,15 +832,11 @@ public void FavorsProducesMetadataOverAttribute() }); } -#nullable enable - [Fact] public void HandleDefaultIAcceptsMetadataForRequiredBodyParameter() { // Arrange - var services = new ServiceCollection(); - var serviceProvider = services.BuildServiceProvider(); - var builder = new TestEndpointRouteBuilder(new ApplicationBuilder(serviceProvider)); + var builder = new TestEndpointRouteBuilder(new ApplicationBuilder(TestServiceProvider.Instance)); builder.MapPost("/api/todos", (InferredJsonClass inferredJsonClass) => ""); var context = new ApiDescriptionProviderContext(Array.Empty()); @@ -872,9 +868,7 @@ public void HandleDefaultIAcceptsMetadataForRequiredBodyParameter() public void HandleDefaultIAcceptsMetadataForOptionalBodyParameter() { // Arrange - var services = new ServiceCollection(); - var serviceProvider = services.BuildServiceProvider(); - var builder = new TestEndpointRouteBuilder(new ApplicationBuilder(serviceProvider)); + var builder = new TestEndpointRouteBuilder(new ApplicationBuilder(TestServiceProvider.Instance)); builder.MapPost("/api/todos", (InferredJsonClass? inferredJsonClass) => ""); var context = new ApiDescriptionProviderContext(Array.Empty()); @@ -906,9 +900,7 @@ public void HandleDefaultIAcceptsMetadataForOptionalBodyParameter() public void HandleIAcceptsMetadataWithConsumesAttributeAndInferredOptionalFromBodyType() { // Arrange - var services = new ServiceCollection(); - var serviceProvider = services.BuildServiceProvider(); - var builder = new TestEndpointRouteBuilder(new ApplicationBuilder(serviceProvider)); + var builder = new TestEndpointRouteBuilder(new ApplicationBuilder(TestServiceProvider.Instance)); builder.MapPost("/api/todos", [Consumes("application/xml")] (InferredJsonClass? inferredJsonClass) => ""); var context = new ApiDescriptionProviderContext(Array.Empty()); @@ -940,9 +932,7 @@ public void HandleIAcceptsMetadataWithConsumesAttributeAndInferredOptionalFromBo public void HandleDefaultIAcceptsMetadataForRequiredFormFileParameter() { // Arrange - var services = new ServiceCollection(); - var serviceProvider = services.BuildServiceProvider(); - var builder = new TestEndpointRouteBuilder(new ApplicationBuilder(serviceProvider)); + var builder = new TestEndpointRouteBuilder(new ApplicationBuilder(TestServiceProvider.Instance)); builder.MapPost("/file/upload", (IFormFile formFile) => ""); var context = new ApiDescriptionProviderContext(Array.Empty()); @@ -971,9 +961,7 @@ public void HandleDefaultIAcceptsMetadataForRequiredFormFileParameter() public void HandleDefaultIAcceptsMetadataForOptionalFormFileParameter() { // Arrange - var services = new ServiceCollection(); - var serviceProvider = services.BuildServiceProvider(); - var builder = new TestEndpointRouteBuilder(new ApplicationBuilder(serviceProvider)); + var builder = new TestEndpointRouteBuilder(new ApplicationBuilder(TestServiceProvider.Instance)); builder.MapPost("/file/upload", (IFormFile? inferredFormFile) => ""); var context = new ApiDescriptionProviderContext(Array.Empty()); @@ -1002,9 +990,7 @@ public void HandleDefaultIAcceptsMetadataForOptionalFormFileParameter() public void AddsMultipartFormDataResponseFormatWhenFormFileSpecified() { // Arrange - var services = new ServiceCollection(); - var serviceProvider = services.BuildServiceProvider(); - var builder = new TestEndpointRouteBuilder(new ApplicationBuilder(serviceProvider)); + var builder = new TestEndpointRouteBuilder(new ApplicationBuilder(TestServiceProvider.Instance)); builder.MapPost("/file/upload", (IFormFile file) => Results.NoContent()); var context = new ApiDescriptionProviderContext(Array.Empty()); @@ -1092,9 +1078,7 @@ public void AddsMultipartFormDataResponseFormatWhenFormFileCollectionSpecified() static void AssertFormFileCollection(Delegate handler, string expectedName) { // Arrange - var services = new ServiceCollection(); - var serviceProvider = services.BuildServiceProvider(); - var builder = new TestEndpointRouteBuilder(new ApplicationBuilder(serviceProvider)); + var builder = new TestEndpointRouteBuilder(new ApplicationBuilder(TestServiceProvider.Instance)); builder.MapPost("/file/upload", handler); var context = new ApiDescriptionProviderContext(Array.Empty()); @@ -1119,8 +1103,6 @@ static void AssertFormFileCollection(Delegate handler, string expectedName) } } -#nullable restore - [Fact] public void ProducesRouteInfoOnlyForRouteParameters() { @@ -1287,9 +1269,9 @@ private static IEnumerable GetSortedMediaTypes(ApiResponseType apiRespon private static IList GetApiDescriptions( Delegate action, - string pattern = null, - IEnumerable httpMethods = null, - string displayName = null) + string? pattern = null, + IEnumerable? httpMethods = null, + string? displayName = null) { var methodInfo = action.Method; var attributes = methodInfo.GetCustomAttributes(); @@ -1318,9 +1300,9 @@ private static IList GetApiDescriptions( new ServiceProviderIsService()); private static TestEndpointRouteBuilder CreateBuilder() => - new TestEndpointRouteBuilder(new ApplicationBuilder(new TestServiceProvider())); + new TestEndpointRouteBuilder(new ApplicationBuilder(TestServiceProvider.Instance)); - private static ApiDescription GetApiDescription(Delegate action, string pattern = null, string displayName = null, IEnumerable httpMethods = null) => + private static ApiDescription GetApiDescription(Delegate action, string? pattern = null, string displayName = null, IEnumerable? httpMethods = null) => Assert.Single(GetApiDescriptions(action, pattern, displayName: displayName, httpMethods: httpMethods)); private static void TestAction() @@ -1393,19 +1375,19 @@ public static bool TryParse(string value, out BindAsyncRecord result) => throw new NotImplementedException(); } - private record ArgumentListRecord([FromRoute] int Foo, int Bar, InferredJsonClass FromBody, HttpContext context); + private record ArgumentListRecord([FromRoute] int Foo, int Bar, InferredJsonClass? FromBody, HttpContext context); - private record struct ArgumentListRecordStruct([FromRoute] int Foo, int Bar, InferredJsonClass FromBody, HttpContext context); + private record struct ArgumentListRecordStruct([FromRoute] int Foo, int Bar, InferredJsonClass? FromBody, HttpContext context); - private record ArgumentListRecordWithoutAttributes(int Foo, int Bar, InferredJsonClass FromBody, HttpContext context); + private record ArgumentListRecordWithoutAttributes(int Foo, int Bar, InferredJsonClass? FromBody, HttpContext context); private record ArgumentListRecordWithoutPositionalParameters { [FromRoute] public int Foo { get; set; } public int Bar { get; set; } - public InferredJsonClass FromBody { get; set; } - public HttpContext Context { get; set; } + public InferredJsonClass? FromBody { get; set; } + public HttpContext Context { get; set; } = null!; } private class ArgumentListClass @@ -1413,8 +1395,8 @@ private class ArgumentListClass [FromRoute] public int Foo { get; set; } public int Bar { get; set; } - public InferredJsonClass FromBody { get; set; } - public HttpContext Context { get; set; } + public InferredJsonClass? FromBody { get; set; } + public HttpContext Context { get; set; } = null!; } private class ArgumentListClassWithReadOnlyProperties : ArgumentListClass @@ -1427,17 +1409,15 @@ private struct ArgumentListStruct [FromRoute] public int Foo { get; set; } public int Bar { get; set; } - public InferredJsonClass FromBody { get; set; } + public InferredJsonClass? FromBody { get; set; } public HttpContext Context { get; set; } } private class TestServiceProvider : IServiceProvider { - public void Dispose() - { - } + public static TestServiceProvider Instance { get; } = new TestServiceProvider(); - public object GetService(Type serviceType) + public object? GetService(Type serviceType) { if (serviceType == typeof(IOptions)) { diff --git a/src/OpenApi/src/OpenApiRouteHandlerBuilderExtensions.cs b/src/OpenApi/src/OpenApiRouteHandlerBuilderExtensions.cs index 83ce59fb6aa6..ed9faa65c5a0 100644 --- a/src/OpenApi/src/OpenApiRouteHandlerBuilderExtensions.cs +++ b/src/OpenApi/src/OpenApiRouteHandlerBuilderExtensions.cs @@ -79,13 +79,13 @@ private static void AddAndConfigureOperationForEndpoint(EndpointBuilder endpoint var pattern = routeEndpointBuilder.RoutePattern; var metadata = new EndpointMetadataCollection(routeEndpointBuilder.Metadata); var methodInfo = metadata.OfType().SingleOrDefault(); - var applicationServices = routeEndpointBuilder.ApplicationServices; - if (methodInfo is null || applicationServices is null) + if (methodInfo is null) { return; } + var applicationServices = routeEndpointBuilder.ApplicationServices; var hostEnvironment = applicationServices.GetService(); var serviceProviderIsService = applicationServices.GetService(); var generator = new OpenApiGenerator(hostEnvironment, serviceProviderIsService); diff --git a/src/OpenApi/test/OpenApiRouteHandlerBuilderExtensionTests.cs b/src/OpenApi/test/OpenApiRouteHandlerBuilderExtensionTests.cs index eae0866fc3fb..6c73d7563ef9 100644 --- a/src/OpenApi/test/OpenApiRouteHandlerBuilderExtensionTests.cs +++ b/src/OpenApi/test/OpenApiRouteHandlerBuilderExtensionTests.cs @@ -134,14 +134,26 @@ static void WithLocalSummary(RouteHandlerBuilder builder) WithLocalSummary(innerGroup.MapDelete("/inner-b", GetString)); - var summaries = builder.DataSources.SelectMany(ds => ds.Endpoints).Select(e => e.Metadata.GetMetadata().Summary).ToArray(); - - Assert.Equal(5, summaries.Length); - Assert.Contains(" | Local Summary | 200 Status Response Content-Type: text/plain", summaries); - Assert.Contains("Outer Group Summary | Local Summary | 200 Status Response Content-Type: text/plain", summaries); - Assert.Contains("Outer Group Summary | Local Summary | 200 Status Response Content-Type: text/plain", summaries); - Assert.Contains("Outer Group Summary | Inner Group Summary | Local Summary | 200 Status Response Content-Type: text/plain", summaries); - Assert.Contains("Outer Group Summary | Inner Group Summary | Local Summary | 200 Status Response Content-Type: text/plain", summaries); + var summaries = builder.DataSources + .SelectMany(ds => ds.Endpoints) + .ToDictionary( + e => ((RouteEndpoint)e).RoutePattern.RawText, + e => e.Metadata.GetMetadata().Summary); + + Assert.Equal(5, summaries.Count); + + Assert.Equal(" | Local Summary | 200 Status Response Content-Type: text/plain", + summaries["/root"]); + + Assert.Equal("Outer Group Summary | Local Summary | 200 Status Response Content-Type: text/plain", + summaries["/outer/outer-a"]); + Assert.Equal("Outer Group Summary | Local Summary | 200 Status Response Content-Type: text/plain", + summaries["/outer/outer-b"]); + + Assert.Equal("Outer Group Summary | Inner Group Summary | Local Summary | 200 Status Response Content-Type: text/plain", + summaries["/outer/inner/inner-a"]); + Assert.Equal("Outer Group Summary | Inner Group Summary | Local Summary | 200 Status Response Content-Type: text/plain", + summaries["/outer/inner/inner-b"]); } private RouteEndpointDataSource GetBuilderEndpointDataSource(IEndpointRouteBuilder endpointRouteBuilder) From 888f3c91432e4aa1a310538a3cd9e6044f13a235 Mon Sep 17 00:00:00 2001 From: Stephen Halter Date: Wed, 15 Jun 2022 22:07:11 -0700 Subject: [PATCH 05/21] Fix CompositeEndpointDataSource.HandleChange ordering - Always call user code outside of our lock. --- .../src/CompositeEndpointDataSource.cs | 68 ++++++++++++------- 1 file changed, 45 insertions(+), 23 deletions(-) diff --git a/src/Http/Routing/src/CompositeEndpointDataSource.cs b/src/Http/Routing/src/CompositeEndpointDataSource.cs index 4573ba092a74..4a4a65158971 100644 --- a/src/Http/Routing/src/CompositeEndpointDataSource.cs +++ b/src/Http/Routing/src/CompositeEndpointDataSource.cs @@ -104,26 +104,43 @@ public void Dispose() { // CompositeDataSource is registered as a singleton by default by AddRouting(). // UseEndpoints() adds all root data sources to this singleton. + List? disposables = null; + lock (_lock) { _disposed = true; - if (_changeTokenRegistrations is not null) + if (_dataSources is ObservableCollection observableDataSources) { - foreach (var registration in _changeTokenRegistrations) + observableDataSources.CollectionChanged -= OnDataSourcesChanged; + } + + foreach (var dataSource in _dataSources) + { + if (dataSource is IDisposable disposableDataSource) { - registration.Dispose(); + disposables ??= new List(); + disposables.Add(disposableDataSource); } } - if (_dataSources is ObservableCollection observableDataSources) + if (_changeTokenRegistrations is not null) { - observableDataSources.CollectionChanged -= OnDataSourcesChanged; + foreach (var registration in _changeTokenRegistrations) + { + disposables ??= new List(); + disposables.Add(registration); + } } + } - foreach (var dataSource in _dataSources) + // Dispose everything outside of the lock in case a registration is blocking on HandleChange completing + // on another thread or something. + if (disposables is not null) + { + foreach (var disposable in disposables) { - (dataSource as IDisposable)?.Dispose(); + disposable.Dispose(); } } } @@ -162,6 +179,8 @@ private void EnsureChangeTokenInitialized() private void HandleChange(bool collectionChanged) { + CancellationTokenSource? oldTokenSource = null; + lock (_lock) { if (_disposed) @@ -169,31 +188,34 @@ private void HandleChange(bool collectionChanged) return; } - // Don't update endpoints if no one has read them yet. - if (_endpoints is not null) - { - // Refresh the endpoints from datasource so that callbacks can get the latest endpoints - _endpoints = _dataSources.SelectMany(d => d.Endpoints).ToArray(); - } - - // Prevent consumers from re-registering callback to inflight events as that can - // cause a stackoverflow + // Prevent consumers from re-registering callback to in-flight events as that can + // cause a stack overflow. // Example: - // 1. B registers A - // 2. A fires event causing B's callback to get called + // 1. B registers A. + // 2. A fires event causing B's callback to get called. // 3. B executes some code in its callback, but needs to re-register callback - // in the same callback - var oldTokenSource = _cts; + // in the same callback. + oldTokenSource = _cts; // Don't create a new change token if no one is listening. if (oldTokenSource is not null) { + // We have to hook to any OnChange callbacks before caching endpoints, + // otherwise we might miss changes that occurred to one of the _dataSources after caching. CreateChangeTokenUnsynchronized(collectionChanged); - // Raise consumer callbacks. Any new callback registration would happen on the new token - // created in earlier step. - oldTokenSource.Cancel(); + } + + // Don't update endpoints if no one has read them yet. + if (_endpoints is not null) + { + // Refresh the endpoints from data source so that callbacks can get the latest endpoints. + _endpoints = _dataSources.SelectMany(d => d.Endpoints).ToArray(); } } + + // Raise consumer callbacks. Any new callback registration would happen on the new token + // created in earlier step. Avoid raising callbacks in a lock to avoid possible deadlocks. + oldTokenSource?.Cancel(); } [MemberNotNull(nameof(_consumerChangeToken))] From 59c9daf4f80086c47eba55d5e9aeaa5077423b5d Mon Sep 17 00:00:00 2001 From: Stephen Halter Date: Thu, 16 Jun 2022 17:40:24 -0700 Subject: [PATCH 06/21] Address PR review feedback --- .../WebApplicationTests.cs | 13 +++-- .../src/RequestDelegateFactory.cs | 12 ++--- .../src/RequestDelegateFactoryOptions.cs | 10 +++- src/Http/samples/MinimalSample/Program.cs | 49 ++++++++++++++----- 4 files changed, 56 insertions(+), 28 deletions(-) diff --git a/src/DefaultBuilder/test/Microsoft.AspNetCore.Tests/WebApplicationTests.cs b/src/DefaultBuilder/test/Microsoft.AspNetCore.Tests/WebApplicationTests.cs index 1ae68e9aa770..52c757f10f70 100644 --- a/src/DefaultBuilder/test/Microsoft.AspNetCore.Tests/WebApplicationTests.cs +++ b/src/DefaultBuilder/test/Microsoft.AspNetCore.Tests/WebApplicationTests.cs @@ -1572,13 +1572,12 @@ public async Task BranchingPipelineHasOwnRoutes() app.Start(); var ds = app.Services.GetRequiredService(); - var displayNames = ds.Endpoints.Select(e => e.DisplayName).ToArray(); - Assert.Equal(5, displayNames.Length); - Assert.Contains("One", displayNames); - Assert.Contains("Two", displayNames); - Assert.Contains("Three", displayNames); - Assert.Contains("Four", displayNames); - Assert.Contains("Five", displayNames); + Assert.Equal(5, ds.Endpoints.Count); + Assert.Equal("One", ds.Endpoints[0].DisplayName); + Assert.Equal("Two", ds.Endpoints[1].DisplayName); + Assert.Equal("Three", ds.Endpoints[2].DisplayName); + Assert.Equal("Four", ds.Endpoints[3].DisplayName); + Assert.Equal("Five", ds.Endpoints[4].DisplayName); var client = app.GetTestClient(); diff --git a/src/Http/Http.Extensions/src/RequestDelegateFactory.cs b/src/Http/Http.Extensions/src/RequestDelegateFactory.cs index cd52459f203b..140fef7ea49e 100644 --- a/src/Http/Http.Extensions/src/RequestDelegateFactory.cs +++ b/src/Http/Http.Extensions/src/RequestDelegateFactory.cs @@ -133,7 +133,7 @@ public static RequestDelegateResult Create(Delegate handler, RequestDelegateFact var targetableRequestDelegate = CreateTargetableRequestDelegate(handler.Method, targetExpression, factoryContext, targetFactory); - return new RequestDelegateResult(httpContext => targetableRequestDelegate(handler.Target, httpContext), GetMetadataList(factoryContext.Metadata)); + return new RequestDelegateResult(httpContext => targetableRequestDelegate(handler.Target, httpContext), AsReadOnlyList(factoryContext.Metadata)); } /// @@ -165,7 +165,7 @@ public static RequestDelegateResult Create(MethodInfo methodInfo, Func untargetableRequestDelegate(null, httpContext), GetMetadataList(factoryContext.Metadata)); + return new RequestDelegateResult(httpContext => untargetableRequestDelegate(null, httpContext), AsReadOnlyList(factoryContext.Metadata)); } targetFactory = context => Activator.CreateInstance(methodInfo.DeclaringType)!; @@ -174,7 +174,7 @@ public static RequestDelegateResult Create(MethodInfo methodInfo, Func targetFactory(context)); - return new RequestDelegateResult(httpContext => targetableRequestDelegate(targetFactory(httpContext), httpContext), GetMetadataList(factoryContext.Metadata)); + return new RequestDelegateResult(httpContext => targetableRequestDelegate(targetFactory(httpContext), httpContext), AsReadOnlyList(factoryContext.Metadata)); } private static FactoryContext CreateFactoryContext(RequestDelegateFactoryOptions? options) @@ -191,11 +191,11 @@ private static FactoryContext CreateFactoryContext(RequestDelegateFactoryOptions }; } - private static List GetMetadataList(IList metadata) + private static IReadOnlyList AsReadOnlyList(IList metadata) { - if (metadata is List normalList) + if (metadata is IReadOnlyList readOnlyList) { - return normalList; + return readOnlyList; } return new List(metadata); diff --git a/src/Http/Http.Extensions/src/RequestDelegateFactoryOptions.cs b/src/Http/Http.Extensions/src/RequestDelegateFactoryOptions.cs index 03c02a8a81cf..8e3edccd36ae 100644 --- a/src/Http/Http.Extensions/src/RequestDelegateFactoryOptions.cs +++ b/src/Http/Http.Extensions/src/RequestDelegateFactoryOptions.cs @@ -39,10 +39,16 @@ public sealed class RequestDelegateFactoryOptions public IReadOnlyList>? RouteHandlerFilterFactories { get; init; } /// - /// The to use for the creation of the . + /// The mutable initial endpoint metadata to add as part of the creation of the . In most cases, + /// this should come from . /// /// - /// This metadata will be returned as if it is non-null. + /// This metadata will be included in before most metadata inferred during creation of the + /// and before any metadata provided by types in the delegate signature that implement + /// or . The exception to this general rule is the + /// that infers automatically + /// without any custom metadata providers which instead is inserted at the start to give it lower precedence. Custom metadata providers can choose to do + /// insert their metadata at the start to give lower precedence, but this is unusual. /// public IList? EndpointMetadata { get; init; } } diff --git a/src/Http/samples/MinimalSample/Program.cs b/src/Http/samples/MinimalSample/Program.cs index a15f67986088..a1de728ac654 100644 --- a/src/Http/samples/MinimalSample/Program.cs +++ b/src/Http/samples/MinimalSample/Program.cs @@ -2,17 +2,12 @@ // The .NET Foundation licenses this file to you under the MIT license. using Microsoft.AspNetCore.Http.HttpResults; +using Microsoft.AspNetCore.Http.Metadata; using Microsoft.AspNetCore.Mvc; var builder = WebApplication.CreateBuilder(args); - var app = builder.Build(); -if (app.Environment.IsDevelopment()) -{ - app.UseDeveloperExceptionPage(); -} - string Plaintext() => "Hello, World!"; app.MapGet("/plaintext", Plaintext); @@ -23,15 +18,43 @@ Date and Time: {DateTime.Now} """); -var nestedGroup = app.MapGroup("/group/{groupName}") +var outer = app.MapGroup("/outer"); +var inner = outer.MapGroup("/inner"); + +inner.AddRouteHandlerFilter((routeContext, next) => +{ + var tags = routeContext.EndpointMetadata.OfType().FirstOrDefault(); + + return async invocationContext => + { + Console.WriteLine("Running filter!"); + var result = await next(invocationContext); + return ((string)result) + " | /inner filter! Tags:" + tags is null ? "(null)" : string.Join(", ", tags.Tags); + }; +}); + +outer.MapGet("/outerget", () => "I'm nested."); +inner.MapGet("/innerget", () => "I'm more nested."); + +inner.AddRouteHandlerFilter((routeContext, next) => +{ + Console.WriteLine($"Building filter! Num args: {routeContext.MethodInfo.GetParameters().Length}"); ; + return async invocationContext => + { + Console.WriteLine("Running filter!"); + var result = await next(invocationContext); + return ((string)result) + "| nested filter!"; + }; +}); + +var superNested = inner.MapGroup("/group/{groupName}") .MapGroup("/nested/{nestedName}") - .WithTags("nested"); + .WithTags("nested", "more", "tags"); -nestedGroup - .MapGet("/", (string groupName, string nestedName) => - { - return $"Hello from {groupName}:{nestedName}!"; - }); +superNested.MapGet("/", (string groupName, string nestedName) => +{ + return $"Hello from {groupName}:{nestedName}!"; +}); object Json() => new { message = "Hello, World!" }; app.MapGet("/json", Json).WithTags("json"); From b9864444b4c092c7ffbf93bac99c36daa908d428 Mon Sep 17 00:00:00 2001 From: Stephen Halter Date: Thu, 16 Jun 2022 18:08:52 -0700 Subject: [PATCH 07/21] Address more PR feedback --- src/Http/Http.Abstractions/src/RequestDelegateResult.cs | 4 ++-- src/Http/Routing/src/Builder/RouteHandlerFilterExtensions.cs | 4 ++-- src/Http/Routing/src/RouteEndpointDataSource.cs | 2 +- 3 files changed, 5 insertions(+), 5 deletions(-) diff --git a/src/Http/Http.Abstractions/src/RequestDelegateResult.cs b/src/Http/Http.Abstractions/src/RequestDelegateResult.cs index 70d2eb956176..cadfb30efe09 100644 --- a/src/Http/Http.Abstractions/src/RequestDelegateResult.cs +++ b/src/Http/Http.Abstractions/src/RequestDelegateResult.cs @@ -23,8 +23,8 @@ public RequestDelegateResult(RequestDelegate requestDelegate, IReadOnlyList - /// Gets endpoint metadata inferred from creating the . This does not contain - /// anything from RequestDelegateFactoryOptions.InitialEndpointMetadata. + /// Gets endpoint metadata inferred from creating the . If a non-null + /// RequestDelegateFactoryOptions.EndpointMetadata list was passed in, this will be the same instance. /// public IReadOnlyList EndpointMetadata { get; } } diff --git a/src/Http/Routing/src/Builder/RouteHandlerFilterExtensions.cs b/src/Http/Routing/src/Builder/RouteHandlerFilterExtensions.cs index 8c03051e828b..548de1c33464 100644 --- a/src/Http/Routing/src/Builder/RouteHandlerFilterExtensions.cs +++ b/src/Http/Routing/src/Builder/RouteHandlerFilterExtensions.cs @@ -87,7 +87,7 @@ public static TBuilder AddRouteHandlerFilter(this TBuilder builder, IR /// Registers a filter given a delegate onto the route handler. /// /// The . - /// A representing the core logic of the filter. + /// A method representing the core logic of the filter. /// A that can be used to further customize the route handler. public static TBuilder AddRouteHandlerFilter(this TBuilder builder, Func> routeHandlerFilter) where TBuilder : IEndpointConventionBuilder @@ -99,7 +99,7 @@ public static TBuilder AddRouteHandlerFilter(this TBuilder builder, Fu /// Register a filter given a delegate representing the filter factory. /// /// The . - /// A representing the logic for constructing the filter. + /// A method representing the logic for constructing the filter. /// A that can be used to further customize the route handler. public static TBuilder AddRouteHandlerFilter(this TBuilder builder, Func filterFactory) where TBuilder : IEndpointConventionBuilder diff --git a/src/Http/Routing/src/RouteEndpointDataSource.cs b/src/Http/Routing/src/RouteEndpointDataSource.cs index e2a0c0699f3d..08588e65f899 100644 --- a/src/Http/Routing/src/RouteEndpointDataSource.cs +++ b/src/Http/Routing/src/RouteEndpointDataSource.cs @@ -202,7 +202,7 @@ private struct RouteEntry } // This private class is only exposed to internal code via ICollection> in RouteEndpointBuilder where only Add is called. - private class ThrowOnAddAfterBuildCollection : List>, ICollection> + private sealed class ThrowOnAddAfterBuildCollection : List>, ICollection> { public bool HasBeenBuilt { get; set; } From 1c5feef7f549e7bbf8c5c44a8724de30815864c0 Mon Sep 17 00:00:00 2001 From: Stephen Halter Date: Thu, 16 Jun 2022 18:40:48 -0700 Subject: [PATCH 08/21] Move MapMethods logic into RouteEndpointDataSource --- .../Builder/EndpointRouteBuilderExtensions.cs | 37 ++----------- .../Routing/src/RouteEndpointDataSource.cs | 53 +++++++++++++++---- .../test/UnitTests/Builder/GroupTest.cs | 4 +- 3 files changed, 49 insertions(+), 45 deletions(-) diff --git a/src/Http/Routing/src/Builder/EndpointRouteBuilderExtensions.cs b/src/Http/Routing/src/Builder/EndpointRouteBuilderExtensions.cs index f4c56b7b7391..e5c87f820255 100644 --- a/src/Http/Routing/src/Builder/EndpointRouteBuilderExtensions.cs +++ b/src/Http/Routing/src/Builder/EndpointRouteBuilderExtensions.cs @@ -331,35 +331,7 @@ public static RouteHandlerBuilder MapMethods( Delegate handler) { ArgumentNullException.ThrowIfNull(httpMethods); - - var disableInferredBody = false; - foreach (var method in httpMethods) - { - disableInferredBody = ShouldDisableInferredBody(method); - if (disableInferredBody is true) - { - break; - } - } - - var initialMetadata = new object[] { new HttpMethodMetadata(httpMethods) }; - var builder = endpoints.Map(RoutePatternFactory.Parse(pattern), handler, disableInferredBody, initialMetadata); - - // Prepends the HTTP method to the DisplayName produced with pattern + method name - builder.Add(b => b.DisplayName = $"HTTP: {string.Join(", ", httpMethods)} {b.DisplayName}"); - - return builder; - - static bool ShouldDisableInferredBody(string method) - { - // GET, DELETE, HEAD, CONNECT, TRACE, and OPTIONS normally do not contain bodies - return method.Equals(HttpMethods.Get, StringComparison.Ordinal) || - method.Equals(HttpMethods.Delete, StringComparison.Ordinal) || - method.Equals(HttpMethods.Head, StringComparison.Ordinal) || - method.Equals(HttpMethods.Options, StringComparison.Ordinal) || - method.Equals(HttpMethods.Trace, StringComparison.Ordinal) || - method.Equals(HttpMethods.Connect, StringComparison.Ordinal); - } + return endpoints.Map(RoutePatternFactory.Parse(pattern), handler, httpMethods); } /// @@ -393,7 +365,7 @@ public static RouteHandlerBuilder Map( RoutePattern pattern, Delegate handler) { - return Map(endpoints, pattern, handler, disableInferBodyFromParameters: false); + return Map(endpoints, pattern, handler, httpMethods: null); } /// @@ -466,8 +438,7 @@ private static RouteHandlerBuilder Map( this IEndpointRouteBuilder endpoints, RoutePattern pattern, Delegate handler, - bool disableInferBodyFromParameters, - IEnumerable? initialEndpointMetadata = null) + IEnumerable? httpMethods = null) { ArgumentNullException.ThrowIfNull(endpoints); ArgumentNullException.ThrowIfNull(pattern); @@ -483,7 +454,7 @@ private static RouteHandlerBuilder Map( endpoints.DataSources.Add(dataSource); } - var conventions = dataSource.AddEndpoint(pattern, handler, initialEndpointMetadata, disableInferBodyFromParameters); + var conventions = dataSource.AddEndpoint(pattern, handler, httpMethods); return new RouteHandlerBuilder(conventions); } diff --git a/src/Http/Routing/src/RouteEndpointDataSource.cs b/src/Http/Routing/src/RouteEndpointDataSource.cs index 08588e65f899..43db8daabef6 100644 --- a/src/Http/Routing/src/RouteEndpointDataSource.cs +++ b/src/Http/Routing/src/RouteEndpointDataSource.cs @@ -28,16 +28,14 @@ public RouteEndpointDataSource(IServiceProvider applicationServices, bool throwO public ICollection> AddEndpoint( RoutePattern pattern, Delegate routeHandler, - IEnumerable? initialEndpointMetadata, - bool disableInferFromBodyParameters) + IEnumerable? httpMethods) { RouteEntry entry = new() { RoutePattern = pattern, RouteHandler = routeHandler, - InitialEndpointMetadata = initialEndpointMetadata, - DisableInferFromBodyParameters = disableInferFromBodyParameters, - Conventions = new() + HttpMethods = httpMethods, + Conventions = new(), }; _routeEntries.Add(entry); @@ -99,6 +97,12 @@ private RouteEndpointBuilder CreateRouteEndpointBuilder(RouteEntry entry, RouteP displayName = $"{displayName} => {endpointName}"; } + if (entry.HttpMethods is not null) + { + // Prepends the HTTP method to the DisplayName produced with pattern + method name + displayName = $"HTTP: {string.Join(", ", entry.HttpMethods)} {displayName}"; + } + RequestDelegate? factoryCreatedRequestDelegate = null; RequestDelegate redirectedRequestDelegate = context => { @@ -123,9 +127,9 @@ private RouteEndpointBuilder CreateRouteEndpointBuilder(RouteEntry entry, RouteP // Add MethodInfo as first metadata item builder.Metadata.Add(handler.Method); - if (entry.InitialEndpointMetadata is not null) + if (entry.HttpMethods is not null) { - metadata.AddRange(entry.InitialEndpointMetadata); + builder.Metadata.Add(new HttpMethodMetadata(entry.HttpMethods)); } // Apply group conventions before entry-specific conventions added to the RouteHandlerBuilder. @@ -174,7 +178,7 @@ private RouteEndpointBuilder CreateRouteEndpointBuilder(RouteEntry entry, RouteP ServiceProvider = _applicationServices, RouteParameterNames = routeParamNames, ThrowOnBadRequest = _throwOnBadRequest, - DisableInferBodyFromParameters = entry.DisableInferFromBodyParameters, + DisableInferBodyFromParameters = ShouldDisableInferredBodyParameters(entry.HttpMethods), EndpointMetadata = metadata, RouteHandlerFilterFactories = routeHandlerFilterFactories, }; @@ -192,12 +196,41 @@ private RouteEndpointBuilder CreateRouteEndpointBuilder(RouteEntry entry, RouteP return builder; } + private static bool ShouldDisableInferredBodyParameters(IEnumerable? httpMethods) + { + static bool ShouldDisableInferredBodyForMethod(string method) => + // GET, DELETE, HEAD, CONNECT, TRACE, and OPTIONS normally do not contain bodies + method.Equals(HttpMethods.Get, StringComparison.Ordinal) || + method.Equals(HttpMethods.Delete, StringComparison.Ordinal) || + method.Equals(HttpMethods.Head, StringComparison.Ordinal) || + method.Equals(HttpMethods.Options, StringComparison.Ordinal) || + method.Equals(HttpMethods.Trace, StringComparison.Ordinal) || + method.Equals(HttpMethods.Connect, StringComparison.Ordinal); + + // If the endpoint accepts any kind of request, we should still infer parameters can come from the body. + if (httpMethods is null) + { + return false; + } + + foreach (var method in httpMethods) + { + if (ShouldDisableInferredBodyForMethod(method)) + { + // If the route handler was mapped explicitly to handle an HTTP method that does not normally have a request body, + // we assume any invocation of the handler will not have a request body no matter what other HTTP methods it may support. + return true; + } + } + + return false; + } + private struct RouteEntry { public RoutePattern RoutePattern { get; init; } public Delegate RouteHandler { get; init; } - public IEnumerable? InitialEndpointMetadata { get; init; } - public bool DisableInferFromBodyParameters { get; init; } + public IEnumerable? HttpMethods { get; init; } public ThrowOnAddAfterBuildCollection Conventions { get; init; } } diff --git a/src/Http/Routing/test/UnitTests/Builder/GroupTest.cs b/src/Http/Routing/test/UnitTests/Builder/GroupTest.cs index 3030dd8749a0..9fb1cffe9283 100644 --- a/src/Http/Routing/test/UnitTests/Builder/GroupTest.cs +++ b/src/Http/Routing/test/UnitTests/Builder/GroupTest.cs @@ -275,7 +275,7 @@ public async Task ChangingMostEndpointBuilderPropertiesInConvention_Works() ((IEndpointConventionBuilder)group).Add(builder => { - builder.DisplayName = "Replaced!"; + builder.DisplayName = $"Prefixed! {builder.DisplayName}"; builder.RequestDelegate = ctx => { replacementCalled = true; @@ -294,7 +294,7 @@ public async Task ChangingMostEndpointBuilderPropertiesInConvention_Works() Assert.False(mapGetCalled); Assert.True(replacementCalled); - Assert.Equal("HTTP: GET Replaced!", endpoint.DisplayName); + Assert.Equal("Prefixed! HTTP: GET /group/", endpoint.DisplayName); var routeEndpoint = Assert.IsType(endpoint); Assert.Equal(42, routeEndpoint.Order); From 539097db0e40212674b073902623dc8699771fd7 Mon Sep 17 00:00:00 2001 From: Stephen Halter Date: Thu, 16 Jun 2022 18:49:46 -0700 Subject: [PATCH 09/21] Fix internal doc comment cref --- src/Http/Routing/src/Builder/RouteHandlerBuilder.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Http/Routing/src/Builder/RouteHandlerBuilder.cs b/src/Http/Routing/src/Builder/RouteHandlerBuilder.cs index 261189b7dd77..2a28657b7a57 100644 --- a/src/Http/Routing/src/Builder/RouteHandlerBuilder.cs +++ b/src/Http/Routing/src/Builder/RouteHandlerBuilder.cs @@ -17,7 +17,7 @@ public sealed class RouteHandlerBuilder : IEndpointConventionBuilder /// Instantiates a new given a single /// . /// - /// The convention list returned from . + /// The convention list returned from . internal RouteHandlerBuilder(ICollection> conventions) { _conventions = conventions; From 3da213698b39898852724fe803e9ff3165bc750c Mon Sep 17 00:00:00 2001 From: Stephen Halter Date: Thu, 16 Jun 2022 19:03:25 -0700 Subject: [PATCH 10/21] Move MapFallback logic to RouteEndpointDataSource too --- .../Builder/EndpointRouteBuilderExtensions.cs | 14 ++++++-------- .../src/Builder/RouteHandlerBuilder.cs | 5 ++--- .../Routing/src/RouteEndpointDataSource.cs | 19 +++++++++++++++---- 3 files changed, 23 insertions(+), 15 deletions(-) diff --git a/src/Http/Routing/src/Builder/EndpointRouteBuilderExtensions.cs b/src/Http/Routing/src/Builder/EndpointRouteBuilderExtensions.cs index dbfe2bf6e6c0..85cab4a767e1 100644 --- a/src/Http/Routing/src/Builder/EndpointRouteBuilderExtensions.cs +++ b/src/Http/Routing/src/Builder/EndpointRouteBuilderExtensions.cs @@ -331,7 +331,7 @@ public static RouteHandlerBuilder MapMethods( Delegate handler) { ArgumentNullException.ThrowIfNull(httpMethods); - return endpoints.Map(RoutePatternFactory.Parse(pattern), handler, httpMethods); + return endpoints.Map(RoutePatternFactory.Parse(pattern), handler, httpMethods, isFallback: false); } /// @@ -365,7 +365,7 @@ public static RouteHandlerBuilder Map( RoutePattern pattern, Delegate handler) { - return Map(endpoints, pattern, handler, httpMethods: null); + return Map(endpoints, pattern, handler, httpMethods: null, isFallback: false); } /// @@ -427,10 +427,7 @@ public static RouteHandlerBuilder MapFallback( ArgumentNullException.ThrowIfNull(pattern); ArgumentNullException.ThrowIfNull(handler); - var conventionBuilder = endpoints.Map(pattern, handler); - conventionBuilder.WithDisplayName("Fallback " + pattern); - conventionBuilder.Add(b => ((RouteEndpointBuilder)b).Order = int.MaxValue); - return conventionBuilder; + return endpoints.Map(RoutePatternFactory.Parse(pattern), handler, httpMethods: null, isFallback: true); } [RequiresUnreferencedCode(MapEndpointTrimmerWarning)] @@ -438,7 +435,8 @@ private static RouteHandlerBuilder Map( this IEndpointRouteBuilder endpoints, RoutePattern pattern, Delegate handler, - IEnumerable? httpMethods = null) + IEnumerable? httpMethods, + bool isFallback) { ArgumentNullException.ThrowIfNull(endpoints); ArgumentNullException.ThrowIfNull(pattern); @@ -454,7 +452,7 @@ private static RouteHandlerBuilder Map( endpoints.DataSources.Add(dataSource); } - var conventions = dataSource.AddEndpoint(pattern, handler, httpMethods); + var conventions = dataSource.AddEndpoint(pattern, handler, httpMethods, isFallback); return new RouteHandlerBuilder(conventions); } diff --git a/src/Http/Routing/src/Builder/RouteHandlerBuilder.cs b/src/Http/Routing/src/Builder/RouteHandlerBuilder.cs index 2a28657b7a57..11baeb88947e 100644 --- a/src/Http/Routing/src/Builder/RouteHandlerBuilder.cs +++ b/src/Http/Routing/src/Builder/RouteHandlerBuilder.cs @@ -14,10 +14,9 @@ public sealed class RouteHandlerBuilder : IEndpointConventionBuilder private readonly ICollection>? _conventions; /// - /// Instantiates a new given a single - /// . + /// Instantiates a new given a ThrowOnAddAfterBuildCollection from . /// - /// The convention list returned from . + /// The convention list returned from . internal RouteHandlerBuilder(ICollection> conventions) { _conventions = conventions; diff --git a/src/Http/Routing/src/RouteEndpointDataSource.cs b/src/Http/Routing/src/RouteEndpointDataSource.cs index 43db8daabef6..126637e87164 100644 --- a/src/Http/Routing/src/RouteEndpointDataSource.cs +++ b/src/Http/Routing/src/RouteEndpointDataSource.cs @@ -28,14 +28,16 @@ public RouteEndpointDataSource(IServiceProvider applicationServices, bool throwO public ICollection> AddEndpoint( RoutePattern pattern, Delegate routeHandler, - IEnumerable? httpMethods) + IEnumerable? httpMethods, + bool isFallback) { RouteEntry entry = new() { RoutePattern = pattern, RouteHandler = routeHandler, HttpMethods = httpMethods, - Conventions = new(), + IsFallback = isFallback, + Conventions = new ThrowOnAddAfterBuildCollection(), }; _routeEntries.Add(entry); @@ -103,6 +105,11 @@ private RouteEndpointBuilder CreateRouteEndpointBuilder(RouteEntry entry, RouteP displayName = $"HTTP: {string.Join(", ", entry.HttpMethods)} {displayName}"; } + if (entry.IsFallback) + { + displayName = $"Fallback {displayName}"; + } + RequestDelegate? factoryCreatedRequestDelegate = null; RequestDelegate redirectedRequestDelegate = context => { @@ -114,8 +121,11 @@ private RouteEndpointBuilder CreateRouteEndpointBuilder(RouteEntry entry, RouteP return factoryCreatedRequestDelegate(context); }; - // The Map methods don't support customizing the order, so we always use the default of 0 unless a convention changes it later. - var builder = new RouteEndpointBuilder(redirectedRequestDelegate, pattern, order: 0) + // The Map methods don't support customizing the order apart from using int.MaxValue to give MapFallback the lowest precedences. + // Otherwise, we always use the default of 0 unless a convention changes it later. + var order = entry.IsFallback ? int.MaxValue : 0; + + var builder = new RouteEndpointBuilder(redirectedRequestDelegate, pattern, order) { DisplayName = displayName, ApplicationServices = _applicationServices, @@ -231,6 +241,7 @@ private struct RouteEntry public RoutePattern RoutePattern { get; init; } public Delegate RouteHandler { get; init; } public IEnumerable? HttpMethods { get; init; } + public bool IsFallback { get; init; } public ThrowOnAddAfterBuildCollection Conventions { get; init; } } From 4e88138d9497b79e3604fee98e688fae0295cac2 Mon Sep 17 00:00:00 2001 From: Stephen Halter Date: Thu, 16 Jun 2022 19:19:46 -0700 Subject: [PATCH 11/21] GetGroupedEndpoints -> GetEndpointGroup --- src/Http/Routing/src/CompositeEndpointDataSource.cs | 4 ++-- src/Http/Routing/src/EndpointDataSource.cs | 2 +- src/Http/Routing/src/PublicAPI.Unshipped.txt | 4 ++-- src/Http/Routing/src/RouteEndpointDataSource.cs | 3 ++- src/Http/Routing/src/RouteGroupBuilder.cs | 6 +++--- src/Http/Routing/src/RouteGroupContext.cs | 4 ++-- .../test/UnitTests/CompositeEndpointDataSourceTest.cs | 8 ++++---- 7 files changed, 16 insertions(+), 15 deletions(-) diff --git a/src/Http/Routing/src/CompositeEndpointDataSource.cs b/src/Http/Routing/src/CompositeEndpointDataSource.cs index 4a4a65158971..37b7cb94048b 100644 --- a/src/Http/Routing/src/CompositeEndpointDataSource.cs +++ b/src/Http/Routing/src/CompositeEndpointDataSource.cs @@ -77,7 +77,7 @@ public override IReadOnlyList Endpoints } /// - public override IReadOnlyList GetGroupedEndpoints(RouteGroupContext context) + public override IReadOnlyList GetEndpointGroup(RouteGroupContext context) { if (_dataSources.Count is 0) { @@ -91,7 +91,7 @@ public override IReadOnlyList GetGroupedEndpoints(RouteGroupContext co foreach (var dataSource in _dataSources) { - groupedEndpoints.AddRange(dataSource.GetGroupedEndpoints(context)); + groupedEndpoints.AddRange(dataSource.GetEndpointGroup(context)); } // There's no need to cache these the way we do with _endpoints. This is only ever used to get intermediate results. diff --git a/src/Http/Routing/src/EndpointDataSource.cs b/src/Http/Routing/src/EndpointDataSource.cs index 71cc91a4a281..ba93b649fa6f 100644 --- a/src/Http/Routing/src/EndpointDataSource.cs +++ b/src/Http/Routing/src/EndpointDataSource.cs @@ -32,7 +32,7 @@ public abstract class EndpointDataSource /// /// Returns a read-only collection of instances given the specified group and . /// - public virtual IReadOnlyList GetGroupedEndpoints(RouteGroupContext context) + public virtual IReadOnlyList GetEndpointGroup(RouteGroupContext context) { // Only evaluate Endpoints once per call. var endpoints = Endpoints; diff --git a/src/Http/Routing/src/PublicAPI.Unshipped.txt b/src/Http/Routing/src/PublicAPI.Unshipped.txt index 042aa1893f5d..84250dfd97f8 100644 --- a/src/Http/Routing/src/PublicAPI.Unshipped.txt +++ b/src/Http/Routing/src/PublicAPI.Unshipped.txt @@ -9,7 +9,7 @@ Microsoft.AspNetCore.Routing.RouteGroupContext.Prefix.get -> Microsoft.AspNetCor Microsoft.AspNetCore.Routing.RouteGroupContext.RouteGroupContext(Microsoft.AspNetCore.Routing.Patterns.RoutePattern! prefix, System.Collections.Generic.IReadOnlyList!>! conventions, System.IServiceProvider! applicationServices) -> void Microsoft.AspNetCore.Routing.RouteOptions.SetParameterPolicy(string! token, System.Type! type) -> void Microsoft.AspNetCore.Routing.RouteOptions.SetParameterPolicy(string! token) -> void -override Microsoft.AspNetCore.Routing.CompositeEndpointDataSource.GetGroupedEndpoints(Microsoft.AspNetCore.Routing.RouteGroupContext! context) -> System.Collections.Generic.IReadOnlyList! +override Microsoft.AspNetCore.Routing.CompositeEndpointDataSource.GetEndpointGroup(Microsoft.AspNetCore.Routing.RouteGroupContext! context) -> System.Collections.Generic.IReadOnlyList! static Microsoft.AspNetCore.Builder.EndpointRouteBuilderExtensions.MapGroup(this Microsoft.AspNetCore.Routing.IEndpointRouteBuilder! endpoints, Microsoft.AspNetCore.Routing.Patterns.RoutePattern! prefix) -> Microsoft.AspNetCore.Routing.RouteGroupBuilder! static Microsoft.AspNetCore.Builder.EndpointRouteBuilderExtensions.MapGroup(this Microsoft.AspNetCore.Routing.IEndpointRouteBuilder! endpoints, string! prefix) -> Microsoft.AspNetCore.Routing.RouteGroupBuilder! static Microsoft.AspNetCore.Builder.EndpointRouteBuilderExtensions.MapPatch(this Microsoft.AspNetCore.Routing.IEndpointRouteBuilder! endpoints, string! pattern, System.Delegate! handler) -> Microsoft.AspNetCore.Builder.RouteHandlerBuilder! @@ -39,5 +39,5 @@ static Microsoft.AspNetCore.Routing.Patterns.RoutePatternFactory.Pattern(Microso static Microsoft.AspNetCore.Routing.Patterns.RoutePatternFactory.Pattern(Microsoft.AspNetCore.Routing.RouteValueDictionary? defaults, Microsoft.AspNetCore.Routing.RouteValueDictionary? parameterPolicies, params Microsoft.AspNetCore.Routing.Patterns.RoutePatternPathSegment![]! segments) -> Microsoft.AspNetCore.Routing.Patterns.RoutePattern! static Microsoft.AspNetCore.Routing.Patterns.RoutePatternFactory.Pattern(string? rawText, Microsoft.AspNetCore.Routing.RouteValueDictionary? defaults, Microsoft.AspNetCore.Routing.RouteValueDictionary? parameterPolicies, System.Collections.Generic.IEnumerable! segments) -> Microsoft.AspNetCore.Routing.Patterns.RoutePattern! static Microsoft.AspNetCore.Routing.Patterns.RoutePatternFactory.Pattern(string? rawText, Microsoft.AspNetCore.Routing.RouteValueDictionary? defaults, Microsoft.AspNetCore.Routing.RouteValueDictionary? parameterPolicies, params Microsoft.AspNetCore.Routing.Patterns.RoutePatternPathSegment![]! segments) -> Microsoft.AspNetCore.Routing.Patterns.RoutePattern! -virtual Microsoft.AspNetCore.Routing.EndpointDataSource.GetGroupedEndpoints(Microsoft.AspNetCore.Routing.RouteGroupContext! context) -> System.Collections.Generic.IReadOnlyList! +virtual Microsoft.AspNetCore.Routing.EndpointDataSource.GetEndpointGroup(Microsoft.AspNetCore.Routing.RouteGroupContext! context) -> System.Collections.Generic.IReadOnlyList! virtual Microsoft.AspNetCore.Routing.Patterns.RoutePatternTransformer.SubstituteRequiredValues(Microsoft.AspNetCore.Routing.Patterns.RoutePattern! original, Microsoft.AspNetCore.Routing.RouteValueDictionary! requiredValues) -> Microsoft.AspNetCore.Routing.Patterns.RoutePattern? diff --git a/src/Http/Routing/src/RouteEndpointDataSource.cs b/src/Http/Routing/src/RouteEndpointDataSource.cs index 126637e87164..6f08c693a351 100644 --- a/src/Http/Routing/src/RouteEndpointDataSource.cs +++ b/src/Http/Routing/src/RouteEndpointDataSource.cs @@ -58,7 +58,8 @@ public override IReadOnlyList Endpoints } } - public override IReadOnlyList GetGroupedEndpoints(RouteGroupContext context) + // I just want to point out that we're using fancy covariant returns introduced by C# 9! Not that it matters here :D + public override IReadOnlyList GetEndpointGroup(RouteGroupContext context) { var endpoints = new List(_routeEntries.Count); foreach (var entry in _routeEntries) diff --git a/src/Http/Routing/src/RouteGroupBuilder.cs b/src/Http/Routing/src/RouteGroupBuilder.cs index b72af091c792..169dd1bc7c1e 100644 --- a/src/Http/Routing/src/RouteGroupBuilder.cs +++ b/src/Http/Routing/src/RouteGroupBuilder.cs @@ -53,7 +53,7 @@ public GroupEndpointDataSource(RouteGroupBuilder groupRouteBuilder) public override IReadOnlyList Endpoints => GetGroupedEndpointsWithNullablePrefix(null, Array.Empty>(), _routeGroupBuilder._outerEndpointRouteBuilder.ServiceProvider); - public override IReadOnlyList GetGroupedEndpoints(RouteGroupContext context) => + public override IReadOnlyList GetEndpointGroup(RouteGroupContext context) => GetGroupedEndpointsWithNullablePrefix(context.Prefix, context.Conventions, context.ApplicationServices); public IReadOnlyList GetGroupedEndpointsWithNullablePrefix(RoutePattern? prefix, IReadOnlyList> conventions, IServiceProvider applicationServices) @@ -69,14 +69,14 @@ public IReadOnlyList GetGroupedEndpointsWithNullablePrefix(RoutePatter if (_routeGroupBuilder._dataSources.Count is 1) { - return _routeGroupBuilder._dataSources[0].GetGroupedEndpoints(new RouteGroupContext(fullPrefix, combinedConventions, applicationServices)); + return _routeGroupBuilder._dataSources[0].GetEndpointGroup(new RouteGroupContext(fullPrefix, combinedConventions, applicationServices)); } var groupedEndpoints = new List(); foreach (var dataSource in _routeGroupBuilder._dataSources) { - groupedEndpoints.AddRange(dataSource.GetGroupedEndpoints(new RouteGroupContext(fullPrefix, combinedConventions, applicationServices))); + groupedEndpoints.AddRange(dataSource.GetEndpointGroup(new RouteGroupContext(fullPrefix, combinedConventions, applicationServices))); } return groupedEndpoints; diff --git a/src/Http/Routing/src/RouteGroupContext.cs b/src/Http/Routing/src/RouteGroupContext.cs index 0afbc08bcbb1..c0e879cc26b3 100644 --- a/src/Http/Routing/src/RouteGroupContext.cs +++ b/src/Http/Routing/src/RouteGroupContext.cs @@ -7,7 +7,7 @@ namespace Microsoft.AspNetCore.Routing; /// -/// Represents the information accessible to . +/// Represents the information accessible to . /// public sealed class RouteGroupContext { @@ -29,7 +29,7 @@ public RouteGroupContext(RoutePattern prefix, IReadOnlyList - /// Gets the prefix for all the on all instances returned by the call to . + /// Gets the prefix for all the on all instances returned by the call to . /// This accounts for nested groups and gives the full group prefix, not just the prefix supplied to the innermost call to . /// public RoutePattern Prefix { get; } diff --git a/src/Http/Routing/test/UnitTests/CompositeEndpointDataSourceTest.cs b/src/Http/Routing/test/UnitTests/CompositeEndpointDataSourceTest.cs index c296a7d2e541..9d5c6b73fa0f 100644 --- a/src/Http/Routing/test/UnitTests/CompositeEndpointDataSourceTest.cs +++ b/src/Http/Routing/test/UnitTests/CompositeEndpointDataSourceTest.cs @@ -43,7 +43,7 @@ public void CreatesShallowCopyOf_ListOfGroupedEndpoints() var conventions = Array.Empty>(); var applicationServices = new ServiceCollection().BuildServiceProvider(); - var groupedEndpoints = compositeDataSource.GetGroupedEndpoints(new RouteGroupContext(prefix, conventions, applicationServices)); + var groupedEndpoints = compositeDataSource.GetEndpointGroup(new RouteGroupContext(prefix, conventions, applicationServices)); var resolvedGroupEndpoints = Assert.Single(dataSource.ResolvedGroupedEndpoints); Assert.NotSame(groupedEndpoints, resolvedGroupEndpoints); @@ -264,7 +264,7 @@ public void GetGroupedEndpoints_ForwardedToChildDataSources() }; var context = new RouteGroupContext(prefix, conventions, applicationServices); - var groupedEndpoints = compositeDataSource.GetGroupedEndpoints(context); + var groupedEndpoints = compositeDataSource.GetEndpointGroup(context); var receivedContext = Assert.Single(dataSource.ReceivedRouteGroupContexts); Assert.Same(context, receivedContext); @@ -299,10 +299,10 @@ private class TestGroupDataSource : EndpointDataSource public List> ResolvedGroupedEndpoints { get; } = new(); - public override IReadOnlyList GetGroupedEndpoints(RouteGroupContext context) + public override IReadOnlyList GetEndpointGroup(RouteGroupContext context) { ReceivedRouteGroupContexts.Add(context); - var resolved = base.GetGroupedEndpoints(context); + var resolved = base.GetEndpointGroup(context); ResolvedGroupedEndpoints.Add(resolved); return resolved; } From 8afc04eeb0cd428a7ddae3d1544b3a91588831ad Mon Sep 17 00:00:00 2001 From: Stephen Halter Date: Thu, 16 Jun 2022 19:55:09 -0700 Subject: [PATCH 12/21] Fix build - Fix nullability in EndpointMetadataApiDescriptionProviderTest - Remove redundant ArgumentNullException checks --- .../Builder/EndpointRouteBuilderExtensions.cs | 7 ---- ...pointMetadataApiDescriptionProviderTest.cs | 38 +++++++++---------- 2 files changed, 19 insertions(+), 26 deletions(-) diff --git a/src/Http/Routing/src/Builder/EndpointRouteBuilderExtensions.cs b/src/Http/Routing/src/Builder/EndpointRouteBuilderExtensions.cs index 85cab4a767e1..35a78918cb5e 100644 --- a/src/Http/Routing/src/Builder/EndpointRouteBuilderExtensions.cs +++ b/src/Http/Routing/src/Builder/EndpointRouteBuilderExtensions.cs @@ -390,9 +390,6 @@ public static RouteHandlerBuilder Map( [RequiresUnreferencedCode(MapEndpointTrimmerWarning)] public static RouteHandlerBuilder MapFallback(this IEndpointRouteBuilder endpoints, Delegate handler) { - ArgumentNullException.ThrowIfNull(endpoints); - ArgumentNullException.ThrowIfNull(handler); - return endpoints.MapFallback("{*path:nonfile}", handler); } @@ -423,10 +420,6 @@ public static RouteHandlerBuilder MapFallback( [StringSyntax("Route")] string pattern, Delegate handler) { - ArgumentNullException.ThrowIfNull(endpoints); - ArgumentNullException.ThrowIfNull(pattern); - ArgumentNullException.ThrowIfNull(handler); - return endpoints.Map(RoutePatternFactory.Parse(pattern), handler, httpMethods: null, isFallback: true); } diff --git a/src/Mvc/Mvc.ApiExplorer/test/EndpointMetadataApiDescriptionProviderTest.cs b/src/Mvc/Mvc.ApiExplorer/test/EndpointMetadataApiDescriptionProviderTest.cs index deda7c9f03d5..60fad3699779 100644 --- a/src/Mvc/Mvc.ApiExplorer/test/EndpointMetadataApiDescriptionProviderTest.cs +++ b/src/Mvc/Mvc.ApiExplorer/test/EndpointMetadataApiDescriptionProviderTest.cs @@ -136,7 +136,7 @@ static void AssertJsonResponse(ApiDescription apiDescription, Type expectedType) var responseType = Assert.Single(apiDescription.SupportedResponseTypes); Assert.Equal(200, responseType.StatusCode); Assert.Equal(expectedType, responseType.Type); - Assert.Equal(expectedType, responseType.ModelMetadata.ModelType); + Assert.Equal(expectedType, responseType.ModelMetadata?.ModelType); var responseFormat = Assert.Single(responseType.ApiResponseFormats); Assert.Equal("application/json", responseFormat.MediaType); @@ -144,7 +144,7 @@ static void AssertJsonResponse(ApiDescription apiDescription, Type expectedType) } AssertJsonResponse(GetApiDescription(() => new InferredJsonClass()), typeof(InferredJsonClass)); - AssertJsonResponse(GetApiDescription(() => (IInferredJsonInterface)null), typeof(IInferredJsonInterface)); + AssertJsonResponse(GetApiDescription(() => (IInferredJsonInterface)null!), typeof(IInferredJsonInterface)); } [Fact] @@ -155,7 +155,7 @@ public void AddsTextResponseFormatWhenFromBodyInferred() var responseType = Assert.Single(apiDescription.SupportedResponseTypes); Assert.Equal(200, responseType.StatusCode); Assert.Equal(typeof(string), responseType.Type); - Assert.Equal(typeof(string), responseType.ModelMetadata.ModelType); + Assert.Equal(typeof(string), responseType.ModelMetadata?.ModelType); var responseFormat = Assert.Single(responseType.ApiResponseFormats); Assert.Equal("text/plain", responseFormat.MediaType); @@ -170,7 +170,7 @@ static void AssertVoid(ApiDescription apiDescription) var responseType = Assert.Single(apiDescription.SupportedResponseTypes); Assert.Equal(200, responseType.StatusCode); Assert.Equal(typeof(void), responseType.Type); - Assert.Equal(typeof(void), responseType.ModelMetadata.ModelType); + Assert.Equal(typeof(void), responseType.ModelMetadata?.ModelType); Assert.Empty(responseType.ApiResponseFormats); } @@ -192,7 +192,7 @@ public void AddsResponseFormatFromMetadata() Assert.Equal(201, responseType.StatusCode); Assert.Equal(typeof(TimeSpan), responseType.Type); - Assert.Equal(typeof(TimeSpan), responseType.ModelMetadata.ModelType); + Assert.Equal(typeof(TimeSpan), responseType.ModelMetadata?.ModelType); var responseFormat = Assert.Single(responseType.ApiResponseFormats); Assert.Equal("application/custom", responseFormat.MediaType); @@ -212,7 +212,7 @@ public void AddsMultipleResponseFormatsFromMetadataWithPoco() Assert.Equal(201, createdResponseType.StatusCode); Assert.Equal(typeof(TimeSpan), createdResponseType.Type); - Assert.Equal(typeof(TimeSpan), createdResponseType.ModelMetadata.ModelType); + Assert.Equal(typeof(TimeSpan), createdResponseType.ModelMetadata?.ModelType); var createdResponseFormat = Assert.Single(createdResponseType.ApiResponseFormats); Assert.Equal("application/json", createdResponseFormat.MediaType); @@ -221,7 +221,7 @@ public void AddsMultipleResponseFormatsFromMetadataWithPoco() Assert.Equal(400, badRequestResponseType.StatusCode); Assert.Equal(typeof(InferredJsonClass), badRequestResponseType.Type); - Assert.Equal(typeof(InferredJsonClass), badRequestResponseType.ModelMetadata.ModelType); + Assert.Equal(typeof(InferredJsonClass), badRequestResponseType.ModelMetadata?.ModelType); var badRequestResponseFormat = Assert.Single(badRequestResponseType.ApiResponseFormats); Assert.Equal("application/json", badRequestResponseFormat.MediaType); @@ -241,7 +241,7 @@ public void AddsMultipleResponseFormatsFromMetadataWithIResult() Assert.Equal(201, createdResponseType.StatusCode); Assert.Equal(typeof(InferredJsonClass), createdResponseType.Type); - Assert.Equal(typeof(InferredJsonClass), createdResponseType.ModelMetadata.ModelType); + Assert.Equal(typeof(InferredJsonClass), createdResponseType.ModelMetadata?.ModelType); var createdResponseFormat = Assert.Single(createdResponseType.ApiResponseFormats); Assert.Equal("application/json", createdResponseFormat.MediaType); @@ -250,7 +250,7 @@ public void AddsMultipleResponseFormatsFromMetadataWithIResult() Assert.Equal(400, badRequestResponseType.StatusCode); Assert.Equal(typeof(void), badRequestResponseType.Type); - Assert.Equal(typeof(void), badRequestResponseType.ModelMetadata.ModelType); + Assert.Equal(typeof(void), badRequestResponseType.ModelMetadata?.ModelType); Assert.Empty(badRequestResponseType.ApiResponseFormats); } @@ -524,7 +524,7 @@ public void AddsMetadataFromRouteEndpoint() .FirstOrDefault(); Assert.NotNull(apiExplorerSettings); - Assert.True(apiExplorerSettings.IgnoreApi); + Assert.True(apiExplorerSettings!.IgnoreApi); } #nullable disable @@ -1194,11 +1194,11 @@ public void HandlesEndpointWithDescriptionAndSummary_WithExtensionMethods() var descriptionMetadata = apiDescription.ActionDescriptor.EndpointMetadata.OfType().SingleOrDefault(); Assert.NotNull(descriptionMetadata); - Assert.Equal("A description", descriptionMetadata.Description); + Assert.Equal("A description", descriptionMetadata!.Description); var summaryMetadata = apiDescription.ActionDescriptor.EndpointMetadata.OfType().SingleOrDefault(); Assert.NotNull(summaryMetadata); - Assert.Equal("A summary", summaryMetadata.Summary); + Assert.Equal("A summary", summaryMetadata!.Summary); } [Fact] @@ -1225,11 +1225,11 @@ public void HandlesEndpointWithDescriptionAndSummary_WithAttributes() var descriptionMetadata = apiDescription.ActionDescriptor.EndpointMetadata.OfType().SingleOrDefault(); Assert.NotNull(descriptionMetadata); - Assert.Equal("A description", descriptionMetadata.Description); + Assert.Equal("A description", descriptionMetadata!.Description); var summaryMetadata = apiDescription.ActionDescriptor.EndpointMetadata.OfType().SingleOrDefault(); Assert.NotNull(summaryMetadata); - Assert.Equal("A summary", summaryMetadata.Summary); + Assert.Equal("A summary", summaryMetadata!.Summary); } [Theory] @@ -1302,7 +1302,7 @@ private static IList GetApiDescriptions( private static TestEndpointRouteBuilder CreateBuilder() => new TestEndpointRouteBuilder(new ApplicationBuilder(TestServiceProvider.Instance)); - private static ApiDescription GetApiDescription(Delegate action, string? pattern = null, string displayName = null, IEnumerable? httpMethods = null) => + private static ApiDescription GetApiDescription(Delegate action, string? pattern = null, string? displayName = null, IEnumerable? httpMethods = null) => Assert.Single(GetApiDescriptions(action, pattern, displayName: displayName, httpMethods: httpMethods)); private static void TestAction() @@ -1332,10 +1332,10 @@ private class ServiceProviderIsService : IServiceProviderIsService private class HostEnvironment : IHostEnvironment { - public string EnvironmentName { get; set; } - public string ApplicationName { get; set; } - public string ContentRootPath { get; set; } - public IFileProvider ContentRootFileProvider { get; set; } + public string EnvironmentName { get; set; } = null!; + public string? ApplicationName { get; set; } + public string ContentRootPath { get; set; } = null!; + public IFileProvider ContentRootFileProvider { get; set; } = null!; } private class TestEndpointRouteBuilder : IEndpointRouteBuilder From 30c3cf714a880509947bc0058382ae11268e7e97 Mon Sep 17 00:00:00 2001 From: Stephen Halter Date: Fri, 17 Jun 2022 00:11:31 -0700 Subject: [PATCH 13/21] I thought I already fixed this lol - 97a5ada5d7c6bb547eaa58203e052ecc29f99c76 --- .../WebApplicationTests.cs | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/src/DefaultBuilder/test/Microsoft.AspNetCore.Tests/WebApplicationTests.cs b/src/DefaultBuilder/test/Microsoft.AspNetCore.Tests/WebApplicationTests.cs index 52c757f10f70..1ae68e9aa770 100644 --- a/src/DefaultBuilder/test/Microsoft.AspNetCore.Tests/WebApplicationTests.cs +++ b/src/DefaultBuilder/test/Microsoft.AspNetCore.Tests/WebApplicationTests.cs @@ -1572,12 +1572,13 @@ public async Task BranchingPipelineHasOwnRoutes() app.Start(); var ds = app.Services.GetRequiredService(); - Assert.Equal(5, ds.Endpoints.Count); - Assert.Equal("One", ds.Endpoints[0].DisplayName); - Assert.Equal("Two", ds.Endpoints[1].DisplayName); - Assert.Equal("Three", ds.Endpoints[2].DisplayName); - Assert.Equal("Four", ds.Endpoints[3].DisplayName); - Assert.Equal("Five", ds.Endpoints[4].DisplayName); + var displayNames = ds.Endpoints.Select(e => e.DisplayName).ToArray(); + Assert.Equal(5, displayNames.Length); + Assert.Contains("One", displayNames); + Assert.Contains("Two", displayNames); + Assert.Contains("Three", displayNames); + Assert.Contains("Four", displayNames); + Assert.Contains("Five", displayNames); var client = app.GetTestClient(); From 77a6a2cf979b596c7ea6cf229a59410959485323 Mon Sep 17 00:00:00 2001 From: Stephen Halter Date: Sat, 18 Jun 2022 12:44:09 -0700 Subject: [PATCH 14/21] cleanup --- .../Builder/RouteHandlerFilterExtensions.cs | 6 +- .../src/CompositeEndpointDataSource.cs | 57 ++++++++++++------- src/Http/Routing/src/EndpointDataSource.cs | 5 +- .../src/Patterns/RoutePatternFactory.cs | 10 ++-- .../Routing/src/RouteEndpointDataSource.cs | 30 +++++----- src/Http/Routing/src/RouteFilterMetadata.cs | 2 +- src/Http/Routing/src/RouteGroupBuilder.cs | 18 +++++- 7 files changed, 80 insertions(+), 48 deletions(-) diff --git a/src/Http/Routing/src/Builder/RouteHandlerFilterExtensions.cs b/src/Http/Routing/src/Builder/RouteHandlerFilterExtensions.cs index 548de1c33464..637ec5fc15e1 100644 --- a/src/Http/Routing/src/Builder/RouteHandlerFilterExtensions.cs +++ b/src/Http/Routing/src/Builder/RouteHandlerFilterExtensions.cs @@ -108,14 +108,14 @@ public static TBuilder AddRouteHandlerFilter(this TBuilder builder, Fu { // Even if we split RouteFilterMetadata across multiple metadata objects, RouteEndointDataSource will still use all of them. // We're just trying to be good citizens and keep the Endpoint.Metadata as small as possible. - RouteFilterMetadata? filterMetadata = null; + RouteHandlerFilterMetadata? filterMetadata = null; foreach (var item in endpointBuilder.Metadata) { - if (item is RouteFilterMetadata metadata) + if (item is RouteHandlerFilterMetadata metadata) { + // Don't break early in case there is somehow higher priority metadata later in the list. filterMetadata = metadata; - break; } } diff --git a/src/Http/Routing/src/CompositeEndpointDataSource.cs b/src/Http/Routing/src/CompositeEndpointDataSource.cs index 37b7cb94048b..5c3e2fa0edb1 100644 --- a/src/Http/Routing/src/CompositeEndpointDataSource.cs +++ b/src/Http/Routing/src/CompositeEndpointDataSource.cs @@ -5,7 +5,6 @@ using System.Collections.Specialized; using System.Diagnostics; using System.Diagnostics.CodeAnalysis; -using System.Linq; using Microsoft.AspNetCore.Http; using Microsoft.Extensions.Primitives; @@ -20,7 +19,7 @@ public sealed class CompositeEndpointDataSource : EndpointDataSource, IDisposabl private readonly object _lock = new(); private readonly ICollection _dataSources; - private IReadOnlyList? _endpoints; + private List? _endpoints; private IChangeToken? _consumerChangeToken; private CancellationTokenSource? _cts; private List? _changeTokenRegistrations; @@ -149,6 +148,11 @@ public void Dispose() [MemberNotNull(nameof(_endpoints))] private void EnsureEndpointsInitialized() { + if (_endpoints is not null) + { + return; + } + lock (_lock) { if (_endpoints is null) @@ -159,7 +163,7 @@ private void EnsureEndpointsInitialized() // Note: we can't use DataSourceDependentCache here because we also need to handle a list of change // tokens, which is a complication most of our code doesn't have. - _endpoints = _dataSources.SelectMany(d => d.Endpoints).ToArray(); + CreateEndopintsUnsynchronized(); } } } @@ -167,6 +171,11 @@ private void EnsureEndpointsInitialized() [MemberNotNull(nameof(_consumerChangeToken))] private void EnsureChangeTokenInitialized() { + if (_consumerChangeToken is not null) + { + return; + } + lock (_lock) { if (_consumerChangeToken is null) @@ -180,6 +189,7 @@ private void EnsureChangeTokenInitialized() private void HandleChange(bool collectionChanged) { CancellationTokenSource? oldTokenSource = null; + List? oldChangeTokenRegistrations = null; lock (_lock) { @@ -196,6 +206,7 @@ private void HandleChange(bool collectionChanged) // 3. B executes some code in its callback, but needs to re-register callback // in the same callback. oldTokenSource = _cts; + oldChangeTokenRegistrations = _changeTokenRegistrations; // Don't create a new change token if no one is listening. if (oldTokenSource is not null) @@ -209,12 +220,21 @@ private void HandleChange(bool collectionChanged) if (_endpoints is not null) { // Refresh the endpoints from data source so that callbacks can get the latest endpoints. - _endpoints = _dataSources.SelectMany(d => d.Endpoints).ToArray(); + CreateEndopintsUnsynchronized(); + } + } + + // Disposing registrations can block on user defined code on running on other threads that could try to acquire the _lock. + if (collectionChanged && oldChangeTokenRegistrations is not null) + { + foreach (var registration in oldChangeTokenRegistrations) + { + registration.Dispose(); } } - // Raise consumer callbacks. Any new callback registration would happen on the new token - // created in earlier step. Avoid raising callbacks in a lock to avoid possible deadlocks. + // Raise consumer callbacks. Any new callback registration would happen on the new token created in earlier step. + // Avoid raising callbacks inside a lock. oldTokenSource?.Cancel(); } @@ -226,19 +246,7 @@ private void CreateChangeTokenUnsynchronized(bool collectionChanged) if (collectionChanged) { - if (_changeTokenRegistrations is null) - { - _changeTokenRegistrations = new(); - } - else - { - foreach (var registration in _changeTokenRegistrations) - { - registration.Dispose(); - } - _changeTokenRegistrations.Clear(); - } - + _changeTokenRegistrations = new(); foreach (var dataSource in _dataSources) { _changeTokenRegistrations.Add(ChangeToken.OnChange( @@ -248,6 +256,17 @@ private void CreateChangeTokenUnsynchronized(bool collectionChanged) } } + [MemberNotNull(nameof(_endpoints))] + private void CreateEndopintsUnsynchronized() + { + _endpoints = new List(); + + foreach (var dataSource in _dataSources) + { + _endpoints.AddRange(dataSource.Endpoints); + } + } + // Use private variable '_endpoints' to avoid initialization private string DebuggerDisplayString => GetDebuggerDisplayStringForEndpoints(_endpoints); } diff --git a/src/Http/Routing/src/EndpointDataSource.cs b/src/Http/Routing/src/EndpointDataSource.cs index ba93b649fa6f..f030e7abcb24 100644 --- a/src/Http/Routing/src/EndpointDataSource.cs +++ b/src/Http/Routing/src/EndpointDataSource.cs @@ -75,7 +75,7 @@ public virtual IReadOnlyList GetEndpointGroup(RouteGroupContext contex // The RoutePattern, RequestDelegate, Order and DisplayName can all be overridden by non-group-aware conventions. // Unlike with metadata, if a convention is applied to a group that changes any of these, I would expect these - // to be overridden as there's reasonable way to merge these properties. + // to be overridden as there's no reasonable way to merge these properties. wrappedEndpoints[i] = (RouteEndpoint)routeEndpointBuilder.Build(); } @@ -83,7 +83,6 @@ public virtual IReadOnlyList GetEndpointGroup(RouteGroupContext contex } // We don't implement DebuggerDisplay directly on the EndpointDataSource base type because this could have side effects. - // REVIEW: Should we just make this public/protected? Derived types that can provide endpoints without side effects might find this useful. internal static string GetDebuggerDisplayStringForEndpoints(IReadOnlyList? endpoints) { if (endpoints is null) @@ -154,7 +153,7 @@ static void FormatValues(StringBuilder sb, IEnumerable GetEndpointGroup(RouteGroupContext context) { var endpoints = new List(_routeEntries.Count); @@ -74,7 +73,7 @@ public override IReadOnlyList GetEndpointGroup(RouteGroupContext // For testing internal RouteEndpointBuilder GetSingleRouteEndpointBuilder() { - if (_routeEntries.Count != 1) + if (_routeEntries.Count is not 1) { throw new InvalidOperationException($"There are {_routeEntries.Count} endpoints defined! This can only be called for a single endpoint."); } @@ -122,11 +121,11 @@ private RouteEndpointBuilder CreateRouteEndpointBuilder(RouteEntry entry, RouteP return factoryCreatedRequestDelegate(context); }; - // The Map methods don't support customizing the order apart from using int.MaxValue to give MapFallback the lowest precedences. + // 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; - var builder = new RouteEndpointBuilder(redirectedRequestDelegate, pattern, order) + RouteEndpointBuilder builder = new(redirectedRequestDelegate, pattern, order) { DisplayName = displayName, ApplicationServices = _applicationServices, @@ -152,7 +151,7 @@ private RouteEndpointBuilder CreateRouteEndpointBuilder(RouteEntry entry, RouteP } } - // Add delegate attributes as metadata before programmatic conventions. + // Add delegate attributes as metadata before entry-specific conventions but after group conventions. var attributes = handler.Method.GetCustomAttributes(); if (attributes is not null) { @@ -170,21 +169,20 @@ private RouteEndpointBuilder CreateRouteEndpointBuilder(RouteEntry entry, RouteP foreach (var item in builder.Metadata) { - if (item is RouteFilterMetadata filterMetadata) + if (item is RouteHandlerFilterMetadata filterMetadata) { routeHandlerFilterFactories ??= new(); routeHandlerFilterFactories.AddRange(filterMetadata.FilterFactories); } } - var routeParams = pattern.Parameters; - var routeParamNames = new List(routeParams.Count); - foreach (var parameter in routeParams) + var routeParamNames = new List(pattern.Parameters.Count); + foreach (var parameter in pattern.Parameters) { routeParamNames.Add(parameter.Name); } - var factoryOptions = new RequestDelegateFactoryOptions + RequestDelegateFactoryOptions factoryOptions = new() { ServiceProvider = _applicationServices, RouteParameterNames = routeParamNames, @@ -239,11 +237,11 @@ static bool ShouldDisableInferredBodyForMethod(string method) => private struct RouteEntry { - public RoutePattern RoutePattern { get; init; } - public Delegate RouteHandler { get; init; } - public IEnumerable? HttpMethods { get; init; } - public bool IsFallback { get; init; } - public ThrowOnAddAfterBuildCollection Conventions { get; init; } + public required RoutePattern RoutePattern { get; init; } + public required Delegate RouteHandler { get; init; } + public required IEnumerable? HttpMethods { get; init; } + public required bool IsFallback { get; init; } + public required ThrowOnAddAfterBuildCollection Conventions { get; init; } } // This private class is only exposed to internal code via ICollection> in RouteEndpointBuilder where only Add is called. diff --git a/src/Http/Routing/src/RouteFilterMetadata.cs b/src/Http/Routing/src/RouteFilterMetadata.cs index 215c09b77668..ed9f5b73dbd0 100644 --- a/src/Http/Routing/src/RouteFilterMetadata.cs +++ b/src/Http/Routing/src/RouteFilterMetadata.cs @@ -5,7 +5,7 @@ namespace Microsoft.AspNetCore.Routing; -internal class RouteFilterMetadata +internal class RouteHandlerFilterMetadata { public List> FilterFactories { get; } = new(); } diff --git a/src/Http/Routing/src/RouteGroupBuilder.cs b/src/Http/Routing/src/RouteGroupBuilder.cs index 169dd1bc7c1e..51352ae27844 100644 --- a/src/Http/Routing/src/RouteGroupBuilder.cs +++ b/src/Http/Routing/src/RouteGroupBuilder.cs @@ -4,6 +4,7 @@ using Microsoft.AspNetCore.Builder; using Microsoft.AspNetCore.Http; using Microsoft.AspNetCore.Routing.Patterns; +using Microsoft.Extensions.FileProviders; using Microsoft.Extensions.Primitives; namespace Microsoft.AspNetCore.Routing; @@ -66,17 +67,18 @@ public IReadOnlyList GetGroupedEndpointsWithNullablePrefix(RoutePatter var fullPrefix = RoutePatternFactory.Combine(prefix, _routeGroupBuilder._partialPrefix); // Apply conventions passed in from the outer group first so their metadata is added earlier in the list at a lower precedent. var combinedConventions = RoutePatternFactory.CombineLists(conventions, _routeGroupBuilder._conventions); + var context = new RouteGroupContext(fullPrefix, combinedConventions, applicationServices); if (_routeGroupBuilder._dataSources.Count is 1) { - return _routeGroupBuilder._dataSources[0].GetEndpointGroup(new RouteGroupContext(fullPrefix, combinedConventions, applicationServices)); + return _routeGroupBuilder._dataSources[0].GetEndpointGroup(context); } var groupedEndpoints = new List(); foreach (var dataSource in _routeGroupBuilder._dataSources) { - groupedEndpoints.AddRange(dataSource.GetEndpointGroup(new RouteGroupContext(fullPrefix, combinedConventions, applicationServices))); + groupedEndpoints.AddRange(dataSource.GetEndpointGroup(context)); } return groupedEndpoints; @@ -84,6 +86,18 @@ public IReadOnlyList GetGroupedEndpointsWithNullablePrefix(RoutePatter public override IChangeToken GetChangeToken() { + if (_routeGroupBuilder._dataSources.Count is 0) + { + return NullChangeToken.Singleton; + } + + if (_routeGroupBuilder._dataSources.Count is 1) + { + return _routeGroupBuilder._dataSources[0].GetChangeToken(); + } + + // We are not guarding against concurrent RouteGroupBuilder._dataSources mutation. + // This is only to avoid double initialization of _compositeDataSource if GetChangeToken() is called concurrently. lock (_routeGroupBuilder._dataSources) { _compositeDataSource ??= new CompositeEndpointDataSource(_routeGroupBuilder._dataSources); From 1bb70c803f09720c68bb99324f99376854e862b1 Mon Sep 17 00:00:00 2001 From: Stephen Halter Date: Sat, 18 Jun 2022 13:56:39 -0700 Subject: [PATCH 15/21] polish - less required :( --- .../src/RequestDelegateFactoryOptions.cs | 2 +- .../src/CompositeEndpointDataSource.cs | 35 ++++++++++--------- .../Routing/src/RouteEndpointDataSource.cs | 10 +++--- 3 files changed, 24 insertions(+), 23 deletions(-) diff --git a/src/Http/Http.Extensions/src/RequestDelegateFactoryOptions.cs b/src/Http/Http.Extensions/src/RequestDelegateFactoryOptions.cs index 8e3edccd36ae..c15644bfa12b 100644 --- a/src/Http/Http.Extensions/src/RequestDelegateFactoryOptions.cs +++ b/src/Http/Http.Extensions/src/RequestDelegateFactoryOptions.cs @@ -47,7 +47,7 @@ public sealed class RequestDelegateFactoryOptions /// and before any metadata provided by types in the delegate signature that implement /// or . The exception to this general rule is the /// that infers automatically - /// without any custom metadata providers which instead is inserted at the start to give it lower precedence. Custom metadata providers can choose to do + /// without any custom metadata providers which instead is inserted at the start to give it lower precedence. Custom metadata providers can choose to /// insert their metadata at the start to give lower precedence, but this is unusual. /// public IList? EndpointMetadata { get; init; } diff --git a/src/Http/Routing/src/CompositeEndpointDataSource.cs b/src/Http/Routing/src/CompositeEndpointDataSource.cs index 5c3e2fa0edb1..1aabe2bc7792 100644 --- a/src/Http/Routing/src/CompositeEndpointDataSource.cs +++ b/src/Http/Routing/src/CompositeEndpointDataSource.cs @@ -123,13 +123,10 @@ public void Dispose() } } - if (_changeTokenRegistrations is not null) + if (_changeTokenRegistrations is { Count: > 0 }) { - foreach (var registration in _changeTokenRegistrations) - { - disposables ??= new List(); - disposables.Add(registration); - } + disposables ??= new List(); + disposables.AddRange(_changeTokenRegistrations); } } @@ -155,16 +152,18 @@ private void EnsureEndpointsInitialized() lock (_lock) { - if (_endpoints is null) + if (_endpoints is not null) { - // Now that we're caching the _enpoints, we're responsible for keeping them up-to-date even if the caller - // hasn't started listening for changes themselves yet. - EnsureChangeTokenInitialized(); - - // Note: we can't use DataSourceDependentCache here because we also need to handle a list of change - // tokens, which is a complication most of our code doesn't have. - CreateEndopintsUnsynchronized(); + return; } + + // Now that we're caching the _enpoints, we're responsible for keeping them up-to-date even if the caller + // hasn't started listening for changes themselves yet. + EnsureChangeTokenInitialized(); + + // Note: we can't use DataSourceDependentCache here because we also need to handle a list of change + // tokens, which is a complication most of our code doesn't have. + CreateEndopintsUnsynchronized(); } } @@ -178,11 +177,13 @@ private void EnsureChangeTokenInitialized() lock (_lock) { - if (_consumerChangeToken is null) + if (_consumerChangeToken is not null) { - // This is our first time initializing the change token, so the collection has "changed" from nothing. - CreateChangeTokenUnsynchronized(collectionChanged: true); + return; } + + // This is our first time initializing the change token, so the collection has "changed" from nothing. + CreateChangeTokenUnsynchronized(collectionChanged: true); } } diff --git a/src/Http/Routing/src/RouteEndpointDataSource.cs b/src/Http/Routing/src/RouteEndpointDataSource.cs index 445c49a654e2..6926a156bae2 100644 --- a/src/Http/Routing/src/RouteEndpointDataSource.cs +++ b/src/Http/Routing/src/RouteEndpointDataSource.cs @@ -237,11 +237,11 @@ static bool ShouldDisableInferredBodyForMethod(string method) => private struct RouteEntry { - public required RoutePattern RoutePattern { get; init; } - public required Delegate RouteHandler { get; init; } - public required IEnumerable? HttpMethods { get; init; } - public required bool IsFallback { get; init; } - public required ThrowOnAddAfterBuildCollection Conventions { get; init; } + public RoutePattern RoutePattern { get; init; } + public Delegate RouteHandler { get; init; } + public IEnumerable? HttpMethods { get; init; } + public bool IsFallback { get; init; } + public ThrowOnAddAfterBuildCollection Conventions { get; init; } } // This private class is only exposed to internal code via ICollection> in RouteEndpointBuilder where only Add is called. From a0ff534d1737d6c3b5b53749635eed377ac47ad8 Mon Sep 17 00:00:00 2001 From: Stephen Halter Date: Sat, 18 Jun 2022 16:57:00 -0700 Subject: [PATCH 16/21] Clean up doc comments --- .../src/RequestDelegateFactory.cs | 6 +++--- src/Http/Routing/src/EndpointDataSource.cs | 6 +++--- .../Routing/src/RouteEndpointDataSource.cs | 18 +++++++++--------- src/Http/Routing/src/RouteGroupBuilder.cs | 6 ------ src/Http/Routing/src/RouteGroupContext.cs | 9 ++++++--- .../test/UnitTests/Builder/GroupTest.cs | 6 ------ 6 files changed, 21 insertions(+), 30 deletions(-) diff --git a/src/Http/Http.Extensions/src/RequestDelegateFactory.cs b/src/Http/Http.Extensions/src/RequestDelegateFactory.cs index 140fef7ea49e..cea842d12884 100644 --- a/src/Http/Http.Extensions/src/RequestDelegateFactory.cs +++ b/src/Http/Http.Extensions/src/RequestDelegateFactory.cs @@ -1648,9 +1648,9 @@ private static Expression BindParameterFromBindAsync(ParameterInfo parameter, Fa private static void InsertInferredAcceptsMetadata(FactoryContext factoryContext, Type type, string[] contentTypes) { - // Unlike most metadata that will probably to be added by filters or metadata providers, we insert the automatically-inferred AcceptsMetadata - // to the beginning of the metadata to give it the lowest precedence. It really doesn't makes sense for this metadata to be overridden but - // we're preserving the old behavior here out of an abundance of caution. + // Insert the automatically-inferred AcceptsMetadata at the beginning of the list to give it the lowest precedence. + // It really doesn't makes sense for this metadata to be overridden, but we're preserving the old behavior out of an abundance of caution. + // I suspect most filters and metadata providers will just add their metadata to the end of the list. factoryContext.Metadata.Insert(0, new AcceptsMetadata(type, factoryContext.AllowEmptyRequestBody, contentTypes)); } diff --git a/src/Http/Routing/src/EndpointDataSource.cs b/src/Http/Routing/src/EndpointDataSource.cs index f030e7abcb24..6cb05074eaa2 100644 --- a/src/Http/Routing/src/EndpointDataSource.cs +++ b/src/Http/Routing/src/EndpointDataSource.cs @@ -26,11 +26,11 @@ public abstract class EndpointDataSource public abstract IReadOnlyList Endpoints { get; } /// - /// Get the instances for this given the specified . + /// Get the instances for this given the specified and . /// - /// Details about how the returned instances should be grouped and a reference to application services. + /// Details about how the returned instances should be grouped and a reference to application services. /// - /// Returns a read-only collection of instances given the specified group and . + /// Returns a read-only collection of instances given the specified group and . /// public virtual IReadOnlyList GetEndpointGroup(RouteGroupContext context) { diff --git a/src/Http/Routing/src/RouteEndpointDataSource.cs b/src/Http/Routing/src/RouteEndpointDataSource.cs index 6926a156bae2..a2542fb5b934 100644 --- a/src/Http/Routing/src/RouteEndpointDataSource.cs +++ b/src/Http/Routing/src/RouteEndpointDataSource.cs @@ -49,10 +49,10 @@ public override IReadOnlyList Endpoints { get { - var endpoints = new List(_routeEntries.Count); - foreach (var entry in _routeEntries) + var endpoints = new RouteEndpoint[_routeEntries.Count]; + for (int i = 0; i < _routeEntries.Count; i++) { - endpoints.Add((RouteEndpoint)CreateRouteEndpointBuilder(entry).Build()); + endpoints[i] = (RouteEndpoint)CreateRouteEndpointBuilder(_routeEntries[i]).Build(); } return endpoints; } @@ -60,10 +60,10 @@ public override IReadOnlyList Endpoints public override IReadOnlyList GetEndpointGroup(RouteGroupContext context) { - var endpoints = new List(_routeEntries.Count); - foreach (var entry in _routeEntries) + var endpoints = new RouteEndpoint[_routeEntries.Count]; + for (int i = 0; i < _routeEntries.Count; i++) { - endpoints.Add((RouteEndpoint)CreateRouteEndpointBuilder(entry, context.Prefix, context.Conventions).Build()); + endpoints[i] = (RouteEndpoint)CreateRouteEndpointBuilder(_routeEntries[i], context.Prefix, context.Conventions).Build(); } return endpoints; } @@ -82,9 +82,9 @@ internal RouteEndpointBuilder GetSingleRouteEndpointBuilder() } [UnconditionalSuppressMessage("Trimmer", "IL2026", - Justification = "We surface a RequireUnreferencedCode in the call to Map method adding this EndpointDataSource. " + - "The trimmer is unable to infer this.")] - private RouteEndpointBuilder CreateRouteEndpointBuilder(RouteEntry entry, RoutePattern? groupPrefix = null, IReadOnlyList>? groupConventions = null) + Justification = "We surface a RequireUnreferencedCode in the call to Map method adding this EndpointDataSource. The trimmer is unable to infer this.")] + private RouteEndpointBuilder CreateRouteEndpointBuilder( + RouteEntry entry, RoutePattern? groupPrefix = null, IReadOnlyList>? groupConventions = null) { var pattern = RoutePatternFactory.Combine(groupPrefix, entry.RoutePattern); var handler = entry.RouteHandler; diff --git a/src/Http/Routing/src/RouteGroupBuilder.cs b/src/Http/Routing/src/RouteGroupBuilder.cs index 51352ae27844..cada696414e1 100644 --- a/src/Http/Routing/src/RouteGroupBuilder.cs +++ b/src/Http/Routing/src/RouteGroupBuilder.cs @@ -35,12 +35,6 @@ internal RouteGroupBuilder(IEndpointRouteBuilder outerEndpointRouteBuilder, Rout ICollection IEndpointRouteBuilder.DataSources => _dataSources; void IEndpointConventionBuilder.Add(Action convention) => _conventions.Add(convention); - // For testing - // This accounts for nested groups and gives the full group prefix, not just the prefix supplied to the last call to MapGroup - // If the _outerEndpointRouteBuilder is not a RouteGroupBuilder (like a wrapper around RouteGroupBuilder) this will not have - // the final prefix used in GroupDataSource.GetGroupedEndpoints() which is why this is not public even though it seems useful. - internal RoutePattern GroupPrefix => RoutePatternFactory.Combine((_outerEndpointRouteBuilder as RouteGroupBuilder)?.GroupPrefix, _partialPrefix); - private sealed class GroupEndpointDataSource : EndpointDataSource, IDisposable { private readonly RouteGroupBuilder _routeGroupBuilder; diff --git a/src/Http/Routing/src/RouteGroupContext.cs b/src/Http/Routing/src/RouteGroupContext.cs index c0e879cc26b3..e27e3cf98e52 100644 --- a/src/Http/Routing/src/RouteGroupContext.cs +++ b/src/Http/Routing/src/RouteGroupContext.cs @@ -29,13 +29,16 @@ public RouteGroupContext(RoutePattern prefix, IReadOnlyList - /// Gets the prefix for all the on all instances returned by the call to . - /// This accounts for nested groups and gives the full group prefix, not just the prefix supplied to the innermost call to . + /// Gets the which should prefix the of all instances + /// returned by the call to . This accounts for nested groups and gives the full group prefix + /// not just the prefix supplied to the innermost call to . /// public RoutePattern Prefix { get; } /// - /// Gets all conventions added to a parent via . + /// Gets all conventions added to ancestor instances returned from + /// via . These should be applied in order when building every + /// returned from . /// public IReadOnlyList> Conventions { get; } diff --git a/src/Http/Routing/test/UnitTests/Builder/GroupTest.cs b/src/Http/Routing/test/UnitTests/Builder/GroupTest.cs index 9fb1cffe9283..53ae8cae1ce8 100644 --- a/src/Http/Routing/test/UnitTests/Builder/GroupTest.cs +++ b/src/Http/Routing/test/UnitTests/Builder/GroupTest.cs @@ -23,9 +23,7 @@ private EndpointDataSource GetEndpointDataSource(IEndpointRouteBuilder endpointR public async Task Prefix_CanBeEmpty() { var builder = new DefaultEndpointRouteBuilder(new ApplicationBuilder(EmptyServiceProvider.Instance)); - var group = builder.MapGroup(""); - Assert.Equal("", group.GroupPrefix.RawText); group.MapGet("/{id}", (int id, HttpContext httpContext) => { @@ -56,9 +54,7 @@ public async Task Prefix_CanBeEmpty() public async Task PrefixWithRouteParameter_CanBeUsed() { var builder = new DefaultEndpointRouteBuilder(new ApplicationBuilder(EmptyServiceProvider.Instance)); - var group = builder.MapGroup("/{org}"); - Assert.Equal("/{org}", group.GroupPrefix.RawText); group.MapGet("/{id}", (string org, int id, HttpContext httpContext) => { @@ -92,9 +88,7 @@ public async Task PrefixWithRouteParameter_CanBeUsed() public async Task NestedPrefixWithRouteParameters_CanBeUsed() { var builder = new DefaultEndpointRouteBuilder(new ApplicationBuilder(EmptyServiceProvider.Instance)); - var group = builder.MapGroup("/{org}").MapGroup("/{id}"); - Assert.Equal("/{org}/{id}", group.GroupPrefix.RawText); group.MapGet("/", (string org, int id, HttpContext httpContext) => { From b0c8b1b657d7fb3a6c109728db9044651ba6f63a Mon Sep 17 00:00:00 2001 From: Stephen Halter Date: Sun, 19 Jun 2022 16:24:16 -0700 Subject: [PATCH 17/21] Address straightforward PR feedback --- .../src/Builder/RouteHandlerBuilder.cs | 3 +- .../src/CompositeEndpointDataSource.cs | 6 +- .../Routing/src/RouteEndpointDataSource.cs | 28 +++++---- src/Http/Routing/src/RouteGroupBuilder.cs | 58 +++++++++---------- 4 files changed, 50 insertions(+), 45 deletions(-) diff --git a/src/Http/Routing/src/Builder/RouteHandlerBuilder.cs b/src/Http/Routing/src/Builder/RouteHandlerBuilder.cs index 11baeb88947e..3910e4a2bd64 100644 --- a/src/Http/Routing/src/Builder/RouteHandlerBuilder.cs +++ b/src/Http/Routing/src/Builder/RouteHandlerBuilder.cs @@ -14,7 +14,8 @@ public sealed class RouteHandlerBuilder : IEndpointConventionBuilder private readonly ICollection>? _conventions; /// - /// Instantiates a new given a ThrowOnAddAfterBuildCollection from . + /// Instantiates a new given a ThrowOnAddAfterEndpointBuiltConventionCollection from + /// . /// /// The convention list returned from . internal RouteHandlerBuilder(ICollection> conventions) diff --git a/src/Http/Routing/src/CompositeEndpointDataSource.cs b/src/Http/Routing/src/CompositeEndpointDataSource.cs index 1aabe2bc7792..f9553720fd4d 100644 --- a/src/Http/Routing/src/CompositeEndpointDataSource.cs +++ b/src/Http/Routing/src/CompositeEndpointDataSource.cs @@ -163,7 +163,7 @@ private void EnsureEndpointsInitialized() // Note: we can't use DataSourceDependentCache here because we also need to handle a list of change // tokens, which is a complication most of our code doesn't have. - CreateEndopintsUnsynchronized(); + CreateEndpointsUnsynchronized(); } } @@ -221,7 +221,7 @@ private void HandleChange(bool collectionChanged) if (_endpoints is not null) { // Refresh the endpoints from data source so that callbacks can get the latest endpoints. - CreateEndopintsUnsynchronized(); + CreateEndpointsUnsynchronized(); } } @@ -258,7 +258,7 @@ private void CreateChangeTokenUnsynchronized(bool collectionChanged) } [MemberNotNull(nameof(_endpoints))] - private void CreateEndopintsUnsynchronized() + private void CreateEndpointsUnsynchronized() { _endpoints = new List(); diff --git a/src/Http/Routing/src/RouteEndpointDataSource.cs b/src/Http/Routing/src/RouteEndpointDataSource.cs index a2542fb5b934..40f034cc183c 100644 --- a/src/Http/Routing/src/RouteEndpointDataSource.cs +++ b/src/Http/Routing/src/RouteEndpointDataSource.cs @@ -37,7 +37,7 @@ public ICollection> AddEndpoint( RouteHandler = routeHandler, HttpMethods = httpMethods, IsFallback = isFallback, - Conventions = new ThrowOnAddAfterBuildCollection(), + Conventions = new ThrowOnAddAfterEndpointBuiltConventionCollection(), }; _routeEntries.Add(entry); @@ -131,10 +131,9 @@ private RouteEndpointBuilder CreateRouteEndpointBuilder( ApplicationServices = _applicationServices, }; - // We own EndpointBuilder.Metadata (in another assembly), so we know it's just a List. - var metadata = (List)builder.Metadata; - - // Add MethodInfo as first metadata item + // 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 (entry.HttpMethods is not null) @@ -155,10 +154,13 @@ private RouteEndpointBuilder CreateRouteEndpointBuilder( var attributes = handler.Method.GetCustomAttributes(); if (attributes is not null) { - metadata.AddRange(attributes); + foreach (var attribute in attributes) + { + builder.Metadata.Add(attribute); + } } - entry.Conventions.HasBeenBuilt = true; + entry.Conventions.IsReadonly = true; foreach (var entrySpecificConvention in entry.Conventions) { entrySpecificConvention(builder); @@ -188,7 +190,7 @@ private RouteEndpointBuilder CreateRouteEndpointBuilder( RouteParameterNames = routeParamNames, ThrowOnBadRequest = _throwOnBadRequest, DisableInferBodyFromParameters = ShouldDisableInferredBodyParameters(entry.HttpMethods), - EndpointMetadata = metadata, + EndpointMetadata = builder.Metadata, RouteHandlerFilterFactories = routeHandlerFilterFactories, }; @@ -241,17 +243,19 @@ private struct RouteEntry public Delegate RouteHandler { get; init; } public IEnumerable? HttpMethods { get; init; } public bool IsFallback { get; init; } - public ThrowOnAddAfterBuildCollection Conventions { get; init; } + public ThrowOnAddAfterEndpointBuiltConventionCollection Conventions { get; init; } } // This private class is only exposed to internal code via ICollection> in RouteEndpointBuilder where only Add is called. - private sealed class ThrowOnAddAfterBuildCollection : List>, ICollection> + private sealed class ThrowOnAddAfterEndpointBuiltConventionCollection : List>, ICollection> { - public bool HasBeenBuilt { get; set; } + // We throw if someone tries to add conventions to the RouteEntry after endpoints have already been resolved meaning the conventions + // will not be observed given RouteEndpointDataSource is not meant to be dynamic and uses NullChangeToken.Singleton. + public bool IsReadonly { get; set; } void ICollection>.Add(Action convention) { - if (HasBeenBuilt) + if (IsReadonly) { throw new InvalidOperationException(Resources.RouteEndpointDataSource_ConventionsCannotBeModifiedAfterBuild); } diff --git a/src/Http/Routing/src/RouteGroupBuilder.cs b/src/Http/Routing/src/RouteGroupBuilder.cs index cada696414e1..1b135d3c786a 100644 --- a/src/Http/Routing/src/RouteGroupBuilder.cs +++ b/src/Http/Routing/src/RouteGroupBuilder.cs @@ -53,21 +53,41 @@ public override IReadOnlyList GetEndpointGroup(RouteGroupContext conte public IReadOnlyList GetGroupedEndpointsWithNullablePrefix(RoutePattern? prefix, IReadOnlyList> conventions, IServiceProvider applicationServices) { - if (_routeGroupBuilder._dataSources.Count is 0) + return _routeGroupBuilder._dataSources.Count switch { - return Array.Empty(); + 0 => Array.Empty(), + 1 => _routeGroupBuilder._dataSources[0].GetEndpointGroup(GetNextRouteGroupContext(prefix, conventions, applicationServices)), + _ => SelectEndpointsFromAllDataSources(GetNextRouteGroupContext(prefix, conventions, applicationServices)), + }; + } + + public override IChangeToken GetChangeToken() => _routeGroupBuilder._dataSources.Count switch + { + 0 => NullChangeToken.Singleton, + 1 => _routeGroupBuilder._dataSources[0].GetChangeToken(), + _ => GetCompositeChangeToken(), + }; + + public void Dispose() + { + _compositeDataSource?.Dispose(); + + foreach (var dataSource in _routeGroupBuilder._dataSources) + { + (dataSource as IDisposable)?.Dispose(); } + } + private RouteGroupContext GetNextRouteGroupContext(RoutePattern? prefix, IReadOnlyList> conventions, IServiceProvider applicationServices) + { var fullPrefix = RoutePatternFactory.Combine(prefix, _routeGroupBuilder._partialPrefix); // Apply conventions passed in from the outer group first so their metadata is added earlier in the list at a lower precedent. var combinedConventions = RoutePatternFactory.CombineLists(conventions, _routeGroupBuilder._conventions); - var context = new RouteGroupContext(fullPrefix, combinedConventions, applicationServices); - - if (_routeGroupBuilder._dataSources.Count is 1) - { - return _routeGroupBuilder._dataSources[0].GetEndpointGroup(context); - } + return new RouteGroupContext(fullPrefix, combinedConventions, applicationServices); + } + private IReadOnlyList SelectEndpointsFromAllDataSources(RouteGroupContext context) + { var groupedEndpoints = new List(); foreach (var dataSource in _routeGroupBuilder._dataSources) @@ -78,18 +98,8 @@ public IReadOnlyList GetGroupedEndpointsWithNullablePrefix(RoutePatter return groupedEndpoints; } - public override IChangeToken GetChangeToken() + private IChangeToken GetCompositeChangeToken() { - if (_routeGroupBuilder._dataSources.Count is 0) - { - return NullChangeToken.Singleton; - } - - if (_routeGroupBuilder._dataSources.Count is 1) - { - return _routeGroupBuilder._dataSources[0].GetChangeToken(); - } - // We are not guarding against concurrent RouteGroupBuilder._dataSources mutation. // This is only to avoid double initialization of _compositeDataSource if GetChangeToken() is called concurrently. lock (_routeGroupBuilder._dataSources) @@ -99,15 +109,5 @@ public override IChangeToken GetChangeToken() return _compositeDataSource.GetChangeToken(); } - - public void Dispose() - { - _compositeDataSource?.Dispose(); - - foreach (var dataSource in _routeGroupBuilder._dataSources) - { - (dataSource as IDisposable)?.Dispose(); - } - } } } From 1aa70bb60e8d64b748c8fb8a45cbbf121b3ce2fb Mon Sep 17 00:00:00 2001 From: Stephen Halter Date: Sun, 19 Jun 2022 17:51:38 -0700 Subject: [PATCH 18/21] Add filters to RouteEndpointBuilder --- .../src/RequestDelegateFactory.cs | 95 +++++++++++++------ .../src/RequestDelegateFactoryOptions.cs | 4 + .../test/RequestDelegateFactoryTests.cs | 31 ++++++ .../Builder/EndpointRouteBuilderExtensions.cs | 15 ++- .../FallbackEndpointRouteBuilderExtensions.cs | 2 + .../Builder/RouteHandlerFilterExtensions.cs | 28 +++--- src/Http/Routing/src/RouteEndpointBuilder.cs | 31 +++++- .../Routing/src/RouteEndpointDataSource.cs | 19 +--- src/Http/Routing/src/RouteFilterMetadata.cs | 11 --- ...egateEndpointRouteBuilderExtensionsTest.cs | 68 ++++++++++++- ...aticFilesEndpointRouteBuilderExtensions.cs | 8 ++ 11 files changed, 238 insertions(+), 74 deletions(-) delete mode 100644 src/Http/Routing/src/RouteFilterMetadata.cs diff --git a/src/Http/Http.Extensions/src/RequestDelegateFactory.cs b/src/Http/Http.Extensions/src/RequestDelegateFactory.cs index cea842d12884..dab86be1e0c4 100644 --- a/src/Http/Http.Extensions/src/RequestDelegateFactory.cs +++ b/src/Http/Http.Extensions/src/RequestDelegateFactory.cs @@ -127,12 +127,19 @@ public static RequestDelegateResult Create(Delegate handler, RequestDelegateFact null => null, }; - var factoryContext = CreateFactoryContext(options); + var factoryContext = CreateFactoryContext(options, handler); Expression> targetFactory = (httpContext) => handler.Target; var targetableRequestDelegate = CreateTargetableRequestDelegate(handler.Method, targetExpression, factoryContext, targetFactory); + if (targetableRequestDelegate is null) + { + // handler is a RequestDelegate that has not been modified by a filter. Short-circuit and return the original RequestDelegate back. + // It's possible a filter factory has still modified the endpoint metadata though. + return new RequestDelegateResult((RequestDelegate)handler, AsReadOnlyList(factoryContext.Metadata)); + } + return new RequestDelegateResult(httpContext => targetableRequestDelegate(handler.Target, httpContext), AsReadOnlyList(factoryContext.Metadata)); } @@ -165,6 +172,9 @@ public static RequestDelegateResult Create(MethodInfo methodInfo, Func untargetableRequestDelegate(null, httpContext), AsReadOnlyList(factoryContext.Metadata)); } @@ -174,19 +184,23 @@ public static RequestDelegateResult Create(MethodInfo methodInfo, Func targetFactory(context)); + // CreateTargetableRequestDelegate can only return null given a RequestDelegate passed into the other RDF.Create() overload. + Debug.Assert(targetableRequestDelegate is not null); + return new RequestDelegateResult(httpContext => targetableRequestDelegate(targetFactory(httpContext), httpContext), AsReadOnlyList(factoryContext.Metadata)); } - private static FactoryContext CreateFactoryContext(RequestDelegateFactoryOptions? options) + private static FactoryContext CreateFactoryContext(RequestDelegateFactoryOptions? options, Delegate? handler = null) { return new FactoryContext { + Handler = handler, ServiceProvider = options?.ServiceProvider, ServiceProviderIsService = options?.ServiceProvider?.GetService(), RouteParameters = options?.RouteParameterNames?.ToList(), ThrowOnBadRequest = options?.ThrowOnBadRequest ?? false, DisableInferredFromBody = options?.DisableInferBodyFromParameters ?? false, - Filters = options?.RouteHandlerFilterFactories?.ToList(), + FilterFactories = options?.RouteHandlerFilterFactories?.ToList(), Metadata = options?.EndpointMetadata ?? new List(), }; } @@ -201,7 +215,7 @@ private static IReadOnlyList AsReadOnlyList(IList metadata) return new List(metadata); } - private static Func CreateTargetableRequestDelegate(MethodInfo methodInfo, Expression? targetExpression, FactoryContext factoryContext, Expression>? targetFactory = null) + private static Func? CreateTargetableRequestDelegate(MethodInfo methodInfo, Expression? targetExpression, FactoryContext factoryContext, Expression>? targetFactory = null) { // Non void return type @@ -230,22 +244,38 @@ private static IReadOnlyList AsReadOnlyList(IList metadata) factoryContext.ServiceProvider, CollectionsMarshal.AsSpan(factoryContext.Parameters)); + RouteHandlerFilterDelegate? filterPipeline = null; + // If there are filters registered on the route handler, then we update the method call and // return type associated with the request to allow for the filter invocation pipeline. - if (factoryContext.Filters is { Count: > 0 }) - { - var filterPipeline = CreateFilterPipeline(methodInfo, targetExpression, factoryContext, targetFactory); - Expression>> invokePipeline = (context) => filterPipeline(context); - returnType = typeof(ValueTask); - // var filterContext = new RouteHandlerInvocationContext(httpContext, name_local, int_local); - // invokePipeline.Invoke(filterContext); - factoryContext.MethodCall = Expression.Block( - new[] { InvokedFilterContextExpr }, - Expression.Assign( - InvokedFilterContextExpr, - CreateRouteHandlerInvocationContextBase(factoryContext)), - Expression.Invoke(invokePipeline, InvokedFilterContextExpr) - ); + if (factoryContext.FilterFactories is { Count: > 0 }) + { + filterPipeline = CreateFilterPipeline(methodInfo, targetExpression, factoryContext, targetFactory); + + if (filterPipeline is not null) + { + Expression>> invokePipeline = (context) => filterPipeline(context); + returnType = typeof(ValueTask); + // var filterContext = new RouteHandlerInvocationContext(httpContext, name_local, int_local); + // invokePipeline.Invoke(filterContext); + factoryContext.MethodCall = Expression.Block( + new[] { InvokedFilterContextExpr }, + Expression.Assign( + InvokedFilterContextExpr, + CreateRouteHandlerInvocationContextBase(factoryContext)), + Expression.Invoke(invokePipeline, InvokedFilterContextExpr) + ); + } + } + + // return null for plain RequestDelegates that have not been modified by filters so we can just pass back the original RequestDelegate. + if (filterPipeline is null && factoryContext.Handler is RequestDelegate) + { + // Make sure we're still not handling a return value. + if (!returnType.IsGenericType || returnType.GetGenericTypeDefinition() != typeof(Task<>)) + { + return null; + } } var responseWritingMethodCall = factoryContext.ParamCheckExpressions.Count > 0 ? @@ -260,9 +290,9 @@ private static IReadOnlyList AsReadOnlyList(IList metadata) return HandleRequestBodyAndCompileRequestDelegate(responseWritingMethodCall, factoryContext); } - private static RouteHandlerFilterDelegate CreateFilterPipeline(MethodInfo methodInfo, Expression? targetExpression, FactoryContext factoryContext, Expression>? targetFactory) + private static RouteHandlerFilterDelegate? CreateFilterPipeline(MethodInfo methodInfo, Expression? targetExpression, FactoryContext factoryContext, Expression>? targetFactory) { - Debug.Assert(factoryContext.Filters is not null); + Debug.Assert(factoryContext.FilterFactories is not null); // httpContext.Response.StatusCode >= 400 // ? Task.CompletedTask // : { @@ -306,14 +336,21 @@ targetExpression is null factoryContext.Metadata, factoryContext.ServiceProvider ?? EmptyServiceProvider.Instance); - for (var i = factoryContext.Filters.Count - 1; i >= 0; i--) + var initialFilteredInvocation = filteredInvocation; + + for (var i = factoryContext.FilterFactories.Count - 1; i >= 0; i--) { - var currentFilterFactory = factoryContext.Filters[i]; - var nextFilter = filteredInvocation; - var currentFilter = currentFilterFactory(routeHandlerContext, nextFilter); - filteredInvocation = (RouteHandlerInvocationContext context) => currentFilter(context); + var currentFilterFactory = factoryContext.FilterFactories[i]; + filteredInvocation = currentFilterFactory(routeHandlerContext, filteredInvocation); + } + // The filter factories have run without modifying per-request behavior, we can skip running the pipeline. + // If a plain old RequestDelegate was passed in (with no generic parameter), we can just return it back directly now. + if (ReferenceEquals(initialFilteredInvocation, filteredInvocation)) + { + return null; } + return filteredInvocation; } @@ -762,7 +799,7 @@ private static Expression CreateParamCheckingResponseWritingMethodCall(Type retu // If filters have been registered, we set the `wasParamCheckFailure` property // but do not return from the invocation to allow the filters to run. - if (factoryContext.Filters is { Count: > 0 }) + if (factoryContext.FilterFactories is { Count: > 0 }) { // if (wasParamCheckFailure) // { @@ -2053,6 +2090,9 @@ private static async Task ExecuteResultWriteResponse(IResult? result, HttpContex private sealed class FactoryContext { // Options + // Handler could be null if the MethodInfo overload of RDF.Create is used, but that doesn't matter because this is + // only referenced to optimize certain cases where a RequestDelegate is the handler and filters don't modify it. + public Delegate? Handler { get; init; } public IServiceProvider? ServiceProvider { get; init; } public IServiceProviderIsService? ServiceProviderIsService { get; init; } public List? RouteParameters { get; init; } @@ -2084,7 +2124,8 @@ private sealed class FactoryContext public Type[] ArgumentTypes { get; set; } = Array.Empty(); public Expression[] ArgumentExpressions { get; set; } = Array.Empty(); public Expression[] BoxedArgs { get; set; } = Array.Empty(); - public List>? Filters { get; init; } + public List>? FilterFactories { get; init; } + public bool FilterFactoriesHaveRunWithoutModifyingPerRequestBehavior { get; set; } public List Parameters { get; set; } = new(); } diff --git a/src/Http/Http.Extensions/src/RequestDelegateFactoryOptions.cs b/src/Http/Http.Extensions/src/RequestDelegateFactoryOptions.cs index c15644bfa12b..58e5d5041a9b 100644 --- a/src/Http/Http.Extensions/src/RequestDelegateFactoryOptions.cs +++ b/src/Http/Http.Extensions/src/RequestDelegateFactoryOptions.cs @@ -51,4 +51,8 @@ public sealed class RequestDelegateFactoryOptions /// insert their metadata at the start to give lower precedence, but this is unusual. /// public IList? EndpointMetadata { get; init; } + + // TODO: Add a RouteEndpointBuilder property and remove the EndpointMetadata property. Then do the same in RouteHandlerContext, EndpointMetadataContext + // and EndpointParameterMetadataContext. This will allow seeing the entire route pattern if the caller chooses to allow it. + // We'll probably want to add the RouteEndpointBuilder constructor without a RequestDelegate back and make it public too. } diff --git a/src/Http/Http.Extensions/test/RequestDelegateFactoryTests.cs b/src/Http/Http.Extensions/test/RequestDelegateFactoryTests.cs index 8372f808e082..6a2b7c4bfb1c 100644 --- a/src/Http/Http.Extensions/test/RequestDelegateFactoryTests.cs +++ b/src/Http/Http.Extensions/test/RequestDelegateFactoryTests.cs @@ -6081,6 +6081,37 @@ public void Create_SetsApplicationServices_OnEndpointParameterMetadataContext() Assert.Contains(result.EndpointMetadata, m => m is MetadataService); } + [Fact] + public void Create_ReturnsSameRequestDelegatePassedIn_IfNotModifiedByFilters() + { + RequestDelegate initialRequestDelegate = static (context) => Task.CompletedTask; + var filter1Tag = new TagsAttribute("filter1"); + var filter2Tag = new TagsAttribute("filter2"); + + RequestDelegateFactoryOptions options = new() + { + RouteHandlerFilterFactories = new List>() + { + (routeHandlerContext, next) => + { + routeHandlerContext.EndpointMetadata.Add(filter1Tag); + return next; + }, + (routeHandlerContext, next) => + { + routeHandlerContext.EndpointMetadata.Add(filter2Tag); + return next; + }, + } + }; + + var result = RequestDelegateFactory.Create(initialRequestDelegate, options); + Assert.Same(initialRequestDelegate, result.RequestDelegate); + Assert.Collection(result.EndpointMetadata, + m => Assert.Same(filter2Tag, m), + m => Assert.Same(filter1Tag, m)); + } + private DefaultHttpContext CreateHttpContext() { var responseFeature = new TestHttpResponseFeature(); diff --git a/src/Http/Routing/src/Builder/EndpointRouteBuilderExtensions.cs b/src/Http/Routing/src/Builder/EndpointRouteBuilderExtensions.cs index 35a78918cb5e..3c593153c8ff 100644 --- a/src/Http/Routing/src/Builder/EndpointRouteBuilderExtensions.cs +++ b/src/Http/Routing/src/Builder/EndpointRouteBuilderExtensions.cs @@ -63,7 +63,7 @@ public static RouteGroupBuilder MapGroup(this IEndpointRouteBuilder endpoints, R /// The route pattern. /// The delegate executed when the endpoint is matched. /// A that can be used to further customize the endpoint. - [RequiresUnreferencedCode(EndpointRouteBuilderExtensions.MapEndpointTrimmerWarning)] + [RequiresUnreferencedCode(MapEndpointTrimmerWarning)] public static IEndpointConventionBuilder MapGet( this IEndpointRouteBuilder endpoints, [StringSyntax("Route")] string pattern, @@ -85,6 +85,7 @@ public static IEndpointConventionBuilder MapGet( /// The route pattern. /// The delegate executed when the endpoint is matched. /// A that can be used to further customize the endpoint. + [RequiresUnreferencedCode(MapEndpointTrimmerWarning)] public static IEndpointConventionBuilder MapPost( this IEndpointRouteBuilder endpoints, [StringSyntax("Route")] string pattern, @@ -101,6 +102,7 @@ public static IEndpointConventionBuilder MapPost( /// The route pattern. /// The delegate executed when the endpoint is matched. /// A that can be used to further customize the endpoint. + [RequiresUnreferencedCode(MapEndpointTrimmerWarning)] public static IEndpointConventionBuilder MapPut( this IEndpointRouteBuilder endpoints, [StringSyntax("Route")] string pattern, @@ -117,6 +119,7 @@ public static IEndpointConventionBuilder MapPut( /// The route pattern. /// The delegate executed when the endpoint is matched. /// A that can be used to further customize the endpoint. + [RequiresUnreferencedCode(MapEndpointTrimmerWarning)] public static IEndpointConventionBuilder MapDelete( this IEndpointRouteBuilder endpoints, [StringSyntax("Route")] string pattern, @@ -133,6 +136,7 @@ public static IEndpointConventionBuilder MapDelete( /// The route pattern. /// The delegate executed when the endpoint is matched. /// A that can be used to further customize the endpoint. + [RequiresUnreferencedCode(MapEndpointTrimmerWarning)] public static IEndpointConventionBuilder MapPatch( this IEndpointRouteBuilder endpoints, [StringSyntax("Route")] string pattern, @@ -150,6 +154,7 @@ public static IEndpointConventionBuilder MapPatch( /// The delegate executed when the endpoint is matched. /// HTTP methods that the endpoint will match. /// A that can be used to further customize the endpoint. + [RequiresUnreferencedCode(MapEndpointTrimmerWarning)] public static IEndpointConventionBuilder MapMethods( this IEndpointRouteBuilder endpoints, [StringSyntax("Route")] string pattern, @@ -172,6 +177,7 @@ public static IEndpointConventionBuilder MapMethods( /// The route pattern. /// The delegate executed when the endpoint is matched. /// A that can be used to further customize the endpoint. + [RequiresUnreferencedCode(MapEndpointTrimmerWarning)] public static IEndpointConventionBuilder Map( this IEndpointRouteBuilder endpoints, [StringSyntax("Route")] string pattern, @@ -188,6 +194,7 @@ public static IEndpointConventionBuilder Map( /// The route pattern. /// The delegate executed when the endpoint is matched. /// A that can be used to further customize the endpoint. + [RequiresUnreferencedCode(MapEndpointTrimmerWarning)] public static IEndpointConventionBuilder Map( this IEndpointRouteBuilder endpoints, RoutePattern pattern, @@ -197,6 +204,12 @@ public static IEndpointConventionBuilder Map( ArgumentNullException.ThrowIfNull(pattern); ArgumentNullException.ThrowIfNull(requestDelegate); + var returnType = requestDelegate.Method.ReturnType; + if (returnType is { IsGenericType: true } && returnType.GetGenericTypeDefinition() == typeof(Task<>)) + { + return Map(endpoints, pattern, requestDelegate as Delegate); + } + const int defaultOrder = 0; var builder = new RouteEndpointBuilder( diff --git a/src/Http/Routing/src/Builder/FallbackEndpointRouteBuilderExtensions.cs b/src/Http/Routing/src/Builder/FallbackEndpointRouteBuilderExtensions.cs index d8c851bbd7dd..173cfb59f468 100644 --- a/src/Http/Routing/src/Builder/FallbackEndpointRouteBuilderExtensions.cs +++ b/src/Http/Routing/src/Builder/FallbackEndpointRouteBuilderExtensions.cs @@ -36,6 +36,7 @@ public static class FallbackEndpointRouteBuilderExtensions /// {*path:nonfile}. The order of the registered endpoint will be int.MaxValue. /// /// + [RequiresUnreferencedCode(EndpointRouteBuilderExtensions.MapEndpointTrimmerWarning)] public static IEndpointConventionBuilder MapFallback(this IEndpointRouteBuilder endpoints, RequestDelegate requestDelegate) { ArgumentNullException.ThrowIfNull(endpoints); @@ -65,6 +66,7 @@ public static IEndpointConventionBuilder MapFallback(this IEndpointRouteBuilder /// to exclude requests for static files. /// /// + [RequiresUnreferencedCode(EndpointRouteBuilderExtensions.MapEndpointTrimmerWarning)] public static IEndpointConventionBuilder MapFallback( this IEndpointRouteBuilder endpoints, [StringSyntax("Route")] string pattern, diff --git a/src/Http/Routing/src/Builder/RouteHandlerFilterExtensions.cs b/src/Http/Routing/src/Builder/RouteHandlerFilterExtensions.cs index 637ec5fc15e1..69c018cbb3ec 100644 --- a/src/Http/Routing/src/Builder/RouteHandlerFilterExtensions.cs +++ b/src/Http/Routing/src/Builder/RouteHandlerFilterExtensions.cs @@ -19,6 +19,8 @@ public static class RouteHandlerFilterExtensions /// The . /// The to register. /// A that can be used to further customize the route handler. + + [RequiresUnreferencedCode(EndpointRouteBuilderExtensions.MapEndpointTrimmerWarning)] public static TBuilder AddRouteHandlerFilter(this TBuilder builder, IRouteHandlerFilter filter) where TBuilder : IEndpointConventionBuilder => builder.AddRouteHandlerFilter((routeHandlerContext, next) => (context) => filter.InvokeAsync(context, next)); @@ -29,6 +31,7 @@ public static TBuilder AddRouteHandlerFilter(this TBuilder builder, IR /// The type of the to register. /// The . /// A that can be used to further customize the route handler. + [RequiresUnreferencedCode(EndpointRouteBuilderExtensions.MapEndpointTrimmerWarning)] public static TBuilder AddRouteHandlerFilter(this TBuilder builder) where TBuilder : IEndpointConventionBuilder where TFilterType : IRouteHandlerFilter @@ -63,6 +66,7 @@ public static TBuilder AddRouteHandlerFilter(this TBuilder builder, IR /// The type of the to register. /// The . /// A that can be used to further customize the route handler. + [RequiresUnreferencedCode(EndpointRouteBuilderExtensions.MapEndpointTrimmerWarning)] public static RouteHandlerBuilder AddRouteHandlerFilter<[DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.PublicConstructors)] TFilterType>(this RouteHandlerBuilder builder) where TFilterType : IRouteHandlerFilter { @@ -76,6 +80,7 @@ public static TBuilder AddRouteHandlerFilter(this TBuilder builder, IR /// The type of the to register. /// The . /// A that can be used to further customize the route handler. + [RequiresUnreferencedCode(EndpointRouteBuilderExtensions.MapEndpointTrimmerWarning)] public static RouteGroupBuilder AddRouteHandlerFilter<[DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.PublicConstructors)] TFilterType>(this RouteGroupBuilder builder) where TFilterType : IRouteHandlerFilter { @@ -89,6 +94,7 @@ public static TBuilder AddRouteHandlerFilter(this TBuilder builder, IR /// The . /// A method representing the core logic of the filter. /// A that can be used to further customize the route handler. + [RequiresUnreferencedCode(EndpointRouteBuilderExtensions.MapEndpointTrimmerWarning)] public static TBuilder AddRouteHandlerFilter(this TBuilder builder, Func> routeHandlerFilter) where TBuilder : IEndpointConventionBuilder { @@ -101,31 +107,19 @@ public static TBuilder AddRouteHandlerFilter(this TBuilder builder, Fu /// The . /// A method representing the logic for constructing the filter. /// A that can be used to further customize the route handler. + [RequiresUnreferencedCode(EndpointRouteBuilderExtensions.MapEndpointTrimmerWarning)] public static TBuilder AddRouteHandlerFilter(this TBuilder builder, Func filterFactory) where TBuilder : IEndpointConventionBuilder { builder.Add(endpointBuilder => { - // Even if we split RouteFilterMetadata across multiple metadata objects, RouteEndointDataSource will still use all of them. - // We're just trying to be good citizens and keep the Endpoint.Metadata as small as possible. - RouteHandlerFilterMetadata? filterMetadata = null; - - foreach (var item in endpointBuilder.Metadata) - { - if (item is RouteHandlerFilterMetadata metadata) - { - // Don't break early in case there is somehow higher priority metadata later in the list. - filterMetadata = metadata; - } - } - - if (filterMetadata is null) + if (endpointBuilder is not RouteEndpointBuilder routeEndpointBuilder) { - filterMetadata = new(); - endpointBuilder.Metadata.Add(filterMetadata); + return; } - filterMetadata.FilterFactories.Add(filterFactory); + routeEndpointBuilder.RouteHandlerFilterFactories ??= new(); + routeEndpointBuilder.RouteHandlerFilterFactories.Add(filterFactory); }); return builder; diff --git a/src/Http/Routing/src/RouteEndpointBuilder.cs b/src/Http/Routing/src/RouteEndpointBuilder.cs index bec1185ce51a..5fe372b76b52 100644 --- a/src/Http/Routing/src/RouteEndpointBuilder.cs +++ b/src/Http/Routing/src/RouteEndpointBuilder.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 Microsoft.AspNetCore.Builder; using Microsoft.AspNetCore.Http; using Microsoft.AspNetCore.Routing.Patterns; @@ -12,6 +13,10 @@ namespace Microsoft.AspNetCore.Routing; /// public sealed class RouteEndpointBuilder : EndpointBuilder { + // TODO: Make this public as a gettable IReadOnlyList>. + // AddRouteHandlerFilter will still be the only way to mutate this list. + internal List>? RouteHandlerFilterFactories { get; set; } + /// /// Gets or sets the associated with this endpoint. /// @@ -39,6 +44,8 @@ public RouteEndpointBuilder( } /// + [UnconditionalSuppressMessage("Trimmer", "IL2026", + Justification = "We surface a RequireUnreferencedCode in AddRouteHandlerFilter which is required to call unreferenced code here. The trimmer is unable to infer this.")] public override Endpoint Build() { if (RequestDelegate is null) @@ -46,8 +53,30 @@ 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, MapHealChecks, etc...), + // ActionEndpointDataSourceBase (MapControllers, MapRozorPages, 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 (RouteHandlerFilterFactories 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() + { + RouteHandlerFilterFactories = RouteHandlerFilterFactories, + 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, + requestDelegate, RoutePattern, Order, new EndpointMetadataCollection(Metadata), diff --git a/src/Http/Routing/src/RouteEndpointDataSource.cs b/src/Http/Routing/src/RouteEndpointDataSource.cs index 40f034cc183c..a9648210d1e8 100644 --- a/src/Http/Routing/src/RouteEndpointDataSource.cs +++ b/src/Http/Routing/src/RouteEndpointDataSource.cs @@ -82,7 +82,7 @@ internal RouteEndpointBuilder GetSingleRouteEndpointBuilder() } [UnconditionalSuppressMessage("Trimmer", "IL2026", - Justification = "We surface a RequireUnreferencedCode in the call to Map method adding this EndpointDataSource. The trimmer is unable to infer this.")] + Justification = "We surface a RequireUnreferencedCode in the call to the Map method adding this EndpointDataSource. The trimmer is unable to infer this.")] private RouteEndpointBuilder CreateRouteEndpointBuilder( RouteEntry entry, RoutePattern? groupPrefix = null, IReadOnlyList>? groupConventions = null) { @@ -166,18 +166,6 @@ private RouteEndpointBuilder CreateRouteEndpointBuilder( entrySpecificConvention(builder); } - // Let's see if any of the conventions added a filter before creating the RequestDelegate. - List>? routeHandlerFilterFactories = null; - - foreach (var item in builder.Metadata) - { - if (item is RouteHandlerFilterMetadata filterMetadata) - { - routeHandlerFilterFactories ??= new(); - routeHandlerFilterFactories.AddRange(filterMetadata.FilterFactories); - } - } - var routeParamNames = new List(pattern.Parameters.Count); foreach (var parameter in pattern.Parameters) { @@ -191,12 +179,15 @@ private RouteEndpointBuilder CreateRouteEndpointBuilder( ThrowOnBadRequest = _throwOnBadRequest, DisableInferBodyFromParameters = ShouldDisableInferredBodyParameters(entry.HttpMethods), EndpointMetadata = builder.Metadata, - RouteHandlerFilterFactories = routeHandlerFilterFactories, + RouteHandlerFilterFactories = builder.RouteHandlerFilterFactories, }; // We ignore the returned EndpointMetadata has been already populated since we passed in non-null EndpointMetadata. factoryCreatedRequestDelegate = RequestDelegateFactory.Create(entry.RouteHandler, factoryOptions).RequestDelegate; + // 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.RouteHandlerFilterFactories = null; + if (ReferenceEquals(builder.RequestDelegate, redirectedRequestDelegate)) { // No convention has changed builder.RequestDelegate, so we can just replace it with the final version as an optimization. diff --git a/src/Http/Routing/src/RouteFilterMetadata.cs b/src/Http/Routing/src/RouteFilterMetadata.cs deleted file mode 100644 index ed9f5b73dbd0..000000000000 --- a/src/Http/Routing/src/RouteFilterMetadata.cs +++ /dev/null @@ -1,11 +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 Microsoft.AspNetCore.Http; - -namespace Microsoft.AspNetCore.Routing; - -internal class RouteHandlerFilterMetadata -{ - public List> FilterFactories { get; } = new(); -} diff --git a/src/Http/Routing/test/UnitTests/Builder/RequestDelegateEndpointRouteBuilderExtensionsTest.cs b/src/Http/Routing/test/UnitTests/Builder/RequestDelegateEndpointRouteBuilderExtensionsTest.cs index 58c3e7ca1ab3..292a8b6ff575 100644 --- a/src/Http/Routing/test/UnitTests/Builder/RequestDelegateEndpointRouteBuilderExtensionsTest.cs +++ b/src/Http/Routing/test/UnitTests/Builder/RequestDelegateEndpointRouteBuilderExtensionsTest.cs @@ -79,8 +79,9 @@ public void MapEndpoint_StringPattern_BuildsEndpoint() Assert.Equal("/", endpointBuilder1.RoutePattern.RawText); } - [Fact] - public async Task MapEndpoint_ReturnGenericTypeTask_GeneratedDelegate() + [Theory] + [MemberData(nameof(MapMethods))] + public async Task MapEndpoint_ReturnGenericTypeTask_GeneratedDelegate(Func map) { var httpContext = new DefaultHttpContext(); var responseBodyStream = new MemoryStream(); @@ -91,7 +92,7 @@ public async Task MapEndpoint_ReturnGenericTypeTask_GeneratedDelegate() static async Task GenericTypeTaskDelegate(HttpContext context) => await Task.FromResult("String Test"); // Act - var endpointBuilder = builder.MapGet("/", GenericTypeTaskDelegate); + var endpointBuilder = map(builder, "/", GenericTypeTaskDelegate); // Assert var dataSource = GetBuilderEndpointDataSource(builder); @@ -106,6 +107,67 @@ public async Task MapEndpoint_ReturnGenericTypeTask_GeneratedDelegate() Assert.Equal("String Test", responseBody); } + [Theory] + [MemberData(nameof(MapMethods))] + public async Task MapEndpoint_CanBeFiltered_ByRouteHandlerFilters(Func map) + { + var builder = new DefaultEndpointRouteBuilder(new ApplicationBuilder(EmptyServiceProvider.Instance)); + var httpContext = new DefaultHttpContext(); + var responseBodyStream = new MemoryStream(); + httpContext.Response.Body = responseBodyStream; + + RequestDelegate initialRequestDelegate = static (context) => Task.CompletedTask; + var filterTag = new TagsAttribute("filter"); + + var endpointBuilder = map(builder, "/", initialRequestDelegate).AddRouteHandlerFilter(filterFactory: (routeHandlerContext, next) => + { + routeHandlerContext.EndpointMetadata.Add(filterTag); + return async invocationContext => + { + Assert.IsAssignableFrom(Assert.Single(invocationContext.Arguments)); + // Ignore thre result and write filtered because we can! + await next(invocationContext); + return "filtered!"; + }; + }); + + var dataSource = GetBuilderEndpointDataSource(builder); + var endpoint = Assert.Single(dataSource.Endpoints); + + Assert.NotSame(initialRequestDelegate, endpoint.RequestDelegate); + Assert.Same(filterTag, endpoint.Metadata.GetMetadata()); + + Assert.NotNull(endpoint.RequestDelegate); + var requestDelegate = endpoint.RequestDelegate!; + await requestDelegate(httpContext); + + var responseBody = Encoding.UTF8.GetString(responseBodyStream.ToArray()); + + Assert.Equal("filtered!", responseBody); + } + + [Theory] + [MemberData(nameof(MapMethods))] + public void MapEndpoint_UsesOriginalRequestDelegateInstance_IfFilterDoesNotChangePerRequestBehavior(Func map) + { + var builder = new DefaultEndpointRouteBuilder(new ApplicationBuilder(EmptyServiceProvider.Instance)); + + RequestDelegate initialRequestDelegate = static (context) => Task.CompletedTask; + var filterTag = new TagsAttribute("filter"); + + var endpointBuilder = map(builder, "/", initialRequestDelegate).AddRouteHandlerFilter((routeHandlerContext, next) => + { + routeHandlerContext.EndpointMetadata.Add(filterTag); + return next; + }); + + var dataSource = GetBuilderEndpointDataSource(builder); + var endpoint = Assert.Single(dataSource.Endpoints); + + Assert.Same(initialRequestDelegate, endpoint.RequestDelegate); + Assert.Same(filterTag, endpoint.Metadata.GetMetadata()); + } + [Fact] public void MapEndpoint_TypedPattern_BuildsEndpoint() { diff --git a/src/Middleware/StaticFiles/src/StaticFilesEndpointRouteBuilderExtensions.cs b/src/Middleware/StaticFiles/src/StaticFilesEndpointRouteBuilderExtensions.cs index deee04917f8c..d13c44d6f5f9 100644 --- a/src/Middleware/StaticFiles/src/StaticFilesEndpointRouteBuilderExtensions.cs +++ b/src/Middleware/StaticFiles/src/StaticFilesEndpointRouteBuilderExtensions.cs @@ -36,6 +36,8 @@ public static class StaticFilesEndpointRouteBuilderExtensions /// {*path:nonfile}. The order of the registered endpoint will be int.MaxValue. /// /// + [UnconditionalSuppressMessage("Trimmer", "IL2026", + Justification = "MapFallback only RequireUnreferencedCode if the RequestDelegate has a Task return type which is not the case here.")] public static IEndpointConventionBuilder MapFallbackToFile( this IEndpointRouteBuilder endpoints, string filePath) @@ -74,6 +76,8 @@ public static IEndpointConventionBuilder MapFallbackToFile( /// {*path:nonfile}. The order of the registered endpoint will be int.MaxValue. /// /// + [UnconditionalSuppressMessage("Trimmer", "IL2026", + Justification = "MapFallback only RequireUnreferencedCode if the RequestDelegate has a Task return type which is not the case here.")] public static IEndpointConventionBuilder MapFallbackToFile( this IEndpointRouteBuilder endpoints, string filePath, @@ -119,6 +123,8 @@ public static IEndpointConventionBuilder MapFallbackToFile( /// to exclude requests for static files. /// /// + [UnconditionalSuppressMessage("Trimmer", "IL2026", + Justification = "MapFallback only RequireUnreferencedCode if the RequestDelegate has a Task return type which is not the case here.")] public static IEndpointConventionBuilder MapFallbackToFile( this IEndpointRouteBuilder endpoints, [StringSyntax("Route")] string pattern, @@ -167,6 +173,8 @@ public static IEndpointConventionBuilder MapFallbackToFile( /// to exclude requests for static files. /// /// + [UnconditionalSuppressMessage("Trimmer", "IL2026", + Justification = "MapFallback only RequireUnreferencedCode if the RequestDelegate has a Task return type which is not the case.")] public static IEndpointConventionBuilder MapFallbackToFile( this IEndpointRouteBuilder endpoints, [StringSyntax("Route")] string pattern, From 9760dcb44ddd77ec5f4db00fb6d0101151d77cdb Mon Sep 17 00:00:00 2001 From: Stephen Halter Date: Sun, 19 Jun 2022 19:30:32 -0700 Subject: [PATCH 19/21] Remove redundant RequestDelegate returnType check --- .../Routing/src/Builder/EndpointRouteBuilderExtensions.cs | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-) diff --git a/src/Http/Routing/src/Builder/EndpointRouteBuilderExtensions.cs b/src/Http/Routing/src/Builder/EndpointRouteBuilderExtensions.cs index 3c593153c8ff..250028ea951b 100644 --- a/src/Http/Routing/src/Builder/EndpointRouteBuilderExtensions.cs +++ b/src/Http/Routing/src/Builder/EndpointRouteBuilderExtensions.cs @@ -69,11 +69,6 @@ public static IEndpointConventionBuilder MapGet( [StringSyntax("Route")] string pattern, RequestDelegate requestDelegate) { - var returnType = requestDelegate.Method.ReturnType; - if (returnType is { IsGenericType: true } && returnType.GetGenericTypeDefinition() == typeof(Task<>)) - { - return MapMethods(endpoints, pattern, GetVerb, requestDelegate as Delegate); - } return MapMethods(endpoints, pattern, GetVerb, requestDelegate); } @@ -205,7 +200,7 @@ public static IEndpointConventionBuilder Map( ArgumentNullException.ThrowIfNull(requestDelegate); var returnType = requestDelegate.Method.ReturnType; - if (returnType is { IsGenericType: true } && returnType.GetGenericTypeDefinition() == typeof(Task<>)) + if (returnType.IsGenericType && returnType.GetGenericTypeDefinition() == typeof(Task<>)) { return Map(endpoints, pattern, requestDelegate as Delegate); } From f094c4813521cb091e5665faba1cf6fc377a2282 Mon Sep 17 00:00:00 2001 From: Stephen Halter Date: Sun, 19 Jun 2022 19:54:43 -0700 Subject: [PATCH 20/21] Fix merge conflict with RDF AOT changes - also fix comment --- src/Http/Http.Extensions/src/RequestDelegateFactory.cs | 2 +- src/Http/Routing/src/RouteEndpointBuilder.cs | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/src/Http/Http.Extensions/src/RequestDelegateFactory.cs b/src/Http/Http.Extensions/src/RequestDelegateFactory.cs index 6f2950ce0305..b508e90b72cc 100644 --- a/src/Http/Http.Extensions/src/RequestDelegateFactory.cs +++ b/src/Http/Http.Extensions/src/RequestDelegateFactory.cs @@ -552,7 +552,7 @@ private static Expression[] CreateArguments(ParameterInfo[]? parameters, Factory factoryContext.BoxedArgs = new Expression[parameters.Length]; factoryContext.Parameters = new List(parameters); - var hasFilters = factoryContext.Filters is { Count: > 0 }; + var hasFilters = factoryContext.FilterFactories is { Count: > 0 }; for (var i = 0; i < parameters.Length; i++) { diff --git a/src/Http/Routing/src/RouteEndpointBuilder.cs b/src/Http/Routing/src/RouteEndpointBuilder.cs index 5fe372b76b52..f037bea26bb2 100644 --- a/src/Http/Routing/src/RouteEndpointBuilder.cs +++ b/src/Http/Routing/src/RouteEndpointBuilder.cs @@ -57,8 +57,8 @@ public override Endpoint Build() // 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, MapHealChecks, etc...), - // ActionEndpointDataSourceBase (MapControllers, MapRozorPages, etc...) and people with custom data sources or otherwise manually building 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 (RouteHandlerFilterFactories is { Count: > 0 }) { From 8e4ece67cb1583dfa627b3dd834637671b45e064 Mon Sep 17 00:00:00 2001 From: Stephen Halter Date: Sun, 19 Jun 2022 20:38:33 -0700 Subject: [PATCH 21/21] Add missed MapHealthChecks suppression --- .../Builder/HealthCheckEndpointRouteBuilderExtensions.cs | 3 +++ .../src/StaticFilesEndpointRouteBuilderExtensions.cs | 8 ++++---- 2 files changed, 7 insertions(+), 4 deletions(-) diff --git a/src/Middleware/HealthChecks/src/Builder/HealthCheckEndpointRouteBuilderExtensions.cs b/src/Middleware/HealthChecks/src/Builder/HealthCheckEndpointRouteBuilderExtensions.cs index c4586cd074f6..069d874281cd 100644 --- a/src/Middleware/HealthChecks/src/Builder/HealthCheckEndpointRouteBuilderExtensions.cs +++ b/src/Middleware/HealthChecks/src/Builder/HealthCheckEndpointRouteBuilderExtensions.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 Microsoft.AspNetCore.Diagnostics.HealthChecks; using Microsoft.AspNetCore.Routing; using Microsoft.Extensions.DependencyInjection; @@ -59,6 +60,8 @@ public static IEndpointConventionBuilder MapHealthChecks( return MapHealthChecksCore(endpoints, pattern, options); } + [UnconditionalSuppressMessage("Trimmer", "IL2026", + Justification = "MapHealthChecksCore only RequireUnreferencedCode if the RequestDelegate has a Task return type which is not the case here.")] private static IEndpointConventionBuilder MapHealthChecksCore(IEndpointRouteBuilder endpoints, string pattern, HealthCheckOptions? options) { if (endpoints.ServiceProvider.GetService(typeof(HealthCheckService)) == null) diff --git a/src/Middleware/StaticFiles/src/StaticFilesEndpointRouteBuilderExtensions.cs b/src/Middleware/StaticFiles/src/StaticFilesEndpointRouteBuilderExtensions.cs index d13c44d6f5f9..ca0162e98c12 100644 --- a/src/Middleware/StaticFiles/src/StaticFilesEndpointRouteBuilderExtensions.cs +++ b/src/Middleware/StaticFiles/src/StaticFilesEndpointRouteBuilderExtensions.cs @@ -37,7 +37,7 @@ public static class StaticFilesEndpointRouteBuilderExtensions /// /// [UnconditionalSuppressMessage("Trimmer", "IL2026", - Justification = "MapFallback only RequireUnreferencedCode if the RequestDelegate has a Task return type which is not the case here.")] + Justification = "MapFallbackToFile RequireUnreferencedCode if the RequestDelegate has a Task return type which is not the case here.")] public static IEndpointConventionBuilder MapFallbackToFile( this IEndpointRouteBuilder endpoints, string filePath) @@ -77,7 +77,7 @@ public static IEndpointConventionBuilder MapFallbackToFile( /// /// [UnconditionalSuppressMessage("Trimmer", "IL2026", - Justification = "MapFallback only RequireUnreferencedCode if the RequestDelegate has a Task return type which is not the case here.")] + Justification = "MapFallbackToFile RequireUnreferencedCode if the RequestDelegate has a Task return type which is not the case here.")] public static IEndpointConventionBuilder MapFallbackToFile( this IEndpointRouteBuilder endpoints, string filePath, @@ -124,7 +124,7 @@ public static IEndpointConventionBuilder MapFallbackToFile( /// /// [UnconditionalSuppressMessage("Trimmer", "IL2026", - Justification = "MapFallback only RequireUnreferencedCode if the RequestDelegate has a Task return type which is not the case here.")] + Justification = "MapFallbackToFile RequireUnreferencedCode if the RequestDelegate has a Task return type which is not the case here.")] public static IEndpointConventionBuilder MapFallbackToFile( this IEndpointRouteBuilder endpoints, [StringSyntax("Route")] string pattern, @@ -174,7 +174,7 @@ public static IEndpointConventionBuilder MapFallbackToFile( /// /// [UnconditionalSuppressMessage("Trimmer", "IL2026", - Justification = "MapFallback only RequireUnreferencedCode if the RequestDelegate has a Task return type which is not the case.")] + Justification = "MapFallbackToFile RequireUnreferencedCode if the RequestDelegate has a Task return type which is not the case.")] public static IEndpointConventionBuilder MapFallbackToFile( this IEndpointRouteBuilder endpoints, [StringSyntax("Route")] string pattern,