[release/7.0-rc1] Add support for running final conventions during build

[captainsafia]

Description

This PR adds support for running conventions on an endpoint builder after "standard conventions" have been run. This allows these conventions to inspect metadata that has been registered by standard conventions and react to it. In particular, it allows groups to examine metadata of their endpoints.

Addresses #42750

Customer Impact

The changes in this PR address a bug that made it impossible for users to leverage the route groups feature, WithOpenApi, and metadata annotations on their endpoints.

var clients = app.MapGroup("/clients/{id}")
  .WithOpenApi();

clients.MapGet("/", () => ...)
  .Produces<Client>(); // Annotation doesn't appear in OpenApi definition without this change

Regression?

• Yes
• No

Risk

• High
• Medium
• Low

Low risk because:

• Changes is limited to ASP.NET Core surface area
• Change is only used in APIs introduced in .NET 7
• Change is additive and doesn't alter existing behavior

Verification

• Manual (required)
• Automated

Packaging changes reviewed?

• Yes
• No
• N/A

[davidfowl]

This change looks nice

[BrennanConroy]

Are we worried that making the default method throw will break existing scenarios? Basically, anywhere in the framework where we now call Finally, is the IEndpointConventionBuilder potentially a user defined one? And if so, was the code path accessible prior to 7.0?

[captainsafia]

Are we worried that making the default method throw will break existing scenarios?

This is a new API so I'm not too anxious about it breaking existing scenarios and the one place where it is used is WithOpenApi which was introduced in 7.0.

Basically, anywhere in the framework where we now call Finally, is the IEndpointConventionBuilder potentially a user defined one?

Yep, I've added implementations in all the IEndpointConvenntionBuilder implementations except those that are used for tests:

• ComponentEndpointConventionBuilder
• RouteHandlerBuilder
• ConnectionEndpointRouteBuilder
• RouteGroupBuilder
• HubEndpointConventionBuilder
• ControllerActionEndpointConventionBuilder
• PageActionEndpointConventionBuilder

[davidfowl]

Do a grep.app on github.

[captainsafia]

Do a grep.app on github.

YARP and gRPC also have implementations of IEndpointConventionBuilder, so we'll want to implement Finally there too.

However, at least for the time being, there's no risk of breaking existing scenarios in those APIs since they don't use it currently.

[BrennanConroy]

It's not about gPRC or YARP or anyone elses implementation using Finally, it's about our code calling Finally on them.

[BrennanConroy]

only place that might call into their Finally

Ok, that's what I was asking without knowing how to ask it 👍

[halter73]

Given that WithOpenApi already skips over non RouteEndpointBuilder endpoints and endpoints without MethodInfo metadata, skipping over unsupported endpoints is already a behavior. Should we swallow the NotImplemented exception from Finally in WithOpenApi? Also, I don't like finalConvention because it implies there can only be one of them.

Suggested change

	void Finally(Action<EndpointBuilder> finalConvention) => throw new NotImplementedException();
	void Finally(Action<EndpointBuilder> finallyConvention) => throw new NotImplementedException();

[captainsafia]

Should we swallow the NotImplemented exception from Finally in WithOpenApi?

I'm not too enthused about treating the exception thrown here as some sort of contract that WithOpenApi will check for.

endpoints without MethodInfo metadata,

It doesn't generate an operation for these types of endpoints but the Finally convention will still be registered on them so it's not totally the same.

Also, I don't like finalConvention because it implies there can only be one of them.

Bleh. This wan't intentional. I used VS Code to do a find/replace of final => finally in the src/Http and src/Mvc directories but it looks like this one might've been skipped. :/

[davidfowl]

Or maybe just call it convention because that also makes sense 😄. The property should be final conventions and the parameter should be convention. They are both adding conventions that run a different times.

[davidfowl]

Imagine this was an abstract base class, the options are:

• Add a bool SupportsFinally, return false by default, make the new virtual member throw, or have a reasonable implementation (like calling Add?)

[captainsafia]

585e4ec contains the following:

• Fixes the typo in the renames for "finally conventions"
• Updates RouteGroupBuilder, RouteEndpointDataSource, and ActionEndpointDataSource implementations to use Stack to represent "finally conventions"
• Remove reversals and reverse-iterations in Build implementations

[captainsafia]

3f1e1cd includes the following:

• Fix ordering of group and entry-specific conventions in parameter list
• Update to use FIFO ordering within finally conventions of the same level
• Add tests for WithOpenApi on group and endpoint
• Revert to using List instead of Stack for convention
• Fix ordering of outer and inner group in RouteGroups

[ghost]

Hi @captainsafia. Please make sure you've updated the PR description to use the Shiproom Template. Also, make sure this PR is not marked as a draft and is ready-to-merge.

To learn more about how to prepare a servicing PR click here.

[halter73]

(Maybe remove required from RouteGroupContext.Prefix though).

[captainsafia]

@dotnet/aspnet-admins Can I get help merging this?

Failing code check is because this PR introduces new APIs. 🙊