From 945e798ca78f749f62dcf2b24d99d9e82d17ef26 Mon Sep 17 00:00:00 2001 From: Ryan Nowak Date: Wed, 22 Aug 2018 08:06:34 -0700 Subject: [PATCH] [Design] Set the endpoint feature only on success --- .../EndpointMiddleware.cs | 18 ++--- .../EndpointRoutingMiddleware.cs | 14 ++-- .../EndpointRoutingBuilderExtensionsTest.cs | 42 +++++++++-- .../EndpointMiddlewareTest.cs | 75 +++++++++++++++---- 4 files changed, 110 insertions(+), 39 deletions(-) diff --git a/src/Microsoft.AspNetCore.Routing/EndpointMiddleware.cs b/src/Microsoft.AspNetCore.Routing/EndpointMiddleware.cs index 46a2c36a25..a9790e6ee1 100644 --- a/src/Microsoft.AspNetCore.Routing/EndpointMiddleware.cs +++ b/src/Microsoft.AspNetCore.Routing/EndpointMiddleware.cs @@ -32,26 +32,18 @@ namespace Microsoft.AspNetCore.Routing public async Task Invoke(HttpContext httpContext) { - var feature = httpContext.Features.Get(); - if (feature == null) + var endpoint = httpContext.Features.Get()?.Endpoint; + if (endpoint?.RequestDelegate != null) { - var message = $"Unable to execute an endpoint because the {nameof(EndpointRoutingMiddleware)} was not run for this request. " + - $"Ensure {nameof(EndpointRoutingMiddleware)} is added to the request execution pipeline before {nameof(EndpointMiddleware)} in application startup code."; - - throw new InvalidOperationException(message); - } - - if (feature.Endpoint?.RequestDelegate != null) - { - Log.ExecutingEndpoint(_logger, feature.Endpoint); + Log.ExecutingEndpoint(_logger, endpoint); try { - await feature.Endpoint.RequestDelegate(httpContext); + await endpoint.RequestDelegate(httpContext); } finally { - Log.ExecutedEndpoint(_logger, feature.Endpoint); + Log.ExecutedEndpoint(_logger, endpoint); } return; diff --git a/src/Microsoft.AspNetCore.Routing/EndpointRoutingMiddleware.cs b/src/Microsoft.AspNetCore.Routing/EndpointRoutingMiddleware.cs index c4c09d5de7..1b0385e212 100644 --- a/src/Microsoft.AspNetCore.Routing/EndpointRoutingMiddleware.cs +++ b/src/Microsoft.AspNetCore.Routing/EndpointRoutingMiddleware.cs @@ -55,11 +55,8 @@ namespace Microsoft.AspNetCore.Routing public async Task Invoke(HttpContext httpContext) { - // For back-compat EndpointRouteValuesFeature implements IEndpointFeature, IRouteValuesFeature and IRoutingFeature var feature = new EndpointFeature(); - SetEndpointFeature(httpContext, feature); - // There's an inherent race condition between waiting for init and accessing the matcher // this is OK because once `_matcher` is initialized, it will not be set to null again. var matcher = await InitializeAsync(); @@ -67,6 +64,10 @@ namespace Microsoft.AspNetCore.Routing await matcher.MatchAsync(httpContext, feature); if (feature.Endpoint != null) { + // Set the endpoint feature only on success. This means we won't overwrite any + // existing state for related features unless we did something. + SetEndpointFeature(httpContext, feature); + Log.MatchSuccess(_logger, feature); } else @@ -79,11 +80,8 @@ namespace Microsoft.AspNetCore.Routing private static void SetEndpointFeature(HttpContext httpContext, EndpointFeature feature) { - // An IRouteValuesFeature might have already been set - // Copy its RouteValues collection if present - var currentRouteValuesFeature = httpContext.Features.Get(); - feature.RouteValues = currentRouteValuesFeature?.RouteValues; - + // For back-compat EndpointRouteValuesFeature implements IEndpointFeature, + // IRouteValuesFeature and IRoutingFeature httpContext.Features.Set(feature); httpContext.Features.Set(feature); httpContext.Features.Set(feature); diff --git a/test/Microsoft.AspNetCore.Routing.Tests/Builder/EndpointRoutingBuilderExtensionsTest.cs b/test/Microsoft.AspNetCore.Routing.Tests/Builder/EndpointRoutingBuilderExtensionsTest.cs index 028b8a6c60..106a47e27b 100644 --- a/test/Microsoft.AspNetCore.Routing.Tests/Builder/EndpointRoutingBuilderExtensionsTest.cs +++ b/test/Microsoft.AspNetCore.Routing.Tests/Builder/EndpointRoutingBuilderExtensionsTest.cs @@ -8,6 +8,7 @@ using Microsoft.AspNetCore.Http; using Microsoft.AspNetCore.Http.Features; using Microsoft.AspNetCore.Internal; using Microsoft.AspNetCore.Routing; +using Microsoft.AspNetCore.Routing.Patterns; using Microsoft.Extensions.DependencyInjection; using Moq; using Xunit; @@ -51,7 +52,7 @@ namespace Microsoft.AspNetCore.Builder } [Fact] - public async Task UseEndpointRouting_ServicesRegistered_SetsFeature() + public async Task UseEndpointRouting_ServicesRegistered_NoMatch_DoesNotSetFeature() { // Arrange var services = CreateServices(); @@ -67,7 +68,36 @@ namespace Microsoft.AspNetCore.Builder await appFunc(httpContext); // Assert - Assert.NotNull(httpContext.Features.Get()); + Assert.Null(httpContext.Features.Get()); + } + + [Fact] + public async Task UseEndpointRouting_ServicesRegistered_Match_DoesNotSetsFeature() + { + // Arrange + var endpoint = new RouteEndpoint( + TestConstants.EmptyRequestDelegate, + RoutePatternFactory.Parse("{*p}"), + 0, + EndpointMetadataCollection.Empty, + "Test"); + + var services = CreateServices(endpoint); + + var app = new ApplicationBuilder(services); + + app.UseEndpointRouting(); + + var appFunc = app.Build(); + var httpContext = new DefaultHttpContext(); + + // Act + await appFunc(httpContext); + + // Assert + var feature = httpContext.Features.Get(); + Assert.NotNull(feature); + Assert.Same(endpoint, feature.Endpoint); } [Fact] @@ -90,7 +120,7 @@ namespace Microsoft.AspNetCore.Builder } [Fact] - public async Task UseEndpoint_ServicesRegisteredAndEndpointRoutingRegistered_SetsFeature() + public async Task UseEndpoint_ServicesRegisteredAndEndpointRoutingRegistered_NoMatch_DoesNotSetFeature() { // Arrange var services = CreateServices(); @@ -107,10 +137,10 @@ namespace Microsoft.AspNetCore.Builder await appFunc(httpContext); // Assert - Assert.NotNull(httpContext.Features.Get()); + Assert.Null(httpContext.Features.Get()); } - private IServiceProvider CreateServices() + private IServiceProvider CreateServices(params Endpoint[] endpoints) { var services = new ServiceCollection(); @@ -118,6 +148,8 @@ namespace Microsoft.AspNetCore.Builder services.AddOptions(); services.AddRouting(); + services.AddSingleton(new DefaultEndpointDataSource(endpoints)); + return services.BuildServiceProvider(); } } diff --git a/test/Microsoft.AspNetCore.Routing.Tests/EndpointMiddlewareTest.cs b/test/Microsoft.AspNetCore.Routing.Tests/EndpointMiddlewareTest.cs index 1cb5427736..6be0ade53b 100644 --- a/test/Microsoft.AspNetCore.Routing.Tests/EndpointMiddlewareTest.cs +++ b/test/Microsoft.AspNetCore.Routing.Tests/EndpointMiddlewareTest.cs @@ -3,11 +3,9 @@ using System; using System.Threading.Tasks; -using Microsoft.AspNetCore.Builder; using Microsoft.AspNetCore.Http; -using Microsoft.Extensions.DependencyInjection; +using Microsoft.AspNetCore.Http.Features; using Microsoft.Extensions.Logging.Abstractions; -using Microsoft.Extensions.Logging.Testing; using Xunit; namespace Microsoft.AspNetCore.Routing @@ -15,7 +13,7 @@ namespace Microsoft.AspNetCore.Routing public class EndpointMiddlewareTest { [Fact] - public async Task Invoke_NoFeature_ThrowFriendlyErrorMessage() + public async Task Invoke_NoFeature_NoOps() { // Arrange var httpContext = new DefaultHttpContext(); @@ -23,22 +21,73 @@ namespace Microsoft.AspNetCore.Routing RequestDelegate next = (c) => { - return Task.FromResult(null); + return Task.CompletedTask; }; var middleware = new EndpointMiddleware(NullLogger.Instance, next); // Act - var invokeTask = middleware.Invoke(httpContext); + await middleware.Invoke(httpContext); + + // Assert - does not throw + } + + [Fact] + public async Task Invoke_NoEndpoint_NoOps() + { + // Arrange + var httpContext = new DefaultHttpContext(); + httpContext.RequestServices = new ServiceProvider(); + + httpContext.Features.Set(new EndpointFeature() + { + Endpoint = null, + }); + + RequestDelegate next = (c) => + { + return Task.CompletedTask; + }; + + var middleware = new EndpointMiddleware(NullLogger.Instance, next); + + // Act + await middleware.Invoke(httpContext); + + // Assert - does not throw + } + + [Fact] + public async Task Invoke_WithEndpoint_InvokesDelegate() + { + // Arrange + var httpContext = new DefaultHttpContext(); + httpContext.RequestServices = new ServiceProvider(); + + var invoked = false; + RequestDelegate endpointFunc = (c) => + { + invoked = true; + return Task.CompletedTask; + }; + + httpContext.Features.Set(new EndpointFeature() + { + Endpoint = new Endpoint(endpointFunc, EndpointMetadataCollection.Empty, "Test"), + }); + + RequestDelegate next = (c) => + { + return Task.CompletedTask; + }; + + var middleware = new EndpointMiddleware(NullLogger.Instance, next); + + // Act + await middleware.Invoke(httpContext); // Assert - var ex = await Assert.ThrowsAsync(async () => await invokeTask); - - Assert.Equal( - "Unable to execute an endpoint because the EndpointRoutingMiddleware was not run for this request. " + - "Ensure EndpointRoutingMiddleware is added to the request execution pipeline before EndpointMiddleware " + - "in application startup code.", - ex.Message); + Assert.True(invoked); } private class ServiceProvider : IServiceProvider