Support Deprecated in Minimal APIs for Open API

[Kahbazi]

Background and Motivation

Mark action deprecated in Minimal APIs, so it could be shown in Open API.

Proposed API

namespace Microsoft.AspNetCore.Builder
{
  public static class OpenApiEndpointConventionBuilderExtensions
  {
      public static MinimalActionEndpointRouteBuilderExtensions Deprecated(this MinimalActionEndpointRouteBuilderExtensions builder);
  }
}

Usage Examples

app.MapGet("/greetings", () => "Hello World!").Deprecated();
app.MapGet("/welcome", () => "Hello World!");

[davidfowl]

How common is this? Is this even possible today with controllers?

cc @bradygaster

[Kahbazi]

Is this even possible today with controllers?

Yes it is.
In Swashbuckle it will check if the ControllerActionDescriptor.MethodInfo or ControllerActionDescriptor.MethodInfo.DeclaringType has ObsoleteAttribute.
https://github.com/domaindrivendev/Swashbuckle.AspNetCore/blob/95cb4d370e08e54eb04cf14e7e6388ca974a686e/src/Swashbuckle.AspNetCore.SwaggerGen/SwaggerGenerator/SwaggerGenerator.cs#L145
https://github.com/domaindrivendev/Swashbuckle.AspNetCore/blob/95cb4d370e08e54eb04cf14e7e6388ca974a686e/src/Swashbuckle.AspNetCore.SwaggerGen/SwaggerGenerator/ApiDescriptionExtensions.cs#L24

And in NSwag it will check ControllerActionDescriptor.MethodInfo
https://github.com/RicoSuter/NSwag/blob/43b598d3595cc6c5a578e8f731a844d844fdd730/src/NSwag.Generation.AspNetCore/AspNetCoreOpenApiDocumentGenerator.cs#L258

But in both case they are looking for ControllerActionDescriptor but right now ActionDescriptor is being set on ApiDescription.

aspnetcore/src/Mvc/Mvc.ApiExplorer/src/EndpointMetadataApiDescriptionProvider.cs

Line 95 in c8a1cd9

ActionDescriptor = new ActionDescriptor

[martincostello]

Yeah I've used this with MVC too. There's a first-class method to configure it built into Swashbuckle (doc).

options.IgnoreObsoleteActions();

I guess given that minimal actions are brand new, adding [Obsolete] to the method directly would look a little odd at the moment, so maybe a dedicated metadata like the new one for hiding the actions from OpenAPI docs would be worth adding? I guess that would need a change in Swashbuckle and/or NSwag to support though?

[bradygaster]

@davidfowl IMO not common enough; folks tend to just delete the code and deploy to a new spot. i'm a fan of the proposed API above I thumbed-up in minimal; that said, i've seen few folks talk about obsolescence and deprecation explicitly in their APIs so i'm not sure there's customer justification for it aside from anecdotal. But maybe folks can comment here in support for the explicit support.

[commonsensesoftware]

@davidfowl / @bradygaster

Deprecation is different from being obsolete (e.g. it's gone). This has been supported in API Versioning from the very beginning. The issue is that there is no way to express this in the API Explorer today. This example shows how the information is bridged between API Versioning and an OpenAPI generator such as Swashbuckle or NSwag.

Ultimately, what is needed to fill this void is to add a new property ApiDescription, e.g.

/// <summary>
/// Gets or sets a value indicating whether an API is deprecated.
/// </summary>
public bool IsDeprecated { get; set; }

This would connect to the producers (ex: API Versioning) with the consumers (ex: Swashbuckle/NSwag) of this information. Honoring it is up to the implementers.

I don't know that .Deprecated() extension method is necessary for a Minimal API, but maybe. If it were to exist, it should be analogous to the support for ApiDescription.GroupName. IMHO, it's unnecessary. With support for attributes in Minimal APIs, the API Explorer logic can be updated to honor ObsoleteAttribute on an endpoint be it from a Minimal API, controller, or individual action. Other produces, such as API Versioning, represent it in a different way.

@davidfowl has asked for Minimal APIs with API Versioning and it's coming. I have something working. I hope to have, at least the code, available for review and/or comment by week's end. A deprecated Minimal API in API Versioning would look like:

app.DefineApi()
   .HasDeprecatedApiVersion(1.0)
   .HasMapping(api => api.MapGet("/hello-world", () => "Hello world"));

[commonsensesoftware]

It's a bit of a sidebar, but it is related to this discussion. API Versioning 6.0+ will also bring support for RFC 8594. This will allow APIs to define a sunset policy. For example:

app.Services.AddApiVersioning(
    options => options.Policies
                      .Sunset(1.0)
                      .Effective(2022, 4, 1)
                      .Link("api-policy.html")
                          .Title("Versioning Policy")
                          .Type("text/html")
                          .Language("en");

This indicates that API version 1.0 will be sunset (e.g. no longer exist) on 4/1/2022. It also provides a link to an HTML page in English that describes the policy. If no date is provided, then a link can still be provided, which outlines what the policy for an API is. This is a common scenario for a current API that doesn't have a known sunset date - yet.

The relevance to this discussion is that if we're going to add more metadata to ApiDescription, then we should consider more than just a Boolean value indicating that an API is deprecated. Deprecation is great, but it doesn't convey policy. I would propose considering that following also be added to ApiDescription:

/// <summary>
/// Gets or sets the API sunset date and time.
/// </summary>
/// <value>The date and time when the API will be permanently sunset (e.g. no longer exist).</value>
public DateTimeOffset? SunsetDate { get; set; }

API Versioning will be supporting links in accordance with RFC 8288, but that doesn't necessarily have to be fully exposed in the API Explorer. It's not a deal breaker, but I think it would be nice to have some formal subset of the most common link properties such as Url, Title, Type, and maybe Language. An Extensions property bag could be used for any other, less common properties. Perhaps something like:

public IList<ApiDescriptionLink> Links { get; } = new List<ApiDescriptionLink>();

If this type of metadata isn't supported, then developers are stuck having to create their own bridges as is the case today. The existing ApiDescripion.Properties property bag is how that is achieved today, but different libraries have no knowledge about what's in there. Adding additional metadata affords better cross-library synergy if the information is provided.

cc: @davidfowl , @bradygaster

[Kahbazi]

@captainsafia I'm interested in doing this. Do you accept PR for this issue at the moment?

[captainsafia]

@captainsafia I'm interested in doing this. Do you accept PR for this issue at the moment?

At the moment, we've departed from our approach of using metadata/attributes for annotating endpoints in MinimalApis in favor of the WithOpenApi method which allows directly modifying the OpenApiOperation that Swashbuckle ends up using.

app.MapGet()
  .WithOpenApi(o => {
    o.Deprecated = true;
  });

I'm not sure if this is helpful enough for API versioning (and that's probably a separate issue) but this is solved strictly for OpenAPI + minimal now.

[Kahbazi]

Oh I see, thanks for your explanation. So I guess this issue could be closed.

[commonsensesoftware]

@captainsafia this would be a fail for API Versioning, which doesn't directly care or depend on any part of OpenAPI nor Swashbuckle or any other OpenAPI document generator. The bridge between these concepts has always been through the API Explorer. Marking an API deprecated in OpenAPI is nice to surface, but that is one very specific use case. If you wanted to know or make decisions about a deprecated API when you are not using OpenAPI, there is no clear way to do this today. If ApiDescription.IsDeprecated existed it would be useful in a number of cases, including with OpenAPI without necessarily having to explicitly set it as you've shown.

This isn't necessarily specific to Minimal APIs, so if a new issue is required, then so be it; however, I feel this issue should be potentially reopened and re-evaluated. Adding a single property that can bridge this gap provides customer value for almost nil engineering effort. If library authors and consumers use this new property, great. If not, there is very little risk or cost.

[captainsafia]

@commonsensesoftware Yep, I think a separate issue makes sense for the API versioning case. This one was specifically around minimal + OpenAPI which is a lot more scoped than the problem identified here.

It might help to include other configurable metadata that API versioning would need in that issue. I know there have been separate threads about route groups + versioning as well.