Skip to content
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

[Route Groups] Support AddFilter, WithOpenApi and other additive conventions #41427

Closed
Tracked by #41433
halter73 opened this issue Apr 28, 2022 · 10 comments · Fixed by #42195 or #42678
Closed
Tracked by #41433

[Route Groups] Support AddFilter, WithOpenApi and other additive conventions #41427

halter73 opened this issue Apr 28, 2022 · 10 comments · Fixed by #42195 or #42678
Assignees
Labels
api-approved API was approved in API review, it can be implemented area-minimal Includes minimal APIs, endpoint filters, parameter binding, request delegate generator etc area-web-frameworks *DEPRECATED* This label is deprecated in favor of the area-mvc and area-minimal labels blog-candidate Consider mentioning this in the release blog post Docs This issue tracks updating documentation feature-minimal-actions Controller-like actions for endpoint routing Priority:0 Work that we can't release without
Milestone

Comments

@halter73
Copy link
Member

halter73 commented Apr 28, 2022

Many metadata-adding extension methods like RequireCors(), RequireAuthorization(), WithGroupName(), etc... can already be applied to an entire group using the GroupRouteBuilder added in #36007.

However, extension methods like AddFilter for RouteHandlerBuilder don't work because GroupRouteBuilder is not a RouteHandlerBuilder. This makes it impossible to apply a route handler filter to a group of endpoints today.

Describe the solution you'd like

Originally there was a proposal to add an public void OfBuilder<T>(Action<T> configureBuilder) where T : IEndpointConventionBuilder; method to GroupRouteBuilder that would support types like RouteHandlerBuilder, but that wasn't approved for the initial design. This is partially due to the ugliness of using an Action<T> to use the RouteHandlerBuilder and the ugliness of the IGroupEndpointDataSource interfaces required to support the API.

Now I'm thinking it could be some abstract static interface method on the type implementing IEndpointConventionBuilder (e.g. RouteHandlerBuilder) could construct itself using an arbitrary IEndpointConventionBuilder and rely on EndpiontBuilder.Metadata to keep state.

interface IEndpointConventionBuilderFactory<TSelf>
  where T : class, IEndpointConventionBuilder, IEndpointConventionBuilderFactory<TSelf>
{
   abstract static T CreateBuilder(IEndpointConventionBuilder builder);
}
// ...
public sealed class RouteHandlerBuilder : IEndpointConventionBuilder, IEndpointConventionBuilderFactory<RouteHandlerBuilder>
{
    // ...
    // This would be called by GroupRouteBuilder.OfBuilder<T>() or whatever
    // with the GroupRouteBuilder as an argument. This should allow returning T
    // instead of us relying on on the caller to provide an Action<T> callback.
    static RouteHandlerBuilder IEndpointConventionBuilderFactory<RouteHandlerBuilder>.CreateBuilder(IEndpointConventionBuilder builder)
    {
        return new RouteHandlerBuilder(builder);
    }
}

The trick is going to be using EndpointBuilder.Metadata to store the RouteHanderFilterFactories and to read them so it can be rehydrated rather than assuming that the RouteHandlerBuilder returned from the initial call to MapGet/MapPost/etc... is the only RouteHandlerBuilder adding filters to a given endpoint.

Figuring out exactly how we can read the metadata late enough is going to be tough. We need to create a RequestDelegate before adding group metadata given the current design. We might want to build the final handler the first time Endpoint.RequestDelegate is called so we can read the final endpoint metadata off of the HttpContext, then create a new RequestDelegate with all the filters and call that from then on out for the new endpoint. This is probably better than locking routing globally on RequestDelegateFactory.Create like we do today in .NET 7 previews.

@halter73 halter73 added the area-web-frameworks *DEPRECATED* This label is deprecated in favor of the area-mvc and area-minimal labels label Apr 28, 2022
@rafikiassumani-msft rafikiassumani-msft added this to the 7.0-preview5 milestone Apr 28, 2022
@halter73 halter73 mentioned this issue Apr 28, 2022
4 tasks
@halter73
Copy link
Member Author

halter73 commented May 18, 2022

Design meeting notes

  • Intro: We want AddFilter, WithOpenApi (and also things like WithTags, WithSummary, WithDescription, etc... but possibly in a different way) to work with groups. ASP.NET Core endpoint routing type & extension hierarchy.
  • AddFilter and WithOpenApi are somewhat special because we only want these to work with minimal endpoints and groups and not things like MapControllers. Things like WithTags, WithSummary and WithDescription can possibly be retargeted to IEndpontConventionBuilder as suggested in [Route Groups] WithTags et al. should target IEndpointConventionBuilder #41428
  • Ideally, we only want APIs and AddFilter these to work with minimal endpoints and groups, but maybe this is too hard in practice. Perhaps retargeting to IEndpontConventionBuilder instead of RouteHandlerBuiler is okay. Should GroupRouteBuilder implement an interface if extension methods are going target? (IGroupRouteBuilder?).
  • Routing has terrible names. IEndointRouteBuilder, IEndpointConventionBuilder, RouteHandlerBuiler. Can we fix this?
  • Can AddFilter be renamed to AddEndpointRouteFilter and extend IEndpointConventionBuilder directly? Retarget without the rename?

Goals

  • We should target AddFilter, WithOpenApi and everything else to TBuilder where TBuilder : IEndpointConventionBuilder.
    • Open question: If we do, do we need to rename AddFilter to AddRouteHandlerFilter, AddEndpointFilter or similar?
  • It'd be nice to avoid building all endpoints on the first request, but not strictly required as part of this change.
  • It would be nice to make it possible for filters to modify endpoint metadata (Allow endpoint filter factories to manipulate endpoint metadata #41722) even when applied to a group. With the current hacky prototype[1] the RequestDelegateFactory gets called too late after the final Endpoint has been created and therefore cannot participate in adding metadata.

1: Here's a hacky prototype. It currently rebuilds the RequestDelegate every time, but could be updated

@halter73 halter73 changed the title [Route Groups] Support custom IEndpointConventionBuilder types [Route Groups] Support AddFilter, WithOpenApi and other additive conventions May 27, 2022
@halter73
Copy link
Member Author

halter73 commented May 27, 2022

The trickiness with MapGroup is that it defines and IEndpointRouteBuilder that registers itself as a EndpointDataSource of a parent IEndpointRouteBuilder.

Unfortunately, this means that the metadata from the "inner" EndpointDataSource.Endpoints has already been added by endpoint-specific Action<EndpointBuilder> conventions before the groupwide Action<EndpointBuilder> conventions time-wise. Metadata added by the groupwide conventions still get added first to the final Endpoint.Metadata collection returned to the root IEndpointRouteBuilder, but endpoint-specific conventions never "see" the group metadata because of how early they are run.

That's why I suggest adding a new API. For example, EndpointDataSource that are group aware that allow MapGroup to pass in the group conventions to the "inner" EndpointDataSource. Maybe via something like:

public abstract class EndpointDataSource
{
+    IReadOnlyList<Endpoint> GetEndpointsForGroup(RoutePattern prefix, IEnumerable<Action<EndpointBuilder>> conventions);
}

It might be better as an interface so we could easily check which EndpointDataSource implemented the API.

I don't see another great way to let existing conventions "see" metadata from outer groups without updating all the conventions themselves. In practice, I suspect most of the EndpointDataSource implementations are our own, and we can always fallback to the existing MapGroup behavior of reordering metadata for EndpointDataSource implementations that don't implement the new API.

#41779 (comment)

I think the above API is a better solution than the hacky prototype I linked to in the last comment even if we cached the RequestDelegate after the first request. To me, this seems way cleaner than calling RequestDelegateFactory.Create() the first time the RequestDelegate is run. Lazily calling RequestDelegateFactory.Create() in the Endpoint.RequestDelegate getter is still probably a good idea so all the RequestDelegateFactory.Create() call for a single application don't happen under a single lock.

@halter73
Copy link
Member Author

halter73 commented Jun 15, 2022

Background and Motivation

Many metadata-adding extension methods like RequireCors(), RequireAuthorization(), WithGroupName(), WithTags(), etc... can already be applied to an entire group using MapGroup.

However, AddFilter and WithOpenApi target RouteHandlerBuilder which don't work because (amoung other things) RouteGroupBuilder does not derive from RouteHandlerBuilder. This makes it impossible to apply a route handler filter to a group of endpoints today.

It should be possible to apply both route handler filters an OpenAPI metadata using groups. And anything done at an outer group level should have a lower precedence than anything specified directly on the endpoint. This means ideally the filters and Func<OpenApiOperation, OpenApiOperation> configureOperation callbacks applied to outer groups should be run before those added applied inner groups and all of that should run before anything applied directly to the endpoint.

To support this, I added a new virtual EndpointDataSource.GetGroupedEndpoints(RouteGroupContext) method. Then I created an internal RouteEndointDataSource that takes over a lot of the complicated logic that used to exist in EndpointRouteBuilderExtensions.

The upside of moving this logic into RouteEndointDataSource other than general cleanliness, is that as a custom EndpointDataSource, it can override EndpointDataSource.GetGroupedEndpoints(GroupContext) and inspect the full group prefix and all the group metadata before calling RequestDelegateFactory.Create() or running any filters.

Even though RequestDelegateFactory now runs after any conventions are added to the endpoint (aside from conventions added by the RequestDelegateFactory), WithOpenApi is fixed by having the RouteEndointDataSource add MethodInfo before running any conventions. RouteEndointDataSource also adds any attributes as metadata. This is more similar to the previous behavior of RequestDelegateFactory in .NET 6 where it did not add this metadata.

Proposed API

namespace Microsoft.AspNetCore.Http;

public sealed class RequestDelegateFactoryOptions
{
-    public IEnumerable<object>? InitialEndpointMetadata { get; init; }
+    public IList<object>? EndpointMetadata { get; init; }
}

namespace Microsoft.AspNetCore.Routing;

public sealed class RouteGroupBuilder : IEndpointRouteBuilder, IEndpointConventionBuilder
{
-    // It was too easy for this to get out of sync with RouteGroupContext.Prefix in real life given RouteGroupBuilder decorators.
-    public RoutePattern GroupPrefix { get; }
}

+public sealed class RouteGroupContext
+{
+    public RouteGroupContext(RoutePattern prefix, IReadOnlyList<Action<EndpointBuilder>> conventions, IServiceProvider applicationServices);
+
+    public RoutePattern Prefix { get; }
+    public IReadOnlyList<Action<EndpointBuilder>> Conventions { get; }
+    public IServiceProvider ApplicationServices { get; }
+}

public abstract class EndpointDataSource
{
+    // What do we think about the name? We could also separate out the parameters, but this would mean breaking
+    // decorators every time we add a parameter. This is also something that will likely never be called by user code
+    // unless they were decorating or implementing their own version of RouteGroupBuilder.
+    // e.g. GetGroupedEndpoints(prefix, conventions, applicationServices). 
+    public virtual IReadOnlyList<Endpoint> GetGroupedEndpoints(RouteGroupContext context);
}

// The composite data source was never disposing change token registrations previously.
// It also didn't even listen to change tokens from EndponitDataSources added to the observable collection late.
-public sealed class CompositeEndpointDataSource : EndpointDataSource
+public sealed class CompositeEndpointDataSource : EndpointDataSource, IDisposable
{
+    public override IReadOnlyList<Endpoint> GetGroupedEndpoints(RouteGroupContext context);
+    public void Dispose();
}

// REVIEW: Renamed `AddFilter` to `AddRouteHandlerFilter` now that it will show up in intellisense after `MapControllers()` and stuff
// Can we come up with a better name? `AddRouteFiter`? Should we rename `RouteHandlerFilterExtensions`?
public static class RouteHandlerFilterExtensions
{
-    public static RouteHandlerBuilder AddFilter(this RouteHandlerBuilder builder, IRouteHandlerFilter filter);
+    public static TBuilder AddRouteHandlerFilter<TBuilder>(this TBuilder builder, IRouteHandlerFilter filter) where TBuilder : IEndpointConventionBuilder;

-    public static RouteHandlerBuilder AddFilter<[DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.PublicConstructors)] TFilterType>(this RouteHandlerBuilder builder)
-        where TFilterType : IRouteHandlerFilter;
+    public static RouteHandlerBuilder AddRouteHandlerFilter<[DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.PublicConstructors)] TFilterType>(this RouteHandlerBuilder builder)
+        where TFilterType : IRouteHandlerFilter;

+    public static TBuilder AddRouteHandlerFilter<TBuilder, [DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.PublicConstructors)] TFilterType>(this TBuilder builder)
+        where TBuilder : IEndpointConventionBuilder
+        where TFilterType : IRouteHandlerFilter;

+    public static RouteGroupBuilder AddRouteHandlerFilter<[DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.PublicConstructors)] TFilterType>(this RouteGroupBuilder builder)
+        where TFilterType : IRouteHandlerFilter;


-    public static RouteHandlerBuilder AddFilter(this RouteHandlerBuilder builder, Func<RouteHandlerInvocationContext, RouteHandlerFilterDelegate, ValueTask<object?>> routeHandlerFilter);
+    public static TBuilder AddRouteHandlerFilter<TBuilder>(this TBuilder builder, Func<RouteHandlerInvocationContext, RouteHandlerFilterDelegate, ValueTask<object?>> routeHandlerFilter)
+        where TBuilder : IEndpointConventionBuilder;

-    public static RouteHandlerBuilder AddFilter(this RouteHandlerBuilder builder, Func<RouteHandlerContext, RouteHandlerFilterDelegate, RouteHandlerFilterDelegate> filterFactory);
+    public static TBuilder AddRouteHandlerFilter<TBuilder>(this TBuilder builder, Func<RouteHandlerContext, RouteHandlerFilterDelegate, RouteHandlerFilterDelegate> filterFactory)
+        where TBuilder : IEndpointConventionBuilder;
}

namespace Microsoft.AspNetCore.OpenApi;

// REVIEW: I decided `WithOpenApi()` isn't too confusing of a name even though it shows up in places it won't work now
// Do we agree this doesn't need renamed?
public static class OpenApiRouteHandlerBuilderExtensions
{
-    public static RouteHandlerBuilder WithOpenApi(this RouteHandlerBuilder builder);
+    public static TBuilder WithOpenApi<TBuilder>(this TBuilder builder) where TBuilder : IEndpointConventionBuilder;

-    public static RouteHandlerBuilder WithOpenApi(this RouteHandlerBuilder builder, Func<OpenApiOperation, OpenApiOperation> configureOperation);
+    public static TBuilder WithOpenApi<TBuilder>(this TBuilder builder, Func<OpenApiOperation, OpenApiOperation> configureOperation);
}

Usage Examples

string GetString() => "Foo";

static void WithLocalSummary(RouteHandlerBuilder builder)
{
    builder.WithOpenApi(operation =>
    {
        operation.Summary += $" | Local Summary | 200 Status Response Content-Type: {operation.Responses["200"].Content.Keys.Single()}";
        return operation;
    });
}

WithLocalSummary(builder.MapDelete("/root", GetString));

var outerGroup = builder.MapGroup("/outer");
var innerGroup = outerGroup.MapGroup("/inner");

WithLocalSummary(outerGroup.MapDelete("/outer-a", GetString));

// The order WithOpenApi() is relative to the MapDelete() methods does not matter.
outerGroup.WithOpenApi(operation =>
{
    operation.Summary = "Outer Group Summary";
    return operation;
});

WithLocalSummary(outerGroup.MapDelete("/outer-b", GetString));
WithLocalSummary(innerGroup.MapDelete("/inner-a", GetString));

innerGroup.WithOpenApi(operation =>
{
    operation.Summary += " | Inner Group Summary";
    return operation;
});

WithLocalSummary(innerGroup.MapDelete("/inner-b", GetString));

var summaries = builder.DataSources.SelectMany(ds => ds.Endpoints).Select(e => e.Metadata.GetMetadata<OpenApiOperation>().Summary).ToArray();

Assert.Equal(5, summaries.Length);
Assert.Contains(" | Local Summary | 200 Status Response Content-Type: text/plain", summaries);
Assert.Contains("Outer Group Summary | Local Summary | 200 Status Response Content-Type: text/plain", summaries);
Assert.Contains("Outer Group Summary | Local Summary | 200 Status Response Content-Type: text/plain", summaries);
Assert.Contains("Outer Group Summary | Inner Group Summary | Local Summary | 200 Status Response Content-Type: text/plain", summaries);
Assert.Contains("Outer Group Summary | Inner Group Summary | Local Summary | 200 Status Response Content-Type: text/plain", summaries);
string PrintId(int id) => $"ID: {id}";

static void AddParamIncrementingFilter(IEndpointConventionBuilder builder)
{
    builder.AddRouteHandlerFilter(async (context, next) =>
    {
        context.Arguments[0] = ((int)context.Arguments[0]!) + 1;
        return await next(context);
    });
}

AddParamIncrementingFilter(builder.Map("/{id}", PrintId));

var outerGroup = builder.MapGroup("/outer");
AddParamIncrementingFilter(outerGroup);
AddParamIncrementingFilter(outerGroup.Map("/{id}", PrintId));

var innerGroup = outerGroup.MapGroup("/inner");
AddParamIncrementingFilter(innerGroup);
AddParamIncrementingFilter(innerGroup.Map("/{id}", PrintId));

var endpoints = builder.DataSources
    .SelectMany(ds => ds.Endpoints)
    .ToDictionary(e => ((RouteEndpoint)e).RoutePattern.RawText!);

Assert.Equal(3, endpoints.Count);

// For each layer of grouping, another filter is applies which increments the expectedId by 1 each time.
await AssertIdAsync(endpoints["/{id}"], expectedPattern: "/{id}", expectedId: 3);
await AssertIdAsync(endpoints["/outer/{id}"], expectedPattern: "/outer/{id}", expectedId: 4);
await AssertIdAsync(endpoints["/outer/inner/{id}"], expectedPattern: "/outer/inner/{id}", expectedId: 5);

Alternative Designs

When discussing the original desiging in #41427 (comment), I had originally planned to wait for the request to call RequestDelegateFacotry.Create() so only the final order of Endpoint.Metadata as the request came in would matter. @brunolins16 pointed out that this means you would not see any errors from RequestDelegateFactory.Create() until the specific endpoint being built was actually hit by a request. I think this would be a terrible development experience.

Furthermore, RequestDelegateFactory would then be unable to modify the endpoint metadata which is a complete non-starter. We designed entire IEndpointMetadataProvider and IEndpointParameterMetadataProvider around adding metadata in RequestDelegateFactory, and we filed an issue to also make this possible for filters (#41722). That's separate public API, but that's also added fixed by #42195 which implements this proposed API.

This also allows other custom EndopintDataSources to observe group conventions and prefix prior to creating their Endpoint instances.

Risks

This preserves the prior (in .NET 7 previews at least) behavior of calling RequestDelegateFactory.Create() for all the endpoints when all the Endpoint instances are initially built. This means locking routing globally while all this is happing.

@halter73 halter73 added the api-ready-for-review API is ready for formal API review - https://github.com/dotnet/apireviews label Jun 15, 2022
@ghost
Copy link

ghost commented Jun 15, 2022

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.

@halter73
Copy link
Member Author

halter73 commented Jun 16, 2022

API Review Notes:

  • Demo time!
  • GetGroupedEndpoints name?
    • What about GetEndpointGroup?
    • I think I prefer this.
  • What about AddRouteHandlerFilter?
    • Is it too long?
    • We don't think AddRouteHandlerFilter could ever work with other kinds of endpoints.
  • Is everyone okay with the RouteHandlerBuiler and RouteGroupBuilder overloads for the typed AddRouteHandlerFilter method?
    • I hear no objections.
  • What about WithOpenApi?
    • Is it too generic?
    • I did make it work if anything adds an OpenApiOperation to the metadata.
namespace Microsoft.AspNetCore.Http;

public sealed class RequestDelegateFactoryOptions
{
-    public IEnumerable<object>? InitialEndpointMetadata { get; init; }
+    public IList<object>? EndpointMetadata { get; init; }
}

namespace Microsoft.AspNetCore.Routing;

public sealed class RouteGroupBuilder : IEndpointRouteBuilder, IEndpointConventionBuilder
{
-    // It was too easy for this to get out of sync with RouteGroupContext.Prefix in real life given RouteGroupBuilder decorators.
-    public RoutePattern GroupPrefix { get; }
}

+public sealed class RouteGroupContext
+{
+    public RouteGroupContext(RoutePattern prefix, IReadOnlyList<Action<EndpointBuilder>> conventions, IServiceProvider applicationServices);
+
+    public RoutePattern Prefix { get; }
+    public IReadOnlyList<Action<EndpointBuilder>> Conventions { get; }
+    public IServiceProvider ApplicationServices { get; }
+}

public abstract class EndpointDataSource
{
+    public virtual IReadOnlyList<Endpoint> GetEndpointGroup(RouteGroupContext context);
}

-public sealed class CompositeEndpointDataSource : EndpointDataSource
+public sealed class CompositeEndpointDataSource : EndpointDataSource, IDisposable
{
+    public override IReadOnlyList<Endpoint> GetEndpointGroup(RouteGroupContext context);
+    public void Dispose();
}

public static class RouteHandlerFilterExtensions
{
-    public static RouteHandlerBuilder AddFilter(this RouteHandlerBuilder builder, IRouteHandlerFilter filter);
+    public static TBuilder AddRouteHandlerFilter<TBuilder>(this TBuilder builder, IRouteHandlerFilter filter) where TBuilder : IEndpointConventionBuilder;

-    public static RouteHandlerBuilder AddFilter<[DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.PublicConstructors)] TFilterType>(this RouteHandlerBuilder builder)
-        where TFilterType : IRouteHandlerFilter;
+    public static RouteHandlerBuilder AddRouteHandlerFilter<[DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.PublicConstructors)] TFilterType>(this RouteHandlerBuilder builder)
+        where TFilterType : IRouteHandlerFilter;

+    public static TBuilder AddRouteHandlerFilter<TBuilder, [DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.PublicConstructors)] TFilterType>(this TBuilder builder)
+        where TBuilder : IEndpointConventionBuilder
+        where TFilterType : IRouteHandlerFilter;

+    public static RouteGroupBuilder AddRouteHandlerFilter<[DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.PublicConstructors)] TFilterType>(this RouteGroupBuilder builder)
+        where TFilterType : IRouteHandlerFilter;

-    public static RouteHandlerBuilder AddFilter(this RouteHandlerBuilder builder, Func<RouteHandlerInvocationContext, RouteHandlerFilterDelegate, ValueTask<object?>> routeHandlerFilter);
+    public static TBuilder AddRouteHandlerFilter<TBuilder>(this TBuilder builder, Func<RouteHandlerInvocationContext, RouteHandlerFilterDelegate, ValueTask<object?>> routeHandlerFilter)
+        where TBuilder : IEndpointConventionBuilder;

-    public static RouteHandlerBuilder AddFilter(this RouteHandlerBuilder builder, Func<RouteHandlerContext, RouteHandlerFilterDelegate, RouteHandlerFilterDelegate> filterFactory);
+    public static TBuilder AddRouteHandlerFilter<TBuilder>(this TBuilder builder, Func<RouteHandlerContext, RouteHandlerFilterDelegate, RouteHandlerFilterDelegate> filterFactory)
+        where TBuilder : IEndpointConventionBuilder;
}

namespace Microsoft.AspNetCore.OpenApi;

public static class OpenApiRouteHandlerBuilderExtensions
{
-    public static RouteHandlerBuilder WithOpenApi(this RouteHandlerBuilder builder);
+    public static TBuilder WithOpenApi<TBuilder>(this TBuilder builder) where TBuilder : IEndpointConventionBuilder;

-    public static RouteHandlerBuilder WithOpenApi(this RouteHandlerBuilder builder, Func<OpenApiOperation, OpenApiOperation> configureOperation);
+    public static TBuilder WithOpenApi<TBuilder>(this TBuilder builder, Func<OpenApiOperation, OpenApiOperation> configureOperation);
}

API Approved!

@halter73 halter73 added api-approved API was approved in API review, it can be implemented and removed api-ready-for-review API is ready for formal API review - https://github.com/dotnet/apireviews labels Jun 16, 2022
@halter73
Copy link
Member Author

We want to rename the new GetEndpointGroup back to GetGroupedEndpoints as originally proposed. See #42195 (comment). Here's the proposed API diff vs what was last approved.

namespace Microsoft.AspNetCore.Http;

public abstract class EndpointDataSource
{
-    public virtual IReadOnlyList<Endpoint> GetEndpointGroup(RouteGroupContext context);
+    public virtual IReadOnlyList<Endpoint> GetGroupedEndpoints(RouteGroupContext context);
}

@halter73 halter73 added api-ready-for-review API is ready for formal API review - https://github.com/dotnet/apireviews and removed api-approved API was approved in API review, it can be implemented labels Jun 20, 2022
@ghost
Copy link

ghost commented Jun 20, 2022

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.

@BrennanConroy
Copy link
Member

Keeping open to track reacting to feedback in #42195 and for the API proposal in #41427 (comment)

@BrennanConroy
Copy link
Member

API review notes:

  • GetEndpointGroup implies that an EndpointGroup type is being returned
  • GetGroupedEndpoints has side-effects like running conventions and doing prefix pre-pending, can we come up with a name that suggests that?
    • No, not really without it being really ugly :D
  • Let's just make sure the doc comment is really clear about what the method is supposed to be doing.
namespace Microsoft.AspNetCore.Http;

public abstract class EndpointDataSource
{
-    public virtual IReadOnlyList<Endpoint> GetEndpointGroup(RouteGroupContext context);
+    public virtual IReadOnlyList<Endpoint> GetGroupedEndpoints(RouteGroupContext context);
}

API approved!

@BrennanConroy BrennanConroy added api-approved API was approved in API review, it can be implemented and removed api-ready-for-review API is ready for formal API review - https://github.com/dotnet/apireviews labels Jun 21, 2022
@rafikiassumani-msft rafikiassumani-msft added Docs This issue tracks updating documentation blog-candidate Consider mentioning this in the release blog post labels Jun 23, 2022
@halter73 halter73 closed this as completed Jul 5, 2022
@halter73 halter73 reopened this Jul 5, 2022
@halter73
Copy link
Member Author

halter73 commented Jul 5, 2022

Reopening to track GetEndpointGroup -> GetGroupedEndpoints rename.

@ghost ghost locked as resolved and limited conversation to collaborators Aug 12, 2022
@amcasey amcasey added the area-minimal Includes minimal APIs, endpoint filters, parameter binding, request delegate generator etc label Jun 2, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api-approved API was approved in API review, it can be implemented area-minimal Includes minimal APIs, endpoint filters, parameter binding, request delegate generator etc area-web-frameworks *DEPRECATED* This label is deprecated in favor of the area-mvc and area-minimal labels blog-candidate Consider mentioning this in the release blog post Docs This issue tracks updating documentation feature-minimal-actions Controller-like actions for endpoint routing Priority:0 Work that we can't release without
Projects
None yet
4 participants