Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Removing from API Description parameters when inferred (FromPath) and not in the route #39607

Merged
merged 9 commits into from
Jan 25, 2022
Merged

Removing from API Description parameters when inferred (FromPath) and not in the route #39607

Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
d78f803
Changing from Path to ModelBinding when inferred
brunolins16 Jan 18, 2022
f9ab96e
Removing whitespace
brunolins16 Jan 18, 2022
cc4dae2
Removing the inferred paraemter when not in the route
brunolins16 Jan 21, 2022
8cd9eeb
Avoiding extra allocation
brunolins16 Jan 24, 2022
c87e360
Remove extra line
brunolins16 Jan 24, 2022
3d1046a
Updating comment
brunolins16 Jan 24, 2022
80d36a8
Updating test to use BindingSource.Path
brunolins16 Jan 24, 2022
d7084a8
Remove extra line
brunolins16 Jan 24, 2022
31b85d7
Update src/Mvc/Mvc.ApiExplorer/src/DefaultApiDescriptionProvider.cs
brunolins16 Jan 25, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 21 additions & 3 deletions src/Mvc/Mvc.ApiExplorer/src/DefaultApiDescriptionProvider.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,12 +2,14 @@
// 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;
Expand Down Expand Up @@ -239,11 +241,13 @@ private void ProcessRouteParameters(ApiParameterContext context)
routeParameters.Add(routeParameter.Name!, CreateRouteInfo(routeParameter));
}

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)
brunolins16 marked this conversation as resolved.
Show resolved Hide resolved
{
if (routeParameters.TryGetValue(parameter.Name, out var routeInfo))
{
Expand All @@ -258,6 +262,20 @@ private void ProcessRouteParameters(ApiParameterContext context)
parameter.Source = BindingSource.Path;
}
}
else
{
if (parameter.Source == BindingSource.Path &&
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: else if?

parameter.ModelMetadata is DefaultModelMetadata defaultModelMetadata &&
!defaultModelMetadata.Attributes.Attributes.OfType<IFromRouteMetadata>().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 and
// will be removed from the API description
// https://github.com/dotnet/aspnetcore/issues/26234
context.Results.RemoveAt(i);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this situation only occur for parameters with default values? Given the scenario in the issue,

[HttpGet]
[Route("foo")]
[Route("bar/{id}")]
public IActionResult Get(int id = 0)

id could never come from the query string for the foo route, could it? Assuming not, this looks good.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It does not because we stamp in id's BindingSource as FromRoute due to this - https://github.com/dotnet/aspnetcore/blob/main/src/Mvc/Mvc.Core/src/ApplicationModels/InferParameterBindingInfoConvention.cs#L109-L126 (essentially there's one ParameterInfo for all of the route templates, so we treat the templates as a union)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just to include an example: #39607 (comment)

}
}
}
}

Expand Down
51 changes: 51 additions & 0 deletions src/Mvc/Mvc.ApiExplorer/test/DefaultApiDescriptionProviderTest.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -243,6 +243,57 @@ public void GetApiDescription_PopulatesParametersThatAppearOnRouteTemplate_AndHa
}
}

[Fact]
public void GetApiDescription_WithInferredBindingSource_ExcludesPathParametersWhenNotPresentInRoute()
{
// Arrange
var action = CreateActionDescriptor(nameof(FromModelBinding));
action.AttributeRouteInfo = new AttributeRouteInfo { Template = "api/products" };

action.Parameters[0].BindingInfo = new BindingInfo()
{
BindingSource = BindingSource.Path
};

// Act
var descriptions = GetApiDescriptions(action);

// Assert
var description = Assert.Single(descriptions);
Assert.Empty(description.ParameterDescriptions);
}

brunolins16 marked this conversation as resolved.
Show resolved Hide resolved
[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 };

action.Parameters[0].BindingInfo = new BindingInfo()
{
BindingSource = BindingSource.Path
};

// Act
var descriptions = GetApiDescriptions(action);

// Assert
var description = Assert.Single(descriptions);
var parameter = Assert.Single(description.ParameterDescriptions);
Assert.Equal(BindingSource.Path, parameter.Source);
Assert.Equal("id", parameter.Name);
}

[Fact]
public void GetApiDescription_ParameterDescription_IncludesParameterDescriptor()
{
Expand Down