NotificationsYou must be signed in to change notification settings
Fork9.7k
Star30.8k

[RateLimiter] Add support for long intervals (months and years)#43060

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

Merged

fabpot merged 1 commit intosymfony:5.4fromalexandre-daubois:feat/rate-limiter-long-interval

Oct 29, 2021

Merged

[RateLimiter] Add support for long intervals (months and years)#43060

fabpot merged 1 commit intosymfony:5.4fromalexandre-daubois:feat/rate-limiter-long-interval

Oct 29, 2021

Conversation

Copy link

Member

alexandre-daubois commentedSep 16, 2021•
edited
Loading

Q	A
Branch?	5.4
Bug fix?	no
New feature?	yes
Deprecations?	no
Tickets	Fix#42784
License	MIT
Doc PR	NA

As mentioned in the issue and as found on Stackoverflow, we may use timestamps instead of convertingDateInterval properties to seconds.

carsonbot added Status: Needs Review Feature RateLimiter labels

Sep 16, 2021

alexandre-daubois force-pushed thefeat/rate-limiter-long-interval branch 2 times, most recently frombed13d3 tofcfde37Compare

September 16, 2021 14:11

Copy link

Member

derrabus commentedSep 16, 2021

Tests please. 🙂

derrabus added Status: Needs Work and removed Status: Needs Review labels

Sep 16, 2021

derrabus added this to the5.4 milestone

Sep 16, 2021

Copy link

MemberAuthor

alexandre-daubois commentedSep 16, 2021

@derrabus indeed! Wasn't sure which tests to add though. But I'll definitely try to find some! 😄

Copy link

Member

derrabus commentedSep 16, 2021

Well, you should be able to come up with a test that fails on 5.4 and passes after your changes. After all, you're fixing a problem here. 😎

Copy link

MemberAuthor

alexandre-daubois commentedSep 16, 2021•
edited
Loading

Oh yeah of course you're right! Just added them. If you see anything else, please tell me!

Speaking of that, should this be rebased to 5.3 as a bug fix or is it considered as a feature and 5.4 is fine?

alexandre-daubois force-pushed thefeat/rate-limiter-long-interval branch fromfcfde37 todd44a83Compare

September 16, 2021 16:33

Copy link

Member

Choose a reason for hiding this comment

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

I would expect that this test will start to fail on March 1st, 2023. 🤓

Copy link

MemberAuthor

alexandre-dauboisSep 17, 2021•
edited
Loading

Choose a reason for hiding this comment

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

I'm struggling a bit with these tests, as months and years, of course, may have different number of days. Any advice welcome about this one!

Copy link

Member

Choose a reason for hiding this comment

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

Is the variable length of the limit desired? So far, the rate limiter worked with fixed lengths, IIRC. We would change that now.

Copy link

MemberAuthor

alexandre-dauboisSep 17, 2021•
edited
Loading

Choose a reason for hiding this comment

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

I'm wondering if it's worth testing this method doing a simple timestamp operation. What I mean is if it changes,FixedWindowLimiter tests and similar will break, as I updatedFixedWindowLimiterTest with long intervals too.

The lack of tests onTimeUtil currently makes me wonder even more.

OskarStark reviewed

Sep 20, 2021

View reviewed changes

src/Symfony/Component/RateLimiter/CHANGELOG.md Outdated

		---

		* The component is not experimental anymore
		* Add support of long intervals (months and years)

Copy link

Contributor

OskarStarkSep 20, 2021

Choose a reason for hiding this comment

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

Suggested change

	* Add supportof long intervals (months and years)
	* Add supportfor long intervals (months and years)

Copy link

MemberAuthor

alexandre-dauboisSep 20, 2021

Choose a reason for hiding this comment

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

Of course, fixed. Thanks, Oskar!

alexandre-daubois changed the title~~[RateLimiter] Add support of long intervals (months and years)~~[RateLimiter] Add support for long intervals (months and years)

Sep 20, 2021