From 28a9fc21d51e15bc70401639078ff5746118e307 Mon Sep 17 00:00:00 2001 From: Safia Abdalla Date: Wed, 15 Sep 2021 09:36:53 -0700 Subject: [PATCH] Check for duplicate endpoint names on startup (#36353) * Check for duplicate endpoint names on startup * Add display name to exception message * Always validate duplicate endpoints and add more info to error --- .../Matching/DataSourceDependentMatcher.cs | 23 ++++- .../DataSourceDependentMatcherTest.cs | 96 +++++++++++++++++++ 2 files changed, 117 insertions(+), 2 deletions(-) diff --git a/src/Http/Routing/src/Matching/DataSourceDependentMatcher.cs b/src/Http/Routing/src/Matching/DataSourceDependentMatcher.cs index e8362bd347d..2906028a02a 100644 --- a/src/Http/Routing/src/Matching/DataSourceDependentMatcher.cs +++ b/src/Http/Routing/src/Matching/DataSourceDependentMatcher.cs @@ -41,14 +41,33 @@ namespace Microsoft.AspNetCore.Routing.Matching private Matcher CreateMatcher(IReadOnlyList endpoints) { var builder = _matcherBuilderFactory(); + var seenEndpointNames = new Dictionary(); for (var i = 0; i < endpoints.Count; i++) { // By design we only look at RouteEndpoint here. It's possible to // register other endpoint types, which are non-routable, and it's // ok that we won't route to them. - if (endpoints[i] is RouteEndpoint endpoint && endpoint.Metadata.GetMetadata()?.SuppressMatching != true) + if (endpoints[i] is RouteEndpoint endpoint) { - builder.AddEndpoint(endpoint); + // Validate that endpoint names are unique. + var endpointName = endpoint.Metadata.GetMetadata()?.EndpointName; + if (endpointName is not null) + { + if (seenEndpointNames.TryGetValue(endpointName, out var existingEndpoint)) + { + throw new InvalidOperationException($"Duplicate endpoint name '{endpointName}' found on '{endpoint.DisplayName}' and '{existingEndpoint}'. Endpoint names must be globally unique."); + } + + seenEndpointNames.Add(endpointName, endpoint.DisplayName ?? endpoint.RoutePattern.RawText); + } + + // We check for duplicate endpoint names on all endpoints regardless + // of whether they suppress matching because endpoint names can be + // used in OpenAPI specifications as well. + if (endpoint.Metadata.GetMetadata()?.SuppressMatching != true) + { + builder.AddEndpoint(endpoint); + } } } diff --git a/src/Http/Routing/test/UnitTests/Matching/DataSourceDependentMatcherTest.cs b/src/Http/Routing/test/UnitTests/Matching/DataSourceDependentMatcherTest.cs index 42374fbe1c1..88563ba750d 100644 --- a/src/Http/Routing/test/UnitTests/Matching/DataSourceDependentMatcherTest.cs +++ b/src/Http/Routing/test/UnitTests/Matching/DataSourceDependentMatcherTest.cs @@ -141,6 +141,102 @@ namespace Microsoft.AspNetCore.Routing.Matching Assert.Same(endpoint, Assert.Single(inner.Endpoints)); } + [Fact] + public void Matcher_ThrowsOnDuplicateEndpoints() + { + // Arrange + var expectedError = "Duplicate endpoint name 'Foo' found on '/bar' and '/foo'. Endpoint names must be globally unique."; + var dataSource = new DynamicEndpointDataSource(); + var lifetime = new DataSourceDependentMatcher.Lifetime(); + dataSource.AddEndpoint(new RouteEndpoint( + TestConstants.EmptyRequestDelegate, + RoutePatternFactory.Parse("/foo"), + 0, + new EndpointMetadataCollection(new EndpointNameMetadata("Foo")), + "/foo" + )); + dataSource.AddEndpoint(new RouteEndpoint( + TestConstants.EmptyRequestDelegate, + RoutePatternFactory.Parse("/bar"), + 0, + new EndpointMetadataCollection(new EndpointNameMetadata("Foo")), + "/bar" + )); + + // Assert + var exception = Assert.Throws( + () => new DataSourceDependentMatcher(dataSource, lifetime, TestMatcherBuilder.Create)); + Assert.Equal(expectedError, exception.Message); + } + + [Fact] + public void Matcher_ThrowsOnDuplicateEndpointsFromMultipleSources() + { + // Arrange + var expectedError = "Duplicate endpoint name 'Foo' found on '/foo2' and '/foo'. Endpoint names must be globally unique."; + var dataSource = new DynamicEndpointDataSource(); + var lifetime = new DataSourceDependentMatcher.Lifetime(); + dataSource.AddEndpoint(new RouteEndpoint( + TestConstants.EmptyRequestDelegate, + RoutePatternFactory.Parse("/foo"), + 0, + new EndpointMetadataCollection(new EndpointNameMetadata("Foo")), + "/foo" + )); + dataSource.AddEndpoint(new RouteEndpoint( + TestConstants.EmptyRequestDelegate, + RoutePatternFactory.Parse("/bar"), + 0, + new EndpointMetadataCollection(new EndpointNameMetadata("Bar")), + "/bar" + )); + var anotherDataSource = new DynamicEndpointDataSource(); + anotherDataSource.AddEndpoint(new RouteEndpoint( + TestConstants.EmptyRequestDelegate, + RoutePatternFactory.Parse("/foo2"), + 0, + new EndpointMetadataCollection(new EndpointNameMetadata("Foo")), + "/foo2" + )); + + var compositeDataSource = new CompositeEndpointDataSource(new[] { dataSource, anotherDataSource }); + + // Assert + var exception = Assert.Throws( + () => new DataSourceDependentMatcher(compositeDataSource, lifetime, TestMatcherBuilder.Create)); + Assert.Equal(expectedError, exception.Message); + } + + [Fact] + public void Matcher_ThrowsOnDuplicateEndpointAddedLater() + { + // Arrange + var expectedError = "Duplicate endpoint name 'Foo' found on '/bar' and '/foo'. Endpoint names must be globally unique."; + var dataSource = new DynamicEndpointDataSource(); + var lifetime = new DataSourceDependentMatcher.Lifetime(); + dataSource.AddEndpoint(new RouteEndpoint( + TestConstants.EmptyRequestDelegate, + RoutePatternFactory.Parse("/foo"), + 0, + new EndpointMetadataCollection(new EndpointNameMetadata("Foo")), + "/foo" + )); + + // Act (should be all good since no duplicate has been added yet) + var matcher = new DataSourceDependentMatcher(dataSource, lifetime, TestMatcherBuilder.Create); + + // Assert that rerunning initializer throws AggregateException + var exception = Assert.Throws( + () => dataSource.AddEndpoint(new RouteEndpoint( + TestConstants.EmptyRequestDelegate, + RoutePatternFactory.Parse("/bar"), + 0, + new EndpointMetadataCollection(new EndpointNameMetadata("Foo")), + "/bar" + ))); + Assert.Equal(expectedError, exception.InnerException.Message); + } + private class TestMatcherBuilder : MatcherBuilder { public static Func Create = () => new TestMatcherBuilder();