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()); + } }