Skip to content

write sample using service for rate limit #14

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

Closed
Rick-Anderson opened this issue Aug 18, 2022 · 9 comments · Fixed by #97
Closed

write sample using service for rate limit #14

Rick-Anderson opened this issue Aug 18, 2022 · 9 comments · Fixed by #97

Comments

@Rick-Anderson
Copy link
Contributor

Use a service per #13 (comment)

Put the code in this folder and delete the readme.md.

@Rick-Anderson
Copy link
Contributor Author

@Kahbazi can you do this issue?

@Rick-Anderson
Copy link
Contributor Author

IServiceCollection AddRateLimiter won't be available until the next release.

@Kahbazi
Copy link
Member

Kahbazi commented Aug 19, 2022

I didn't quite understand the purpose of this issue. We could update some samples in https://github.com/dotnet/AspNetCore.Docs.Samples/blob/257f38f079bd0dd41824a6ef452a6ce838ecdb04/fundamentals/middleware/rate-limit/WebRateLimitAuth/Program.cs to use AddRateLimiter instead of passing the options to the middleware.

Do we need to write new samples just for using AddRateLimiter??

I suggest we update all samples to use AddRateLimiter which might be the most use case. Then add one sample with two app.UseRateLimitingMiddleware with different options and every middleware in a different place in pipeline.

@Rick-Anderson
Copy link
Contributor Author

Per this comment, add sample code with attribute

We're removing AddNoLimiter in rc1 (we are also adding an attribute that allows you to disable rate limiting entirely for an endpoint, including the global limiter)

@Rick-Anderson
Copy link
Contributor Author

Rick-Anderson commented Sep 9, 2022

@Kahbazi Use a service per #13 (comment)

Long term I'd prefer using AddRateLimiter w/ services: https://github.com/dotnet/aspnetcore/blob/4cf483ee7a1cb470b8b2290a174ad34f450a5eab/src/Middleware/RateLimiting/src/RateLimiterServiceCollectionExtensions.cs#L20

So just rewrite the code so it's not using app.UseRateLimiter(new RateLimiterOptions()
but using it as service

Would you be able to update all the code to use the service approach? Wasn't that the intention of the comment? Do you think that's a better approach?

And add any new features in rate limiting that we want to document that have been added since this sample was written.

@Rick-Anderson
Copy link
Contributor Author

@fiyazbinhasan @Elfocrash @sammychinedu2ky @martincostello are you interested in changing the sample to use AddRateLimiter?

Use a service per #13 (comment)

Long term I'd prefer using AddRateLimiter w/ services: https://github.com/dotnet/aspnetcore/blob/4cf483ee7a1cb470b8b2290a174ad34f450a5eab/src/Middleware/RateLimiting/src/RateLimiterServiceCollectionExtensions.cs#L20

@fiyazbinhasan
Copy link
Contributor

I would like to take a look at it if no one has already started. However, the current sample is not runnable. I'll try to fix it along the way.

@Rick-Anderson
Copy link
Contributor Author

I would like to take a look at it if no one has already started. However, the current sample is not runnable. I'll try to fix it along the way.

Thanks, it's all yours.

@Rick-Anderson
Copy link
Contributor Author

Rick-Anderson commented Oct 11, 2022

@fiyazbinhasan when you do the update, can you add the following class:

Wrong issue, ignore

public class SampleProblemDetailsWriter : IProblemDetailsWriter
{
    public bool CanWrite(ProblemDetailsContext context)
        => context.HttpContext.Response.StatusCode == 400;

    public ValueTask WriteAsync(ProblemDetailsContext context)
    {
        //Additional customizations

        // Write to the response
        return new ValueTask(context.HttpContext.Response.WriteAsJsonAsync(context.ProblemDetails));
    }
}

From this comment

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants