From b649133eec344e733d1724964a3255b972ed5ae5 Mon Sep 17 00:00:00 2001 From: James Newton-King Date: Thu, 30 Aug 2018 08:57:53 +1200 Subject: [PATCH] Refactor KnownRouteValueConstraint to not require HttpContext (#8352) --- build/dependencies.props | 4 +- .../Routing/KnownRouteValueConstraint.cs | 82 ++++++++----- .../Internal/MvcEndpointDataSourceTests.cs | 32 +++-- .../Routing/KnownRouteValueConstraintTests.cs | 109 ++++++++++++++++-- 4 files changed, 172 insertions(+), 55 deletions(-) diff --git a/build/dependencies.props b/build/dependencies.props index b0c3540479..d718620eed 100644 --- a/build/dependencies.props +++ b/build/dependencies.props @@ -48,8 +48,8 @@ 2.2.0-preview2-35090 2.2.0-preview2-35090 2.2.0-preview2-35090 - 2.2.0-preview2-35090 - 2.2.0-preview2-35090 + 2.2.0-a-preview2-routeconstraint-httpcontext-16912 + 2.2.0-a-preview2-routeconstraint-httpcontext-16912 2.2.0-preview2-35090 2.2.0-preview2-35090 2.2.0-preview2-35090 diff --git a/src/Microsoft.AspNetCore.Mvc.Core/Routing/KnownRouteValueConstraint.cs b/src/Microsoft.AspNetCore.Mvc.Core/Routing/KnownRouteValueConstraint.cs index a80b3b510c..6989c22695 100644 --- a/src/Microsoft.AspNetCore.Mvc.Core/Routing/KnownRouteValueConstraint.cs +++ b/src/Microsoft.AspNetCore.Mvc.Core/Routing/KnownRouteValueConstraint.cs @@ -14,8 +14,26 @@ namespace Microsoft.AspNetCore.Mvc.Routing { public class KnownRouteValueConstraint : IRouteConstraint { + private readonly IActionDescriptorCollectionProvider _actionDescriptorCollectionProvider; private RouteValuesCollection _cachedValuesCollection; + [Obsolete("This constructor is obsolete. Use KnownRouteValueConstraint.ctor(IActionDescriptorCollectionProvider) instead.")] + public KnownRouteValueConstraint() + { + // Empty constructor for backwards compatibility + // Services will need to be resolved from HttpContext when this ctor is used + } + + public KnownRouteValueConstraint(IActionDescriptorCollectionProvider actionDescriptorCollectionProvider) + { + if (actionDescriptorCollectionProvider == null) + { + throw new ArgumentNullException(nameof(actionDescriptorCollectionProvider)); + } + + _actionDescriptorCollectionProvider = actionDescriptorCollectionProvider; + } + public bool Match( HttpContext httpContext, IRouter route, @@ -23,16 +41,6 @@ namespace Microsoft.AspNetCore.Mvc.Routing RouteValueDictionary values, RouteDirection routeDirection) { - if (httpContext == null) - { - throw new ArgumentNullException(nameof(httpContext)); - } - - if (route == null) - { - throw new ArgumentNullException(nameof(route)); - } - if (routeKey == null) { throw new ArgumentNullException(nameof(routeKey)); @@ -49,7 +57,9 @@ namespace Microsoft.AspNetCore.Mvc.Routing var value = obj as string; if (value != null) { - var allValues = GetAndCacheAllMatchingValues(routeKey, httpContext); + var actionDescriptors = GetAndValidateActionDescriptors(httpContext); + + var allValues = GetAndCacheAllMatchingValues(routeKey, actionDescriptors); foreach (var existingValue in allValues) { if (string.Equals(value, existingValue, StringComparison.OrdinalIgnoreCase)) @@ -63,9 +73,36 @@ namespace Microsoft.AspNetCore.Mvc.Routing return false; } - private string[] GetAndCacheAllMatchingValues(string routeKey, HttpContext httpContext) + private ActionDescriptorCollection GetAndValidateActionDescriptors(HttpContext httpContext) + { + var actionDescriptorsProvider = _actionDescriptorCollectionProvider; + + if (actionDescriptorsProvider == null) + { + // Only validate that HttpContext was passed to constraint if it is needed + if (httpContext == null) + { + throw new ArgumentNullException(nameof(httpContext)); + } + + var services = httpContext.RequestServices; + actionDescriptorsProvider = services.GetRequiredService(); + } + + var actionDescriptors = actionDescriptorsProvider.ActionDescriptors; + if (actionDescriptors == null) + { + throw new InvalidOperationException( + Resources.FormatPropertyOfTypeCannotBeNull( + nameof(IActionDescriptorCollectionProvider.ActionDescriptors), + actionDescriptorsProvider.GetType())); + } + + return actionDescriptors; + } + + private string[] GetAndCacheAllMatchingValues(string routeKey, ActionDescriptorCollection actionDescriptors) { - var actionDescriptors = GetAndValidateActionDescriptorCollection(httpContext); var version = actionDescriptors.Version; var valuesCollection = _cachedValuesCollection; @@ -77,8 +114,7 @@ namespace Microsoft.AspNetCore.Mvc.Routing { var action = actionDescriptors.Items[i]; - string value; - if (action.RouteValues.TryGetValue(routeKey, out value) && + if (action.RouteValues.TryGetValue(routeKey, out var value) && !string.IsNullOrEmpty(value)) { values.Add(value); @@ -92,22 +128,6 @@ namespace Microsoft.AspNetCore.Mvc.Routing return _cachedValuesCollection.Items; } - private static ActionDescriptorCollection GetAndValidateActionDescriptorCollection(HttpContext httpContext) - { - var services = httpContext.RequestServices; - var provider = services.GetRequiredService(); - var descriptors = provider.ActionDescriptors; - - if (descriptors == null) - { - throw new InvalidOperationException( - Resources.FormatPropertyOfTypeCannotBeNull("ActionDescriptors", - provider.GetType())); - } - - return descriptors; - } - private class RouteValuesCollection { public RouteValuesCollection(int version, string[] items) diff --git a/test/Microsoft.AspNetCore.Mvc.Core.Test/Internal/MvcEndpointDataSourceTests.cs b/test/Microsoft.AspNetCore.Mvc.Core.Test/Internal/MvcEndpointDataSourceTests.cs index be31acba0b..fe5d8dbbf7 100644 --- a/test/Microsoft.AspNetCore.Mvc.Core.Test/Internal/MvcEndpointDataSourceTests.cs +++ b/test/Microsoft.AspNetCore.Mvc.Core.Test/Internal/MvcEndpointDataSourceTests.cs @@ -180,7 +180,15 @@ namespace Microsoft.AspNetCore.Mvc.Internal var actionDescriptorCollection = GetActionDescriptorCollection( new { controller = "TestController", action = "TestAction", area = "TestArea" }); var dataSource = CreateMvcEndpointDataSource(actionDescriptorCollection); - dataSource.ConventionalEndpointInfos.Add(CreateEndpointInfo(string.Empty, endpointInfoRoute)); + + var services = new ServiceCollection(); + services.AddRouting(); + services.AddSingleton(actionDescriptorCollection); + + var routeOptionsSetup = new MvcCoreRouteOptionsSetup(); + services.Configure(routeOptionsSetup.Configure); + + dataSource.ConventionalEndpointInfos.Add(CreateEndpointInfo(string.Empty, endpointInfoRoute, serviceProvider: services.BuildServiceProvider())); // Act var endpoints = dataSource.Endpoints; @@ -719,13 +727,13 @@ namespace Microsoft.AspNetCore.Mvc.Internal Array.Empty()); } - var serviceProviderMock = new Mock(); - serviceProviderMock.Setup(m => m.GetService(typeof(IActionDescriptorCollectionProvider))).Returns(actionDescriptorCollectionProvider); + var services = new ServiceCollection(); + services.AddSingleton(actionDescriptorCollectionProvider); var dataSource = new MvcEndpointDataSource( actionDescriptorCollectionProvider, mvcEndpointInvokerFactory ?? new MvcEndpointInvokerFactory(new ActionInvokerFactory(Array.Empty())), - serviceProviderMock.Object); + services.BuildServiceProvider()); return dataSource; } @@ -735,15 +743,19 @@ namespace Microsoft.AspNetCore.Mvc.Internal string template, RouteValueDictionary defaults = null, IDictionary constraints = null, - RouteValueDictionary dataTokens = null) + RouteValueDictionary dataTokens = null, + IServiceProvider serviceProvider = null) { - var serviceCollection = new ServiceCollection(); - serviceCollection.AddRouting(); + if (serviceProvider == null) + { + var serviceCollection = new ServiceCollection(); + serviceCollection.AddRouting(); - var routeOptionsSetup = new MvcCoreRouteOptionsSetup(); - serviceCollection.Configure(routeOptionsSetup.Configure); + var routeOptionsSetup = new MvcCoreRouteOptionsSetup(); + serviceCollection.Configure(routeOptionsSetup.Configure); - var serviceProvider = serviceCollection.BuildServiceProvider(); + serviceProvider = serviceCollection.BuildServiceProvider(); + } var parameterPolicyFactory = serviceProvider.GetRequiredService(); return new MvcEndpointInfo(name, template, defaults, constraints, dataTokens, parameterPolicyFactory); diff --git a/test/Microsoft.AspNetCore.Mvc.Core.Test/Routing/KnownRouteValueConstraintTests.cs b/test/Microsoft.AspNetCore.Mvc.Core.Test/Routing/KnownRouteValueConstraintTests.cs index 1e1d1f8951..5aecf45a77 100644 --- a/test/Microsoft.AspNetCore.Mvc.Core.Test/Routing/KnownRouteValueConstraintTests.cs +++ b/test/Microsoft.AspNetCore.Mvc.Core.Test/Routing/KnownRouteValueConstraintTests.cs @@ -2,7 +2,6 @@ // Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. using System; -using System.Collections.Generic; using System.Linq; using Microsoft.AspNetCore.Http; using Microsoft.AspNetCore.Mvc.Abstractions; @@ -10,6 +9,7 @@ using Microsoft.AspNetCore.Mvc.Controllers; using Microsoft.AspNetCore.Mvc.Infrastructure; using Microsoft.AspNetCore.Mvc.Internal; using Microsoft.AspNetCore.Routing; +using Microsoft.Extensions.DependencyInjection; using Moq; using Xunit; @@ -17,7 +17,47 @@ namespace Microsoft.AspNetCore.Mvc.Routing { public class KnownRouteValueConstraintTests { +#pragma warning disable CS0618 // Type or member is obsolete private readonly IRouteConstraint _constraint = new KnownRouteValueConstraint(); +#pragma warning restore CS0618 // Type or member is obsolete + + [Fact] + public void ResolveFromServices_InjectsServiceProvider_HttpContextNotNeeded() + { + // Arrange + var actionDescriptor = CreateActionDescriptor("testArea", + "testController", + "testAction"); + actionDescriptor.RouteValues.Add("randomKey", "testRandom"); + var descriptorCollectionProvider = CreateActionDesciprtorCollectionProvider(actionDescriptor); + + var services = new ServiceCollection(); + services.AddRouting(); + services.AddSingleton(descriptorCollectionProvider); + + var routeOptionsSetup = new MvcCoreRouteOptionsSetup(); + services.Configure(routeOptionsSetup.Configure); + + var serviceProvider = services.BuildServiceProvider(); + + var inlineConstraintResolver = serviceProvider.GetRequiredService(); + var constraint = inlineConstraintResolver.ResolveConstraint("exists"); + + var values = new RouteValueDictionary() + { + { "area", "testArea" }, + { "controller", "testController" }, + { "action", "testAction" }, + { "randomKey", "testRandom" } + }; + + // Act + var knownRouteValueConstraint = Assert.IsType(constraint); + var match = knownRouteValueConstraint.Match(httpContext: null, route: null, "area", values, RouteDirection.IncomingRequest); + + // Assert + Assert.True(match); + } [Theory] [InlineData("area", RouteDirection.IncomingRequest)] @@ -55,8 +95,8 @@ namespace Microsoft.AspNetCore.Mvc.Routing { // Arrange var actionDescriptor = CreateActionDescriptor("testArea", - "testController", - "testAction"); + "testController", + "testAction"); actionDescriptor.RouteValues.Add("randomKey", "testRandom"); var httpContext = GetHttpContext(actionDescriptor); var route = Mock.Of(); @@ -115,8 +155,8 @@ namespace Microsoft.AspNetCore.Mvc.Routing public void RouteValue_IsNotAString_MatchFails(RouteDirection direction) { var actionDescriptor = CreateActionDescriptor("testArea", - controller: null, - action: null); + controller: null, + action: null); var httpContext = GetHttpContext(actionDescriptor); var route = Mock.Of(); var values = new RouteValueDictionary() @@ -157,7 +197,57 @@ namespace Microsoft.AspNetCore.Mvc.Routing ex.Message); } - private static HttpContext GetHttpContext(ActionDescriptor actionDescriptor) + [Theory] + [InlineData("area", RouteDirection.IncomingRequest)] + [InlineData("controller", RouteDirection.IncomingRequest)] + [InlineData("action", RouteDirection.IncomingRequest)] + [InlineData("randomKey", RouteDirection.IncomingRequest)] + [InlineData("area", RouteDirection.UrlGeneration)] + [InlineData("controller", RouteDirection.UrlGeneration)] + [InlineData("action", RouteDirection.UrlGeneration)] + [InlineData("randomKey", RouteDirection.UrlGeneration)] + public void ServiceInjected_RouteKey_Exists_MatchSucceeds(string keyName, RouteDirection direction) + { + // Arrange + var actionDescriptor = CreateActionDescriptor("testArea", + "testController", + "testAction"); + actionDescriptor.RouteValues.Add("randomKey", "testRandom"); + + var provider = CreateActionDesciprtorCollectionProvider(actionDescriptor); + + var constraint = new KnownRouteValueConstraint(provider); + + var values = new RouteValueDictionary() + { + { "area", "testArea" }, + { "controller", "testController" }, + { "action", "testAction" }, + { "randomKey", "testRandom" } + }; + + // Act + var match = constraint.Match(httpContext: null, route: null, keyName, values, direction); + + // Assert + Assert.True(match); + } + + private static HttpContext GetHttpContext(ActionDescriptor actionDescriptor, bool setupRequestServices = true) + { + var descriptorCollectionProvider = CreateActionDesciprtorCollectionProvider(actionDescriptor); + + var context = new Mock(); + if (setupRequestServices) + { + context.Setup(o => o.RequestServices + .GetService(typeof(IActionDescriptorCollectionProvider))) + .Returns(descriptorCollectionProvider); + } + return context.Object; + } + + private static IActionDescriptorCollectionProvider CreateActionDesciprtorCollectionProvider(ActionDescriptor actionDescriptor) { var actionProvider = new Mock(MockBehavior.Strict); @@ -176,12 +266,7 @@ namespace Microsoft.AspNetCore.Mvc.Routing var descriptorCollectionProvider = new DefaultActionDescriptorCollectionProvider( new[] { actionProvider.Object }, Enumerable.Empty()); - - var context = new Mock(); - context.Setup(o => o.RequestServices - .GetService(typeof(IActionDescriptorCollectionProvider))) - .Returns(descriptorCollectionProvider); - return context.Object; + return descriptorCollectionProvider; } private static ActionDescriptor CreateActionDescriptor(string area, string controller, string action)