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..2228d066d1 --- /dev/null +++ b/src/Validator/Rules/QueryComplexity.php @@ -0,0 +1,81 @@ +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; + } +} 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..81be29693c 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. + ([#839](https://github.com/thecodingmachine/graphqlite/pull/839)) @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.