From 5e20c313d9416712d870bcecc4af023f9feae9cc Mon Sep 17 00:00:00 2001 From: kishanAnem Date: Tue, 26 Jun 2018 20:43:46 +0200 Subject: [PATCH 1/5] Array or List in query string does not get parsed #7712 (#7967) - exclude collections when detecting complex types in `ApiBehaviorApplicationModelProvider` - add test cases --- .../ApiBehaviorApplicationModelProvider.cs | 8 +- ...ApiBehaviorApplicationModelProviderTest.cs | 106 +++++++++++++++++- 2 files changed, 109 insertions(+), 5 deletions(-) diff --git a/src/Microsoft.AspNetCore.Mvc.Core/Internal/ApiBehaviorApplicationModelProvider.cs b/src/Microsoft.AspNetCore.Mvc.Core/Internal/ApiBehaviorApplicationModelProvider.cs index 9d86de29e5..1661eefc03 100644 --- a/src/Microsoft.AspNetCore.Mvc.Core/Internal/ApiBehaviorApplicationModelProvider.cs +++ b/src/Microsoft.AspNetCore.Mvc.Core/Internal/ApiBehaviorApplicationModelProvider.cs @@ -193,7 +193,7 @@ namespace Microsoft.AspNetCore.Mvc.Internal var metadata = _modelMetadataProvider.GetMetadataForProperty( controllerModel.ControllerType, property.PropertyInfo.Name); - if (metadata.IsComplexType) + if (metadata.IsComplexType && !metadata.IsCollectionType) { property.BindingInfo.BinderModelName = string.Empty; } @@ -254,9 +254,9 @@ namespace Microsoft.AspNetCore.Mvc.Internal private bool IsComplexTypeParameter(ParameterModel parameter) { // No need for information from attributes on the parameter. Just use its type. - return _modelMetadataProvider - .GetMetadataForType(parameter.ParameterInfo.ParameterType) - .IsComplexType; + var metadata = _modelMetadataProvider + .GetMetadataForType(parameter.ParameterInfo.ParameterType); + return metadata.IsComplexType && !metadata.IsCollectionType; } } } diff --git a/test/Microsoft.AspNetCore.Mvc.Core.Test/Internal/ApiBehaviorApplicationModelProviderTest.cs b/test/Microsoft.AspNetCore.Mvc.Core.Test/Internal/ApiBehaviorApplicationModelProviderTest.cs index a90dbc2943..5b4b210d1a 100644 --- a/test/Microsoft.AspNetCore.Mvc.Core.Test/Internal/ApiBehaviorApplicationModelProviderTest.cs +++ b/test/Microsoft.AspNetCore.Mvc.Core.Test/Internal/ApiBehaviorApplicationModelProviderTest.cs @@ -2,6 +2,7 @@ // 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.ComponentModel; using System.Linq; using System.Reflection; @@ -619,6 +620,75 @@ Environment.NewLine + "int b"; Assert.Equal("gps", bindingInfo.BinderModelName); } + [Fact] + public void PreservesBindingSourceInference_ForFromQueryParameterOnCollectionType() + { + // Arrange + var modelMetadataProvider = TestModelMetadataProvider.CreateDefaultProvider(); + var actionName = nameof(ParameterBindingController.FromQueryOnCollectionType); + var context = GetContext(typeof(ParameterBindingController), modelMetadataProvider); + var provider = GetProvider(); + + // Act + provider.OnProvidersExecuting(context); + + // Assert + var controller = Assert.Single(context.Result.Controllers); + var action = Assert.Single(controller.Actions, a => a.ActionName == actionName); + var parameter = Assert.Single(action.Parameters); + + var bindingInfo = parameter.BindingInfo; + Assert.NotNull(bindingInfo); + Assert.Same(BindingSource.Query, bindingInfo.BindingSource); + Assert.Null(bindingInfo.BinderModelName); + } + + [Fact] + public void PreservesBindingSourceInference_ForFromQueryOnArrayType() + { + // Arrange + var modelMetadataProvider = TestModelMetadataProvider.CreateDefaultProvider(); + var actionName = nameof(ParameterBindingController.FromQueryOnArrayType); + var context = GetContext(typeof(ParameterBindingController), modelMetadataProvider); + var provider = GetProvider(); + + // Act + provider.OnProvidersExecuting(context); + + // Assert + var controller = Assert.Single(context.Result.Controllers); + var action = Assert.Single(controller.Actions, a => a.ActionName == actionName); + var parameter = Assert.Single(action.Parameters); + + var bindingInfo = parameter.BindingInfo; + Assert.NotNull(bindingInfo); + Assert.Same(BindingSource.Query, bindingInfo.BindingSource); + Assert.Null(bindingInfo.BinderModelName); + } + + [Fact] + public void PreservesBindingSourceInference_FromQueryOnArrayTypeWithCustomName() + { + // Arrange + var modelMetadataProvider = TestModelMetadataProvider.CreateDefaultProvider(); + var actionName = nameof(ParameterBindingController.FromQueryOnArrayTypeWithCustomName); + var context = GetContext(typeof(ParameterBindingController), modelMetadataProvider); + var provider = GetProvider(); + + // Act + provider.OnProvidersExecuting(context); + + // Assert + var controller = Assert.Single(context.Result.Controllers); + var action = Assert.Single(controller.Actions, a => a.ActionName == actionName); + var parameter = Assert.Single(action.Parameters); + + var bindingInfo = parameter.BindingInfo; + Assert.NotNull(bindingInfo); + Assert.Same(BindingSource.Query, bindingInfo.BindingSource); + Assert.Equal("ids", bindingInfo.BinderModelName); + } + [Fact] public void PreservesBindingSourceInference_ForFromRouteParameter_WithDefaultName() { @@ -794,6 +864,22 @@ Environment.NewLine + "int b"; Assert.Equal(string.Empty, property.BindingInfo.BinderModelName); } + [Fact] + public void InferBoundPropertyModelPrefixes_SetsModelPrefix_ForCollectionTypeFromValueProvider() + { + // Arrange + var controller = GetControllerModel(typeof(ControllerWithBoundCollectionProperty)); + + var provider = GetProvider(); + + // Act + provider.InferBoundPropertyModelPrefixes(controller); + + // Assert + var property = Assert.Single(controller.ControllerProperties); + Assert.Null(property.BindingInfo.BinderModelName); + } + [Fact] public void InferParameterModelPrefixes_SetsModelPrefix_ForComplexTypeFromValueProvider() { @@ -1008,7 +1094,7 @@ Environment.NewLine + "int b"; [HttpGet("parameter-with-model-binder-attribute")] public IActionResult ModelBinderAttribute([ModelBinder(Name = "top")] int value) => null; - + [HttpGet("parameter-with-fromquery")] public IActionResult FromQuery([FromQuery] int value) => null; @@ -1021,6 +1107,15 @@ Environment.NewLine + "int b"; [HttpGet("parameter-with-fromquery-on-complextype-and-customname")] public IActionResult FromQueryOnComplexTypeWithCustomName([FromQuery(Name = "gps")] GpsCoordinates gpsCoordinates) => null; + [HttpGet("parameter-with-fromquery-on-collection-type")] + public IActionResult FromQueryOnCollectionType([FromQuery] ICollection value) => null; + + [HttpGet("parameter-with-fromquery-on-array-type")] + public IActionResult FromQueryOnArrayType([FromQuery] int[] value) => null; + + [HttpGet("parameter-with-fromquery-on-array-type-customname")] + public IActionResult FromQueryOnArrayTypeWithCustomName([FromQuery(Name = "ids")] int[] value) => null; + [HttpGet("parameter-with-fromroute")] public IActionResult FromRoute([FromRoute] int value) => null; @@ -1118,6 +1213,15 @@ Environment.NewLine + "int b"; public IActionResult SomeAction([FromQuery] TestModel test) => null; } + [ApiController] + private class ControllerWithBoundCollectionProperty + { + [FromQuery] + public List TestProperty { get; set; } + + public IActionResult SomeAction([FromQuery] List test) => null; + } + private class Car { } [ApiController] From cb0627b28aefe94e7f06d297f73d64aa78380177 Mon Sep 17 00:00:00 2001 From: Ryan Nowak Date: Sat, 25 Aug 2018 21:25:25 -0700 Subject: [PATCH 2/5] Addressing a skipped test I think something that was meant to be revisited in a PR got left behing. I found this while fixing aspnet/Routing#772 and it seems worth addressing. This change removes the hardcoding of action/controller/area in the data source, and corrects the behavior of required route values when they aren't in that set. --- .../Internal/MvcEndpointDataSource.cs | 161 +++++++++--------- .../Internal/MvcEndpointDataSourceTests.cs | 53 ++++-- 2 files changed, 127 insertions(+), 87 deletions(-) diff --git a/src/Microsoft.AspNetCore.Mvc.Core/Internal/MvcEndpointDataSource.cs b/src/Microsoft.AspNetCore.Mvc.Core/Internal/MvcEndpointDataSource.cs index 128e699774..2837e6ec4a 100644 --- a/src/Microsoft.AspNetCore.Mvc.Core/Internal/MvcEndpointDataSource.cs +++ b/src/Microsoft.AspNetCore.Mvc.Core/Internal/MvcEndpointDataSource.cs @@ -89,73 +89,88 @@ namespace Microsoft.AspNetCore.Mvc.Internal // - Home/Login foreach (var endpointInfo in ConventionalEndpointInfos) { - if (MatchRouteValue(action, endpointInfo, "Area") - && MatchRouteValue(action, endpointInfo, "Controller") - && MatchRouteValue(action, endpointInfo, "Action")) + // An 'endpointInfo' is applicable if: + // 1. it has a parameter (or default value) for 'required' non-null route value + // 2. it does not have a parameter (or default value) for 'required' null route value + var isApplicable = true; + foreach (var routeKey in action.RouteValues.Keys) { - var newPathSegments = endpointInfo.ParsedPattern.PathSegments.ToList(); - - for (var i = 0; i < newPathSegments.Count; i++) + if (!MatchRouteValue(action, endpointInfo, routeKey)) { - // Check if the pattern can be shortened because the remaining parameters are optional - // - // e.g. Matching pattern {controller=Home}/{action=Index}/{id?} against HomeController.Index - // can resolve to the following endpoints: - // - /Home/Index/{id?} - // - /Home - // - / - if (UseDefaultValuePlusRemainingSegementsOptional(i, action, endpointInfo, newPathSegments)) + isApplicable = false; + break; + } + } + + if (!isApplicable) + { + continue; + } + + var newPathSegments = endpointInfo.ParsedPattern.PathSegments.ToList(); + + for (var i = 0; i < newPathSegments.Count; i++) + { + // Check if the pattern can be shortened because the remaining parameters are optional + // + // e.g. Matching pattern {controller=Home}/{action=Index}/{id?} against HomeController.Index + // can resolve to the following endpoints: + // - /Home/Index/{id?} + // - /Home + // - / + if (UseDefaultValuePlusRemainingSegementsOptional(i, action, endpointInfo, newPathSegments)) + { + var subPathSegments = newPathSegments.Take(i); + + var subEndpoint = CreateEndpoint( + action, + endpointInfo.Name, + GetPattern(ref patternStringBuilder, subPathSegments), + subPathSegments, + endpointInfo.Defaults, + ++conventionalRouteOrder, + endpointInfo, + suppressLinkGeneration: false); + endpoints.Add(subEndpoint); + } + + List segmentParts = null; // Initialize only as needed + var segment = newPathSegments[i]; + for (var j = 0; j < segment.Parts.Count; j++) + { + var part = segment.Parts[j]; + + if (part.IsParameter && + part is RoutePatternParameterPart parameterPart && + action.RouteValues.ContainsKey(parameterPart.Name)) { - var subPathSegments = newPathSegments.Take(i); - - var subEndpoint = CreateEndpoint( - action, - endpointInfo.Name, - GetPattern(ref patternStringBuilder, subPathSegments), - subPathSegments, - endpointInfo.Defaults, - ++conventionalRouteOrder, - endpointInfo, - suppressLinkGeneration: false); - endpoints.Add(subEndpoint); - } - - List segmentParts = null; // Initialize only as needed - var segment = newPathSegments[i]; - for (var j = 0; j < segment.Parts.Count; j++) - { - var part = segment.Parts[j]; - - if (part.IsParameter && part is RoutePatternParameterPart parameterPart && IsMvcParameter(parameterPart.Name)) + if (segmentParts == null) { - if (segmentParts == null) - { - segmentParts = segment.Parts.ToList(); - } - - // Replace parameter with literal value - segmentParts[j] = RoutePatternFactory.LiteralPart(action.RouteValues[parameterPart.Name]); + segmentParts = segment.Parts.ToList(); } - } - // A parameter part was replaced so replace segment with updated parts - if (segmentParts != null) - { - newPathSegments[i] = RoutePatternFactory.Segment(segmentParts); + // Replace parameter with literal value + segmentParts[j] = RoutePatternFactory.LiteralPart(action.RouteValues[parameterPart.Name]); } } - var endpoint = CreateEndpoint( - action, - endpointInfo.Name, - GetPattern(ref patternStringBuilder, newPathSegments), - newPathSegments, - endpointInfo.Defaults, - ++conventionalRouteOrder, - endpointInfo, - suppressLinkGeneration: false); - endpoints.Add(endpoint); + // A parameter part was replaced so replace segment with updated parts + if (segmentParts != null) + { + newPathSegments[i] = RoutePatternFactory.Segment(segmentParts); + } } + + var endpoint = CreateEndpoint( + action, + endpointInfo.Name, + GetPattern(ref patternStringBuilder, newPathSegments), + newPathSegments, + endpointInfo.Defaults, + ++conventionalRouteOrder, + endpointInfo, + suppressLinkGeneration: false); + endpoints.Add(endpoint); } } else @@ -190,18 +205,6 @@ namespace Microsoft.AspNetCore.Mvc.Internal } } - private bool IsMvcParameter(string name) - { - if (string.Equals(name, "Area", StringComparison.OrdinalIgnoreCase) - || string.Equals(name, "Controller", StringComparison.OrdinalIgnoreCase) - || string.Equals(name, "Action", StringComparison.OrdinalIgnoreCase)) - { - return true; - } - - return false; - } - private bool UseDefaultValuePlusRemainingSegementsOptional( int segmentIndex, ActionDescriptor action, @@ -225,7 +228,7 @@ namespace Microsoft.AspNetCore.Mvc.Internal continue; } - if (IsMvcParameter(parameterPart.Name)) + if (action.RouteValues.ContainsKey(parameterPart.Name)) { if (endpointInfo.MergedDefaults[parameterPart.Name] is string defaultValue && action.RouteValues.TryGetValue(parameterPart.Name, out var routeValue) @@ -266,8 +269,10 @@ namespace Microsoft.AspNetCore.Mvc.Internal { // Action does not have a value for this routeKey, most likely because action is not in an area // Check that the pattern does not have a parameter for the routeKey - var matchingParameter = endpointInfo.ParsedPattern.Parameters.SingleOrDefault(p => string.Equals(p.Name, routeKey, StringComparison.OrdinalIgnoreCase)); - if (matchingParameter == null) + var matchingParameter = endpointInfo.ParsedPattern.GetParameter(routeKey); + if (matchingParameter == null && + (!endpointInfo.ParsedPattern.Defaults.TryGetValue(routeKey, out var value) || + !string.IsNullOrEmpty(Convert.ToString(value)))) { return true; } @@ -279,7 +284,7 @@ namespace Microsoft.AspNetCore.Mvc.Internal return true; } - var matchingParameter = endpointInfo.ParsedPattern.Parameters.SingleOrDefault(p => string.Equals(p.Name, routeKey, StringComparison.OrdinalIgnoreCase)); + var matchingParameter = endpointInfo.ParsedPattern.GetParameter(routeKey); if (matchingParameter != null) { // Check that the value matches against constraints on that parameter @@ -358,10 +363,12 @@ namespace Microsoft.AspNetCore.Mvc.Internal object source, bool suppressLinkGeneration) { - var metadata = new List(); - // REVIEW: Used for debugging. Consider removing before release - metadata.Add(source); - metadata.Add(action); + var metadata = new List + { + // REVIEW: Used for debugging. Consider removing before release + source, + action + }; if (action.EndpointMetadata != null) { diff --git a/test/Microsoft.AspNetCore.Mvc.Core.Test/Internal/MvcEndpointDataSourceTests.cs b/test/Microsoft.AspNetCore.Mvc.Core.Test/Internal/MvcEndpointDataSourceTests.cs index fbee5e6f55..e4230b1d56 100644 --- a/test/Microsoft.AspNetCore.Mvc.Core.Test/Internal/MvcEndpointDataSourceTests.cs +++ b/test/Microsoft.AspNetCore.Mvc.Core.Test/Internal/MvcEndpointDataSourceTests.cs @@ -511,29 +511,40 @@ namespace Microsoft.AspNetCore.Mvc.Internal Assert.Empty(endpoints); } - // Since area, controller, action and page are special, check to see if the followin test succeeds for a - // custom required value too. - [Fact(Skip = "Needs review")] + // area, controller, action and page are special, but not hardcoded. Actions can define custom required + // route values. This has been used successfully for localization, versioning and similar schemes. We should + // be able to replace custom route values too. + [Fact] public void NonReservedRequiredValue_WithNoCorresponding_TemplateParameter_DoesNotProduceEndpoint() { // Arrange - var requiredValues = new RouteValueDictionary(new { controller = "home", action = "index", foo = "bar" }); - var actionDescriptorCollection = GetActionDescriptorCollection(requiredValues); + var action1 = new RouteValueDictionary(new { controller = "home", action = "index", locale = "en-NZ" }); + var action2 = new RouteValueDictionary(new { controller = "home", action = "about", locale = "en-CA" }); + var action3 = new RouteValueDictionary(new { controller = "home", action = "index", locale = (string)null }); + + var actionDescriptorCollection = GetActionDescriptorCollection(action1, action2, action3); var dataSource = CreateMvcEndpointDataSource(actionDescriptorCollection); + + // Adding a localized route a non-localized route + dataSource.ConventionalEndpointInfos.Add(CreateEndpointInfo(string.Empty, "{locale}/{controller}/{action}")); dataSource.ConventionalEndpointInfos.Add(CreateEndpointInfo(string.Empty, "{controller}/{action}")); // Act var endpoints = dataSource.Endpoints; // Assert - Assert.Empty(endpoints); + Assert.Collection( + endpoints.Cast().OrderBy(e => e.RoutePattern.RawText), + e => Assert.Equal("en-CA/home/about", e.RoutePattern.RawText), + e => Assert.Equal("en-NZ/home/index", e.RoutePattern.RawText), + e => Assert.Equal("home/index", e.RoutePattern.RawText)); } [Fact] public void TemplateParameter_WithNoDefaultOrRequiredValue_DoesNotProduceEndpoint() { // Arrange - var requiredValues = new RouteValueDictionary(new { controller = "home", action = "index" }); + var requiredValues = new RouteValueDictionary(new { controller = "home", action = "index", area = (string)null }); var actionDescriptorCollection = GetActionDescriptorCollection(requiredValues); var dataSource = CreateMvcEndpointDataSource(actionDescriptorCollection); dataSource.ConventionalEndpointInfos.Add(CreateEndpointInfo(string.Empty, "{area}/{controller}/{action}")); @@ -606,7 +617,7 @@ namespace Microsoft.AspNetCore.Mvc.Internal var actionDescriptorCollection = GetActionDescriptorCollection(requiredValues: requiredValues); var dataSource = CreateMvcEndpointDataSource(actionDescriptorCollection); dataSource.ConventionalEndpointInfos.Add( - CreateEndpointInfo(string.Empty, "{controller=Home}/{action=Index}")); + CreateEndpointInfo(string.Empty, "{subarea}/{controller=Home}/{action=Index}")); // Act var endpoints = dataSource.Endpoints; @@ -614,10 +625,29 @@ namespace Microsoft.AspNetCore.Mvc.Internal // Assert var endpoint = Assert.Single(endpoints); var matcherEndpoint = Assert.IsType(endpoint); - Assert.Equal("Foo/Bar", matcherEndpoint.RoutePattern.RawText); + Assert.Equal("test/Foo/Bar", matcherEndpoint.RoutePattern.RawText); AssertIsSubset(expectedDefaults, matcherEndpoint.RoutePattern.Defaults); } + [Fact] + public void RequiredValues_NotPresent_InDefaultValuesOrParameter_EndpointNotCreated() + { + // Arrange + var requiredValues = new RouteValueDictionary( + new { controller = "Foo", action = "Bar", subarea = "test" }); + var expectedDefaults = requiredValues; + var actionDescriptorCollection = GetActionDescriptorCollection(requiredValues: requiredValues); + var dataSource = CreateMvcEndpointDataSource(actionDescriptorCollection); + dataSource.ConventionalEndpointInfos.Add( + CreateEndpointInfo(string.Empty, "{controller=Home}/{action=Index}")); + + // Act + var endpoints = dataSource.Endpoints; + + // Assert + Assert.Empty(endpoints); + } + [Fact] public void RequiredValues_IsSubsetOf_DefaultValues() { @@ -629,7 +659,10 @@ namespace Microsoft.AspNetCore.Mvc.Internal var actionDescriptorCollection = GetActionDescriptorCollection(requiredValues: requiredValues); var dataSource = CreateMvcEndpointDataSource(actionDescriptorCollection); dataSource.ConventionalEndpointInfos.Add( - CreateEndpointInfo(string.Empty, "{controller=Home}/{action=Index}/{subscription=general}")); + CreateEndpointInfo( + string.Empty, + "{controller=Home}/{action=Index}/{subscription=general}", + defaults: new RouteValueDictionary(new { subarea = "test", }))); // Act var endpoints = dataSource.Endpoints; From a634f6b116ceeef5c36d928ec4e15ac5d01cd75b Mon Sep 17 00:00:00 2001 From: Ryan Nowak Date: Sat, 25 Aug 2018 21:43:06 -0700 Subject: [PATCH 3/5] add another test --- .../Internal/MvcEndpointDataSourceTests.cs | 56 ++++++++++++++++++- 1 file changed, 55 insertions(+), 1 deletion(-) diff --git a/test/Microsoft.AspNetCore.Mvc.Core.Test/Internal/MvcEndpointDataSourceTests.cs b/test/Microsoft.AspNetCore.Mvc.Core.Test/Internal/MvcEndpointDataSourceTests.cs index e4230b1d56..416e359291 100644 --- a/test/Microsoft.AspNetCore.Mvc.Core.Test/Internal/MvcEndpointDataSourceTests.cs +++ b/test/Microsoft.AspNetCore.Mvc.Core.Test/Internal/MvcEndpointDataSourceTests.cs @@ -656,7 +656,7 @@ namespace Microsoft.AspNetCore.Mvc.Internal new { controller = "Foo", action = "Bar", subarea = "test" }); var expectedDefaults = new RouteValueDictionary( new { controller = "Foo", action = "Bar", subarea = "test", subscription = "general" }); - var actionDescriptorCollection = GetActionDescriptorCollection(requiredValues: requiredValues); + var actionDescriptorCollection = GetActionDescriptorCollection(requiredValues); var dataSource = CreateMvcEndpointDataSource(actionDescriptorCollection); dataSource.ConventionalEndpointInfos.Add( CreateEndpointInfo( @@ -674,6 +674,60 @@ namespace Microsoft.AspNetCore.Mvc.Internal AssertIsSubset(expectedDefaults, matcherEndpoint.RoutePattern.Defaults); } + [Fact] + public void RequiredValues_DoesNotMatchParameterDefaults_Included() + { + // Arrange + var action = new RouteValueDictionary( + new { controller = "Foo", action = "Baz", }); // Doesn't match default + var expectedDefaults = new RouteValueDictionary( + new { controller = "Foo", action = "Baz", }); + var actionDescriptorCollection = GetActionDescriptorCollection(action); + var dataSource = CreateMvcEndpointDataSource(actionDescriptorCollection); + dataSource.ConventionalEndpointInfos.Add( + CreateEndpointInfo( + string.Empty, + "{controller}/{action}/{id?}", + defaults: new RouteValueDictionary(new { controller = "Foo", action = "Bar" }))); + + // Act + var endpoints = dataSource.Endpoints; + + // Assert + var endpoint = Assert.Single(endpoints); + var matcherEndpoint = Assert.IsType(endpoint); + Assert.Equal("Foo/Baz/{id?}", matcherEndpoint.RoutePattern.RawText); + AssertIsSubset(expectedDefaults, matcherEndpoint.RoutePattern.Defaults); + } + + [Fact] + public void RequiredValues_DoesNotMatchNonParameterDefaults_FilteredOut() + { + // Arrange + var action1 = new RouteValueDictionary( + new { controller = "Foo", action = "Bar", }); + var action2 = new RouteValueDictionary( + new { controller = "Foo", action = "Baz", }); // Doesn't match default + var expectedDefaults = new RouteValueDictionary( + new { controller = "Foo", action = "Bar", }); + var actionDescriptorCollection = GetActionDescriptorCollection(action1, action2); + var dataSource = CreateMvcEndpointDataSource(actionDescriptorCollection); + dataSource.ConventionalEndpointInfos.Add( + CreateEndpointInfo( + string.Empty, + "Blog/{*slug}", + defaults: new RouteValueDictionary(new { controller = "Foo", action = "Bar" }))); + + // Act + var endpoints = dataSource.Endpoints; + + // Assert + var endpoint = Assert.Single(endpoints); + var matcherEndpoint = Assert.IsType(endpoint); + Assert.Equal("Blog/{*slug}", matcherEndpoint.RoutePattern.RawText); + AssertIsSubset(expectedDefaults, matcherEndpoint.RoutePattern.Defaults); + } + [Fact] public void RequiredValues_HavingNull_AndNotPresentInDefaultValues_IsAddedToDefaultValues() { From 8ed9d0aac223e2566af4334b6293624def5e7cd5 Mon Sep 17 00:00:00 2001 From: James Newton-King Date: Tue, 28 Aug 2018 11:00:11 +1200 Subject: [PATCH 4/5] Use Endpoint instead of RouteEndpoint where possible (#8331) --- build/dependencies.props | 146 +++++++++--------- .../Routing/ConsumesMatcherPolicy.cs | 4 +- .../ActionConstraintMatcherPolicyTest.cs | 6 +- 3 files changed, 76 insertions(+), 80 deletions(-) diff --git a/build/dependencies.props b/build/dependencies.props index d6804c2325..b0c3540479 100644 --- a/build/dependencies.props +++ b/build/dependencies.props @@ -7,7 +7,7 @@ is not otherwise referenced. They avoid unnecessary changes to the Universe build graph or to product dependencies. Do not use these properties elsewhere. --> - + 0.9.9 0.10.13 2.1.1 @@ -16,87 +16,87 @@ 0.43.0 2.1.1.1 2.1.1 - 2.2.0-preview1-34967 + 2.2.0-preview2-35090 2.2.0-preview1-20180807.2 - 2.2.0-preview1-34967 - 2.2.0-preview1-34967 - 2.2.0-preview1-34967 - 2.2.0-preview1-34967 - 2.2.0-preview1-34967 - 2.2.0-preview1-34967 - 2.2.0-preview1-34967 - 2.2.0-preview1-34967 - 2.2.0-preview1-34967 - 2.2.0-preview1-34967 - 2.2.0-preview1-34967 - 2.2.0-preview1-34967 - 2.2.0-preview1-34967 - 2.2.0-preview1-34967 - 2.2.0-preview1-34967 - 2.2.0-preview1-34967 - 2.2.0-preview1-34967 - 2.2.0-preview1-34967 - 2.2.0-preview1-34967 - 2.2.0-preview1-34967 - 2.2.0-preview1-34967 - 2.2.0-preview1-34967 - 2.2.0-preview1-34967 - 2.2.0-preview1-34967 - 2.2.0-preview1-34967 - 2.2.0-preview1-34967 - 2.2.0-preview1-34967 - 2.2.0-preview1-34967 - 2.2.0-preview1-34967 - 2.2.0-preview1-34967 - 2.2.0-a-preview2-routepattern-defaults-16901 - 2.2.0-a-preview2-routepattern-defaults-16901 - 2.2.0-preview1-34967 - 2.2.0-preview1-34967 - 2.2.0-preview1-34967 - 2.2.0-preview1-34967 - 2.2.0-preview1-34967 - 2.2.0-preview1-34967 - 2.2.0-preview1-34967 + 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-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-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-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-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-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-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-preview2-35090 + 2.2.0-preview2-35090 + 2.2.0-preview2-35090 + 2.2.0-preview2-35090 5.2.6 2.8.0 2.8.0 - 2.2.0-preview1-34967 + 2.2.0-preview2-35090 1.7.0 - 2.2.0-preview1-34967 - 2.2.0-preview1-34967 - 2.2.0-preview1-34967 - 2.2.0-preview1-34967 - 2.2.0-preview1-34967 - 2.2.0-preview1-34967 - 2.2.0-preview1-34967 + 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-preview2-35090 + 2.2.0-preview2-35090 2.1.0 - 2.2.0-preview1-34967 - 2.2.0-preview1-34967 - 2.2.0-preview1-34967 - 2.2.0-preview1-34967 - 2.2.0-preview1-34967 - 2.2.0-preview1-34967 - 2.2.0-preview1-34967 - 2.2.0-preview1-34967 - 2.2.0-preview1-34967 - 2.2.0-preview1-34967 - 2.2.0-preview1-34967 - 2.2.0-preview1-34967 - 2.2.0-preview1-34967 - 2.2.0-preview1-34967 - 2.2.0-preview1-34967 - 2.2.0-preview1-34967 - 2.2.0-preview1-34967 - 2.2.0-preview1-34967 - 2.2.0-preview1-34967 - 2.2.0-preview1-34967 - 2.2.0-preview1-34967 - 2.2.0-preview1-34967 + 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-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-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-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-preview2-35090 + 2.2.0-preview2-35090 2.0.9 2.1.2 2.2.0-preview1-26618-02 - 2.2.0-preview1-34967 - 2.2.0-preview1-34967 + 2.2.0-preview2-35090 + 2.2.0-preview2-35090 15.6.1 4.7.49 2.0.3 diff --git a/src/Microsoft.AspNetCore.Mvc.Core/Routing/ConsumesMatcherPolicy.cs b/src/Microsoft.AspNetCore.Mvc.Core/Routing/ConsumesMatcherPolicy.cs index 507d3bcf68..65dd1d117b 100644 --- a/src/Microsoft.AspNetCore.Mvc.Core/Routing/ConsumesMatcherPolicy.cs +++ b/src/Microsoft.AspNetCore.Mvc.Core/Routing/ConsumesMatcherPolicy.cs @@ -135,14 +135,12 @@ namespace Microsoft.AspNetCore.Mvc.Routing private Endpoint CreateRejectionEndpoint() { - return new RouteEndpoint( + return new Endpoint( (context) => { context.Response.StatusCode = StatusCodes.Status415UnsupportedMediaType; return Task.CompletedTask; }, - RoutePatternFactory.Parse("/"), - 0, EndpointMetadataCollection.Empty, Http415EndpointDisplayName); } diff --git a/test/Microsoft.AspNetCore.Mvc.Core.Test/Routing/ActionConstraintMatcherPolicyTest.cs b/test/Microsoft.AspNetCore.Mvc.Core.Test/Routing/ActionConstraintMatcherPolicyTest.cs index e2f02e7f63..298786c121 100644 --- a/test/Microsoft.AspNetCore.Mvc.Core.Test/Routing/ActionConstraintMatcherPolicyTest.cs +++ b/test/Microsoft.AspNetCore.Mvc.Core.Test/Routing/ActionConstraintMatcherPolicyTest.cs @@ -370,13 +370,11 @@ namespace Microsoft.AspNetCore.Mvc.Routing return httpContext; } - private static RouteEndpoint CreateEndpoint(ActionDescriptor action) + private static Endpoint CreateEndpoint(ActionDescriptor action) { var metadata = new List() { action, }; - return new RouteEndpoint( + return new Endpoint( (context) => Task.CompletedTask, - RoutePatternFactory.Parse("/"), - 0, new EndpointMetadataCollection(metadata), $"test: {action?.DisplayName}"); } From 0fcf2448c3d8f92a018927dfdaec42fc869c73db Mon Sep 17 00:00:00 2001 From: Ryan Nowak Date: Sat, 25 Aug 2018 19:42:32 -0700 Subject: [PATCH 5/5] Fix aspnet/Routing#722 Exposes a separate change token that will be triggered after action descriptors have been updated. --- .../MvcEndpointDatasourceBenchmark.cs | 1 - .../MvcCoreServiceCollectionExtensions.cs | 2 +- .../ActionDescriptorCollectionProvider.cs | 33 ++ ...faultActionDescriptorCollectionProvider.cs | 160 +++++++++ .../IActionDescriptorChangeProvider.cs | 11 + .../IActionDescriptorCollectionProvider.cs | 12 +- .../ActionDescriptorCollectionProvider.cs | 95 ----- .../Internal/MvcEndpointDataSource.cs | 338 +++++++++--------- ...ActionDescriptorCollectionProviderTest.cs} | 88 ++--- .../Internal/ActionConstraintCacheTest.cs | 2 +- .../Internal/ActionSelectorTest.cs | 4 +- .../Internal/MvcEndpointDataSourceTests.cs | 75 +--- .../ActionConstraintMatcherPolicyTest.cs | 4 +- .../Routing/KnownRouteValueConstraintTests.cs | 2 +- .../ApiExplorerTest.cs | 2 +- 15 files changed, 448 insertions(+), 381 deletions(-) create mode 100644 src/Microsoft.AspNetCore.Mvc.Core/Infrastructure/ActionDescriptorCollectionProvider.cs create mode 100644 src/Microsoft.AspNetCore.Mvc.Core/Infrastructure/DefaultActionDescriptorCollectionProvider.cs delete mode 100644 src/Microsoft.AspNetCore.Mvc.Core/Internal/ActionDescriptorCollectionProvider.cs rename test/Microsoft.AspNetCore.Mvc.Core.Test/{Internal/ActionDescriptorCollectionProviderTest.cs => Infrastructure/DefaultActionDescriptorCollectionProviderTest.cs} (70%) diff --git a/benchmarks/Microsoft.AspNetCore.Mvc.Performance/MvcEndpointDatasourceBenchmark.cs b/benchmarks/Microsoft.AspNetCore.Mvc.Performance/MvcEndpointDatasourceBenchmark.cs index 85bf14c1cf..893e8c8df8 100644 --- a/benchmarks/Microsoft.AspNetCore.Mvc.Performance/MvcEndpointDatasourceBenchmark.cs +++ b/benchmarks/Microsoft.AspNetCore.Mvc.Performance/MvcEndpointDatasourceBenchmark.cs @@ -111,7 +111,6 @@ namespace Microsoft.AspNetCore.Mvc.Performance var dataSource = new MvcEndpointDataSource( actionDescriptorCollectionProvider, new MvcEndpointInvokerFactory(new ActionInvokerFactory(Array.Empty())), - Array.Empty(), new MockServiceProvider()); return dataSource; diff --git a/src/Microsoft.AspNetCore.Mvc.Core/DependencyInjection/MvcCoreServiceCollectionExtensions.cs b/src/Microsoft.AspNetCore.Mvc.Core/DependencyInjection/MvcCoreServiceCollectionExtensions.cs index 33d7e6aa93..767aa0c33f 100644 --- a/src/Microsoft.AspNetCore.Mvc.Core/DependencyInjection/MvcCoreServiceCollectionExtensions.cs +++ b/src/Microsoft.AspNetCore.Mvc.Core/DependencyInjection/MvcCoreServiceCollectionExtensions.cs @@ -165,7 +165,7 @@ namespace Microsoft.Extensions.DependencyInjection services.TryAddEnumerable( ServiceDescriptor.Transient()); - services.TryAddSingleton(); + services.TryAddSingleton(); // // Action Selection diff --git a/src/Microsoft.AspNetCore.Mvc.Core/Infrastructure/ActionDescriptorCollectionProvider.cs b/src/Microsoft.AspNetCore.Mvc.Core/Infrastructure/ActionDescriptorCollectionProvider.cs new file mode 100644 index 0000000000..633890e08f --- /dev/null +++ b/src/Microsoft.AspNetCore.Mvc.Core/Infrastructure/ActionDescriptorCollectionProvider.cs @@ -0,0 +1,33 @@ +// Copyright (c) .NET Foundation. All rights reserved. +// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. + +using Microsoft.AspNetCore.Mvc.Abstractions; +using Microsoft.Extensions.Primitives; + +namespace Microsoft.AspNetCore.Mvc.Infrastructure +{ + /// + /// A base class for which also provides an + /// for reactive notifications of changes. + /// + /// + /// is used as a base class by the default implementation of + /// . To retrieve an instance of , + /// obtain the from the dependency injection provider and + /// downcast to . + /// + public abstract class ActionDescriptorCollectionProvider : IActionDescriptorCollectionProvider + { + /// + /// Returns the current cached + /// + public abstract ActionDescriptorCollection ActionDescriptors { get; } + + /// + /// Gets an that will be signaled after the + /// collection has changed. + /// + /// The . + public abstract IChangeToken GetChangeToken(); + } +} diff --git a/src/Microsoft.AspNetCore.Mvc.Core/Infrastructure/DefaultActionDescriptorCollectionProvider.cs b/src/Microsoft.AspNetCore.Mvc.Core/Infrastructure/DefaultActionDescriptorCollectionProvider.cs new file mode 100644 index 0000000000..6204ce3945 --- /dev/null +++ b/src/Microsoft.AspNetCore.Mvc.Core/Infrastructure/DefaultActionDescriptorCollectionProvider.cs @@ -0,0 +1,160 @@ +// Copyright (c) .NET Foundation. All rights reserved. +// 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.Collections.ObjectModel; +using System.Diagnostics; +using System.Linq; +using System.Threading; +using Microsoft.AspNetCore.Mvc.Abstractions; +using Microsoft.Extensions.Primitives; + +namespace Microsoft.AspNetCore.Mvc.Infrastructure +{ + internal class DefaultActionDescriptorCollectionProvider : ActionDescriptorCollectionProvider + { + private readonly IActionDescriptorProvider[] _actionDescriptorProviders; + private readonly IActionDescriptorChangeProvider[] _actionDescriptorChangeProviders; + + // The lock is used to protect WRITES to the following (do not need to protect reads once initialized). + private readonly object _lock; + private ActionDescriptorCollection _collection; + private IChangeToken _changeToken; + private CancellationTokenSource _cancellationTokenSource; + private int _version = 0; + + public DefaultActionDescriptorCollectionProvider( + IEnumerable actionDescriptorProviders, + IEnumerable actionDescriptorChangeProviders) + { + _actionDescriptorProviders = actionDescriptorProviders + .OrderBy(p => p.Order) + .ToArray(); + + _actionDescriptorChangeProviders = actionDescriptorChangeProviders.ToArray(); + + ChangeToken.OnChange( + GetCompositeChangeToken, + UpdateCollection); + + _lock = new object(); + } + + /// + /// Returns a cached collection of . + /// + public override ActionDescriptorCollection ActionDescriptors + { + get + { + Initialize(); + Debug.Assert(_collection != null); + Debug.Assert(_changeToken != null); + + return _collection; + } + } + + /// + /// Gets an that will be signaled after the + /// collection has changed. + /// + /// The . + public override IChangeToken GetChangeToken() + { + Initialize(); + Debug.Assert(_collection != null); + Debug.Assert(_changeToken != null); + + return _changeToken; + } + + private IChangeToken GetCompositeChangeToken() + { + if (_actionDescriptorChangeProviders.Length == 1) + { + return _actionDescriptorChangeProviders[0].GetChangeToken(); + } + + var changeTokens = new IChangeToken[_actionDescriptorChangeProviders.Length]; + for (var i = 0; i < _actionDescriptorChangeProviders.Length; i++) + { + changeTokens[i] = _actionDescriptorChangeProviders[i].GetChangeToken(); + } + + return new CompositeChangeToken(changeTokens); + } + + private void Initialize() + { + // Using double-checked locking on initialization because we fire change token callbacks + // when the collection changes. We don't want to do that repeatedly for redundant changes. + // + // The main call path of this code on the first call is async initialization from Endpoint Routing + // which is done in a non-blocking way so in practice no caller will ever block here. + if (_collection == null) + { + lock (_lock) + { + if (_collection == null) + { + UpdateCollection(); + } + } + } + } + + private void UpdateCollection() + { + // Using the lock to initialize writes means that we serialize changes. This eliminates + // the potential for changes to be processed out of order - the risk is that newer data + // could be overwritten by older data. + lock (_lock) + { + var context = new ActionDescriptorProviderContext(); + + for (var i = 0; i < _actionDescriptorProviders.Length; i++) + { + _actionDescriptorProviders[i].OnProvidersExecuting(context); + } + + for (var i = _actionDescriptorProviders.Length - 1; i >= 0; i--) + { + _actionDescriptorProviders[i].OnProvidersExecuted(context); + } + + // The sequence for an update is important because we don't want anyone to obtain + // the new change token but the old action descriptor collection. + // 1. Obtain the old cancellation token source (don't trigger it yet) + // 2. Set the new action descriptor collection + // 3. Set the new change token + // 4. Trigger the old cancellation token source + // + // Consumers who poll will observe a new action descriptor collection at step 2 - they will see + // the new collection and ignore the change token. + // + // Consumers who listen to the change token will requery at step 4 - they will see the new collection + // and new change token. + // + // Anyone who acquires the collection and change token between steps 2 and 3 will be notified of + // a no-op change at step 4. + + // Step 1. + var oldCancellationTokenSource = _cancellationTokenSource; + + // Step 2. + _collection = new ActionDescriptorCollection( + new ReadOnlyCollection(context.Results), + _version++); + + // Step 3. + _cancellationTokenSource = new CancellationTokenSource(); + _changeToken = new CancellationChangeToken(_cancellationTokenSource.Token); + + // Step 4 - might be null if it's the first time. + oldCancellationTokenSource?.Cancel(); + } + } + } +} \ No newline at end of file diff --git a/src/Microsoft.AspNetCore.Mvc.Core/Infrastructure/IActionDescriptorChangeProvider.cs b/src/Microsoft.AspNetCore.Mvc.Core/Infrastructure/IActionDescriptorChangeProvider.cs index d736512f1c..7b524dd10c 100644 --- a/src/Microsoft.AspNetCore.Mvc.Core/Infrastructure/IActionDescriptorChangeProvider.cs +++ b/src/Microsoft.AspNetCore.Mvc.Core/Infrastructure/IActionDescriptorChangeProvider.cs @@ -1,6 +1,7 @@ // Copyright (c) .NET Foundation. All rights reserved. // Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. +using Microsoft.AspNetCore.Mvc.Abstractions; using Microsoft.Extensions.Primitives; namespace Microsoft.AspNetCore.Mvc.Infrastructure @@ -9,6 +10,11 @@ namespace Microsoft.AspNetCore.Mvc.Infrastructure /// Provides a way to signal invalidation of the cached collection of from an /// . /// + /// + /// The change token returned from is only for use inside the MVC infrastructure. + /// Use to be notified of + /// changes. + /// public interface IActionDescriptorChangeProvider { /// @@ -16,6 +22,11 @@ namespace Microsoft.AspNetCore.Mvc.Infrastructure /// instances. /// /// The . + /// + /// The change token returned from is only for use inside the MVC infrastructure. + /// Use to be notified of + /// changes. + /// IChangeToken GetChangeToken(); } } \ No newline at end of file diff --git a/src/Microsoft.AspNetCore.Mvc.Core/Infrastructure/IActionDescriptorCollectionProvider.cs b/src/Microsoft.AspNetCore.Mvc.Core/Infrastructure/IActionDescriptorCollectionProvider.cs index 3e64a66677..ee1648e170 100644 --- a/src/Microsoft.AspNetCore.Mvc.Core/Infrastructure/IActionDescriptorCollectionProvider.cs +++ b/src/Microsoft.AspNetCore.Mvc.Core/Infrastructure/IActionDescriptorCollectionProvider.cs @@ -1,18 +1,28 @@ // Copyright (c) .NET Foundation. All rights reserved. // Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. +using Microsoft.Extensions.Primitives; + namespace Microsoft.AspNetCore.Mvc.Infrastructure { /// /// Provides the currently cached collection of . /// /// + /// /// The default implementation internally caches the collection and uses /// to invalidate this cache, incrementing /// the collection is reconstructed. - /// + /// + /// + /// To be reactively notified of changes, downcast to and + /// subcribe to the change token returned from + /// using . + /// + /// /// Default consumers of this service, are aware of the version and will recache /// data as appropriate, but rely on the version being unique. + /// /// public interface IActionDescriptorCollectionProvider { diff --git a/src/Microsoft.AspNetCore.Mvc.Core/Internal/ActionDescriptorCollectionProvider.cs b/src/Microsoft.AspNetCore.Mvc.Core/Internal/ActionDescriptorCollectionProvider.cs deleted file mode 100644 index 49cb219178..0000000000 --- a/src/Microsoft.AspNetCore.Mvc.Core/Internal/ActionDescriptorCollectionProvider.cs +++ /dev/null @@ -1,95 +0,0 @@ -// Copyright (c) .NET Foundation. All rights reserved. -// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. - -using System.Collections.Generic; -using System.Collections.ObjectModel; -using System.Linq; -using System.Threading; -using Microsoft.AspNetCore.Mvc.Abstractions; -using Microsoft.AspNetCore.Mvc.Infrastructure; -using Microsoft.Extensions.Primitives; - -namespace Microsoft.AspNetCore.Mvc.Internal -{ - /// - /// Default implementation of . - /// - public class ActionDescriptorCollectionProvider : IActionDescriptorCollectionProvider - { - private readonly IActionDescriptorProvider[] _actionDescriptorProviders; - private readonly IActionDescriptorChangeProvider[] _actionDescriptorChangeProviders; - private ActionDescriptorCollection _collection; - private int _version = -1; - - /// - /// Initializes a new instance of the class. - /// - /// The sequence of . - /// The sequence of . - public ActionDescriptorCollectionProvider( - IEnumerable actionDescriptorProviders, - IEnumerable actionDescriptorChangeProviders) - { - _actionDescriptorProviders = actionDescriptorProviders - .OrderBy(p => p.Order) - .ToArray(); - - _actionDescriptorChangeProviders = actionDescriptorChangeProviders.ToArray(); - - ChangeToken.OnChange( - GetCompositeChangeToken, - UpdateCollection); - } - - private IChangeToken GetCompositeChangeToken() - { - if (_actionDescriptorChangeProviders.Length == 1) - { - return _actionDescriptorChangeProviders[0].GetChangeToken(); - } - - var changeTokens = new IChangeToken[_actionDescriptorChangeProviders.Length]; - for (var i = 0; i < _actionDescriptorChangeProviders.Length; i++) - { - changeTokens[i] = _actionDescriptorChangeProviders[i].GetChangeToken(); - } - - return new CompositeChangeToken(changeTokens); - } - - /// - /// Returns a cached collection of . - /// - public ActionDescriptorCollection ActionDescriptors - { - get - { - if (_collection == null) - { - UpdateCollection(); - } - - return _collection; - } - } - - private void UpdateCollection() - { - var context = new ActionDescriptorProviderContext(); - - for (var i = 0; i < _actionDescriptorProviders.Length; i++) - { - _actionDescriptorProviders[i].OnProvidersExecuting(context); - } - - for (var i = _actionDescriptorProviders.Length - 1; i >= 0; i--) - { - _actionDescriptorProviders[i].OnProvidersExecuted(context); - } - - _collection = new ActionDescriptorCollection( - new ReadOnlyCollection(context.Results), - Interlocked.Increment(ref _version)); - } - } -} \ No newline at end of file diff --git a/src/Microsoft.AspNetCore.Mvc.Core/Internal/MvcEndpointDataSource.cs b/src/Microsoft.AspNetCore.Mvc.Core/Internal/MvcEndpointDataSource.cs index 2837e6ec4a..560016a4c4 100644 --- a/src/Microsoft.AspNetCore.Mvc.Core/Internal/MvcEndpointDataSource.cs +++ b/src/Microsoft.AspNetCore.Mvc.Core/Internal/MvcEndpointDataSource.cs @@ -3,8 +3,10 @@ using System; using System.Collections.Generic; +using System.Diagnostics; using System.Linq; using System.Text; +using System.Threading; using Microsoft.AspNetCore.Builder; using Microsoft.AspNetCore.Http; using Microsoft.AspNetCore.Http.Features; @@ -13,25 +15,27 @@ using Microsoft.AspNetCore.Mvc.Infrastructure; using Microsoft.AspNetCore.Mvc.Routing; using Microsoft.AspNetCore.Routing; using Microsoft.AspNetCore.Routing.Patterns; -using Microsoft.Extensions.FileProviders; using Microsoft.Extensions.Primitives; namespace Microsoft.AspNetCore.Mvc.Internal { internal class MvcEndpointDataSource : EndpointDataSource { - private readonly object _lock = new object(); private readonly IActionDescriptorCollectionProvider _actions; private readonly MvcEndpointInvokerFactory _invokerFactory; private readonly DefaultHttpContext _httpContextInstance; - private readonly IActionDescriptorChangeProvider[] _actionDescriptorChangeProviders; + // The following are protected by this lock for WRITES only. This pattern is similar + // to DefaultActionDescriptorChangeProvider - see comments there for details on + // all of the threading behaviors. + private readonly object _lock = new object(); private List _endpoints; + private CancellationTokenSource _cancellationTokenSource; + private IChangeToken _changeToken; public MvcEndpointDataSource( IActionDescriptorCollectionProvider actions, MvcEndpointInvokerFactory invokerFactory, - IEnumerable actionDescriptorChangeProviders, IServiceProvider serviceProvider) { if (actions == null) @@ -44,11 +48,6 @@ namespace Microsoft.AspNetCore.Mvc.Internal throw new ArgumentNullException(nameof(invokerFactory)); } - if (actionDescriptorChangeProviders == null) - { - throw new ArgumentNullException(nameof(actionDescriptorChangeProviders)); - } - if (serviceProvider == null) { throw new ArgumentNullException(nameof(serviceProvider)); @@ -56,139 +55,199 @@ namespace Microsoft.AspNetCore.Mvc.Internal _actions = actions; _invokerFactory = invokerFactory; - _actionDescriptorChangeProviders = actionDescriptorChangeProviders.ToArray(); _httpContextInstance = new DefaultHttpContext() { RequestServices = serviceProvider }; ConventionalEndpointInfos = new List(); + + // It's possible for someone to override the collection provider without providing + // change notifications. If that's the case we won't process changes. + if (actions is ActionDescriptorCollectionProvider collectionProviderWithChangeToken) + { + ChangeToken.OnChange( + () => collectionProviderWithChangeToken.GetChangeToken(), + UpdateEndpoints); + } } - private List CreateEndpoints() + public List ConventionalEndpointInfos { get; } + + public override IReadOnlyList Endpoints { - var endpoints = new List(); - StringBuilder patternStringBuilder = null; - - foreach (var action in _actions.ActionDescriptors.Items) + get { - if (action.AttributeRouteInfo == null) + Initialize(); + Debug.Assert(_changeToken != null); + Debug.Assert(_endpoints != null); + return _endpoints; + } + } + + public override IChangeToken GetChangeToken() + { + Initialize(); + Debug.Assert(_changeToken != null); + Debug.Assert(_endpoints != null); + return _changeToken; + } + + private void Initialize() + { + if (_endpoints == null) + { + lock (_lock) { - // In traditional conventional routing setup, the routes defined by a user have a static order - // defined by how they are added into the list. We would like to maintain the same order when building - // up the endpoints too. - // - // Start with an order of '1' for conventional routes as attribute routes have a default order of '0'. - // This is for scenarios dealing with migrating existing Router based code to Endpoint Routing world. - var conventionalRouteOrder = 0; - - // Check each of the conventional patterns to see if the action would be reachable - // If the action and pattern are compatible then create an endpoint with the - // area/controller/action parameter parts replaced with literals - // - // e.g. {controller}/{action} with HomeController.Index and HomeController.Login - // would result in endpoints: - // - Home/Index - // - Home/Login - foreach (var endpointInfo in ConventionalEndpointInfos) + if (_endpoints == null) { - // An 'endpointInfo' is applicable if: - // 1. it has a parameter (or default value) for 'required' non-null route value - // 2. it does not have a parameter (or default value) for 'required' null route value - var isApplicable = true; - foreach (var routeKey in action.RouteValues.Keys) + UpdateEndpoints(); + } + } + } + } + + private void UpdateEndpoints() + { + lock (_lock) + { + var endpoints = new List(); + StringBuilder patternStringBuilder = null; + + foreach (var action in _actions.ActionDescriptors.Items) + { + if (action.AttributeRouteInfo == null) + { + // In traditional conventional routing setup, the routes defined by a user have a static order + // defined by how they are added into the list. We would like to maintain the same order when building + // up the endpoints too. + // + // Start with an order of '1' for conventional routes as attribute routes have a default order of '0'. + // This is for scenarios dealing with migrating existing Router based code to Endpoint Routing world. + var conventionalRouteOrder = 0; + + // Check each of the conventional patterns to see if the action would be reachable + // If the action and pattern are compatible then create an endpoint with the + // area/controller/action parameter parts replaced with literals + // + // e.g. {controller}/{action} with HomeController.Index and HomeController.Login + // would result in endpoints: + // - Home/Index + // - Home/Login + foreach (var endpointInfo in ConventionalEndpointInfos) { - if (!MatchRouteValue(action, endpointInfo, routeKey)) + // An 'endpointInfo' is applicable if: + // 1. it has a parameter (or default value) for 'required' non-null route value + // 2. it does not have a parameter (or default value) for 'required' null route value + var isApplicable = true; + foreach (var routeKey in action.RouteValues.Keys) { - isApplicable = false; - break; - } - } - - if (!isApplicable) - { - continue; - } - - var newPathSegments = endpointInfo.ParsedPattern.PathSegments.ToList(); - - for (var i = 0; i < newPathSegments.Count; i++) - { - // Check if the pattern can be shortened because the remaining parameters are optional - // - // e.g. Matching pattern {controller=Home}/{action=Index}/{id?} against HomeController.Index - // can resolve to the following endpoints: - // - /Home/Index/{id?} - // - /Home - // - / - if (UseDefaultValuePlusRemainingSegementsOptional(i, action, endpointInfo, newPathSegments)) - { - var subPathSegments = newPathSegments.Take(i); - - var subEndpoint = CreateEndpoint( - action, - endpointInfo.Name, - GetPattern(ref patternStringBuilder, subPathSegments), - subPathSegments, - endpointInfo.Defaults, - ++conventionalRouteOrder, - endpointInfo, - suppressLinkGeneration: false); - endpoints.Add(subEndpoint); - } - - List segmentParts = null; // Initialize only as needed - var segment = newPathSegments[i]; - for (var j = 0; j < segment.Parts.Count; j++) - { - var part = segment.Parts[j]; - - if (part.IsParameter && - part is RoutePatternParameterPart parameterPart && - action.RouteValues.ContainsKey(parameterPart.Name)) + if (!MatchRouteValue(action, endpointInfo, routeKey)) { - if (segmentParts == null) - { - segmentParts = segment.Parts.ToList(); - } - - // Replace parameter with literal value - segmentParts[j] = RoutePatternFactory.LiteralPart(action.RouteValues[parameterPart.Name]); + isApplicable = false; + break; } } - // A parameter part was replaced so replace segment with updated parts - if (segmentParts != null) + if (!isApplicable) { - newPathSegments[i] = RoutePatternFactory.Segment(segmentParts); + continue; } - } + var newPathSegments = endpointInfo.ParsedPattern.PathSegments.ToList(); + + for (var i = 0; i < newPathSegments.Count; i++) + { + // Check if the pattern can be shortened because the remaining parameters are optional + // + // e.g. Matching pattern {controller=Home}/{action=Index}/{id?} against HomeController.Index + // can resolve to the following endpoints: + // - /Home/Index/{id?} + // - /Home + // - / + if (UseDefaultValuePlusRemainingSegementsOptional(i, action, endpointInfo, newPathSegments)) + { + var subPathSegments = newPathSegments.Take(i); + + var subEndpoint = CreateEndpoint( + action, + endpointInfo.Name, + GetPattern(ref patternStringBuilder, subPathSegments), + subPathSegments, + endpointInfo.Defaults, + ++conventionalRouteOrder, + endpointInfo, + suppressLinkGeneration: false); + endpoints.Add(subEndpoint); + } + + List segmentParts = null; // Initialize only as needed + var segment = newPathSegments[i]; + for (var j = 0; j < segment.Parts.Count; j++) + { + var part = segment.Parts[j]; + + if (part.IsParameter && + part is RoutePatternParameterPart parameterPart && + action.RouteValues.ContainsKey(parameterPart.Name)) + { + if (segmentParts == null) + { + segmentParts = segment.Parts.ToList(); + } + + // Replace parameter with literal value + segmentParts[j] = RoutePatternFactory.LiteralPart(action.RouteValues[parameterPart.Name]); + } + } + + // A parameter part was replaced so replace segment with updated parts + if (segmentParts != null) + { + newPathSegments[i] = RoutePatternFactory.Segment(segmentParts); + } + } + + var endpoint = CreateEndpoint( + action, + endpointInfo.Name, + GetPattern(ref patternStringBuilder, newPathSegments), + newPathSegments, + endpointInfo.Defaults, + ++conventionalRouteOrder, + endpointInfo, + suppressLinkGeneration: false); + endpoints.Add(endpoint); + } + } + else + { var endpoint = CreateEndpoint( action, - endpointInfo.Name, - GetPattern(ref patternStringBuilder, newPathSegments), - newPathSegments, - endpointInfo.Defaults, - ++conventionalRouteOrder, - endpointInfo, - suppressLinkGeneration: false); + action.AttributeRouteInfo.Name, + action.AttributeRouteInfo.Template, + RoutePatternFactory.Parse(action.AttributeRouteInfo.Template).PathSegments, + nonInlineDefaults: null, + action.AttributeRouteInfo.Order, + action.AttributeRouteInfo, + suppressLinkGeneration: action.AttributeRouteInfo.SuppressLinkGeneration); endpoints.Add(endpoint); } } - else - { - var endpoint = CreateEndpoint( - action, - action.AttributeRouteInfo.Name, - action.AttributeRouteInfo.Template, - RoutePatternFactory.Parse(action.AttributeRouteInfo.Template).PathSegments, - nonInlineDefaults: null, - action.AttributeRouteInfo.Order, - action.AttributeRouteInfo, - suppressLinkGeneration: action.AttributeRouteInfo.SuppressLinkGeneration); - endpoints.Add(endpoint); - } - } - return endpoints; + // See comments in DefaultActionDescriptorCollectionProvider. These steps are done + // in a specific order to ensure callers always see a consistent state. + + // Step 1 - capture old token + var oldCancellationTokenSource = _cancellationTokenSource; + + // Step 2 - update endpoints + _endpoints = endpoints; + + // Step 3 - create new change token + _cancellationTokenSource = new CancellationTokenSource(); + _changeToken = new CancellationChangeToken(_cancellationTokenSource.Token); + + // Step 4 - trigger old token + oldCancellationTokenSource?.Cancel(); + } string GetPattern(ref StringBuilder sb, IEnumerable segments) { @@ -442,49 +501,6 @@ namespace Microsoft.AspNetCore.Mvc.Internal } } - private IChangeToken GetCompositeChangeToken() - { - if (_actionDescriptorChangeProviders.Length == 1) - { - return _actionDescriptorChangeProviders[0].GetChangeToken(); - } - - var changeTokens = new IChangeToken[_actionDescriptorChangeProviders.Length]; - for (var i = 0; i < _actionDescriptorChangeProviders.Length; i++) - { - changeTokens[i] = _actionDescriptorChangeProviders[i].GetChangeToken(); - } - - return new CompositeChangeToken(changeTokens); - } - - public override IChangeToken GetChangeToken() => NullChangeToken.Singleton; - - public override IReadOnlyList Endpoints - { - get - { - // Want to initialize endpoints once and then cache while ensuring a null collection is never returned - // Local copy for thread safety + double check locking - var localEndpoints = _endpoints; - if (localEndpoints == null) - { - lock (_lock) - { - localEndpoints = _endpoints; - if (localEndpoints == null) - { - _endpoints = localEndpoints = CreateEndpoints(); - } - } - } - - return localEndpoints; - } - } - - public List ConventionalEndpointInfos { get; } - private class SuppressLinkGenerationMetadata : ISuppressLinkGenerationMetadata { } } } \ No newline at end of file diff --git a/test/Microsoft.AspNetCore.Mvc.Core.Test/Internal/ActionDescriptorCollectionProviderTest.cs b/test/Microsoft.AspNetCore.Mvc.Core.Test/Infrastructure/DefaultActionDescriptorCollectionProviderTest.cs similarity index 70% rename from test/Microsoft.AspNetCore.Mvc.Core.Test/Internal/ActionDescriptorCollectionProviderTest.cs rename to test/Microsoft.AspNetCore.Mvc.Core.Test/Infrastructure/DefaultActionDescriptorCollectionProviderTest.cs index 0c599b47c6..cac4f58aec 100644 --- a/test/Microsoft.AspNetCore.Mvc.Core.Test/Internal/ActionDescriptorCollectionProviderTest.cs +++ b/test/Microsoft.AspNetCore.Mvc.Core.Test/Infrastructure/DefaultActionDescriptorCollectionProviderTest.cs @@ -4,14 +4,13 @@ using System.Linq; using System.Threading; using Microsoft.AspNetCore.Mvc.Abstractions; -using Microsoft.AspNetCore.Mvc.Infrastructure; using Microsoft.Extensions.Primitives; using Moq; using Xunit; -namespace Microsoft.AspNetCore.Mvc.Internal +namespace Microsoft.AspNetCore.Mvc.Infrastructure { - public class ActionDescriptorCollectionProviderTest + public class DefaultActionDescriptorCollectionProviderTest { [Fact] public void ActionDescriptors_ReadsDescriptorsFromActionDescriptorProviders() @@ -24,7 +23,7 @@ namespace Microsoft.AspNetCore.Mvc.Internal var expected3 = new ActionDescriptor(); var actionDescriptorProvider2 = GetActionDescriptorProvider(expected2, expected3); - var actionDescriptorCollectionProvider = new ActionDescriptorCollectionProvider( + var actionDescriptorCollectionProvider = new DefaultActionDescriptorCollectionProvider( new[] { actionDescriptorProvider1, actionDescriptorProvider2 }, Enumerable.Empty()); @@ -46,7 +45,7 @@ namespace Microsoft.AspNetCore.Mvc.Internal // Arrange var actionDescriptorProvider = GetActionDescriptorProvider(new ActionDescriptor()); - var actionDescriptorCollectionProvider = new ActionDescriptorCollectionProvider( + var actionDescriptorCollectionProvider = new DefaultActionDescriptorCollectionProvider( new[] { actionDescriptorProvider }, Enumerable.Empty()); @@ -66,55 +65,7 @@ namespace Microsoft.AspNetCore.Mvc.Internal } [Fact] - public void ActionDescriptors_UpdateWhenChangeTokenProviderChanges() - { - // Arrange - var actionDescriptorProvider = new Mock(); - var expected1 = new ActionDescriptor(); - var expected2 = new ActionDescriptor(); - - var invocations = 0; - actionDescriptorProvider - .Setup(p => p.OnProvidersExecuting(It.IsAny())) - .Callback((ActionDescriptorProviderContext context) => - { - if (invocations == 0) - { - context.Results.Add(expected1); - } - else - { - context.Results.Add(expected2); - } - - invocations++; - }); - var changeProvider = new TestChangeProvider(); - var actionDescriptorCollectionProvider = new ActionDescriptorCollectionProvider( - new[] { actionDescriptorProvider.Object }, - new[] { changeProvider }); - - // Act - 1 - var collection1 = actionDescriptorCollectionProvider.ActionDescriptors; - - // Assert - 1 - Assert.Equal(0, collection1.Version); - Assert.Collection(collection1.Items, - item => Assert.Same(expected1, item)); - - // Act - 2 - changeProvider.TokenSource.Cancel(); - var collection2 = actionDescriptorCollectionProvider.ActionDescriptors; - - // Assert - 2 - Assert.NotSame(collection1, collection2); - Assert.Equal(1, collection2.Version); - Assert.Collection(collection2.Items, - item => Assert.Same(expected2, item)); - } - - [Fact] - public void ActionDescriptors_SubscribesToNewChangeNotificationsAfterInvalidating() + public void ActionDescriptors_UpdatesAndResubscripes_WhenChangeTokenTriggers() { // Arrange var actionDescriptorProvider = new Mock(); @@ -143,34 +94,61 @@ namespace Microsoft.AspNetCore.Mvc.Internal invocations++; }); var changeProvider = new TestChangeProvider(); - var actionDescriptorCollectionProvider = new ActionDescriptorCollectionProvider( + var actionDescriptorCollectionProvider = new DefaultActionDescriptorCollectionProvider( new[] { actionDescriptorProvider.Object }, new[] { changeProvider }); // Act - 1 + var changeToken1 = actionDescriptorCollectionProvider.GetChangeToken(); var collection1 = actionDescriptorCollectionProvider.ActionDescriptors; + ActionDescriptorCollection captured = null; + changeToken1.RegisterChangeCallback((_) => + { + captured = actionDescriptorCollectionProvider.ActionDescriptors; + }, null); + // Assert - 1 + Assert.False(changeToken1.HasChanged); Assert.Equal(0, collection1.Version); Assert.Collection(collection1.Items, item => Assert.Same(expected1, item)); // Act - 2 changeProvider.TokenSource.Cancel(); + var changeToken2 = actionDescriptorCollectionProvider.GetChangeToken(); var collection2 = actionDescriptorCollectionProvider.ActionDescriptors; + changeToken2.RegisterChangeCallback((_) => + { + captured = actionDescriptorCollectionProvider.ActionDescriptors; + }, null); + // Assert - 2 + Assert.NotSame(changeToken1, changeToken2); + Assert.True(changeToken1.HasChanged); + Assert.False(changeToken2.HasChanged); + Assert.NotSame(collection1, collection2); + Assert.NotNull(captured); + Assert.Same(captured, collection2); Assert.Equal(1, collection2.Version); Assert.Collection(collection2.Items, item => Assert.Same(expected2, item)); // Act - 3 changeProvider.TokenSource.Cancel(); + var changeToken3 = actionDescriptorCollectionProvider.GetChangeToken(); var collection3 = actionDescriptorCollectionProvider.ActionDescriptors; // Assert - 3 + Assert.NotSame(changeToken2, changeToken3); + Assert.True(changeToken2.HasChanged); + Assert.False(changeToken3.HasChanged); + Assert.NotSame(collection2, collection3); + Assert.NotNull(captured); + Assert.Same(captured, collection3); Assert.Equal(2, collection3.Version); Assert.Collection(collection3.Items, item => Assert.Same(expected3, item)); diff --git a/test/Microsoft.AspNetCore.Mvc.Core.Test/Internal/ActionConstraintCacheTest.cs b/test/Microsoft.AspNetCore.Mvc.Core.Test/Internal/ActionConstraintCacheTest.cs index 0187f81666..c4374a5bbb 100644 --- a/test/Microsoft.AspNetCore.Mvc.Core.Test/Internal/ActionConstraintCacheTest.cs +++ b/test/Microsoft.AspNetCore.Mvc.Core.Test/Internal/ActionConstraintCacheTest.cs @@ -159,7 +159,7 @@ namespace Microsoft.AspNetCore.Mvc.Internal private static ActionConstraintCache CreateCache(params IActionConstraintProvider[] providers) { - var descriptorProvider = new ActionDescriptorCollectionProvider( + var descriptorProvider = new DefaultActionDescriptorCollectionProvider( Enumerable.Empty(), Enumerable.Empty()); return new ActionConstraintCache(descriptorProvider, providers); diff --git a/test/Microsoft.AspNetCore.Mvc.Core.Test/Internal/ActionSelectorTest.cs b/test/Microsoft.AspNetCore.Mvc.Core.Test/Internal/ActionSelectorTest.cs index 5835a579f0..5b3adffcbc 100644 --- a/test/Microsoft.AspNetCore.Mvc.Core.Test/Internal/ActionSelectorTest.cs +++ b/test/Microsoft.AspNetCore.Mvc.Core.Test/Internal/ActionSelectorTest.cs @@ -932,7 +932,7 @@ namespace Microsoft.AspNetCore.Mvc.Infrastructure private ControllerActionDescriptor InvokeActionSelector(RouteContext context) { var actionDescriptorProvider = GetActionDescriptorProvider(); - var actionDescriptorCollectionProvider = new ActionDescriptorCollectionProvider( + var actionDescriptorCollectionProvider = new DefaultActionDescriptorCollectionProvider( new[] { actionDescriptorProvider }, Enumerable.Empty()); @@ -1092,7 +1092,7 @@ namespace Microsoft.AspNetCore.Mvc.Infrastructure private static ActionConstraintCache GetActionConstraintCache(IActionConstraintProvider[] actionConstraintProviders = null) { - var descriptorProvider = new ActionDescriptorCollectionProvider( + var descriptorProvider = new DefaultActionDescriptorCollectionProvider( Enumerable.Empty(), Enumerable.Empty()); return new ActionConstraintCache(descriptorProvider, actionConstraintProviders.AsEnumerable() ?? new List()); diff --git a/test/Microsoft.AspNetCore.Mvc.Core.Test/Internal/MvcEndpointDataSourceTests.cs b/test/Microsoft.AspNetCore.Mvc.Core.Test/Internal/MvcEndpointDataSourceTests.cs index 416e359291..be31acba0b 100644 --- a/test/Microsoft.AspNetCore.Mvc.Core.Test/Internal/MvcEndpointDataSourceTests.cs +++ b/test/Microsoft.AspNetCore.Mvc.Core.Test/Internal/MvcEndpointDataSourceTests.cs @@ -133,47 +133,6 @@ namespace Microsoft.AspNetCore.Mvc.Internal Assert.True(actionInvokerCalled); } - [Fact(Skip = "https://github.com/aspnet/Routing/issues/722")] - public void GetChangeToken_MultipleChangeTokenProviders_ComposedResult() - { - // Arrange - var featureCollection = new FeatureCollection(); - featureCollection.Set(new EndpointFeature - { - RouteValues = new RouteValueDictionary() - }); - - var httpContextMock = new Mock(); - httpContextMock.Setup(m => m.Features).Returns(featureCollection); - - var descriptorProviderMock = new Mock(); - descriptorProviderMock.Setup(m => m.ActionDescriptors).Returns(new ActionDescriptorCollection(new List(), 0)); - - var actionInvokerMock = new Mock(); - - var actionInvokerProviderMock = new Mock(); - actionInvokerProviderMock.Setup(m => m.CreateInvoker(It.IsAny())).Returns(actionInvokerMock.Object); - - var changeTokenMock = new Mock(); - - var changeProvider1Mock = new Mock(); - changeProvider1Mock.Setup(m => m.GetChangeToken()).Returns(changeTokenMock.Object); - var changeProvider2Mock = new Mock(); - changeProvider2Mock.Setup(m => m.GetChangeToken()).Returns(changeTokenMock.Object); - - var dataSource = CreateMvcEndpointDataSource( - descriptorProviderMock.Object, - new MvcEndpointInvokerFactory(actionInvokerProviderMock.Object), - new[] { changeProvider1Mock.Object, changeProvider2Mock.Object }); - - // Act - var changeToken = dataSource.GetChangeToken(); - - // Assert - var compositeChangeToken = Assert.IsType(changeToken); - Assert.Equal(2, compositeChangeToken.ChangeTokens.Count); - } - [Theory] [InlineData("{controller}/{action}/{id?}", new[] { "TestController/TestAction/{id?}" })] [InlineData("{controller}/{id?}", new string[] { })] @@ -287,11 +246,11 @@ namespace Microsoft.AspNetCore.Mvc.Internal actionDescriptorCollectionProviderMock.VerifyGet(m => m.ActionDescriptors, Times.Once); } - [Fact(Skip = "https://github.com/aspnet/Routing/issues/722")] + [Fact] public void Endpoints_ChangeTokenTriggered_EndpointsRecreated() { // Arrange - var actionDescriptorCollectionProviderMock = new Mock(); + var actionDescriptorCollectionProviderMock = new Mock(); actionDescriptorCollectionProviderMock .Setup(m => m.ActionDescriptors) .Returns(new ActionDescriptorCollection(new[] @@ -300,19 +259,18 @@ namespace Microsoft.AspNetCore.Mvc.Internal }, version: 0)); CancellationTokenSource cts = null; + actionDescriptorCollectionProviderMock + .Setup(m => m.GetChangeToken()) + .Returns(() => + { + cts = new CancellationTokenSource(); + var changeToken = new CancellationChangeToken(cts.Token); - var changeProviderMock = new Mock(); - changeProviderMock.Setup(m => m.GetChangeToken()).Returns(() => - { - cts = new CancellationTokenSource(); - var changeToken = new CancellationChangeToken(cts.Token); + return changeToken; + }); - return changeToken; - }); - var dataSource = CreateMvcEndpointDataSource( - actionDescriptorCollectionProviderMock.Object, - actionDescriptorChangeProviders: new[] { changeProviderMock.Object }); + var dataSource = CreateMvcEndpointDataSource(actionDescriptorCollectionProviderMock.Object); dataSource.ConventionalEndpointInfos.Add(CreateEndpointInfo( string.Empty, "{controller}/{action}", @@ -752,15 +710,13 @@ namespace Microsoft.AspNetCore.Mvc.Internal private MvcEndpointDataSource CreateMvcEndpointDataSource( IActionDescriptorCollectionProvider actionDescriptorCollectionProvider = null, - MvcEndpointInvokerFactory mvcEndpointInvokerFactory = null, - IEnumerable actionDescriptorChangeProviders = null) + MvcEndpointInvokerFactory mvcEndpointInvokerFactory = null) { if (actionDescriptorCollectionProvider == null) { - var mockDescriptorProvider = new Mock(); - mockDescriptorProvider.Setup(m => m.ActionDescriptors).Returns(new ActionDescriptorCollection(new List(), 0)); - - actionDescriptorCollectionProvider = mockDescriptorProvider.Object; + actionDescriptorCollectionProvider = new DefaultActionDescriptorCollectionProvider( + Array.Empty(), + Array.Empty()); } var serviceProviderMock = new Mock(); @@ -769,7 +725,6 @@ namespace Microsoft.AspNetCore.Mvc.Internal var dataSource = new MvcEndpointDataSource( actionDescriptorCollectionProvider, mvcEndpointInvokerFactory ?? new MvcEndpointInvokerFactory(new ActionInvokerFactory(Array.Empty())), - actionDescriptorChangeProviders ?? Array.Empty(), serviceProviderMock.Object); return dataSource; diff --git a/test/Microsoft.AspNetCore.Mvc.Core.Test/Routing/ActionConstraintMatcherPolicyTest.cs b/test/Microsoft.AspNetCore.Mvc.Core.Test/Routing/ActionConstraintMatcherPolicyTest.cs index 298786c121..5936d34ef6 100644 --- a/test/Microsoft.AspNetCore.Mvc.Core.Test/Routing/ActionConstraintMatcherPolicyTest.cs +++ b/test/Microsoft.AspNetCore.Mvc.Core.Test/Routing/ActionConstraintMatcherPolicyTest.cs @@ -351,7 +351,7 @@ namespace Microsoft.AspNetCore.Mvc.Routing } }); - var actionDescriptorCollectionProvider = new ActionDescriptorCollectionProvider( + var actionDescriptorCollectionProvider = new DefaultActionDescriptorCollectionProvider( new IActionDescriptorProvider[] { actionDescriptorProvider.Object, }, Enumerable.Empty()); @@ -398,7 +398,7 @@ namespace Microsoft.AspNetCore.Mvc.Routing private static ActionConstraintCache GetActionConstraintCache(IActionConstraintProvider[] actionConstraintProviders = null) { - var descriptorProvider = new ActionDescriptorCollectionProvider( + var descriptorProvider = new DefaultActionDescriptorCollectionProvider( Enumerable.Empty(), Enumerable.Empty()); return new ActionConstraintCache(descriptorProvider, actionConstraintProviders.AsEnumerable() ?? new List()); diff --git a/test/Microsoft.AspNetCore.Mvc.Core.Test/Routing/KnownRouteValueConstraintTests.cs b/test/Microsoft.AspNetCore.Mvc.Core.Test/Routing/KnownRouteValueConstraintTests.cs index e6ee1638f6..1e1d1f8951 100644 --- a/test/Microsoft.AspNetCore.Mvc.Core.Test/Routing/KnownRouteValueConstraintTests.cs +++ b/test/Microsoft.AspNetCore.Mvc.Core.Test/Routing/KnownRouteValueConstraintTests.cs @@ -173,7 +173,7 @@ namespace Microsoft.AspNetCore.Mvc.Routing .Setup(p => p.OnProvidersExecuted(It.IsAny())) .Verifiable(); - var descriptorCollectionProvider = new ActionDescriptorCollectionProvider( + var descriptorCollectionProvider = new DefaultActionDescriptorCollectionProvider( new[] { actionProvider.Object }, Enumerable.Empty()); diff --git a/test/Microsoft.AspNetCore.Mvc.FunctionalTests/ApiExplorerTest.cs b/test/Microsoft.AspNetCore.Mvc.FunctionalTests/ApiExplorerTest.cs index 40e556ad72..d216ad4aff 100644 --- a/test/Microsoft.AspNetCore.Mvc.FunctionalTests/ApiExplorerTest.cs +++ b/test/Microsoft.AspNetCore.Mvc.FunctionalTests/ApiExplorerTest.cs @@ -1087,7 +1087,7 @@ namespace Microsoft.AspNetCore.Mvc.FunctionalTests }); } - [Fact(Skip = "https://github.com/aspnet/Routing/issues/722")] + [Fact] public async Task ApiExplorer_Updates_WhenActionDescriptorCollectionIsUpdated() { // Act - 1