UseRateLimiter API deviates by the Action-based config approach

[Elfocrash]

Background and Motivation

The most common approach when it comes to configuring options/settings in AddXXX, UseXXX and MapXXX methods during project setup is to use the Action-based approach.
For example AddControllers uses

public static IMvcBuilder AddControllers(this IServiceCollection services, Action<MvcOptions>? configure){}

not

public static IMvcBuilder AddControllers(this IServiceCollection services, MvcOptions? options){}

Which in return is being invoced with the services.Configure(configure) method which registers them as Options.
This is pretty standard across most .NET APIs (and libraries).

The UserRateLimiter API is using the object based approach which feels out of place compared to the other ones

public static IApplicationBuilder UseRateLimiter(this IApplicationBuilder app, RateLimiterOptions options)

Proposed API

Instead of providing the object based API, provide an Action based one instead:

public static IApplicationBuilder UseRateLimiter(this IApplicationBuilder app, Action<RateLimiterOptions> configure)

Usage Examples

app.UseRateLimiter(options =>
{
    options.DefaultRejectionStatusCode = 429;
});

Risks

We are still in previews so there isn't a breaking change risk.

[halter73]

There's a difference between IServiceCollection extension methods which add services like AddControllers and IApplicationBuilder extension methods that add middleware like UseRateLimiter when it comes to configuring options.

Sometimes, services are required for middleware to function. In those cases, the Use... IApplicationBuilder extension method won't allow you to configure options, and instead you're expected to configure options when you call the associated Add... IServiceCollection extension method. Examples of these include AddMvc/UseMvc and AddResponseCaching/UseResponseCaching. These days we recommend MapControllers over UseMvc, but I digress.

In other cases where middleware does not require services to function, we don't want to force people to call an Add... method just to configure options. It's one thing when the Add... call would be necessary anyway to add services, but this is another case entirely. Of course, you're not proposing introducing a new AddRateLimiter(this IServiceCollection services, Action<RateLimiterOptions> configure) method, you're suggesting changing the existing UseRateLimiter method to take a callback instead of the options directly, but that's also a problem.

By the time the IApplicationBuilder has been created, the service provider has already been built, so it's impossible to configure options the standard way which is using the options pattern the way AddMvc and all other Add... methods do. Following the options pattern ensures that anything that resolves IOptions<MvcOptions> from DI sees the final options value after all the configuration callbacks have been applied since all the callbacks are ultimately run as part of resolving a IConfigureOptions<MvcOptions> service that was added to DI. Any time you see a method taking Action<SomeOptionsType> in ASP.NET Core, you should be able to expect that it's calling Configure for you behind the scenes.

If you prefer using callbacks to configure your rate limiter options, you can still call Configure yourself:

builder.Services.Configure<RateLimiterOptions>(options =>
{
    options.DefaultRejectionStatusCode = 429;
});

// ...

app.UseRateLimiter();

People concerned about playing nice with DI and combining with other configuration sources and callbacks should configure options before the host and DI container are built. But when you're not worried about playing nice with DI, I think it's cleaner to directly pass in options than to mutate options objects in callbacks:

app.UseRateLimiter(new()
{
    DefaultRejectionStatusCode = 429
});

This isn't a new pattern either. UseStaticFiles handles options the same way as UseRateLimiter. So does UseHttpMethodOverride, UseWebSockets and probably others I'm not thinking of right now.

aspnetcore/src/Middleware/StaticFiles/src/StaticFileExtensions.cs

Line 55 in db4c946

public static IApplicationBuilder UseStaticFiles(this IApplicationBuilder app, StaticFileOptions options)

aspnetcore/src/Middleware/HttpOverrides/src/HttpMethodOverrideExtensions.cs

Line 38 in db4c946

public static IApplicationBuilder UseHttpMethodOverride(this IApplicationBuilder builder, HttpMethodOverrideOptions options)

aspnetcore/src/Middleware/WebSockets/src/WebSocketMiddlewareExtensions.cs

Line 45 in db4c946

public static IApplicationBuilder UseWebSockets(this IApplicationBuilder app, WebSocketOptions options)

Maybe we slipped up somewhere and are inconsistent with this, but I can't think of any IApplicationBuilder extension methods that take a callback for configuring options right now.

[Elfocrash]

There's a difference between IServiceCollection extension methods which add services like AddControllers and IApplicationBuilder extension methods

This one is actually unclear to me because there isn't any obvious consistency for the IApplicationBuilder extension methods on which ones use which approach and the reasoning behind the choice.

For example (not exhaustive list):

• UseCors, UseEndpoints and UseRouter use an Action only
• UseRewriter, UseSession and UseCookiePolicy use a raw object only
• UseSwagger and UseRequestLocalization have overloads for both Action and raw object

But when you're not worried about playing nice with DI, I think it's cleaner to directly pass in options than to mutate options objects in callbacks

What's the reasoning behind "cleaner" here? And what makes it unclean in this case but clean in all the other ones outlined above. For reference (and I know that this isn't directly affecting ASPNET Core but it's more of an ecosystem thing) the most popular .NET libraries approach their own middleware or configuration with Actions over raw objects, so more often than not, the current UseRateLimiter approach looks like the odd one out.

As a consumer, assuming that I own the UseRateLimiter call, I won't bother with the Configure call at all (which historically I use to configure things that I don't have direct control over (for example JsonOptions)) and instead just use the settings in the UseRateLimiter invocation directly.

Ultimately I am looking more of a reasoning behind the choice other than anything so I can apply it to my own code too, especially since the original Rate Limiting API Proposal by @rafikiassumani-msft used an Action over the raw object too.

[ShreyasJejurkar]

@Elfocrash Instead of changing that existing overload, we can have another one with Action<TOption>. As the documentation suggests people can pass Action<TOption> as well and raw options object as well. It's up to them what to use.

@halter73 I am pretty sure what you said is correct, but I didn't find anywhere that is documented so that people could know what's the recommended and general practices. Again the best place for that documentation would be here https://docs.microsoft.com/en-us/dotnet/core/extensions/options-library-authors

[Elfocrash]

@ShreyasJejurkar Does it make sense then to change the default UseRateLimiter method to be

public static IApplicationBuilder UseRateLimiter(this IApplicationBuilder app, Action<RateLimiterOptions>? configure = null)

and allow people to provide the value if they need it and then just keep the other overload that just uses RateLimiterOptions?

Or is there a preference towards having all 3 (empty, direct object, action-based)?

[ShreyasJejurkar]

Is there any paramter in options which is mandatory to set to RateLimiter work it properly!? (I don't know as I am yet to explore). If yes then no point of having empty overload, if no, then we should have empty overload to avoid cases like this (#39251)

[Elfocrash]

The default behavior of the limiter when no custom options are passed is to register a NoLimiter<HttpContext>() limiter, with no OnRejected action and a 503 status code for the throttling which won't happens since there is no limited by default. Does this even make sense? Should registering something that doesn't have any effect be possible? I assume that the idea behind this would be that you use the Services.Configure<RateLimiterOptions> call on the services to configure that logic separately.

[Tratcher]

@Elfocrash Instead of changing that existing overload, we can have another one with Action<TOption>. As the documentation suggests people can pass Action<TOption> as well and raw options object as well. It's up to them what to use.

@halter73 I am pretty sure what you said is correct, but I didn't find anywhere that is documented so that people could know what's the recommended and general practices. Again the best place for that documentation would be here https://docs.microsoft.com/en-us/dotnet/core/extensions/options-library-authors

Please don't add overloads that vary only by style.

Overall, we haven't been consistent about these patterns. We revisit it every few years and seem to come to different conclusions.

[ShreyasJejurkar]

@Elfocrash Instead of changing that existing overload, we can have another one with Action<TOption>. As the documentation suggests people can pass Action<TOption> as well and raw options object as well. It's up to them what to use.
@halter73 I am pretty sure what you said is correct, but I didn't find anywhere that is documented so that people could know what's the recommended and general practices. Again the best place for that documentation would be here https://docs.microsoft.com/en-us/dotnet/core/extensions/options-library-authors

Please don't add overloads that vary only by style.

Overall, we haven't been consistent about these patterns. We revisit it every few years and seem to come to different conclusions.

I thought the same some days ago like what's the point of having this many overloads which does the same thing under the hood? But then I thought (considered) if the docs are saying, then there might be something that I don't know!!

[halter73]

UseCors, UseEndpoints and UseRouter use an Action only

The difference with all these methods is that that those they configuring builders rather than options. UseEndpoints and UseRouter pass in an IEndpointRouteBuilder and IRouteBuilder to their callbacks respectively. UseCors passes a in a CorsPolicyBuilder.

IEndpointRouteBuilder and IRouteBuilder are very different from your typical options as these are used to Map... various endpoints after you've built your host.

Even CorsPolicyBuilder, which feels a bit more like options, is specific for when you want to configure just a single policy without a name. CorsOptions supports specifying a default policy and multiple named policies using AddDefaultPolicy and AddPolicy with different CorsPolicyBuilders, but no overload of UseCors takes an Action<CorsOptions>. If you want to use a default or named policy specified by CorsOptions, you cannot use the Action<CorsPolicyBuilder> overload.

If you call builder.Services.Configure<CorsPolicyBuilder>(builder => it won't do anything, but builder.Services.Configure<CorsOptions>(builder => works.

I still cannot think of any IApplicationBuilder extension methods that take a callback for configuring any IOptions.

[halter73]

The default behavior of the limiter when no custom options are passed is to register a NoLimiter<HttpContext>() limiter, with no OnRejected action and a 503 status code for the throttling which won't happens since there is no limited by default. Does this even make sense? Should registering something that doesn't have any effect be possible?

I agree that this is not very usable in its current state. In the PR that introduces the feature (#41008), @wtgodbe commented:

No endpoint awareness or piecemeal limiter construction is supported yet, that will be in the next pass (Right now if the user wants endpoint awareness, they must build it in to their PartitionedRateLimiter implementation)

I think we will end up adding a AddRateLimiter method when we add endpoint awareness. As soon as we create a non-trivial PartitionedRateLimiter for you, we'll probably want to register it as a service so it gets disposed along with the host. I imagine this AddRateLimiter method will have an overload take a Action<RateLimiterOptions> method which would be consistent with other middleware that uses services.

@wtgodbe Do we have any issues filed yet tracking adding endpoint awareness to UseRateLimiter()?

[wtgodbe]

@wtgodbe Do we have any issues filed yet tracking adding endpoint awareness to UseRateLimiter()?

Not yet, I just opened #41667.