-
Notifications
You must be signed in to change notification settings - Fork 10.3k
[Route Groups] Add default metadata to endpoints exposing group structure #41429
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Comments
Thanks for contacting us. We're moving this issue to the |
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 Would something like those names - given the nice names are already in use for openapi - be accepted in api review? |
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 This issue was originally intended to track adding metadata to route groups automatically without the need to call any |
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 When it comes to implementation, it probably would fit the spirit of what Either way thanks for your reply! |
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. Unfortunately, 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:
This is also true for the Swagger UI, where most people have a mental model of:
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 I considered creating some other group-like construct in front of Ultimately, for API Versioning, there are two significant shortcomings in the current design:
At the end of the day, we do have something that works, but I think we can do better. 😉 cc: @davidfowl, @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.
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.
Can you clarify what you mean here? |
Glad you got a chance to play with things. I just released To clarify what I mean, sets of APIs are logically collated; in this case, let's call the API Orders. When a developer sets Now, when we're finally getting ready to create endpoints, we run all the conventions as can be seen at:
However, this is running individual conventions for a specific
This is not supported out-of-the-box. In the case of API Versioning, it needs API Versioning needs to decorate the |
@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) |
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 public interface IGroupEndpointRouteBuilder :
IEndpointRouteBuilder,
IEndpointConventionBuilder
{
IList<object> Metadata { get; }
void BeforeConventions(IReadOnlyList<EndpointBuilder> builders);
void AfterConventions(IReadOnlyList<EndpointBuilder> builders);
}
This would address the current limitations of
The way I [currently] think this would be passed down through each group is by wiring up the callbacks to the public sealed class RouteGroupContext
{
// ...
+ required public Action<IReadOnlyList<EndpointBuilder>> BeforeConventions { get; init; }
+ required public Action<IReadOnlyList<EndpointBuilder>> AfterConventions { get; init; }
} Then in 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);
}
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, 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. 🙏🏽 |
Example 1OK, so the first example. Why does var v0 = app.MapGroup("orders");
v0.HasApiVersion(1.0); // ← group metadata
v0.MapGet("/{id}", (string id) => Results.Ok());
v0.MapPost("/", (V1.Order order) => Results.Ok());
v0.MapDelete("/{id}", (string id) => Results.NoContent()).IsApiVersionNeutral();
var v1 = app.MapGroup("orders");
v1.HasApiVersion(1.0); // ← group metadata
v1.MapGet("/{id}", (string id) => Results.Ok());
v1.MapPost("/", (V1.Order order) => Results.Ok());
v1.MapDelete("/{id}", (string id) => Results.NoContent()).IsApiVersionNeutral(); What is missing there? Besides the nesting API, which we punted for now (but I understand is nicer to look at from a logical nesting PoV). Example 2var g = app.MapGroup(""); // Hacky, no prefix I now but it works 😄
g.DefineApi("Weather Forecast")
.AdvertisesApiVersion(new DateOnly(2022, 11, 01)) // ← api version implemented elsewhere
.ReportApiVersions(); // ← applies to all endpoints in group
var g1 = g.MapGroup("weatherforecast");
g1.HasApiVersion(0.9).HasApiVersion(1.0);
// GET /weatherforecast?api-version=0.9|1.0
g1.MapGet("/", () => Results.Ok());
// GET /weatherforecast/{city}?api-version=1.0
g1.MapGet("/{city}", (string city) => Results.Ok())
.MapToApiVersion(1.0); // ← explicitly maps to 1.0 only;
// 0.9 returns a client error
var g2 = g.MapGroup("weatherforecast");
// all endpoints implicitly map to 2.0
g2.HasApiVersion(2.0);
// GET /weatherforecast?api-version=2.0
g2.MapGet("/", () => Results.Ok());
// GET /weatherforecast/{city}?api-version=2.0
g2.MapGet("/{city}", (string city) => Results.Ok());
// POST /weatherforecast?api-version=2.0
g2.MapPost("/", (WeatherForecast forecast ) => Result.Ok()); Is it about the features or is it about the API style? As far as we're concerned, groups are already general purpose and can be nested. They are also logical, so you get a new group when MapGroup is called. That means you can declare 2 groups with the same prefix and different metadata already. Does this solve your problems? If not, what am I missing? PS: There are some tweaks we want to make to groups for .NET 8. Some that come to mind are:
|
I suppose if The only thing that you missed is the setup needs to be: app.MapGroup( "/orders" ).WithApiVersionSet(); Which was from my earlier comment. That is what I currently have working in preview. So - yes - some future If The current grouping API design does not intrinsically support group-level conventions so you've missed that. I linked to the source implementation I have a couple of times above (as it's nontrivial to copy over), but long story short, I have to decorate the public interface IVersionedEndpointRouteBuilder :
IEndpointRouteBuilder,
IEndpointConventionBuilder
{
} So that I can decorate Currently, I don't have support for adding metadata via To be crystal clear, I do have something working. I will take all the feedback and challenge to design you are willing to put forth (here or back in the API Versioning repo). I do, however, think there are some opportunities to make defining these groups less clunky as we look into the future. 😉 |
This sample is what I was anticipating the experience would look like. With the noticeable tweak that versions would be registered something like this: var v0 = app.DefineApi("v0");
v0.MapTodos();
var v1 = app.DefineApi("v1");
v1.MapTodos();
v1.MapUsers();
I like having The notable exception being that there isn't a way to represent the
I'll try the changes added in the last week and share any feedback. |
Thanks for all the dialog. The way I was able to get the metadata to flow through the groups was by decorating It may take me a couple of days, but I have a few additional ideas about how this might work. For example: // MapApiGroup might make sense too
// regardless, it's a shortcut over MapGroup + Custom metadata
var todo = app.DefineApi( "ToDo" );
// 1.0
var v1 = todo.MapGroup( "todo" );
v1.HasApiVersion( 1.0 ); // ← attached to this group and applies to all mapped endpoints
v1.MapGet( "/", () => Results.Ok() );
// 2.0
var v2 = todo.MapGroup( "todo" );
v2.HasApiVersion( 2.0 ); // ← attached to this group and applies to all mapped endpoints
v2.MapGet( "/", () => Results.Ok() );
v2.MapGet( "/{id}", (string id) => Results.Ok() );
v2.MapPost( "/", (ToDo todo) => Results.Ok() ); All subgroups roll back up to I'll report back my findings and any additional pain points. |
😔. can you show that code? |
context = new RouteGroupContext()
{
// decorate IServiceProvider with group-level metadata so it's accessible in the Finally convention
ApplicationServices = new ServiceProviderDecorator( context.ApplicationServices, versionSetBuilder ),
Conventions = context.Conventions,
FinallyConventions = context.FinallyConventions,
Prefix = context.Prefix,
}; |
It went smoother and faster than I thought. Groups of groups are working correctly with only minor refactoring. Unless there are other recommendations, this is were API Versioning will land for its .NET 7 Preview 2: // stick with the MapXXX convention and indicate this is a group. this is just a shortcut for:
// app.MapGroup( "" ).WithApiVersionSet( "ToDo" );
var todo = app.MapApiGroup( "ToDo" );
// 1.0
var v1 = todo.MapGroup( "todo" ).HasApiVersion( 1.0 );
v1.MapGet( "/", () => Results.Ok() );
// 2.0
var v2 = todo.MapGroup( "todo" ).HasApiVersion( 2.0 );
v2.MapGet( "/", () => Results.Ok() );
v2.MapGet( "/{id}", (string id) => Results.Ok() );
v2.MapPost( "/", (ToDo todo) => Results.Ok() ); To see a full example with OpenAPI support, you can peek at this example. In terms of grouping constructs, the design holds up. We could split hairs on style, but that's irrelevant. The two remaining challenges as it related to this issue are:
I have it working, but not without a lot of trial and error, which ultimately required decorating Another area to consider is validation. There just may not be a general form for it and it will be up to extenders. For API Versioning there are a number of validation scenarios such as:
Food for thought. I don't have any good ideas here. I tried through |
Uh oh!
There was an error while loading. Please reload this page.
Is there an existing issue for this?
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 useEndpointGroupNameAttribute
likeWithGroupName
, 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.
The text was updated successfully, but these errors were encountered: