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

[brunolins16]

Changing from Path to ModelBinding when inferred API parameters

• You've read the Contributor Guide and Code of Conduct.
• You've included unit or integration tests for your change, where applicable.
• You've included inline docs for your change, where applicable.
• There's an open issue for the PR that you are making. If you'd like to propose a new feature or change, please open an issue to discuss the change or find an existing issue.

Description

The InferParameterBindingInfoConvention will follow the rule described here ApiBehaviorOptions.SuppressInferBindingSourcesForParameters Property

aspnetcore/src/Mvc/Mvc.Core/src/ApplicationModels/InferParameterBindingInfoConvention.cs

Line 94 in 2325e12

internal BindingSource InferBindingSourceForParameter(ParameterModel parameter)

However, it causes the issue #26234, since both will contain the parameter listed from route. With that in this PR I am adding a small check to verify if the parameter with BindingSource == Path and that does not contain a FromRoute attribute is part of the current route and if not, it will be changed back to ModelBinding.

This change will affect only the APIExplorer provider.

Fixes #26234

[brunolins16]

As mentioned in the PR description, I am changing only the DefaultApiDescriptionProvider avoiding to the InferParameterBindingInfoConvention, since it explicit set the parameter BindingSource as Path when it is part of any route (even if the route does not add it). However, that might be a good question if this part is really correct.

[pranavkm]

What's the inferred binding source for id? From what I can recall, it's supposed to be FromRoute as long as it appears in at least one route: https://github.com/dotnet/aspnetcore/blob/main/src/Mvc/Mvc.Core/src/ApplicationModels/InferParameterBindingInfoConvention.cs#L101-L104.

[brunolins16]

What's the inferred binding source for id? From what I can recall, it's supposed to be FromRoute as long as it appears in at least one route: https://github.com/dotnet/aspnetcore/blob/main/src/Mvc/Mvc.Core/src/ApplicationModels/InferParameterBindingInfoConvention.cs#L101-L104.

That is exactly what it does, it set as Path, that means FromRoute. but it causes the API explorer add the parameter from route to all route templates

[captainsafia]

@brunolins16 Can you educate me about where the InferParameterBinderConvention gets called before the changeset here gets called?

I just had a thought about whether or not it is possible to tweak that logic in the convention without disabling it outright but not sure if it has access to all the state we need to decide the binding source for the buggy scenario.

[brunolins16]

@brunolins16 Bruno Lins de Oliveira FTE Can you educate me about where the InferParameterBinderConvention gets called before the changeset here gets called?

I just had a thought about whether or not it is possible to tweak that logic in the convention without disabling it outright but not sure if it has access to all the state we need to decide the binding source for the buggy scenario.

@captainsafia Sure.

Here is what I found.

It is called from ApiBehaviorApplicationModelProvider that is a ApplicationModelProvider called from ApplicationModelFactory. The ApplicationModelFactory is called by the ControllerActionDescriptorProvider.OnProvidersExecuting.

aspnetcore/src/Mvc/Mvc.Core/src/ApplicationModels/ApiBehaviorApplicationModelProvider.cs

Line 48 in 1ca0709

if (!options.SuppressInferBindingSourcesForParameters)

aspnetcore/src/Mvc/Mvc.Core/src/ApplicationModels/ApiBehaviorApplicationModelProvider.cs

Line 80 in 1ca0709

foreach (var convention in ActionModelConventions)

aspnetcore/src/Mvc/Mvc.Core/src/ApplicationModels/ApplicationModelFactory.cs

Line 40 in 1ca0709

public ApplicationModel CreateApplicationModel(IEnumerable<TypeInfo> controllerTypes)

aspnetcore/src/Mvc/Mvc.Core/src/ApplicationModels/ControllerActionDescriptorProvider.cs

Line 85 in 1ca0709

internal IEnumerable<ControllerActionDescriptor> GetDescriptors()

The ApiDescriptionGroupCollectionProvider will use ActionDescriptorCollectionProvider.ActionDescriptors (will call the ControllerActionDescriptorProvider and others only if the collection is not initialized yet) to call the ApiDescriptionProviders to process the actions.

aspnetcore/src/Mvc/Mvc.ApiExplorer/src/ApiDescriptionGroupCollectionProvider.cs

Line 39 in 1ca0709

var actionDescriptors = _actionDescriptorCollectionProvider.ActionDescriptors;

aspnetcore/src/Mvc/Mvc.Core/src/Infrastructure/DefaultActionDescriptorCollectionProvider.cs

Line 47 in 1ca0709

public override ActionDescriptorCollection ActionDescriptors

I tried to find a way to not process the InferParameterBinderConvention without break anything, but I was not able to. Will be great if you have ideas to share.

[captainsafia]

@brunolins16 Ah, I see. I was thinking about making a change somewhere here but I don't believe that we have access to the ApiParameterContext.RouteParameters object to be able to make the decision on whether or not the parameter appeared on the route here.

aspnetcore/src/Mvc/Mvc.Core/src/ApplicationModels/InferParameterBindingInfoConvention.cs

Lines 63 to 76 in 6850cc8

	internal void InferParameterBindingSources(ActionModel action)
	{
	for (var i = 0; i < action.Parameters.Count; i++)
	{
	var parameter = action.Parameters[i];
	var bindingSource = parameter.BindingInfo?.BindingSource;
	if (bindingSource == null)
	{
	bindingSource = InferBindingSourceForParameter(parameter);
	parameter.BindingInfo = parameter.BindingInfo ?? new BindingInfo();
	parameter.BindingInfo.BindingSource = bindingSource;
	}
	}

[brunolins16]

be able to make the decision on whether or not the parameter appeared on the route here.

If I understood your idea, that is exactly what it does here (but to find if the parameter is part of any route and set all as FromRoute)

aspnetcore/src/Mvc/Mvc.Core/src/ApplicationModels/InferParameterBindingInfoConvention.cs

Line 109 in 6850cc8

private static bool ParameterExistsInAnyRoute(ActionModel action, string parameterName)

The problem, that I was not able to figure out, is because the ActionDescriptors (InferParameterBindingInfoConvention) could be populated before the ApiDescriptionProviders we don't have any way to change them other them re-populating that will affect the execution.

[brunolins16]

What's the inferred binding source for id? From what I can recall, it's supposed to be FromRoute as long as it appears in at least one route: https://github.com/dotnet/aspnetcore/blob/main/src/Mvc/Mvc.Core/src/ApplicationModels/InferParameterBindingInfoConvention.cs#L101-L104.

@pranavkm Do you happen to know what is the reason why we set all of them FromRoute if it appears in one of the routes?

My question is to understand what the expected binding behavior is for an action like this:

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

Today the following is happening, and I don't feel it is completely right.

[controller]/foo => id = 0
[controller]/foo?id=10 => id = 0
[controller]/foo/10 => Http 404
[controller]/bar/10 => id = 10 * Only scenario where the binding actual works
[controller]/bar?id=10 => Http 404

[pranavkm]

Today the following is happening, and I don't feel it is completely right.

We'd wanted the rules to be easy to reason about. It's fairly unusual to have a parameter be bound from the route for one template but not the other. That said, it would be a breaking change to tweak this behavior particularly since nobody's really complained about the runtime behavior.

[brunolins16]

Today the following is happening, and I don't feel it is completely right.

We'd wanted the rules to be easy to reason about. It's fairly unusual to have a parameter be bound from the route for one template but not the other. That said, it would be a breaking change to tweak this behavior particularly since nobody's really complained about the runtime behavior.

Got it, so, in this case my fix is wrong because what should happen in this case, if I understood correctly, is the API explorer not include the parameter as part of the API description, is that right?

[pranavkm]

I don't know if it should not be included, but in the test:

    [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)

I would expect the inferred and expected binding source to be the same for all 3. If enough people complained that BindingSource.Path is the incorrect inferred value for id, then we could consider adding a switch / quirks mode to the infer parameter binding convention, but I wouldn't change just the api description because the runtime behavior would still only support binding from a route. (?id=10 wouldn't bind but my-route/10 would)

[brunolins16]

d value for id, then we could consider adding a switch / quirks mode to the infer parameter binding convention, but I wouldn't change just the api description because the runtime behavior would still only support binding from a route. (?id=10 wouldn't bind but my-route/10 would)

I got the binding behavior, however, if I have a controller like this

    [HttpGet("api/products", Name = "Foo")]
    [HttpGet("api/products/{id}", Name = "Bar")]
    public IActionResult Get(int id = 0);

And use the swagger.json to generate the client. I will get this:

public System.Threading.Tasks.Task BarAsync(int id);
public System.Threading.Tasks.Task FooAsync(int id);

This in my opinion, is wrong because the id will never be bound in the FooAsync(int id) and that is what I mean about remove the parameter from the API Explorer.

Maybe it is an unusual scenario and, also, the API name will help the client, however, I still feel that it is wrong.

[pranavkm]

and that is what I mean about remove the parameter from the API Explorer.

Ok, that makes sense. Assuming the code doesn't end up being too gnarly, we could consider it.

[halter73]

Nit: else if?

[halter73]

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.

[pranavkm]

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)

[brunolins16]

Just to include an example: #39607 (comment)