Skip to content
This repository has been archived by the owner on Dec 14, 2018. It is now read-only.

Commit

Permalink
[Fixes #1035] RouteGroupConstraint should only be added once for non
Browse files Browse the repository at this point in the history 
attribute routed actions

1. Changed ReflectedActionDescriptorProvider to add RouteGroupConstraint only once
   for non attribute routed actions.

2. Added tests to cover the scenario.
  • Loading branch information
@javiercn
javiercn committed Aug 19, 2014
1 parent 64d797a commit b4bd2e4
Show file tree
Hide file tree
Showing 2 changed files with 144 additions and 34 deletions.
59 changes: 31 additions & 28 deletions src/Microsoft.AspNet.Mvc.Core/ReflectedActionDescriptorProvider.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -289,41 +289,44 @@ public List<ReflectedActionDescriptor> Build(ReflectedApplicationModel model)

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);
}
}
}
Expand Down
119 changes: 113 additions & 6 deletions test/Microsoft.AspNet.Mvc.Core.Test/ReflectedActionDescriptorProviderTests.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -84,16 +84,16 @@ public void GetDescriptors_WithRouteDataConstraint_WithBlockNonAttributedActions
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
Expand Down Expand Up @@ -265,6 +265,67 @@ public void AttributeRouting_TokenReplacement_ThrowsWithMultipleMessages()
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());

var nonAttributeRoutedAction = Assert.Single(actionDescriptors, ad => ad.Name.Equals("AnotherNonAttributedAction"));

This comment has been minimized.

Sign in to view

Copy link
@rynowak

rynowak Aug 19, 2014

Member

why not check all of them? the test doesn't do what the name suggests
Assert.Equal(5, nonAttributeRoutedAction.RouteConstraints.Count);

var routeGroupConstraint = Assert.Single(nonAttributeRoutedAction.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);

This comment has been minimized.

Sign in to view

Copy link
@rynowak

rynowak Aug 19, 2014

Member

This is going to change every time we change the code that generates this string. Can you think of a better way or just leave it out?
Assert.Equal("__ROUTE__0-INDEX", routeGroupConstraint.RouteValue);

Assert.Equal(4, 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 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()
{
Expand Down Expand Up @@ -318,7 +379,7 @@ public void AttributeRouting_DoesNotValidateParameters()
}

private ReflectedActionDescriptorProvider GetProvider(
TypeInfo controllerTypeInfo,
TypeInfo controllerTypeInfo,
IEnumerable<IFilter> filters = null)
{
var conventions = new StaticActionDiscoveryConventions(controllerTypeInfo);
Expand All @@ -338,6 +399,26 @@ public void AttributeRouting_DoesNotValidateParameters()
return provider;
}

private ReflectedActionDescriptorProvider GetProvider(
params TypeInfo[] controllerTypeInfo)
{
var conventions = new StaticActionDiscoveryConventions(controllerTypeInfo);

var assemblyProvider = new Mock<IControllerAssemblyProvider>();
assemblyProvider
.SetupGet(ap => ap.CandidateAssemblies)
.Returns(new Assembly[] { controllerTypeInfo.First().Assembly });

var provider = new ReflectedActionDescriptorProvider(
assemblyProvider.Object,
conventions,
Enumerable.Empty<IFilter>(),
new MockMvcOptionsAccessor(),
Mock.Of<IInlineConstraintResolver>());

return provider;
}

private IEnumerable<ActionDescriptor> GetDescriptors(params TypeInfo[] controllerTypeInfos)
{
var conventions = new StaticActionDiscoveryConventions(controllerTypeInfos);
Expand All @@ -353,7 +434,7 @@ private IEnumerable<ActionDescriptor> GetDescriptors(params TypeInfo[] controlle
null,
new MockMvcOptionsAccessor(),
null);

return provider.GetDescriptors();
}

Expand Down Expand Up @@ -386,6 +467,14 @@ public MyRouteConstraintAttribute(bool blockNonAttributedActions)
}
}

public class MySecondRouteConstraintAttribute : RouteConstraintAttribute
{
public MySecondRouteConstraintAttribute(bool blockNonAttributedActions)
: base("second", "value", blockNonAttributedActions)
{
}
}

This comment has been minimized.

Sign in to view

Copy link
@rynowak

rynowak Aug 19, 2014

Member

Would be good to include a test for an attribute routed action in an area here. That's in the cross section of constraints + already has value.
[MyRouteConstraintAttribute(blockNonAttributedActions: true)]
private class BlockNonAttributedActionsController
{
Expand Down Expand Up @@ -458,5 +547,23 @@ private class SameGroupIdController
[HttpGet("stub/Action1")]
public void Action2() { }
}

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

0 comments on commit b4bd2e4

Please sign in to comment.