-
Notifications
You must be signed in to change notification settings - Fork 10.2k
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
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
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.
- 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 <[email protected]>
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.
🎉
src/Http/Routing/src/Builder/RoutingEndpointConventionBuilderExtensions.cs
Outdated
Show resolved
Hide resolved
src/Mvc/Mvc.ApiExplorer/src/EndpointMetadataApiDescriptionProvider.cs
Outdated
Show resolved
Hide resolved
/// <inheritdoc /> | ||
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
foreach (var metadataAttribute in requestMetadataAttributes) | |
{ | |
metadataAttribute.SetContentTypes(contentTypes); | |
} |
@@ -607,7 +607,7 @@ public async Task ExplicitResponseTypeDecoration_SuppressesDefaultStatus_AlsoHon | |||
// Arrange | |||
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks wrong - the action says that it only returns xml:
Lines 66 to 69 in 8b30d86
[Produces("text/xml")] // Has status code as 200 but is not applied as it does not set 'Type' | |
[ProducesResponseType(typeof(Product), 201)] | |
[ProducesResponseType(typeof(SerializableError), 400)] | |
public Product CreateProductWithLimitedResponseContentTypes(Product product) |
We can't change this.
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.
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
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.
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. |
SuppressAPI
extension methods to resolve Add IgnoreApi() extension method for IEndpointConventionBuilder #34068WithName
extension method to resolve Add IgnoreApi() extension method for IEndpointConventionBuilder #34068WithGroupName
extension 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 #34541SetContentTypes
inProducesResponseTypeAttribute
to resolve UpdateProducesResponseTypeAttribute
to support setting content types for the defined response #34542