Extract RateLimitAnnotationListener logic into service classes #86
Conversation
* Extract annotation logic into separate class AnnotationLimitProcessor, cover with tests moved logic from getAliasForRequest method * Deprecate findBestMethodMatch method. Keep it Backward Compatible * Deprecate PathLimitProcessor->getMatchedPath. Keep it Backward Compatible * Create LimitProcessorInterface, add to PathLimitProcessor, AnnotationLimitProcessor * Fix getAliasForRequest, controller as object with __invoke method implemented
* Created RateLimitInfoManager with all RateLimitInfo logic * Set request rate_limit_info attr in the end, after all RateLimitInfo logic * Tests/EventListener/MockStorage::createMockRate add optional reset time param * Move remaining attempts calculation into RateLimitInfo::getRemainingAttempts, cover with tests
* Highlight some existing features * Adding example of integration in Laravel
|
Looks good! I like the idea, just trying to review and see how best this can be done.. |
| */ | ||
| private $controller; | ||
|
|
||
| public function __construct(array $annotations, callable $controller) |
There was a problem hiding this comment.
If think this should be a service class, and these could be arguments to the functions getRateLimit and getRateLimitAlias.
In fact - the logic that gets the annotations from the request should be moved into getRateLimit and then this arg is not needed
There was a problem hiding this comment.
Sorry, didn't get the point about service class, can you please describe idea in more details?
In fact - the logic that gets the annotations from the request should be moved into getRateLimit and then this arg is not needed
In case of Laravel application it doesn't have by default annotations, exists some packaged, but if its possible to handle rate limit logic with configuration files I think it will be easier to start using the package by other laravel developers.
From current perspective as I see, Annotation vs PathLimit processor good fit into existing package logic. In other case I assume, there is will be much more changes.
There was a problem hiding this comment.
This point has not yet been addressed. As service, this class should not take as parameters things that depend on the request (controller and annotations).
array $annotations, callable $controller should be moved to the two methods below.
As example getRateLimitAlias does not need the $annotations array, and getRateLimit does not need the callable $controller.
There was a problem hiding this comment.
@goetas I don't think about AnnotationLimitProcessor as service class in general.
Main idea of LimitProcessorInterface were to provide interface for implementing additional rate limit rules. If we will move arguments from AnnotationLimitProcessor constructor into methods below, we will broke interface and will do it implementation related, not based on abstraction. Current LimitProcessorInterface fit the need of extending the rate limit rules logic. I will demonstrate some examples with current package version.
As you can see, at current MR, we already replacing PathLimitProcessor based on annotations in system, proving another implementation for LimitProcessorInterface for action with annotation rule:
$limitProcessor = $this->pathLimitProcessor;
if ($annotations) {
$limitProcessor = new AnnotationLimitProcessor($annotations, $event->getController());
}Additional case, that I already faced with in my application: were needed to restrict rate limit only for POST actions on /documents/long-processed-resource with specific type.
For that case we can create event and in event replace rate limit rule only for that single action:
$documentProcessing = new class() implements LimitProcessorInterface
{
/**
* @param Request $request
* @return RateLimit|null
*/
public function getRateLimit(Request $request)
{
return new RateLimit([
'limit' => 5,
'period' => 60,
'methods' => 'POST'
]);
}
/**
* @param Request $request
* @return string
*/
public function getRateLimitAlias(Request $request)
{
return 'documents/long-processed-resource/123';
}
};In fact - the logic that gets the annotations from the request should be moved into getRateLimit and then this arg is not needed
In current version, with events usage rate limit rule replaced dynamically.
To provide service level and take annotations inside getRateLimit, as I see at current moment, we need to know about all annotations declared and it will be another approach, differ from applied right now.
RateLimitService add getRateLimitInfo method from RateLimitInfoManager
|
@mcfedr can you please check discussions? |
|
Hi, sorry, was pretty busy in the last period, will do my best to review it in the next weeks |
There was a problem hiding this comment.
Nice refactoring. Here my review.
AnnotationLimitProcessorissues have not yet been addressed, see my comment #86 (comment)- Minor change on the deprecated method being still called by this library
| // Find the best match | ||
| $annotations = $event->getRequest()->attributes->get('_x-rate-limit', array()); | ||
|
|
||
| $rateLimit = $this->findBestMethodMatch($event->getRequest(), $annotations); |
There was a problem hiding this comment.
if findBestMethodMatch has been deprecated, this library should not use it too
There was a problem hiding this comment.
Nice catch!
Indeed, I already moved the logic into class methods, so do not use findBestMethodMatch and keep it just for BC
| */ | ||
| private $controller; | ||
|
|
||
| public function __construct(array $annotations, callable $controller) |
There was a problem hiding this comment.
This point has not yet been addressed. As service, this class should not take as parameters things that depend on the request (controller and annotations).
array $annotations, callable $controller should be moved to the two methods below.
As example getRateLimitAlias does not need the $annotations array, and getRateLimit does not need the callable $controller.
LimitProcessorInterface.php
Outdated
| { | ||
| /** | ||
| * @param Request $request | ||
| * @return mixed|RateLimit|null |
- do not use deprecated findBestMethodMatch
- fix phpdoc
- remove line to trigger travis build
|
Looks like, travis is broken: |
|
I know that a lot of time passed since you have created this pull request, if you are still interested in having it merged, could you please rebase it? (some if the problems you had are solved since now we support less symfony versions and php versions) |
3 participants
Need to replace existing Laravel middleware solution https://github.com/illuminate/routing/blob/master/Middleware/ThrottleRequests.php with current package, that support restrict access by actions/controllers, general rule applying, easier configuration with configuration files.
Extracted logic from
RateLimitAnnotationListenerclass into separate classes, so it can be reused. Laravel application using Symfony Request class, so from this point it is enough to build logic based on Request class.