RateLimitingMiddleware updates #43053
Conversation
wtgodbe
requested review from
halter73,
davidfowl,
adityamandaleeka,
DamianEdwards,
sebastienros and
HaoK
ghost
added
the
src/Middleware/RateLimiting/src/DisableRateLimitingAttribute.cs
Outdated
Show resolved
Hide resolved
src/Middleware/RateLimiting/src/RateLimiterOptionsExtensions.cs
Outdated
Show resolved
Hide resolved
src/Middleware/RateLimiting/test/RateLimitingMiddlewareTests.cs
Outdated
Show resolved
Hide resolved
src/Middleware/RateLimiting/src/DisableRateLimitingAttribute.cs
Outdated
Show resolved
Hide resolved
src/Middleware/RateLimiting/src/RateLimiterEndpointConventionBuilderExtensions.cs
Outdated
Show resolved
Hide resolved
| /// <summary> | ||
| /// The name of the policy which needs to be applied. | ||
| /// </summary> | ||
| public string? PolicyName { get; } |
There was a problem hiding this comment.
This feels super weird, from a consumer perspective there is no way this can be null.
There was a problem hiding this comment.
Sure it can. Add an inline policy using the RequireRateLimiting extension method and then look at the endpoint metadata. I get the point about not being able to trivially construct such an attribute, but hopefully we can add more first-class support for inline policies later.
| /// <summary> | ||
| /// The policy which needs to be applied, if present. | ||
| /// </summary> | ||
| internal DefaultRateLimiterPolicy? Policy { get; } |
There was a problem hiding this comment.
And this is sad, someone who wants to write their own rate limiting middleware (maybe they disagree with some design decision, like want to run global and endpoint specific limits no matter if one fails) with our options and attributes won't be able to access this.
There was a problem hiding this comment.
This is also the case with named polices. It's not like the policy maps on RateLimiterOptions are public. If we want to design for people replacing the middleware but not the options/metadata, we have to consider that way up front and not now. Maybe we could do it in a different major release if it's important. I'm not convinced yet.
wtgodbe
added
the
blog-candidate
|
@wtgodbe, this change will be considered for inclusion in the blog post for the release it'll ship in. Nice work! Please ensure that the original comment in this thread contains a clear explanation of what the change does, why it's important (what problem does it solve?), and, if relevant, include things like code samples and/or performance numbers. This content may not be exactly what goes into the blog post, but it will help the team putting together the announcement. Thanks! |
src/Middleware/RateLimiting/src/RateLimiterEndpointConventionBuilderExtensions.cs
Show resolved
Hide resolved
| var options = CreateOptionsAccessor(); | ||
| // Policy will disallow | ||
| var policy = new TestRateLimiterPolicy("myKey1", 404, false); | ||
| var defaultRateLimiterPolicy = new DefaultRateLimiterPolicy(RateLimiterOptions.ConvertPartitioner<string>(null, policy.GetPartition), policy.OnRejected); |
There was a problem hiding this comment.
I'd prefer if this test used RequireRateLimiting and passed in TestRateLimiterPolicy instead of calling internal only code.
There was a problem hiding this comment.
Will do this in a follow-up PR: #43298
amcasey
added
area-middleware
Resolves #42667
Action<Options>Rather than an Options instanceStill need to update the sample to use an MVC controller, which I'll do in this PR, but wanted to open it now to get eyes on it.