Limit manager as a service #2

goetas · 2019-09-06T14:44:33Z

Hi, I have created this PR with the code changes I was suggesting.
Going to comment in the code the main changes

goetas

I have highlighted the main changes, did not update the tests yet, if you are ok with it I can do that too.

goetas · 2019-09-06T14:46:32Z

EventListener/RateLimitAnnotationListener.php

     * @param EventDispatcherInterface $eventDispatcher
     * @param RateLimitService $rateLimitService
-     * @param PathLimitProcessor $pathLimitProcessor
+     * @param LimitProcessorInterface $limitProcessor


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

goetas · 2019-09-06T14:47:10Z

EventListener/RateLimitAnnotationListener.php

            return;
        }

-        // Find the best match


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

goetas · 2019-09-06T14:48:51Z

Util/AnnotationLimitProcessor.php

 use Noxlogic\RateLimitBundle\LimitProcessorInterface;
 use Symfony\Component\HttpFoundation\Request;

 class AnnotationLimitProcessor implements LimitProcessorInterface


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

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.

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.

goetas · 2019-09-06T14:49:46Z

LimitProcessorInterface.php

     * @return string
     */
-    public function getRateLimitAlias(Request $request);
+    public function getRateLimitAlias(Request $request, callable $controller);


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

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.

goetas · 2019-09-06T14:50:14Z

Util/AnnotationLimitProcessor.php

-    private $controller;
-
-    public function __construct(array $annotations, callable $controller)
+    public function __construct(LimitProcessorInterface $fallback)


Here using the chain of responsibility pattern

amanto

@goetas added some ideas. Will think about common interface.

amanto · 2019-09-10T08:32:39Z

LimitProcessorInterface.php

     * @return string
     */
-    public function getRateLimitAlias(Request $request);
+    public function getRateLimitAlias(Request $request, callable $controller);


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.

amanto · 2019-09-10T08:39:44Z

Util/AnnotationLimitProcessor.php

 use Noxlogic\RateLimitBundle\LimitProcessorInterface;
 use Symfony\Component\HttpFoundation\Request;

 class AnnotationLimitProcessor implements LimitProcessorInterface


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.

amanto · 2019-09-10T08:49:06Z

Util/AnnotationLimitProcessor.php

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


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.

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.

amanto · 2019-09-10T08:56:34Z

Util/AnnotationLimitProcessor.php

    }

-    public function getRateLimitAlias(Request $request)
+    public function getRateLimitAlias(Request $request, callable $controller)


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

That's definitely an option! That would be great

use more "service oriented" implementation for the rate limiter

dddd936

goetas force-pushed the feature/extract-rate-limit-manager branch from f30fd09 to dddd936 Compare September 6, 2019 14:45

goetas commented Sep 6, 2019

View reviewed changes

goetas mentioned this pull request Sep 6, 2019

Extract RateLimitAnnotationListener logic into service classes jaytaph/RateLimitBundle#86

Open

amanto reviewed Sep 10, 2019

View reviewed changes

Limit manager as a service #2

Are you sure you want to change the base?

Limit manager as a service #2

Uh oh!

Conversation

goetas commented Sep 6, 2019

Uh oh!

goetas left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

amanto left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

goetas left a comment •

edited

Loading