From 2eec0bbf6d1c712ebc138d144239f3090523126f Mon Sep 17 00:00:00 2001 From: jacalvar Date: Mon, 18 Aug 2014 18:48:04 -0700 Subject: [PATCH] [Fixes #1035] RouteGroupConstraint should only be added once for non attribute routed actions 1. Changed ReflectedActionDescriptorProvider to add RouteGroupConstraint only once for non attribute routed actions. 2. Added tests to cover the scenario. --- .../ReflectedActionDescriptorProvider.cs | 59 +++++---- .../Routing/ActionSelectorDecisionTree.cs | 10 +- .../ReflectedActionDescriptorProviderTests.cs | 124 +++++++++++++++++- 3 files changed, 152 insertions(+), 41 deletions(-) diff --git a/src/Microsoft.AspNet.Mvc.Core/ReflectedActionDescriptorProvider.cs b/src/Microsoft.AspNet.Mvc.Core/ReflectedActionDescriptorProvider.cs index 8d263f89ee..0090626a58 100644 --- a/src/Microsoft.AspNet.Mvc.Core/ReflectedActionDescriptorProvider.cs +++ b/src/Microsoft.AspNet.Mvc.Core/ReflectedActionDescriptorProvider.cs @@ -289,41 +289,44 @@ namespace Microsoft.AspNet.Mvc actions.Add(actionDescriptor); } + } - foreach (var actionDescriptor in actions) + foreach (var actionDescriptor in actions) + { + if (actionDescriptor.AttributeRouteInfo == null || + actionDescriptor.AttributeRouteInfo.Template == null) { + // Any any attribute routes are in use, then non-attribute-routed ADs can't be selected + // when a route group returned by the route. + if (hasAttributeRoutes) + { + actionDescriptor.RouteConstraints.Add(new RouteDataActionConstraint( + AttributeRouting.RouteGroupKey, + RouteKeyHandling.DenyKey)); + } + foreach (var key in removalConstraints) { - if (actionDescriptor.AttributeRouteInfo == null || - actionDescriptor.AttributeRouteInfo.Template == null) + if (!HasConstraint(actionDescriptor.RouteConstraints, key)) { - // Any any attribute routes are in use, then non-attribute-routed ADs can't be selected - // when a route group returned by the route. - if (hasAttributeRoutes) - { - actionDescriptor.RouteConstraints.Add(new RouteDataActionConstraint( - AttributeRouting.RouteGroupKey, - RouteKeyHandling.DenyKey)); - } - - if (!HasConstraint(actionDescriptor.RouteConstraints, key)) - { - actionDescriptor.RouteConstraints.Add(new RouteDataActionConstraint( - key, - RouteKeyHandling.DenyKey)); - } + actionDescriptor.RouteConstraints.Add(new RouteDataActionConstraint( + key, + RouteKeyHandling.DenyKey)); } - else + } + } + else + { + // We still want to add a 'null' for any constraint with DenyKey so that link generation + // works properly. + // + // Consider an action like { area = "", controller = "Home", action = "Index" }. Even if + // it's attribute routed, it needs to know that area must be null to generate a link. + foreach (var key in removalConstraints) + { + if (!actionDescriptor.RouteValueDefaults.ContainsKey(key)) { - // We still want to add a 'null' for any constraint with DenyKey so that link generation - // works properly. - // - // Consider an action like { area = "", controller = "Home", action = "Index" }. Even if - // it's attribute routed, it needs to know that area must be null to generate a link. - if (!actionDescriptor.RouteValueDefaults.ContainsKey(key)) - { - actionDescriptor.RouteValueDefaults.Add(key, null); - } + actionDescriptor.RouteValueDefaults.Add(key, null); } } } diff --git a/src/Microsoft.AspNet.Mvc.Core/Routing/ActionSelectorDecisionTree.cs b/src/Microsoft.AspNet.Mvc.Core/Routing/ActionSelectorDecisionTree.cs index fb1eced959..867fd7f347 100644 --- a/src/Microsoft.AspNet.Mvc.Core/Routing/ActionSelectorDecisionTree.cs +++ b/src/Microsoft.AspNet.Mvc.Core/Routing/ActionSelectorDecisionTree.cs @@ -168,19 +168,15 @@ namespace Microsoft.AspNet.Mvc.Routing // would throw. #if NET45 throw new InvalidEnumArgumentException( - "item", - (int)constraint.KeyHandling, + "item", + (int)constraint.KeyHandling, typeof(RouteKeyHandling)); #else throw new ArgumentOutOfRangeException("item"); #endif } - // Workaround for Javier's cool bug. - if (!results.ContainsKey(constraint.RouteKey)) - { - results.Add(constraint.RouteKey, value); - } + results.Add(constraint.RouteKey, value); } } diff --git a/test/Microsoft.AspNet.Mvc.Core.Test/ReflectedActionDescriptorProviderTests.cs b/test/Microsoft.AspNet.Mvc.Core.Test/ReflectedActionDescriptorProviderTests.cs index 68bdc0fc5b..e4bc637118 100644 --- a/test/Microsoft.AspNet.Mvc.Core.Test/ReflectedActionDescriptorProviderTests.cs +++ b/test/Microsoft.AspNet.Mvc.Core.Test/ReflectedActionDescriptorProviderTests.cs @@ -84,16 +84,16 @@ namespace Microsoft.AspNet.Mvc.Test typeof(BlockNonAttributedActionsController).GetTypeInfo()).ToArray(); var descriptorWithoutConstraint = Assert.Single( - descriptors, + descriptors, ad => ad.RouteConstraints.Any( c => c.RouteKey == "key" && c.KeyHandling == RouteKeyHandling.DenyKey)); var descriptorWithConstraint = Assert.Single( descriptors, ad => ad.RouteConstraints.Any( - c => - c.KeyHandling == RouteKeyHandling.RequireKey && - c.RouteKey == "key" && + c => + c.KeyHandling == RouteKeyHandling.RequireKey && + c.RouteKey == "key" && c.RouteValue == "value")); // Assert @@ -265,6 +265,71 @@ namespace Microsoft.AspNet.Mvc.Test Assert.Equal(expectedMessage, ex.Message); } + [Fact] + public void AttributeRouting_RouteGroupConstraint_IsAddedOnceForNonAttributeRoutes() + { + // Arrange + var provider = GetProvider( + typeof(MixedAttributeRouteController).GetTypeInfo(), + typeof(ConstrainedController).GetTypeInfo()); + + // Act + var actionDescriptors = provider.GetDescriptors(); + + // Assert + Assert.NotNull(actionDescriptors); + Assert.Equal(4, actionDescriptors.Count()); + + foreach (var actionDescriptor in actionDescriptors.Where(ad => ad.AttributeRouteInfo == null)) + { + Assert.Equal(6, actionDescriptor.RouteConstraints.Count); + var routeGroupConstraint = Assert.Single(actionDescriptor.RouteConstraints, + rc => rc.RouteKey.Equals(AttributeRouting.RouteGroupKey)); + Assert.Equal(RouteKeyHandling.DenyKey, routeGroupConstraint.KeyHandling); + } + } + + [Fact] + public void AttributeRouting_AddsDefaultRouteValues_ForAttributeRoutedActions() + { + // Arrange + var provider = GetProvider( + typeof(MixedAttributeRouteController).GetTypeInfo(), + typeof(ConstrainedController).GetTypeInfo()); + + // Act + var actionDescriptors = provider.GetDescriptors(); + + // Assert + Assert.NotNull(actionDescriptors); + Assert.Equal(4, actionDescriptors.Count()); + + var indexAction = Assert.Single(actionDescriptors, ad => ad.Name.Equals("Index")); + + Assert.Equal(1, indexAction.RouteConstraints.Count); + + var routeGroupConstraint = Assert.Single(indexAction.RouteConstraints, rc => rc.RouteKey.Equals(AttributeRouting.RouteGroupKey)); + Assert.Equal(RouteKeyHandling.RequireKey, routeGroupConstraint.KeyHandling); + Assert.NotNull(routeGroupConstraint.RouteValue); + + Assert.Equal(5, indexAction.RouteValueDefaults.Count); + + var controllerDefault = Assert.Single(indexAction.RouteValueDefaults, rd => rd.Key.Equals("controller", StringComparison.OrdinalIgnoreCase)); + Assert.Equal("MixedAttributeRoute", controllerDefault.Value); + + var actionDefault = Assert.Single(indexAction.RouteValueDefaults, rd => rd.Key.Equals("action", StringComparison.OrdinalIgnoreCase)); + Assert.Equal("Index", actionDefault.Value); + + var areaDefault = Assert.Single(indexAction.RouteValueDefaults, rd => rd.Key.Equals("area", StringComparison.OrdinalIgnoreCase)); + Assert.Equal("Home", areaDefault.Value); + + var myRouteConstraintDefault = Assert.Single(indexAction.RouteValueDefaults, rd => rd.Key.Equals("key", StringComparison.OrdinalIgnoreCase)); + Assert.Null(myRouteConstraintDefault.Value); + + var anotherRouteConstraint = Assert.Single(indexAction.RouteValueDefaults, rd => rd.Key.Equals("second", StringComparison.OrdinalIgnoreCase)); + Assert.Null(anotherRouteConstraint.Value); + } + [Fact] public void AttributeRouting_TokenReplacement_CaseInsensitive() { @@ -318,7 +383,7 @@ namespace Microsoft.AspNet.Mvc.Test } private ReflectedActionDescriptorProvider GetProvider( - TypeInfo controllerTypeInfo, + TypeInfo controllerTypeInfo, IEnumerable filters = null) { var conventions = new StaticActionDiscoveryConventions(controllerTypeInfo); @@ -338,6 +403,26 @@ namespace Microsoft.AspNet.Mvc.Test return provider; } + private ReflectedActionDescriptorProvider GetProvider( + params TypeInfo[] controllerTypeInfo) + { + var conventions = new StaticActionDiscoveryConventions(controllerTypeInfo); + + var assemblyProvider = new Mock(); + assemblyProvider + .SetupGet(ap => ap.CandidateAssemblies) + .Returns(new Assembly[] { controllerTypeInfo.First().Assembly }); + + var provider = new ReflectedActionDescriptorProvider( + assemblyProvider.Object, + conventions, + Enumerable.Empty(), + new MockMvcOptionsAccessor(), + Mock.Of()); + + return provider; + } + private IEnumerable GetDescriptors(params TypeInfo[] controllerTypeInfos) { var conventions = new StaticActionDiscoveryConventions(controllerTypeInfos); @@ -353,7 +438,7 @@ namespace Microsoft.AspNet.Mvc.Test null, new MockMvcOptionsAccessor(), null); - + return provider.GetDescriptors(); } @@ -386,6 +471,14 @@ namespace Microsoft.AspNet.Mvc.Test } } + public class MySecondRouteConstraintAttribute : RouteConstraintAttribute + { + public MySecondRouteConstraintAttribute(bool blockNonAttributedActions) + : base("second", "value", blockNonAttributedActions) + { + } + } + [MyRouteConstraintAttribute(blockNonAttributedActions: true)] private class BlockNonAttributedActionsController { @@ -458,5 +551,24 @@ namespace Microsoft.AspNet.Mvc.Test [HttpGet("stub/Action1")] public void Action2() { } } + + [Area("Home")] + private class MixedAttributeRouteController + { + [HttpGet("Index")] + public void Index() { } + + [HttpGet("Edit")] + public void Edit() { } + + public void AnotherNonAttributedAction() { } + } + + [MyRouteConstraint(blockNonAttributedActions: true)] + [MySecondRouteConstraint(blockNonAttributedActions: true)] + private class ConstrainedController + { + public void ConstrainedNonAttributedAction() { } + } } }