Enable middleware support for different quotas per key. - #292
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #292 +/- ##
==========================================
+ Coverage 97.61% 97.71% +0.09%
==========================================
Files 32 32
Lines 1974 2101 +127
==========================================
+ Hits 1927 2053 +126
- Misses 47 48 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Wowza, @sp1ff, that's really cool. I've only superficially read over it so far, but it does sound very reasonable. I see a bunch of tests that are failing, so I'll treat this as a draft until you give me the go-ahead, but great work so far! |
|
OK, cool. I saw the failed tests, but I didn't want to spend time fixing them if your feedback was something like "this is a bad idea." Since you're receptive, my plan is to work a bit on a crate I want to build on top of governor (with a path dependency to my local fork) to gauge usability, then come back to this PR (I think it also needs to be rebased). |
This patch address issue boinkor-net#193 (KeyedRateLimiter with different quota per key) It adds a method, `get_quota()` to trait `RateLimitingMiddleware`. `get_quota()` returns an `Option<Gcra>`, and the default implementation simply returns `None` (largely along the lines suggested by twitu). As antifuchs anticipated, this has required some restructuring: - until now, middleware were simply types (no state)-- now they can have state. I changed `RateLimiter` to no longer have a phantom member for the middleware, but to actually own its `RateLimitingMiddleware` instance - I was able to preserve the `with_middleware()` signature by implementing `Default` on the stateless middlewares, and added a new method `use_middleware()` that can take stateful middlewares. - I moved `StateSnapshot` into gcra.rs in order to avoid introducing a cyclic dependency between modules gcra and middleware (`gcra::NotUntil` needs `StateSnapshot` and `RateLimitingMiddleware` now needs `gcra::Gcra`) - I made `Quota` and `Gcra` public (not just pub(crate)) so that middleware authors that want to support per-key quotas can use them - I moved the generic type parameter representing the keys off of `RateLimitingMiddleware` methods and on to the trait itself. It's only with the introduction of `get_quota()` that the type becomes important; it really can't be *anything*, but needs to line-up with the key type parameter to `RateLimiter`. More tests, additional constructors, and field testing are needed, but I want to post this to begin gathering feedback.
This is likely to be more convenient for implementors of keyed middleware; this way they won't have to fuss with locks, proxies, and all that there. This patch also updates `check_key_n()` to check the middleware for per-key quotas and to use this idiom.
1eb221d to
3b8515d
Compare
|
Alright. I'd like to add docs & squash my commits, but I think it's ready for review. Do you think this commit should include a few implementations of middleware that support per-key quotas (say, one based on |
|
@antifuchs Any chance I could get a review on this? I have a crate built on it that I'd like to publish (but crates.io won't allow dependencies outside itself, like a "git" dependency to my fork of this project). |
2 participants
This patch address issue #193 (KeyedRateLimiter with different quota per key)
It adds a method,
get_quota()to traitRateLimitingMiddleware.get_quota()returns anOption<Gcra>, and the default implementation simply returnsNone(largely along the lines suggested by twitu).As antifuchs anticipated, this has required some restructuring:
RateLimiterto no longer have a phantom member for the middleware, but to actually own itsRateLimitingMiddlewareinstancewith_middleware()signature by implementingDefaulton the stateless middlewares, and added a new methoduse_middleware()that can take stateful middlewares.StateSnapshotinto gcra.rs in order to avoid introducing a cyclic dependency between modules gcra and middleware (gcra::NotUntilneedsStateSnapshotandRateLimitingMiddlewarenow needsgcra::Gcra)QuotaandGcrapublic (not just pub(crate)) so that middleware authors that want to support per-key quotas can use themRateLimitingMiddlewaremethods and on to the trait itself. It's only with the introduction ofget_quota()that the type becomes important; it really can't be anything, but needs to line-up with the key type parameter toRateLimiter.More tests, additional constructors, and field testing are needed, but I want to post this to begin gathering feedback.