Open halter73 opened 2 years ago
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.
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?
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.
We already have a WithGroupName method that can be applied to the RouteGroupBuilder returned from MapGroup, but https://github.com/dotnet/aspnetcore/issues/36414#issuecomment-917739692.
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!
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:
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 hereWithApiVersionSet()
, 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 somethingAt 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.
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?
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:
However, this is running individual conventions for a specific Endpoint
. What is really needed is:
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):
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
.
@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 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:
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. 🙏🏽
OK, so the first example. Why does DefineApi
need to exist? Here at all? Is it adding metadata somewhere? Using .NET 7's route groups this would look like:
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).
var 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 MapGroup("")
or MapGroup()
works, then that would be acceptable. DefineApi
doesn't need to exist. You asked how I would want it to look if I could change anything. 😉 Using MapGroup
can most definitely work, but it's a bit unnatural because you aren't mapping any routes - yet. Maybe that's the intent. I've seen no such examples or documentation.
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 DefineApi
would add metadata to a group builder. It would be a group that doesn't itself represent any endpoints. It is purely a builder that collates metadata for endpoints further down the chain.
If MapGroup
can achieve that, then I'll have to consider whether I leave it as is or add something like DefineApi
, which would merely be shorthand for MapGroup( "" ).WithApiVersionSet()
.
The current grouping API design does not intrinsically support group-level conventions so you've missed that. MapGroup
collates on the routes, but API Versioning collates on logical API sets. In MVC Core, this was achieved by controller name. The ApiVersionSet
serves as a similar monad for Minimal APIs which collates API versions together; regardless of name (but a developer can provide one). This makes ApiVersioningOptions.ReportApiVersions = true
continue to work for Minimal APIs. The example above /weatherforecast?api-version=1.0
and /weatherforecast?api-version=2.0
would both return the header api-supported-versions: 1.0, 2.0
.
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 EndpointDataSource
so that I can collate all API versioning metadata for related APIs before the endpoints are generated and run their own conventions. Am I blocked? No. Is the grouping API painful to extend - kinda. Since the RouteGroupBuilder
is sealed
and implements two disjoint interfaces, I had to create the interface:
public interface IVersionedEndpointRouteBuilder :
IEndpointRouteBuilder,
IEndpointConventionBuilder
{
}
So that I can decorate RouteGroupBuilder
, retain its functionality, and add the necessary collation logic. The problem is the inverse of what you stated: two different groups with the same metadata. The shared metadata is the what is collated across all endpoints. If that can thread through a group a la MapGroup( "" )
, then that might just work.
Currently, I don't have support for adding metadata via RouteGroupBuilder
, but I think I could. It's an implementation detail, but HasApiVersion
implicitly defined on a group has a different meaning that HasApiVersion
explicitly on an Endpoint
. That might seem strange, but most people want to define versioned APIs in logical sets. It is possible to have two different endpoints in a set with the same route template map to different versions a la MapToApiVersion
. In a controller this would be different action methods. You can, however, explicitly define API versions on any specific endpoint. When you do that, any implicit API versions (from the group) are ignored. To make it work, I'm pretty sure I'd have to track if the metadata was defined by a group or explicitly on an endpoint. Achievable, but a lot of internal ceremony (if only it was my day job 😆).
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. 😉
OK, so the first example. Why does DefineApi need to exist? Here at all? Is it adding metadata somewhere? Using .NET 7's route groups this would look like:
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();
or add something like DefineApi, which would merely be shorthand for MapGroup( "" ).WithApiVersionSet().
I like having DefineApi
as a shorthand for MapGroup
+ WithApiVersionSet
and I believe we currently have all the moving parts to make this happen.
The notable exception being that there isn't a way to represent the ApiVersionSetBuilder
as group metadata.
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'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 IServiceProvider
on the EndpointBuilder
and making it resolvable that way.
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 todo
and will be correctly collated. Extension methods as shortcuts are still an option and supported, but in most situations, the configuration will be different. At a minimum, I would expect the model to be different between versions, but it might solvable via generics. In any case, it's in the hands of the developer.
I'll report back my findings and any additional pain points.
The way I was able to get the metadata to flow through the groups was by decorating IServiceProvider on the EndpointBuilder and making it resolvable that way.
😔. can you show that code?
@davidfowl
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 RouteGroupBuilder
so I could get in front of it to do group-level convention processing. You can see the main problem area here.
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:
MapApiGroup
on a nested group (it's illogical and likely will not flow correctly)WithApiVersionSet
after MapApiGroup
or vice versaNewApiVersionSet
and pass it to WithApiVersionSet
for an endpoint (in the .NET 6 style) after you've used one of the supported group constructsFood for thought. I don't have any good ideas here. I tried through Finally
, but that doesn't work in my scenario. Conventions can be added to a group or specific endpoint, but they have different validation semantics. The goal is to help developers detect and surface misconfigurations as early as possible.
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 https://github.com/dotnet/aspnetcore/issues/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.