[5.4] RateLimiter bug fixes #18139
Merged
[5.4] RateLimiter bug fixes #18139
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| $cache->shouldReceive('increment')->once()->with('key'); | ||
| $rateLimiter = new RateLimiter($cache); | ||
|
|
||
| $rateLimiter->hit('key', 1); | ||
| } | ||
|
|
||
| public function testRetriesLeftReturnsCorrectCount() |
There was a problem hiding this comment.
This test seems kinda useless since the response is entirely mocked.
There was a problem hiding this comment.
The only mocked part is the caching of current attempts (set to 3).
The logic of retiresLeft() is not mocked. If you run that test now on the current unmodified code - it fails and returns 3.
It should be 2 - which this test detects.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There are 3x simultaneous bugs in the current RateLimiter. They currently cancel each other out so as whole the class is working - but individually none of the functions work as intended:
tooManyAttempts()Currently if you have
5attempts in the cache, and a$maxAttemptsof5- this function returnsfalse, when it should betrue.The reason this was not noticed until now is:
hit()Sets the initial count to
1, then immediately increments it to2. So the number of attempts is actually artificially increased by one.The reason that might never have been noticed until now is:
retriesLeft()Currently the function does this:
Which you can see is actually artificially inflating the retriesLeft by 1. The reason is because, if you had the initial
hit()wrong - the retriesLeft was also wrong. This PR reduces that code to justBecause you dont need to adjust
$attemptsanymore - you can take it "as is".This PR fixes all 3 bugs, yet should preserve the current behavior.
I've added an additional test for
retiresLeft()to help prevent regressions.