From 3ab0c3af29f68a891386f8b7f54ae49037a5cc18 Mon Sep 17 00:00:00 2001 From: Javier Calvarro Nelson Date: Thu, 31 Jul 2014 12:34:25 -0700 Subject: [PATCH] [Issue #733] Attribute Routing: Implement Order 1. Added an Order property to IRouteTemplateProvider, ReflectedAttributeRouteModel, AttributeRouteInfo, AttributeRouteLinkGenerationEntry and AttributeRouteMatchingEntry. 2. Changed the implementation of AttributeRoute to take the order into account when routing incomming requests and generating links. 3. Ensured a stable ordering of route entries with the same order and precedence for route matching and link generation based on the template text. 4. Added tests to validate that the precedence gets respected in route matching and link generation. 5. Added tests to validate that the order gets respected in route matching and link generation. 6. Added tests to validate that the order gets respected over the precedence for route matching and link generation. 7. Added tests to validate that routes with the same order and precedence expose a stable ordering for route matching and link generation. --- samples/MvcSample.Web/HomeController.cs | 31 ++ .../HttpDeleteAttribute.cs | 16 +- .../HttpGetAttribute.cs | 16 +- .../HttpMethodAttribute.cs | 74 ++++ .../HttpPatchAttribute.cs | 16 +- .../HttpPostAttribute.cs | 17 +- .../HttpPutAttribute.cs | 16 +- .../Microsoft.AspNet.Mvc.Core.kproj | 4 +- .../ReflectedActionDescriptorProvider.cs | 35 +- .../ReflectedAttributeRouteModel.cs | 57 ++- .../RouteAttribute.cs | 23 + .../Routing/AttributeRoute.cs | 34 +- .../Routing/AttributeRouteInfo.cs | 7 + .../AttributeRouteLinkGenerationEntry.cs | 10 + .../Routing/AttributeRouteMatchingEntry.cs | 5 + .../Routing/AttributeRouting.cs | 19 +- .../Routing/IRouteTemplateProvider.cs | 8 + .../Microsoft.AspNet.Mvc.Core.Test.kproj | 3 + .../ReflectedAttributeRouteModelTests.cs | 332 +++++++++------ .../RouteTemplateProviderAttributesTests.cs | 32 ++ .../Routing/AttributeRouteTests.cs | 401 +++++++++++++++++- .../RoutingTests.cs | 80 +++- .../Controllers/TeamController.cs | 64 +++ .../RoutingWebSite/HttpMergeAttribute.cs | 4 + .../RoutingWebSite/RoutingWebSite.kproj | 1 + 25 files changed, 1047 insertions(+), 258 deletions(-) create mode 100644 src/Microsoft.AspNet.Mvc.Core/HttpMethodAttribute.cs create mode 100644 test/Microsoft.AspNet.Mvc.Core.Test/RouteTemplateProviderAttributesTests.cs create mode 100644 test/WebSites/RoutingWebSite/Controllers/TeamController.cs diff --git a/samples/MvcSample.Web/HomeController.cs b/samples/MvcSample.Web/HomeController.cs index 2928cde9e4..a7f8251811 100644 --- a/samples/MvcSample.Web/HomeController.cs +++ b/samples/MvcSample.Web/HomeController.cs @@ -126,6 +126,37 @@ namespace MvcSample.Web return user; } + [HttpGet("/AttributeRouting/{other}", Order = 0)] + public string LowerPrecedence(string param) + { + return "Lower"; + } + + // Normally this route would be tried before the one above + // as it is more explicit (doesn't have a parameter), but + // due to the fact that it has a higher order, it will be + // tried after the route above. + [HttpGet("/AttributeRouting/HigherPrecedence", Order = 1)] + public string HigherOrder() + { + return "Higher"; + } + + // Both routes have the same template, which would make + // them ambiguous, but the order we defined in the routes + // disambiguates them. + [HttpGet("/AttributeRouting/SameTemplate", Order = 0)] + public string SameTemplateHigherOrderPrecedence() + { + return "HigherOrderPrecedence"; + } + + [HttpGet("/AttributeRouting/SameTemplate", Order = 1)] + public string SameTemplateLowerOrderPrecedence() + { + return "LowerOrderPrecedence"; + } + /// /// Action that exercises default view names. /// diff --git a/src/Microsoft.AspNet.Mvc.Core/HttpDeleteAttribute.cs b/src/Microsoft.AspNet.Mvc.Core/HttpDeleteAttribute.cs index 6a1fb69183..7595f906d6 100644 --- a/src/Microsoft.AspNet.Mvc.Core/HttpDeleteAttribute.cs +++ b/src/Microsoft.AspNet.Mvc.Core/HttpDeleteAttribute.cs @@ -3,15 +3,13 @@ using System; using System.Collections.Generic; -using Microsoft.AspNet.Mvc.Routing; namespace Microsoft.AspNet.Mvc { /// /// Identifies an action that only supports the HTTP DELETE method. /// - [AttributeUsage(AttributeTargets.Method, AllowMultiple = false, Inherited = true)] - public class HttpDeleteAttribute : Attribute, IActionHttpMethodProvider, IRouteTemplateProvider + public class HttpDeleteAttribute : HttpMethodAttribute { private static readonly IEnumerable _supportedMethods = new string[] { "DELETE" }; @@ -19,6 +17,7 @@ namespace Microsoft.AspNet.Mvc /// Creates a new . /// public HttpDeleteAttribute() + : base(_supportedMethods) { } @@ -27,17 +26,8 @@ namespace Microsoft.AspNet.Mvc /// /// The route template. May not be null. public HttpDeleteAttribute([NotNull] string template) + : base(_supportedMethods, template) { - Template = template; } - - /// - public IEnumerable HttpMethods - { - get { return _supportedMethods; } - } - - /// - public string Template { get; private set; } } } \ No newline at end of file diff --git a/src/Microsoft.AspNet.Mvc.Core/HttpGetAttribute.cs b/src/Microsoft.AspNet.Mvc.Core/HttpGetAttribute.cs index 72c3145108..8ef6d09939 100644 --- a/src/Microsoft.AspNet.Mvc.Core/HttpGetAttribute.cs +++ b/src/Microsoft.AspNet.Mvc.Core/HttpGetAttribute.cs @@ -3,15 +3,13 @@ using System; using System.Collections.Generic; -using Microsoft.AspNet.Mvc.Routing; namespace Microsoft.AspNet.Mvc { /// /// Identifies an action that only supports the HTTP GET method. /// - [AttributeUsage(AttributeTargets.Method, AllowMultiple = false, Inherited = true)] - public class HttpGetAttribute : Attribute, IActionHttpMethodProvider, IRouteTemplateProvider + public class HttpGetAttribute : HttpMethodAttribute { private static readonly IEnumerable _supportedMethods = new string[] { "GET" }; @@ -19,6 +17,7 @@ namespace Microsoft.AspNet.Mvc /// Creates a new . /// public HttpGetAttribute() + : base(_supportedMethods) { } @@ -27,17 +26,8 @@ namespace Microsoft.AspNet.Mvc /// /// The route template. May not be null. public HttpGetAttribute([NotNull] string template) + : base(_supportedMethods, template) { - Template = template; } - - /// - public IEnumerable HttpMethods - { - get { return _supportedMethods; } - } - - /// - public string Template { get; private set; } } } \ No newline at end of file diff --git a/src/Microsoft.AspNet.Mvc.Core/HttpMethodAttribute.cs b/src/Microsoft.AspNet.Mvc.Core/HttpMethodAttribute.cs new file mode 100644 index 0000000000..c7d96f1074 --- /dev/null +++ b/src/Microsoft.AspNet.Mvc.Core/HttpMethodAttribute.cs @@ -0,0 +1,74 @@ +// 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 System; +using System.Collections.Generic; +using Microsoft.AspNet.Mvc.Routing; + +namespace Microsoft.AspNet.Mvc +{ + /// + /// Identifies an action that only supports a given set of HTTP methods. + /// + [AttributeUsage(AttributeTargets.Method, AllowMultiple = false, Inherited = true)] + public abstract class HttpMethodAttribute : Attribute, IActionHttpMethodProvider, IRouteTemplateProvider + { + private int? _order; + private readonly IEnumerable _httpMethods; + + /// + /// Creates a new with the given + /// set of HTTP methods. + /// The set of supported HTTP methods. + /// + public HttpMethodAttribute([NotNull] IEnumerable httpMethods) + : this(httpMethods, null) + { + } + + /// + /// Creates a new with the given + /// set of HTTP methods an the given route template. + /// + /// The set of supported methods. + /// The route template. May not be null. + public HttpMethodAttribute([NotNull] IEnumerable httpMethods, string template) + { + _httpMethods = httpMethods; + Template = template; + } + + /// + public IEnumerable HttpMethods + { + get + { + return _httpMethods; + } + } + + /// + public string Template { get; private set; } + + /// + /// Gets the route order. The order determines the order of route execution. Routes with a lower + /// order value are tried first. When a route doesn't specify a value, it gets the value of the + /// or a default value of 0 if the + /// doesn't define a value on the controller. + /// + public int Order + { + get { return _order ?? 0; } + set { _order = value; } + } + + /// + int? IRouteTemplateProvider.Order + { + get + { + return _order; + } + } + } +} \ No newline at end of file diff --git a/src/Microsoft.AspNet.Mvc.Core/HttpPatchAttribute.cs b/src/Microsoft.AspNet.Mvc.Core/HttpPatchAttribute.cs index 4710ff29b7..143b361d7e 100644 --- a/src/Microsoft.AspNet.Mvc.Core/HttpPatchAttribute.cs +++ b/src/Microsoft.AspNet.Mvc.Core/HttpPatchAttribute.cs @@ -3,15 +3,13 @@ using System; using System.Collections.Generic; -using Microsoft.AspNet.Mvc.Routing; namespace Microsoft.AspNet.Mvc { /// /// Identifies an action that only supports the HTTP PATCH method. /// - [AttributeUsage(AttributeTargets.Method, AllowMultiple = false, Inherited = true)] - public class HttpPatchAttribute : Attribute, IActionHttpMethodProvider, IRouteTemplateProvider + public class HttpPatchAttribute : HttpMethodAttribute { private static readonly IEnumerable _supportedMethods = new string[] { "PATCH" }; @@ -19,6 +17,7 @@ namespace Microsoft.AspNet.Mvc /// Creates a new . /// public HttpPatchAttribute() + : base(_supportedMethods) { } @@ -27,17 +26,8 @@ namespace Microsoft.AspNet.Mvc /// /// The route template. May not be null. public HttpPatchAttribute([NotNull] string template) + : base(_supportedMethods, template) { - Template = template; } - - /// - public IEnumerable HttpMethods - { - get { return _supportedMethods; } - } - - /// - public string Template { get; private set; } } } \ No newline at end of file diff --git a/src/Microsoft.AspNet.Mvc.Core/HttpPostAttribute.cs b/src/Microsoft.AspNet.Mvc.Core/HttpPostAttribute.cs index 5a771992d4..3a06c25aa8 100644 --- a/src/Microsoft.AspNet.Mvc.Core/HttpPostAttribute.cs +++ b/src/Microsoft.AspNet.Mvc.Core/HttpPostAttribute.cs @@ -3,23 +3,21 @@ using System; using System.Collections.Generic; -using Microsoft.AspNet.Mvc.Routing; namespace Microsoft.AspNet.Mvc { /// /// Identifies an action that only supports the HTTP POST method. /// - [AttributeUsage(AttributeTargets.Method, AllowMultiple = false, Inherited = true)] - public class HttpPostAttribute : Attribute, IActionHttpMethodProvider, IRouteTemplateProvider + public class HttpPostAttribute : HttpMethodAttribute { private static readonly IEnumerable _supportedMethods = new string[] { "POST" }; /// /// Creates a new . /// - /// The route template. May not be null. public HttpPostAttribute() + : base(_supportedMethods) { } @@ -28,17 +26,8 @@ namespace Microsoft.AspNet.Mvc /// /// The route template. May not be null. public HttpPostAttribute([NotNull] string template) + : base(_supportedMethods, template) { - Template = template; } - - /// - public IEnumerable HttpMethods - { - get { return _supportedMethods; } - } - - /// - public string Template { get; private set; } } } \ No newline at end of file diff --git a/src/Microsoft.AspNet.Mvc.Core/HttpPutAttribute.cs b/src/Microsoft.AspNet.Mvc.Core/HttpPutAttribute.cs index 9ef79a1bb4..c1beff4256 100644 --- a/src/Microsoft.AspNet.Mvc.Core/HttpPutAttribute.cs +++ b/src/Microsoft.AspNet.Mvc.Core/HttpPutAttribute.cs @@ -3,15 +3,13 @@ using System; using System.Collections.Generic; -using Microsoft.AspNet.Mvc.Routing; namespace Microsoft.AspNet.Mvc { /// /// Identifies an action that only supports the HTTP PUT method. /// - [AttributeUsage(AttributeTargets.Method, AllowMultiple = false, Inherited = true)] - public class HttpPutAttribute : Attribute, IActionHttpMethodProvider, IRouteTemplateProvider + public class HttpPutAttribute : HttpMethodAttribute { private static readonly IEnumerable _supportedMethods = new string[] { "PUT" }; @@ -19,6 +17,7 @@ namespace Microsoft.AspNet.Mvc /// Creates a new . /// public HttpPutAttribute() + : base(_supportedMethods) { } @@ -27,17 +26,8 @@ namespace Microsoft.AspNet.Mvc /// /// The route template. May not be null. public HttpPutAttribute([NotNull] string template) + : base(_supportedMethods, template) { - Template = template; } - - /// - public IEnumerable HttpMethods - { - get { return _supportedMethods; } - } - - /// - public string Template { get; private set; } } } \ No newline at end of file diff --git a/src/Microsoft.AspNet.Mvc.Core/Microsoft.AspNet.Mvc.Core.kproj b/src/Microsoft.AspNet.Mvc.Core/Microsoft.AspNet.Mvc.Core.kproj index 46a286d607..208f9ec964 100644 --- a/src/Microsoft.AspNet.Mvc.Core/Microsoft.AspNet.Mvc.Core.kproj +++ b/src/Microsoft.AspNet.Mvc.Core/Microsoft.AspNet.Mvc.Core.kproj @@ -31,6 +31,7 @@ + @@ -190,6 +191,7 @@ + @@ -247,10 +249,10 @@ + - diff --git a/src/Microsoft.AspNet.Mvc.Core/ReflectedActionDescriptorProvider.cs b/src/Microsoft.AspNet.Mvc.Core/ReflectedActionDescriptorProvider.cs index 0d9eb23a51..e7d3e3cd62 100644 --- a/src/Microsoft.AspNet.Mvc.Core/ReflectedActionDescriptorProvider.cs +++ b/src/Microsoft.AspNet.Mvc.Core/ReflectedActionDescriptorProvider.cs @@ -17,8 +17,16 @@ namespace Microsoft.AspNet.Mvc { public class ReflectedActionDescriptorProvider : IActionDescriptorProvider { + /// + /// Represents the default order associated with this provider for dependency injection + /// purposes. + /// public static readonly int DefaultOrder = 0; + // This is the default order for attribute routes whose order calculated from + // the reflected model is null. + private const int DefaultAttributeRouteOrder = 0; + private readonly IControllerAssemblyProvider _controllerAssemblyProvider; private readonly IActionDiscoveryConventions _conventions; private readonly IEnumerable _globalFilters; @@ -114,7 +122,7 @@ namespace Microsoft.AspNet.Mvc { var actions = new List(); - var routeGroupsByTemplate = new Dictionary(StringComparer.OrdinalIgnoreCase); + var hasAttributeRoutes = false; var removalConstraints = new HashSet(StringComparer.OrdinalIgnoreCase); var routeTemplateErrors = new List(); @@ -152,7 +160,8 @@ namespace Microsoft.AspNet.Mvc var attributeRouteInfo = combinedRoute == null ? null : new AttributeRouteInfo() { - Template = combinedRoute.Template + Template = combinedRoute.Template, + Order = combinedRoute.Order ?? DefaultAttributeRouteOrder }; var actionDescriptor = new ReflectedActionDescriptor() @@ -215,6 +224,7 @@ namespace Microsoft.AspNet.Mvc if (actionDescriptor.AttributeRouteInfo != null && actionDescriptor.AttributeRouteInfo.Template != null) { + hasAttributeRoutes = true; // An attribute routed action will ignore conventional routed constraints. We still // want to provide these values as ambient values. foreach (var constraint in actionDescriptor.RouteConstraints) @@ -243,19 +253,14 @@ namespace Microsoft.AspNet.Mvc actionDescriptor.AttributeRouteInfo.Template = templateText; - // An attribute routed action is matched by its 'route group' which identifies all equivalent - // actions. - string routeGroup; - if (!routeGroupsByTemplate.TryGetValue(templateText, out routeGroup)) - { - routeGroup = GetRouteGroup(templateText); - routeGroupsByTemplate.Add(templateText, routeGroup); - } + var routeGroupValue = GetRouteGroupValue( + actionDescriptor.AttributeRouteInfo.Order, + templateText); var routeConstraints = new List(); routeConstraints.Add(new RouteDataActionConstraint( AttributeRouting.RouteGroupKey, - routeGroup)); + routeGroupValue)); actionDescriptor.RouteConstraints = routeConstraints; } @@ -279,7 +284,7 @@ namespace Microsoft.AspNet.Mvc { // 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 (routeGroupsByTemplate.Any()) + if (hasAttributeRoutes) { actionDescriptor.RouteConstraints.Add(new RouteDataActionConstraint( AttributeRouting.RouteGroupKey, @@ -320,10 +325,10 @@ namespace Microsoft.AspNet.Mvc return actions; } - // Returns a unique, stable key per-route-template (OrdinalIgnoreCase) - private static string GetRouteGroup(string template) + private static string GetRouteGroupValue(int order, string template) { - return ("__route__" + template).ToUpperInvariant(); + var group = string.Format("{0}-{1}", order, template); + return ("__route__" + group).ToUpperInvariant(); } } } diff --git a/src/Microsoft.AspNet.Mvc.Core/ReflectedModelBuilder/ReflectedAttributeRouteModel.cs b/src/Microsoft.AspNet.Mvc.Core/ReflectedModelBuilder/ReflectedAttributeRouteModel.cs index 875677a030..a58f46e490 100644 --- a/src/Microsoft.AspNet.Mvc.Core/ReflectedModelBuilder/ReflectedAttributeRouteModel.cs +++ b/src/Microsoft.AspNet.Mvc.Core/ReflectedModelBuilder/ReflectedAttributeRouteModel.cs @@ -20,14 +20,17 @@ namespace Microsoft.AspNet.Mvc.ReflectedModelBuilder public ReflectedAttributeRouteModel([NotNull] IRouteTemplateProvider templateProvider) { Template = templateProvider.Template; + Order = templateProvider.Order; } public string Template { get; set; } - /// + public int? Order { get; set; } + + /// /// Combines two instances and returns /// a new instance with the result. - /// + /// /// The left . /// The right . /// A new instance of that represents the @@ -37,30 +40,31 @@ namespace Microsoft.AspNet.Mvc.ReflectedModelBuilder ReflectedAttributeRouteModel left, ReflectedAttributeRouteModel right) { - left = left ?? _default; right = right ?? _default; - var template = CombineTemplates(left.Template, right.Template); + // If the right template is an override template (starts with / or ~/) + // we ignore the values from left. + if (left == null || IsOverridePattern(right.Template)) + { + left = _default; + } + + var combinedTemplate = CombineTemplates(left.Template, right.Template); // The action is not attribute routed. - if (template == null) + if (combinedTemplate == null) { return null; } return new ReflectedAttributeRouteModel() - { - Template = template + { + Template = combinedTemplate, + Order = right.Order ?? left.Order }; } - /// - /// Combines attribute routing templates. - /// - /// The left template. - /// The right template. - /// A combined template. - public static string CombineTemplates(string left, string right) + internal static string CombineTemplates(string left, string right) { var result = CombineCore(left, right); return CleanTemplate(result); @@ -72,19 +76,11 @@ namespace Microsoft.AspNet.Mvc.ReflectedModelBuilder { return null; } - else if (left == null) - { - return right; - } else if (right == null) { return left; } - - if (right.StartsWith("~/", StringComparison.OrdinalIgnoreCase) || - right.StartsWith("/", StringComparison.OrdinalIgnoreCase) || - left.Equals("~/", StringComparison.OrdinalIgnoreCase) || - left.Equals("/", StringComparison.OrdinalIgnoreCase)) + else if (IsEmptyLeftSegment(left) || IsOverridePattern(right)) { return right; } @@ -98,6 +94,21 @@ namespace Microsoft.AspNet.Mvc.ReflectedModelBuilder return left + '/' + right; } + private static bool IsOverridePattern(string template) + { + return template != null && + (template.StartsWith("~/", StringComparison.OrdinalIgnoreCase) || + template.StartsWith("/", StringComparison.OrdinalIgnoreCase)); + } + + private static bool IsEmptyLeftSegment(string template) + { + return template == null || + template.Equals(string.Empty, StringComparison.OrdinalIgnoreCase) || + template.Equals("~/", StringComparison.OrdinalIgnoreCase) || + template.Equals("/", StringComparison.OrdinalIgnoreCase); + } + private static string CleanTemplate(string result) { if (result == null) diff --git a/src/Microsoft.AspNet.Mvc.Core/RouteAttribute.cs b/src/Microsoft.AspNet.Mvc.Core/RouteAttribute.cs index b27fd64df4..138215762f 100644 --- a/src/Microsoft.AspNet.Mvc.Core/RouteAttribute.cs +++ b/src/Microsoft.AspNet.Mvc.Core/RouteAttribute.cs @@ -12,6 +12,8 @@ namespace Microsoft.AspNet.Mvc [AttributeUsage(AttributeTargets.Class, AllowMultiple = false, Inherited = true)] public class RouteAttribute : Attribute, IRouteTemplateProvider { + private int? _order; + /// /// Creates a new with the given route template. /// @@ -23,5 +25,26 @@ namespace Microsoft.AspNet.Mvc /// public string Template { get; private set; } + + /// + /// Gets the route order. The order determines the order of route execution. Routes with a lower order + /// value are tried first. If an action defines a route by providing an + /// with a non null order, that order is used instead of this value. If neither the action nor the + /// controller defines an order, a default value of 0 is used. + /// + public int Order + { + get { return _order ?? 0; } + set { _order = value; } + } + + /// + int? IRouteTemplateProvider.Order + { + get + { + return _order; + } + } } } \ No newline at end of file diff --git a/src/Microsoft.AspNet.Mvc.Core/Routing/AttributeRoute.cs b/src/Microsoft.AspNet.Mvc.Core/Routing/AttributeRoute.cs index f4ffe7d273..a71d902924 100644 --- a/src/Microsoft.AspNet.Mvc.Core/Routing/AttributeRoute.cs +++ b/src/Microsoft.AspNet.Mvc.Core/Routing/AttributeRoute.cs @@ -29,20 +29,30 @@ namespace Microsoft.AspNet.Mvc.Routing /// The next router. Invoked when a route entry matches. /// The set of route entries. public AttributeRoute( - [NotNull] IRouter next, + [NotNull] IRouter next, [NotNull] IEnumerable matchingEntries, [NotNull] IEnumerable linkGenerationEntries, [NotNull] ILoggerFactory factory) { _next = next; - // FOR RIGHT NOW - this is just an array of regular template routes. We'll follow up by implementing - // a good data-structure here. See #740 - _matchingRoutes = matchingEntries.OrderBy(e => e.Precedence).Select(e => e.Route).ToArray(); + // Order all the entries by order, then precedence, and then finally by template in order to provide + // a stable routing and link generation order for templates with same order and precedence. + // We use ordinal comparison for the templates because we only care about them being exactly equal and + // we don't want to make any equivalence between templates based on the culture of the machine. - // FOR RIGHT NOW - this is just an array of entries. We'll follow up by implementing - // a good data-structure here. See #741 - _linkGenerationEntries = linkGenerationEntries.OrderBy(e => e.Precedence).ToArray(); + _matchingRoutes = matchingEntries + .OrderBy(o => o.Order) + .ThenBy(e => e.Precedence) + .ThenBy(e => e.Route.RouteTemplate, StringComparer.Ordinal) + .Select(e => e.Route) + .ToArray(); + + _linkGenerationEntries = linkGenerationEntries + .OrderBy(o => o.Order) + .ThenBy(e => e.Precedence) + .ThenBy(e => e.TemplateText, StringComparer.Ordinal) + .ToArray(); _logger = factory.Create(); _constraintLogger = factory.Create(typeof(RouteConstraintMatcher).FullName); @@ -53,12 +63,12 @@ namespace Microsoft.AspNet.Mvc.Routing { using (_logger.BeginScope("AttributeRoute.RouteAsync")) { - foreach (var route in _matchingRoutes) - { - await route.RouteAsync(context); + foreach (var route in _matchingRoutes) + { + await route.RouteAsync(context); - if (context.IsHandled) - { + if (context.IsHandled) + { break; } } diff --git a/src/Microsoft.AspNet.Mvc.Core/Routing/AttributeRouteInfo.cs b/src/Microsoft.AspNet.Mvc.Core/Routing/AttributeRouteInfo.cs index f8ba92e7d9..463de8c510 100644 --- a/src/Microsoft.AspNet.Mvc.Core/Routing/AttributeRouteInfo.cs +++ b/src/Microsoft.AspNet.Mvc.Core/Routing/AttributeRouteInfo.cs @@ -12,5 +12,12 @@ namespace Microsoft.AspNet.Mvc.Routing /// The route template. May be null if the action has no attribute routes. /// public string Template { get; set; } + + /// + /// Gets the order of the route associated with this . This property determines + /// the order in which routes get executed. Routes with a lower order value are tried first. In case a route + /// doesn't specify a value, it gets a default order of 0. + /// + public int Order { get; set; } } } \ No newline at end of file diff --git a/src/Microsoft.AspNet.Mvc.Core/Routing/AttributeRouteLinkGenerationEntry.cs b/src/Microsoft.AspNet.Mvc.Core/Routing/AttributeRouteLinkGenerationEntry.cs index f2fc7204dc..f088991513 100644 --- a/src/Microsoft.AspNet.Mvc.Core/Routing/AttributeRouteLinkGenerationEntry.cs +++ b/src/Microsoft.AspNet.Mvc.Core/Routing/AttributeRouteLinkGenerationEntry.cs @@ -28,6 +28,11 @@ namespace Microsoft.AspNet.Mvc.Routing /// public IDictionary Defaults { get; set; } + /// + /// The order of the template. + /// + public int Order { get; set; } + /// /// The precedence of the template. /// @@ -47,5 +52,10 @@ namespace Microsoft.AspNet.Mvc.Routing /// The . /// public RouteTemplate Template { get; set; } + + /// + /// The original representing the route template. + /// + public string TemplateText { get; set; } } } \ No newline at end of file diff --git a/src/Microsoft.AspNet.Mvc.Core/Routing/AttributeRouteMatchingEntry.cs b/src/Microsoft.AspNet.Mvc.Core/Routing/AttributeRouteMatchingEntry.cs index 39790523d6..8816b3882f 100644 --- a/src/Microsoft.AspNet.Mvc.Core/Routing/AttributeRouteMatchingEntry.cs +++ b/src/Microsoft.AspNet.Mvc.Core/Routing/AttributeRouteMatchingEntry.cs @@ -12,6 +12,11 @@ namespace Microsoft.AspNet.Mvc.Routing /// public class AttributeRouteMatchingEntry { + /// + /// The order of the template. + /// + public int Order { get; set; } + /// /// The precedence of the template. /// diff --git a/src/Microsoft.AspNet.Mvc.Core/Routing/AttributeRouting.cs b/src/Microsoft.AspNet.Mvc.Core/Routing/AttributeRouting.cs index d7cbecdebb..a9922c846f 100644 --- a/src/Microsoft.AspNet.Mvc.Core/Routing/AttributeRouting.cs +++ b/src/Microsoft.AspNet.Mvc.Core/Routing/AttributeRouting.cs @@ -41,10 +41,12 @@ namespace Microsoft.AspNet.Mvc.Routing Binder = new TemplateBinder(routeInfo.ParsedTemplate, routeInfo.Defaults), Defaults = routeInfo.Defaults, Constraints = routeInfo.Constraints, + Order = routeInfo.Order, Precedence = routeInfo.Precedence, RequiredLinkValues = routeInfo.ActionDescriptor.RouteValueDefaults, RouteGroup = routeInfo.RouteGroup, Template = routeInfo.ParsedTemplate, + TemplateText = routeInfo.RouteTemplate }); } @@ -57,6 +59,7 @@ namespace Microsoft.AspNet.Mvc.Routing { matchingEntries.Add(new AttributeRouteMatchingEntry() { + Order = routeInfo.Order, Precedence = routeInfo.Precedence, Route = new TemplateRoute( target, @@ -72,9 +75,9 @@ namespace Microsoft.AspNet.Mvc.Routing } return new AttributeRoute( - target, - matchingEntries, - generationEntries, + target, + matchingEntries, + generationEntries, services.GetService()); } @@ -133,11 +136,11 @@ namespace Microsoft.AspNet.Mvc.Routing if (errors.Count > 0) { var allErrors = string.Join( - Environment.NewLine + Environment.NewLine, + Environment.NewLine + Environment.NewLine, errors.Select( e => Resources.FormatAttributeRoute_IndividualErrorMessage( - e.ActionDescriptor.DisplayName, - Environment.NewLine, + e.ActionDescriptor.DisplayName, + Environment.NewLine, e.ErrorMessage))); var message = Resources.FormatAttributeRoute_AggregateErrorMessage(Environment.NewLine, allErrors); @@ -208,6 +211,8 @@ namespace Microsoft.AspNet.Mvc.Routing } } + routeInfo.Order = action.AttributeRouteInfo.Order; + routeInfo.Precedence = AttributeRoutePrecedence.Compute(routeInfo.ParsedTemplate); routeInfo.Constraints = routeInfo.ParsedTemplate.Parameters @@ -233,6 +238,8 @@ namespace Microsoft.AspNet.Mvc.Routing public RouteTemplate ParsedTemplate { get; set; } + public int Order { get; set; } + public decimal Precedence { get; set; } public string RouteGroup { get; set; } diff --git a/src/Microsoft.AspNet.Mvc.Core/Routing/IRouteTemplateProvider.cs b/src/Microsoft.AspNet.Mvc.Core/Routing/IRouteTemplateProvider.cs index 2dc7969d2d..4cbd7ff3d1 100644 --- a/src/Microsoft.AspNet.Mvc.Core/Routing/IRouteTemplateProvider.cs +++ b/src/Microsoft.AspNet.Mvc.Core/Routing/IRouteTemplateProvider.cs @@ -12,5 +12,13 @@ namespace Microsoft.AspNet.Mvc.Routing /// The route template. May be null. /// string Template { get; } + + /// + /// Gets the route order. The order determines the order of route execution. Routes with a lower + /// order value are tried first. When a route doesn't specify a value, it gets a default value of 0. + /// A null value for the Order property means that the user didn't specify an explicit order for the + /// route. + /// + int? Order { get; } } } \ No newline at end of file diff --git a/test/Microsoft.AspNet.Mvc.Core.Test/Microsoft.AspNet.Mvc.Core.Test.kproj b/test/Microsoft.AspNet.Mvc.Core.Test/Microsoft.AspNet.Mvc.Core.Test.kproj index 3b8eddb6e7..8040731a17 100644 --- a/test/Microsoft.AspNet.Mvc.Core.Test/Microsoft.AspNet.Mvc.Core.Test.kproj +++ b/test/Microsoft.AspNet.Mvc.Core.Test/Microsoft.AspNet.Mvc.Core.Test.kproj @@ -35,6 +35,7 @@ + @@ -88,6 +89,7 @@ + @@ -103,6 +105,7 @@ + diff --git a/test/Microsoft.AspNet.Mvc.Core.Test/ReflectedModelBuilder/ReflectedAttributeRouteModelTests.cs b/test/Microsoft.AspNet.Mvc.Core.Test/ReflectedModelBuilder/ReflectedAttributeRouteModelTests.cs index 8f5647aa59..304b6a9a90 100644 --- a/test/Microsoft.AspNet.Mvc.Core.Test/ReflectedModelBuilder/ReflectedAttributeRouteModelTests.cs +++ b/test/Microsoft.AspNet.Mvc.Core.Test/ReflectedModelBuilder/ReflectedAttributeRouteModelTests.cs @@ -101,6 +101,210 @@ namespace Microsoft.AspNet.Mvc.ReflectedModelBuilder Assert.Equal(expected, combined); } + [Theory] + [MemberData("ReplaceTokens_ValueValuesData")] + public void ReplaceTokens_ValidValues(string template, object values, string expected) + { + // Arrange + var valuesDictionary = values as IDictionary; + if (valuesDictionary == null) + { + valuesDictionary = new RouteValueDictionary(values); + } + + // Act + var result = ReflectedAttributeRouteModel.ReplaceTokens(template, valuesDictionary); + + // Assert + Assert.Equal(expected, result); + } + + [Theory] + [MemberData("ReplaceTokens_InvalidFormatValuesData")] + public void ReplaceTokens_InvalidFormat(string template, object values, string reason) + { + // Arrange + var valuesDictionary = values as IDictionary; + if (valuesDictionary == null) + { + valuesDictionary = new RouteValueDictionary(values); + } + + var expected = string.Format( + "The route template '{0}' has invalid syntax. {1}", + template, + reason); + + // Act + var ex = Assert.Throws( + () => { ReflectedAttributeRouteModel.ReplaceTokens(template, valuesDictionary); }); + + // Assert + Assert.Equal(expected, ex.Message); + } + + [Fact] + public void ReplaceTokens_UnknownValue() + { + // Arrange + var template = "[area]/[controller]/[action2]"; + var values = new RouteValueDictionary() + { + { "area", "Help" }, + { "controller", "Admin" }, + { "action", "SeeUsers" }, + }; + + var expected = + "While processing template '[area]/[controller]/[action2]', " + + "a replacement value for the token 'action2' could not be found. " + + "Available tokens: 'area, controller, action'."; + + // Act + var ex = Assert.Throws( + () => { ReflectedAttributeRouteModel.ReplaceTokens(template, values); }); + + // Assert + Assert.Equal(expected, ex.Message); + } + + [Theory] + [MemberData("CombineOrdersTestData")] + public void Combine_Orders( + ReflectedAttributeRouteModel left, + ReflectedAttributeRouteModel right, + int? expected) + { + // Arrange & Act + var combined = ReflectedAttributeRouteModel.CombineReflectedAttributeRouteModel(left, right); + + // Assert + Assert.NotNull(combined); + Assert.Equal(expected, combined.Order); + } + + [Theory] + [MemberData("ValidReflectedAttributeRouteModelsTestData")] + public void Combine_ValidReflectedAttributeRouteModels( + ReflectedAttributeRouteModel left, + ReflectedAttributeRouteModel right, + ReflectedAttributeRouteModel expectedResult) + { + // Arrange & Act + var combined = ReflectedAttributeRouteModel.CombineReflectedAttributeRouteModel(left, right); + + // Assert + Assert.NotNull(combined); + Assert.Equal(expectedResult.Template, combined.Template); + } + + [Theory] + [MemberData("NullOrNullTemplateReflectedAttributeRouteModelTestData")] + public void Combine_NullOrNullTemplateReflectedAttributeRouteModels( + ReflectedAttributeRouteModel left, + ReflectedAttributeRouteModel right) + { + // Arrange & Act + var combined = ReflectedAttributeRouteModel.CombineReflectedAttributeRouteModel(left, right); + + // Assert + Assert.Null(combined); + } + + [Theory] + [MemberData("RightOverridesReflectedAttributeRouteModelTestData")] + public void Combine_RightOverridesReflectedAttributeRouteModel( + ReflectedAttributeRouteModel left, + ReflectedAttributeRouteModel right) + { + // Arrange + var expectedTemplate = ReflectedAttributeRouteModel.CombineTemplates(null, right.Template); + + // Act + var combined = ReflectedAttributeRouteModel.CombineReflectedAttributeRouteModel(left, right); + + // Assert + Assert.NotNull(combined); + Assert.Equal(expectedTemplate, combined.Template); + Assert.Equal(combined.Order, right.Order); + } + + public static IEnumerable CombineOrdersTestData + { + get + { + var data = new TheoryData(); + + data.Add(Create("", order: 1), Create("", order: 2), 2); + data.Add(Create("", order: 1), Create("", order: null), 1); + data.Add(Create("", order: 1), null, 1); + data.Add(Create("", order: 1), Create("/", order: 2), 2); + data.Add(Create("", order: 1), Create("/", order: null), null); + data.Add(Create("", order: null), Create("", order: 2), 2); + data.Add(Create("", order: null), Create("", order: null), null); + data.Add(Create("", order: null), null, null); + data.Add(Create("", order: null), Create("/", order: 2), 2); + data.Add(Create("", order: null), Create("/", order: null), null); + data.Add(null, Create("", order: 2), 2); + data.Add(null, Create("", order: null), null); + data.Add(null, Create("/", order: 2), 2); + data.Add(null, Create("/", order: null), null); + + // We don't a test case for (left = null, right = null) as it is already tested in another test + // and will produce a null ReflectedAttributeRouteModel, which complicates the test case. + + return data; + } + } + + public static IEnumerable RightOverridesReflectedAttributeRouteModelTestData + { + get + { + var data = new TheoryData(); + var leftModel = Create("Home", order: 3); + + data.Add(leftModel, Create("/")); + data.Add(leftModel, Create("~/")); + data.Add(null, Create("/")); + data.Add(null, Create("~/")); + data.Add(Create(null), Create("/")); + data.Add(Create(null), Create("~/")); + + return data; + } + } + + public static IEnumerable NullOrNullTemplateReflectedAttributeRouteModelTestData + { + get + { + var data = new TheoryData(); + data.Add(null, null); + data.Add(null, Create(null)); + data.Add(Create(null), null); + data.Add(Create(null), Create(null)); + + return data; + } + } + + public static IEnumerable ValidReflectedAttributeRouteModelsTestData + { + get + { + var data = new TheoryData(); + data.Add(null, Create("Index"), Create("Index")); + data.Add(Create(null), Create("Index"), Create("Index"));; + data.Add(Create("Home"), null, Create("Home")); + data.Add(Create("Home"), Create(null), Create("Home")); + data.Add(Create("Home"), Create("Index"), Create("Home/Index")); + data.Add(Create("Blog"), Create("/Index"), Create("Index")); + + return data; + } + } + public static IEnumerable ReplaceTokens_ValueValuesData { get @@ -182,24 +386,6 @@ namespace Microsoft.AspNet.Mvc.ReflectedModelBuilder } } - [Theory] - [MemberData("ReplaceTokens_ValueValuesData")] - public void ReplaceTokens_ValidValues(string template, object values, string expected) - { - // Arrange - var valuesDictionary = values as IDictionary; - if (valuesDictionary == null) - { - valuesDictionary = new RouteValueDictionary(values); - } - - // Act - var result = ReflectedAttributeRouteModel.ReplaceTokens(template, valuesDictionary); - - // Assert - Assert.Equal(expected, result); - } - public static IEnumerable ReplaceTokens_InvalidFormatValuesData { get @@ -265,116 +451,12 @@ namespace Microsoft.AspNet.Mvc.ReflectedModelBuilder } } - [Theory] - [MemberData("ReplaceTokens_InvalidFormatValuesData")] - public void ReplaceTokens_InvalidFormat(string template, object values, string reason) - { - // Arrange - var valuesDictionary = values as IDictionary; - if (valuesDictionary == null) - { - valuesDictionary = new RouteValueDictionary(values); - } - - var expected = string.Format( - "The route template '{0}' has invalid syntax. {1}", - template, - reason); - - // Act - var ex = Assert.Throws( - () => { ReflectedAttributeRouteModel.ReplaceTokens(template, valuesDictionary); }); - - // Assert - Assert.Equal(expected, ex.Message); - } - - [Fact] - public void ReplaceTokens_UnknownValue() - { - // Arrange - var template = "[area]/[controller]/[action2]"; - var values = new RouteValueDictionary() - { - { "area", "Help" }, - { "controller", "Admin" }, - { "action", "SeeUsers" }, - }; - - var expected = - "While processing template '[area]/[controller]/[action2]', " + - "a replacement value for the token 'action2' could not be found. " + - "Available tokens: 'area, controller, action'."; - - // Act - var ex = Assert.Throws( - () => { ReflectedAttributeRouteModel.ReplaceTokens(template, values); }); - - // Assert - Assert.Equal(expected, ex.Message); - } - - [Theory] - [MemberData("ValidReflectedAttributeRouteModelsTestData")] - public void Combine_ValidReflectedAttributeRouteModels( - ReflectedAttributeRouteModel left, - ReflectedAttributeRouteModel right, - ReflectedAttributeRouteModel expectedResult) - { - // Arrange & Act - var combined = ReflectedAttributeRouteModel.CombineReflectedAttributeRouteModel(left, right); - - // Assert - Assert.NotNull(combined); - Assert.Equal(expectedResult.Template, combined.Template); - } - - [Theory] - [MemberData("NullOrNullTemplateReflectedAttributeRouteModelTestData")] - public void Combine_NullOrNullTemplateReflectedAttributeRouteModels( - ReflectedAttributeRouteModel left, - ReflectedAttributeRouteModel right) - { - // Arrange & Act - var combined = ReflectedAttributeRouteModel.CombineReflectedAttributeRouteModel(left, right); - - // Assert - Assert.Null(combined); - } - - public static IEnumerable NullOrNullTemplateReflectedAttributeRouteModelTestData - { - get - { - var data = new TheoryData(); - data.Add(null, null); - data.Add(null, Create(null)); - data.Add(Create(null), null); - data.Add(Create(null), Create(null)); - - return data; - } - } - - public static IEnumerable ValidReflectedAttributeRouteModelsTestData - { - get - { - var data = new TheoryData(); - data.Add(null, Create("Index"), Create("Index")); - data.Add(Create("Home"), null, Create("Home")); - data.Add(Create("Home"), Create("Index"), Create("Home/Index")); - data.Add(Create("Blog"), Create("/Index"), Create("Index")); - - return data; - } - } - - private static ReflectedAttributeRouteModel Create(string template) + private static ReflectedAttributeRouteModel Create(string template, int? order = null) { return new ReflectedAttributeRouteModel { - Template = template + Template = template, + Order = order }; } } diff --git a/test/Microsoft.AspNet.Mvc.Core.Test/RouteTemplateProviderAttributesTests.cs b/test/Microsoft.AspNet.Mvc.Core.Test/RouteTemplateProviderAttributesTests.cs new file mode 100644 index 0000000000..fdc72b161e --- /dev/null +++ b/test/Microsoft.AspNet.Mvc.Core.Test/RouteTemplateProviderAttributesTests.cs @@ -0,0 +1,32 @@ +using Microsoft.AspNet.Mvc.Routing; +using Xunit; + +namespace Microsoft.AspNet.Mvc +{ + public class RouteTemplateProviderAttributesTest + { + [Theory] + [MemberData("RouteTemplateProvidersTestData")] + public void Order_Defaults_ToNull(IRouteTemplateProvider routeTemplateProvider) + { + // Act & Assert + Assert.Null(routeTemplateProvider.Order); + } + + public static TheoryData RouteTemplateProvidersTestData + { + get + { + var data = new TheoryData(); + data.Add(new HttpGetAttribute()); + data.Add(new HttpPostAttribute()); + data.Add(new HttpPutAttribute()); + data.Add(new HttpPatchAttribute()); + data.Add(new HttpDeleteAttribute()); + data.Add(new RouteAttribute("")); + + return data; + } + } + } +} \ No newline at end of file diff --git a/test/Microsoft.AspNet.Mvc.Core.Test/Routing/AttributeRouteTests.cs b/test/Microsoft.AspNet.Mvc.Core.Test/Routing/AttributeRouteTests.cs index 19afc71eea..a23054ff22 100644 --- a/test/Microsoft.AspNet.Mvc.Core.Test/Routing/AttributeRouteTests.cs +++ b/test/Microsoft.AspNet.Mvc.Core.Test/Routing/AttributeRouteTests.cs @@ -13,11 +13,372 @@ using Microsoft.Framework.OptionsModel; using Microsoft.Framework.Logging; using Moq; using Xunit; +using Microsoft.AspNet.PipelineCore; namespace Microsoft.AspNet.Mvc.Routing { - public class AttributeRouteTests + public class AttributeRouteTest { + [Theory] + [InlineData("template/5", "template/{parameter:int}")] + [InlineData("template/5", "template/{parameter}")] + [InlineData("template/5", "template/{*parameter:int}")] + [InlineData("template/5", "template/{*parameter}")] + [InlineData("template/{parameter:int}", "template/{parameter}")] + [InlineData("template/{parameter:int}", "template/{*parameter:int}")] + [InlineData("template/{parameter:int}", "template/{*parameter}")] + [InlineData("template/{parameter}", "template/{*parameter:int}")] + [InlineData("template/{parameter}", "template/{*parameter}")] + [InlineData("template/{*parameter:int}", "template/{*parameter}")] + public async Task AttributeRoute_RouteAsync_RespectsPrecedence( + string firstTemplate, + string secondTemplate) + { + // Arrange + var expectedRouteGroup = string.Format("{0}&&{1}", 0, firstTemplate); + + // We need to force the creation of a closure in order to avoid an issue with Moq and Roslyn. + var numberOfCalls = 0; + Action callBack = ctx => { ctx.IsHandled = true; numberOfCalls++; }; + + var next = new Mock(); + next.Setup(r => r.RouteAsync(It.IsAny())) + .Callback(callBack) + .Returns(Task.FromResult(true)) + .Verifiable(); + + var firstRoute = CreateMatchingEntry(next.Object, firstTemplate, order: 0); + var secondRoute = CreateMatchingEntry(next.Object, secondTemplate, order: 0); + + // We setup the route entries in reverse order of precedence to ensure that when we + // try to route the request, the route with a higher precedence gets tried first. + var matchingRoutes = new[] { secondRoute, firstRoute }; + + var linkGenerationEntries = Enumerable.Empty(); + + var route = new AttributeRoute(next.Object, matchingRoutes, linkGenerationEntries, NullLoggerFactory.Instance); + + var context = CreateRouteContext("/template/5"); + + // Act + await route.RouteAsync(context); + + // Assert + Assert.Equal(expectedRouteGroup, context.RouteData.Values["test_route_group"]); + } + + [Theory] + [InlineData("template/5", "template/{parameter:int}")] + [InlineData("template/5", "template/{parameter}")] + [InlineData("template/5", "template/{*parameter:int}")] + [InlineData("template/5", "template/{*parameter}")] + [InlineData("template/{parameter:int}", "template/{parameter}")] + [InlineData("template/{parameter:int}", "template/{*parameter:int}")] + [InlineData("template/{parameter:int}", "template/{*parameter}")] + [InlineData("template/{parameter}", "template/{*parameter:int}")] + [InlineData("template/{parameter}", "template/{*parameter}")] + [InlineData("template/{*parameter:int}", "template/{*parameter}")] + public async Task AttributeRoute_RouteAsync_RespectsOrderOverPrecedence( + string firstTemplate, + string secondTemplate) + { + // Arrange + var expectedRouteGroup = string.Format("{0}&&{1}", 0, secondTemplate); + + // We need to force the creation of a closure in order to avoid an issue with Moq and Roslyn. + var numberOfCalls = 0; + Action callBack = ctx => { ctx.IsHandled = true; numberOfCalls++; }; + + var next = new Mock(); + next.Setup(r => r.RouteAsync(It.IsAny())) + .Callback(callBack) + .Returns(Task.FromResult(true)) + .Verifiable(); + + var firstRoute = CreateMatchingEntry(next.Object, firstTemplate, order: 1); + var secondRoute = CreateMatchingEntry(next.Object, secondTemplate, order: 0); + + // We setup the route entries with a lower relative order and higher relative precedence + // first to ensure that when we try to route the request, the route with the higher + // relative order gets tried first. + var matchingRoutes = new[] { firstRoute, secondRoute }; + + var linkGenerationEntries = Enumerable.Empty(); + + var route = new AttributeRoute(next.Object, matchingRoutes, linkGenerationEntries, NullLoggerFactory.Instance); + + var context = CreateRouteContext("/template/5"); + + // Act + await route.RouteAsync(context); + + // Assert + Assert.Equal(expectedRouteGroup, context.RouteData.Values["test_route_group"]); + } + + [Theory] + [InlineData("template/5")] + [InlineData("template/{parameter:int}")] + [InlineData("template/{parameter}")] + [InlineData("template/{*parameter:int}")] + [InlineData("template/{*parameter}")] + public async Task AttributeRoute_RouteAsync_RespectsOrder(string template) + { + // Arrange + var expectedRouteGroup = string.Format("{0}&&{1}", 0, template); + + // We need to force the creation of a closure in order to avoid an issue with Moq and Roslyn. + var numberOfCalls = 0; + Action callBack = ctx => { ctx.IsHandled = true; numberOfCalls++; }; + + var next = new Mock(); + next.Setup(r => r.RouteAsync(It.IsAny())) + .Callback(callBack) + .Returns(Task.FromResult(true)) + .Verifiable(); + + var firstRoute = CreateMatchingEntry(next.Object, template, order: 1); + var secondRoute = CreateMatchingEntry(next.Object, template, order: 0); + + // We setup the route entries with a lower relative order first to ensure that when + // we try to route the request, the route with the higher relative order gets tried first. + var matchingRoutes = new[] { firstRoute, secondRoute }; + + var linkGenerationEntries = Enumerable.Empty(); + + var route = new AttributeRoute(next.Object, matchingRoutes, linkGenerationEntries, NullLoggerFactory.Instance); + + var context = CreateRouteContext("/template/5"); + + // Act + await route.RouteAsync(context); + + // Assert + Assert.Equal(expectedRouteGroup, context.RouteData.Values["test_route_group"]); + } + + [Theory] + [InlineData("template/{first:int}", "template/{second:int}")] + [InlineData("template/{first}", "template/{second}")] + [InlineData("template/{*first:int}", "template/{*second:int}")] + [InlineData("template/{*first}", "template/{*second}")] + public async Task AttributeRoute_RouteAsync_EnsuresStableOrdering(string first, string second) + { + // Arrange + var expectedRouteGroup = string.Format("{0}&&{1}", 0, first); + + // We need to force the creation of a closure in order to avoid an issue with Moq and Roslyn. + var numberOfCalls = 0; + Action callBack = ctx => { ctx.IsHandled = true; numberOfCalls++; }; + + var next = new Mock(); + next.Setup(r => r.RouteAsync(It.IsAny())) + .Callback(callBack) + .Returns(Task.FromResult(true)) + .Verifiable(); + + var secondRouter = new Mock(MockBehavior.Strict); + + var firstRoute = CreateMatchingEntry(next.Object, first, order: 0); + var secondRoute = CreateMatchingEntry(next.Object, second, order: 0); + + // We setup the route entries with a lower relative template order first to ensure that when + // we try to route the request, the route with the higher template order gets tried first. + var matchingRoutes = new[] { secondRoute, firstRoute }; + + var linkGenerationEntries = Enumerable.Empty(); + + var route = new AttributeRoute(next.Object, matchingRoutes, linkGenerationEntries, NullLoggerFactory.Instance); + + var context = CreateRouteContext("/template/5"); + + // Act + await route.RouteAsync(context); + + // Assert + Assert.Equal(expectedRouteGroup, context.RouteData.Values["test_route_group"]); + } + + [Theory] + [InlineData("template/5", "template/{parameter:int}")] + [InlineData("template/5", "template/{parameter}")] + [InlineData("template/5", "template/{*parameter:int}")] + [InlineData("template/5", "template/{*parameter}")] + [InlineData("template/{parameter:int}", "template/{parameter}")] + [InlineData("template/{parameter:int}", "template/{*parameter:int}")] + [InlineData("template/{parameter:int}", "template/{*parameter}")] + [InlineData("template/{parameter}", "template/{*parameter:int}")] + [InlineData("template/{parameter}", "template/{*parameter}")] + [InlineData("template/{*parameter:int}", "template/{*parameter}")] + public void AttributeRoute_GenerateLink_RespectsPrecedence(string firstTemplate, string secondTemplate) + { + // Arrange + var expectedGroup = CreateRouteGroup(0, firstTemplate); + + string selectedGroup = null; + + var next = new Mock(); + next.Setup(n => n.GetVirtualPath(It.IsAny())).Callback(ctx => + { + selectedGroup = (string)ctx.ProvidedValues[AttributeRouting.RouteGroupKey]; + ctx.IsBound = true; + }) + .Returns((string)null); + + var matchingRoutes = Enumerable.Empty(); + + var firstEntry = CreateGenerationEntry(firstTemplate, requiredValues: null); + var secondEntry = CreateGenerationEntry(secondTemplate, requiredValues: null, order: 0); + + // We setup the route entries in reverse order of precedence to ensure that when we + // try to generate a link, the route with a higher precedence gets tried first. + var linkGenerationEntries = new[] { secondEntry, firstEntry }; + + var route = new AttributeRoute(next.Object, matchingRoutes, linkGenerationEntries, NullLoggerFactory.Instance); + + var context = CreateVirtualPathContext(values: null, ambientValues: new { parameter = 5 }); + + // Act + string result = route.GetVirtualPath(context); + + // Assert + Assert.NotNull(result); + Assert.Equal("template/5", result); + Assert.Equal(expectedGroup, selectedGroup); + } + + [Theory] + [InlineData("template/5", "template/{parameter:int}")] + [InlineData("template/5", "template/{parameter}")] + [InlineData("template/5", "template/{*parameter:int}")] + [InlineData("template/5", "template/{*parameter}")] + [InlineData("template/{parameter:int}", "template/{parameter}")] + [InlineData("template/{parameter:int}", "template/{*parameter:int}")] + [InlineData("template/{parameter:int}", "template/{*parameter}")] + [InlineData("template/{parameter}", "template/{*parameter:int}")] + [InlineData("template/{parameter}", "template/{*parameter}")] + [InlineData("template/{*parameter:int}", "template/{*parameter}")] + public void AttributeRoute_GenerateLink_RespectsOrderOverPrecedence(string firstTemplate, string secondTemplate) + { + // Arrange + var selectedGroup = CreateRouteGroup(0, secondTemplate); + + string firstRouteGroupSelected = null; + var next = new Mock(); + next.Setup(n => n.GetVirtualPath(It.IsAny())).Callback(ctx => + { + firstRouteGroupSelected = (string)ctx.ProvidedValues[AttributeRouting.RouteGroupKey]; + ctx.IsBound = true; + }) + .Returns((string)null); + + var matchingRoutes = Enumerable.Empty(); + + var firstRoute = CreateGenerationEntry(firstTemplate, requiredValues: null, order: 1); + var secondRoute = CreateGenerationEntry(secondTemplate, requiredValues: null, order: 0); + + // We setup the route entries with a lower relative order and higher relative precedence + // first to ensure that when we try to generate a link, the route with the higher + // relative order gets tried first. + var linkGenerationEntries = new[] { firstRoute, secondRoute }; + + var route = new AttributeRoute(next.Object, matchingRoutes, linkGenerationEntries, NullLoggerFactory.Instance); + + var context = CreateVirtualPathContext(null, ambientValues: new { parameter = 5 }); + + // Act + string result = route.GetVirtualPath(context); + + // Assert + Assert.NotNull(result); + Assert.Equal("template/5", result); + Assert.Equal(selectedGroup, firstRouteGroupSelected); + } + + [Theory] + [InlineData("template/5", "template/5")] + [InlineData("template/{first:int}", "template/{second:int}")] + [InlineData("template/{first}", "template/{second}")] + [InlineData("template/{*first:int}", "template/{*second:int}")] + [InlineData("template/{*first}", "template/{*second}")] + public void AttributeRoute_GenerateLink_RespectsOrder(string firstTemplate, string secondTemplate) + { + // Arrange + var expectedGroup = CreateRouteGroup(0, secondTemplate); + + var next = new Mock(); + string selectedGroup = null; + next.Setup(n => n.GetVirtualPath(It.IsAny())).Callback(ctx => + { + selectedGroup = (string)ctx.ProvidedValues[AttributeRouting.RouteGroupKey]; + ctx.IsBound = true; + }) + .Returns((string)null); + + var matchingRoutes = Enumerable.Empty(); + + var firstRoute = CreateGenerationEntry(firstTemplate, requiredValues: null, order: 1); + var secondRoute = CreateGenerationEntry(secondTemplate, requiredValues: null, order: 0); + + // We setup the route entries with a lower relative order first to ensure that when + // we try to generate a link, the route with the higher relative order gets tried first. + var linkGenerationEntries = new[] { firstRoute, secondRoute }; + + var route = new AttributeRoute(next.Object, matchingRoutes, linkGenerationEntries, NullLoggerFactory.Instance); + + var context = CreateVirtualPathContext(values: null, ambientValues: new { first = 5, second = 5 }); + + // Act + string result = route.GetVirtualPath(context); + + // Assert + Assert.NotNull(result); + Assert.Equal("template/5", result); + Assert.Equal(expectedGroup, selectedGroup); + } + + [Theory] + [InlineData("first/5", "second/5")] + [InlineData("first/{first:int}", "second/{second:int}")] + [InlineData("first/{first}", "second/{second}")] + [InlineData("first/{*first:int}", "second/{*second:int}")] + [InlineData("first/{*first}", "second/{*second}")] + public void AttributeRoute_GenerateLink_EnsuresStableOrder(string firstTemplate, string secondTemplate) + { + // Arrange + var expectedGroup = CreateRouteGroup(0, firstTemplate); + + var next = new Mock(); + string selectedGroup = null; + next.Setup(n => n.GetVirtualPath(It.IsAny())).Callback(ctx => + { + selectedGroup = (string)ctx.ProvidedValues[AttributeRouting.RouteGroupKey]; + ctx.IsBound = true; + }) + .Returns((string)null); + + var matchingRoutes = Enumerable.Empty(); + + var firstRoute = CreateGenerationEntry(firstTemplate, requiredValues: null, order: 0); + var secondRoute = CreateGenerationEntry(secondTemplate, requiredValues: null, order: 0); + + // We setup the route entries with a lower relative template order first to ensure that when + // we try to generate a link, the route with the higher template order gets tried first. + var linkGenerationEntries = new[] { secondRoute, firstRoute }; + + var route = new AttributeRoute(next.Object, matchingRoutes, linkGenerationEntries, NullLoggerFactory.Instance); + + var context = CreateVirtualPathContext(values: null, ambientValues: new { first = 5, second = 5 }); + + // Act + string result = route.GetVirtualPath(context); + + // Assert + Assert.NotNull(result); + Assert.Equal("first/5", result); + Assert.Equal(expectedGroup, selectedGroup); + } + [Fact] public async void AttributeRoute_RouteAsyncHandled_LogsCorrectValues() { @@ -55,7 +416,7 @@ namespace Microsoft.AspNet.Mvc.Routing Assert.Equal(true, values.Handled); } - [Fact] + [Fact] public async void AttributeRoute_RouteAsyncNotHandled_LogsCorrectValues() { // Arrange @@ -291,7 +652,7 @@ namespace Microsoft.AspNet.Mvc.Routing var entry = CreateGenerationEntry("api/Store", new { action = "Index", controller = "Store" }); var route = CreateAttributeRoute(entry); - var context = CreateVirtualPathContext(new { action = "Index", id = 5}, new { controller = "Store" }); + var context = CreateVirtualPathContext(new { action = "Index", id = 5 }, new { controller = "Store" }); // Act var path = route.GetVirtualPath(context); @@ -356,7 +717,7 @@ namespace Microsoft.AspNet.Mvc.Routing // Reject entry 1. callCount++; return !c.ProvidedValues.Contains(new KeyValuePair( - AttributeRouting.RouteGroupKey, + AttributeRouting.RouteGroupKey, entry1.RouteGroup)); }; @@ -489,16 +850,34 @@ namespace Microsoft.AspNet.Mvc.Routing .Returns(NullLoggerFactory.Instance); return new VirtualPathContext( - mockHttpContext.Object, - new RouteValueDictionary(ambientValues), + mockHttpContext.Object, + new RouteValueDictionary(ambientValues), new RouteValueDictionary(values)); } - private static AttributeRouteLinkGenerationEntry CreateGenerationEntry(string template, object requiredValues) + private static AttributeRouteMatchingEntry CreateMatchingEntry(IRouter router, string template, int order) + { + var constraintResolver = CreateConstraintResolver(); + + var routeTemplate = TemplateParser.Parse(template, constraintResolver); + + var entry = new AttributeRouteMatchingEntry(); + entry.Route = new TemplateRoute(router, template, constraintResolver); + entry.Precedence = AttributeRoutePrecedence.Compute(routeTemplate); + entry.Order = order; + + string routeGroup = string.Format("{0}&&{1}", order, template); + entry.Route.Defaults.Add("test_route_group", routeGroup); + + return entry; + } + + private static AttributeRouteLinkGenerationEntry CreateGenerationEntry(string template, object requiredValues, int order = 0) { var constraintResolver = CreateConstraintResolver(); var entry = new AttributeRouteLinkGenerationEntry(); + entry.TemplateText = template; entry.Template = TemplateParser.Parse(template, constraintResolver); var defaults = entry.Template.Parameters @@ -512,9 +891,10 @@ namespace Microsoft.AspNet.Mvc.Routing entry.Constraints = constraints; entry.Defaults = defaults; entry.Binder = new TemplateBinder(entry.Template, defaults); + entry.Order = order; entry.Precedence = AttributeRoutePrecedence.Compute(entry.Template); entry.RequiredLinkValues = new RouteValueDictionary(requiredValues); - entry.RouteGroup = template; + entry.RouteGroup = CreateRouteGroup(order, template); return entry; } @@ -543,6 +923,11 @@ namespace Microsoft.AspNet.Mvc.Routing return entry; } + private static string CreateRouteGroup(int order, string template) + { + return string.Format("{0}&{1}", order, template); + } + private static DefaultInlineConstraintResolver CreateConstraintResolver() { var services = Mock.Of(); diff --git a/test/Microsoft.AspNet.Mvc.FunctionalTests/RoutingTests.cs b/test/Microsoft.AspNet.Mvc.FunctionalTests/RoutingTests.cs index e6495a539a..b4b4c44661 100644 --- a/test/Microsoft.AspNet.Mvc.FunctionalTests/RoutingTests.cs +++ b/test/Microsoft.AspNet.Mvc.FunctionalTests/RoutingTests.cs @@ -76,7 +76,7 @@ namespace Microsoft.AspNet.Mvc.FunctionalTests { { "controller", "Home" }, { "action", "Index" }, - }, + }, result.RouteValues); } @@ -360,7 +360,83 @@ namespace Microsoft.AspNet.Mvc.FunctionalTests result.RouteValues); } - [Fact] + [Fact] + public async Task AttributeRoutedAction_OverrideActionOverridesOrderOnController() + { + // Arrange + var server = TestServer.Create(_services, _app); + var client = server.Handler; + + // Act + var response = await client.GetAsync("http://localhost/Team/5"); + + // Assert + Assert.Equal(200, response.StatusCode); + + var body = await response.ReadBodyAsStringAsync(); + var result = JsonConvert.DeserializeObject(body); + + Assert.Contains("/Team/5", result.ExpectedUrls); + Assert.Equal("Team", result.Controller); + Assert.Equal("GetOrganization", result.Action); + + Assert.Contains( + new KeyValuePair("teamId", "5"), + result.RouteValues); + } + + [Fact] + public async Task AttributeRoutedAction_OrderOnActionOverridesOrderOnController() + { + // Arrange + var server = TestServer.Create(_services, _app); + var client = server.Handler; + + // Act + var response = await client.GetAsync("http://localhost/Teams"); + + // Assert + Assert.Equal(200, response.StatusCode); + + var body = await response.ReadBodyAsStringAsync(); + var result = JsonConvert.DeserializeObject(body); + + Assert.Contains("/Teams", result.ExpectedUrls); + Assert.Equal("Team", result.Controller); + Assert.Equal("GetOrganizations", result.Action); + } + + [Fact] + public async Task AttributeRoutedAction_LinkGeneration_OverrideActionOverridesOrderOnController() + { + // Arrange + var server = TestServer.Create(_services, _app); + var client = server.Handler; + + // Act + var response = await client.GetStringAsync("http://localhost/Organization/5"); + + // Assert + Assert.NotNull(response); + Assert.Equal("/Club/5", response); + } + + [Fact] + public async Task AttributeRoutedAction__LinkGeneration_OrderOnActionOverridesOrderOnController() + { + // Arrange + var server = TestServer.Create(_services, _app); + var client = server.Handler; + + // Act + var response = await client.GetStringAsync("http://localhost/Teams/AllTeams"); + + // Assert + Assert.NotNull(response); + Assert.Equal("/Teams/AllOrganizations", response); + } + + [Fact] public async Task AttributeRoutedAction_LinkToSelf() { // Arrange diff --git a/test/WebSites/RoutingWebSite/Controllers/TeamController.cs b/test/WebSites/RoutingWebSite/Controllers/TeamController.cs new file mode 100644 index 0000000000..85989830c8 --- /dev/null +++ b/test/WebSites/RoutingWebSite/Controllers/TeamController.cs @@ -0,0 +1,64 @@ +using Microsoft.AspNet.Mvc; +using System; + +namespace RoutingWebSite +{ + [Route("/Teams", Order = 1)] + public class TeamController : Controller + { + private readonly TestResponseGenerator _generator; + + public TeamController(TestResponseGenerator generator) + { + _generator = generator; + } + + [HttpGet("/Team/{teamId}", Order = 2)] + public ActionResult GetTeam(int teamId) + { + return _generator.Generate("/Team/" + teamId); + } + + [HttpGet("/Team/{teamId}")] + public ActionResult GetOrganization(int teamId) + { + return _generator.Generate("/Team/" + teamId); + } + + [HttpGet("")] + public ActionResult GetTeams() + { + return _generator.Generate("/Teams"); + } + + [HttpGet("", Order = 0)] + public ActionResult GetOrganizations() + { + return _generator.Generate("/Teams"); + } + + [HttpGet("/Club/{clubId?}")] + public ActionResult GetClub() + { + return Content(Url.Action(),"text/plain"); + } + + [HttpGet("/Organization/{clubId?}", Order = 1)] + public ActionResult GetClub(int clubId) + { + return Content(Url.Action(), "text/plain"); + } + + [HttpGet("AllTeams")] + public ActionResult GetAllTeams() + { + return Content(Url.Action(), "text/plain"); + } + + [HttpGet("AllOrganizations", Order = 0)] + public ActionResult GetAllTeams(int notRelevant) + { + return Content(Url.Action(), "text/plain"); + } + } +} \ No newline at end of file diff --git a/test/WebSites/RoutingWebSite/HttpMergeAttribute.cs b/test/WebSites/RoutingWebSite/HttpMergeAttribute.cs index 9c10c7118a..d497aea828 100644 --- a/test/WebSites/RoutingWebSite/HttpMergeAttribute.cs +++ b/test/WebSites/RoutingWebSite/HttpMergeAttribute.cs @@ -24,6 +24,10 @@ namespace RoutingWebSite get { return _supportedMethods; } } + /// public string Template { get; private set; } + + /// + public int? Order { get; set; } } } \ No newline at end of file diff --git a/test/WebSites/RoutingWebSite/RoutingWebSite.kproj b/test/WebSites/RoutingWebSite/RoutingWebSite.kproj index 8eef3bf9bd..a7ccec35b7 100644 --- a/test/WebSites/RoutingWebSite/RoutingWebSite.kproj +++ b/test/WebSites/RoutingWebSite/RoutingWebSite.kproj @@ -36,6 +36,7 @@ +