OpenAPI convention on route group cannot examine endpoint-specific metadata

[captainsafia]

Is there an existing issue for this?

• I have searched the existing issues

Describe the bug

The OpenAPI convention registered by the WithOpenApi extension examines metadata on an endpoint to produce an OpenAPI annotation.

At the moment, group conventions have no way running after conventions on it's child endpoint and cannot examine their metadata. As a result, the following code will not produce the correct OpenAPI annotation because the Produces-metadata cannot be examined.

var trainers = app.MapGroup("/trainers")
    .WithOpenApi();

trainers.MapGet("/", (TrainingDb db) => Results.Ok(db.Trainers)).Produces<IEnumerable<Trainer>>();

Since there are challenges to supporting controlling ordering of conventions on the builder and this problem is unique to the OpenAPI convention, it might be sufficient to consider a one-off solutions for OpenAPI to resolve this.

Expected Behavior

Metadata registered on an individual endpoint should be observable by the OpenAPI convention on that endpoint's parent group.

[halter73]

Two different ideas here:

1. Make a generic RouteEndpointBuilder.Finally(Action<EndpointBuilder>) that runs during RouteEndpointBuilder.Build() after all other conventions allowing .WithOpenApi() to look at the final metadata and generate the proper OpenApiOperation for the endpoint.
2. @BrennanConroy suggested making the OpenApiOperation metadata Func<OpenApiOperation> instead allowing us to look at the final metadata lazily the first time someone tries to resolve the operation.
   • This would cache. We could use Lazy<OpenApiOperation>, but Func<OpenApiOperation> is flexible.
   • This would mean OpenAPI generation would happen the first time the metadata is retrieved and looked at instead of when the endpoint is being built (which would be the first time any request hits routing). I think this is okay.
   • @captainsafia @domaindrivendev Do you know of anything else that is already using the OpenApiOperation added by .WithOpenApi()? Would calling a Func<OpenApiOperation> instead of getting OpenApiOperation directly be okay?

[halter73]

Background and Motivation

See above.

Proposed API 1

Technically, this nothing that would show up in a PublicAPI.Unshipped.txt, but it should be considered like an API, because it's an interface we'd be expected to maintain going into the future. The proposal is to change the endpoint metadata type:

// This is a diff if the Type we put in EndpointBuilder.Metadata
- Microsoft.OpenApi.Models.OpenApiOperation
+ System.Func<Microsoft.OpenApi.Models.OpenApiOperation>

Usage Example

var operation = endpoint.Metadata.GetMetadata<Func<OpenApiOperation>>()?.Invoke();

Alternative Designs

Proposed API 2

A problem with the above two proposals is that they may be considered too lazy. We don't want to generate the OpenApiOperation via an old EndpointBuilder.Metadata reference that hopefully has all the metadata once it's accessed. Frankly, this is feels fragile. Instead of using a Func<OpenApiOperation>, we could add a Func<IEnumerable<object>, OpenApiOperation>> so the caller can pass the final endpoint metadata in.

// This is a diff if the Type we put in EndpointBuilder.Metadata
// Use IEnumerable<object> instead of EndpointMetadataCollection to better support ActionDescriptor.EndopintMetadata
- Microsoft.OpenApi.Models.OpenApiOperation
+ System.Func<IEnumerable<object>, Microsoft.OpenApi.Models.OpenApiOperation>

Proposed API 3

Instead of using (or abusing) Func<TResult> to lazily create the OpenApiOperation, we could define an interface with a property that we can add documentation to and potentially extend in the future. The problem is then that anyone reading the metadata needs to depend on both the Microsoft.OpenApi and Microsoft.AspNetCore.OpenApi packages. This would include project like Swashbuckle and potentially NSwag.

+ namespace Microsoft.AspNetCore.OpenApi;
+
+ public interface IOpenApiMetadata
+ {
+     OpenApiOperation OpenApiOperation { get; }
+ }

Proposed API 4

A problem main proposal is that it may be considered too lazy. We don't want to generate the OpenApiOperation via an old EndpointBuilder.Metadata reference. Ideally, we'd generate it immediately as soon as all the endpoint metadata is finalized via a first-class mechanism. .WithOpenApi() only works for RouteEndpoints via RouteEndointBuilders. This is a sealed type that we own, so we can add members to it without worrying about a "default" implementation.

We could add a Finally(Action) that gets called by RouteEndpointBuilder.Build() just before it creates the RouteEndpoint. It would call callbacks in LIFO order like HttpResonse.OnCompleted callbacks for similar reasons.

namespace Microsoft.AspNetCore.Routing;

public sealed class RouteEndpointBuilder : EndpointBuilder
{
+     public void Finally(Action finalizeBuild);
}

Risks

The main risk of the primary and secondary proposal is needing to support a clunky API that we'd like to change in the future but cannot easily.

The third easier to use (if you can swallow taking a Microsoft.AspNetCore.OpenApi NuGet dependency at least), but it's still just as janky as the first proposal unless it also takes metadata in as an argument.

The fourth proposal is probably the most convenient to use, but it does feel a bit heavy handed to add a new Finally(Action) method to a relatively core type like RouteEndpointBuilder considering WithOpenApi is the only use case for this API we've identified so far. That said, RouteEndpointBuilder is not exposed in many places without as-casting.

@captainsafia @BrennanConroy @davidfowl @brunolins16 @domaindrivendev @RicoSuter

[ghost]

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.

[davidfowl]

Finally might be useful for other things like building optimized caches for metadata, so maybe it's worth discussing that API and seeing what it would look like for this scenario.

[halter73]

API Review Notes:

• There seems to be some interest in option 4, "Finally".
  • Does it run before the RouteEndpointBuilder is built?
    • Yes.
    • Let's add a RouteEndopintBuilder parameter to make it clear that it's not built yet.
• Can we add the method to IEndpointConventionBuilder instead?
  • Yes, but is this too visible?
  • It's already annoying that IEndpointConventionBuilder.Add has the same visibility as extension methods. Adding, Finally to the list could be sad.
• Can we come up with a better name?
  • OnBuild?
    • Aren't all conventions called during build?
      • Not during Build(), but basically, yeah.
  • @captainsafia votes for Finally because "during build" is ambiguous.

API Approved!

namespace Microsoft.AspNetCore.Builder;
 
public interface IEndpointConventionBuilder
{
    void Add(Action<EndpointBuilder> convention);
+   void Finally(Action<EndpointBuilder> finallyConvention);
}

namespace Microsoft.AspNetCore.Routing;
 
public sealed class RouteGroupContext
{
-    public RouteGroupContext(RoutePattern prefix, IReadOnlyList<Action<EndpointBuilder>> conventions, IServiceProvider applicationServices);

-    public RoutePattern Prefix { get; }
+    public required RoutePattern Prefix { get; init; }
-    public IReadOnlyList<Action<EndpointBuilder>> Conventions { get; }
+    public IReadOnlyList<Action<EndpointBuilder>> Conventions { get; init; }
+    public IReadOnlyList<Action<EndpointBuilder>> FinallyConventions { get; init; }
-    public IServiceProvider ApplicationServices { get; }
+    public IServiceProvider ApplicationServices { get; init; }
}

[captainsafia]

What are your thoughts on renaming FinallyConventions to FinalConventions?

It feels weird to prefix this name with an adverb when an adjective makes more sense.

[halter73]

I like that Finally implies the LIFO ordering because it's like a C# try/finally where the innermost finally blocks run before the outer ones. I do agree it sounds a little tortured, but at least it's clear. They were added by a Finally method after all.

If we did go with FinalConventions, I would reverse the order of the conventions in RouteGroupBuilder rather than in each individual EndopintDataSource. Maybe that's better anyway. Less likely for a custom EndopintDataSource to mess up the subtle LIFO ordering convention we use to call these conventions 😄

Okay, I think you've convinced me. Here's the updated proposal:

namespace Microsoft.AspNetCore.Builder;
 
public interface IEndpointConventionBuilder
{
    void Add(Action<EndpointBuilder> convention);
+   void Finally(Action<EndpointBuilder> finallyConvention);
}

namespace Microsoft.AspNetCore.Routing;
 
public sealed class RouteGroupContext
{
-    public RouteGroupContext(RoutePattern prefix, IReadOnlyList<Action<EndpointBuilder>> conventions, IServiceProvider applicationServices);

-    public RoutePattern Prefix { get; }
+    public required RoutePattern Prefix { get; init; }
-    public IReadOnlyList<Action<EndpointBuilder>> Conventions { get; }
+    public IReadOnlyList<Action<EndpointBuilder>> Conventions { get; init; }
+    public IReadOnlyList<Action<EndpointBuilder>> FinalConventions { get; init; }
-    public IServiceProvider ApplicationServices { get; }
+    public IServiceProvider ApplicationServices { get; init; }
}

Does that look good to you @captainsafia? Everyone else?

[ghost]

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.

[captainsafia]

Does that look good to you @captainsafia? Everyone else?

Looks good with one adjustment for the parameter name on the Finally method.

- void Finally(Action<EndpointBuilder> finallyConvention);
+ void Finally(Action<EndpointBuilder> finalConvention);

I would reverse the order of the conventions in RouteGroupBuilder rather than in each individual EndopintDataSource.

This seems sensible to me regardless of whether it's FinallyConvention or FinalConvention.

[halter73]

One thing that I don't like about flipping the FinallyConventions to pre-reversed FinalConventions is that the custom EndpointDataSources still need to know to run the Finally callbacks passed directly to the endpoints' IEndpointConventionBuilders need to still be manually called in LIFO order. So it doesn't completely absolve data source writers from having to know which order these callbacks are called in, and data source writers might mistakenly double-reverse the FinalConventions list.

Also, I think void Finally(Action<EndpointBuilder> finalConvention) implies that there can only be one "final" convention. Seeing that name might make me think I can override the finalConvention by calling Finally again. I now think we should stick with "finally" in all the API names and avoid using "final" everywhere. I'm trying to stick with the try/finally metaphor.

@dotnet/aspnet-api-review Does anyone else have an opinion on FinallyConventions vs FinalConventions?

[halter73]

@captainsafia and I discussed this more. While she's not a fan, in the interest of time, we're going with what was already approved in #42750 (comment).

namespace Microsoft.AspNetCore.Builder;
 
public interface IEndpointConventionBuilder
{
    void Add(Action<EndpointBuilder> convention);
+   void Finally(Action<EndpointBuilder> finallyConvention);
}

namespace Microsoft.AspNetCore.Routing;
 
public sealed class RouteGroupContext
{
-    public RouteGroupContext(RoutePattern prefix, IReadOnlyList<Action<EndpointBuilder>> conventions, IServiceProvider applicationServices);

-    public RoutePattern Prefix { get; }
+    public required RoutePattern Prefix { get; init; }
-    public IReadOnlyList<Action<EndpointBuilder>> Conventions { get; }
+    public IReadOnlyList<Action<EndpointBuilder>> Conventions { get; init; }
+    public IReadOnlyList<Action<EndpointBuilder>> FinallyConventions { get; init; }
-    public IServiceProvider ApplicationServices { get; }
+    public IServiceProvider ApplicationServices { get; init; }
}