From ed8ba5ae9c14221b2f0f220e62051c2d72006f71 Mon Sep 17 00:00:00 2001 From: Ryan Nowak Date: Fri, 14 Nov 2014 10:55:03 -0800 Subject: [PATCH] Fix for #1194 - Error using [HttpPost] and [Route] together 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. --- .../DefaultActionModelBuilder.cs | 76 +++++++-- .../ControllerActionDescriptorBuilder.cs | 158 ++++-------------- .../Properties/Resources.Designer.cs | 44 +---- src/Microsoft.AspNet.Mvc.Core/Resources.resx | 12 +- .../DefaultActionModelBuilderTest.cs | 119 +++++++++++++ ...ControllerActionDescriptorProviderTests.cs | 156 +++++++++++------ .../RoutingTests.cs | 48 ++++++ .../Controllers/BanksController.cs | 21 ++- 8 files changed, 391 insertions(+), 243 deletions(-) diff --git a/src/Microsoft.AspNet.Mvc.Core/ApplicationModels/DefaultActionModelBuilder.cs b/src/Microsoft.AspNet.Mvc.Core/ApplicationModels/DefaultActionModelBuilder.cs index ec69dab2a3..62c209a589 100644 --- a/src/Microsoft.AspNet.Mvc.Core/ApplicationModels/DefaultActionModelBuilder.cs +++ b/src/Microsoft.AspNet.Mvc.Core/ApplicationModels/DefaultActionModelBuilder.cs @@ -34,24 +34,28 @@ namespace Microsoft.AspNet.Mvc.ApplicationModels // 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(); - - var hasSilentRouteAttribute = false; + var routeProviders = new List(); + + var createActionForSilentRouteProviders = false; foreach (var attribute in attributes) { var routeTemplateProvider = attribute as IRouteTemplateProvider; @@ -59,35 +63,70 @@ namespace Microsoft.AspNet.Mvc.ApplicationModels { 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(); - 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(); 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); @@ -97,12 +136,12 @@ namespace Microsoft.AspNet.Mvc.ApplicationModels actionModels.Add(CreateActionModel(methodInfo, filteredAttributes)); } - if (hasSilentRouteAttribute) + if (createActionForSilentRouteProviders) { var filteredAttributes = new List(); foreach (var attribute in attributes) { - if (!splitAttributes.Contains(attribute)) + if (!routeProviders.Contains(attribute)) { filteredAttributes.Add(attribute); } @@ -250,8 +289,13 @@ namespace Microsoft.AspNet.Mvc.ApplicationModels .SelectMany(a => a.HttpMethods) .Distinct()); - var routeTemplateProvider = attributes.OfType().FirstOrDefault(); - if (routeTemplateProvider != null && !IsSilentRouteAttribute(routeTemplateProvider)) + var routeTemplateProvider = + attributes + .OfType() + .Where(a => !IsSilentRouteAttribute(a)) + .SingleOrDefault(); + + if (routeTemplateProvider != null) { actionModel.AttributeRouteModel = new AttributeRouteModel(routeTemplateProvider); } diff --git a/src/Microsoft.AspNet.Mvc.Core/ControllerActionDescriptorBuilder.cs b/src/Microsoft.AspNet.Mvc.Core/ControllerActionDescriptorBuilder.cs index 7ec79baad5..160d244174 100644 --- a/src/Microsoft.AspNet.Mvc.Core/ControllerActionDescriptorBuilder.cs +++ b/src/Microsoft.AspNet.Mvc.Core/ControllerActionDescriptorBuilder.cs @@ -592,13 +592,9 @@ namespace Microsoft.AspNet.Mvc ControllerActionDescriptor actionDescriptor, IDictionary routingConfigurationErrors) { - string combinedErrorMessage = null; - var hasAttributeRoutedActions = false; var hasConventionallyRoutedActions = false; - var invalidHttpMethodActions = new Dictionary>(); - var actionsForMethod = methodMap[actionDescriptor.MethodInfo]; foreach (var reflectedAction in actionsForMethod) { @@ -613,132 +609,24 @@ namespace Microsoft.AspNet.Mvc 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( + var message = CreateMixedRoutedActionDescriptorsErrorMessage( actionDescriptor, actionsForMethod); + + routingConfigurationErrors.Add(actionDescriptor.MethodInfo, message); } - - // 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( - actionDescriptor, - invalidHttpMethodActions, - actionsForMethod); - - combinedErrorMessage = CombineErrorMessage(combinedErrorMessage, errorMessage); - } - - if (combinedErrorMessage != null) - { - routingConfigurationErrors.Add(actionDescriptor.MethodInfo, combinedErrorMessage); - } - } - - private static void ValidateActionHttpMethodProviders( - ActionModel reflectedAction, - IDictionary> 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; - } - else - { - combinedErrorMessage = string.Join( - Environment.NewLine, - combinedErrorMessage, - errorMessage); - } - - return combinedErrorMessage; - } - - private static string CreateInvalidActionHttpMethodProviderErrorMessage( - ControllerActionDescriptor actionDescriptor, - IDictionary> invalidHttpMethodActions, - IDictionary> actionsForMethod) - { - var messagesForMethodInfo = new List(); - 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( @@ -748,12 +636,22 @@ namespace Microsoft.AspNet.Mvc // 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(); + foreach (var action in actionsForMethod.SelectMany(kvp => kvp.Value)) + { + var routeTemplate = action.AttributeRouteInfo?.Template ?? nullTemplate; + + var verbs = action.ActionConstraints.OfType().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, @@ -762,10 +660,14 @@ namespace Microsoft.AspNet.Mvc 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, diff --git a/src/Microsoft.AspNet.Mvc.Core/Properties/Resources.Designer.cs b/src/Microsoft.AspNet.Mvc.Core/Properties/Resources.Designer.cs index bb9de0d044..1f8dbdbdd2 100644 --- a/src/Microsoft.AspNet.Mvc.Core/Properties/Resources.Designer.cs +++ b/src/Microsoft.AspNet.Mvc.Core/Properties/Resources.Designer.cs @@ -1419,39 +1419,7 @@ namespace Microsoft.AspNet.Mvc.Core } /// - /// A method '{0}' that defines attribute routed actions must not have attributes that implement '{1}' and do not implement '{2}':{3}{4} - /// - internal static string AttributeRoute_InvalidHttpConstraints - { - get { return GetString("AttributeRoute_InvalidHttpConstraints"); } - } - - /// - /// A method '{0}' that defines attribute routed actions must not have attributes that implement '{1}' and do not implement '{2}':{3}{4} - /// - internal static string FormatAttributeRoute_InvalidHttpConstraints(object p0, object p1, object p2, object p3, object p4) - { - return string.Format(CultureInfo.CurrentCulture, GetString("AttributeRoute_InvalidHttpConstraints"), p0, p1, p2, p3, p4); - } - - /// - /// Action '{0}' with route template '{1}' has '{2}' invalid '{3}' attributes. - /// - internal static string AttributeRoute_InvalidHttpConstraints_Item - { - get { return GetString("AttributeRoute_InvalidHttpConstraints_Item"); } - } - - /// - /// Action '{0}' with route template '{1}' has '{2}' invalid '{3}' attributes. - /// - internal static string FormatAttributeRoute_InvalidHttpConstraints_Item(object p0, object p1, object p2, object p3) - { - return string.Format(CultureInfo.CurrentCulture, GetString("AttributeRoute_InvalidHttpConstraints_Item"), p0, p1, p2, p3); - } - - /// - /// A method '{0}' must not define attribute routed actions and non attribute routed actions at the same time:{1}{2} + /// A method '{0}' must not define attribute routed actions and non attribute routed actions at the same time:{1}{2}{1}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. /// internal static string AttributeRoute_MixedAttributeAndConventionallyRoutedActions_ForMethod { @@ -1459,7 +1427,7 @@ namespace Microsoft.AspNet.Mvc.Core } /// - /// A method '{0}' must not define attribute routed actions and non attribute routed actions at the same time:{1}{2} + /// A method '{0}' must not define attribute routed actions and non attribute routed actions at the same time:{1}{2}{1}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. /// internal static string FormatAttributeRoute_MixedAttributeAndConventionallyRoutedActions_ForMethod(object p0, object p1, object p2) { @@ -1467,7 +1435,7 @@ namespace Microsoft.AspNet.Mvc.Core } /// - /// Action: '{0}' - Template: '{1}' + /// Action: '{0}' - Route Template: '{1}' - HTTP Verbs: '{2}' /// internal static string AttributeRoute_MixedAttributeAndConventionallyRoutedActions_ForMethod_Item { @@ -1475,11 +1443,11 @@ namespace Microsoft.AspNet.Mvc.Core } /// - /// Action: '{0}' - Template: '{1}' + /// Action: '{0}' - Route Template: '{1}' - HTTP Verbs: '{2}' /// - internal static string FormatAttributeRoute_MixedAttributeAndConventionallyRoutedActions_ForMethod_Item(object p0, object p1) + internal static string FormatAttributeRoute_MixedAttributeAndConventionallyRoutedActions_ForMethod_Item(object p0, object p1, object p2) { - return string.Format(CultureInfo.CurrentCulture, GetString("AttributeRoute_MixedAttributeAndConventionallyRoutedActions_ForMethod_Item"), p0, p1); + return string.Format(CultureInfo.CurrentCulture, GetString("AttributeRoute_MixedAttributeAndConventionallyRoutedActions_ForMethod_Item"), p0, p1, p2); } /// diff --git a/src/Microsoft.AspNet.Mvc.Core/Resources.resx b/src/Microsoft.AspNet.Mvc.Core/Resources.resx index 4714a8d58e..ffc734af90 100644 --- a/src/Microsoft.AspNet.Mvc.Core/Resources.resx +++ b/src/Microsoft.AspNet.Mvc.Core/Resources.resx @@ -388,20 +388,12 @@ The result of value factory cannot be null. - - A method '{0}' that defines attribute routed actions must not have attributes that implement '{1}' and do not implement '{2}':{3}{4} - {0} is the MethodInfo.FullName, {1} is typeof(IActionHttpMethodProvider).FullName, {2} is typeof(IRouteTemplateProvider).FullName, {3} is Environment.NewLine, {4} is the list of actions and their respective invalid IActionHttpMethodProvider attributes formatted using AttributeRoute_InvalidHttpMethodConstraints_Item - - - Action '{0}' with route template '{1}' has '{2}' invalid '{3}' attributes. - {0} The display name of the action, {1} the route template, {2} the formatted list of invalid attributes using string.Join(", ", attributes), {3} is typeof(IActionHttpMethodProvider).FullName. - - A method '{0}' must not define attribute routed actions and non attribute routed actions at the same time:{1}{2} + A method '{0}' must not define attribute routed actions and non attribute routed actions at the same time:{1}{2}{1}{1}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. {0} is the MethodInfo.FullName, {1} is Environment.NewLine, {2} is the formatted list of actions defined by that method info. - Action: '{0}' - Template: '{1}' + Action: '{0}' - Route Template: '{1}' - HTTP Verbs: '{2}' (none) diff --git a/test/Microsoft.AspNet.Mvc.Core.Test/ApplicationModel/DefaultActionModelBuilderTest.cs b/test/Microsoft.AspNet.Mvc.Core.Test/ApplicationModel/DefaultActionModelBuilderTest.cs index 16facdff4a..a53f487e27 100644 --- a/test/Microsoft.AspNet.Mvc.Core.Test/ApplicationModel/DefaultActionModelBuilderTest.cs +++ b/test/Microsoft.AspNet.Mvc.Core.Test/ApplicationModel/DefaultActionModelBuilderTest.cs @@ -576,6 +576,89 @@ namespace Microsoft.AspNet.Mvc.ApplicationModels Assert.Single(actions, ai => ai.AttributeRouteModel.Template.Equals("All")); } + [Fact] + public void GetActions_MixedHttpVerbsAndRoutes_EmptyVerbWithRoute() + { + // Arrange + var builder = new DefaultActionModelBuilder(); + var typeInfo = typeof(MixedHttpVerbsAndRouteAttributeController).GetTypeInfo(); + var actionName = nameof(MixedHttpVerbsAndRouteAttributeController.VerbAndRoute); + + // Act + var actions = builder.BuildActionModels(typeInfo, typeInfo.GetMethod(actionName)); + + // Assert + var action = Assert.Single(actions); + Assert.Equal(new string[] { "GET" }, action.HttpMethods); + Assert.Equal("Products", action.AttributeRouteModel.Template); + } + + [Fact] + public void GetActions_MixedHttpVerbsAndRoutes_MultipleEmptyVerbsWithMultipleRoutes() + { + // Arrange + var builder = new DefaultActionModelBuilder(); + var typeInfo = typeof(MixedHttpVerbsAndRouteAttributeController).GetTypeInfo(); + var actionName = nameof(MixedHttpVerbsAndRouteAttributeController.MultipleVerbsAndRoutes); + + // Act + var actions = builder.BuildActionModels(typeInfo, typeInfo.GetMethod(actionName)); + + // Assert + Assert.Equal(2, actions.Count()); + + var action = Assert.Single(actions, a => a.AttributeRouteModel.Template == "Products"); + Assert.Equal(new string[] { "GET", "POST" }, action.HttpMethods); + + action = Assert.Single(actions, a => a.AttributeRouteModel.Template == "v2/Products"); + Assert.Equal(new string[] { "GET", "POST" }, action.HttpMethods); + } + + [Fact] + public void GetActions_MixedHttpVerbsAndRoutes_MultipleEmptyAndNonEmptyVerbsWithMultipleRoutes() + { + // Arrange + var builder = new DefaultActionModelBuilder(); + var typeInfo = typeof(MixedHttpVerbsAndRouteAttributeController).GetTypeInfo(); + var actionName = nameof(MixedHttpVerbsAndRouteAttributeController.MultipleVerbsWithAnyWithoutTemplateAndRoutes); + + // Act + var actions = builder.BuildActionModels(typeInfo, typeInfo.GetMethod(actionName)); + + // Assert + Assert.Equal(3, actions.Count()); + + var action = Assert.Single(actions, a => a.AttributeRouteModel.Template == "Products"); + Assert.Equal(new string[] { "GET" }, action.HttpMethods); + + action = Assert.Single(actions, a => a.AttributeRouteModel.Template == "v2/Products"); + Assert.Equal(new string[] { "GET" }, action.HttpMethods); + + action = Assert.Single(actions, a => a.AttributeRouteModel.Template == "Products/Buy"); + Assert.Equal(new string[] { "POST" }, action.HttpMethods); + } + + [Fact] + public void GetActions_MixedHttpVerbsAndRoutes_MultipleEmptyAndNonEmptyVerbs() + { + // Arrange + var builder = new DefaultActionModelBuilder(); + var typeInfo = typeof(MixedHttpVerbsAndRouteAttributeController).GetTypeInfo(); + var actionName = nameof(MixedHttpVerbsAndRouteAttributeController.Invalid); + + // Act + var actions = builder.BuildActionModels(typeInfo, typeInfo.GetMethod(actionName)); + + // Assert + Assert.Equal(2, actions.Count()); + + var action = Assert.Single(actions, a => a.AttributeRouteModel?.Template == "Products"); + Assert.Equal(new string[] { "POST" }, action.HttpMethods); + + action = Assert.Single(actions, a => a.AttributeRouteModel?.Template == null); + Assert.Equal(new string[] { "GET" }, action.HttpMethods); + } + private class AccessibleActionModelBuilder : DefaultActionModelBuilder { public new bool IsAction([NotNull] TypeInfo typeInfo, [NotNull]MethodInfo methodInfo) @@ -765,6 +848,42 @@ namespace Microsoft.AspNet.Mvc.ApplicationModels public void Delete() { } } + private class MixedHttpVerbsAndRouteAttributeController : Controller + { + // Should produce a single action constrained to GET + [HttpGet] + [Route("Products")] + public void VerbAndRoute() { } + + // Should produce two actions constrained to GET,POST + [HttpGet] + [HttpPost] + [Route("Products")] + [Route("v2/Products")] + public void MultipleVerbsAndRoutes() { } + + // Produces: + // + // Products - GET + // v2/Products - GET + // Products/Buy - POST + [HttpGet] + [Route("Products")] + [Route("v2/Products")] + [HttpPost("Products/Buy")] + public void MultipleVerbsWithAnyWithoutTemplateAndRoutes() { } + + // Produces: + // + // (no route) - GET + // Products - POST + // + // This is invalid, and will throw during the ADP construction phase. + [HttpGet] + [HttpPost("Products")] + public void Invalid() { } + } + // Here the constraints on the methods are acting as an IActionHttpMethodProvider and // not as an IRouteTemplateProvider given that there is no RouteAttribute // on the controller and the template for all the constraints on a method is null. diff --git a/test/Microsoft.AspNet.Mvc.Core.Test/ControllerActionDescriptorProviderTests.cs b/test/Microsoft.AspNet.Mvc.Core.Test/ControllerActionDescriptorProviderTests.cs index e4af25eb31..c3aefaba87 100644 --- a/test/Microsoft.AspNet.Mvc.Core.Test/ControllerActionDescriptorProviderTests.cs +++ b/test/Microsoft.AspNet.Mvc.Core.Test/ControllerActionDescriptorProviderTests.cs @@ -78,37 +78,20 @@ namespace Microsoft.AspNet.Mvc.Test } [Fact] - public void GetDescriptors_ThrowsIfHttpMethodConstraints_OnAttributeRoutedActions() + public void GetDescriptors_HttpMethodConstraint_RouteOnController() { // Arrange - var expectedExceptionMessage = - "The following errors occurred with attribute routing information:" + Environment.NewLine + - Environment.NewLine + - "Error 1:" + Environment.NewLine + - "A method 'Microsoft.AspNet.Mvc.Test.ControllerActionDescriptorProviderTests+" + - "AttributeRoutedHttpMethodController.PutOrPatch'" + - " that defines attribute routed actions must not have attributes that implement " + - "'Microsoft.AspNet.Mvc.IActionHttpMethodProvider' and do not implement " + - "'Microsoft.AspNet.Mvc.Routing.IRouteTemplateProvider':" + Environment.NewLine + - "Action 'Microsoft.AspNet.Mvc.Test.ControllerActionDescriptorProviderTests+" + - "AttributeRoutedHttpMethodController.PutOrPatch' with route template 'Products' has " + - "'Microsoft.AspNet.Mvc.Test.ControllerActionDescriptorProviderTests+CustomHttpMethodConstraintAttribute'" + - " invalid 'Microsoft.AspNet.Mvc.IActionHttpMethodProvider' attributes." + Environment.NewLine + - "Action 'Microsoft.AspNet.Mvc.Test.ControllerActionDescriptorProviderTests+" + - "AttributeRoutedHttpMethodController.PutOrPatch' with route template 'Items' has " + - "'Microsoft.AspNet.Mvc.Test.ControllerActionDescriptorProviderTests+CustomHttpMethodConstraintAttribute'" + - " invalid 'Microsoft.AspNet.Mvc.IActionHttpMethodProvider' attributes."; - - var provider = GetProvider( - typeof(AttributeRoutedHttpMethodController) - .GetTypeInfo()); + var provider = GetProvider(typeof(AttributeRoutedHttpMethodController).GetTypeInfo()); // Act - var ex = Assert.Throws( - () => provider.GetDescriptors()); + var descriptors = provider.GetDescriptors(); + var descriptor = Assert.Single(descriptors); - // Act - VerifyMultiLineError(expectedExceptionMessage, ex.Message); + // Assert + Assert.Equal("Items", descriptor.AttributeRouteInfo.Template); + + var constraint = Assert.IsType(Assert.Single(descriptor.ActionConstraints)); + Assert.Equal(new string[] { "PUT", "PATCH" }, constraint.HttpMethods); } [Fact] @@ -575,7 +558,7 @@ namespace Microsoft.AspNet.Mvc.Test var ex = Assert.Throws(() => { provider.GetDescriptors(); }); // Assert - VerifyMultiLineError(expectedMessage, ex.Message); + VerifyMultiLineError(expectedMessage, ex.Message, unorderedStart: 2, unorderedLineCount: 6); } [Fact] @@ -656,7 +639,7 @@ namespace Microsoft.AspNet.Mvc.Test } [Fact] - public void AttributeRouting_AcceptVerbsOnAction_DoesNotApplyHttpMethods_ToOtherAttributeRoutes() + public void AttributeRouting_AcceptVerbsOnAction_WithoutTemplate_MergesVerb() { // Arrange var provider = GetProvider(typeof(MultiRouteAttributesController).GetTypeInfo()); @@ -666,6 +649,45 @@ namespace Microsoft.AspNet.Mvc.Test // Assert var actions = descriptors.Where(d => d.Name == "AcceptVerbsRouteAttributeAndHttpPut"); + Assert.Equal(4, actions.Count()); + + foreach (var action in actions) + { + Assert.Equal("MultiRouteAttributes", action.ControllerName); + + Assert.NotNull(action.AttributeRouteInfo); + Assert.NotNull(action.AttributeRouteInfo.Template); + } + + var constrainedActions = actions.Where(a => a.ActionConstraints != null); + Assert.Equal(4, constrainedActions.Count()); + + // Actions generated by PutAttribute + var putActions = constrainedActions.Where( + a => a.ActionConstraints.OfType().Single().HttpMethods.Single() == "PUT"); + Assert.Equal(2, putActions.Count()); + Assert.Single(putActions, a => a.AttributeRouteInfo.Template.Equals("v1/All")); + Assert.Single(putActions, a => a.AttributeRouteInfo.Template.Equals("v2/All")); + + // Actions generated by RouteAttribute + var routeActions = actions.Where( + a => a.ActionConstraints.OfType().Single().HttpMethods.Single() == "POST"); + Assert.Equal(2, routeActions.Count()); + Assert.Single(routeActions, a => a.AttributeRouteInfo.Template.Equals("v1/List")); + Assert.Single(routeActions, a => a.AttributeRouteInfo.Template.Equals("v2/List")); + } + + [Fact] + public void AttributeRouting_AcceptVerbsOnAction_WithTemplate_DoesNotMergeVerb() + { + // Arrange + var provider = GetProvider(typeof(MultiRouteAttributesController).GetTypeInfo()); + + // Act + var descriptors = provider.GetDescriptors(); + + // Assert + var actions = descriptors.Where(d => d.Name == "AcceptVerbsRouteAttributeWithTemplateAndHttpPut"); Assert.Equal(6, actions.Count()); foreach (var action in actions) @@ -764,17 +786,14 @@ namespace Microsoft.AspNet.Mvc.Test "AttributeAndNonAttributeRoutedActionsOnSameMethodController.Method'" + " must not define attribute routed actions and non attribute routed actions at the same time:" + Environment.NewLine + "Action: 'Microsoft.AspNet.Mvc.Test.ControllerActionDescriptorProviderTests+" + - "AttributeAndNonAttributeRoutedActionsOnSameMethodController.Method' - Template: 'AttributeRouted'" + Environment.NewLine + + "AttributeAndNonAttributeRoutedActionsOnSameMethodController.Method' - Route Template: 'AttributeRouted' - " + + "HTTP Verbs: 'GET'" + Environment.NewLine + "Action: 'Microsoft.AspNet.Mvc.Test.ControllerActionDescriptorProviderTests+" + - "AttributeAndNonAttributeRoutedActionsOnSameMethodController.Method' - Template: '(none)'" + Environment.NewLine + - "A method 'Microsoft.AspNet.Mvc.Test.ControllerActionDescriptorProviderTests+" + - "AttributeAndNonAttributeRoutedActionsOnSameMethodController.Method' that defines attribute routed actions must not" + - " have attributes that implement 'Microsoft.AspNet.Mvc.IActionHttpMethodProvider' and do not implement" + - " 'Microsoft.AspNet.Mvc.Routing.IRouteTemplateProvider':" + Environment.NewLine + - "Action 'Microsoft.AspNet.Mvc.Test.ControllerActionDescriptorProviderTests+" + - "AttributeAndNonAttributeRoutedActionsOnSameMethodController.Method' with route template 'AttributeRouted' has " + - "'Microsoft.AspNet.Mvc.Test.ControllerActionDescriptorProviderTests+CustomHttpMethodConstraintAttribute'" + - " invalid 'Microsoft.AspNet.Mvc.IActionHttpMethodProvider' attributes."; + "AttributeAndNonAttributeRoutedActionsOnSameMethodController.Method' - Route Template: '(none)' - " + + "HTTP Verbs: 'DELETE, PATCH, POST, PUT'" + Environment.NewLine + + Environment.NewLine + + "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."; var provider = GetProvider( typeof(AttributeAndNonAttributeRoutedActionsOnSameMethodController).GetTypeInfo()); @@ -783,7 +802,7 @@ namespace Microsoft.AspNet.Mvc.Test var exception = Assert.Throws(() => provider.GetDescriptors()); // Assert - VerifyMultiLineError(expectedMessage, exception.Message); + VerifyMultiLineError(expectedMessage, exception.Message, unorderedStart: 1, unorderedLineCount: 2); } [Fact] @@ -1137,6 +1156,7 @@ namespace Microsoft.AspNet.Mvc.Test var options = new MockMvcOptionsAccessor(); options.Options.ApplicationModelConventions.Add(applicationConvention.Object); + var applicationModel = new ApplicationModel(); var controller = new ControllerModel(typeof(ConventionsController).GetTypeInfo(), @@ -1345,7 +1365,7 @@ namespace Microsoft.AspNet.Mvc.Test } private ControllerActionDescriptorProvider GetProvider( - TypeInfo type, + TypeInfo type, IOptions options) { var modelBuilder = new StaticControllerModelBuilder(type); @@ -1380,14 +1400,48 @@ namespace Microsoft.AspNet.Mvc.Test return provider.GetDescriptors(); } - private static void VerifyMultiLineError(string expectedMessage, string actualMessage) + private static void VerifyMultiLineError( + string expectedMessage, + string actualMessage, + int unorderedStart, + int unorderedLineCount) { - // The error message depends on the order of attributes returned by reflection which is not consistent across - // platforms. We'll compare them individually instead. - Assert.Equal(expectedMessage.Split(new[] { Environment.NewLine }, StringSplitOptions.None) - .OrderBy(m => m, StringComparer.Ordinal), - actualMessage.Split(new[] { Environment.NewLine }, StringSplitOptions.None) - .OrderBy(m => m, StringComparer.Ordinal)); + var expectedLines = expectedMessage + .Split(new[] { Environment.NewLine }, StringSplitOptions.None) + .ToArray(); + + var actualLines = actualMessage + .Split(new[] { Environment.NewLine }, StringSplitOptions.None) + .ToArray(); + + for (var i = 0; i < unorderedStart; i++) + { + Assert.Equal(expectedLines[i], actualLines[i]); + } + + var orderedExpectedLines = expectedLines + .Skip(unorderedStart) + .Take(unorderedLineCount) + .OrderBy(l => l, StringComparer.Ordinal) + .ToArray(); + + var orderedActualLines = actualLines + .Skip(unorderedStart) + .Take(unorderedLineCount) + .OrderBy(l => l, StringComparer.Ordinal) + .ToArray(); + + for (var i = 0; i < unorderedLineCount; i++) + { + Assert.Equal(orderedExpectedLines[i], orderedActualLines[i]); + } + + for (var i = unorderedStart + unorderedLineCount; i < expectedLines.Length; i++) + { + Assert.Equal(expectedLines[i], actualLines[i]); + } + + Assert.Equal(expectedLines.Length, actualLines.Length); } private class HttpMethodController @@ -1398,7 +1452,6 @@ namespace Microsoft.AspNet.Mvc.Test } } - [Route("Products")] [Route("Items")] private class AttributeRoutedHttpMethodController { @@ -1588,6 +1641,11 @@ namespace Microsoft.AspNet.Mvc.Test [Route("List")] [HttpPut("All")] public void AcceptVerbsRouteAttributeAndHttpPut() { } + + [AcceptVerbs("POST", Route = "")] + [Route("List")] + [HttpPut("All")] + public void AcceptVerbsRouteAttributeWithTemplateAndHttpPut() { } } [Route("Products")] @@ -1757,7 +1815,7 @@ namespace Microsoft.AspNet.Mvc.Test public void Edit() { } } - + private class ApiExplorerNameOnActionController { [ApiExplorerSettings(GroupName = "Blog")] diff --git a/test/Microsoft.AspNet.Mvc.FunctionalTests/RoutingTests.cs b/test/Microsoft.AspNet.Mvc.FunctionalTests/RoutingTests.cs index 5282d7fdcc..27182925da 100644 --- a/test/Microsoft.AspNet.Mvc.FunctionalTests/RoutingTests.cs +++ b/test/Microsoft.AspNet.Mvc.FunctionalTests/RoutingTests.cs @@ -1324,6 +1324,54 @@ namespace Microsoft.AspNet.Mvc.FunctionalTests Assert.Null(result.Link); } + [Theory] + [InlineData("/Bank/Deposit", "PUT", "Deposit")] + [InlineData("/Bank/Deposit", "POST", "Deposit")] + [InlineData("/Bank/Deposit/5", "PUT", "Deposit")] + [InlineData("/Bank/Deposit/5", "POST", "Deposit")] + [InlineData("/Bank/Withdraw/5", "POST", "Withdraw")] + public async Task AttributeRouting_MixedAcceptVerbsAndRoute_Reachable(string path, string verb, string actionName) + { + // Arrange + var server = TestServer.Create(_services, _app); + var client = server.CreateClient(); + + var request = new HttpRequestMessage(new HttpMethod(verb), "http://localhost" + path); + + // Act + var response = await client.SendAsync(request); + + // Assert + Assert.Equal(HttpStatusCode.OK, response.StatusCode); + + var body = await response.Content.ReadAsStringAsync(); + var result = JsonConvert.DeserializeObject(body); + + Assert.Contains(path, result.ExpectedUrls); + Assert.Equal("Banks", result.Controller); + Assert.Equal(actionName, result.Action); + } + + // These verbs don't match + [Theory] + [InlineData("/Bank/Deposit", "GET")] + [InlineData("/Bank/Deposit/5", "DELETE")] + [InlineData("/Bank/Withdraw/5", "GET")] + public async Task AttributeRouting_MixedAcceptVerbsAndRoute_Unreachable(string path, string verb) + { + // Arrange + var server = TestServer.Create(_services, _app); + var client = server.CreateClient(); + + var request = new HttpRequestMessage(new HttpMethod(verb), "http://localhost" + path); + + // Act + var response = await client.SendAsync(request); + + // Assert + Assert.Equal(HttpStatusCode.NotFound, response.StatusCode); + } + private static LinkBuilder LinkFrom(string url) { return new LinkBuilder(url); diff --git a/test/WebSites/RoutingWebSite/Controllers/BanksController.cs b/test/WebSites/RoutingWebSite/Controllers/BanksController.cs index 0458571362..d41e418abb 100644 --- a/test/WebSites/RoutingWebSite/Controllers/BanksController.cs +++ b/test/WebSites/RoutingWebSite/Controllers/BanksController.cs @@ -1,5 +1,7 @@ -using Microsoft.AspNet.Mvc; -using System; +// Copyright (c) Microsoft Open Technologies, Inc. All rights reserved. +// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. + +using Microsoft.AspNet.Mvc; namespace RoutingWebSite { @@ -31,5 +33,20 @@ namespace RoutingWebSite Url.Action(), Url.RouteUrl(new { })); } + + [AcceptVerbs("PUT", "POST")] + [Route("Bank/Deposit")] + [Route("Bank/Deposit/{amount}")] + public ActionResult Deposit() + { + return _generator.Generate("/Bank/Deposit", "/Bank/Deposit/5"); + } + + [HttpPost] + [Route("Bank/Withdraw/{id}")] + public ActionResult Withdraw(int id) + { + return _generator.Generate("/Bank/Withdraw/5"); + } } } \ No newline at end of file