[LIMITATION] Feature: allow multiple Limiters with different configurations#292
[LIMITATION] Feature: allow multiple Limiters with different configurations#292GreeFine wants to merge 16 commits into
Conversation
JohnTitor
left a comment
JohnTitor
left a comment
There was a problem hiding this comment.
Sorry for the late review.
At a quick glance, I think the current has some issues and there could be a better approach.
| pub struct RateLimiter { | ||
| /// Used to define multiple limiter, with different configurations | ||
| /// | ||
| /// WARNING: When used (not None) the middleware will expect a `HashMap<Limiter>` in the actix-web `app_data` |
There was a problem hiding this comment.
This doesn't match with the implementation.
| limiters | ||
| .get(scope) | ||
| .unwrap_or_else(|| panic!("Unable to find defined limiter with scope: {}", scope)) | ||
| .clone() |
There was a problem hiding this comment.
I'm not sure how it's expensive to clone on each request, we should ensure it before merging.
No worries, thanks for the review ! I don't want to spend time on something that is not going to be used, but otherwise I will gladly take another go at this :) |
3 participants
PR Type
Feature
PR Checklist
cargo +nightly fmt).Overview
Currently I can declare and use only one Limiter who can hold one configuration
This PR adds the possibility to create a multitude of Limiter so we can have different configurations
Note
I added a method to build the Limiter with an already existing Client (
builder_with_redis_client) to prevent re-creating a new client for each new LimiterI don't actually know if this was a good idea and is making any performance improvements. And I didn't have the time to test it