nikolaposa / rate-limit

🚔 General purpose rate limiter implementation.
MIT License
269 stars 47 forks source link

Allow setting the limit externally #13

Closed poursal closed 5 years ago

nikolaposa commented 5 years ago

I don't like this idea of making RateLimiter object mutable. There are plenty other ways for achieving the goal of having multiple rate limiting strategies. For instance, you could have DefaultRateLimiter and AuthenticatedUsersRateLimiter as two separate services in your dependency injection container. Alternatively, you can create a ProxyRateLimiter that would lazy-load actual properly configured RateLimiter:

final class ProxyRateLimiter implements RateLimiterInterface
{
    private $authenticationService;

    private $realRateLimiter;

    public function __construct(AuthenticationService $authenticationService)
    {
        $this->authenticationService = $authenticationService;
    }

    public function hit(string $key)
    {
        return $this->getRateLimiter()->hit($key);
    }

    //... implement all the other methods by proxying to real rate limiter

    private function getRateLimiter() : RateLimiterInterface
    {
        if (null === $this->rateLimiter) {
            $this->rateLimiter = new InMemoryRateLimiter(
                $this->authenticationService->hasIdentity() 
                    ? 1000 
                    : 10
            );
        }

        return $this->rateLimiter;
    }
}

Closing.

poursal commented 5 years ago

Hello Nikola,

I believe that the real problem is modifying the window and not the limit (this is why I only included the setter for the limit). Since the core philosophy of the rate limiter is on incrementing a counter I see no problem is allowing this setter.

Nevertheless one can use the proposed alternative with the additional complexity.

nikolaposa commented 5 years ago

@poursal Yeah, but I even think that including getters for limit and window in the RateLimiter interface was a bad idea on its own, and I'll probably remove those in some major release. RateLimiter API should not expose them as they are basically implementation details specific for a concrete RateLimiter implementation.