From 6875ee55d3c8632a92432b4b80b139f293f14804 Mon Sep 17 00:00:00 2001 From: Ryan Nowak Date: Tue, 24 Nov 2015 10:45:40 -0800 Subject: [PATCH] Remove Magic Link Generation This change resolves #3512 and #3636 by removing 'magic' link generation and adding an extension method to add routes to areas correctly using the new pattern. This is pretty much exactly the same as how MapWebApiRoute works. For site authors, we recommend adding area-specific routes in a way that includes a default AND constraint for the area. Put your most specific (for link generation) routes FIRST. Ex: routes.MapRoute( "Admin/{controller}/{action}/{id?}", defaults: new { area = "Admin" }, constraints: new { area = "Admin" }); The bulk of the changes here are to tests that unwittingly relied on the old behavior. --- .../Builder/MvcAreaRouteBuilderExtensions.cs | 140 ++++++++++ .../Infrastructure/MvcRouteHandler.cs | 6 +- .../MvcAreaRouteBuilderExtensionsTest.cs | 258 ++++++++++++++++++ .../InlineConstraintTests.cs | 9 +- .../PrecompilationTest.cs | 2 +- .../RoutingTests.cs | 12 +- ...ite.HtmlGeneration_Home.Index.Encoded.html | 22 +- ...tionWebSite.HtmlGeneration_Home.Index.html | 22 +- .../WebSites/HtmlGenerationWebSite/Startup.cs | 8 +- .../Views/HtmlGeneration_Home/Index.cshtml | 8 +- test/WebSites/RoutingWebSite/Startup.cs | 15 +- .../WebApiCompatShimWebSite/Startup.cs | 6 +- 12 files changed, 453 insertions(+), 55 deletions(-) create mode 100644 src/Microsoft.AspNet.Mvc.Core/Builder/MvcAreaRouteBuilderExtensions.cs create mode 100644 test/Microsoft.AspNet.Mvc.Core.Test/Builder/MvcAreaRouteBuilderExtensionsTest.cs diff --git a/src/Microsoft.AspNet.Mvc.Core/Builder/MvcAreaRouteBuilderExtensions.cs b/src/Microsoft.AspNet.Mvc.Core/Builder/MvcAreaRouteBuilderExtensions.cs new file mode 100644 index 0000000000..59901c303d --- /dev/null +++ b/src/Microsoft.AspNet.Mvc.Core/Builder/MvcAreaRouteBuilderExtensions.cs @@ -0,0 +1,140 @@ +// Copyright (c) .NET Foundation. All rights reserved. +// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. + +using System; +using Microsoft.AspNet.Mvc.Core; +using Microsoft.AspNet.Routing; + +namespace Microsoft.AspNet.Builder +{ + /// + /// Extension methods for . + /// + public static class MvcAreaRouteBuilderExtensions + { + /// + /// Adds a route to the with the given MVC area with the specified + /// , and . + /// + /// The to add the route to. + /// The name of the route. + /// The MVC area name. + /// The URL pattern of the route. + /// A reference to this instance after the operation has completed. + public static IRouteBuilder MapAreaRoute( + this IRouteBuilder routeBuilder, + string name, + string areaName, + string template) + { + MapAreaRoute(routeBuilder, name, areaName, template, defaults: null, constraints: null, dataTokens: null); + return routeBuilder; + } + + /// + /// Adds a route to the with the given MVC area with the specified + /// , , , and + /// . + /// + /// The to add the route to. + /// The name of the route. + /// The MVC area name. + /// The URL pattern of the route. + /// + /// An object that contains default values for route parameters. The object's properties represent the + /// names and values of the default values. + /// + /// A reference to this instance after the operation has completed. + public static IRouteBuilder MapAreaRoute( + this IRouteBuilder routeBuilder, + string name, + string areaName, + string template, + object defaults) + { + MapAreaRoute(routeBuilder, name, areaName, template, defaults, constraints: null, dataTokens: null); + return routeBuilder; + } + + /// + /// Adds a route to the with the given MVC area with the specified + /// , , , + /// , and . + /// + /// The to add the route to. + /// The name of the route. + /// The MVC area name. + /// The URL pattern of the route. + /// + /// An object that contains default values for route parameters. The object's properties represent the + /// names and values of the default values. + /// + /// + /// An object that contains constraints for the route. The object's properties represent the names and + /// values of the constraints. + /// + /// A reference to this instance after the operation has completed. + public static IRouteBuilder MapAreaRoute( + this IRouteBuilder routeBuilder, + string name, + string areaName, + string template, + object defaults, + object constraints) + { + MapAreaRoute(routeBuilder, name, areaName, template, defaults, constraints, dataTokens: null); + return routeBuilder; + } + + /// + /// Adds a route to the with the given MVC area with the specified + /// , , , + /// , , and . + /// + /// The to add the route to. + /// The name of the route. + /// The MVC area name. + /// The URL pattern of the route. + /// + /// An object that contains default values for route parameters. The object's properties represent the + /// names and values of the default values. + /// + /// + /// An object that contains constraints for the route. The object's properties represent the names and + /// values of the constraints. + /// + /// + /// An object that contains data tokens for the route. The object's properties represent the names and + /// values of the data tokens. + /// + /// A reference to this instance after the operation has completed. + public static IRouteBuilder MapAreaRoute( + this IRouteBuilder routeBuilder, + string name, + string areaName, + string template, + object defaults, + object constraints, + object dataTokens) + { + if (routeBuilder == null) + { + throw new ArgumentNullException(nameof(routeBuilder)); + } + + if (string.IsNullOrEmpty(areaName)) + { + throw new ArgumentException(Resources.ArgumentCannotBeNullOrEmpty, nameof(areaName)); + } + + var defaultsDictionary = new RouteValueDictionary(defaults); + defaultsDictionary["area"] = areaName; + + var constraintsDictionary = new RouteValueDictionary(constraints); + constraintsDictionary["area"] = areaName; + + routeBuilder.MapRoute(name, template, defaultsDictionary, constraintsDictionary, dataTokens); + return routeBuilder; + } + } +} diff --git a/src/Microsoft.AspNet.Mvc.Core/Infrastructure/MvcRouteHandler.cs b/src/Microsoft.AspNet.Mvc.Core/Infrastructure/MvcRouteHandler.cs index b813756d35..97e869e2f6 100644 --- a/src/Microsoft.AspNet.Mvc.Core/Infrastructure/MvcRouteHandler.cs +++ b/src/Microsoft.AspNet.Mvc.Core/Infrastructure/MvcRouteHandler.cs @@ -32,11 +32,7 @@ namespace Microsoft.AspNet.Mvc.Infrastructure throw new ArgumentNullException(nameof(context)); } - EnsureServices(context.Context); - - // The contract of this method is to check that the values coming in from the route are valid; - // that they match an existing action, setting IsBound = true if the values are OK. - context.IsBound = _actionSelector.HasValidAction(context); + context.IsBound = true; // We return null here because we're not responsible for generating the url, the route is. return null; diff --git a/test/Microsoft.AspNet.Mvc.Core.Test/Builder/MvcAreaRouteBuilderExtensionsTest.cs b/test/Microsoft.AspNet.Mvc.Core.Test/Builder/MvcAreaRouteBuilderExtensionsTest.cs new file mode 100644 index 0000000000..62d181fcfd --- /dev/null +++ b/test/Microsoft.AspNet.Mvc.Core.Test/Builder/MvcAreaRouteBuilderExtensionsTest.cs @@ -0,0 +1,258 @@ +// Copyright (c) .NET Foundation. 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 System.Linq; +using Microsoft.AspNet.Routing; +using Microsoft.AspNet.Routing.Constraints; +using Microsoft.AspNet.Routing.Template; +using Microsoft.Extensions.DependencyInjection; +using Moq; +using Xunit; + +namespace Microsoft.AspNet.Builder +{ + public class MvcAreaRouteBuilderExtensionsTest + { + [Fact] + public void MapAreaRoute_Simple() + { + // Arrange + var builder = new RouteBuilder() + { + DefaultHandler = Mock.Of(), + ServiceProvider = CreateServices(), + }; + + // Act + builder.MapAreaRoute(name: null, areaName: "admin", template: "site/Admin/"); + + // Assert + var route = Assert.IsType((Assert.Single(builder.Routes))); + + Assert.Null(route.Name); + Assert.Equal("site/Admin/", route.RouteTemplate); + Assert.Collection( + route.Constraints.OrderBy(kvp => kvp.Key), + kvp => + { + Assert.Equal(kvp.Key, "area"); + Assert.IsType(kvp.Value); + }); + Assert.Empty(route.DataTokens); + Assert.Collection( + route.Defaults.OrderBy(kvp => kvp.Key), + kvp => + { + Assert.Equal(kvp.Key, "area"); + Assert.Equal(kvp.Value, "admin"); + }); + } + + [Fact] + public void MapAreaRoute_Defaults() + { + // Arrange + var builder = new RouteBuilder() + { + DefaultHandler = Mock.Of(), + ServiceProvider = CreateServices(), + }; + + // Act + builder.MapAreaRoute( + name: "admin_area", + areaName: "admin", + template: "site/Admin/", + defaults: new { action = "Home" }); + + // Assert + var route = Assert.IsType((Assert.Single(builder.Routes))); + + Assert.Equal("admin_area", route.Name); + Assert.Equal("site/Admin/", route.RouteTemplate); + Assert.Collection( + route.Constraints.OrderBy(kvp => kvp.Key), + kvp => + { + Assert.Equal(kvp.Key, "area"); + Assert.IsType(kvp.Value); + }); + Assert.Empty(route.DataTokens); + Assert.Collection( + route.Defaults.OrderBy(kvp => kvp.Key), + kvp => + { + Assert.Equal(kvp.Key, "action"); + Assert.Equal(kvp.Value, "Home"); + }, + kvp => + { + Assert.Equal(kvp.Key, "area"); + Assert.Equal(kvp.Value, "admin"); + }); + } + + [Fact] + public void MapAreaRoute_DefaultsAndConstraints() + { + // Arrange + var builder = new RouteBuilder() + { + DefaultHandler = Mock.Of(), + ServiceProvider = CreateServices(), + }; + + // Act + builder.MapAreaRoute( + name: "admin_area", + areaName: "admin", + template: "site/Admin/", + defaults: new { action = "Home" }, + constraints: new { id = new IntRouteConstraint() }); + + // Assert + var route = Assert.IsType((Assert.Single(builder.Routes))); + + Assert.Equal("admin_area", route.Name); + Assert.Equal("site/Admin/", route.RouteTemplate); + Assert.Collection( + route.Constraints.OrderBy(kvp => kvp.Key), + kvp => + { + Assert.Equal(kvp.Key, "area"); + Assert.IsType(kvp.Value); + }, + kvp => + { + Assert.Equal(kvp.Key, "id"); + Assert.IsType(kvp.Value); + }); + Assert.Empty(route.DataTokens); + Assert.Collection( + route.Defaults.OrderBy(kvp => kvp.Key), + kvp => + { + Assert.Equal(kvp.Key, "action"); + Assert.Equal(kvp.Value, "Home"); + }, + kvp => + { + Assert.Equal(kvp.Key, "area"); + Assert.Equal(kvp.Value, "admin"); + }); + } + + [Fact] + public void MapAreaRoute_DefaultsConstraintsAndDataTokens() + { + // Arrange + var builder = new RouteBuilder() + { + DefaultHandler = Mock.Of(), + ServiceProvider = CreateServices(), + }; + + // Act + builder.MapAreaRoute( + name: "admin_area", + areaName: "admin", + template: "site/Admin/", + defaults: new { action = "Home" }, + constraints: new { id = new IntRouteConstraint() }, + dataTokens: new { some_token = "hello" }); + + // Assert + var route = Assert.IsType((Assert.Single(builder.Routes))); + + Assert.Equal("admin_area", route.Name); + Assert.Equal("site/Admin/", route.RouteTemplate); + Assert.Collection( + route.Constraints.OrderBy(kvp => kvp.Key), + kvp => + { + Assert.Equal(kvp.Key, "area"); + Assert.IsType(kvp.Value); + }, + kvp => + { + Assert.Equal(kvp.Key, "id"); + Assert.IsType(kvp.Value); + }); + Assert.Collection( + route.DataTokens.OrderBy(kvp => kvp.Key), + kvp => + { + Assert.Equal(kvp.Key, "some_token"); + Assert.Equal(kvp.Value, "hello"); + }); + Assert.Collection( + route.Defaults.OrderBy(kvp => kvp.Key), + kvp => + { + Assert.Equal(kvp.Key, "action"); + Assert.Equal(kvp.Value, "Home"); + }, + kvp => + { + Assert.Equal(kvp.Key, "area"); + Assert.Equal(kvp.Value, "admin"); + }); + } + + [Fact] + public void MapAreaRoute_ReplacesValuesForArea() + { + // Arrange + var builder = new RouteBuilder() + { + DefaultHandler = Mock.Of(), + ServiceProvider = CreateServices(), + }; + + // Act + builder.MapAreaRoute( + name: "admin_area", + areaName: "admin", + template: "site/Admin/", + defaults: new { area = "Home" }, + constraints: new { area = new IntRouteConstraint() }, + dataTokens: new { some_token = "hello" }); + + // Assert + var route = Assert.IsType((Assert.Single(builder.Routes))); + + Assert.Equal("admin_area", route.Name); + Assert.Equal("site/Admin/", route.RouteTemplate); + Assert.Collection( + route.Constraints.OrderBy(kvp => kvp.Key), + kvp => + { + Assert.Equal(kvp.Key, "area"); + Assert.IsType(kvp.Value); + }); + Assert.Collection( + route.DataTokens.OrderBy(kvp => kvp.Key), + kvp => + { + Assert.Equal(kvp.Key, "some_token"); + Assert.Equal(kvp.Value, "hello"); + }); + Assert.Collection( + route.Defaults.OrderBy(kvp => kvp.Key), + kvp => + { + Assert.Equal(kvp.Key, "area"); + Assert.Equal(kvp.Value, "admin"); + }); + } + + private IServiceProvider CreateServices() + { + var services = new ServiceCollection(); + services.AddRouting(); + return services.BuildServiceProvider(); + } + } +} diff --git a/test/Microsoft.AspNet.Mvc.FunctionalTests/InlineConstraintTests.cs b/test/Microsoft.AspNet.Mvc.FunctionalTests/InlineConstraintTests.cs index 233ef31339..69cd67f780 100644 --- a/test/Microsoft.AspNet.Mvc.FunctionalTests/InlineConstraintTests.cs +++ b/test/Microsoft.AspNet.Mvc.FunctionalTests/InlineConstraintTests.cs @@ -408,7 +408,8 @@ namespace Microsoft.AspNet.Mvc.FunctionalTests "InlineConstraints_Products", "GetProductById", "id", - "sdsd", "" + "sdsd", + "/area-exists/InlineConstraints_Products/GetProductById?id=sdsd" }; // Attribute Route, name:alpha constraint @@ -458,7 +459,7 @@ namespace Microsoft.AspNet.Mvc.FunctionalTests "GetProductByCategoryId", "catId", "500", - "" + "/area-exists/InlineConstraints_Products/GetProductByCategoryId?catId=500" }; // Attribute Route, name:length(1,20)? constraint @@ -488,7 +489,7 @@ namespace Microsoft.AspNet.Mvc.FunctionalTests "GetProductByManufacturerId", "manId", "qwer", - "" + "/area-exists/InlineConstraints_Products/GetProductByManufacturerId?manId=qwer" }; // Attribute Route, manId:int:min(10)? constraint @@ -498,7 +499,7 @@ namespace Microsoft.AspNet.Mvc.FunctionalTests "GetProductByManufacturerId", "manId", "1", - "" + "/area-exists/InlineConstraints_Products/GetProductByManufacturerId?manId=1" }; // Attribute Route, dateTime:datetime constraint diff --git a/test/Microsoft.AspNet.Mvc.FunctionalTests/PrecompilationTest.cs b/test/Microsoft.AspNet.Mvc.FunctionalTests/PrecompilationTest.cs index deaaacd734..96a9099e92 100644 --- a/test/Microsoft.AspNet.Mvc.FunctionalTests/PrecompilationTest.cs +++ b/test/Microsoft.AspNet.Mvc.FunctionalTests/PrecompilationTest.cs @@ -75,7 +75,7 @@ namespace Microsoft.AspNet.Mvc.FunctionalTests @"Back to List"; + @"id=""Age"" name=""Age"" value="""" />Back to List"; // Act var response = await Client.GetStringAsync("http://localhost/TagHelpers/Add"); diff --git a/test/Microsoft.AspNet.Mvc.FunctionalTests/RoutingTests.cs b/test/Microsoft.AspNet.Mvc.FunctionalTests/RoutingTests.cs index 1b23fb489a..22403d710a 100644 --- a/test/Microsoft.AspNet.Mvc.FunctionalTests/RoutingTests.cs +++ b/test/Microsoft.AspNet.Mvc.FunctionalTests/RoutingTests.cs @@ -903,7 +903,7 @@ namespace Microsoft.AspNet.Mvc.FunctionalTests } [Fact] - public async Task ConventionalRoutedAction_InArea_ImplicitLeaveArea() + public async Task ConventionalRoutedAction_InArea_StaysInArea() { // Arrange var url = LinkFrom("http://localhost/Travel/Flight").To(new { action = "Contact", controller = "Home", }); @@ -919,7 +919,7 @@ namespace Microsoft.AspNet.Mvc.FunctionalTests Assert.Equal("Flight", result.Controller); Assert.Equal("Index", result.Action); - Assert.Equal("/Home/Contact", result.Link); + Assert.Equal("/Travel/Home/Contact", result.Link); } [Fact] @@ -985,7 +985,7 @@ namespace Microsoft.AspNet.Mvc.FunctionalTests } [Fact] - public async Task AttributeRoutedAction_InArea_ImplicitLeaveArea() + public async Task AttributeRoutedAction_InArea_StaysInArea_ActionDoesntExist() { // Arrange var url = LinkFrom("http://localhost/ContosoCorp/Trains") @@ -1002,7 +1002,7 @@ namespace Microsoft.AspNet.Mvc.FunctionalTests Assert.Equal("Rail", result.Controller); Assert.Equal("Index", result.Action); - Assert.Equal("/Home/Contact", result.Link); + Assert.Equal("/Travel/Home/Contact", result.Link); } [Fact] @@ -1158,7 +1158,7 @@ namespace Microsoft.AspNet.Mvc.FunctionalTests } [Fact] - public async Task ControllerWithCatchAll_GenerateLink_FailsWithoutCountry() + public async Task ControllerWithCatchAll_GenerateLink_FallsThroughWithoutCountry() { // Arrange var url = LinkFrom("http://localhost/") @@ -1170,7 +1170,7 @@ namespace Microsoft.AspNet.Mvc.FunctionalTests // Assert var body = await response.Content.ReadAsStringAsync(); var result = JsonConvert.DeserializeObject(body); - Assert.Null(result.Link); + Assert.Equal("/Products/GetProducts", result.Link); } [Theory] diff --git a/test/Microsoft.AspNet.Mvc.FunctionalTests/compiler/resources/HtmlGenerationWebSite.HtmlGeneration_Home.Index.Encoded.html b/test/Microsoft.AspNet.Mvc.FunctionalTests/compiler/resources/HtmlGenerationWebSite.HtmlGeneration_Home.Index.Encoded.html index 81a53c4d49..07ca814913 100644 --- a/test/Microsoft.AspNet.Mvc.FunctionalTests/compiler/resources/HtmlGenerationWebSite.HtmlGeneration_Home.Index.Encoded.html +++ b/test/Microsoft.AspNet.Mvc.FunctionalTests/compiler/resources/HtmlGenerationWebSite.HtmlGeneration_Home.Index.Encoded.html @@ -1,10 +1,10 @@ @@ -39,32 +39,32 @@
Non-existent Area diff --git a/test/Microsoft.AspNet.Mvc.FunctionalTests/compiler/resources/HtmlGenerationWebSite.HtmlGeneration_Home.Index.html b/test/Microsoft.AspNet.Mvc.FunctionalTests/compiler/resources/HtmlGenerationWebSite.HtmlGeneration_Home.Index.html index cdfb98dd1b..ea526da9a7 100644 --- a/test/Microsoft.AspNet.Mvc.FunctionalTests/compiler/resources/HtmlGenerationWebSite.HtmlGeneration_Home.Index.html +++ b/test/Microsoft.AspNet.Mvc.FunctionalTests/compiler/resources/HtmlGenerationWebSite.HtmlGeneration_Home.Index.html @@ -1,10 +1,10 @@ @@ -39,32 +39,32 @@
Non-existent Area diff --git a/test/WebSites/HtmlGenerationWebSite/Startup.cs b/test/WebSites/HtmlGenerationWebSite/Startup.cs index 12c56332d5..e3d1ae34ea 100644 --- a/test/WebSites/HtmlGenerationWebSite/Startup.cs +++ b/test/WebSites/HtmlGenerationWebSite/Startup.cs @@ -25,10 +25,6 @@ namespace HtmlGenerationWebSite app.UseMvc(routes => { - routes.MapRoute( - name: "default", - template: "{controller}/{action}/{id?}", - defaults: new { controller = "HtmlGeneration_Home", action = "Index" }); routes.MapRoute( name: "areaRoute", template: "{area:exists}/{controller}/{action}/{id?}", @@ -37,6 +33,10 @@ namespace HtmlGenerationWebSite name: "productRoute", template: "Product/{action}", defaults: new { controller = "Product" }); + routes.MapRoute( + name: "default", + template: "{controller}/{action}/{id?}", + defaults: new { controller = "HtmlGeneration_Home", action = "Index" }); }); } } diff --git a/test/WebSites/HtmlGenerationWebSite/Views/HtmlGeneration_Home/Index.cshtml b/test/WebSites/HtmlGenerationWebSite/Views/HtmlGeneration_Home/Index.cshtml index 1f47b429b9..cc90370e4b 100644 --- a/test/WebSites/HtmlGenerationWebSite/Views/HtmlGeneration_Home/Index.cshtml +++ b/test/WebSites/HtmlGenerationWebSite/Views/HtmlGeneration_Home/Index.cshtml @@ -7,10 +7,10 @@ diff --git a/test/WebSites/RoutingWebSite/Startup.cs b/test/WebSites/RoutingWebSite/Startup.cs index a4f35a7f5c..15b3f6170f 100644 --- a/test/WebSites/RoutingWebSite/Startup.cs +++ b/test/WebSites/RoutingWebSite/Startup.cs @@ -24,18 +24,21 @@ namespace RoutingWebSite app.UseMvc(routes => { - routes.MapRoute("areaRoute", - "{area:exists}/{controller}/{action}", - new { controller = "Home", action = "Index" }); - - routes.MapRoute("ActionAsMethod", "{controller}/{action}", - defaults: new { controller = "Home", action = "Index" }); + routes.MapRoute( + "areaRoute", + "{area:exists}/{controller}/{action}", + new { controller = "Home", action = "Index" }); routes.MapRoute( "products", "api/Products/{country}/{action}", defaults: new { controller = "Products" }); + routes.MapRoute( + "ActionAsMethod", + "{controller}/{action}", + defaults: new { controller = "Home", action = "Index" }); + // Added this route to validate that we throw an exception when a conventional // route matches a link generated by a named attribute route. // The conventional route will match first, but when the attribute route generates diff --git a/test/WebSites/WebApiCompatShimWebSite/Startup.cs b/test/WebSites/WebApiCompatShimWebSite/Startup.cs index 3a86f8427b..aaf6483151 100644 --- a/test/WebSites/WebApiCompatShimWebSite/Startup.cs +++ b/test/WebSites/WebApiCompatShimWebSite/Startup.cs @@ -23,15 +23,15 @@ namespace WebApiCompatShimWebSite app.UseMvc(routes => { - // This route can't access any of our webapi controllers - routes.MapRoute("default", "{controller}/{action}/{id?}"); - // Tests include different styles of WebAPI conventional routing and action selection - the prefix keeps // them from matching too eagerly. routes.MapWebApiRoute("named-action", "api/Blog/{controller}/{action}/{id?}"); routes.MapWebApiRoute("unnamed-action", "api/Admin/{controller}/{id?}"); routes.MapWebApiRoute("name-as-parameter", "api/Store/{controller}/{name?}"); routes.MapWebApiRoute("extra-parameter", "api/Support/{extra}/{controller}/{id?}"); + + // This route can't access any of our webapi controllers + routes.MapRoute("default", "{controller}/{action}/{id?}"); }); } }