Fix ApiExplorer to include FromQuery(Name) prefix in parameter names - #68344
Conversation
There was a problem hiding this comment.
Pull request overview
Fixes ApiExplorer’s parameter name generation so that when a complex action parameter has an explicit binder name via [FromQuery(Name = "...")], the generated parameter names (e.g., for Swagger/OpenAPI) include that prefix (e.g., custom.Input) to match actual model binding behavior.
Changes:
- Update
DefaultApiDescriptionProvidertraversal to incorporateBinderModelNamewhen building the container/prefix for complex parameters. - Adjust the existing ApiExplorer test to assert the prefixed query parameter name (
employee.Name) for a complex[FromQuery(Name="employee")]parameter.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| src/Mvc/Mvc.ApiExplorer/src/DefaultApiDescriptionProvider.cs | Includes BinderModelName when computing the container name for complex models so prefixed query parameter names are produced. |
| src/Mvc/Mvc.ApiExplorer/test/DefaultApiDescriptionProviderTest.cs | Updates assertion to validate the new prefixed parameter naming behavior. |
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
| var id = Assert.Single(description.ParameterDescriptions, p => p.Name == "employee.Name"); | ||
| Assert.Same(BindingSource.Query, id.Source); | ||
| Assert.Equal(typeof(string), id.Type); |
| Assert.Single(description.ParameterDescriptions); | ||
|
|
||
| var id = Assert.Single(description.ParameterDescriptions, p => p.Name == "Name"); | ||
| var id = Assert.Single(description.ParameterDescriptions, p => p.Name == "employee.Name"); |
There was a problem hiding this comment.
What happens if Employee class had a property that had [FromHeader("X-MyCustomHeader")]?
Are we prefixing this as well? And is it expected to be prefixed? Same question applies to FromRoute
In addition, let's make sure we have equivalent tests for both Mvc and minimal API, including tests directed for OpenAPI
There was a problem hiding this comment.
If no scenario here is applicable for minimal API (e.g, an analyzer warning is produced), feel free to skip the minimal API part here.
| @@ -635,7 +635,7 @@ private void Visit( | |||
|
|
|||
| // We don't want to append the **parameter** name when building a model name. | |||
There was a problem hiding this comment.
Let's update this comment as well to reflect the updated condition.
And correct me if I'm wrong, I think modelMetadata.ContainerType != null is responsible for appending a property name, so it prevents appending parameter name. But then the other check can append a parameter name but if and only if explicitly given via IModelNameProvider.
| var description = Assert.Single(descriptions); | ||
| Assert.Equal(2, description.ParameterDescriptions.Count); | ||
|
|
||
| var header = Assert.Single(description.ParameterDescriptions, p => p.Name == "employee.X-MyCustomHeader"); |
There was a problem hiding this comment.
@snemeckayova Have you double checked the actual behavior of the API at runtime? Does it accept employee.X-MyCustomHeader? or should it be X-MyCustomHeader?
There was a problem hiding this comment.
Checked it, at runtime the API accepts X-MyCustomHeader, not employee.X-MyCustomHeader, so this is basically wrong. Should I fix it under this PR?
There was a problem hiding this comment.
Yes, I think it will otherwise be a regression introduced in this PR.
| var route = Assert.Single(description.ParameterDescriptions, p => p.Name == "employee.employeeid"); | ||
| Assert.Same(BindingSource.Path, route.Source); | ||
| Assert.Equal(typeof(string), route.Type); |
There was a problem hiding this comment.
Same question here, but Path is a bit special as it relates to the placeholder in the template.
Does this parameter bind correctly for MapGet(/api/{employeeid}, ...)? Or for {employee.employee.id}? Or none of them actually binds?
There was a problem hiding this comment.
{employee.employeeid} binds; {employeeid} and {employee.employee.id} don't
There was a problem hiding this comment.
@snemeckayova Thanks for confirming. So only headers are special here?
There was a problem hiding this comment.
Yes, that's correct
When using
[FromQuery(Name = "prefix")]on complex types, ApiExplorer was incorrectly omitting the prefix from parameter names in Swagger/OpenAPI documentation. This caused a mismatch between the documented parameter names and the actual query parameter names that ASP.NET Core's model binder expects.For example, with
[FromQuery(Name = "custom")] CustomType customType, the actual query parameter iscustom.Input, but Swagger docs were showing justInput.This fix ensures ApiExplorer includes the
FromQuery(Name)prefix when generating parameter names, so Swagger documentation accurately reflects the actual binding behavior.Fixes #43464