NotificationsYou must be signed in to change notification settings
Fork9.6k
Star30.4k

[RateLimiter] [WIP] add attribute for controllers methods#59920

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Jump to bottom

Open

RazielRodrigues wants to merge5 commits intosymfony:7.4

base:7.4

Choose a base branch

fromRazielRodrigues:razielrodrigues-feature/rate-limiter-controller-attribute

Open

[RateLimiter] [WIP] add attribute for controllers methods#59920

RazielRodrigues wants to merge5 commits intosymfony:7.4fromRazielRodrigues:razielrodrigues-feature/rate-limiter-controller-attribute

Conversation

Copy link

RazielRodrigues commentedMar 5, 2025•
edited by kbond
Loading

Q	A
Branch?	7.3
Bug fix?	no
New feature?	yes
Deprecations?	no
Issues	Fix#52518
License	MIT

feat: creating rate limiter attribute

00a59a3

carsonbot added Status: Needs Review Feature RateLimiter labels

Mar 5, 2025

carsonbot added this to the7.3 milestone

Mar 5, 2025

Copy link

carsonbot commentedMar 5, 2025

Hey!

I see that this is your first PR. That is great! Welcome!

Symfony has acontribution guide which I suggest you to read.

In short:

Always add tests
Keep backward compatibility (seehttps://symfony.com/bc).
Bug fixes must be submitted against the lowest maintained branch where they apply (seehttps://symfony.com/releases)
Features and deprecations must be submitted against the 7.3 branch.

Review the GitHub status checks of your pull request and try to solve the reported issues. If some tests are failing, try to see if they are failing because of this change.

When two Symfony core team members approve this change, it will be merged and you will become an official Symfony contributor!
If this PR is merged in a lower version branch, it will be merged up to all maintained branches within a few days.

I am going to sit back now and wait for the reviews.

Cheers!

Carsonbot

This comment was marked as resolved.

RazielRodrigues mentioned this pull request

Mar 5, 2025

[RateLimiter] controller attribute#52518

Open

Copy link

Member

alexandre-daubois commentedMar 6, 2025

Hello, first of all thank you for your interest in contributing to Symfony!

It seems the PR is incomplete. The new attribute is indeed created, but it lacks the mechanism to register it and actually rate limit methods, right?

kbond reviewed

Mar 6, 2025

View reviewed changes

Copy link

Member

kbond left a comment•
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Hey@RazielRodrigues, thanks for getting this started!

This will work very similar toIsGranted andIsGrantedAttributeListener so check out the code around these.

src/Symfony/Component/RateLimiter/Attribute/RateLimitAttribute.php Outdated

		* @author Raziel Rodrigues <raziel.rodrigues@outlook.pt>
		*/
		#[\Attribute(\Attribute::IS_REPEATABLE \| \Attribute::TARGET_METHOD)]
		final class RateLimit

Copy link

Member

kbondMar 6, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

You'll need to rename this file to match the class name:RateLimit.php

Copy link

Author

RazielRodriguesMay 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

done

src/Symfony/Component/RateLimiter/Attribute/RateLimitAttribute.php Outdated

		*/
		public function __construct(
		public string $limiter,
		public array $methods

Copy link

Member

kbondMar 6, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Suggested change

	public array$methods
	public array$methods = [],

My thinking is:[] === all methods (the default)

src/Symfony/Component/RateLimiter/Attribute/RateLimitAttribute.php Outdated

Comment on lines 25 to 26

		* @param string $limiter The name of the limiter to use
		* @param array $methods Methods to apply the rate limit

Copy link

Member

kbondMar 6, 2025•
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Suggested change

	* @param string$limiter Thename of thelimitertouse
	* @paramarray$methodsMethodsto apply the rate limit
	* @param string$limiter Theconfiguredlimitername
	* @paramstring[]$methodsRequest methodsto apply the rate limit (`[]` for all)

src/Symfony/Component/RateLimiter/Attribute/RateLimitAttribute.php Outdated

		namespace Symfony\Component\RateLimiter\Attribute;

		/**
		* Add the hability to rate limit a method from a controller

Copy link

Member

kbondMar 6, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Suggested change

	*Add the hability to ratelimita method from acontroller
	*Ratelimitthecontroller.

kbond changed the title~~[RateLimiter] add attribute for controllers methods~~[WIP][RateLimiter] add attribute for controllers methods

Mar 6, 2025

kbond changed the title~~[WIP][RateLimiter] add attribute for controllers methods~~[RateLimiter] [WIP] add attribute for controllers methods

Mar 6, 2025

Copy link

Member

yceruto commentedMar 6, 2025

Question: should the #[RateLimit] attribute point to an implementation ofRequestRateLimiterInterface? From the linked proposal, it looks like it's referencing the factory configuration, which seems incorrect.

Copy link

Member

kbond commentedMar 6, 2025•
edited
Loading

Question: should the #[RateLimit] attribute point to an implementation of RequestRateLimiterInterface?

I'd actually forgotten aboutRequestRateLimiterInterface. My thinking of this features was indeed:#RateLimit('name') would use the configured rate limiter factoryname. By default, the limiter would be created for a key with url, method, client ip. This could be customized with an expression - maybe even having the user available?

#[RateLimit('my_limiter', key:'url~method~request.getClientIp()~user?.getUsername()')

But now, remembering theRequestRateLimiterInterface, I wonder if it could be leveraged...

Copy link

Author

RazielRodrigues commentedMar 18, 2025

Question: should the #[RateLimit] attribute point to an implementation of RequestRateLimiterInterface?
I'd actually forgotten aboutRequestRateLimiterInterface. My thinking of this features was indeed:#RateLimit('name') would use the configured rate limiter factoryname. By default, the limiter would be created for a key with url, method, client ip. This could be customized with an expression - maybe even having the user available?
#[RateLimit('my_limiter', key: 'url~method~request.getClientIp()~user?.getUsername()')
But now, remembering theRequestRateLimiterInterface, I wonder if it could be leveraged...

Thanks for all the comments guys, I will work on this MR this week, I was a bit busy but now I am totally free to work on this merge request, ASAP I will commit the changes

RazielRodrigues added3 commits

May 12, 2025 23:16

feat: creating rate limiter attribute

6b0c5a9

refactor: changing the name of file

67316e3

feat: adding the rate limit attribute listener

279217d

Copy link

Author

RazielRodrigues commentedMay 13, 2025

Hello, first of all thank you for your interest in contributing to Symfony!
It seems the PR is incomplete. The new attribute is indeed created, but it lacks the mechanism to register it and actually rate limit methods, right?

Hi do you know how can I test the stuff I have done? Because I implemented the event and so on but now I need to test it and I don't know how