Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Add Description to ProducesResponseType (and others) for better OpenAPI document creation #58193

Merged
merged 25 commits into from
Oct 29, 2024
Merged

Add Description to ProducesResponseType (and others) for better OpenAPI document creation #58193

merged 25 commits into from
Oct 29, 2024

Conversation

sander1095
Copy link
Contributor

@sander1095 sander1095 commented Oct 1, 2024 •
edited
Loading

Add Description to ProducesResponseType (and other types) for better OpenAPI documents in Controllers

  • 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

This pull request introduces the concept of adding OpenAPI's description to responses returned by ASP.NET Core. The changes include adding a Description property to response metadata interfaces, classes, and attributes, as well as updating related tests.

As discussed in #55656 , Minimal API's are not (or minimally) addressed in this PR. This should instead be built in #57963 (which I would love to work on in the future!)

Fixes #55656

sander1095 added 21 commits March 17, 2024 18:32
@sander1095
Started adding Description to several attributes
1f433b6
@sander1095
Merge branch 'dotnet:main' into main
364e278
@sander1095
Add new properties to unshipped apis
d71759c
@sander1095
Some more progress of adding Description in some places
019bd17
@sander1095
Merge branch 'dotnet:main' into main
e7885ab
@sander1095
Merge branch 'dotnet:main' into main
4b75efc
@sander1095
Small improvements based on API review comments
6da811a
@sander1095
Added missing modifier to property
951676d
@sander1095
Make changes in unshipped.txt so the http project builds
3bdbe26
@sander1095
Add Description to OpenApiRouteHandlerBuilderExtensions and extra con…
442d101 
…structor and static methods to ProducesResponseTypeMetadata for correct description overloads
@sander1095
Changed code to follow overload rules and fix compile issues
f7416da
@sander1095
Fix minor typo
443babd
@sander1095
Remove code from OpenApiRouteHandlerBUilderExtensions based on Safiaś…
7700ccc 
… comment

Also removed new constructors based on Safiaś comment
@sander1095
Fix incorrect XML Comment
9614111
@sander1095
Fixed some more Public API issues
365c7d6
@sander1095
Merge branch 'dotnet:main' into main
6964a4d
@sander1095
Add unit test
a384d69
@sander1095
Add some more unit tests
c071c35
@sander1095
Remove unnecessary set from interface
aa366b7
@sander1095
Merge branch 'dotnet:main' into main
2d1314f
@sander1095
Merge branch 'dotnet:main' into main
4e9fcd8
@dotnet-issue-labeler dotnet-issue-labeler bot added the old-area-web-frameworks-do-not-use *DEPRECATED* This label is deprecated in favor of the area-mvc and area-minimal labels label Oct 1, 2024
@dotnet-policy-service dotnet-policy-service bot added the community-contribution Indicates that the PR has been added by a community member label Oct 1, 2024
@sander1095
Copy link
Contributor Author

sander1095 commented Oct 1, 2024 •
edited
Loading

Hey @captainsafia, ASP.NET Core team and community!

I'm extremely excited to be able to create this PR! I'm dedicating this to the .NET and OpenAPI community, and I shouldn't forget to mention hacktoberfest as well! I'd love to get this merged or marked as accepted in the month of October, so keep the PR feedback coming!

I've been working on this for a while since talking to Safia about it at the MVP Summit 2024, and I'm very grateful for the opportunity.

Now, let's talk about some details 😁.

The state of the PR

I believe the PR is coming along nicely, but I find it difficult to say if it is done. I would need help from the ASP.NET Core team or community to get the following answered:

  • How can I test these changes? I have added unit tests, but because I do not know how I can test this functionally in an ASP.NET Core project, I do not know what I still might need to do.
    • I couldn't find any docs about this, so perhaps it'd be useful to add some documentation about this in the future?
  • How can Swashbuckle.AspNetCore, NSwag and Microsoft.AspNetCore.OpenAPI benefit from this change? Are there code changes needed on their side, if so, how do we coordinate this? Library authors, let me know if I can help!
  • I've already mentioned this in the PR description, but I believe we also need to get feature parity on the Minimal API side (Allow OpenAPI's response descriptions to be set using Minimal API #57963). However, this still needs some discussion as implementing it could apparently lead to source incompatibility issues. I'd love assistance with knowing how we will tackle this; after that I'd love a crack at implementing it!

And finally:

Please let me know what other changes need to be made to make this work functionally! Again, my goal is to get this to an accepted (or even merged!) state in october, if possible!

@sander1095
Copy link
Contributor Author

@dotnet-policy-service agree

@mkArtakMSFT mkArtakMSFT added area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions and removed old-area-web-frameworks-do-not-use *DEPRECATED* This label is deprecated in favor of the area-mvc and area-minimal labels labels Oct 2, 2024
@sander1095
Copy link
Contributor Author

Hi @mkArtakMSFT and @adityamandaleeka ! When can I expect a review and response to my comment with questions? I am very excited to get started :)

captainsafia
captainsafia reviewed Oct 7, 2024
View reviewed changes
Copy link
Member

@captainsafia captainsafia left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great start! We'll also want to update the OpenAPI implementation to respect this new property. The code for constructing OpenAPI responses is located here. Tests for this area are here.

@sander1095
Copy link
Contributor Author

Hey @captainsafia! Thanks so much! I will look into this as soon as possible.

@sander1095
Copy link
Contributor Author

sander1095 commented Oct 9, 2024 •
edited
Loading

Hi @captainsafia ! I've implemented your suggestions, please see the new commit and let me know if there's anything I can still do!

captainsafia
captainsafia approved these changes Oct 29, 2024
View reviewed changes
Copy link
Member

@captainsafia captainsafia left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Thanks for adding tests and the M.A.OpenApi changes.

@captainsafia captainsafia merged commit d7130a2 into dotnet:main Oct 29, 2024
27 checks passed
@dotnet-policy-service dotnet-policy-service bot added this to the 10.0-preview1 milestone Oct 29, 2024
@sander1095 sander1095 mentioned this pull request Oct 31, 2024
Open
@mikekistler mikekistler mentioned this pull request Feb 11, 2025
Closed
captainsafia pushed a commit that referenced this pull request Feb 11, 2025
@sander1095 @captainsafia
Add Description to ProducesResponseType (and others) for better Open…
d85f256 
…API document creation (#58193)

* Started adding Description to several attributes

* Add new properties to unshipped apis

* Some more progress of adding Description in some places

* Small improvements based on API review comments

* Added missing modifier to property

* Make changes in unshipped.txt so the http project builds

* Add Description to OpenApiRouteHandlerBuilderExtensions and extra constructor and static methods to ProducesResponseTypeMetadata for correct description overloads

* Changed code to follow overload rules and fix compile issues

* Fix minor typo

* Remove code from OpenApiRouteHandlerBUilderExtensions based on Safiaś comment

Also removed new constructors based on Safiaś comment

* Fix incorrect XML Comment

* Fixed some more Public API issues

* Add unit test

* Add some more unit tests

* Remove unnecessary set from interface

* Remove unnecessary change in public api shipped file

* Also update unshipped file so the build succeeds!

* Apply the new Description in response models
@captainsafia captainsafia mentioned this pull request Feb 20, 2025
Closed
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@captainsafia captainsafia captainsafia approved these changes

@halter73 halter73 Awaiting requested review from halter73 halter73 is a code owner

@brunolins16 brunolins16 Awaiting requested review from brunolins16

@BrennanConroy BrennanConroy Awaiting requested review from BrennanConroy BrennanConroy is a code owner

Assignees

@captainsafia captainsafia

Labels
area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions community-contribution Indicates that the PR has been added by a community member feature-openapi
Projects
None yet
Milestone
10.0-preview1
Development

Successfully merging this pull request may close these issues.

Add Description to ProducesResponseType (and others) for better OpenAPI documents
5 participants
@sander1095 @captainsafia @adityamandaleeka @amcasey @mkArtakMSFT