From f3074d92fd9cc292459c7e70471c1d98e6868b55 Mon Sep 17 00:00:00 2001
From: Ryan Nowak <nowakra@gmail.com>
Date: Tue, 26 Nov 2019 12:49:38 -0800
Subject: [PATCH] Improve error for RRV substitution case

Fixes: #14789

Users can hit this error case when using a route parameter that has a
special meaning in MVC, in attribute routing with a route constraint.
This will cause us to fail while expanding the route pattern, because
the canonical value associated with the parameter won't satisfy the
constraint.

TLDR: don't do that. Using MVC's reserved parameter names for
application-level concerns will always cause bugs.

This was a case where you'd fail with a totally unactionable error
message. Updating it to reflect the proper root cause and our guidance
to fix it.
---
 .../src/Routing/ActionEndpointFactory.cs         |  9 ++++++++-
 .../test/Routing/ActionEndpointFactoryTest.cs    | 16 ++++++++++++++++
 2 files changed, 24 insertions(+), 1 deletion(-)

diff --git a/src/Mvc/Mvc.Core/src/Routing/ActionEndpointFactory.cs b/src/Mvc/Mvc.Core/src/Routing/ActionEndpointFactory.cs
index 8f3b4dd74b..8d09d344f8 100644
--- a/src/Mvc/Mvc.Core/src/Routing/ActionEndpointFactory.cs
+++ b/src/Mvc/Mvc.Core/src/Routing/ActionEndpointFactory.cs
@@ -132,7 +132,14 @@ namespace Microsoft.AspNetCore.Mvc.Routing
                 var updatedRoutePattern = _routePatternTransformer.SubstituteRequiredValues(resolvedRoutePattern, resolvedRouteValues);
                 if (updatedRoutePattern == null)
                 {
-                    throw new InvalidOperationException("Failed to update route pattern with required values.");
+                    // This kind of thing can happen when a route pattern uses a *reserved* route value such as `action`.
+                    // See: https://github.com/aspnet/AspNetCore/issues/14789
+                    var formattedRouteKeys = string.Join(", ", resolvedRouteValues.Keys.Select(k => $"'{k}'"));
+                    throw new InvalidOperationException(
+                        $"Failed to update the route pattern '{resolvedRoutePattern.RawText}' with required route values. " +
+                        $"This can occur when the route pattern contains parameters with reserved names such as: {formattedRouteKeys} " +
+                        $"and also uses route constraints such as '{{action:int}}'. " +
+                        $"To fix this error, choose a different parmaeter name.");
                 }
 
                 var builder = new RouteEndpointBuilder(_requestDelegate, updatedRoutePattern, action.AttributeRouteInfo.Order)
diff --git a/src/Mvc/Mvc.Core/test/Routing/ActionEndpointFactoryTest.cs b/src/Mvc/Mvc.Core/test/Routing/ActionEndpointFactoryTest.cs
index 0eb69069bf..00f89b09d2 100644
--- a/src/Mvc/Mvc.Core/test/Routing/ActionEndpointFactoryTest.cs
+++ b/src/Mvc/Mvc.Core/test/Routing/ActionEndpointFactoryTest.cs
@@ -208,6 +208,22 @@ namespace Microsoft.AspNetCore.Mvc.Routing
             Assert.Empty(endpoints);
         }
 
+        [Fact]
+        public void AddEndpoints_AttributeRouted_ContainsParameterUsingReservedNameWithConstraint_ExceptionThrown()
+        {
+            // Arrange
+            var values = new { controller = "TestController", action = "TestAction", page = (string)null };
+            var action = CreateActionDescriptor(values, "Products/{action:int}");
+
+            // Act & Assert
+            var exception = Assert.Throws<InvalidOperationException>(() => CreateAttributeRoutedEndpoint(action));
+            Assert.Equal(
+                "Failed to update the route pattern 'Products/{action:int}' with required route values. " +
+                "This can occur when the route pattern contains parameters with reserved names such as: 'controller', 'action', 'page' and also uses route constraints such as '{action:int}'. " +
+                "To fix this error, choose a different parmaeter name.",
+                exception.Message);
+        }
+
         [Fact]
         public void AddEndpoints_AttributeRouted_ContainsParameterWithNullRequiredRouteValue_EndpointCreated()
         {