[RateLimiter] Add RateLimiterBuilder
#60084
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
| @@ -0,0 +1,77 @@ | ||||||
| <?php | ||||||
|
|
||||||
| /* | ||||||
| * This file is part of the Symfony package. | ||||||
| * | ||||||
| * (c) Fabien Potencier <fabien@symfony.com> | ||||||
| * | ||||||
| * For the full copyright and license information, please view the LICENSE | ||||||
| * file that was distributed with this source code. | ||||||
| */ | ||||||
|
|
||||||
| namespace Symfony\Component\RateLimiter; | ||||||
|
|
||||||
| use Symfony\Component\Lock\LockInterface; | ||||||
| use Symfony\Component\RateLimiter\Policy\FixedWindowLimiter; | ||||||
| use Symfony\Component\RateLimiter\Policy\NoLimiter; | ||||||
| use Symfony\Component\RateLimiter\Policy\Rate; | ||||||
| use Symfony\Component\RateLimiter\Policy\SlidingWindowLimiter; | ||||||
| use Symfony\Component\RateLimiter\Policy\TokenBucketLimiter; | ||||||
| use Symfony\Component\RateLimiter\Storage\StorageInterface; | ||||||
| use Symfony\Component\RateLimiter\Util\TimeUtil; | ||||||
|
|
||||||
| /** | ||||||
| * @author Kevin Bond <kevinbond@gmail.com> | ||||||
| */ | ||||||
| class RateLimiterBuilder | ||||||
| { | ||||||
| public function __construct( | ||||||
| private StorageInterface $storage, | ||||||
| private ?LockInterface $lock = null, | ||||||
| ) { | ||||||
| } | ||||||
|
|
||||||
| /** | ||||||
| * @param string $id Unique identifier for the rate limiter | ||||||
| * @param int $limit Maximum allowed hits for the passed interval | ||||||
| * @param \DateInterval|string $interval If string, must be a number followed by "second", | ||||||
| * "minute", "hour", "day", "week" or "month" (or their | ||||||
| * plural equivalent) | ||||||
| */ | ||||||
| public function slidingWindow(string $id, int $limit, \DateInterval|string $interval): LimiterInterface | ||||||
|
There was a problem hiding this comment.
Suggested change
this is a bit a nitpick... but I like type correctness/safety where possible this function is intended to create a generalization here potentially allows There was a problem hiding this comment. Using the implementation class name in the return type prevents us from doing any refactoring in the future (like returning a decorated implementation for instance) as widening a return type is a BC break. And having the same interface for all limiter implementations is the whole point of having a LimiterInterface as the main type in the component. There was a problem hiding this comment. TL;DR
I see it as a way to prevent an unsafe change; when the type changes this communicates to upstream devs that they need to update their code.
This is still possible to do with expected implementation classes, but requires more work. It would require proxy class code generation (like Doctrine does, complicated), or creating tracing specializations of each limiter policy (does not scale well), or use of some AOP library (slow)...
Interface is preferably ok for as developer cannot make assumption on what concrete instance will be returned; arguably this is not the case for theRateLimiterBuilder's methods.
The contract for the methods of this class is type checked within this test which is good, but this test cannot guarantee types returned by the methods of classes derived from the RateLimiterBuilder
There was a problem hiding this comment. This would go against the IoC principle, which we strive for. Symfony is a piece of reusable and extensible software, it has to provide flexibility and face the possibility that people will do silly things. Closing the implementation like you're suggesting is on the contrary building on the idea that we should prevent end users from doing those silly things, at the cost of also preventing power users from doing what they want to achieve. That's not the approach we have in this project as a whole. We always favor SOLID principles because they're empowering end users (nothing prevents silly things anyway.) There was a problem hiding this comment.
No it is not good, because it means it is actually forbidden to change this (due to our BC policy) and so we would not be able to add support for traceable limiters in the future (duplicating traceable logic to have implementations done using inheritance instead of composition is a no-go. We did things like that in the early days of Symfony and it was a maintenance nightmare) There was a problem hiding this comment. with all due respect
IoC, here, is possible because this class does not follow
... in the favor of
I agree, I strongly believe that the architecture should favor both empowerment of users and prevention of invalid state (a.k.a. foot-guns or even CVEs)
This, however, is quite SOLID argument ;) |
||||||
| { | ||||||
| return new SlidingWindowLimiter($id, $limit, TimeUtil::intervalNormalizer($interval), $this->storage, $this->lock); | ||||||
| } | ||||||
|
|
||||||
| /** | ||||||
| * @param string $id Unique identifier for the rate limiter | ||||||
| * @param int $limit Maximum allowed hits for the passed interval | ||||||
| * @param \DateInterval|string $interval If string, must be a number followed by "second", | ||||||
| * "minute", "hour", "day", "week" or "month" (or their | ||||||
| * plural equivalent) | ||||||
| */ | ||||||
| public function fixedWindow(string $id, int $limit, \DateInterval|string $interval): LimiterInterface | ||||||
| { | ||||||
| return new FixedWindowLimiter($id, $limit, TimeUtil::intervalNormalizer($interval), $this->storage, $this->lock); | ||||||
| } | ||||||
|
|
||||||
| /** | ||||||
| * @param string $id Unique identifier for the rate limiter | ||||||
| * @param int $maxBurst The maximum allowed hits in a burst | ||||||
| * @param Rate $rate The rate at which tokens are added to the bucket | ||||||
| */ | ||||||
| public function tokenBucket(string $id, int $maxBurst, Rate $rate): LimiterInterface | ||||||
| { | ||||||
| return new TokenBucketLimiter($id, $maxBurst, $rate, $this->storage, $this->lock); | ||||||
| } | ||||||
|
|
||||||
| public function compound(LimiterInterface ...$limiters): LimiterInterface | ||||||
| { | ||||||
| return new CompoundLimiter($limiters); | ||||||
| } | ||||||
|
|
||||||
| public function noop(): LimiterInterface | ||||||
|
There was a problem hiding this comment.
Suggested change
the policy name in config for There was a problem hiding this comment. Hmm, yeah I really like Let's see what others think. |
||||||
| { | ||||||
| return new NoLimiter(); | ||||||
| } | ||||||
| } | ||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,37 @@ | ||
| <?php | ||
|
|
||
| /* | ||
| * This file is part of the Symfony package. | ||
| * | ||
| * (c) Fabien Potencier <fabien@symfony.com> | ||
| * | ||
| * For the full copyright and license information, please view the LICENSE | ||
| * file that was distributed with this source code. | ||
| */ | ||
|
|
||
| namespace Symfony\Component\RateLimiter\Tests; | ||
|
|
||
| use PHPUnit\Framework\TestCase; | ||
| use Symfony\Component\RateLimiter\CompoundLimiter; | ||
| use Symfony\Component\RateLimiter\LimiterInterface; | ||
| use Symfony\Component\RateLimiter\Policy\FixedWindowLimiter; | ||
| use Symfony\Component\RateLimiter\Policy\NoLimiter; | ||
| use Symfony\Component\RateLimiter\Policy\Rate; | ||
| use Symfony\Component\RateLimiter\Policy\SlidingWindowLimiter; | ||
| use Symfony\Component\RateLimiter\Policy\TokenBucketLimiter; | ||
| use Symfony\Component\RateLimiter\RateLimiterBuilder; | ||
| use Symfony\Component\RateLimiter\Storage\InMemoryStorage; | ||
|
|
||
| class RateLimiterBuilderTest extends TestCase | ||
| { | ||
| public function testCreateMethods() | ||
| { | ||
| $builder = new RateLimiterBuilder(new InMemoryStorage()); | ||
|
|
||
| $this->assertInstanceOf(SlidingWindowLimiter::class, $builder->slidingWindow('foo', 5, '1 minute')); | ||
| $this->assertInstanceOf(FixedWindowLimiter::class, $builder->fixedWindow('foo', 5, '1 minute')); | ||
| $this->assertInstanceOf(TokenBucketLimiter::class, $builder->tokenBucket('foo', 5, Rate::perHour(2))); | ||
| $this->assertInstanceOf(CompoundLimiter::class, $builder->compound($this->createMock(LimiterInterface::class))); | ||
| $this->assertInstanceOf(NoLimiter::class, $builder->noop()); | ||
| } | ||
| } |
Uh oh!
There was an error while loading. Please reload this page.