-
Notifications
You must be signed in to change notification settings - Fork 10.5k
Respect IModelNameProvider when matching OpenAPI parameters #64535
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.
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
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This pull request adds support for respecting IModelNameProvider attributes (like FromQuery(Name = "...")) when matching OpenAPI operation parameters with .NET action parameters for XML comment documentation. This ensures that XML documentation comments are correctly applied to parameters that have custom names specified via model binding attributes.
Key Changes:
- Introduces a new
GetOperationParameterhelper method that considers both the parameter's original name and any custom names fromIModelNameProviderattributes - Adds early null check for
parameterInfobefore attempting to match operation parameters - Updates generated code across all snapshot test files to include the new logic
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| src/OpenApi/gen/XmlCommentGenerator.cs | Adds System.Reflection using statement for ParameterInfo type |
| src/OpenApi/gen/XmlCommentGenerator.Emitter.cs | Implements the core logic for parameter matching with IModelNameProvider support; adds GetOperationParameter helper method and includes Microsoft.AspNetCore.Mvc.ModelBinding namespace |
| src/OpenApi/test/.../snapshots/*.verified.cs | Updates all snapshot test files to reflect the generated code changes from the source generator |
68d5b6f to
16a616e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 12 out of 12 changed files in this pull request and generated 1 comment.
16a616e to
d6a6aaa
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 12 out of 12 changed files in this pull request and generated 3 comments.
| var requestBody = operation.RequestBody; | ||
| if (requestBody is not null) | ||
| { | ||
| requestBody.Description = parameterComment.Description; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@captainsafia Do you know why this fallback exists? We're seeing the CancellationToken argument being applied to the request body because of this, which is weird 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added a check to skip CancellationToken to avoid applying its XML doc to the request body. I can't see a scenario where you'd ever want to include a cancellation token in your OpenAPI docs 😅
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ideally, instead of checking against typeof(CancellationToken) directly, it should probably check against the binding source to see if it's BindingSource.Body instead of just blindly falling back to setting the request body description.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 12 out of 12 changed files in this pull request and generated 1 comment.
...enApi/test/Microsoft.AspNetCore.OpenApi.SourceGenerators.Tests/OperationTests.Controllers.cs
Outdated
Show resolved
Hide resolved
|
@khellang Could this be merged soon? |
Sure. If it was up to me, this would've shipped long ago, as it's a major blocker for .NET 10 adoption for us. Unfortunately I don't control the review and merge process here. Someone on the ASP.NET team needs to review and merge it. And judging by the lack of activity here, it looks like it could take a while 😅 |
|
Thank you for this contribution! I spent a little time this morning investigating and it appears that this problem also exists for Minimal APIs. I'm going to add that tag to the PR. Could you add a test in src/OpenApi/test/Microsoft.AspNetCore.OpenApi.SourceGenerators.Tests/OperationTests.MinimalApis.cs that verifies the fix also works for Minimal APIs? |
Will add it and check 👍🏻 |
78cbcd9 to
d6049c6
Compare
d6049c6 to
99560e2
Compare
|
I've reverted the change from @desjoerd's comment and added a test that verifies the fix for Minimal APIs as well. This should be ready for review again. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 14 out of 14 changed files in this pull request and generated no new comments.
mikekistler
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! 👍
@captainsafia I will defer to you for formal approval, but this all looks good to me.
Description
This makes sure any attribute implementing
IModelNameProvideris respected when matching OpenAPI operation parameters with .NET action parameters.Fixes #64534
Fixes #64521