[Route Groups] Add default metadata to endpoints exposing group structure

[halter73]

Is there an existing issue for this?

• I have searched the existing issues

Is your feature request related to a problem? Please describe the problem.

Currently the MapGroup API described in #36007 does not add any default metadata enabling endpoint to observe what groups they're in if any.

Describe the solution you'd like

MapGroup should add default metadata. It's tempting to use EndpointGroupNameAttribute like WithGroupName, but this implications on the swagger.json produced by Swashbuckle and NSwag (see #36414), so we want to be careful here.

We also need to consider what the default group name should be (just the prefix?) and whether nested groups should add multiple pieced of metadata or if the nested group structure can be encapsulated in a single object.

Thanks for contacting us.

We're moving this issue to the .NET 7 Planning milestone for future evaluation / consideration. We would like to keep this around to collect more feedback, which can help us with prioritizing this work. We will re-evaluate this issue, during our next planning meeting(s).
If we later determine, that the issue has no community involvement, or it's very rare and low-impact issue, we will close it - so that the team can focus on more important and high impact issues.
To learn more about what to expect next and how this issue will be handled you can read more about our triage process here.

[NinoFloris]

This would be fairly useful to get into 7.0. It's very pleasant to be able to filter application subsystems in middleware through this kind of metadata on GetEndpoint.

Currently I'm trialing RouteGroupBuilder.WithRouteGroupName with metadata being stored in RouteGroupNameAttribute. And it would indeed be nice to have WithGroup add the prefix as default metadata.

Would something like those names - given the nice names are already in use for openapi - be accepted in api review?

[halter73]

It's hard to say what will be accepted by API review as it's typically a decision made by multiple reviewers. But if you make a proposal, we can look at it. Sadly, the bar for getting new API in .NET 7 is quite high now, so this might have to be something that waits for .NET 8.

We already have a WithGroupName method that can be applied to the RouteGroupBuilder returned from MapGroup, but it causes multiple swagger documents to be generated which is usually undesirable. Whatever we come up with, we'll have to reconcile it with this pre-existing WithGroupName method. WithRouteGroupName is a different method name, but is it different enough?

This issue was originally intended to track adding metadata to route groups automatically without the need to call any WithGroup, WithGroupName, WithRouteGroupName, etc... methods. The question is what would this metadata be called and what properties would it have? RouteGroupNameAttribute might work as the metadata, but groups don't really have names automatically. Maybe RouteGroupPrefixAttribute would work and it could have a single public RoutePattern GroupPrefix { get; } property.

[NinoFloris]

We already have a WithGroupName method that can be applied to the RouteGroupBuilder returned from MapGroup, but #36414 (comment).

I read that in your initial post yeah, which is why I mentioned the good names are already used. Not sure what we can do about that at this point.

I agree having both WithGroupName and WithRouteGroupName (the latter can at least be relegated to RouteGroupBuilder only) will probably raise some questions. Though the xml docs are already quite descriptive for WithGroupName. YMMV but it's how I found out I probably didn't want to use it and started searching the issues to see if route group metadata was already being considered.

When it comes to implementation, it probably would fit the spirit of what object.ToString does. Where it's automatically provided with the prefix being the value, and users can override this to provide a better value. As I do really think there is value in having an api for users to provide a custom name. Especially when route groups could presumably overlap when it comes to prefix (think prefix "/").

Either way thanks for your reply!

[commonsensesoftware]

Of the remaining, open issue this appears to be the most relevant. I didn't want to comment on a closed issue. If this discussion should be moved to a different issue, just let me know.

Let's start with the good. MapGroup was a welcomed addition and it certainly makes API configuration more natural. Great work. IEndpointConventionBuilder.Finally addressed a big gap, but sadly came in the 11th hour, which provided little time for review or feedback.

Unfortunately, MapGroup misses the mark on what grouping means or what developers expect from a group. The current design is opinioned around route construction, but in practice that isn't always the case.

Let's consider how a Minimal API is versioned:

var builder = WebApplication.CreateBuilder( args );

builder.Services.AddApiVersioning();

var app = builder.Build();
var orders = app.MapGroup( "/order" ).WithApiVersionSet();

// GET ~/order/{id}?api-version=1.0
orders.MapGet( "/{id}", ( string id ) => new V1.Order() { Id = id, Customer = "John Doe" } )
      .HasApiVersion( 1.0 );

// GET ~/order/{id}?api-version=2.0
orders.MapGet( "/{id:int}", ( int id ) => new V2.Order() { Id = id, Customer = "John Doe", Phone = "555-555-5555" } )
      .HasApiVersion( 2.0 );

// DELETE ~/order/{id}?api-version=2.0
orders.MapDelete( "/{id:int}", ( int id ) => Results.NoContent() )
      .HasApiVersion( 2.0 );

The grouping construct requires you to define the route templates first and apply version metadata in a very flat way. This is unnatural to the way that developers think about defining a set of versioned APIs, which is typically:

├ 1.0
│  └ GET /order/{id}
└ 2.0
   ├ GET /order/{id:int}
   └ DELETE /order/{id:int}

This is also true for the Swagger UI, where most people have a mental model of:

└ API Version (e.g. group group)
   └ API Name (e.g. logical name)
      ├ API (e.g. endpoint) 
      └ ...

Interestingly, the name issue also affects versioned APIs because there is a logical name to a set of APIs. Even though a single endpoint can be an API, there is typically a logical name for a set of related APIs. An API version set can have such a name, which may optimally be used as the metadata for the name provided in OpenAPI if no other name is provided. A version set is required because there is a logical correlation between similar endpoints. This is achieved through the ApiVersionSet monad for all endpoints registered against it; regardless of name.

I considered creating some other group-like construct in front of MapXXX, but @davidfowl strongly encouraged me to not do that (so I didn't). To me, that implies we're looking for one grouping API to rule them all. Perhaps that was intentional as a starting experiment, but the use of concrete, sealed types makes it very, very difficult to extend or customize the current design.

Ultimately, for API Versioning, there are two significant shortcomings in the current design:

1. MapGroup tracks a series of conventions for an endpoint, but there are no actual group conventions
   a. IEndpointConventionBuilder is for each specific Endpoint; group conventions are copied to each Endpoint
   b. There exists a concept to run before and after each group, which is not intrinsically supported
   c. It took a long time to figure out how to get in front of things, but you can see one example how yucky it is here
2. If a developer doesn't call WithApiVersionSet(), then all other calls to HasApiVersion and so on will apply metadata to the endpoint, but nothing is collated and ultimately will not work, but there is no way to validate or let the developer know they've missed something

At the end of the day, we do have something that works, but I think we can do better. 😉

cc: @davidfowl, @captainsafia

[captainsafia]

I had the chance to play with ApiVersioning some more recently and have some hands on experience with how clunky groups + versioning can be.

MapGroup tracks a series of conventions for an endpoint, but there are no actual group conventions

I agree with this statement in principal, but I'm not sure how much of an impact it has on the particular problem. What's the difference between setting a single metadata item once on a group vs. the same metadata item on each element within a group?

I think there is something to be said for having clearer concepts around groups in the API (group metadata, group conventions) instead of having groups be largely route pattern-based as you mentioned. It would be good to get more scenarios that would be positively impacted by this.

which is not intrinsically supported

Can you clarify what you mean here?

[commonsensesoftware]

Glad you got a chance to play with things. I just released 7.0.0-preview.1 which will have support for MapGroup as shown in the example above. It's less clunky, but it's still less than ideal. Even though putting an API Versioning specific grouping construct in front of MapGroup makes sense, I'm very hesitant to do so since our melding of minds may produce a better, more universal API surface in .NET 8. I don't want to introduce a potential breaking change in the future because I chose to go off script.

To clarify what I mean, sets of APIs are logically collated; in this case, let's call the API Orders. When a developer sets ApiVersioningOptions.ReportApiVersions = true, the expectation is that the HTTP header api-support-versions: 1.0, 2.0 would be emitted in all responses for Orders. In order to achieve that, all API versions for a logical API have to be collated. In MVC Core, this achieved by pivoting on the controller name (e.g. logical API name) via the Application Model. That doesn't exist for Minimal APIs (and rightly so) and a Minimal API doesn't necessarily have a required, logical name or any other correlation to other Minimal APIs; hence, ApiVersionSet was born. The version set serves as means to track API version metadata across endpoints.

Now, when we're finally getting ready to create endpoints, we run all the conventions as can be seen at:

aspnetcore/src/Http/Routing/src/RouteEndpointDataSource.cs

Line 216 in 5918bc9

foreach (var entrySpecificConvention in entry.Conventions)

However, this is running individual conventions for a specific Endpoint. What is really needed is:

1. Run conventions per group
2. Run conventions per endpoint
3. Run finalizer conventions per endpoint
4. Run finalizer conventions per group

This is not supported out-of-the-box. In the case of API Versioning, it needs 1. so that it can collate API versions from all endpoints in the same version set. A declared API version is what maps to a specific Endpoint for dispatch, whereas a supported API version is merely implemented - somewhere. As can be seen here (and linked above):

https://github.com/dotnet/aspnet-api-versioning/blob/5515464b1b7cbead8a26e2b6da4d2015db608372/src/AspNetCore/WebApi/src/Asp.Versioning.Http/Builder/VersionedEndpointRouteBuilder.cs#L98

API Versioning needs to decorate the EndpointDataSource so that when GetGroupedEndpoints is called this collation happens before the group is processed. Although API Versioning doesn't need post-processing, others might, which is where 4. comes in. In fact, although I don't recall any design discussions about it (but maybe I missed it), I suspect that is how Finally ended up being added. Add effectively runs first/before and Finally runs after. There are scenarios, like API Versioning, where this needs to happen at the group level itself and not just run on a single Endpoint.

[davidfowl]

@commonsensesoftware it would help if you could show a code sample of what code you want to right. Sometimes it's really hard to match the requirements here from the outcome you're trying to achieve. Can you write 3 or 4 examples of what you want to enable? (Assuming you could change anything)

[commonsensesoftware]

Sure... let's start with 2: a really basic example and one that's a little more advanced.

var builder = WebApplication.CreateBuilder( args );

builder.Services.AddApiVersioning();

var app = builder.Build();

// SCENARIO 1 -  a really simple Orders API

app.DefineApi() // ← logical group
   .MapGroup( "/orders", orders =>
    {
    	orders.HasApiVersion( 1.0 ); // ← group metadata
    	orders.MapGet( "/{id}", (string id) => Results.Ok() );
    	orders.MapPost( "/", (V1.Order order) => Results.Ok() );
    	orders.MapDelete( "/{id}", (string id) => Results.NoContent() ).IsApiVersionNeutral();
    }
   .MapGroup( "/orders", orders =>
    {
    	orders.HasApiVersion( 2.0 ); // ← group metadata
    	orders.MapGet( "/{id:int}", (int id) => Results.Ok() );
    	orders.MapPost( "/", (V2.Order order) => Results.Ok() );
    });

// SCENARIO 2 - a more advanced Weather Forecast API

app.DefineApi( "Weather Forecast" )                      // ← logical api group
   .AdvertisesApiVersion( new DateOnly( 2022, 11, 01 ) ) // ← api version implemented elsewhere
   .ReportApiVersions()                                  // ← applies to all endpoints in group
   .MapGroup( "/weatherforecast", group =>               // ← group within a group
    {
    	// all endpoints implicitly map to 0.9 and 1.0
    	group.HasApiVersion( 0.9 )
    	     .HasApiVersion( 1.0 );

        // GET /weatherforecast?api-version=0.9|1.0
        group.MapGet( "/", () => Results.Ok() );

        // GET /weatherforecast/{city}?api-version=1.0
        group.MapGet( "/{city}", (string city) => Results.Ok() )
             .MapToApiVersion( 1.0 );                    // ← explicitly maps to 1.0 only;
                                                         //   0.9 returns a client error

        // DELETE /weatherforecast/{city}
        // note: this can be declared anywhere, but there can be only one
        group.MapDelete( "/{city}", (string city) => Results.NoContent() )
             .IsApiVersionNeutral();
    } )
   .MapGroup( "/weatherforecast", group =>
    {
    	// all endpoints implicitly map to 2.0
    	group.HasApiVersion( 2.0 );

        // GET /weatherforecast?api-version=2.0
        group.MapGet( "/", () => Results.Ok() );

        // GET /weatherforecast/{city}?api-version=2.0
        group.MapGet( "/{city}", (string city) => Results.Ok() );

        // POST /weatherforecast?api-version=2.0
        group.MapPost( "/", ( WeatherForecast forecast ) => Result.Ok() );
    } );

app.Run();

Honestly, I think I could build out and make an implementation work like that today, but it would all be custom without any generalization provided by ASP.NET Core. Now that I've had some time to deeply play with what MapGroup can and can't do, I think a generic grouping interface would look something like:

public interface IGroupEndpointRouteBuilder :
    IEndpointRouteBuilder,
    IEndpointConventionBuilder
{
    IList<object> Metadata { get; }
    void BeforeConventions(IReadOnlyList<EndpointBuilder> builders);
    void AfterConventions(IReadOnlyList<EndpointBuilder> builders);
}

This is meant to spark discussion. It's not a formal API proposal.

This would address the current limitations of RouteGroupBuilder, namely:

• Pass group-level metadata through one or more group builders
• Perform group-level conventions before each endpoint convention is applied
• Perform group-level post-conventions after each endpoint convention is applied

The way I [currently] think this would be passed down through each group is by wiring up the callbacks to the RouteGroupContext like so:

public sealed class RouteGroupContext
{
	// ...
+	required public Action<IReadOnlyList<EndpointBuilder>> BeforeConventions { get; init; }
+	required public Action<IReadOnlyList<EndpointBuilder>> AfterConventions { get; init; }
}

Then in EndpointDataSource.GetGroupedEndpoints you'd be able to have something like:

public virtual IReadOnlyList<Endpoint> GetGroupedEndpoints(RouteGroupContext context)
{
    RouteEndpointBuilder[] builders = CreateBuilders( Endpoints );
    
    context.BeforeConventions(builders);

    for (int i = 0; i < builders.Count; i++)
    {
    	var builder = builders[i];

    	foreach (var convention in context.Conventions)
        {
            convention(builder);
        }

        foreach (var metadata in routeEndpoint.Metadata)
        {
            builder.Metadata.Add(metadata);
        }

        foreach (var finallyConvention in context.FinallyConventions)
        {
            finallyConvention(builder);
        }
    }

    context.AfterConventions(builders);

    return ToWrappedEndpoints(builders);
}

Abridged, pseudocode.

Using some variant of this approach, we should be able to have any type of grouping, including groups of groups (of groups, etc). The only significant side effect I see (so far) are more loops during endpoint construction, but that should only affect cold start times. IMHO, this would be negligable and acceptable. The example above, DefineApi would return a IGroupEndpointRouteBuilder that can be combined with other types of groups. Most other extension methods would be used to attach metadata or create nested groups. There may be opportunities to generalize how metadata is applied (maybe another interface?) so that a single set of extensions methods can apply metadata to a group or a specific endpoint. I could see authorization or OpenAPI metadata being applied this way too. Grouping endpoints doesn't have to use callbacks, but it does provide a nice visualization in the code that we're closing over a group.

Hopefully that starts turning some gears. I'm sure there's plenty of things I've missed or overlooked, but I think this is enough to keep the dialog going. 🙏🏽