[RateLimiter] Add `RateLimiterBuilder` #60084

kbond · 2025-03-29T18:17:37Z

Q	A
Branch?	7.3
Bug fix?	no
New feature?	yes
Deprecations?	no
Issues	Fix #51401 (comment)
License	MIT

This is an alternative to #51403.

The idea here is to make it simpler to create one-off rate limiters. Currently, if you had 10 things you want to rate limit differently, you need to configure/inject 10 different rate limiters. This adds an option for a simpler approach: inject this builder and create your rate limiters on the fly.

If there's agreement on this feature, I'll add tests/configuration.

I'm not a huge fan of RateLimiterBuilder for the name but RateLimiterFactory is already taken. I'm open to alternatives.

Usage

class ApiController extends AbstractController
{
    public function registerUser(
        Request $request,
        RateLimiterBuilder $rateLimiterBuilder,
    ): Response {
        $limiter = $rateLimiterBuilder->slidingWindow(
            id: '...',
            limit: 10,
            interval: '10 minutes',
        );

        // ...
    }
}

TODO

Update changelog
Tests
FrameworkBundle configuration

src/Symfony/Component/RateLimiter/RateLimiterBuilder.php

kaznovac · 2025-04-07T09:45:52Z

src/Symfony/Component/RateLimiter/RateLimiterBuilder.php

+        return new CompoundLimiter($limiters);
+    }
+
+    public function noop(): LimiterInterface


Suggested change

public function noop(): LimiterInterface

public function noLimit(): LimiterInterface

the policy name in config for NoLimiter is no_limit - it's nice to be consistent for discoverability purposes (personally, I like the noop name better)

Hmm, yeah I really like noop too - especially since the other methods don't include "Limit" in their name.

Let's see what others think.

src/Symfony/Component/RateLimiter/RateLimiterBuilder.php

kaznovac · 2025-04-07T10:06:02Z

src/Symfony/Component/RateLimiter/RateLimiterBuilder.php

+     *                                       "minute", "hour", "day", "week" or "month" (or their
+     *                                       plural equivalent)
+     */
+    public function slidingWindow(string $id, int $limit, \DateInterval|string $interval): LimiterInterface


Suggested change

public function slidingWindow(string $id, int $limit, \DateInterval|string $interval): LimiterInterface

public function slidingWindow(string $id, int $limit, \DateInterval|string $interval): SlidingWindowLimiter

this is a bit a nitpick... but I like type correctness/safety where possible

this function is intended to create a slidingWindow or its specialization (yes the limiter classes are final so specialization is out-of-reach; but his might change in the future)

generalization here potentially allows slidingWindow to return any other Limiter policy in other RateLimiterBuilder specializations (consider marking this class as final) which is undesirable/unexpected... and might require upstream casting of return value if the concrete implementation method/property is required (currently not the case - all Limiter implementation have the same interface)

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.
For instance, we could imagine that in the future the returned limiters could be decorated with a traceable limiter in dev to be integrated with the profiler.

And having the same interface for all limiter implementations is the whole point of having a LimiterInterface as the main type in the component.

TL;DR
I'd suggest making this RateLimiterBuilder class final

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.

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.
Yes, changing this type is an explicit BC break, and that is actually good - it can be detected with static analyzers and in runtime type checking; using the interface type allows contributions that potentially introduce unexpected behavior in the runtime. Interface approach leaves the function name as the sole 'contract' - more of a phpDoc hint.

For instance, we could imagine that in the future the returned limiters could be decorated with a traceable limiter in dev to be integrated with the profiler.

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)...

And having the same interface for all limiter implementations is the whole point of having a LimiterInterface as the main type in the component.

Interface is preferably ok for

symfony/src/Symfony/Component/RateLimiter/RateLimiterFactory.php

Line 42 in 8b44a51

public function create(?string $key = null): LimiterInterface

as developer cannot make assumption on what concrete instance will be returned; arguably this is not the case for the RateLimiterBuilder's methods.

The contract for the methods of this class is type checked within this test

symfony/src/Symfony/Component/RateLimiter/Tests/RateLimiterBuilderTest.php

Lines 27 to 36 in eae6a0d

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());

}

which is good, but this test cannot guarantee types returned by the methods of classes derived from the RateLimiterBuilder

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.)

Yes, changing this type is an explicit BC break, and that is actually good

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)

with all due respect

This would go against the IoC principle, which we strive for. ...

IoC, here, is possible because this class does not follow SOLID principles:
S - breached - multiple classes instantiated
O[C] - not enforced - not open for extension (not implemented, do not confuse with class extends), not closed (this is class extends, and it is not forbidden)
L - not enforced - interface for return type makes breaching this one easy
I - not implemented
D - ok - made possible by compromises above

Closing the implementation like you're suggesting is ...

... in the favor of C in O[C] principle of SOLID

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.)

I agree, SOLID is a great design guide to follow (albeit class explosion and discovery), but this is a utility class and should probably be spared from 'power'-future-proofing.

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)

(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)

This, however, is quite SOLID argument ;)

src/Symfony/Bundle/FrameworkBundle/Tests/DependencyInjection/PhpFrameworkExtensionTest.php

nicolas-grekas · 2025-04-08T13:57:26Z

I'd prefer a solution that doesn't create a new sibling concept that's hard to grasp vs "factory".
Can't we have something like a wither API on the factory class?

kbond · 2025-04-08T14:17:10Z

Can't we have something like a wither API on the factory class?

Can you describe what you're thinking here a bit more? Feels like this ship has sailed as we now have 2 RateLimiterFactoryInterface implementations.

I guess you mean something like:

$factory->slidingWindow(...)->create(...);
$factory->fixedWindow(...)->create(...);

It wouldn't make sense to add these methods to the RateLimiterFactoryInterface (because $compoundFactory->slidingWindow()->create() doesn't make sense) - so it'd have to be another implementation... which sort of brings us back to the same problem. But I guess we could continue using the name *RateLimiterFactory - ManualRateLimiterFactory or CustomRateLimiterFactory maybe?

kaznovac · 2025-04-09T20:32:09Z

maybe something like this:

$factory->createBuilder(): RateLimiterBuilderInterface

interface RateLimiterBuilderInterface {
  function setLock(?LockInterface $lock): self; /** if desired to allow changing of factory provided services */
  function setStorage(StorageInterface $storage): self;

  function setId(string $id): self; /** or better setName */
  /** @param 'fixed_window'|'token_bucket'|'sliding_window'|'no_limit' $policy */
  function setPolicy(string $policy): self;

  function setLimit(int $limit): self;
  function setInterval(string $interval): self;
  function setRate(string $interval, int $amount): self; /** these can be noop where not-applicable to policy */

  function reset(): self; /** optional */

  function build(): LimiterInterface;
}

If you want to follow dogmatic Builder pattern - for each Limiter class a dedicated builder class should be crated; strenuous, but allows nice strategy pattern in the new factory's createBuilder(string $policy): RateLimiterBuilderInterface
(this aproach fully satisfies O - open for extension - new builders can be registered per policy, and factory is closed for modification as it's explicitly final; and typehint it is through phpstan conditional types)

Regarding CompoundLimiterBuilder, it can have addLimiter(LimiterInterface $limiter): self and just wrap all provided limiters in adequate CompoundLimiter instance on build.

kbond added RFC RFC = Request For Comments (proposals about features that you want to be discussed) DX DX = Developer eXperience (anything that improves the experience of using Symfony) labels Mar 29, 2025

carsonbot changed the title ~~[RateLimiter] Add RateLimiterBuilder~~ Add RateLimiterBuilder Mar 29, 2025

carsonbot added Status: Needs Review Feature RateLimiter labels Mar 29, 2025

carsonbot added this to the 7.3 milestone Mar 29, 2025

carsonbot changed the title ~~Add RateLimiterBuilder~~ [RateLimiter] Add RateLimiterBuilder Mar 29, 2025

kbond force-pushed the rate-limiter/builder branch from 48b0607 to 16a7dd5 Compare March 30, 2025 14:01

alexandre-daubois reviewed Mar 31, 2025

View reviewed changes

src/Symfony/Component/RateLimiter/RateLimiterBuilder.php Show resolved Hide resolved

stof reviewed Mar 31, 2025

View reviewed changes

src/Symfony/Component/RateLimiter/RateLimiterBuilder.php Outdated Show resolved Hide resolved

kbond force-pushed the rate-limiter/builder branch 2 times, most recently from 3f5d2d9 to 6d69999 Compare April 2, 2025 23:45

kbond removed the RFC RFC = Request For Comments (proposals about features that you want to be discussed) label Apr 2, 2025

kbond added 4 commits April 4, 2025 10:29

[RateLimiter] Add RateLimiterBuilder

b791d92

frameworkbundle configuration/tests

9fa23d5

changelog

5f3bce5

test

224dbab

kbond force-pushed the rate-limiter/builder branch from 6d69999 to 224dbab Compare April 4, 2025 14:30

fix tests

84d055e

kaznovac reviewed Apr 7, 2025

View reviewed changes

review

eae6a0d

fabpot modified the milestones: 7.3, 7.4 May 26, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

[RateLimiter] Add `RateLimiterBuilder` #60084

[RateLimiter] Add `RateLimiterBuilder` #60084

Uh oh!

kbond commented Mar 29, 2025 •

edited

Loading

Uh oh!

Uh oh!

Uh oh!

kaznovac Apr 7, 2025

Uh oh!

kbond Apr 7, 2025

Uh oh!

Uh oh!

kaznovac Apr 7, 2025

Uh oh!

stof Apr 7, 2025

Uh oh!

kaznovac Apr 8, 2025

Uh oh!

nicolas-grekas Apr 8, 2025 •

edited

Loading

Uh oh!

stof Apr 8, 2025

Uh oh!

kaznovac Apr 9, 2025

Uh oh!

Uh oh!

nicolas-grekas commented Apr 8, 2025 •

edited

Loading

Uh oh!

kbond commented Apr 8, 2025 •

edited

Loading

Uh oh!

kaznovac commented Apr 9, 2025

Uh oh!

Uh oh!

	public function noop(): LimiterInterface
	public function noLimit(): LimiterInterface

	public function slidingWindow(string $id, int $limit, \DateInterval\|string $interval): LimiterInterface
	public function slidingWindow(string $id, int $limit, \DateInterval\|string $interval): SlidingWindowLimiter

	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!

[RateLimiter] Add RateLimiterBuilder #60084

Are you sure you want to change the base?

[RateLimiter] Add RateLimiterBuilder #60084

Uh oh!

Conversation

kbond commented Mar 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

kaznovac Apr 7, 2025

Choose a reason for hiding this comment

Uh oh!

kbond Apr 7, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

kaznovac Apr 7, 2025

Choose a reason for hiding this comment

Uh oh!

stof Apr 7, 2025

Choose a reason for hiding this comment

Uh oh!

kaznovac Apr 8, 2025

Choose a reason for hiding this comment

Uh oh!

nicolas-grekas Apr 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stof Apr 8, 2025

Choose a reason for hiding this comment

Uh oh!

kaznovac Apr 9, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

nicolas-grekas commented Apr 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kbond commented Apr 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kaznovac commented Apr 9, 2025

Uh oh!

Uh oh!

[RateLimiter] Add `RateLimiterBuilder` #60084

[RateLimiter] Add `RateLimiterBuilder` #60084

kbond commented Mar 29, 2025 •

edited

Loading

nicolas-grekas Apr 8, 2025 •

edited

Loading

nicolas-grekas commented Apr 8, 2025 •

edited

Loading

kbond commented Apr 8, 2025 •

edited

Loading