[release/6.0] Fixed duplicate metadata in ModelDataSource (#36340)

* Fixed duplicate metadata in ModelDataSource - Today when we make endpoints for the ModelDataSource, we apply the callbacks to the EndpointBuilder before creating the endpoint. This happens every time anyone accesses the endpoints property. Instead of re-applying the conventions, we do it once on Build. We also throw if more conventions are added after building. - Added tests

* Apply suggestions from code review

Co-authored-by: Pranav K <prkrishn@hotmail.com>

Co-authored-by: David Fowler <davidfowl@gmail.com>
Co-authored-by: Pranav K <prkrishn@hotmail.com>

src/Http/Routing/src/Builder/RoutingEndpointConventionBuilderExtensions.cs

@@ -130,7 +130,7 @@ namespace Microsoft.AspNetCore.Builder
         /// <returns>The <see cref="IEndpointConventionBuilder"/>.</returns>
         public static TBuilder WithName<TBuilder>(this TBuilder builder, string endpointName) where TBuilder : IEndpointConventionBuilder
         {
-            builder.WithMetadata(new EndpointNameAttribute(endpointName), new RouteNameMetadata(endpointName));
+            builder.WithMetadata(new EndpointNameMetadata(endpointName), new RouteNameMetadata(endpointName));
             return builder;
         }
 

src/Http/Routing/src/DefaultEndpointConventionBuilder.cs

@@ -12,24 +12,37 @@ namespace Microsoft.AspNetCore.Routing
     {
         internal EndpointBuilder EndpointBuilder { get; }
 
-        private readonly List<Action<EndpointBuilder>> _conventions;
+        private List<Action<EndpointBuilder>>? _conventions;
 
         public DefaultEndpointConventionBuilder(EndpointBuilder endpointBuilder)
         {
             EndpointBuilder = endpointBuilder;
-            _conventions = new List<Action<EndpointBuilder>>();
+            _conventions = new();
         }
 
         public void Add(Action<EndpointBuilder> convention)
         {
-            _conventions.Add(convention);
+            var conventions = _conventions;
+
+            if (conventions is null)
+            {
+                throw new InvalidOperationException("Conventions cannot be added after building the endpoint");
+            }
+
+            conventions.Add(convention);
         }
 
         public Endpoint Build()
         {
-            foreach (var convention in _conventions)
+            // Only apply the conventions once
+            var conventions = Interlocked.Exchange(ref _conventions, null);
+
+            if (conventions is not null)
             {
-                convention(EndpointBuilder);
+                foreach (var convention in conventions)
+                {
+                    convention(EndpointBuilder);
+                }
             }
 
             return EndpointBuilder.Build();

src/Http/Routing/test/UnitTests/Builder/DelegateEndpointRouteBuilderExtensionsTest.cs

@@ -24,28 +24,32 @@ namespace Microsoft.AspNetCore.Builder
             return Assert.IsType<RouteEndpointBuilder>(Assert.Single(GetBuilderEndpointDataSource(endpointRouteBuilder).EndpointBuilders));
         }
 
-        public static object[][] MapMethods
+        public static object?[]?[] MapMethods
         {
             get
             {
-                void MapGet(IEndpointRouteBuilder routes, string template, Delegate action) =>
+                IEndpointConventionBuilder MapGet(IEndpointRouteBuilder routes, string template, Delegate action) =>
                     routes.MapGet(template, action);
 
-                void MapPost(IEndpointRouteBuilder routes, string template, Delegate action) =>
+                IEndpointConventionBuilder MapPost(IEndpointRouteBuilder routes, string template, Delegate action) =>
                     routes.MapPost(template, action);
 
-                void MapPut(IEndpointRouteBuilder routes, string template, Delegate action) =>
+                IEndpointConventionBuilder MapPut(IEndpointRouteBuilder routes, string template, Delegate action) =>
                     routes.MapPut(template, action);
 
-                void MapDelete(IEndpointRouteBuilder routes, string template, Delegate action) =>
+                IEndpointConventionBuilder MapDelete(IEndpointRouteBuilder routes, string template, Delegate action) =>
                     routes.MapDelete(template, action);
 
-                return new object[][]
+                IEndpointConventionBuilder Map(IEndpointRouteBuilder routes, string template, Delegate action) =>
+                    routes.Map(template, action);
+
+                return new object?[]?[]
                 {
-                    new object[] { (Action<IEndpointRouteBuilder, string, Delegate>)MapGet, "GET" },
-                    new object[] { (Action<IEndpointRouteBuilder, string, Delegate>)MapPost, "POST" },
-                    new object[] { (Action<IEndpointRouteBuilder, string, Delegate>)MapPut, "PUT" },
-                    new object[] { (Action<IEndpointRouteBuilder, string, Delegate>)MapDelete, "DELETE" },
+                    new object?[] { (Func<IEndpointRouteBuilder, string, Delegate, IEndpointConventionBuilder>)MapGet, "GET" },
+                    new object?[] { (Func<IEndpointRouteBuilder, string, Delegate, IEndpointConventionBuilder>)MapPost, "POST" },
+                    new object?[] { (Func<IEndpointRouteBuilder, string, Delegate, IEndpointConventionBuilder>)MapPut, "PUT" },
+                    new object?[] { (Func<IEndpointRouteBuilder, string, Delegate, IEndpointConventionBuilder>)MapDelete, "DELETE" },
+                    new object?[] { (Func<IEndpointRouteBuilder, string, Delegate, IEndpointConventionBuilder>)Map, null },
                 };
             }
         }
@@ -178,7 +182,61 @@ namespace Microsoft.AspNetCore.Builder
 
         [Theory]
         [MemberData(nameof(MapMethods))]
-        public async Task MapVerbWithExplicitRouteParameterIsCaseInsensitive(Action<IEndpointRouteBuilder, string, Delegate> map, string expectedMethod)
+        public void MapVerbDoesNotDuplicateMetadata(Func<IEndpointRouteBuilder, string, Delegate, IEndpointConventionBuilder> map, string expectedMethod)
+        {
+            var builder = new DefaultEndpointRouteBuilder(new ApplicationBuilder(new EmptyServiceProvider()));
+
+            map(builder, "/{ID}", () => { }).WithName("Foo");
+
+            var dataSource = GetBuilderEndpointDataSource(builder);
+
+            // Access endpoints a couple of times to make sure it gets built
+            _ = dataSource.Endpoints;
+            _ = dataSource.Endpoints;
+            _ = dataSource.Endpoints;
+
+            var endpoint = Assert.Single(dataSource.Endpoints);
+
+            var endpointNameMetadata = Assert.Single(endpoint.Metadata.GetOrderedMetadata<IEndpointNameMetadata>());
+            var routeNameMetadata = Assert.Single(endpoint.Metadata.GetOrderedMetadata<IRouteNameMetadata>());
+            Assert.Equal("Foo", endpointNameMetadata.EndpointName);
+            Assert.Equal("Foo", routeNameMetadata.RouteName);
+
+            if (expectedMethod is not null)
+            {
+                var methodMetadata = endpoint.Metadata.GetMetadata<IHttpMethodMetadata>();
+                Assert.NotNull(methodMetadata);
+                var method = Assert.Single(methodMetadata!.HttpMethods);
+                Assert.Equal(expectedMethod, method);
+            }
+        }
+
+        [Theory]
+        [MemberData(nameof(MapMethods))]
+        public void AddingMetadataAfterBuildingEndpointThrows(Func<IEndpointRouteBuilder, string, Delegate, IEndpointConventionBuilder> map, string expectedMethod)
+        {
+            var builder = new DefaultEndpointRouteBuilder(new ApplicationBuilder(new EmptyServiceProvider()));
+
+            var endpointBuilder = map(builder, "/{ID}", () => { });
+
+            var dataSource = GetBuilderEndpointDataSource(builder);
+
+            var endpoint = Assert.Single(dataSource.Endpoints);
+
+            if (expectedMethod is not null)
+            {
+                var methodMetadata = endpoint.Metadata.GetMetadata<IHttpMethodMetadata>();
+                Assert.NotNull(methodMetadata);
+                var method = Assert.Single(methodMetadata!.HttpMethods);
+                Assert.Equal(expectedMethod, method);
+            }
+
+            Assert.Throws<InvalidOperationException>(() => endpointBuilder.WithMetadata(new RouteNameMetadata("Foo")));
+        }
+
+        [Theory]
+        [MemberData(nameof(MapMethods))]
+        public async Task MapVerbWithExplicitRouteParameterIsCaseInsensitive(Func<IEndpointRouteBuilder, string, Delegate, IEndpointConventionBuilder> map, string expectedMethod)
         {
             var builder = new DefaultEndpointRouteBuilder(new ApplicationBuilder(new EmptyServiceProvider()));
 
@@ -194,13 +252,19 @@ namespace Microsoft.AspNetCore.Builder
             // Trigger Endpoint build by calling getter.
             var endpoint = Assert.Single(dataSource.Endpoints);
 
-            var methodMetadata = endpoint.Metadata.GetMetadata<IHttpMethodMetadata>();
-            Assert.NotNull(methodMetadata);
-            var method = Assert.Single(methodMetadata!.HttpMethods);
-            Assert.Equal(expectedMethod, method);
+            if (expectedMethod is not null)
+            {
+                var methodMetadata = endpoint.Metadata.GetMetadata<IHttpMethodMetadata>();
+                Assert.NotNull(methodMetadata);
+                var method = Assert.Single(methodMetadata!.HttpMethods);
+                Assert.Equal(expectedMethod, method);
+            }
 
             var routeEndpointBuilder = GetRouteEndpointBuilder(builder);
-            Assert.Equal($"HTTP: {expectedMethod} /{{ID}}", routeEndpointBuilder.DisplayName);
+            if (expectedMethod is not null)
+            {
+                Assert.Equal($"HTTP: {expectedMethod} /{{ID}}", routeEndpointBuilder.DisplayName);
+            }
             Assert.Equal($"/{{ID}}", routeEndpointBuilder.RoutePattern.RawText);
 
             var httpContext = new DefaultHttpContext();
@@ -214,7 +278,7 @@ namespace Microsoft.AspNetCore.Builder
 
         [Theory]
         [MemberData(nameof(MapMethods))]
-        public async Task MapVerbWithRouteParameterDoesNotFallbackToQuery(Action<IEndpointRouteBuilder, string, Delegate> map, string expectedMethod)
+        public async Task MapVerbWithRouteParameterDoesNotFallbackToQuery(Func<IEndpointRouteBuilder, string, Delegate, IEndpointConventionBuilder> map, string expectedMethod)
         {
             var builder = new DefaultEndpointRouteBuilder(new ApplicationBuilder(new EmptyServiceProvider()));
 
@@ -229,14 +293,19 @@ namespace Microsoft.AspNetCore.Builder
             var dataSource = GetBuilderEndpointDataSource(builder);
             // Trigger Endpoint build by calling getter.
             var endpoint = Assert.Single(dataSource.Endpoints);
-
-            var methodMetadata = endpoint.Metadata.GetMetadata<IHttpMethodMetadata>();
-            Assert.NotNull(methodMetadata);
-            var method = Assert.Single(methodMetadata!.HttpMethods);
-            Assert.Equal(expectedMethod, method);
+            if (expectedMethod is not null)
+            {
+                var methodMetadata = endpoint.Metadata.GetMetadata<IHttpMethodMetadata>();
+                Assert.NotNull(methodMetadata);
+                var method = Assert.Single(methodMetadata!.HttpMethods);
+                Assert.Equal(expectedMethod, method);
+            }
 
             var routeEndpointBuilder = GetRouteEndpointBuilder(builder);
-            Assert.Equal($"HTTP: {expectedMethod} /{{ID}}", routeEndpointBuilder.DisplayName);
+            if (expectedMethod is not null)
+            {
+                Assert.Equal($"HTTP: {expectedMethod} /{{ID}}", routeEndpointBuilder.DisplayName);
+            }
             Assert.Equal($"/{{ID}}", routeEndpointBuilder.RoutePattern.RawText);
 
             // Assert that we don't fallback to the query string

src/Http/Routing/test/UnitTests/Builder/MapEndpointEndpointDataSourceBuilderExtensionsTest.cs

@@ -26,6 +26,36 @@ namespace Microsoft.AspNetCore.Builder
             return Assert.IsType<RouteEndpointBuilder>(Assert.Single(GetBuilderEndpointDataSource(endpointRouteBuilder).EndpointBuilders));
         }
 
+        public static object[][] MapMethods
+        {
+            get
+            {
+                IEndpointConventionBuilder MapGet(IEndpointRouteBuilder routes, string template, RequestDelegate action) =>
+                    routes.MapGet(template, action);
+
+                IEndpointConventionBuilder MapPost(IEndpointRouteBuilder routes, string template, RequestDelegate action) =>
+                    routes.MapPost(template, action);
+
+                IEndpointConventionBuilder MapPut(IEndpointRouteBuilder routes, string template, RequestDelegate action) =>
+                    routes.MapPut(template, action);
+
+                IEndpointConventionBuilder MapDelete(IEndpointRouteBuilder routes, string template, RequestDelegate action) =>
+                    routes.MapDelete(template, action);
+
+                IEndpointConventionBuilder Map(IEndpointRouteBuilder routes, string template, RequestDelegate action) =>
+                    routes.Map(template, action);
+
+                return new object[][]
+                {
+                    new object[] { (Func<IEndpointRouteBuilder, string, RequestDelegate, IEndpointConventionBuilder>)MapGet },
+                    new object[] { (Func<IEndpointRouteBuilder, string, RequestDelegate, IEndpointConventionBuilder>)MapPost },
+                    new object[] { (Func<IEndpointRouteBuilder, string, RequestDelegate, IEndpointConventionBuilder>)MapPut },
+                    new object[] { (Func<IEndpointRouteBuilder, string, RequestDelegate, IEndpointConventionBuilder>)MapDelete },
+                    new object[] { (Func<IEndpointRouteBuilder, string, RequestDelegate, IEndpointConventionBuilder>)Map },
+                };
+            }
+        }
+
         [Fact]
         public void MapEndpoint_StringPattern_BuildsEndpoint()
         {
@@ -69,7 +99,7 @@ namespace Microsoft.AspNetCore.Builder
             var builder = new DefaultEndpointRouteBuilder(Mock.Of<IApplicationBuilder>());
 
             // Act
-            var endpointBuilder = builder.Map(RoutePatternFactory.Parse("/"),  Handle);
+            var endpointBuilder = builder.Map(RoutePatternFactory.Parse("/"), Handle);
 
             // Assert
             var endpointBuilder1 = GetRouteEndpointBuilder(builder);
@@ -123,6 +153,49 @@ namespace Microsoft.AspNetCore.Builder
             }
         }
 
+        [Theory]
+        [MemberData(nameof(MapMethods))]
+        public void Map_EndpointMetadataNotDuplicated(Func<IEndpointRouteBuilder, string, RequestDelegate, IEndpointConventionBuilder> map)
+        {
+            // Arrange
+            var builder = new DefaultEndpointRouteBuilder(Mock.Of<IApplicationBuilder>());
+
+            // Act
+            var endpointBuilder = map(builder, "/", context => Task.CompletedTask).WithMetadata(new EndpointNameMetadata("MapMe"));
+
+            // Assert
+            var ds = GetBuilderEndpointDataSource(builder);
+
+            _ = ds.Endpoints;
+            _ = ds.Endpoints;
+            _ = ds.Endpoints;
+
+            Assert.Single(ds.Endpoints);
+            var endpoint = ds.Endpoints.Single();
+
+            Assert.Single(endpoint.Metadata.GetOrderedMetadata<IEndpointNameMetadata>());
+        }
+
+        [Theory]
+        [MemberData(nameof(MapMethods))]
+        public void AddingMetadataAfterBuildingEndpointThrows(Func<IEndpointRouteBuilder, string, RequestDelegate, IEndpointConventionBuilder> map)
+        {
+            // Arrange
+            var builder = new DefaultEndpointRouteBuilder(Mock.Of<IApplicationBuilder>());
+
+            // Act
+            var endpointBuilder = map(builder, "/", context => Task.CompletedTask).WithMetadata(new EndpointNameMetadata("MapMe"));
+
+            // Assert
+            var ds = GetBuilderEndpointDataSource(builder);
+
+            var endpoint = Assert.Single(ds.Endpoints);
+
+            Assert.Single(endpoint.Metadata.GetOrderedMetadata<IEndpointNameMetadata>());
+
+            Assert.Throws<InvalidOperationException>(() => endpointBuilder.WithMetadata(new RouteNameMetadata("Foo")));
+        }
+
         [Attribute1]
         [Attribute2]
         private static Task Handle(HttpContext context) => Task.CompletedTask;