From e07825954762b50a2a2d0c14cc00fb50cd61a822 Mon Sep 17 00:00:00 2001 From: Pranav K Date: Wed, 6 Jan 2016 12:47:36 -0800 Subject: [PATCH] Modify UrlHelper to use a single StringBuilder --- .../Routing/UrlHelper.cs | 113 +++++++++--------- .../Routing/UrlHelperTest.cs | 71 ++++++----- 2 files changed, 100 insertions(+), 84 deletions(-) diff --git a/src/Microsoft.AspNet.Mvc.Core/Routing/UrlHelper.cs b/src/Microsoft.AspNet.Mvc.Core/Routing/UrlHelper.cs index 6bf2138e0b..697487bd32 100644 --- a/src/Microsoft.AspNet.Mvc.Core/Routing/UrlHelper.cs +++ b/src/Microsoft.AspNet.Mvc.Core/Routing/UrlHelper.cs @@ -19,9 +19,7 @@ namespace Microsoft.AspNet.Mvc.Routing /// Initializes a new instance of the class using the specified action context and /// action selector. /// - /// - /// The for the current request. - /// + /// The for the current request. public UrlHelper(ActionContext actionContext) { if (actionContext == null) @@ -79,13 +77,8 @@ namespace Microsoft.AspNet.Mvc.Routing valuesDictionary["controller"] = actionContext.Controller; } - var path = GeneratePathFromRoute(routeName: null, values: valuesDictionary); - if (path == null) - { - return null; - } - - return GenerateUrl(actionContext.Protocol, actionContext.Host, path, actionContext.Fragment); + var virtualPathData = GetVirtualPathData(routeName: null, values: valuesDictionary); + return GenerateUrl(actionContext.Protocol, actionContext.Host, virtualPathData, actionContext.Fragment); } /// @@ -110,61 +103,53 @@ namespace Microsoft.AspNet.Mvc.Routing } var valuesDictionary = new RouteValueDictionary(routeContext.Values); - - var path = GeneratePathFromRoute(routeContext.RouteName, valuesDictionary); - if (path == null) - { - return null; - } - - return GenerateUrl(routeContext.Protocol, routeContext.Host, path, routeContext.Fragment); + var virtualPathData = GetVirtualPathData(routeContext.RouteName, valuesDictionary); + return GenerateUrl(routeContext.Protocol, routeContext.Host, virtualPathData, routeContext.Fragment); } /// - /// Generates the absolute path of the url for the specified route values by - /// using the specified route name. + /// Gets the for the specified route values by using the specified route name. /// - /// The name of the route that is used to generate the URL. + /// The name of the route that is used to generate the . + /// /// A dictionary that contains the parameters for a route. - /// The absolute path of the URL. - protected virtual string GeneratePathFromRoute(string routeName, RouteValueDictionary values) + /// The . + protected virtual VirtualPathData GetVirtualPathData(string routeName, RouteValueDictionary values) { var context = new VirtualPathContext(HttpContext, AmbientValues, values, routeName); - var pathData = Router.GetVirtualPath(context); - if (pathData == null) - { - return null; - } + return Router.GetVirtualPath(context); + } - // VirtualPathData.VirtualPath returns string.Empty for null. - Debug.Assert(pathData.VirtualPath != null); + // Internal for unit testing. + internal void AppendPathAndFragment(StringBuilder builder, VirtualPathData pathData, string fragment) + { var pathBase = HttpContext.Request.PathBase; + if (!pathBase.HasValue) { if (pathData.VirtualPath.Length == 0) { - return "/"; - } - else if (!pathData.VirtualPath.StartsWith("/", StringComparison.Ordinal)) - { - return "/" + pathData.VirtualPath; + builder.Append("/"); } else { - return pathData.VirtualPath; + if (!pathData.VirtualPath.StartsWith("/", StringComparison.Ordinal)) + { + builder.Append("/"); + } + + builder.Append(pathData.VirtualPath); } } else { if (pathData.VirtualPath.Length == 0) { - return pathBase; + builder.Append(pathBase.Value); } else { - var builder = new StringBuilder( - pathBase.Value, - pathBase.Value.Length + pathData.VirtualPath.Length); + builder.Append(pathBase.Value); if (pathBase.Value.EndsWith("/", StringComparison.Ordinal)) { @@ -177,10 +162,13 @@ namespace Microsoft.AspNet.Mvc.Routing } builder.Append(pathData.VirtualPath); - - return builder.ToString(); } } + + if (!string.IsNullOrEmpty(fragment)) + { + builder.Append("#").Append(fragment); + } } /// @@ -213,34 +201,47 @@ namespace Microsoft.AspNet.Mvc.Routing }); } - private string GenerateUrl(string protocol, string host, string path, string fragment) + /// + /// Generates the URL using the specified components. + /// + /// The protocol. + /// The host. + /// The . + /// The URL fragment. + /// The generated URL. + protected virtual string GenerateUrl(string protocol, string host, VirtualPathData pathData, string fragment) { - Debug.Assert(path != null); - var url = path; - if (!string.IsNullOrEmpty(fragment)) + if (pathData == null) { - url += "#" + fragment; + return null; } + // VirtualPathData.VirtualPath returns string.Empty instead of null. + Debug.Assert(pathData.VirtualPath != null); + + var builder = new StringBuilder(); if (string.IsNullOrEmpty(protocol) && string.IsNullOrEmpty(host)) { - // We're returning a partial url (just path + query + fragment), but we still want it - // to be rooted. - if (!url.StartsWith("/", StringComparison.Ordinal)) + AppendPathAndFragment(builder, pathData, fragment); + // We're returning a partial URL (just path + query + fragment), but we still want it to be rooted. + if (builder.Length == 0 || builder[0] != '/') { - url = "/" + url; + builder.Insert(0, '/'); } - - return url; } else { protocol = string.IsNullOrEmpty(protocol) ? "http" : protocol; - host = string.IsNullOrEmpty(host) ? HttpContext.Request.Host.Value : host; + builder.Append(protocol); - url = protocol + "://" + host + url; - return url; + builder.Append("://"); + + host = string.IsNullOrEmpty(host) ? HttpContext.Request.Host.Value : host; + builder.Append(host); + AppendPathAndFragment(builder, pathData, fragment); } + + return builder.ToString(); } } } \ No newline at end of file diff --git a/test/Microsoft.AspNet.Mvc.Core.Test/Routing/UrlHelperTest.cs b/test/Microsoft.AspNet.Mvc.Core.Test/Routing/UrlHelperTest.cs index 6d09a8558b..b8e8ce9c2c 100644 --- a/test/Microsoft.AspNet.Mvc.Core.Test/Routing/UrlHelperTest.cs +++ b/test/Microsoft.AspNet.Mvc.Core.Test/Routing/UrlHelperTest.cs @@ -3,6 +3,7 @@ using System; using System.Collections.Generic; +using System.Text; using System.Text.Encodings.Web; using System.Threading.Tasks; using Microsoft.AspNet.Builder; @@ -892,25 +893,50 @@ namespace Microsoft.AspNet.Mvc.Routing [Theory] [MemberData(nameof(GeneratePathFromRoute_HandlesLeadingAndTrailingSlashesData))] - public void GeneratePathFromRoute_HandlesLeadingAndTrailingSlashes( + public void AppendPathAndFragment_HandlesLeadingAndTrailingSlashes( string appBase, string virtualPath, string expected) { // Arrage - var router = new Mock(); - router.Setup(r => r.GetVirtualPath(It.IsAny())) - .Returns(new VirtualPathData(router.Object, virtualPath) - { - VirtualPath = virtualPath - }); - var urlHelper = CreateUrlHelper(appBase, router.Object); + var router = Mock.Of(); + var pathData = new VirtualPathData(router, virtualPath) + { + VirtualPath = virtualPath + }; + var urlHelper = CreateUrlHelper(appBase, router); + var builder = new StringBuilder(); // Act - var result = urlHelper.GeneratePathFromRoutePublic("some-name", new RouteValueDictionary()); + urlHelper.AppendPathAndFragment(builder, pathData, string.Empty); // Assert - Assert.Equal(expected, result); + Assert.Equal(expected, builder.ToString()); + } + + [Theory] + [MemberData(nameof(GeneratePathFromRoute_HandlesLeadingAndTrailingSlashesData))] + public void AppendPathAndFragment_AppendsFragments( + string appBase, + string virtualPath, + string expected) + { + // Arrage + var fragmentValue = "fragment-value"; + expected += $"#{fragmentValue}"; + var router = Mock.Of(); + var pathData = new VirtualPathData(router, virtualPath) + { + VirtualPath = virtualPath + }; + var urlHelper = CreateUrlHelper(appBase, router); + var builder = new StringBuilder(); + + // Act + urlHelper.AppendPathAndFragment(builder, pathData, fragmentValue); + + // Assert + Assert.Equal(expected, builder.ToString()); } private static HttpContext CreateHttpContext( @@ -939,13 +965,13 @@ namespace Microsoft.AspNet.Mvc.Routing return new ActionContext(context, routeData, new ActionDescriptor()); } - private static TestableUrlHelper CreateUrlHelper() + private static UrlHelper CreateUrlHelper() { var services = CreateServices(); var context = CreateHttpContext(services, string.Empty); var actionContext = CreateActionContext(context); - return new TestableUrlHelper(actionContext); + return new UrlHelper(actionContext); } private static UrlHelper CreateUrlHelper(ActionContext context) @@ -964,7 +990,7 @@ namespace Microsoft.AspNet.Mvc.Routing return new UrlHelper(actionContext); } - private static TestableUrlHelper CreateUrlHelper(string host, string protocol, IRouter router) + private static UrlHelper CreateUrlHelper(string host, string protocol, IRouter router) { var services = CreateServices(); var context = CreateHttpContext(services, string.Empty); @@ -973,19 +999,19 @@ namespace Microsoft.AspNet.Mvc.Routing var actionContext = CreateActionContext(context, router); - return new TestableUrlHelper(actionContext); + return new UrlHelper(actionContext); } - private static TestableUrlHelper CreateUrlHelper(string appBase, IRouter router) + private static UrlHelper CreateUrlHelper(string appBase, IRouter router) { var services = CreateServices(); var context = CreateHttpContext(services, appBase); var actionContext = CreateActionContext(context, router); - return new TestableUrlHelper(actionContext); + return new UrlHelper(actionContext); } - private static TestableUrlHelper CreateUrlHelperWithRouteCollection( + private static UrlHelper CreateUrlHelperWithRouteCollection( IServiceProvider services, string appPrefix) { @@ -1066,16 +1092,5 @@ namespace Microsoft.AspNet.Mvc.Routing return Task.FromResult(false); } } - - private class TestableUrlHelper : UrlHelper - { - public TestableUrlHelper(ActionContext context) - : base(context) - { - } - - public string GeneratePathFromRoutePublic(string routeName, RouteValueDictionary values) => - GeneratePathFromRoute(routeName, values); - } } } \ No newline at end of file