Remove Request from the ThrottleDirective constructor (#2511)

nuwave · Feb 19, 2024 · eed725d · eed725d
1 parent 5b8c75f
commit eed725d
Show file tree

Hide file tree

Showing 4 changed files with 105 additions and 53 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -9,6 +9,12 @@ You can find and compare releases at the [GitHub release page](https://github.co
 
 ## Unreleased
 
+## v6.33.2
+
+### Fixed
+
+- Fix `ThrottleDirective` to not reuse the first `Request` it had encountered https://github.com/nuwave/lighthouse/pull/2511
+
 ## v6.33.1
 
 ### Fixed

diff --git a/src/Schema/Directives/ThrottleDirective.php b/src/Schema/Directives/ThrottleDirective.php
@@ -7,7 +7,6 @@
 use GraphQL\Language\AST\ObjectTypeDefinitionNode;
 use Illuminate\Cache\RateLimiter;
 use Illuminate\Cache\RateLimiting\Unlimited;
-use Illuminate\Http\Request;
 use Illuminate\Support\Arr;
 use Nuwave\Lighthouse\Exceptions\DefinitionException;
 use Nuwave\Lighthouse\Exceptions\RateLimitException;
@@ -23,7 +22,6 @@ class ThrottleDirective extends BaseDirective implements FieldMiddleware, FieldM
 {
     public function __construct(
         protected RateLimiter $limiter,
-        protected Request $request,
     ) {}
 
     public static function definition(): string
@@ -58,45 +56,42 @@ public static function definition(): string
 
     public function handleField(FieldValue $fieldValue): void
     {
-        /** @var array<int, array{key: string, maxAttempts: int, decayMinutes: float}> $limits */
-        $limits = [];
-
         $name = $this->directiveArgValue('name');
-        if ($name !== null) {
-            // @phpstan-ignore-next-line limiter() can actually return null, some Laravel versions lie
-            $limiter = $this->limiter->limiter($name)
-                ?? throw new DefinitionException("Named limiter {$name} not found.");
-
-            $limiterResponse = $limiter($this->request);
-            if ($limiterResponse instanceof Unlimited) {
-                return;
-            }
+        $limiter = $name !== null ? $this->limiter->limiter($name) : null;
 
-            if ($limiterResponse instanceof Response) {
-                throw new DefinitionException("Expected named limiter {$name} to return an array, got instance of " . $limiterResponse::class);
-            }
+        $prefix = $this->directiveArgValue('prefix', '');
+        $maxAttempts = $this->directiveArgValue('maxAttempts', 60);
+        $decayMinutes = $this->directiveArgValue('decayMinutes', 1.0);
 
-            foreach (Arr::wrap($limiterResponse) as $limit) {
-                $limits[] = [
-                    'key' => sha1($name . $limit->key),
-                    'maxAttempts' => $limit->maxAttempts,
-                    'decayMinutes' => $limit->decayMinutes,
-                ];
+        $fieldValue->wrapResolver(fn (callable $resolver): \Closure => function (mixed $root, array $args, GraphQLContext $context, ResolveInfo $resolveInfo) use ($resolver, $name, $limiter, $prefix, $maxAttempts, $decayMinutes): mixed {
+            $request = $context->request();
+            if ($request === null) {
+                return $resolver($root, $args, $context, $resolveInfo);
             }
-        } else {
-            $limits[] = [
-                'key' => sha1($this->directiveArgValue('prefix') . $this->request->ip()),
-                'maxAttempts' => $this->directiveArgValue('maxAttempts', 60),
-                'decayMinutes' => $this->directiveArgValue('decayMinutes', 1.0),
-            ];
-        }
 
-        $fieldValue->wrapResolver(fn (callable $resolver): \Closure => function (mixed $root, array $args, GraphQLContext $context, ResolveInfo $resolveInfo) use ($resolver, $limits) {
-            foreach ($limits as $limit) {
+            if ($limiter !== null) {
+                $limiterResponse = $limiter($request);
+                if ($limiterResponse instanceof Unlimited) {
+                    return $resolver($root, $args, $context, $resolveInfo);
+                }
+
+                if ($limiterResponse instanceof Response) {
+                    throw new DefinitionException("Expected named limiter {$name} to return an array, got instance of " . $limiterResponse::class);
+                }
+
+                foreach (Arr::wrap($limiterResponse) as $limit) {
+                    $this->handleLimit(
+                        sha1($name . $limit->key),
+                        $limit->maxAttempts,
+                        $limit->decayMinutes,
+                        "{$resolveInfo->parentType}.{$resolveInfo->fieldName}",
+                    );
+                }
+            } else {
                 $this->handleLimit(
-                    $limit['key'],
-                    $limit['maxAttempts'],
-                    $limit['decayMinutes'],
+                    sha1($prefix . $request->ip()),
+                    $maxAttempts,
+                    $decayMinutes,
                     "{$resolveInfo->parentType}.{$resolveInfo->fieldName}",
                 );
             }
@@ -109,7 +104,7 @@ public function manipulateFieldDefinition(DocumentAST &$documentAST, FieldDefini
     {
         $name = $this->directiveArgValue('name');
         if ($name !== null) {
-            // @phpstan-ignore-next-line $limiter may be null although it's not specified in limiter() PHPDoc
+            // @phpstan-ignore-next-line limiter() can actually return null, some Laravel versions lie
             $this->limiter->limiter($name)
                 ?? throw new DefinitionException("Named limiter {$name} is not found.");
         }

diff --git a/tests/Integration/Schema/Directives/ThrottleDirectiveTest.php b/tests/Integration/Schema/Directives/ThrottleDirectiveTest.php
@@ -2,6 +2,7 @@
 
 namespace Tests\Integration\Schema\Directives;
 
+use Faker\Factory;
 use Illuminate\Cache\RateLimiter;
 use Illuminate\Cache\RateLimiting\Limit;
 use Illuminate\Http\Response;
@@ -58,27 +59,25 @@ public function testNamedLimiter(): void
         }
         ';
 
+        $query = /** @lang GraphQL */ '
+        {
+            foo
+        }
+        ';
+
         $rateLimiter = $this->app->make(RateLimiter::class);
         $rateLimiter->for(
             'test',
             static fn (): Limit => Limit::perMinute(1),
         );
 
-        $this->graphQL(/** @lang GraphQL */ '
-        {
-            foo
-        }
-        ')->assertJson([
+        $this->graphQL($query)->assertJson([
             'data' => [
                 'foo' => Foo::THE_ANSWER,
             ],
         ]);
 
-        $this->graphQL(/** @lang GraphQL */ '
-        {
-            foo
-        }
-        ')->assertGraphQLError(
+        $this->graphQL($query)->assertGraphQLError(
             new RateLimitException('Query.foo'),
         );
     }
@@ -91,22 +90,30 @@ public function testInlineLimiter(): void
         }
         ';
 
-        $this->graphQL(/** @lang GraphQL */ '
+        $query = /** @lang GraphQL */ '
         {
             foo
         }
-        ')->assertJson([
+        ';
+
+        $faker = Factory::create()->unique();
+        $ip = $faker->ipv4;
+        $ip2 = $faker->ipv4;
+
+        $this->graphQL($query, [], [], ['REMOTE_ADDR' => $ip])->assertJson([
             'data' => [
                 'foo' => Foo::THE_ANSWER,
             ],
         ]);
 
-        $this->graphQL(/** @lang GraphQL */ '
-        {
-            foo
-        }
-        ')->assertGraphQLError(
+        $this->graphQL($query, [], [], ['REMOTE_ADDR' => $ip])->assertGraphQLError(
             new RateLimitException('Query.foo'),
         );
+
+        $this->graphQL($query, [], [], ['REMOTE_ADDR' => $ip2])->assertJson([
+            'data' => [
+                'foo' => Foo::THE_ANSWER,
+            ],
+        ]);
     }
 }
diff --git a/tests/Unit/Schema/Directives/ThrottleDirectiveTest.php b/tests/Unit/Schema/Directives/ThrottleDirectiveTest.php
@@ -4,7 +4,11 @@
 
 use Illuminate\Cache\RateLimiter;
 use Illuminate\Cache\RateLimiting\Limit;
+use Illuminate\Http\Request;
+use Nuwave\Lighthouse\Support\Contracts\CreatesContext;
+use Nuwave\Lighthouse\Support\Contracts\GraphQLContext;
 use Tests\TestCase;
+use Tests\Unit\Execution\Fixtures\FooContext;
 use Tests\Utils\Queries\Foo;
 
 final class ThrottleDirectiveTest extends TestCase
@@ -88,4 +92,44 @@ public function testUnlimitedNamedLimiter(): void
             ],
         ]);
     }
+
+    public function testWithNullRequest(): void
+    {
+        $this->schema = /** @lang GraphQL */ '
+        type Query {
+            foo: Int @throttle(name: "test")
+        }
+        ';
+
+        $rateLimiter = $this->createMock(RateLimiter::class);
+        $rateLimiter->expects(self::atLeast(1))
+            ->method('limiter')
+            ->with('test')
+            ->willReturn(static fn (): array => [
+                Limit::perMinute(1),
+            ]);
+
+        $rateLimiter->expects(self::never())
+            ->method('hit');
+
+        // create a context with null request
+        $this->app->singleton(CreatesContext::class, static fn (): CreatesContext => new class() implements CreatesContext {
+            public function generate(?Request $request): GraphQLContext
+            {
+                return new FooContext();
+            }
+        });
+
+        $this->app->singleton(RateLimiter::class, static fn (): RateLimiter => $rateLimiter);
+
+        $this->graphQL(/** @lang GraphQL */ '
+        {
+            foo
+        }
+        ')->assertJson([
+            'data' => [
+                'foo' => Foo::THE_ANSWER,
+            ],
+        ]);
+    }
 }