Fix: Mark catch-all route parameters as optional (#60392) #60544
Conversation
- Added a unit test in DefaultApiDescriptionProviderTest to validate that catch-all route parameters (e.g., "{**catchAllParameter}") are reported as optional.
- Modified ProcessIsRequired in DefaultApiDescriptionProvider to detect catch-all parameters (by checking for the "{**" pattern in the route template) and mark them as not required.
- This change aligns ApiExplorer metadata with the actual runtime behavior, ensuring more accurate API documentation.
- Follows TDD practices by first writing a failing test and then implementing the fix.
dotnet-issue-labeler
bot
added
the
area-mvc
dotnet-policy-service
bot
added
the
community-contribution
|
@dotnet-policy-service agree |
| parameter.IsRequired = true; | ||
| // If the route template contains a catch-all parameter marker ("{**"), treat it as optional. | ||
| var template = context.ActionDescriptor.AttributeRouteInfo?.Template; | ||
| if (!string.IsNullOrEmpty(template) && template.Contains("{**")) |
There was a problem hiding this comment.
Does this effectively end up setting IsRequired = false for route parameters that appear in a route with a catch-all? For example, /products/{category}/items/{group}/inventory/{**any} would end up treating category and group as optional with this logic?
I wonder if there is a way we can use the ApiParameterContext.RouteParameters property to check if a route template part is a catch-all and only set it for the matching parameter source.
How does that sound?
There was a problem hiding this comment.
Thank you for the feedback!
I have updated the implementation to use the ApiParameterContext.RouteParameters property along with the IsCatchAll flag to accurately determine which parameter is a catch-all.
With this change, only the parameter explicitly marked as catch-all is treated as optional. This ensures that in a route such as: /products/{category}/items/{group}/inventory/{**any}
only the {**any} parameter is considered optional, while {category} and {group} remain required.
I appreciate your suggestion, as this update better aligns the ApiExplorer behavior with the intended routing semantics.
Please let me know if you have any further feedback.
…ers as optional
Previously, ApiExplorer marked all parameters as optional if the route template contained a catch-all marker ("{**"). This update refines the logic by leveraging ApiParameterContext.RouteParameters and the IsCatchAll flag to identify and mark only the catch-all parameter as optional. This change aligns the API metadata with the actual routing semantics and addresses dotnet#60392.
Fix: Catch-All Route Parameters Incorrectly Marked as Required in ApiExplorer
Description
This pull request addresses an issue in ASP.NET Core’s
ApiExplorerwhere catch-all route parameters (e.g.,{**catchAllParameter}) are erroneously marked as required, despite being optional in practice. At runtime, catch-all parameters match all values—including empty strings—so they should be considered optional. This discrepancy results in misleading API metadata, negatively impacting API documentation and client generation.Changes Made
DefaultApiDescriptionProviderTestto validate that catch-all route parameters are reported as optional.ProcessIsRequiredmethod inDefaultApiDescriptionProvider:{**).IsRequiredproperty is set tofalse.ApiExplorermetadata aligns with actual routing semantics.Testing
ApiExplorerfunctionality.Impact