From 0a0c74cd84a542332197e9d1bbc7d62a6277f50d Mon Sep 17 00:00:00 2001 From: Jacob Thomason Date: Sun, 11 Oct 2026 02:05:10 -0400 Subject: [PATCH 1/3] fix: leave invalid variables to the executor under limitQueryComplexity() webonyx's QueryComplexity rule coerces the operation's variables while pricing fields by their arguments, and throws a single error joining every coercion message, without locations. That preempts the executor, which refuses the same variables with one located error per variable. limitQueryComplexity() now registers TheCodingMachine\GraphQLite\Validator\Rules\QueryComplexity, a subclass that wraps the parent's document visitor: when the variables really fail coercion it reports nothing and the executor refuses them; any other error is rethrown, so the limit holds. --- src/Http/Psr15GraphQLMiddlewareBuilder.php | 2 +- src/Validator/Rules/QueryComplexity.php | 79 +++++++++ tests/Integration/QueryComplexityTest.php | 29 ++++ tests/Validator/Rules/QueryComplexityTest.php | 162 ++++++++++++++++++ website/docs/CHANGELOG.md | 4 + website/docs/operation-complexity.md | 6 + 6 files changed, 281 insertions(+), 1 deletion(-) create mode 100644 src/Validator/Rules/QueryComplexity.php create mode 100644 tests/Validator/Rules/QueryComplexityTest.php diff --git a/src/Http/Psr15GraphQLMiddlewareBuilder.php b/src/Http/Psr15GraphQLMiddlewareBuilder.php index 90ace911ad..cc63729e0e 100644 --- a/src/Http/Psr15GraphQLMiddlewareBuilder.php +++ b/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; diff --git a/src/Validator/Rules/QueryComplexity.php b/src/Validator/Rules/QueryComplexity.php new file mode 100644 index 0000000000..4e8f86b73f --- /dev/null +++ b/src/Validator/Rules/QueryComplexity.php @@ -0,0 +1,79 @@ +variablesFailCoercion($context->getSchema(), $document)) { + return null; + } + + 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; + } +} diff --git a/tests/Integration/QueryComplexityTest.php b/tests/Integration/QueryComplexityTest.php index a8d9e9cf65..b12d6bd892 100644 --- a/tests/Integration/QueryComplexityTest.php +++ b/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 { diff --git a/tests/Validator/Rules/QueryComplexityTest.php b/tests/Validator/Rules/QueryComplexityTest.php new file mode 100644 index 0000000000..124bc4b763 --- /dev/null +++ b/tests/Validator/Rules/QueryComplexityTest.php @@ -0,0 +1,162 @@ +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 $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', + ], + ], + ]), + ]); + } +} diff --git a/website/docs/CHANGELOG.md b/website/docs/CHANGELOG.md index ab5c4d1502..a1bdea9c80 100644 --- a/website/docs/CHANGELOG.md +++ b/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. + ([#PENDING](https://github.com/thecodingmachine/graphqlite/pull/PENDING)) @oojacoboo ## >8.0.0 diff --git a/website/docs/operation-complexity.md b/website/docs/operation-complexity.md index 88902c88d9..3de3fd6592 100644 --- a/website/docs/operation-complexity.md +++ b/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. From 35a875c033ae2abb89d5486c8f965c05b29d1e41 Mon Sep 17 00:00:00 2001 From: Jacob Thomason Date: Sun, 11 Oct 2026 02:05:26 -0400 Subject: [PATCH 2/3] docs: link #839 in the CHANGELOG --- website/docs/CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/website/docs/CHANGELOG.md b/website/docs/CHANGELOG.md index a1bdea9c80..81be29693c 100644 --- a/website/docs/CHANGELOG.md +++ b/website/docs/CHANGELOG.md @@ -68,7 +68,7 @@ sidebar_label: Changelog - `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. - ([#PENDING](https://github.com/thecodingmachine/graphqlite/pull/PENDING)) @oojacoboo + ([#839](https://github.com/thecodingmachine/graphqlite/pull/839)) @oojacoboo ## >8.0.0 From c9df0b98d77aa57bfc99f4b10c4414204958e872 Mon Sep 17 00:00:00 2001 From: Jacob Thomason Date: Sun, 11 Oct 2026 02:31:07 -0400 Subject: [PATCH 3/3] fix: type the wrapped Document visitor for phpstan on lowest and latest webonyx --- src/Validator/Rules/QueryComplexity.php | 20 +++++++++++--------- 1 file changed, 11 insertions(+), 9 deletions(-) diff --git a/src/Validator/Rules/QueryComplexity.php b/src/Validator/Rules/QueryComplexity.php index 4e8f86b73f..2228d066d1 100644 --- a/src/Validator/Rules/QueryComplexity.php +++ b/src/Validator/Rules/QueryComplexity.php @@ -7,13 +7,13 @@ 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_array; use function is_callable; /** @@ -31,20 +31,22 @@ public function getVisitor(QueryValidationContext $context): array { $visitor = parent::getVisitor($context); $documentVisitor = $visitor[NodeKind::DOCUMENT] ?? null; - $leave = is_array($documentVisitor) ? $documentVisitor['leave'] ?? null : null; - if (! is_callable($leave)) { + if ($documentVisitor === null || is_callable($documentVisitor)) { return $visitor; } - $documentVisitor['leave'] = function (DocumentNode $document, mixed ...$arguments) use ($leave, $context): mixed { + $leave = $documentVisitor['leave'] ?? null; + if ($leave === null) { + return $visitor; + } + + $documentVisitor['leave'] = function (Node $node, mixed ...$arguments) use ($leave, $context): void { try { - return $leave($document, ...$arguments); + $leave($node, ...$arguments); } catch (Error $error) { - if ($this->variablesFailCoercion($context->getSchema(), $document)) { - return null; + if (! $node instanceof DocumentNode || ! $this->variablesFailCoercion($context->getSchema(), $node)) { + throw $error; } - - throw $error; } }; $visitor[NodeKind::DOCUMENT] = $documentVisitor;