fix: leave invalid variables to the executor under limitQueryComplexity()

src/Http/Psr15GraphQLMiddlewareBuilder.php

@@ -11,7 +11,6 @@
 use GraphQL\Server\ServerConfig;
 use GraphQL\Type\Schema;
 use GraphQL\Validator\DocumentValidator;
-use GraphQL\Validator\Rules\QueryComplexity;
 use GraphQL\Validator\Rules\ValidationRule;
 use Laminas\Diactoros\ResponseFactory;
 use Laminas\Diactoros\StreamFactory;
@@ -24,6 +23,7 @@
 use TheCodingMachine\GraphQLite\GraphQLRuntimeException;
 use TheCodingMachine\GraphQLite\Server\PersistedQuery\CachePersistedQueryLoader;
 use TheCodingMachine\GraphQLite\Server\PersistedQuery\NotSupportedPersistedQueryLoader;
+use TheCodingMachine\GraphQLite\Validator\Rules\QueryComplexity;
 
 use function class_exists;
 use function is_callable;

src/Validator/Rules/QueryComplexity.php

@@ -0,0 +1,81 @@
+<?php
+
+declare(strict_types=1);
+
+namespace TheCodingMachine\GraphQLite\Validator\Rules;
+
+use GraphQL\Error\Error;
+use GraphQL\Executor\Values;
+use GraphQL\Language\AST\DocumentNode;
+use GraphQL\Language\AST\Node;
+use GraphQL\Language\AST\NodeKind;
+use GraphQL\Language\AST\OperationDefinitionNode;
+use GraphQL\Type\Schema;
+use GraphQL\Validator\QueryValidationContext;
+use GraphQL\Validator\Rules\QueryComplexity as WebonyxQueryComplexity;
+
+use function is_callable;
+
+/**
+ * Limits operation complexity, leaving invalid variables to the executor
+ *
+ * Pricing a field by its arguments coerces the operation's variables during validation, and webonyx
+ * throws a single error joining every coercion message, without locations. When the variables really
+ * fail coercion, this rule reports nothing, so the executor refuses them with one located error per
+ * variable. Any other error is rethrown, so it can't disable the limit.
+ */
+final class QueryComplexity extends WebonyxQueryComplexity
+{
+    /** @inheritDoc */
+    public function getVisitor(QueryValidationContext $context): array
+    {
+        $visitor = parent::getVisitor($context);
+        $documentVisitor = $visitor[NodeKind::DOCUMENT] ?? null;
+        if ($documentVisitor === null || is_callable($documentVisitor)) {
+            return $visitor;
+        }
+
+        $leave = $documentVisitor['leave'] ?? null;
+        if ($leave === null) {
+            return $visitor;
+        }
+
+        $documentVisitor['leave'] = function (Node $node, mixed ...$arguments) use ($leave, $context): void {
+            try {
+                $leave($node, ...$arguments);
+            } catch (Error $error) {
+                if (! $node instanceof DocumentNode || ! $this->variablesFailCoercion($context->getSchema(), $node)) {
+                    throw $error;
+                }
+            }
+        };
+        $visitor[NodeKind::DOCUMENT] = $documentVisitor;
+
+        return $visitor;
+    }
+
+    /**
+     * Whether the executor will refuse the variables, whichever operation of the document it runs
+     *
+     * The operation name isn't known during validation, so every operation's own variable
+     * definitions must fail coercion.
+     */
+    private function variablesFailCoercion(Schema $schema, DocumentNode $document): bool
+    {
+        $failing = false;
+        foreach ($document->definitions as $definition) {
+            if (! $definition instanceof OperationDefinitionNode) {
+                continue;
+            }
+
+            [$errors] = Values::getVariableValues($schema, $definition->variableDefinitions, $this->getRawVariableValues());
+            if ($errors === null) {
+                return false;
+            }
+
+            $failing = true;
+        }
+
+        return $failing;
+    }
+}

tests/Integration/QueryComplexityTest.php

@@ -8,6 +8,7 @@
 use GraphQL\Validator\Rules\QueryComplexity;
 use PHPUnit\Framework\Attributes\DataProvider;
 use TheCodingMachine\GraphQLite\Schema;
+use TheCodingMachine\GraphQLite\Validator\Rules\QueryComplexity as GraphQLiteQueryComplexity;
 
 class QueryComplexityTest extends IntegrationTestCase
 {
@@ -59,6 +60,34 @@ public function testExceedsAllowedQueryComplexity(): void
         $this->assertSame('Max query complexity should be 5 but got 60.', $result->toArray(DebugFlag::RETHROW_UNSAFE_EXCEPTIONS)['errors'][0]['message']);
     }
 
+    public function testInvalidVariableIsRefusedByTheExecutor(): void
+    {
+        $schema = $this->mainContainer->get(Schema::class);
+        assert($schema instanceof Schema);
+
+        $result = GraphQL::executeQuery(
+            $schema,
+            <<<'GRAPHQL'
+                query ($take: Int) {
+                    articles(take: $take) {
+                        title
+                    }
+                }
+            GRAPHQL,
+            variableValues: ['take' => 'many'],
+            validationRules: [
+                ...DocumentValidator::allRules(),
+                new GraphQLiteQueryComplexity(60),
+            ]
+        );
+
+        $errors = $result->toArray(DebugFlag::RETHROW_UNSAFE_EXCEPTIONS)['errors'];
+
+        $this->assertCount(1, $errors);
+        $this->assertSame('Variable "$take" got invalid value "many"; Int cannot represent non-integer value: "many"', $errors[0]['message']);
+        $this->assertSame([['line' => 1, 'column' => 12]], $errors[0]['locations']);
+    }
+
     #[DataProvider('calculatesCorrectQueryCostProvider')]
     public function testCalculatesCorrectQueryCost(int $expectedCost, string $query): void
     {

tests/Validator/Rules/QueryComplexityTest.php

@@ -0,0 +1,162 @@
+<?php
+
+declare(strict_types=1);
+
+namespace TheCodingMachine\GraphQLite\Validator\Rules;
+
+use GraphQL\Error\Error;
+use GraphQL\GraphQL;
+use GraphQL\Language\AST\Node;
+use GraphQL\Language\AST\StringValueNode;
+use GraphQL\Type\Definition\CustomScalarType;
+use GraphQL\Type\Definition\ObjectType;
+use GraphQL\Type\Definition\Type;
+use GraphQL\Type\Schema;
+use GraphQL\Validator\DocumentValidator;
+use PHPUnit\Framework\TestCase;
+
+use function is_string;
+use function preg_match;
+
+class QueryComplexityTest extends TestCase
+{
+    public function testInvalidVariableIsRefusedByTheExecutorWithItsLocation(): void
+    {
+        $result = $this->execute(
+            'query ($isbn: Isbn) { books(isbn: $isbn) { title } }',
+            ['isbn' => 'abc'],
+        )->toArray();
+
+        $this->assertNull($result['data'] ?? null);
+        $this->assertCount(1, $result['errors']);
+        $this->assertSame(
+            'Variable "$isbn" got invalid value "abc"; An ISBN has 13 digits.',
+            $result['errors'][0]['message'],
+        );
+        $this->assertSame([['line' => 1, 'column' => 8]], $result['errors'][0]['locations']);
+    }
+
+    public function testEachInvalidVariableIsRefusedSeparately(): void
+    {
+        $result = $this->execute(
+            'query ($first: Isbn, $second: Isbn) { a: books(isbn: $first) { title } b: books(isbn: $second) { title } }',
+            ['first' => 'abc', 'second' => 'def'],
+        )->toArray();
+
+        $this->assertNull($result['data'] ?? null);
+        $this->assertCount(2, $result['errors']);
+        $this->assertStringStartsWith('Variable "$first" got invalid value "abc"', $result['errors'][0]['message']);
+        $this->assertSame([['line' => 1, 'column' => 8]], $result['errors'][0]['locations']);
+        $this->assertStringStartsWith('Variable "$second" got invalid value "def"', $result['errors'][1]['message']);
+        $this->assertSame([['line' => 1, 'column' => 22]], $result['errors'][1]['locations']);
+    }
+
+    public function testValidOperationOverTheLimitIsRefused(): void
+    {
+        $result = $this->execute(
+            'query ($isbn: Isbn, $take: Int) { books(isbn: $isbn, take: $take) { title } }',
+            ['isbn' => '9780000000001', 'take' => 50],
+        )->toArray();
+
+        $this->assertNull($result['data'] ?? null);
+        $this->assertSame('Max query complexity should be 20 but got 100.', $result['errors'][0]['message']);
+    }
+
+    public function testValidOperationUnderTheLimitIsExecuted(): void
+    {
+        $result = $this->execute(
+            'query ($isbn: Isbn, $take: Int) { books(isbn: $isbn, take: $take) { title } }',
+            ['isbn' => '9780000000001', 'take' => 5],
+        )->toArray();
+
+        $this->assertArrayNotHasKey('errors', $result);
+        $this->assertSame(['books' => [['title' => 'Title']]], $result['data']);
+    }
+
+    public function testUnrelatedPricingErrorIsNotSwallowed(): void
+    {
+        $result = $this->execute('{ unpriceable }')->toArray();
+
+        $this->assertNull($result['data'] ?? null);
+        $this->assertSame('Pricing failed.', $result['errors'][0]['message']);
+    }
+
+    public function testInvalidVariableOfAnotherOperationKeepsTheLimit(): void
+    {
+        $result = $this->execute(
+            'query byIsbn($isbn: Isbn) { books(isbn: $isbn) { title } } query all { books { title } }',
+            ['isbn' => 'abc'],
+            'all',
+        )->toArray();
+
+        $this->assertNull($result['data'] ?? null);
+        $this->assertStringStartsWith('Variable "$isbn" got invalid value "abc"', $result['errors'][0]['message']);
+        $this->assertArrayNotHasKey('locations', $result['errors'][0]);
+    }
+
+    /** @param array<string, mixed> $variables */
+    private function execute(string $query, array $variables = [], string|null $operationName = null): \GraphQL\Executor\ExecutionResult
+    {
+        return GraphQL::executeQuery(
+            $this->createSchema(),
+            $query,
+            variableValues: $variables,
+            operationName: $operationName,
+            validationRules: [
+                ...DocumentValidator::allRules(),
+                new QueryComplexity(20),
+            ],
+        );
+    }
+
+    private function createSchema(): Schema
+    {
+        $isbn = new CustomScalarType([
+            'name' => 'Isbn',
+            'serialize' => static fn (mixed $value): mixed => $value,
+            'parseValue' => static function (mixed $value): string {
+                if (! is_string($value) || ! preg_match('/^\d{13}$/', $value)) {
+                    throw new Error('An ISBN has 13 digits.');
+                }
+
+                return $value;
+            },
+            'parseLiteral' => static function (Node $node): string {
+                if (! $node instanceof StringValueNode || ! preg_match('/^\d{13}$/', $node->value)) {
+                    throw new Error('An ISBN has 13 digits.');
+                }
+
+                return $node->value;
+            },
+        ]);
+
+        $book = new ObjectType([
+            'name' => 'Book',
+            'fields' => [
+                'title' => ['type' => Type::string(), 'resolve' => static fn (): string => 'Title'],
+            ],
+        ]);
+
+        return new Schema([
+            'query' => new ObjectType([
+                'name' => 'Query',
+                'fields' => [
+                    'books' => [
+                        'type' => Type::listOf($book),
+                        'args' => [
+                            'isbn' => ['type' => $isbn],
+                            'take' => ['type' => Type::int()],
+                        ],
+                        'complexity' => static fn (int $childrenComplexity, array $args): int => ($args['take'] ?? 1) * ($childrenComplexity + 1),
+                        'resolve' => static fn (): array => [[]],
+                    ],
+                    'unpriceable' => [
+                        'type' => Type::string(),
+                        'complexity' => static fn (): int => throw new Error('Pricing failed.'),
+                        'resolve' => static fn (): string => 'Resolved',
+                    ],
+                ],
+            ]),
+        ]);
+    }
+}

website/docs/CHANGELOG.md

@@ -65,6 +65,10 @@ sidebar_label: Changelog
 - [#793](https://github.com/thecodingmachine/graphqlite/pull/793) Argument `@param` descriptions no longer
   reach the schema when `SchemaFactory::setDocblockDescriptionsEnabled(false)` is set.
   ([#838](https://github.com/thecodingmachine/graphqlite/pull/838)) @oojacoboo
+- `Psr15GraphQLMiddlewareBuilder::limitQueryComplexity()` no longer turns invalid variables into a single joined
+  error without locations. Its rule, `TheCodingMachine\GraphQLite\Validator\Rules\QueryComplexity`, leaves them
+  to the executor, which refuses each variable with its own located error.
+  ([#839](https://github.com/thecodingmachine/graphqlite/pull/839)) @oojacoboo
 
 ## >8.0.0
 

website/docs/operation-complexity.md

@@ -232,5 +232,11 @@ $builder = new Psr15GraphQLMiddlewareBuilder($schema);
 $builder->limitQueryComplexity(1000);
 ```
 
+`limitQueryComplexity()` registers GraphQLite's `TheCodingMachine\GraphQLite\Validator\Rules\QueryComplexity`,
+a subclass of webonyx's `QueryComplexity` rule. Pricing fields by their arguments coerces the operation's variables
+during validation; when a variable is invalid, the operation is refused by the executor with one error per variable,
+including its location, rather than a single joined validation error. If you register the rule yourself with
+`addValidationRule()`, prefer this class over webonyx's.
+
 Beware that introspection queries would also be limited in complexity. A full introspection
 query sits at around `107` points, so we recommend a minimum of `150` for query complexity limit.