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.

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

Merged
merged 20 commits into from
Aug 6, 2021

Conversation

captainsafia
Copy link
Member

@captainsafia captainsafia added api-ready-for-review API is ready for formal API review - https://github.com/dotnet/apireviews area-web-frameworks *DEPRECATED* This label is deprecated in favor of the area-mvc and area-minimal labels labels Jul 31, 2021
@ghost
Copy link

ghost commented Jul 31, 2021

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:

  • The PR contains changes to the reference-assembly that describe the API change. Or, you have included a snippet of reference-assembly-style code that illustrates the API change.
  • The PR describes the impact to users, both positive (useful new APIs) and negative (breaking changes).
  • Someone is assigned to "champion" this change in the meeting, and they understand the impact and design of the change.

Copy link
Contributor

@pranavkm pranavkm left a 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/Http/Routing/src/ISuppressApiMetadata.cs Outdated Show resolved Hide resolved
src/Http/Routing/src/EndpointGroupNameMetadata.cs Outdated Show resolved Hide resolved
src/Http/Routing/src/EndpointGroupNameAttribute.cs Outdated Show resolved Hide resolved
src/Mvc/Mvc.Core/src/ProducesResponseTypeAttribute.cs Outdated Show resolved Hide resolved
src/Mvc/Mvc.Core/src/ProducesResponseTypeAttribute.cs Outdated Show resolved Hide resolved
src/Mvc/Mvc.Core/src/ProducesResponseTypeAttribute.cs Outdated Show resolved Hide resolved
@DamianEdwards
Copy link
Member

I have a mild concern regarding the name ExcludeFromApiExplorer (yes even though I suggested it 😄) as it ties this so tightly to ApiExplorer as the mechanism by which metadata about APIs is consumed. We had discussed at one point the idea that eventually systems like Swashbuckle should look directly at endpoint metadata to build up the details required for their functioning rather than relying exclusively on ApiExplorer as it's an MVC concept and minimal APIs are not MVC. That said, the only alternative naming I can think of that would cater to that concern would be something like ExcludeFromApiDescriptions or ExcludeFromApiDocumentation.

Like I said, it's a mild concern but I figured I'd raise it now.

Copy link
Member

@martincostello martincostello left a comment

Choose a reason for hiding this comment

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

🎉

/// <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();
Copy link
Contributor

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.

Copy link
Member Author

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.

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" };
Copy link
Contributor

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:

[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.

Copy link
Contributor

@pranavkm pranavkm left a 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.

@pranavkm pranavkm added api-approved API was approved in API review, it can be implemented and removed api-ready-for-review API is ready for formal API review - https://github.com/dotnet/apireviews labels Aug 4, 2021
Copy link
Contributor

@pranavkm pranavkm left a 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.

@captainsafia captainsafia enabled auto-merge (squash) August 6, 2021 18:22
@captainsafia captainsafia merged commit c2f1caa into main Aug 6, 2021
@captainsafia captainsafia deleted the safia/minimal-open-api branch August 6, 2021 19:12
@ghost ghost added this to the 6.0-rc1 milestone Aug 6, 2021
@DamianEdwards
Copy link
Member

🎉

@ghost
Copy link

ghost commented Aug 6, 2021

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.

@bradygaster
Copy link
Member

also 🎉

@ghost
Copy link

ghost commented Aug 6, 2021

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api-approved API was approved in API review, it can be implemented area-web-frameworks *DEPRECATED* This label is deprecated in favor of the area-mvc and area-minimal labels
Projects
None yet
7 participants