From fc2d3e588fb0f5e4bb02123e1adaaf3265b8c86c Mon Sep 17 00:00:00 2001 From: Ryan Nowak Date: Thu, 8 Aug 2019 09:01:12 -0700 Subject: [PATCH] Fix dynamic routes with no route values Fixes: #12915 This was just missing a null check. Also added unit tests that were missing for these types. --- .../Infrastructure/ActionSelectionTable.cs | 18 +- .../DynamicControllerEndpointMatcherPolicy.cs | 7 +- .../DynamicControllerEndpointSelector.cs | 9 +- ...amicControllerEndpointMatcherPolicyTest.cs | 272 ++++++++++++++++++ .../DynamicPageEndpointMatcherPolicy.cs | 7 +- .../DynamicPageEndpointSelector.cs | 9 +- .../DynamicPageEndpointMatcherPolicyTest.cs | 265 +++++++++++++++++ 7 files changed, 575 insertions(+), 12 deletions(-) create mode 100644 src/Mvc/Mvc.Core/test/Routing/DynamicControllerEndpointMatcherPolicyTest.cs create mode 100644 src/Mvc/Mvc.RazorPages/test/Infrastructure/DynamicPageEndpointMatcherPolicyTest.cs diff --git a/src/Mvc/Mvc.Core/src/Infrastructure/ActionSelectionTable.cs b/src/Mvc/Mvc.Core/src/Infrastructure/ActionSelectionTable.cs index c028fcf6ed..4521e310ad 100644 --- a/src/Mvc/Mvc.Core/src/Infrastructure/ActionSelectionTable.cs +++ b/src/Mvc/Mvc.Core/src/Infrastructure/ActionSelectionTable.cs @@ -66,10 +66,11 @@ namespace Microsoft.AspNetCore.Mvc.Infrastructure // For action selection, ignore attribute routed actions items: actions.Items.Where(a => a.AttributeRouteInfo == null), - getRouteKeys: a => a.RouteValues.Keys, + getRouteKeys: a => a.RouteValues?.Keys, getRouteValue: (a, key) => { - a.RouteValues.TryGetValue(key, out var value); + string value = null; + a.RouteValues?.TryGetValue(key, out value); return value ?? string.Empty; }); } @@ -87,10 +88,11 @@ namespace Microsoft.AspNetCore.Mvc.Infrastructure return e.GetType() == typeof(Endpoint); }), - getRouteKeys: e => e.Metadata.GetMetadata().RouteValues.Keys, + getRouteKeys: e => e.Metadata.GetMetadata()?.RouteValues?.Keys, getRouteValue: (e, key) => { - e.Metadata.GetMetadata().RouteValues.TryGetValue(key, out var value); + string value = null; + e.Metadata.GetMetadata()?.RouteValues?.TryGetValue(key, out value); return Convert.ToString(value, CultureInfo.InvariantCulture) ?? string.Empty; }); } @@ -112,9 +114,13 @@ namespace Microsoft.AspNetCore.Mvc.Infrastructure foreach (var item in items) { - foreach (var key in getRouteKeys(item)) + var keys = getRouteKeys(item); + if (keys != null) { - routeKeys.Add(key); + foreach (var key in keys) + { + routeKeys.Add(key); + } } } diff --git a/src/Mvc/Mvc.Core/src/Routing/DynamicControllerEndpointMatcherPolicy.cs b/src/Mvc/Mvc.Core/src/Routing/DynamicControllerEndpointMatcherPolicy.cs index 369502d633..118a5b8e84 100644 --- a/src/Mvc/Mvc.Core/src/Routing/DynamicControllerEndpointMatcherPolicy.cs +++ b/src/Mvc/Mvc.Core/src/Routing/DynamicControllerEndpointMatcherPolicy.cs @@ -138,9 +138,12 @@ namespace Microsoft.AspNetCore.Mvc.Routing var values = new RouteValueDictionary(dynamicValues); // Include values that were matched by the fallback route. - foreach (var kvp in originalValues) + if (originalValues != null) { - values.TryAdd(kvp.Key, kvp.Value); + foreach (var kvp in originalValues) + { + values.TryAdd(kvp.Key, kvp.Value); + } } // Update the route values diff --git a/src/Mvc/Mvc.Core/src/Routing/DynamicControllerEndpointSelector.cs b/src/Mvc/Mvc.Core/src/Routing/DynamicControllerEndpointSelector.cs index fbe0337420..f07b22b94b 100644 --- a/src/Mvc/Mvc.Core/src/Routing/DynamicControllerEndpointSelector.cs +++ b/src/Mvc/Mvc.Core/src/Routing/DynamicControllerEndpointSelector.cs @@ -11,10 +11,17 @@ namespace Microsoft.AspNetCore.Mvc.Routing { internal class DynamicControllerEndpointSelector : IDisposable { - private readonly ControllerActionEndpointDataSource _dataSource; + private readonly EndpointDataSource _dataSource; private readonly DataSourceDependentCache> _cache; public DynamicControllerEndpointSelector(ControllerActionEndpointDataSource dataSource) + : this((EndpointDataSource)dataSource) + { + } + + // Exposed for tests. We need to accept a more specific type in the constructor for DI + // to work. + protected DynamicControllerEndpointSelector(EndpointDataSource dataSource) { if (dataSource == null) { diff --git a/src/Mvc/Mvc.Core/test/Routing/DynamicControllerEndpointMatcherPolicyTest.cs b/src/Mvc/Mvc.Core/test/Routing/DynamicControllerEndpointMatcherPolicyTest.cs new file mode 100644 index 0000000000..52e7dcece6 --- /dev/null +++ b/src/Mvc/Mvc.Core/test/Routing/DynamicControllerEndpointMatcherPolicyTest.cs @@ -0,0 +1,272 @@ +// 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 System.Threading.Tasks; +using Microsoft.AspNetCore.Http; +using Microsoft.AspNetCore.Mvc.Abstractions; +using Microsoft.AspNetCore.Mvc.Controllers; +using Microsoft.AspNetCore.Routing; +using Microsoft.AspNetCore.Routing.Matching; +using Microsoft.Extensions.DependencyInjection; +using Xunit; + +namespace Microsoft.AspNetCore.Mvc.Routing +{ + public class DynamicControllerEndpointMatcherPolicyTest + { + public DynamicControllerEndpointMatcherPolicyTest() + { + var actions = new ActionDescriptor[] + { + new ControllerActionDescriptor() + { + RouteValues = new Dictionary(StringComparer.OrdinalIgnoreCase) + { + ["action"] = "Index", + ["controller"] = "Home", + }, + }, + new ControllerActionDescriptor() + { + RouteValues = new Dictionary(StringComparer.OrdinalIgnoreCase) + { + ["action"] = "About", + ["controller"] = "Home", + }, + }, + new ControllerActionDescriptor() + { + RouteValues = new Dictionary(StringComparer.OrdinalIgnoreCase) + { + ["action"] = "Index", + ["controller"] = "Blog", + }, + } + }; + + ControllerEndpoints = new[] + { + new Endpoint(_ => Task.CompletedTask, new EndpointMetadataCollection(actions[0]), "Test1"), + new Endpoint(_ => Task.CompletedTask, new EndpointMetadataCollection(actions[1]), "Test2"), + new Endpoint(_ => Task.CompletedTask, new EndpointMetadataCollection(actions[2]), "Test3"), + }; + + DynamicEndpoint = new Endpoint( + _ => Task.CompletedTask, + new EndpointMetadataCollection(new object[] + { + new DynamicControllerRouteValueTransformerMetadata(typeof(CustomTransformer)), + }), + "dynamic"); + + DataSource = new DefaultEndpointDataSource(ControllerEndpoints); + + Selector = new TestDynamicControllerEndpointSelector(DataSource); + + var services = new ServiceCollection(); + services.AddRouting(); + services.AddScoped(s => + { + var transformer = new CustomTransformer(); + transformer.Transform = (c, values) => Transform(c, values); + return transformer; + }); + Services = services.BuildServiceProvider(); + + Comparer = Services.GetRequiredService(); + } + + private EndpointMetadataComparer Comparer { get; } + + private DefaultEndpointDataSource DataSource { get; } + + private Endpoint[] ControllerEndpoints { get; } + + private Endpoint DynamicEndpoint { get; } + + private DynamicControllerEndpointSelector Selector { get; } + + private IServiceProvider Services { get; } + + private Func> Transform { get; set; } + + [Fact] + public async Task ApplyAsync_NoMatch() + { + // Arrange + var policy = new DynamicControllerEndpointMatcherPolicy(Selector, Comparer); + + var endpoints = new[] { DynamicEndpoint, }; + var values = new RouteValueDictionary[] { null, }; + var scores = new[] { 0, }; + + var candidates = new CandidateSet(endpoints, values, scores); + candidates.SetValidity(0, false); + + Transform = (c, values) => + { + throw new InvalidOperationException(); + }; + + var httpContext = new DefaultHttpContext() + { + RequestServices = Services, + }; + + // Act + await policy.ApplyAsync(httpContext, candidates); + + // Assert + Assert.False(candidates.IsValidCandidate(0)); + } + + [Fact] + public async Task ApplyAsync_HasMatchNoEndpointFound() + { + // Arrange + var policy = new DynamicControllerEndpointMatcherPolicy(Selector, Comparer); + + var endpoints = new[] { DynamicEndpoint, }; + var values = new RouteValueDictionary[] { null, }; + var scores = new[] { 0, }; + + var candidates = new CandidateSet(endpoints, values, scores); + + Transform = (c, values) => + { + return new ValueTask(new RouteValueDictionary()); + }; + + var httpContext = new DefaultHttpContext() + { + RequestServices = Services, + }; + + // Act + await policy.ApplyAsync(httpContext, candidates); + + // Assert + Assert.Null(candidates[0].Endpoint); + Assert.Null(candidates[0].Values); + Assert.False(candidates.IsValidCandidate(0)); + } + + [Fact] + public async Task ApplyAsync_HasMatchFindsEndpoint_WithoutRouteValues() + { + // Arrange + var policy = new DynamicControllerEndpointMatcherPolicy(Selector, Comparer); + + var endpoints = new[] { DynamicEndpoint, }; + var values = new RouteValueDictionary[] { null, }; + var scores = new[] { 0, }; + + var candidates = new CandidateSet(endpoints, values, scores); + + Transform = (c, values) => + { + return new ValueTask(new RouteValueDictionary(new + { + controller = "Home", + action = "Index", + })); + }; + + var httpContext = new DefaultHttpContext() + { + RequestServices = Services, + }; + + // Act + await policy.ApplyAsync(httpContext, candidates); + + // Assert + Assert.Same(ControllerEndpoints[0], candidates[0].Endpoint); + Assert.Collection( + candidates[0].Values.OrderBy(kvp => kvp.Key), + kvp => + { + Assert.Equal("action", kvp.Key); + Assert.Equal("Index", kvp.Value); + }, + kvp => + { + Assert.Equal("controller", kvp.Key); + Assert.Equal("Home", kvp.Value); + }); + Assert.True(candidates.IsValidCandidate(0)); + } + + [Fact] + public async Task ApplyAsync_HasMatchFindsEndpoint_WithRouteValues() + { + // Arrange + var policy = new DynamicControllerEndpointMatcherPolicy(Selector, Comparer); + + var endpoints = new[] { DynamicEndpoint, }; + var values = new RouteValueDictionary[] { new RouteValueDictionary(new { slug = "test", }), }; + var scores = new[] { 0, }; + + var candidates = new CandidateSet(endpoints, values, scores); + + Transform = (c, values) => + { + return new ValueTask(new RouteValueDictionary(new + { + controller = "Home", + action = "Index", + })); + }; + + var httpContext = new DefaultHttpContext() + { + RequestServices = Services, + }; + + // Act + await policy.ApplyAsync(httpContext, candidates); + + // Assert + Assert.Same(ControllerEndpoints[0], candidates[0].Endpoint); + Assert.Collection( + candidates[0].Values.OrderBy(kvp => kvp.Key), + kvp => + { + Assert.Equal("action", kvp.Key); + Assert.Equal("Index", kvp.Value); + }, + kvp => + { + Assert.Equal("controller", kvp.Key); + Assert.Equal("Home", kvp.Value); + }, + kvp => + { + Assert.Equal("slug", kvp.Key); + Assert.Equal("test", kvp.Value); + }); + Assert.True(candidates.IsValidCandidate(0)); + } + + private class TestDynamicControllerEndpointSelector : DynamicControllerEndpointSelector + { + public TestDynamicControllerEndpointSelector(EndpointDataSource dataSource) + : base(dataSource) + { + } + } + + private class CustomTransformer : DynamicRouteValueTransformer + { + public Func> Transform { get; set; } + + public override ValueTask TransformAsync(HttpContext httpContext, RouteValueDictionary values) + { + return Transform(httpContext, values); + } + } + } +} diff --git a/src/Mvc/Mvc.RazorPages/src/Infrastructure/DynamicPageEndpointMatcherPolicy.cs b/src/Mvc/Mvc.RazorPages/src/Infrastructure/DynamicPageEndpointMatcherPolicy.cs index 6751d41658..ea68558d4b 100644 --- a/src/Mvc/Mvc.RazorPages/src/Infrastructure/DynamicPageEndpointMatcherPolicy.cs +++ b/src/Mvc/Mvc.RazorPages/src/Infrastructure/DynamicPageEndpointMatcherPolicy.cs @@ -146,9 +146,12 @@ namespace Microsoft.AspNetCore.Mvc.RazorPages.Infrastructure var values = new RouteValueDictionary(dynamicValues); // Include values that were matched by the fallback route. - foreach (var kvp in originalValues) + if (originalValues != null) { - values.TryAdd(kvp.Key, kvp.Value); + foreach (var kvp in originalValues) + { + values.TryAdd(kvp.Key, kvp.Value); + } } // Update the route values diff --git a/src/Mvc/Mvc.RazorPages/src/Infrastructure/DynamicPageEndpointSelector.cs b/src/Mvc/Mvc.RazorPages/src/Infrastructure/DynamicPageEndpointSelector.cs index 0e143db00b..b333f2947b 100644 --- a/src/Mvc/Mvc.RazorPages/src/Infrastructure/DynamicPageEndpointSelector.cs +++ b/src/Mvc/Mvc.RazorPages/src/Infrastructure/DynamicPageEndpointSelector.cs @@ -11,10 +11,17 @@ namespace Microsoft.AspNetCore.Mvc.RazorPages.Infrastructure { internal class DynamicPageEndpointSelector : IDisposable { - private readonly PageActionEndpointDataSource _dataSource; + private readonly EndpointDataSource _dataSource; private readonly DataSourceDependentCache> _cache; public DynamicPageEndpointSelector(PageActionEndpointDataSource dataSource) + : this((EndpointDataSource)dataSource) + { + } + + // Exposed for tests. We need to accept a more specific type in the constructor for DI + // to work. + protected DynamicPageEndpointSelector(EndpointDataSource dataSource) { if (dataSource == null) { diff --git a/src/Mvc/Mvc.RazorPages/test/Infrastructure/DynamicPageEndpointMatcherPolicyTest.cs b/src/Mvc/Mvc.RazorPages/test/Infrastructure/DynamicPageEndpointMatcherPolicyTest.cs new file mode 100644 index 0000000000..1cea019f5c --- /dev/null +++ b/src/Mvc/Mvc.RazorPages/test/Infrastructure/DynamicPageEndpointMatcherPolicyTest.cs @@ -0,0 +1,265 @@ +// 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 System.Text; +using System.Threading.Tasks; +using Microsoft.AspNetCore.Http; +using Microsoft.AspNetCore.Mvc.Abstractions; +using Microsoft.AspNetCore.Mvc.Controllers; +using Microsoft.AspNetCore.Mvc.Routing; +using Microsoft.AspNetCore.Routing; +using Microsoft.AspNetCore.Routing.Matching; +using Microsoft.Extensions.DependencyInjection; +using Moq; +using Xunit; + +namespace Microsoft.AspNetCore.Mvc.RazorPages.Infrastructure +{ + public class DynamicPageEndpointMatcherPolicyTest + { + public DynamicPageEndpointMatcherPolicyTest() + { + var actions = new ActionDescriptor[] + { + new PageActionDescriptor() + { + RouteValues = new Dictionary(StringComparer.OrdinalIgnoreCase) + { + ["page"] = "/Index", + }, + }, + new PageActionDescriptor() + { + RouteValues = new Dictionary(StringComparer.OrdinalIgnoreCase) + { + ["page"] = "/About", + }, + }, + }; + + PageEndpoints = new[] + { + new Endpoint(_ => Task.CompletedTask, new EndpointMetadataCollection(actions[0]), "Test1"), + new Endpoint(_ => Task.CompletedTask, new EndpointMetadataCollection(actions[1]), "Test2"), + }; + + DynamicEndpoint = new Endpoint( + _ => Task.CompletedTask, + new EndpointMetadataCollection(new object[] + { + new DynamicPageRouteValueTransformerMetadata(typeof(CustomTransformer)), + }), + "dynamic"); + + DataSource = new DefaultEndpointDataSource(PageEndpoints); + + Selector = new TestDynamicPageEndpointSelector(DataSource); + + var services = new ServiceCollection(); + services.AddRouting(); + services.AddScoped(s => + { + var transformer = new CustomTransformer(); + transformer.Transform = (c, values) => Transform(c, values); + return transformer; + }); + Services = services.BuildServiceProvider(); + + Comparer = Services.GetRequiredService(); + + LoadedEndpoint = new Endpoint(_ => Task.CompletedTask, EndpointMetadataCollection.Empty, "Loaded"); + + var loader = new Mock(); + loader + .Setup(l => l.LoadAsync(It.IsAny())) + .Returns(Task.FromResult(new CompiledPageActionDescriptor() { Endpoint = LoadedEndpoint, })); + Loader = loader.Object; + + } + + private EndpointMetadataComparer Comparer { get; } + + private DefaultEndpointDataSource DataSource { get; } + + private Endpoint[] PageEndpoints { get; } + + private Endpoint DynamicEndpoint { get; } + + private Endpoint LoadedEndpoint { get; } + + private PageLoader Loader { get; } + + private DynamicPageEndpointSelector Selector { get; } + + private IServiceProvider Services { get; } + + private Func> Transform { get; set; } + + [Fact] + public async Task ApplyAsync_NoMatch() + { + // Arrange + var policy = new DynamicPageEndpointMatcherPolicy(Selector, Loader, Comparer); + + var endpoints = new[] { DynamicEndpoint, }; + var values = new RouteValueDictionary[] { null, }; + var scores = new[] { 0, }; + + var candidates = new CandidateSet(endpoints, values, scores); + candidates.SetValidity(0, false); + + Transform = (c, values) => + { + throw new InvalidOperationException(); + }; + + var httpContext = new DefaultHttpContext() + { + RequestServices = Services, + }; + + // Act + await policy.ApplyAsync(httpContext, candidates); + + // Assert + Assert.False(candidates.IsValidCandidate(0)); + } + + [Fact] + public async Task ApplyAsync_HasMatchNoEndpointFound() + { + // Arrange + var policy = new DynamicPageEndpointMatcherPolicy(Selector, Loader, Comparer); + + var endpoints = new[] { DynamicEndpoint, }; + var values = new RouteValueDictionary[] { null, }; + var scores = new[] { 0, }; + + var candidates = new CandidateSet(endpoints, values, scores); + + Transform = (c, values) => + { + return new ValueTask(new RouteValueDictionary()); + }; + + var httpContext = new DefaultHttpContext() + { + RequestServices = Services, + }; + + // Act + await policy.ApplyAsync(httpContext, candidates); + + // Assert + Assert.Null(candidates[0].Endpoint); + Assert.Null(candidates[0].Values); + Assert.False(candidates.IsValidCandidate(0)); + } + + [Fact] + public async Task ApplyAsync_HasMatchFindsEndpoint_WithoutRouteValues() + { + // Arrange + var policy = new DynamicPageEndpointMatcherPolicy(Selector, Loader, Comparer); + + var endpoints = new[] { DynamicEndpoint, }; + var values = new RouteValueDictionary[] { null, }; + var scores = new[] { 0, }; + + var candidates = new CandidateSet(endpoints, values, scores); + + Transform = (c, values) => + { + return new ValueTask(new RouteValueDictionary(new + { + page = "/Index", + })); + }; + + var httpContext = new DefaultHttpContext() + { + RequestServices = Services, + }; + + // Act + await policy.ApplyAsync(httpContext, candidates); + + // Assert + Assert.Same(LoadedEndpoint, candidates[0].Endpoint); + Assert.Collection( + candidates[0].Values.OrderBy(kvp => kvp.Key), + kvp => + { + Assert.Equal("page", kvp.Key); + Assert.Equal("/Index", kvp.Value); + }); + Assert.True(candidates.IsValidCandidate(0)); + } + + [Fact] + public async Task ApplyAsync_HasMatchFindsEndpoint_WithRouteValues() + { + // Arrange + var policy = new DynamicPageEndpointMatcherPolicy(Selector, Loader, Comparer); + + var endpoints = new[] { DynamicEndpoint, }; + var values = new RouteValueDictionary[] { new RouteValueDictionary(new { slug = "test", }), }; + var scores = new[] { 0, }; + + var candidates = new CandidateSet(endpoints, values, scores); + + Transform = (c, values) => + { + return new ValueTask(new RouteValueDictionary(new + { + page = "/Index", + })); + }; + + var httpContext = new DefaultHttpContext() + { + RequestServices = Services, + }; + + // Act + await policy.ApplyAsync(httpContext, candidates); + + // Assert + Assert.Same(LoadedEndpoint, candidates[0].Endpoint); + Assert.Collection( + candidates[0].Values.OrderBy(kvp => kvp.Key), + kvp => + { + Assert.Equal("page", kvp.Key); + Assert.Equal("/Index", kvp.Value); + }, + kvp => + { + Assert.Equal("slug", kvp.Key); + Assert.Equal("test", kvp.Value); + }); + Assert.True(candidates.IsValidCandidate(0)); + } + + private class TestDynamicPageEndpointSelector : DynamicPageEndpointSelector + { + public TestDynamicPageEndpointSelector(EndpointDataSource dataSource) + : base(dataSource) + { + } + } + + private class CustomTransformer : DynamicRouteValueTransformer + { + public Func> Transform { get; set; } + + public override ValueTask TransformAsync(HttpContext httpContext, RouteValueDictionary values) + { + return Transform(httpContext, values); + } + } + } +}