Skip to content

Limit manager as a service #2

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.

Sign up for GitHub

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

Already on GitHub? Sign in to your account

Open
goetas wants to merge 1 commit into
base: feature/extract-rate-limit-manager
Choose a base branch
from
Open

Limit manager as a service #2

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
31 changes: 11 additions & 20 deletions EventListener/RateLimitAnnotationListener.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,8 +9,6 @@
use Noxlogic\RateLimitBundle\Exception\RateLimitExceptionInterface;
use Noxlogic\RateLimitBundle\LimitProcessorInterface;
use Noxlogic\RateLimitBundle\Service\RateLimitService;
use Noxlogic\RateLimitBundle\Util\AnnotationLimitProcessor;
use Noxlogic\RateLimitBundle\Util\PathLimitProcessor;
use Symfony\Component\EventDispatcher\EventDispatcherInterface;
use Symfony\Component\HttpFoundation\Request;
use Symfony\Component\HttpFoundation\Response;
Expand All @@ -31,23 +29,23 @@ class RateLimitAnnotationListener extends BaseListener
protected $rateLimitService;

/**
* @var \Noxlogic\RateLimitBundle\Util\PathLimitProcessor
* @var LimitProcessorInterface
*/
protected $pathLimitProcessor;
protected $limitProcessor;

/**
* @param EventDispatcherInterface $eventDispatcher
* @param RateLimitService $rateLimitService
* @param PathLimitProcessor $pathLimitProcessor
* @param LimitProcessorInterface $limitProcessor
Copy link
Author

@goetas goetas Sep 6, 2019

Choose a reason for hiding this comment

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

This class now depends only on the interface, not on implementations anymore

*/
public function __construct(
EventDispatcherInterface $eventDispatcher,
RateLimitService $rateLimitService,
PathLimitProcessor $pathLimitProcessor
LimitProcessorInterface $limitProcessor
) {
$this->eventDispatcher = $eventDispatcher;
$this->rateLimitService = $rateLimitService;
$this->pathLimitProcessor = $pathLimitProcessor;
$this->limitProcessor = $limitProcessor;
}

/**
Expand All @@ -65,15 +63,7 @@ public function onKernelController(FilterControllerEvent $event)
return;
}

// Find the best match
Copy link
Author

@goetas goetas Sep 6, 2019

Choose a reason for hiding this comment

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

this logic has been moved in the AnnotationLimitProcessor as it is the one responsible for annotations

$annotations = $event->getRequest()->attributes->get('_x-rate-limit', array());

$limitProcessor = $this->pathLimitProcessor;
if ($annotations) {
$limitProcessor = new AnnotationLimitProcessor($annotations, $event->getController());
}

$rateLimit = $limitProcessor->getRateLimit($event->getRequest());
$rateLimit = $this->limitProcessor->getRateLimit($event->getRequest());

// Another treatment before applying RateLimit ?
$checkedRateLimitEvent = new CheckedRateLimitEvent($event->getRequest(), $rateLimit);
Expand All @@ -85,7 +75,7 @@ public function onKernelController(FilterControllerEvent $event)
return;
}

$key = $this->getKey($limitProcessor, $rateLimit, $event->getRequest());
$key = $this->getKey($rateLimit, $event);

$rateLimitInfo = $this->rateLimitService->getRateLimitInfo($key, $rateLimit);
if (!$rateLimitInfo) {
Expand Down Expand Up @@ -139,7 +129,7 @@ protected function findBestMethodMatch(Request $request, array $annotations)

// Empty array, check the path limits
if (count($annotations) == 0) {
return $this->pathLimitProcessor->getRateLimit($request);
return $this->limitProcessor->getRateLimit($request);
}

$best_match = null;
Expand All @@ -160,13 +150,14 @@ protected function findBestMethodMatch(Request $request, array $annotations)
return $best_match;
}

private function getKey(LimitProcessorInterface $limitProcessor, RateLimit $rateLimit, Request $request)
private function getKey(RateLimit $rateLimit, FilterControllerEvent $event)
{
// Let listeners manipulate the key
$request = $event->getRequest();
$keyEvent = new GenerateKeyEvent($request, '', $rateLimit->getPayload());

$keyEvent->addToKey(join('.', $rateLimit->getMethods()));
$keyEvent->addToKey($limitProcessor->getRateLimitAlias($request));
$keyEvent->addToKey($this->limitProcessor->getRateLimitAlias($request, $event->getController()));

$this->eventDispatcher->dispatch(RateLimitEvents::GENERATE_KEY, $keyEvent);

Expand Down
3 changes: 2 additions & 1 deletion LimitProcessorInterface.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,8 @@ public function getRateLimit(Request $request);

/**
* @param Request $request
* @param callable $controller
* @return string
*/
public function getRateLimitAlias(Request $request);
public function getRateLimitAlias(Request $request, callable $controller);
Copy link
Author

@goetas goetas Sep 6, 2019

Choose a reason for hiding this comment

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

only thing that I did not manager to solve in an elegant way is the $controller callable

Copy link
Owner

@amanto amanto Sep 10, 2019

Choose a reason for hiding this comment

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

Will think about that case.
Cause original intention were to make Rate Limit framework agnostic.
With controller argument we are in situation, when interface is dictated by DI and symfony logic, that request and current controller can differ.

}
9 changes: 8 additions & 1 deletion Resources/config/services.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
<parameter key="noxlogic_rate_limit.rate_limit_service.class">Noxlogic\RateLimitBundle\Service\RateLimitService</parameter>
<parameter key="noxlogic_rate_limit.oauth_key_generate_listener.class">Noxlogic\RateLimitBundle\EventListener\OauthKeyGenerateListener</parameter>
<parameter key="noxlogic_rate_limit.path_limit_processor.class">Noxlogic\RateLimitBundle\Util\PathLimitProcessor</parameter>
<parameter key="noxlogic_rate_limit.annotation_limit_processor.class">Noxlogic\RateLimitBundle\Util\AnnotationLimitProcessor</parameter>
</parameters>

<services>
Expand All @@ -23,14 +24,20 @@
</call>
</service>

<service alias="noxlogic_rate_limit.processor" id="noxlogic_rate_limit.annotation_limit_processor" public="true"/>

<service id="noxlogic_rate_limit.annotation_limit_processor" class="%noxlogic_rate_limit.annotation_limit_processor.class%" public="false">
<argument id="noxlogic_rate_limit.path_limit_processor" type="service"/>
</service>

<service id="noxlogic_rate_limit.path_limit_processor" class="%noxlogic_rate_limit.path_limit_processor.class%">
<argument>%noxlogic_rate_limit.path_limits%</argument>
</service>

<service id="noxlogic_rate_limit.rate_limit_annotation_listener" class="%noxlogic_rate_limit.rate_limit_annotation_listener.class%">
<argument type="service" id="event_dispatcher" />
<argument type="service" id="noxlogic_rate_limit.rate_limit_service" />
<argument type="service" id="noxlogic_rate_limit.path_limit_processor" />
<argument type="service" id="noxlogic_rate_limit.processor" />

<tag name="kernel.event_listener" event="kernel.controller" method="onKernelController" priority="-10" />

Expand Down
32 changes: 18 additions & 14 deletions Util/AnnotationLimitProcessor.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,25 +9,25 @@
class AnnotationLimitProcessor implements LimitProcessorInterface
Copy link
Author

@goetas goetas Sep 6, 2019

Choose a reason for hiding this comment

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

This class is a service now, it is instantiated only once by the symfony DI. If the annotations package is not available, disabling it will not require changes in the code, only changing the noxlogic_rate_limit.processor alias

Copy link
Owner

@amanto amanto Sep 10, 2019

Choose a reason for hiding this comment

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

Main reason to move it into the service - to create object only once via DI?

Disabling class in DI good approach. Maybe I wrong, but it doesn't bring big value, cause if annotation package is not available we need to disable rate_limit_annotation_listener. And without events usage, annotation class will not be used as well.

Before that changes, Util/AnnotationLimitProcessor.php were tightly coupled with symfony framework, cause its use symfony related attributes.

So in general, I agree, that it can be service class, cause in any case it right now symfony specific.

Copy link
Author

@goetas goetas Sep 10, 2019

Choose a reason for hiding this comment

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

Having stateless services is always a good idea, in the previous implementation AnnotationLimitProcessor had to be instantiated in the request process, while now is instantiated via DI (with all the possible optimizations that symfony will be able to do as the upcoming preloading in 7.4).

Did not know that the aim of this bundle is to be framework agnostic.Wiring it into laravel might be possible, but not sure is the main goal.

{
/**
* @var array
* @var LimitProcessorInterface
*/
private $annotations;
private $fallback;

/**
* @var callable
*/
private $controller;

public function __construct(array $annotations, callable $controller)
public function __construct(LimitProcessorInterface $fallback)
Copy link
Author

@goetas goetas Sep 6, 2019

Choose a reason for hiding this comment

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

Here using the chain of responsibility pattern

{
$this->annotations = $annotations;
$this->controller = $controller;
$this->fallback = $fallback;
}

public function getRateLimit(Request $request)
{
$annotations = $request->attributes->get('_x-rate-limit', array());

if (!$annotations) {
return $this->fallback->getRateLimit($request);
}

$best_match = null;
foreach ($this->annotations as $annotation) {
foreach ($annotations as $annotation) {
// cast methods to array, even method holds a string
$methods = is_array($annotation->getMethods()) ? $annotation->getMethods() : array($annotation->getMethods());

Expand All @@ -44,14 +44,18 @@ public function getRateLimit(Request $request)
return $best_match;
}

public function getRateLimitAlias(Request $request)
public function getRateLimitAlias(Request $request, callable $controller)
Copy link
Owner

@amanto amanto Sep 10, 2019

Choose a reason for hiding this comment

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

Can't we use $request->attributes->get('_controller') as $controller ?

Copy link
Author

@goetas goetas Sep 11, 2019

Choose a reason for hiding this comment

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

That's definitely an option! That would be great

{
$annotations = $request->attributes->get('_x-rate-limit', array());
Copy link
Owner

@amanto amanto Sep 10, 2019

Choose a reason for hiding this comment

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

We are repeating this part in each of method.
I think, that its related to the fact, that current implementation with events implies very dynamic way to work with annotations. From my point of view, that fact push do not use service or need to rebuild service each time as new object.

Best option as I see - to load all annotation ahead, so we can really use fallback logic with chain of responsibility. Not sure if its possible in symfony, will try to find out. One worry, that it can be breacking change.

Copy link
Author

@goetas goetas Sep 10, 2019

Choose a reason for hiding this comment

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

Instantiating a new object is more expensive in therms of CPU than calling a method (but I think is too soon to consider this kind of optimizations).

Currently symfony does not allow to have dependency-injection containers per url/path, so this kind of if checks are inevitable. The same if was done here in the previous implementation.

But then based on that if condition was changing the underlying services, by swapping the rate limit processor with AnnotationLimitProcessor... that to me sounds a dirty trick. While the proposed implementation used the well known chain-of-responsibility pattern.


if (!$annotations) {
return $this->fallback->getRateLimitAlias($request, $controller);
}

if (($route = $request->attributes->get('_route'))) {
return $route;
}

$controller = $this->controller;

if (is_string($controller) && false !== strpos($controller, '::')) {
$controller = explode('::', $controller);
}
Expand Down
2 changes: 1 addition & 1 deletion Util/PathLimitProcessor.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,7 @@ public function getMatchedPath(Request $request)
return $this->getMatchedLimitPath($request);
}

public function getRateLimitAlias(Request $request)
public function getRateLimitAlias(Request $request, callable $controller)
{
return str_replace('/', '.', $this->getMatchedLimitPath($request));
}
Expand Down