Improve support for OpenAPI in minimal actions#34906
Conversation
|
Thank you for submitting this for API review. This will be reviewed by @dotnet/aspnet-api-review at the next meeting of the ASP.NET Core API Review group. Please ensure you take a look at the API review process documentation and ensure that:
|
src/Mvc/Mvc.Core/src/Builder/OpenApiEndpointConventionBuilderExtensions.cs
Outdated
Show resolved
Hide resolved
src/Mvc/Mvc.Core/src/Builder/OpenApiEndpointConventionBuilderExtensions.cs
Outdated
Show resolved
Hide resolved
src/Mvc/Mvc.Core/src/Builder/OpenApiEndpointConventionBuilderExtensions.cs
Outdated
Show resolved
Hide resolved
pranavkm
left a comment
There was a problem hiding this comment.
- SuppressApi -> ExclueFromApiExplorer
- Do not type forward types from MVC.
- We would want to update ApiExplorer features to implement the new interfaces
- We'll keep an attribute per feature e.g. WithGroupName, WithName,
ExcludeFromApiExplorer
src/Mvc/Mvc.Core/src/Builder/OpenApiEndpointConventionBuilderExtensions.cs
Outdated
Show resolved
Hide resolved
|
I have a mild concern regarding the name Like I said, it's a mild concern but I figured I'd raise it now. |
src/Mvc/Mvc.Core/src/Builder/OpenApiEndpointConventionBuilderExtensions.cs
Outdated
Show resolved
Hide resolved
src/Mvc/Mvc.Core/src/Builder/OpenApiEndpointConventionBuilderExtensions.cs
Outdated
Show resolved
Hide resolved
Co-authored-by: Martin Costello <martin@martincostello.com>
src/Http/Routing/src/Builder/RoutingEndpointConventionBuilderExtensions.cs
Outdated
Show resolved
Hide resolved
src/Mvc/Mvc.ApiExplorer/src/EndpointMetadataApiDescriptionProvider.cs
Outdated
Show resolved
Hide resolved
| void IApiResponseMetadataProvider.SetContentTypes(MediaTypeCollection contentTypes) | ||
| { | ||
| // Users are supposed to use the 'Produces' attribute to set the content types that an action can support. | ||
| contentTypes.Clear(); |
There was a problem hiding this comment.
Let me do a quick sanity test on this. I don't super remember how this gets used, but I'd like to make sure we have additional MVC integration tests for this now that we're enabling this.
There was a problem hiding this comment.
Sure. From what I could tell, this gets used like an out var but it is tricky to reason about.
aspnetcore/src/Mvc/Mvc.ApiExplorer/src/DefaultApiDescriptionProvider.cs
Lines 439 to 442 in 709038b
| var type1 = typeof(ApiExplorerWebSite.Product).FullName; | ||
| var type2 = typeof(SerializableError).FullName; | ||
| var expectedMediaTypes = new[] { "text/xml" }; | ||
| var expectedMediaTypes = new[] { "application/xml", "text/xml", "application/json", "text/json" }; |
There was a problem hiding this comment.
This looks wrong - the action says that it only returns xml:
We can't change this.
pranavkm
left a comment
There was a problem hiding this comment.
Marking this as blocked until we can resolve the test change.
src/Mvc/Mvc.ApiExplorer/src/EndpointMetadataApiDescriptionProvider.cs
Outdated
Show resolved
Hide resolved
src/Mvc/Mvc.Core/src/Builder/OpenApiEndpointConventionBuilderExtensions.cs
Outdated
Show resolved
Hide resolved
src/Mvc/Mvc.Core/src/Builder/OpenApiEndpointConventionBuilderExtensions.cs
Outdated
Show resolved
Hide resolved
pranavkm
left a comment
There was a problem hiding this comment.
Please add the test so we have the behavior documented. Outside of that, this looks good.
|
🎉 |
|
Hi @DamianEdwards. It looks like you just commented on a closed PR. The team will most probably miss it. If you'd like to bring something important up to their attention, consider filing a new issue and add enough details to build context. |
|
also 🎉 |
|
Hi @bradygaster. It looks like you just commented on a closed PR. The team will most probably miss it. If you'd like to bring something important up to their attention, consider filing a new issue and add enough details to build context. |
SuppressAPIextension methods to resolve Add IgnoreApi() extension method for IEndpointConventionBuilder #34068WithNameextension method to resolve Add IgnoreApi() extension method for IEndpointConventionBuilder #34068WithGroupNameextension method and metadata classes to resolve Allow setting a group name for endpoints and have it be used to populate ApiDescription.GroupName in ApiExplorer for minimal APIs #34541SetContentTypesinProducesResponseTypeAttributeto resolve UpdateProducesResponseTypeAttributeto support setting content types for the defined response #34542