Don't generate Action links to non-Action named endpoints (dotnet#35955)

src/Http/Routing/src/RouteCollection.cs

@@ -15,7 +15,7 @@
 namespace Microsoft.AspNetCore.Routing
 {
     /// <summary>
-    /// Supports managing a collection fo multiple routes.
+    /// Supports managing a collection for multiple routes.
     /// </summary>
     public class RouteCollection : IRouteCollection
     {

src/Http/Routing/src/RouteValuesAddressScheme.cs

@@ -73,7 +73,7 @@ private static IEnumerable<Endpoint> GetEndpoints(IList<OutboundMatchResult> mat
 
         private StateEntry Initialize(IReadOnlyList<Endpoint> endpoints)
         {
-            var allOutboundMatches = new List<OutboundMatch>();
+            var matchesWithRequiredValues = new List<OutboundMatch>();
             var namedOutboundMatchResults = new Dictionary<string, List<OutboundMatchResult>>(StringComparer.OrdinalIgnoreCase);
 
             // Decision tree is built using the 'required values' of actions.
@@ -118,7 +118,15 @@ private StateEntry Initialize(IReadOnlyList<Endpoint> endpoints)
                     metadata?.RouteName);
 
                 var outboundMatch = new OutboundMatch() { Entry = entry };
-                allOutboundMatches.Add(outboundMatch);
+
+                if (routeEndpoint.RoutePattern.RequiredValues.Count > 0)
+                {
+                    // Entries with a route name but no required values can only be matched by name.
+                    // Otherwise, these endpoints will match any attempt at action link generation.
+                    // Entries with neither a route name nor required values have already been skipped above.
+                    // See https://github.com/dotnet/aspnetcore/issues/35592
+                    matchesWithRequiredValues.Add(outboundMatch);
+                }
 
                 if (string.IsNullOrEmpty(entry.RouteName))
                 {
@@ -134,8 +142,8 @@ private StateEntry Initialize(IReadOnlyList<Endpoint> endpoints)
             }
 
             return new StateEntry(
-                allOutboundMatches,
-                new LinkGenerationDecisionTree(allOutboundMatches),
+                matchesWithRequiredValues,
+                new LinkGenerationDecisionTree(matchesWithRequiredValues),
                 namedOutboundMatchResults);
         }
 
@@ -166,16 +174,16 @@ public void Dispose()
         internal class StateEntry
         {
             // For testing
-            public readonly List<OutboundMatch> AllMatches;
+            public readonly List<OutboundMatch> MatchesWithRequiredValues;
             public readonly LinkGenerationDecisionTree AllMatchesLinkGenerationTree;
             public readonly Dictionary<string, List<OutboundMatchResult>> NamedMatches;
 
             public StateEntry(
-                List<OutboundMatch> allMatches,
+                List<OutboundMatch> matchesWithRequiredValues,
                 LinkGenerationDecisionTree allMatchesLinkGenerationTree,
                 Dictionary<string, List<OutboundMatchResult>> namedMatches)
             {
-                AllMatches = allMatches;
+                MatchesWithRequiredValues = matchesWithRequiredValues;
                 AllMatchesLinkGenerationTree = allMatchesLinkGenerationTree;
                 NamedMatches = namedMatches;
             }

src/Http/Routing/test/UnitTests/RouteValuesAddressSchemeTest.cs

@@ -1,12 +1,9 @@
 // Licensed to the .NET Foundation under one or more agreements.
 // The .NET Foundation licenses this file to you under the MIT license.
 
-using System.Collections.Generic;
-using System.Linq;
 using Microsoft.AspNetCore.Http;
 using Microsoft.AspNetCore.Routing.Patterns;
 using Microsoft.AspNetCore.Routing.TestObjects;
-using Xunit;
 
 namespace Microsoft.AspNetCore.Routing
 {
@@ -23,9 +20,8 @@ public void GetOutboundMatches_GetsNamedMatchesFor_EndpointsHaving_IRouteNameMet
             var addressScheme = CreateAddressScheme(endpoint1, endpoint2);
 
             // Assert
-            Assert.NotNull(addressScheme.State.AllMatches);
-            Assert.Equal(2, addressScheme.State.AllMatches.Count());
-            Assert.NotNull(addressScheme.State.NamedMatches);
+            var allMatches = GetMatchesWithRequiredValuesPlusNamedMatches(addressScheme);
+            Assert.Equal(2, allMatches.Count);
             Assert.True(addressScheme.State.NamedMatches.TryGetValue("named", out var namedMatches));
             var namedMatch = Assert.Single(namedMatches);
             var actual = Assert.IsType<RouteEndpoint>(namedMatch.Match.Entry.Data);
@@ -44,9 +40,8 @@ public void GetOutboundMatches_GroupsMultipleEndpoints_WithSameName()
             var addressScheme = CreateAddressScheme(endpoint1, endpoint2, endpoint3);
 
             // Assert
-            Assert.NotNull(addressScheme.State.AllMatches);
-            Assert.Equal(3, addressScheme.State.AllMatches.Count());
-            Assert.NotNull(addressScheme.State.NamedMatches);
+            var allMatches = GetMatchesWithRequiredValuesPlusNamedMatches(addressScheme);
+            Assert.Equal(3, allMatches.Count);
             Assert.True(addressScheme.State.NamedMatches.TryGetValue("named", out var namedMatches));
             Assert.Equal(2, namedMatches.Count);
             Assert.Same(endpoint2, Assert.IsType<RouteEndpoint>(namedMatches[0].Match.Entry.Data));
@@ -65,9 +60,8 @@ public void GetOutboundMatches_GroupsMultipleEndpoints_WithSameName_IgnoringCase
             var addressScheme = CreateAddressScheme(endpoint1, endpoint2, endpoint3);
 
             // Assert
-            Assert.NotNull(addressScheme.State.AllMatches);
-            Assert.Equal(3, addressScheme.State.AllMatches.Count());
-            Assert.NotNull(addressScheme.State.NamedMatches);
+            var allMatches = GetMatchesWithRequiredValuesPlusNamedMatches(addressScheme);
+            Assert.Equal(3, allMatches.Count);
             Assert.True(addressScheme.State.NamedMatches.TryGetValue("named", out var namedMatches));
             Assert.Equal(2, namedMatches.Count);
             Assert.Same(endpoint2, Assert.IsType<RouteEndpoint>(namedMatches[0].Match.Entry.Data));
@@ -86,8 +80,11 @@ public void EndpointDataSource_ChangeCallback_Refreshes_OutboundMatches()
 
             // Assert 1
             var state = addressScheme.State;
-            Assert.NotNull(state.AllMatches);
-            var match = Assert.Single(state.AllMatches);
+            var allMatches = GetMatchesWithRequiredValuesPlusNamedMatches(addressScheme);
+
+            Assert.NotEmpty(allMatches);
+
+            var match = Assert.Single(allMatches);
             var actual = Assert.IsType<RouteEndpoint>(match.Entry.Data);
             Assert.Same(endpoint1, actual);
 
@@ -124,9 +121,11 @@ public void EndpointDataSource_ChangeCallback_Refreshes_OutboundMatches()
             Assert.NotSame(state, addressScheme.State);
             state = addressScheme.State;
 
-            Assert.NotNull(state.AllMatches);
+            allMatches = GetMatchesWithRequiredValuesPlusNamedMatches(addressScheme);
+
+            Assert.NotEmpty(allMatches);
             Assert.Collection(
-                state.AllMatches,
+                allMatches,
                 (m) =>
                 {
                     actual = Assert.IsType<RouteEndpoint>(m.Entry.Data);
@@ -335,19 +334,66 @@ public void FindEndpoints_AlwaysReturnsEndpointsByRouteName_IgnoringMissingRequi
             Assert.Same(expected, actual);
         }
 
+        [Fact]
+        public void GetOutboundMatches_Includes_SameEndpointInNamedMatchesAndMatchesWithRequiredValues()
+        {
+            // Arrange
+            var endpoint = CreateEndpoint(
+                "api/orders/{id}",
+                defaults: new { controller = "Orders", action = "GetById" },
+                metadataRequiredValues: new { controller = "Orders", action = "GetById" },
+                routeName: "a");
+
+            // Act
+            var addressScheme = CreateAddressScheme(endpoint);
+
+            // Assert
+            var matchWithRequiredValue = Assert.Single(addressScheme.State.MatchesWithRequiredValues);
+            var namedMatches = Assert.Single(addressScheme.State.NamedMatches).Value;
+            var namedMatch = Assert.Single(namedMatches).Match;
+
+            Assert.Same(endpoint, matchWithRequiredValue.Entry.Data);
+            Assert.Same(endpoint, namedMatch.Entry.Data);
+        }
+
+        // Regression test for https://github.com/dotnet/aspnetcore/issues/35592
+        [Fact]
+        public void GetOutboundMatches_DoesNotInclude_EndpointsWithoutRequiredValuesInMatchesWithRequiredValues()
+        {
+            // Arrange
+            var endpoint = CreateEndpoint(
+                "api/orders/{id}",
+                defaults: new { controller = "Orders", action = "GetById" },
+                routeName: "a");
+
+            // Act
+            var addressScheme = CreateAddressScheme(endpoint);
+
+            // Assert
+            Assert.Empty(addressScheme.State.MatchesWithRequiredValues);
+
+            var namedMatches = Assert.Single(addressScheme.State.NamedMatches).Value;
+            var namedMatch = Assert.Single(namedMatches).Match;
+            Assert.Same(endpoint, namedMatch.Entry.Data);
+        }
+
         [Fact]
         public void GetOutboundMatches_DoesNotInclude_EndpointsWithSuppressLinkGenerationMetadata()
         {
             // Arrange
             var endpoint = CreateEndpoint(
-                "/a",
+                "api/orders/{id}",
+                defaults: new { controller = "Orders", action = "GetById" },
+                metadataRequiredValues: new { controller = "Orders", action = "GetById" },
+                routeName: "a",
                 metadataCollection: new EndpointMetadataCollection(new[] { new SuppressLinkGenerationMetadata() }));
 
             // Act
             var addressScheme = CreateAddressScheme(endpoint);
 
             // Assert
-            Assert.Empty(addressScheme.State.AllMatches);
+            var allMatches = GetMatchesWithRequiredValuesPlusNamedMatches(addressScheme);
+            Assert.Empty(allMatches);
         }
 
         [Fact]
@@ -356,13 +402,14 @@ public void AddressScheme_UnsuppressedEndpoint_IsUsed()
             // Arrange
             var endpoint = EndpointFactory.CreateRouteEndpoint(
                 "/a",
-                metadata: new object[] { new SuppressLinkGenerationMetadata(), new EncourageLinkGenerationMetadata(), new RouteNameMetadata(string.Empty), });
+                metadata: new object[] { new SuppressLinkGenerationMetadata(), new EncourageLinkGenerationMetadata(), new RouteNameMetadata("a"), });
 
             // Act
             var addressScheme = CreateAddressScheme(endpoint);
 
             // Assert
-            Assert.Same(endpoint, Assert.Single(addressScheme.State.AllMatches).Entry.Data);
+            var allMatches = GetMatchesWithRequiredValuesPlusNamedMatches(addressScheme);
+            Assert.Same(endpoint, Assert.Single(allMatches).Entry.Data);
         }
 
         private RouteValuesAddressScheme CreateAddressScheme(params Endpoint[] endpoints)
@@ -401,6 +448,18 @@ private RouteEndpoint CreateEndpoint(
                 null);
         }
 
+        private static List<Tree.OutboundMatch> GetMatchesWithRequiredValuesPlusNamedMatches(RouteValuesAddressScheme routeValuesAddressScheme)
+        {
+            var state = routeValuesAddressScheme.State;
+
+            Assert.NotNull(state.MatchesWithRequiredValues);
+            Assert.NotNull(state.NamedMatches);
+
+            var namedMatches = state.NamedMatches.Aggregate(Enumerable.Empty<Tree.OutboundMatch>(),
+                (acc, kvp) => acc.Concat(kvp.Value.Select(matchResult => matchResult.Match)));
+            return state.MatchesWithRequiredValues.Concat(namedMatches).ToList();
+        }
+
         private class EncourageLinkGenerationMetadata : ISuppressLinkGenerationMetadata
         {
             public bool SuppressLinkGeneration => false;

src/Mvc/Mvc.Core/test/Routing/EndpointRoutingUrlHelperTest.cs

@@ -1,18 +1,11 @@
-// Licensed to the .NET Foundation under one or more agreements.
+// Licensed to the .NET Foundation under one or more agreements.
 // The .NET Foundation licenses this file to you under the MIT license.
 
-using System;
-using System.Collections.Generic;
-using System.Linq;
-using System.Threading.Tasks;
 using Microsoft.AspNetCore.Http;
-using Microsoft.AspNetCore.Http.Features;
 using Microsoft.AspNetCore.Routing;
-using Microsoft.AspNetCore.Routing.Matching;
 using Microsoft.AspNetCore.Routing.Patterns;
 using Microsoft.Extensions.DependencyInjection;
 using Microsoft.Extensions.DependencyInjection.Extensions;
-using Xunit;
 
 namespace Microsoft.AspNetCore.Mvc.Routing
 {
@@ -145,7 +138,7 @@ protected override IUrlHelper CreateUrlHelperWithDefaultRoutes(
         protected override IUrlHelper CreateUrlHelper(ActionContext actionContext)
         {
             var httpContext = actionContext.HttpContext;
-           httpContext.SetEndpoint(new Endpoint(context => Task.CompletedTask, EndpointMetadataCollection.Empty, null));
+            httpContext.SetEndpoint(new Endpoint(context => Task.CompletedTask, EndpointMetadataCollection.Empty, null));
 
             var urlHelperFactory = httpContext.RequestServices.GetRequiredService<IUrlHelperFactory>();
             var urlHelper = urlHelperFactory.GetUrlHelper(actionContext);
@@ -164,9 +157,10 @@ protected override IUrlHelper CreateUrlHelper(
             string protocol,
             string routeName,
             string template,
-            object defaults)
+            object defaults,
+            object requiredValues)
         {
-            var endpoint = CreateEndpoint(template, new RouteValueDictionary(defaults), routeName: routeName);
+            var endpoint = CreateEndpoint(template, new RouteValueDictionary(defaults), requiredValues, routeName: routeName);
             var services = CreateServices(new[] { endpoint });
             var httpContext = CreateHttpContext(services, appRoot: "", host: null, protocol: null);
             var actionContext = CreateActionContext(httpContext);

src/Mvc/Mvc.Core/test/Routing/UrlHelperTest.cs

@@ -1,14 +1,10 @@
 // Licensed to the .NET Foundation under one or more agreements.
 // The .NET Foundation licenses this file to you under the MIT license.
 
-using System;
-using System.Text;
-using System.Threading.Tasks;
 using Microsoft.AspNetCore.Builder;
 using Microsoft.AspNetCore.Routing;
 using Microsoft.Extensions.DependencyInjection;
 using Moq;
-using Xunit;
 
 namespace Microsoft.AspNetCore.Mvc.Routing
 {
@@ -68,7 +64,8 @@ protected override IUrlHelper CreateUrlHelper(
             string protocol,
             string routeName,
             string template,
-            object defaults)
+            object defaults,
+            object requiredValues)
         {
             var services = CreateServices();
             var routeBuilder = CreateRouteBuilder(services);
@@ -152,4 +149,4 @@ public Task RouteAsync(RouteContext context)
             }
         }
     }
-}
+}