From d78f8031e5be5d726fd96aa927663e710118b3a6 Mon Sep 17 00:00:00 2001 From: Bruno Oliveira Date: Tue, 18 Jan 2022 10:00:49 -0800 Subject: [PATCH 1/9] Changing from Path to ModelBinding when inferred --- .../src/DefaultApiDescriptionProvider.cs | 17 ++++++++++ .../test/DefaultApiDescriptionProviderTest.cs | 34 +++++++++++++++++++ 2 files changed, 51 insertions(+) diff --git a/src/Mvc/Mvc.ApiExplorer/src/DefaultApiDescriptionProvider.cs b/src/Mvc/Mvc.ApiExplorer/src/DefaultApiDescriptionProvider.cs index 2096d88decb9..9b9b6330a853 100644 --- a/src/Mvc/Mvc.ApiExplorer/src/DefaultApiDescriptionProvider.cs +++ b/src/Mvc/Mvc.ApiExplorer/src/DefaultApiDescriptionProvider.cs @@ -2,12 +2,15 @@ // The .NET Foundation licenses this file to you under the MIT license. using System.Linq; + +using Microsoft.AspNetCore.Http.Metadata; using Microsoft.AspNetCore.Mvc.Abstractions; using Microsoft.AspNetCore.Mvc.ActionConstraints; using Microsoft.AspNetCore.Mvc.Controllers; using Microsoft.AspNetCore.Mvc.Formatters; using Microsoft.AspNetCore.Mvc.Infrastructure; using Microsoft.AspNetCore.Mvc.ModelBinding; +using Microsoft.AspNetCore.Mvc.ModelBinding.Metadata; using Microsoft.AspNetCore.Routing; using Microsoft.AspNetCore.Routing.Template; using Microsoft.Extensions.Options; @@ -258,6 +261,20 @@ private void ProcessRouteParameters(ApiParameterContext context) parameter.Source = BindingSource.Path; } } + else + { + if (parameter.Source == BindingSource.Path && + parameter.ModelMetadata is DefaultModelMetadata defaultModelMetadata && + !defaultModelMetadata.Attributes.Attributes.OfType().Any()) + { + // If we didn't see the parameter in the route and no FromRoute metadata is set, it probably means + // the parameter binding source was inferred (InferParameterBindingInfoConvention) + // probably because another route to this action contains it as route parameter. + // let's switch it to ModelBinding + parameter.Source = BindingSource.ModelBinding; + } + + } } } diff --git a/src/Mvc/Mvc.ApiExplorer/test/DefaultApiDescriptionProviderTest.cs b/src/Mvc/Mvc.ApiExplorer/test/DefaultApiDescriptionProviderTest.cs index 09663da47c5c..100c60ed7a75 100644 --- a/src/Mvc/Mvc.ApiExplorer/test/DefaultApiDescriptionProviderTest.cs +++ b/src/Mvc/Mvc.ApiExplorer/test/DefaultApiDescriptionProviderTest.cs @@ -243,6 +243,40 @@ public void GetApiDescription_PopulatesParametersThatAppearOnRouteTemplate_AndHa } } + [Theory] + [InlineData("api/products/{id}", nameof(BindingSource.Path), nameof(BindingSource.Path))] + [InlineData("api/products", nameof(BindingSource.Path), nameof(BindingSource.ModelBinding))] + [InlineData("api/products", nameof(BindingSource.Body), nameof(BindingSource.Body))] + public void GetApiDescription_WithInferredBindingSource( + string template, + string inferredBindingSourceName, + string expectedBindingSourceName) + { + // Arrange + var action = CreateActionDescriptor(nameof(FromModelBinding)); + action.AttributeRouteInfo = new AttributeRouteInfo { Template = template }; + + foreach (var actionParameter in action.Parameters) + { + actionParameter.BindingInfo = new BindingInfo() + { + BindingSource = new BindingSource(inferredBindingSourceName, displayName: null, isGreedy: false, isFromRequest: true) + }; + } + + var expectedBindingSource = new BindingSource(expectedBindingSourceName, displayName: null, isGreedy: false, isFromRequest: true); + + // Act + var descriptions = GetApiDescriptions(action); + + // Assert + var description = Assert.Single(descriptions); + + var parameter = Assert.Single(description.ParameterDescriptions); + Assert.Equal(expectedBindingSource, parameter.Source); + Assert.Equal("id", parameter.Name); + } + [Fact] public void GetApiDescription_ParameterDescription_IncludesParameterDescriptor() { From f9ab96ecd289c4f0f7ccd567bcf1d250585aec1b Mon Sep 17 00:00:00 2001 From: Bruno Oliveira Date: Tue, 18 Jan 2022 10:11:46 -0800 Subject: [PATCH 2/9] Removing whitespace --- src/Mvc/Mvc.ApiExplorer/src/DefaultApiDescriptionProvider.cs | 1 - 1 file changed, 1 deletion(-) diff --git a/src/Mvc/Mvc.ApiExplorer/src/DefaultApiDescriptionProvider.cs b/src/Mvc/Mvc.ApiExplorer/src/DefaultApiDescriptionProvider.cs index 9b9b6330a853..e097f53f5892 100644 --- a/src/Mvc/Mvc.ApiExplorer/src/DefaultApiDescriptionProvider.cs +++ b/src/Mvc/Mvc.ApiExplorer/src/DefaultApiDescriptionProvider.cs @@ -2,7 +2,6 @@ // The .NET Foundation licenses this file to you under the MIT license. using System.Linq; - using Microsoft.AspNetCore.Http.Metadata; using Microsoft.AspNetCore.Mvc.Abstractions; using Microsoft.AspNetCore.Mvc.ActionConstraints; From cc4dae228dbbd01ae10607ab16d44174fd133b77 Mon Sep 17 00:00:00 2001 From: Bruno Oliveira Date: Fri, 21 Jan 2022 11:55:05 -0800 Subject: [PATCH 3/9] Removing the inferred paraemter when not in the route --- .../src/DefaultApiDescriptionProvider.cs | 12 ++++++++-- .../test/DefaultApiDescriptionProviderTest.cs | 22 ++++++++++++------- 2 files changed, 24 insertions(+), 10 deletions(-) diff --git a/src/Mvc/Mvc.ApiExplorer/src/DefaultApiDescriptionProvider.cs b/src/Mvc/Mvc.ApiExplorer/src/DefaultApiDescriptionProvider.cs index e097f53f5892..958005b654f8 100644 --- a/src/Mvc/Mvc.ApiExplorer/src/DefaultApiDescriptionProvider.cs +++ b/src/Mvc/Mvc.ApiExplorer/src/DefaultApiDescriptionProvider.cs @@ -241,6 +241,7 @@ private void ProcessRouteParameters(ApiParameterContext context) routeParameters.Add(routeParameter.Name!, CreateRouteInfo(routeParameter)); } + var inferredPathParametersToRemove = new List(); foreach (var parameter in context.Results) { if (parameter.Source == BindingSource.Path || @@ -269,14 +270,21 @@ parameter.ModelMetadata is DefaultModelMetadata defaultModelMetadata && // If we didn't see the parameter in the route and no FromRoute metadata is set, it probably means // the parameter binding source was inferred (InferParameterBindingInfoConvention) // probably because another route to this action contains it as route parameter. - // let's switch it to ModelBinding - parameter.Source = BindingSource.ModelBinding; + // let's add it to be removed from the API description + inferredPathParametersToRemove.Add(parameter); } } } } + // Remove all parameters detected as Inferred FromPath + // but that are not in the route + foreach (var inferredParameter in inferredPathParametersToRemove) + { + context.Results.Remove(inferredParameter); + } + // Lastly, create a parameter representation for each route parameter that did not find // a partner. foreach (var routeParameter in routeParameters) diff --git a/src/Mvc/Mvc.ApiExplorer/test/DefaultApiDescriptionProviderTest.cs b/src/Mvc/Mvc.ApiExplorer/test/DefaultApiDescriptionProviderTest.cs index 100c60ed7a75..5eb29258579a 100644 --- a/src/Mvc/Mvc.ApiExplorer/test/DefaultApiDescriptionProviderTest.cs +++ b/src/Mvc/Mvc.ApiExplorer/test/DefaultApiDescriptionProviderTest.cs @@ -244,13 +244,14 @@ public void GetApiDescription_PopulatesParametersThatAppearOnRouteTemplate_AndHa } [Theory] - [InlineData("api/products/{id}", nameof(BindingSource.Path), nameof(BindingSource.Path))] - [InlineData("api/products", nameof(BindingSource.Path), nameof(BindingSource.ModelBinding))] - [InlineData("api/products", nameof(BindingSource.Body), nameof(BindingSource.Body))] + [InlineData("api/products/{id}", nameof(BindingSource.Path), true)] + [InlineData("api/products", nameof(BindingSource.Path), false)] + [InlineData("api/products", nameof(BindingSource.Body), true)] + [InlineData("api/products", nameof(BindingSource.Query), true)] public void GetApiDescription_WithInferredBindingSource( string template, string inferredBindingSourceName, - string expectedBindingSourceName) + bool inferredParameterShouldBeIncluded) { // Arrange var action = CreateActionDescriptor(nameof(FromModelBinding)); @@ -264,7 +265,7 @@ public void GetApiDescription_WithInferredBindingSource( }; } - var expectedBindingSource = new BindingSource(expectedBindingSourceName, displayName: null, isGreedy: false, isFromRequest: true); + var expectedBindingSource = new BindingSource(inferredBindingSourceName, displayName: null, isGreedy: false, isFromRequest: true); // Act var descriptions = GetApiDescriptions(action); @@ -272,9 +273,14 @@ public void GetApiDescription_WithInferredBindingSource( // Assert var description = Assert.Single(descriptions); - var parameter = Assert.Single(description.ParameterDescriptions); - Assert.Equal(expectedBindingSource, parameter.Source); - Assert.Equal("id", parameter.Name); + Assert.Equal(inferredParameterShouldBeIncluded, description.ParameterDescriptions.Count == 1); + + if (inferredParameterShouldBeIncluded) + { + var parameter = Assert.Single(description.ParameterDescriptions); + Assert.Equal(expectedBindingSource, parameter.Source); + Assert.Equal("id", parameter.Name); + } } [Fact] From 8cd9eeb16cb62fea9d5d5b2b88b9064d3a9ecf6c Mon Sep 17 00:00:00 2001 From: Bruno Oliveira Date: Mon, 24 Jan 2022 13:28:37 -0800 Subject: [PATCH 4/9] Avoiding extra allocation --- .../src/DefaultApiDescriptionProvider.cs | 22 +++---- .../test/DefaultApiDescriptionProviderTest.cs | 63 ++++++++++++------- 2 files changed, 47 insertions(+), 38 deletions(-) diff --git a/src/Mvc/Mvc.ApiExplorer/src/DefaultApiDescriptionProvider.cs b/src/Mvc/Mvc.ApiExplorer/src/DefaultApiDescriptionProvider.cs index 958005b654f8..6737e48f72c9 100644 --- a/src/Mvc/Mvc.ApiExplorer/src/DefaultApiDescriptionProvider.cs +++ b/src/Mvc/Mvc.ApiExplorer/src/DefaultApiDescriptionProvider.cs @@ -241,12 +241,13 @@ private void ProcessRouteParameters(ApiParameterContext context) routeParameters.Add(routeParameter.Name!, CreateRouteInfo(routeParameter)); } - var inferredPathParametersToRemove = new List(); - foreach (var parameter in context.Results) + for (var i = context.Results.Count - 1; i >= 0; i--) { + var parameter = context.Results[i]; + if (parameter.Source == BindingSource.Path || - parameter.Source == BindingSource.ModelBinding || - parameter.Source == BindingSource.Custom) + parameter.Source == BindingSource.ModelBinding || + parameter.Source == BindingSource.Custom) { if (routeParameters.TryGetValue(parameter.Name, out var routeInfo)) { @@ -269,22 +270,15 @@ parameter.ModelMetadata is DefaultModelMetadata defaultModelMetadata && { // If we didn't see the parameter in the route and no FromRoute metadata is set, it probably means // the parameter binding source was inferred (InferParameterBindingInfoConvention) - // probably because another route to this action contains it as route parameter. - // let's add it to be removed from the API description - inferredPathParametersToRemove.Add(parameter); + // probably because another route to this action contains it as route parameter and + // will be emoved from the API description + context.Results.RemoveAt(i); } } } } - // Remove all parameters detected as Inferred FromPath - // but that are not in the route - foreach (var inferredParameter in inferredPathParametersToRemove) - { - context.Results.Remove(inferredParameter); - } - // Lastly, create a parameter representation for each route parameter that did not find // a partner. foreach (var routeParameter in routeParameters) diff --git a/src/Mvc/Mvc.ApiExplorer/test/DefaultApiDescriptionProviderTest.cs b/src/Mvc/Mvc.ApiExplorer/test/DefaultApiDescriptionProviderTest.cs index 5eb29258579a..84a3577e927f 100644 --- a/src/Mvc/Mvc.ApiExplorer/test/DefaultApiDescriptionProviderTest.cs +++ b/src/Mvc/Mvc.ApiExplorer/test/DefaultApiDescriptionProviderTest.cs @@ -5,6 +5,7 @@ using System.ComponentModel.DataAnnotations; using System.Reflection; using System.Text; + using Microsoft.AspNetCore.Http; using Microsoft.AspNetCore.Mvc.Abstractions; using Microsoft.AspNetCore.Mvc.ActionConstraints; @@ -21,6 +22,7 @@ using Microsoft.AspNetCore.Routing.Template; using Microsoft.Extensions.Options; using Microsoft.Net.Http.Headers; + using Moq; namespace Microsoft.AspNetCore.Mvc.Description; @@ -243,44 +245,57 @@ public void GetApiDescription_PopulatesParametersThatAppearOnRouteTemplate_AndHa } } - [Theory] - [InlineData("api/products/{id}", nameof(BindingSource.Path), true)] - [InlineData("api/products", nameof(BindingSource.Path), false)] - [InlineData("api/products", nameof(BindingSource.Body), true)] - [InlineData("api/products", nameof(BindingSource.Query), true)] - public void GetApiDescription_WithInferredBindingSource( - string template, - string inferredBindingSourceName, - bool inferredParameterShouldBeIncluded) + [Fact] + public void GetApiDescription_WithInferredBindingSource_ExcludesPathParametersWhenNotPresentInRoute() { // Arrange var action = CreateActionDescriptor(nameof(FromModelBinding)); - action.AttributeRouteInfo = new AttributeRouteInfo { Template = template }; + action.AttributeRouteInfo = new AttributeRouteInfo { Template = "api/products" }; - foreach (var actionParameter in action.Parameters) + action.Parameters[0].BindingInfo = new BindingInfo() { - actionParameter.BindingInfo = new BindingInfo() - { - BindingSource = new BindingSource(inferredBindingSourceName, displayName: null, isGreedy: false, isFromRequest: true) - }; - } - - var expectedBindingSource = new BindingSource(inferredBindingSourceName, displayName: null, isGreedy: false, isFromRequest: true); + BindingSource = new BindingSource("Path", displayName: null, isGreedy: false, isFromRequest: true) + }; // Act var descriptions = GetApiDescriptions(action); // Assert var description = Assert.Single(descriptions); + Assert.Empty(description.ParameterDescriptions); + } - Assert.Equal(inferredParameterShouldBeIncluded, description.ParameterDescriptions.Count == 1); - if (inferredParameterShouldBeIncluded) + [Theory] + [InlineData("api/products/{id}")] + [InlineData("api/products/{id?}")] + [InlineData("api/products/{id=5}")] + [InlineData("api/products/{id:int}")] + [InlineData("api/products/{id:int?}")] + [InlineData("api/products/{id:int=5}")] + [InlineData("api/products/{*id}")] + [InlineData("api/products/{*id:int}")] + [InlineData("api/products/{*id:int=5}")] + public void GetApiDescription_WithInferredBindingSource_IncludesPathParametersWhenPresentInRoute(string template) + { + // Arrange + var action = CreateActionDescriptor(nameof(FromModelBinding)); + action.AttributeRouteInfo = new AttributeRouteInfo { Template = template }; + + var bindingSource = new BindingSource("Path", displayName: null, isGreedy: false, isFromRequest: true); + action.Parameters[0].BindingInfo = new BindingInfo() { - var parameter = Assert.Single(description.ParameterDescriptions); - Assert.Equal(expectedBindingSource, parameter.Source); - Assert.Equal("id", parameter.Name); - } + BindingSource = bindingSource + }; + + // Act + var descriptions = GetApiDescriptions(action); + + // Assert + var description = Assert.Single(descriptions); + var parameter = Assert.Single(description.ParameterDescriptions); + Assert.Equal(bindingSource, parameter.Source); + Assert.Equal("id", parameter.Name); } [Fact] From c87e3601df8c5687027c78262a2d1e3808928615 Mon Sep 17 00:00:00 2001 From: Bruno Oliveira Date: Mon, 24 Jan 2022 13:31:54 -0800 Subject: [PATCH 5/9] Remove extra line --- src/Mvc/Mvc.ApiExplorer/src/DefaultApiDescriptionProvider.cs | 1 - 1 file changed, 1 deletion(-) diff --git a/src/Mvc/Mvc.ApiExplorer/src/DefaultApiDescriptionProvider.cs b/src/Mvc/Mvc.ApiExplorer/src/DefaultApiDescriptionProvider.cs index 6737e48f72c9..9e9bb4b1eee6 100644 --- a/src/Mvc/Mvc.ApiExplorer/src/DefaultApiDescriptionProvider.cs +++ b/src/Mvc/Mvc.ApiExplorer/src/DefaultApiDescriptionProvider.cs @@ -274,7 +274,6 @@ parameter.ModelMetadata is DefaultModelMetadata defaultModelMetadata && // will be emoved from the API description context.Results.RemoveAt(i); } - } } } From 3d1046a54e65357904751ce8ae9bf36b402571f2 Mon Sep 17 00:00:00 2001 From: Bruno Oliveira Date: Mon, 24 Jan 2022 13:32:38 -0800 Subject: [PATCH 6/9] Updating comment --- src/Mvc/Mvc.ApiExplorer/src/DefaultApiDescriptionProvider.cs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/Mvc/Mvc.ApiExplorer/src/DefaultApiDescriptionProvider.cs b/src/Mvc/Mvc.ApiExplorer/src/DefaultApiDescriptionProvider.cs index 9e9bb4b1eee6..f320ba2803d4 100644 --- a/src/Mvc/Mvc.ApiExplorer/src/DefaultApiDescriptionProvider.cs +++ b/src/Mvc/Mvc.ApiExplorer/src/DefaultApiDescriptionProvider.cs @@ -271,7 +271,8 @@ parameter.ModelMetadata is DefaultModelMetadata defaultModelMetadata && // If we didn't see the parameter in the route and no FromRoute metadata is set, it probably means // the parameter binding source was inferred (InferParameterBindingInfoConvention) // probably because another route to this action contains it as route parameter and - // will be emoved from the API description + // will be removed from the API description + // https://github.com/dotnet/aspnetcore/issues/26234 context.Results.RemoveAt(i); } } From 80d36a8abc6f0baa142514d1aa379b25db162f81 Mon Sep 17 00:00:00 2001 From: Bruno Oliveira Date: Mon, 24 Jan 2022 13:35:15 -0800 Subject: [PATCH 7/9] Updating test to use BindingSource.Path --- .../test/DefaultApiDescriptionProviderTest.cs | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/src/Mvc/Mvc.ApiExplorer/test/DefaultApiDescriptionProviderTest.cs b/src/Mvc/Mvc.ApiExplorer/test/DefaultApiDescriptionProviderTest.cs index 84a3577e927f..83d9027343cb 100644 --- a/src/Mvc/Mvc.ApiExplorer/test/DefaultApiDescriptionProviderTest.cs +++ b/src/Mvc/Mvc.ApiExplorer/test/DefaultApiDescriptionProviderTest.cs @@ -254,7 +254,7 @@ public void GetApiDescription_WithInferredBindingSource_ExcludesPathParametersWh action.Parameters[0].BindingInfo = new BindingInfo() { - BindingSource = new BindingSource("Path", displayName: null, isGreedy: false, isFromRequest: true) + BindingSource = BindingSource.Path }; // Act @@ -282,10 +282,9 @@ public void GetApiDescription_WithInferredBindingSource_IncludesPathParametersWh var action = CreateActionDescriptor(nameof(FromModelBinding)); action.AttributeRouteInfo = new AttributeRouteInfo { Template = template }; - var bindingSource = new BindingSource("Path", displayName: null, isGreedy: false, isFromRequest: true); action.Parameters[0].BindingInfo = new BindingInfo() { - BindingSource = bindingSource + BindingSource = BindingSource.Path }; // Act @@ -294,7 +293,7 @@ public void GetApiDescription_WithInferredBindingSource_IncludesPathParametersWh // Assert var description = Assert.Single(descriptions); var parameter = Assert.Single(description.ParameterDescriptions); - Assert.Equal(bindingSource, parameter.Source); + Assert.Equal(BindingSource.Path, parameter.Source); Assert.Equal("id", parameter.Name); } From d7084a877776b27308eb8876a51874cc8bbfdbc0 Mon Sep 17 00:00:00 2001 From: Bruno Oliveira Date: Mon, 24 Jan 2022 13:36:55 -0800 Subject: [PATCH 8/9] Remove extra line --- .../Mvc.ApiExplorer/test/DefaultApiDescriptionProviderTest.cs | 3 --- 1 file changed, 3 deletions(-) diff --git a/src/Mvc/Mvc.ApiExplorer/test/DefaultApiDescriptionProviderTest.cs b/src/Mvc/Mvc.ApiExplorer/test/DefaultApiDescriptionProviderTest.cs index 83d9027343cb..67ccd80e75e8 100644 --- a/src/Mvc/Mvc.ApiExplorer/test/DefaultApiDescriptionProviderTest.cs +++ b/src/Mvc/Mvc.ApiExplorer/test/DefaultApiDescriptionProviderTest.cs @@ -5,7 +5,6 @@ using System.ComponentModel.DataAnnotations; using System.Reflection; using System.Text; - using Microsoft.AspNetCore.Http; using Microsoft.AspNetCore.Mvc.Abstractions; using Microsoft.AspNetCore.Mvc.ActionConstraints; @@ -22,7 +21,6 @@ using Microsoft.AspNetCore.Routing.Template; using Microsoft.Extensions.Options; using Microsoft.Net.Http.Headers; - using Moq; namespace Microsoft.AspNetCore.Mvc.Description; @@ -265,7 +263,6 @@ public void GetApiDescription_WithInferredBindingSource_ExcludesPathParametersWh Assert.Empty(description.ParameterDescriptions); } - [Theory] [InlineData("api/products/{id}")] [InlineData("api/products/{id?}")] From 31b85d780eb1384759891dcab83c05bd68f44208 Mon Sep 17 00:00:00 2001 From: Bruno Oliveira Date: Tue, 25 Jan 2022 08:25:02 -0800 Subject: [PATCH 9/9] Update src/Mvc/Mvc.ApiExplorer/src/DefaultApiDescriptionProvider.cs Co-authored-by: Stephen Halter --- src/Mvc/Mvc.ApiExplorer/src/DefaultApiDescriptionProvider.cs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/Mvc/Mvc.ApiExplorer/src/DefaultApiDescriptionProvider.cs b/src/Mvc/Mvc.ApiExplorer/src/DefaultApiDescriptionProvider.cs index f320ba2803d4..879c425f183e 100644 --- a/src/Mvc/Mvc.ApiExplorer/src/DefaultApiDescriptionProvider.cs +++ b/src/Mvc/Mvc.ApiExplorer/src/DefaultApiDescriptionProvider.cs @@ -246,8 +246,8 @@ private void ProcessRouteParameters(ApiParameterContext context) var parameter = context.Results[i]; if (parameter.Source == BindingSource.Path || - parameter.Source == BindingSource.ModelBinding || - parameter.Source == BindingSource.Custom) + parameter.Source == BindingSource.ModelBinding || + parameter.Source == BindingSource.Custom) { if (routeParameters.TryGetValue(parameter.Name, out var routeInfo)) {