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

Commit

Permalink
Fix for #1194 - Error using [HttpPost] and [Route] together
Browse files Browse the repository at this point in the history 
This change enables some compatibility scenarios with MVC 5 by expanding
the set of legal ways to configure attribute routing. Most promiently, the
following example is now legal:

[HttpPost]
[Route("Products")]
public void MyAction() { }

This will define a single action that accepts POST on route "Products".

See the comments in #1194 for a more detailed description of what changed
with more examples.
  • Loading branch information
@rynowak
rynowak committed Nov 19, 2014
1 parent e21f157 commit ed8ba5a
Show file tree
Hide file tree
Showing 8 changed files with 390 additions and 242 deletions.
76 changes: 60 additions & 16 deletions src/Microsoft.AspNet.Mvc.Core/ApplicationModels/DefaultActionModelBuilder.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -34,60 +34,99 @@ public IEnumerable<ActionModel> BuildActionModels([NotNull] TypeInfo typeInfo, [
// The set of route attributes are split into those that 'define' a route versus those that are
// 'silent'.
//
// We need to define from action for each attribute that 'defines' a route, and a single action
// We need to define an action for each attribute that 'defines' a route, and a single action
// for all of the ones that don't (if any exist).
//
// If the attribute that 'defines' a route is NOT an IActionHttpMethodProvider, then we'll include with
// it, any IActionHttpMethodProvider that are 'silent' IRouteTemplateProviders. In this case the 'extra'
// action for silent route providers isn't needed.
//
// Ex:
// [HttpGet]
// [AcceptVerbs("POST", "PUT")]
// [Route("Api/Things")]
// [HttpPost("Api/Things")]
// public void DoThing()
//
// This will generate 2 actions:
// 1. [Route("Api/Things")]
// 1. [HttpPost("Api/Things")]
// 2. [HttpGet], [AcceptVerbs("POST", "PUT")]
//
// Note that having a route attribute that doesn't define a route template _might_ be an error. We
// don't have enough context to really know at this point so we just pass it on.
var splitAttributes = new List<object>();

var hasSilentRouteAttribute = false;
var routeProviders = new List<object>();
var createActionForSilentRouteProviders = false;
foreach (var attribute in attributes)
{
var routeTemplateProvider = attribute as IRouteTemplateProvider;
if (routeTemplateProvider != null)
{
if (IsSilentRouteAttribute(routeTemplateProvider))
{
hasSilentRouteAttribute = true;
createActionForSilentRouteProviders = true;
}
else
{
splitAttributes.Add(attribute);
routeProviders.Add(attribute);
}
}
}

foreach (var routeProvider in routeProviders)
{
// If we see an attribute like
// [Route(...)]
//
// Then we want to group any attributes like [HttpGet] with it.
//
// Basically...
//
// [HttpGet]
// [HttpPost("Products")]
// public void Foo() { }
//
// Is two actions. And...
//
// [HttpGet]
// [Route("Products")]
// public void Foo() { }
//
// Is one action.
if (!(routeProvider is IActionHttpMethodProvider))
{
createActionForSilentRouteProviders = false;
}
}

var actionModels = new List<ActionModel>();
if (splitAttributes.Count == 0 && !hasSilentRouteAttribute)
if (routeProviders.Count == 0 && !createActionForSilentRouteProviders)
{
actionModels.Add(CreateActionModel(methodInfo, attributes));
}
else
{
foreach (var splitAttribute in splitAttributes)
// Each of these routeProviders are the ones that actually have routing information on them
// something like [HttpGet] won't show up here, but [HttpGet("Products")] will.
foreach (var routeProvider in routeProviders)
{
var filteredAttributes = new List<object>();
foreach (var attribute in attributes)
{
if (attribute == splitAttribute)
if (attribute == routeProvider)
{
filteredAttributes.Add(attribute);
}
else if (attribute is IRouteTemplateProvider)
else if (routeProviders.Contains(attribute))
{
// Exclude other route template providers
}
else if (
routeProvider is IActionHttpMethodProvider &&
attribute is IActionHttpMethodProvider)
{
// Exclude other http method providers if this route is an
// http method provider.
}
else
{
filteredAttributes.Add(attribute);
Expand All @@ -97,12 +136,12 @@ public IEnumerable<ActionModel> BuildActionModels([NotNull] TypeInfo typeInfo, [
actionModels.Add(CreateActionModel(methodInfo, filteredAttributes));
}

if (hasSilentRouteAttribute)
if (createActionForSilentRouteProviders)
{
var filteredAttributes = new List<object>();
foreach (var attribute in attributes)
{
if (!splitAttributes.Contains(attribute))
if (!routeProviders.Contains(attribute))
{
filteredAttributes.Add(attribute);
}
Expand Down Expand Up @@ -250,8 +289,13 @@ private bool IsIDisposableMethod(MethodInfo methodInfo, TypeInfo typeInfo)
.SelectMany(a => a.HttpMethods)
.Distinct());

var routeTemplateProvider = attributes.OfType<IRouteTemplateProvider>().FirstOrDefault();
if (routeTemplateProvider != null && !IsSilentRouteAttribute(routeTemplateProvider))
var routeTemplateProvider =
attributes
.OfType<IRouteTemplateProvider>()
.Where(a => !IsSilentRouteAttribute(a))
.SingleOrDefault();

if (routeTemplateProvider != null)
{
actionModel.AttributeRouteModel = new AttributeRouteModel(routeTemplateProvider);
}
Expand Down
156 changes: 29 additions & 127 deletions src/Microsoft.AspNet.Mvc.Core/ControllerActionDescriptorBuilder.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -592,13 +592,9 @@ private static bool IsAttributeRoutedAction(ControllerActionDescriptor actionDes
ControllerActionDescriptor actionDescriptor,
IDictionary<MethodInfo, string> routingConfigurationErrors)
{
string combinedErrorMessage = null;

var hasAttributeRoutedActions = false;
var hasConventionallyRoutedActions = false;

var invalidHttpMethodActions = new Dictionary<ActionModel, IEnumerable<string>>();

var actionsForMethod = methodMap[actionDescriptor.MethodInfo];
foreach (var reflectedAction in actionsForMethod)
{
Expand All @@ -613,132 +609,24 @@ private static bool IsAttributeRoutedAction(ControllerActionDescriptor actionDes
hasConventionallyRoutedActions = true;
}
}

// Keep a list of actions with possible invalid IHttpActionMethodProvider attributes
// to generate an error in case the method generates attribute routed actions.
ValidateActionHttpMethodProviders(reflectedAction.Key, invalidHttpMethodActions);
}

// Validate that no method result in attribute and non attribute actions at the same time.
// By design, mixing attribute and conventionally actions in the same method is not allowed.
// This is for example the case when someone uses[HttpGet("Products")] and[HttpPost]
// on the same method.
//
// This for example:
//
// [HttpGet]
// [HttpPost("Foo")]
// public void Foo() { }
if (hasAttributeRoutedActions && hasConventionallyRoutedActions)
{
combinedErrorMessage = CreateMixedRoutedActionDescriptorsErrorMessage(
actionDescriptor,
actionsForMethod);
}

// Validate that no method that creates attribute routed actions and
// also uses attributes that only constrain the set of HTTP methods. For example,
// if an attribute that implements IActionHttpMethodProvider but does not implement
// IRouteTemplateProvider is used with an attribute that implements IRouteTemplateProvider on
// the same action, the HTTP methods provided by the attribute that only implements
// IActionHttpMethodProvider would be silently ignored, so we choose to throw to
// inform the user of the invalid configuration.
if (hasAttributeRoutedActions && invalidHttpMethodActions.Any())
{
var errorMessage = CreateInvalidActionHttpMethodProviderErrorMessage(
var message = CreateMixedRoutedActionDescriptorsErrorMessage(
actionDescriptor,
invalidHttpMethodActions,
actionsForMethod);

combinedErrorMessage = CombineErrorMessage(combinedErrorMessage, errorMessage);
}

if (combinedErrorMessage != null)
{
routingConfigurationErrors.Add(actionDescriptor.MethodInfo, combinedErrorMessage);
}
}

private static void ValidateActionHttpMethodProviders(
ActionModel reflectedAction,
IDictionary<ActionModel, IEnumerable<string>> invalidHttpMethodActions)
{
var invalidHttpMethodProviderAttributes = reflectedAction.Attributes
.Where(attr => attr is IActionHttpMethodProvider &&
!(attr is IRouteTemplateProvider))
.Select(attr => attr.GetType().FullName);

if (invalidHttpMethodProviderAttributes.Any())
{
invalidHttpMethodActions.Add(
reflectedAction,
invalidHttpMethodProviderAttributes);
}
}

private static string CombineErrorMessage(string combinedErrorMessage, string errorMessage)
{
if (combinedErrorMessage == null)
{
combinedErrorMessage = errorMessage;
routingConfigurationErrors.Add(actionDescriptor.MethodInfo, message);
}
else
{
combinedErrorMessage = string.Join(
Environment.NewLine,
combinedErrorMessage,
errorMessage);
}

return combinedErrorMessage;
}

private static string CreateInvalidActionHttpMethodProviderErrorMessage(
ControllerActionDescriptor actionDescriptor,
IDictionary<ActionModel, IEnumerable<string>> invalidHttpMethodActions,
IDictionary<ActionModel, IList<ControllerActionDescriptor>> actionsForMethod)
{
var messagesForMethodInfo = new List<string>();
foreach (var invalidAction in invalidHttpMethodActions)
{
var invalidAttributesList = string.Join(", ", invalidAction.Value);

foreach (var descriptor in actionsForMethod[invalidAction.Key])
{
// We only report errors in attribute routed actions. For example, an action
// that contains [HttpGet("Products")], [HttpPost] and [HttpHead], where [HttpHead]
// only implements IHttpActionMethodProvider and restricts the action to only allow
// the head method, will report that the action contains invalid IActionHttpMethodProvider
// attributes only for the action generated by [HttpGet("Products")].
// [HttpPost] will be treated as an action that produces a conventionally routed action
// and the fact that the method generates attribute and non attributed actions will be
// reported as a different error.
if (IsAttributeRoutedAction(descriptor))
{
var messageItem = Resources.FormatAttributeRoute_InvalidHttpConstraints_Item(
descriptor.DisplayName,
descriptor.AttributeRouteInfo.Template,
invalidAttributesList,
typeof(IActionHttpMethodProvider).FullName);

messagesForMethodInfo.Add(messageItem);
}
}
}

var methodFullName = string.Format(
CultureInfo.InvariantCulture,
"{0}.{1}",
actionDescriptor.MethodInfo.DeclaringType.FullName,
actionDescriptor.MethodInfo.Name);

// Sample message:
// A method 'MyApplication.CustomerController.Index' that defines attribute routed actions must
// not have attributes that implement 'Microsoft.AspNet.Mvc.IActionHttpMethodProvider'
// and do not implement 'Microsoft.AspNet.Mvc.Routing.IRouteTemplateProvider':
// Action 'MyApplication.CustomerController.Index' has 'Namespace.CustomHttpMethodAttribute'
// invalid 'Microsoft.AspNet.Mvc.IActionHttpMethodProvider' attributes.
return
Resources.FormatAttributeRoute_InvalidHttpConstraints(
methodFullName,
typeof(IActionHttpMethodProvider).FullName,
typeof(IRouteTemplateProvider).FullName,
Environment.NewLine,
string.Join(Environment.NewLine, messagesForMethodInfo));
}

private static string CreateMixedRoutedActionDescriptorsErrorMessage(
Expand All @@ -748,12 +636,22 @@ private static string CombineErrorMessage(string combinedErrorMessage, string er
// Text to show as the attribute route template for conventionally routed actions.
var nullTemplate = Resources.AttributeRoute_NullTemplateRepresentation;

var actionDescriptions = actionsForMethod
.SelectMany(a => a.Value)
.Select(ad =>
var actionDescriptions = new List<string>();
foreach (var action in actionsForMethod.SelectMany(kvp => kvp.Value))
{
var routeTemplate = action.AttributeRouteInfo?.Template ?? nullTemplate;

var verbs = action.ActionConstraints.OfType<HttpMethodConstraint>().FirstOrDefault()?.HttpMethods;
var formattedVerbs = string.Join(", ", verbs.OrderBy(v => v, StringComparer.Ordinal));

var description =
Resources.FormatAttributeRoute_MixedAttributeAndConventionallyRoutedActions_ForMethod_Item(
ad.DisplayName,
ad.AttributeRouteInfo != null ? ad.AttributeRouteInfo.Template : nullTemplate));
action.DisplayName,
routeTemplate,
formattedVerbs);

actionDescriptions.Add(description);
}

var methodFullName = string.Format(
CultureInfo.InvariantCulture,
Expand All @@ -762,10 +660,14 @@ private static string CombineErrorMessage(string combinedErrorMessage, string er
actionDescriptor.MethodInfo.Name);

// Sample error message:
//
// A method 'MyApplication.CustomerController.Index' must not define attributed actions and
// non attributed actions at the same time:
// Action: 'MyApplication.CustomerController.Index' - Template: 'Products'
// Action: 'MyApplication.CustomerController.Index' - Template: '(none)'
// Action: 'MyApplication.CustomerController.Index' - Route Template: 'Products' - HTTP Verbs: 'PUT'
// Action: 'MyApplication.CustomerController.Index' - Route Template: '(none)' - HTTP Verbs: 'POST'
//
// Use 'AcceptVerbsAttribute' to create a single route that allows multiple HTTP verbs and defines a route,
// or set a route template in all attributes that constrain HTTP verbs.
return
Resources.FormatAttributeRoute_MixedAttributeAndConventionallyRoutedActions_ForMethod(
methodFullName,
Expand Down

0 comments on commit ed8ba5a

Please sign in to comment.