From 1d2e49cebfb78641b611ff10e297279fb99cd3af Mon Sep 17 00:00:00 2001 From: Ruud Kamphuis Date: Tue, 25 Aug 2026 14:29:09 +0200 Subject: [PATCH] Report QueryComplexity variable coercion errors instead of throwing MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `DocumentValidator::validate()` is documented to return validation errors, and every other rule reports them through the validation context. But `QueryComplexity` coerces variable values (it needs them to evaluate @include/@skip and to build arguments for `complexityFn`) and threw a raw `Error` when coercion failed. Callers using `DocumentValidator` directly rather than `GraphQL::promiseToExecute()` — which sets the variables and wraps everything in try/catch — got an uncaught exception for something that is a plain input problem. Coercion errors now go to `$context->reportError()` and complexity analysis is abandoned for the rest of the document: without usable variable values the computed complexity is meaningless, so reporting a max-complexity error on top of it would be misleading, and calling a user-supplied `complexityFn` with empty arguments could throw. For the same reason `getQueryComplexity()` is reset to 0, so it cannot hand back the partial sum accumulated before coercion failed. This also improves the errors themselves. The old code concatenated the messages of all coercion errors into one `Error`, discarding the source locations; each error is now reported individually with the location of its variable definition, matching what the executor produces for the same bad input. While here, `buildFieldArguments()` reuses the per-document coercion cache instead of coercing all variables again for every field that has a `complexityFn`. Fixes #1967 --- CHANGELOG.md | 4 + src/Validator/Rules/QueryComplexity.php | 73 ++++++++--- tests/Validator/QueryComplexityTest.php | 166 ++++++++++++++++++++++++ 3 files changed, 228 insertions(+), 15 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 597124776..ab7f8929a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,6 +9,10 @@ You can find and compare releases at the [GitHub release page](https://github.co ## Unreleased +### Fixed + +- Report variable coercion errors from `QueryComplexity` through the validation context instead of throwing them out of `DocumentValidator::validate()` https://github.com/webonyx/graphql-php/issues/1967 + ## v15.37.2 ### Changed diff --git a/src/Validator/Rules/QueryComplexity.php b/src/Validator/Rules/QueryComplexity.php index 231adc500..62dbe31a6 100644 --- a/src/Validator/Rules/QueryComplexity.php +++ b/src/Validator/Rules/QueryComplexity.php @@ -45,6 +45,13 @@ class QueryComplexity extends QuerySecurityRule /** @var array|null Lazily coerced variable values; reset per document. */ private ?array $coercedVariableValues = null; + /** + * Did variable coercion fail for this document? + * + * The errors have been reported to the context, so complexity analysis is abandoned. + */ + private bool $variableCoercionFailed = false; + /** @throws \InvalidArgumentException */ public function __construct(int $maxQueryComplexity) { @@ -58,6 +65,7 @@ public function getVisitor(QueryValidationContext $context): array $this->variableDefs = new NodeList([]); $this->fieldNodeAndDefs = new \ArrayObject(); $this->coercedVariableValues = null; + $this->variableCoercionFailed = false; return $this->invokeIfNeeded( $context, @@ -95,6 +103,16 @@ public function getVisitor(QueryValidationContext $context): array $this->queryComplexity = $this->fieldComplexity($definition->selectionSet); + // Without usable variable values the computed complexity is + // meaningless: it is a partial sum of whatever was visited + // before coercion failed. Reset it so `getQueryComplexity()` + // does not expose it. The coercion errors were reported. + if ($this->variableCoercionFailed) { + $this->queryComplexity = 0; + + return; + } + if ($this->queryComplexity > $this->maxQueryComplexity) { $context->reportError( new Error(static::maxQueryComplexityErrorMessage( @@ -118,6 +136,10 @@ protected function fieldComplexity(SelectionSetNode $selectionSet): int $complexity = 0; foreach ($selectionSet->selections as $selection) { + if ($this->variableCoercionFailed) { + return 0; + } + $complexity += $this->nodeComplexity($selection); } @@ -146,6 +168,10 @@ protected function nodeComplexity(SelectionNode $node): int if ($fieldDef instanceof FieldDefinition && $fieldDef->complexityFn !== null) { $fieldArguments = $this->buildFieldArguments($node); + if ($this->variableCoercionFailed) { + return 0; + } + return ($fieldDef->complexityFn)($childrenComplexity, $fieldArguments); } @@ -187,10 +213,15 @@ protected function directiveExcludesField(FieldNode $node): bool { foreach ($node->directives as $directiveNode) { if ($directiveNode->name->value === Directive::INCLUDE_NAME) { + $variableValues = $this->getCoercedVariableValues(); + if ($variableValues === null) { + return false; + } + $includeArguments = Values::getArgumentValues( Directive::includeDirective(), $directiveNode, - $this->getCoercedVariableValues() + $variableValues ); assert(is_bool($includeArguments['if']), 'ensured by query validation'); @@ -200,10 +231,15 @@ protected function directiveExcludesField(FieldNode $node): bool } if ($directiveNode->name->value === Directive::SKIP_NAME) { + $variableValues = $this->getCoercedVariableValues(); + if ($variableValues === null) { + return false; + } + $skipArguments = Values::getArgumentValues( Directive::skipDirective(), $directiveNode, - $this->getCoercedVariableValues() + $variableValues ); assert(is_bool($skipArguments['if']), 'ensured by query validation'); @@ -219,13 +255,20 @@ protected function directiveExcludesField(FieldNode $node): bool /** * Coerce variable values once per document and cache them. * + * Returns `null` when coercion failed, in which case the coercion errors have been + * reported to the validation context. + * * @throws \Exception * @throws InvariantViolation * - * @return array + * @return array|null */ - private function getCoercedVariableValues(): array + private function getCoercedVariableValues(): ?array { + if ($this->variableCoercionFailed) { + return null; + } + if ($this->coercedVariableValues !== null) { return $this->coercedVariableValues; } @@ -236,7 +279,13 @@ private function getCoercedVariableValues(): array $this->getRawVariableValues() ); if ($errors !== null && $errors !== []) { - throw new Error(implode("\n\n", array_map(static fn (Error $error): string => $error->getMessage(), $errors))); + $this->variableCoercionFailed = true; + + foreach ($errors as $error) { + $this->context->reportError($error); + } + + return null; } return $this->coercedVariableValues = $variableValues ?? []; @@ -256,27 +305,21 @@ public function setRawVariableValues(?array $rawVariableValues = null): void /** * @throws \Exception - * @throws Error + * @throws InvariantViolation * * @return array */ protected function buildFieldArguments(FieldNode $node): array { - $rawVariableValues = $this->getRawVariableValues(); $fieldDef = $this->fieldDefinition($node); /** @var array $args */ $args = []; if ($fieldDef instanceof FieldDefinition) { - [$errors, $variableValues] = Values::getVariableValues( - $this->context->getSchema(), - $this->variableDefs, - $rawVariableValues - ); - - if (is_array($errors) && $errors !== []) { - throw new Error(implode("\n\n", array_map(static fn ($error) => $error->getMessage(), $errors))); + $variableValues = $this->getCoercedVariableValues(); + if ($variableValues === null) { + return $args; } $args = Values::getArgumentValues($fieldDef, $node, $variableValues); diff --git a/tests/Validator/QueryComplexityTest.php b/tests/Validator/QueryComplexityTest.php index db6050a01..659e8bd7a 100644 --- a/tests/Validator/QueryComplexityTest.php +++ b/tests/Validator/QueryComplexityTest.php @@ -5,6 +5,7 @@ use GraphQL\Error\Error; use GraphQL\Language\AST\NodeKind; use GraphQL\Language\Parser; +use GraphQL\Language\SourceLocation; use GraphQL\Type\Introspection; use GraphQL\Validator\DocumentValidator; use GraphQL\Validator\Rules\CustomValidationRule; @@ -331,4 +332,169 @@ public function testVariableCoercionIsCachedAcrossMultipleDirectives(): void // skipIt=false means nothing is skipped: complexity = human(1) + firstName(1) + dogs(10) + name(1) = 13 $this->assertDocumentValidators($query, 13, 14); } + + /** + * Variable coercion errors must be reported through the validation context rather + * than thrown, so that `DocumentValidator::validate()` returns them like any other + * validation error. + * + * @see https://github.com/webonyx/graphql-php/issues/1967 + * + * @dataProvider variableCoercionFailureProvider + * + * @param array $rawVariableValues + */ + public function testReportsVariableCoercionErrorsInsteadOfThrowing( + string $query, + array $rawVariableValues, + string $expectedMessage + ): void { + $rule = $this->getRule(1); + $rule->setRawVariableValues($rawVariableValues); + + $errors = DocumentValidator::validate( + QuerySecuritySchema::buildSchema(), + Parser::parse($query), + [$rule] + ); + + self::assertCount(1, $errors); + self::assertSame($expectedMessage, $errors[0]->getMessage()); + + // The complexity limit of 1 is not reported: without usable variable values the + // computed complexity is meaningless. + self::assertStringNotContainsString('Max query complexity', $errors[0]->getMessage()); + self::assertSame(0, $rule->getQueryComplexity()); + } + + /** @return iterable, string}> */ + public static function variableCoercionFailureProvider(): iterable + { + yield 'missing required variable used by @include' => [ + 'query MyQuery($withDogs: Boolean!) { human { dogs(name: "Root") @include(if: $withDogs) { name } } }', + [], + 'Variable "$withDogs" of required type "Boolean!" was not provided.', + ]; + + yield 'missing required variable used by @skip' => [ + 'query MyQuery($withoutDogs: Boolean!) { human { dogs(name: "Root") @skip(if: $withoutDogs) { name } } }', + [], + 'Variable "$withoutDogs" of required type "Boolean!" was not provided.', + ]; + + yield 'missing required variable used as a field argument' => [ + 'query MyQuery($dog: String!) { human { dogs(name: $dog) { name } } }', + [], + 'Variable "$dog" of required type "String!" was not provided.', + ]; + + yield 'invalid variable value used as a field argument' => [ + 'query MyQuery($dog: String!) { human { dogs(name: $dog) { name } } }', + ['dog' => 42], + 'Variable "$dog" got invalid value 42; String cannot represent a non string value: 42', + ]; + } + + /** Every failing variable is reported, not just the first. */ + public function testReportsAllVariableCoercionErrors(): void + { + $rule = $this->getRule(1); + $rule->setRawVariableValues([]); + + $errors = DocumentValidator::validate( + QuerySecuritySchema::buildSchema(), + Parser::parse('query MyQuery($withDogs: Boolean!, $dog: String!) { human { dogs(name: $dog) @include(if: $withDogs) { name } } }'), + [$rule] + ); + + self::assertSame( + [ + 'Variable "$withDogs" of required type "Boolean!" was not provided.', + 'Variable "$dog" of required type "String!" was not provided.', + ], + array_map(static fn (Error $error): string => $error->getMessage(), $errors) + ); + } + + /** + * Variable coercion errors carry the location of the offending variable definition, + * which the previously thrown error lost by concatenating messages. + */ + public function testVariableCoercionErrorsHaveLocations(): void + { + $rule = $this->getRule(1); + $rule->setRawVariableValues([]); + + $errors = DocumentValidator::validate( + QuerySecuritySchema::buildSchema(), + Parser::parse('query MyQuery($withDogs: Boolean!) { human { dogs(name: "Root") @include(if: $withDogs) { name } } }'), + [$rule] + ); + + self::assertCount(1, $errors); + self::assertEquals([new SourceLocation(1, 15)], $errors[0]->getLocations()); + } + + /** + * Coercion is only attempted once per document, even though the rule short-circuits + * the rest of the traversal after it fails. + */ + public function testVariableCoercionFailureIsReportedOncePerDocument(): void + { + $query = <<<'GRAPHQL' + query MyQuery($withoutDogs: Boolean!) { + human { + firstName @skip(if: $withoutDogs) + dogs(name: "Root") @skip(if: $withoutDogs) { name } + } + } + GRAPHQL; + + $rule = $this->getRule(1); + $rule->setRawVariableValues([]); + + $errors = DocumentValidator::validate( + QuerySecuritySchema::buildSchema(), + Parser::parse($query), + [$rule] + ); + + self::assertCount(1, $errors); + } + + /** + * A coercion failure leaves `fieldComplexity()` holding a partial sum of whatever + * was visited before it failed — here `human` is counted but `dogs` short-circuits + * to 0. That number must not reach `getQueryComplexity()`. + */ + public function testDoesNotExposePartialComplexityAfterVariableCoercionFailure(): void + { + $rule = $this->getRule(100); + $rule->setRawVariableValues([]); + + $errors = DocumentValidator::validate( + QuerySecuritySchema::buildSchema(), + Parser::parse('query MyQuery($dog: String!) { human { dogs(name: $dog) { name } } }'), + [$rule] + ); + + self::assertCount(1, $errors); + self::assertSame(0, $rule->getQueryComplexity()); + } + + /** A failed document must not leak its failure into the next one. */ + public function testVariableCoercionFailureIsResetBetweenDocuments(): void + { + $query = 'query MyQuery($withoutDogs: Boolean!) { human { dogs(name: "Root") @skip(if: $withoutDogs) { name } } }'; + $schema = QuerySecuritySchema::buildSchema(); + $ast = Parser::parse($query); + + $rule = $this->getRule(100); + $rule->setRawVariableValues([]); + self::assertCount(1, DocumentValidator::validate($schema, $ast, [$rule])); + + $rule->setRawVariableValues(['withoutDogs' => false]); + self::assertSame([], DocumentValidator::validate($schema, $ast, [$rule])); + self::assertSame(3, $rule->getQueryComplexity()); + } }