Fix: Mark catch-all route parameters as optional (#60392) #60544
+28
−2
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
- 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
|
Thanks for your PR, @mapedersen. Someone from the team will get assigned to your PR shortly and we'll get it reviewed. |
dotnet-policy-service
bot
added
the
community-contribution
1 task
|
@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?
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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