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() { } + } } }