Skip to content

Conversation

@captainsafia
Copy link
Member

@captainsafia captainsafia commented Sep 3, 2021

Background and Motivation

When the OpenAPI extension methods were added in RC1, they were added in the Mvc.Core assembly under the Microsoft.AspNetCore.Http namespace like so:

namespace Microsoft.AspNetCore.Http
{
    public static class OpenApiDelegateEndpointConventionBuilderExtensions
    {
        ...

        public static DelegateEndpointConventionBuilder ExcludeFromDescription(this DelegateEndpointConventionBuilder builder)

        ...
    }
}

For RC2, we want to move these extension methods to the Routing assembly since they are designed to be used with the DelegateEndpointConventionBuilder. The types will stay in the same namespace. Functionally, the user won't have to change anything in their code but this change will set us up for success moving forward with regard to making trimming of minimal apps a lot easier.

Proposed API

To support this change, some new types had to be added to support the relocation of the API.

First, the MediaTypeCollection type is being moved to the Microsoft.AspNetCore.Http assembly. The type is still in the same namespace.

This PR introduces a new IProducesTypeMetadata to the Microsoft.AspNetCore.Http.Metadata namespace

// Assembly: Microsoft.AspNetCore.Http
namespace Microsoft.AspNetCore.Http.Metadata
{
    public interface IProducesResponseTypeMetadata
    {
        Type? Type { get; }

        int StatusCode { get; }

        void SetContentTypes(MediaTypeCollection contentTypes);
    }
}

Usage Examples

The IProducesResponseTypeMetadata is designed to be used as the implementing interface for the ProducesResponseTypeAttributes like so:

internal class ProducesResponseTypeAttribute : IProducesResponseTypeMetadata

Fixes #35905

@Pilchie Pilchie 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 Sep 7, 2021
@captainsafia captainsafia added the api-ready-for-review API is ready for formal API review - https://github.com/dotnet/apireviews label Sep 7, 2021
@ghost
Copy link

ghost commented Sep 7, 2021

Thank you for your API proposal. I'm removing the api-ready-for-review label. API Proposals should be submitted for review through Issues based on this template.

@ghost ghost removed the api-ready-for-review API is ready for formal API review - https://github.com/dotnet/apireviews label Sep 7, 2021
@captainsafia captainsafia added the api-ready-for-review API is ready for formal API review - https://github.com/dotnet/apireviews label Sep 7, 2021
@ghost
Copy link

ghost commented Sep 7, 2021

Thank you for your API proposal. I'm removing the api-ready-for-review label. API Proposals should be submitted for review through Issues based on this template.

@ghost ghost removed the api-ready-for-review API is ready for formal API review - https://github.com/dotnet/apireviews label Sep 7, 2021
@captainsafia captainsafia marked this pull request as ready for review September 7, 2021 18:41
/// <summary>
/// Gets the content types supported by the metadata.
/// </summary>
IReadOnlyCollection<string>? ContentTypes { get; }
Copy link
Member

Choose a reason for hiding this comment

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

Should this be nullable or empty? Does it mean anything different in this case?

Copy link
Member Author

Choose a reason for hiding this comment

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

I think null more clearly communicates "no content types set by user" which is technically possible for void-returning endpoints where the user does something like app.MapPost("/process", () => { }).Produces(200)

Type = type ?? throw new ArgumentNullException(nameof(type));
StatusCode = statusCode;
IsResponseTypeSetByDefault = false;
_contentTypes = new List<string>();
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
_contentTypes = new List<string>();
_contentTypes = Enumerable.Empty<string>();

@captainsafia captainsafia added the api-approved API was approved in API review, it can be implemented label Sep 8, 2021
Comment on lines 650 to 655
// System.Console.WriteLine($"Waiting for debugger to attach on {System.Environment.ProcessId}");
// while (!System.Diagnostics.Debugger.IsAttached)
// {
// Thread.Sleep(1000);
// }
// System.Console.WriteLine("Debugger attached");
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove

Copy link
Member Author

Choose a reason for hiding this comment

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

Ooof -- I need a git hook to check for these. #justvscodeproblems

@captainsafia captainsafia merged commit 8e067e2 into main Sep 8, 2021
@captainsafia captainsafia deleted the safia/move-openapi branch September 8, 2021 22:58
@ghost ghost added this to the 7.0-preview1 milestone Sep 8, 2021
@captainsafia
Copy link
Member Author

/backport to release/6.0

@github-actions
Copy link
Contributor

github-actions bot commented Sep 8, 2021

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 old-area-web-frameworks-do-not-use *DEPRECATED* This label is deprecated in favor of the area-mvc and area-minimal labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Move OpenAPI extension methods out of MVC assembly

5 participants