From 35c0785f8d533451dd9c475dac98974d7f66a2da Mon Sep 17 00:00:00 2001 From: Safia Abdalla Date: Thu, 9 Sep 2021 12:00:03 -0700 Subject: [PATCH 1/3] Check for duplicate endpoint names on startup --- .../Matching/DataSourceDependentMatcher.cs | 11 +++ .../DataSourceDependentMatcherTest.cs | 96 +++++++++++++++++++ 2 files changed, 107 insertions(+) diff --git a/src/Http/Routing/src/Matching/DataSourceDependentMatcher.cs b/src/Http/Routing/src/Matching/DataSourceDependentMatcher.cs index e8362bd347da..9e3acc1e4bfa 100644 --- a/src/Http/Routing/src/Matching/DataSourceDependentMatcher.cs +++ b/src/Http/Routing/src/Matching/DataSourceDependentMatcher.cs @@ -41,6 +41,7 @@ public override Task MatchAsync(HttpContext httpContext) private Matcher CreateMatcher(IReadOnlyList endpoints) { var builder = _matcherBuilderFactory(); + var seenEndpointNames = new HashSet(); for (var i = 0; i < endpoints.Count; i++) { // By design we only look at RouteEndpoint here. It's possible to @@ -48,6 +49,16 @@ private Matcher CreateMatcher(IReadOnlyList endpoints) // ok that we won't route to them. if (endpoints[i] is RouteEndpoint endpoint && endpoint.Metadata.GetMetadata()?.SuppressMatching != true) { + // Validate that endpoint names are unique. + var endpointName = endpoint.Metadata.GetMetadata()?.EndpointName; + if (endpointName is not null) + { + if (!seenEndpointNames.Add(endpointName)) + { + throw new InvalidOperationException($"Duplicate endpoint name {endpointName} found. Endpoint names must be globally unique."); + } + } + builder.AddEndpoint(endpoint); } } diff --git a/src/Http/Routing/test/UnitTests/Matching/DataSourceDependentMatcherTest.cs b/src/Http/Routing/test/UnitTests/Matching/DataSourceDependentMatcherTest.cs index 42374fbe1c1a..d99c0b65c4d4 100644 --- a/src/Http/Routing/test/UnitTests/Matching/DataSourceDependentMatcherTest.cs +++ b/src/Http/Routing/test/UnitTests/Matching/DataSourceDependentMatcherTest.cs @@ -141,6 +141,102 @@ public void Matcher_UnsuppressedEndpoint_IsUsed() Assert.Same(endpoint, Assert.Single(inner.Endpoints)); } + [Fact] + public void Matcher_ThrowsOnDuplicateEndpoints() + { + // Arrange + var expectedError = "Duplicate endpoint name Foo found. 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. 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")), + "/foo" + )); + + 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. 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(); From bd7269f3877b8e08f5f96c3188df84ff70bb0a23 Mon Sep 17 00:00:00 2001 From: Safia Abdalla Date: Mon, 13 Sep 2021 19:07:08 +0000 Subject: [PATCH 2/3] Add display name to exception message --- .../Routing/src/Matching/DataSourceDependentMatcher.cs | 2 +- .../UnitTests/Matching/DataSourceDependentMatcherTest.cs | 8 ++++---- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/src/Http/Routing/src/Matching/DataSourceDependentMatcher.cs b/src/Http/Routing/src/Matching/DataSourceDependentMatcher.cs index 9e3acc1e4bfa..9a6dccfca820 100644 --- a/src/Http/Routing/src/Matching/DataSourceDependentMatcher.cs +++ b/src/Http/Routing/src/Matching/DataSourceDependentMatcher.cs @@ -55,7 +55,7 @@ private Matcher CreateMatcher(IReadOnlyList endpoints) { if (!seenEndpointNames.Add(endpointName)) { - throw new InvalidOperationException($"Duplicate endpoint name {endpointName} found. Endpoint names must be globally unique."); + throw new InvalidOperationException($"Duplicate endpoint name '{endpointName}' found on '{endpoint.DisplayName}'. Endpoint names must be globally unique."); } } diff --git a/src/Http/Routing/test/UnitTests/Matching/DataSourceDependentMatcherTest.cs b/src/Http/Routing/test/UnitTests/Matching/DataSourceDependentMatcherTest.cs index d99c0b65c4d4..32a96a29d0b3 100644 --- a/src/Http/Routing/test/UnitTests/Matching/DataSourceDependentMatcherTest.cs +++ b/src/Http/Routing/test/UnitTests/Matching/DataSourceDependentMatcherTest.cs @@ -145,7 +145,7 @@ public void Matcher_UnsuppressedEndpoint_IsUsed() public void Matcher_ThrowsOnDuplicateEndpoints() { // Arrange - var expectedError = "Duplicate endpoint name Foo found. Endpoint names must be globally unique."; + var expectedError = "Duplicate endpoint name 'Foo' found on '/bar'. Endpoint names must be globally unique."; var dataSource = new DynamicEndpointDataSource(); var lifetime = new DataSourceDependentMatcher.Lifetime(); dataSource.AddEndpoint(new RouteEndpoint( @@ -173,7 +173,7 @@ public void Matcher_ThrowsOnDuplicateEndpoints() public void Matcher_ThrowsOnDuplicateEndpointsFromMultipleSources() { // Arrange - var expectedError = "Duplicate endpoint name Foo found. Endpoint names must be globally unique."; + var expectedError = "Duplicate endpoint name 'Foo' found on '/foo2'. Endpoint names must be globally unique."; var dataSource = new DynamicEndpointDataSource(); var lifetime = new DataSourceDependentMatcher.Lifetime(); dataSource.AddEndpoint(new RouteEndpoint( @@ -196,7 +196,7 @@ public void Matcher_ThrowsOnDuplicateEndpointsFromMultipleSources() RoutePatternFactory.Parse("/foo2"), 0, new EndpointMetadataCollection(new EndpointNameMetadata("Foo")), - "/foo" + "/foo2" )); var compositeDataSource = new CompositeEndpointDataSource(new[] { dataSource, anotherDataSource }); @@ -211,7 +211,7 @@ public void Matcher_ThrowsOnDuplicateEndpointsFromMultipleSources() public void Matcher_ThrowsOnDuplicateEndpointAddedLater() { // Arrange - var expectedError = "Duplicate endpoint name Foo found. Endpoint names must be globally unique."; + var expectedError = "Duplicate endpoint name 'Foo' found on '/bar'. Endpoint names must be globally unique."; var dataSource = new DynamicEndpointDataSource(); var lifetime = new DataSourceDependentMatcher.Lifetime(); dataSource.AddEndpoint(new RouteEndpoint( From 7299feb22c8e5d728e98aefd129b4cebeb457e3f Mon Sep 17 00:00:00 2001 From: Safia Abdalla Date: Mon, 13 Sep 2021 20:14:09 +0000 Subject: [PATCH 3/3] Always validate duplicate endpoints and add more info to error --- .../src/Matching/DataSourceDependentMatcher.cs | 18 +++++++++++++----- .../Matching/DataSourceDependentMatcherTest.cs | 6 +++--- 2 files changed, 16 insertions(+), 8 deletions(-) diff --git a/src/Http/Routing/src/Matching/DataSourceDependentMatcher.cs b/src/Http/Routing/src/Matching/DataSourceDependentMatcher.cs index 9a6dccfca820..2906028a02a7 100644 --- a/src/Http/Routing/src/Matching/DataSourceDependentMatcher.cs +++ b/src/Http/Routing/src/Matching/DataSourceDependentMatcher.cs @@ -41,25 +41,33 @@ public override Task MatchAsync(HttpContext httpContext) private Matcher CreateMatcher(IReadOnlyList endpoints) { var builder = _matcherBuilderFactory(); - var seenEndpointNames = new HashSet(); + 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) { // Validate that endpoint names are unique. var endpointName = endpoint.Metadata.GetMetadata()?.EndpointName; if (endpointName is not null) { - if (!seenEndpointNames.Add(endpointName)) + if (seenEndpointNames.TryGetValue(endpointName, out var existingEndpoint)) { - throw new InvalidOperationException($"Duplicate endpoint name '{endpointName}' found on '{endpoint.DisplayName}'. Endpoint names must be globally unique."); + 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); } - builder.AddEndpoint(endpoint); + // 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 32a96a29d0b3..88563ba750d0 100644 --- a/src/Http/Routing/test/UnitTests/Matching/DataSourceDependentMatcherTest.cs +++ b/src/Http/Routing/test/UnitTests/Matching/DataSourceDependentMatcherTest.cs @@ -145,7 +145,7 @@ public void Matcher_UnsuppressedEndpoint_IsUsed() public void Matcher_ThrowsOnDuplicateEndpoints() { // Arrange - var expectedError = "Duplicate endpoint name 'Foo' found on '/bar'. Endpoint names must be globally unique."; + 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( @@ -173,7 +173,7 @@ public void Matcher_ThrowsOnDuplicateEndpoints() public void Matcher_ThrowsOnDuplicateEndpointsFromMultipleSources() { // Arrange - var expectedError = "Duplicate endpoint name 'Foo' found on '/foo2'. Endpoint names must be globally unique."; + 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( @@ -211,7 +211,7 @@ public void Matcher_ThrowsOnDuplicateEndpointsFromMultipleSources() public void Matcher_ThrowsOnDuplicateEndpointAddedLater() { // Arrange - var expectedError = "Duplicate endpoint name 'Foo' found on '/bar'. Endpoint names must be globally unique."; + 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(