From 63dcdd6ca5136e1751417e4b78884c91e61b4a47 Mon Sep 17 00:00:00 2001 From: Ryan Nowak Date: Wed, 2 Jul 2014 13:39:58 -0700 Subject: [PATCH] Fix for #77 - pass ambient values not in the template to constraints This change adds tests and makes the behavior consistent with legacy MVC as far as what values are visible in constraints. This is important because it allows constraints to make decisions based on whether or not a value is present even if it's not in the template. This is similar to the behavior of WebAPI link generation or Area link generation in MVC 5 - but without hardcoding. --- .../Microsoft.AspNet.Routing.kproj | 1 + .../Template/TemplateBinder.cs | 26 +++- .../Template/TemplateRoute.cs | 8 +- .../Template/TemplateValuesResult.cs | 31 +++++ .../Microsoft.AspNet.Routing.Tests.kproj | 1 - .../Template/TemplateBinderTests.cs | 16 +-- .../Template/TemplateRouteTests.cs | 131 +++++++++++++++++- 7 files changed, 197 insertions(+), 17 deletions(-) create mode 100644 src/Microsoft.AspNet.Routing/Template/TemplateValuesResult.cs diff --git a/src/Microsoft.AspNet.Routing/Microsoft.AspNet.Routing.kproj b/src/Microsoft.AspNet.Routing/Microsoft.AspNet.Routing.kproj index 386d4eb8cb..8b6727ba9a 100644 --- a/src/Microsoft.AspNet.Routing/Microsoft.AspNet.Routing.kproj +++ b/src/Microsoft.AspNet.Routing/Microsoft.AspNet.Routing.kproj @@ -70,6 +70,7 @@ + diff --git a/src/Microsoft.AspNet.Routing/Template/TemplateBinder.cs b/src/Microsoft.AspNet.Routing/Template/TemplateBinder.cs index d4e4dc78bf..877286d874 100644 --- a/src/Microsoft.AspNet.Routing/Template/TemplateBinder.cs +++ b/src/Microsoft.AspNet.Routing/Template/TemplateBinder.cs @@ -28,7 +28,7 @@ namespace Microsoft.AspNet.Routing.Template } // Step 1: Get the list of values we're going to try to use to match and generate this URI - public IDictionary GetAcceptedValues(IDictionary ambientValues, + public TemplateValuesResult GetValues(IDictionary ambientValues, IDictionary values) { var context = new TemplateBindingContext(_defaults, values); @@ -145,7 +145,29 @@ namespace Microsoft.AspNet.Routing.Template } } - return context.AcceptedValues; + // Add any ambient values that don't match parameters - they need to be visible to constraints + // but they will ignored by link generation. + var combinedValues = new Dictionary(context.AcceptedValues, StringComparer.OrdinalIgnoreCase); + if (ambientValues != null) + { + foreach (var kvp in ambientValues) + { + if (IsRoutePartNonEmpty(kvp.Value)) + { + var parameter = GetParameter(kvp.Key); + if (parameter == null && !context.AcceptedValues.ContainsKey(kvp.Key)) + { + combinedValues.Add(kvp.Key, kvp.Value); + } + } + } + } + + return new TemplateValuesResult() + { + AcceptedValues = context.AcceptedValues, + CombinedValues = combinedValues, + }; } // Step 2: If the route is a match generate the appropriate URI diff --git a/src/Microsoft.AspNet.Routing/Template/TemplateRoute.cs b/src/Microsoft.AspNet.Routing/Template/TemplateRoute.cs index 4af40c6e2c..382fc01d57 100644 --- a/src/Microsoft.AspNet.Routing/Template/TemplateRoute.cs +++ b/src/Microsoft.AspNet.Routing/Template/TemplateRoute.cs @@ -103,7 +103,7 @@ namespace Microsoft.AspNet.Routing.Template public string GetVirtualPath(VirtualPathContext context) { - var values = _binder.GetAcceptedValues(context.AmbientValues, context.Values); + var values = _binder.GetValues(context.AmbientValues, context.Values); if (values == null) { // We're missing one of the required values for this route. @@ -111,7 +111,7 @@ namespace Microsoft.AspNet.Routing.Template } if (!RouteConstraintMatcher.Match(Constraints, - values, + values.CombinedValues, context.Context, this, RouteDirection.UrlGeneration)) @@ -120,7 +120,7 @@ namespace Microsoft.AspNet.Routing.Template } // Validate that the target can accept these values. - var childContext = CreateChildVirtualPathContext(context, values); + var childContext = CreateChildVirtualPathContext(context, values.AcceptedValues); var path = _target.GetVirtualPath(childContext); if (path != null) { @@ -134,7 +134,7 @@ namespace Microsoft.AspNet.Routing.Template return null; } - path = _binder.BindValues(values); + path = _binder.BindValues(values.AcceptedValues); if (path != null) { context.IsBound = true; diff --git a/src/Microsoft.AspNet.Routing/Template/TemplateValuesResult.cs b/src/Microsoft.AspNet.Routing/Template/TemplateValuesResult.cs new file mode 100644 index 0000000000..1876678fcb --- /dev/null +++ b/src/Microsoft.AspNet.Routing/Template/TemplateValuesResult.cs @@ -0,0 +1,31 @@ +// 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.Collections.Generic; + +namespace Microsoft.AspNet.Routing.Template +{ + /// + /// The values used as inputs for constraints and link generation. + /// + public class TemplateValuesResult + { + /// + /// The set of values that will appear in the URL. + /// + public Dictionary AcceptedValues { get; set; } + + /// + /// The set of values that that were supplied for URL generation. + /// + /// + /// This combines implicit (ambient) values from the of the current request + /// (if applicable), explictly provided values, and default values for parameters that appear in + /// the route template. + /// + /// Implicit (ambient) values which are invalidated due to changes in values lexically earlier in the + /// route template are excluded from this set. + /// + public Dictionary CombinedValues { get; set; } + } +} \ No newline at end of file diff --git a/test/Microsoft.AspNet.Routing.Tests/Microsoft.AspNet.Routing.Tests.kproj b/test/Microsoft.AspNet.Routing.Tests/Microsoft.AspNet.Routing.Tests.kproj index 7ab2d1633b..04c3acab62 100644 --- a/test/Microsoft.AspNet.Routing.Tests/Microsoft.AspNet.Routing.Tests.kproj +++ b/test/Microsoft.AspNet.Routing.Tests/Microsoft.AspNet.Routing.Tests.kproj @@ -43,7 +43,6 @@ - diff --git a/test/Microsoft.AspNet.Routing.Tests/Template/TemplateBinderTests.cs b/test/Microsoft.AspNet.Routing.Tests/Template/TemplateBinderTests.cs index ae5fb835f0..9fdc2274fb 100644 --- a/test/Microsoft.AspNet.Routing.Tests/Template/TemplateBinderTests.cs +++ b/test/Microsoft.AspNet.Routing.Tests/Template/TemplateBinderTests.cs @@ -138,8 +138,8 @@ namespace Microsoft.AspNet.Routing.Template.Tests defaults); // Act & Assert - var acceptedValues = binder.GetAcceptedValues(null, values); - if (acceptedValues == null) + var result = binder.GetValues(ambientValues: null, values: values); + if (result == null) { if (expected == null) { @@ -147,11 +147,11 @@ namespace Microsoft.AspNet.Routing.Template.Tests } else { - Assert.NotNull(acceptedValues); + Assert.NotNull(result); } } - var boundTemplate = binder.BindValues(acceptedValues); + var boundTemplate = binder.BindValues(result.AcceptedValues); if (expected == null) { Assert.Null(boundTemplate); @@ -971,8 +971,8 @@ namespace Microsoft.AspNet.Routing.Template.Tests var binder = new TemplateBinder(TemplateParser.Parse(template, _inlineConstraintResolver), defaults); // Act & Assert - var acceptedValues = binder.GetAcceptedValues(ambientValues, values); - if (acceptedValues == null) + var result = binder.GetValues(ambientValues, values); + if (result == null) { if (expected == null) { @@ -980,11 +980,11 @@ namespace Microsoft.AspNet.Routing.Template.Tests } else { - Assert.NotNull(acceptedValues); + Assert.NotNull(result); } } - var boundTemplate = binder.BindValues(acceptedValues); + var boundTemplate = binder.BindValues(result.AcceptedValues); if (expected == null) { Assert.Null(boundTemplate); diff --git a/test/Microsoft.AspNet.Routing.Tests/Template/TemplateRouteTests.cs b/test/Microsoft.AspNet.Routing.Tests/Template/TemplateRouteTests.cs index 2409f3628e..df90bece69 100644 --- a/test/Microsoft.AspNet.Routing.Tests/Template/TemplateRouteTests.cs +++ b/test/Microsoft.AspNet.Routing.Tests/Template/TemplateRouteTests.cs @@ -364,6 +364,117 @@ namespace Microsoft.AspNet.Routing.Template.Tests Assert.Equal(expectedValues, childContext.ProvidedValues); } + // Any ambient values from the current request should be visible to constraint, even + // if they have nothing to do with the route generating a link + [Fact] + public void GetVirtualPath_ConstraintsSeeAmbientValues() + { + // Arrange + var constraint = new CapturingConstraint(); + var route = CreateRoute( + template: "slug/{controller}/{action}", + defaults: null, + accept: true, + constraints: new { c = constraint }); + + var context = CreateVirtualPathContext( + values: new { action = "Store" }, + ambientValues: new { Controller = "Home", action = "Blog", extra = "42" }); + + var expectedValues = new RouteValueDictionary( + new { controller = "Home", action = "Store", extra = "42" }); + + // Act + var path = route.GetVirtualPath(context); + + // Assert + Assert.Equal("slug/Home/Store", path); + Assert.Equal(expectedValues, constraint.Values); + } + + // Non-parameter default values from the routing generating a link are not in the 'values' + // collection when constraints are processed. + [Fact] + public void GetVirtualPath_ConstraintsDontSeeDefaults_WhenTheyArentParameters() + { + // Arrange + var constraint = new CapturingConstraint(); + var route = CreateRoute( + template: "slug/{controller}/{action}", + defaults: new { otherthing = "17" }, + accept: true, + constraints: new { c = constraint }); + + var context = CreateVirtualPathContext( + values: new { action = "Store" }, + ambientValues: new { Controller = "Home", action = "Blog" }); + + var expectedValues = new RouteValueDictionary( + new { controller = "Home", action = "Store" }); + + // Act + var path = route.GetVirtualPath(context); + + // Assert + Assert.Equal("slug/Home/Store", path); + Assert.Equal(expectedValues, constraint.Values); + } + + // Default values are visible to the constraint when they are used to fill a parameter. + [Fact] + public void GetVirtualPath_ConstraintsSeesDefault_WhenThereItsAParamter() + { + // Arrange + var constraint = new CapturingConstraint(); + var route = CreateRoute( + template: "slug/{controller}/{action}", + defaults: new { action = "Index" }, + accept: true, + constraints: new { c = constraint }); + + var context = CreateVirtualPathContext( + values: new { controller = "Shopping" }, + ambientValues: new { Controller = "Home", action = "Blog" }); + + var expectedValues = new RouteValueDictionary( + new { controller = "Shopping", action = "Index" }); + + // Act + var path = route.GetVirtualPath(context); + + // Assert + Assert.Equal("slug/Shopping", path); + Assert.Equal(expectedValues, constraint.Values); + } + + // Default values from the routing generating a link are in the 'values' collection when + // constraints are processed - IFF they are specified as values or ambient values. + [Fact] + public void GetVirtualPath_ConstraintsSeeDefaults_IfTheyAreSpecifiedOrAmbient() + { + // Arrange + var constraint = new CapturingConstraint(); + var route = CreateRoute( + template: "slug/{controller}/{action}", + defaults: new { otherthing = "17", thirdthing = "13" }, + accept: true, + constraints: new { c = constraint }); + + var context = CreateVirtualPathContext( + values: new { action = "Store", thirdthing = "13" }, + ambientValues: new { Controller = "Home", action = "Blog", otherthing = "17" }); + + var expectedValues = new RouteValueDictionary( + new { controller = "Home", action = "Store", otherthing = "17", thirdthing = "13" }); + + // Act + var path = route.GetVirtualPath(context); + + // Assert + Assert.Equal("slug/Home/Store", path); + Assert.Equal(expectedValues.OrderBy(kvp => kvp.Key), constraint.Values.OrderBy(kvp => kvp.Key)); + } + private static VirtualPathContext CreateVirtualPathContext(object values) { return CreateVirtualPathContext(new RouteValueDictionary(values), null); @@ -520,12 +631,12 @@ namespace Microsoft.AspNet.Routing.Template.Tests return new TemplateRoute(CreateTarget(accept), template, _inlineConstraintResolver); } - private static TemplateRoute CreateRoute(string template, object defaults, bool accept = true, IDictionary constraints = null) + private static TemplateRoute CreateRoute(string template, object defaults, bool accept = true, object constraints = null) { return new TemplateRoute(CreateTarget(accept), template, new RouteValueDictionary(defaults), - constraints, + (constraints as IDictionary) ?? new RouteValueDictionary(constraints), _inlineConstraintResolver); } @@ -569,6 +680,22 @@ namespace Microsoft.AspNet.Routing.Template.Tests resolverMock.Setup(o => o.ResolveConstraint("int")).Returns(new IntRouteConstraint()); return resolverMock.Object; } + + private class CapturingConstraint : IRouteConstraint + { + public IDictionary Values { get; private set; } + + public bool Match( + HttpContext httpContext, + IRouter route, + string routeKey, + IDictionary values, + RouteDirection routeDirection) + { + Values = new RouteValueDictionary(values); + return true; + } + } } }