From c807d5c34fbe0482aa982b619a81fea7477e6add Mon Sep 17 00:00:00 2001 From: Ruud Kamphuis Date: Mon, 10 Jun 2024 11:41:22 +0200 Subject: [PATCH] Remove redundant lines in ReferenceExecutor Somehow this was added in #1548 but I fail to see why. Let's remove it. This might be the cause for the performance degradation reported in #1548. Add property `unaliasedPath` to `ResolveInfo` --- docs/class-reference.md | 33 +++++++-- src/Error/Error.php | 45 ++++++++++-- src/Executor/ReferenceExecutor.php | 109 ++++++++++++++++++++-------- src/Type/Definition/ResolveInfo.php | 25 +++++-- tests/Type/ResolveInfoTest.php | 73 +++++++++++++++++++ 5 files changed, 235 insertions(+), 50 deletions(-) diff --git a/docs/class-reference.md b/docs/class-reference.md index 5f0f11c4c..35f8d46a1 100644 --- a/docs/class-reference.md +++ b/docs/class-reference.md @@ -308,7 +308,7 @@ Passed as 4th argument to every field resolver. See [docs on field resolving (da @phpstan-import-type QueryPlanOptions from QueryPlan -@phpstan-type Path array +@phpstan-type Path list ### GraphQL\Type\Definition\ResolveInfo Props @@ -351,16 +351,27 @@ public $fieldNodes; public $parentType; /** - * Path to this field from the very root value. + * Path to this field from the very root value. When fields are aliased, the path includes aliases. * * @api * - * @var array + * @var list * * @phpstan-var Path */ public $path; +/** + * Path to this field from the very root value. This will never include aliases. + * + * @api + * + * @var list + * + * @phpstan-var Path + */ +public $unaliasedPath; + /** * Instance of a schema used for execution. * @@ -1730,15 +1741,27 @@ function getLocations(): array ```php /** * Returns an array describing the path from the root value to the field which produced this error. - * Only included for execution errors. + * Only included for execution errors. When fields are aliased, the path includes aliases. * - * @return array|null + * @return list|null * * @api */ function getPath(): ?array ``` +```php +/** + * Returns an array describing the path from the root value to the field which produced this error. + * Only included for execution errors. This will never include aliases. + * + * @return list|null + * + * @api + */ +function getUnaliasedPath(): ?array +``` + ## GraphQL\Error\Warning Encapsulates warnings produced by the library. diff --git a/src/Error/Error.php b/src/Error/Error.php index f30bf4fc9..d644ed103 100644 --- a/src/Error/Error.php +++ b/src/Error/Error.php @@ -34,11 +34,21 @@ class Error extends \Exception implements \JsonSerializable, ClientAware, Provid /** * An array describing the JSON-path into the execution response which * corresponds to this error. Only included for errors during execution. + * When fields are aliased, the path includes aliases. * - * @var array|null + * @var list|null */ public ?array $path; + /** + * An array describing the JSON-path into the execution response which + * corresponds to this error. Only included for errors during execution. + * This will never include aliases. + * + * @var list|null + */ + public ?array $unaliasedPath; + /** * An array of GraphQL AST Nodes corresponding to this error. * @@ -65,8 +75,9 @@ class Error extends \Exception implements \JsonSerializable, ClientAware, Provid /** * @param iterable|Node|null $nodes * @param array|null $positions - * @param array|null $path + * @param list|null $path * @param array|null $extensions + * @param list|null $unaliasedPath */ public function __construct( string $message = '', @@ -75,7 +86,8 @@ public function __construct( ?array $positions = null, ?array $path = null, ?\Throwable $previous = null, - ?array $extensions = null + ?array $extensions = null, + ?array $unaliasedPath = null ) { parent::__construct($message, 0, $previous); @@ -93,6 +105,7 @@ public function __construct( $this->source = $source; $this->positions = $positions; $this->path = $path; + $this->unaliasedPath = $unaliasedPath; if (\is_array($extensions) && $extensions !== []) { $this->extensions = $extensions; @@ -114,9 +127,10 @@ public function __construct( * * @param mixed $error * @param iterable|Node|null $nodes - * @param array|null $path + * @param list|null $path + * @param list|null $unaliasedPath */ - public static function createLocatedError($error, $nodes = null, ?array $path = null): Error + public static function createLocatedError($error, $nodes = null, ?array $path = null, ?array $unaliasedPath = null): Error { if ($error instanceof self) { if ($error->isLocated()) { @@ -125,6 +139,7 @@ public static function createLocatedError($error, $nodes = null, ?array $path = $nodes ??= $error->getNodes(); $path ??= $error->getPath(); + $unaliasedPath ??= $error->getUnaliasedPath(); } $source = null; @@ -159,7 +174,8 @@ public static function createLocatedError($error, $nodes = null, ?array $path = $positions, $path, $originalError, - $extensions + $extensions, + $unaliasedPath ); } @@ -251,9 +267,9 @@ public function getNodes(): ?array /** * Returns an array describing the path from the root value to the field which produced this error. - * Only included for execution errors. + * Only included for execution errors. When fields are aliased, the path includes aliases. * - * @return array|null + * @return list|null * * @api */ @@ -262,6 +278,19 @@ public function getPath(): ?array return $this->path; } + /** + * Returns an array describing the path from the root value to the field which produced this error. + * Only included for execution errors. This will never include aliases. + * + * @return list|null + * + * @api + */ + public function getUnaliasedPath(): ?array + { + return $this->unaliasedPath; + } + /** @return array|null */ public function getExtensions(): ?array { diff --git a/src/Executor/ReferenceExecutor.php b/src/Executor/ReferenceExecutor.php index 434047a96..a5de08c68 100644 --- a/src/Executor/ReferenceExecutor.php +++ b/src/Executor/ReferenceExecutor.php @@ -287,6 +287,7 @@ protected function executeOperation(OperationDefinitionNode $operation, $rootVal $type = $this->getOperationRootType($this->exeContext->schema, $operation); $fields = $this->collectFields($type, $operation->selectionSet, new \ArrayObject(), new \ArrayObject()); $path = []; + $unaliasedPath = []; // Errors from sub-fields of a NonNull type may propagate to the top level, // at which point we still log the error and null the parent field, which // in this case is the entire response. @@ -294,8 +295,8 @@ protected function executeOperation(OperationDefinitionNode $operation, $rootVal // Similar to completeValueCatchingError. try { $result = $operation->operation === 'mutation' - ? $this->executeFieldsSerially($type, $rootValue, $path, $fields, $this->exeContext->contextValue) - : $this->executeFields($type, $rootValue, $path, $fields, $this->exeContext->contextValue); + ? $this->executeFieldsSerially($type, $rootValue, $path, $unaliasedPath, $fields, $this->exeContext->contextValue) + : $this->executeFields($type, $rootValue, $path, $unaliasedPath, $fields, $this->exeContext->contextValue); $promise = $this->getPromise($result); if ($promise !== null) { @@ -521,24 +522,32 @@ protected function doesFragmentConditionMatch(Node $fragment, ObjectType $type): * Implements the "Evaluating selection sets" section of the spec for "write" mode. * * @param mixed $rootValue - * @param array $path + * @param list $path + * @param list $unaliasedPath * @param mixed $contextValue * * @phpstan-param Fields $fields * * @return array|Promise|\stdClass */ - protected function executeFieldsSerially(ObjectType $parentType, $rootValue, array $path, \ArrayObject $fields, $contextValue) + protected function executeFieldsSerially(ObjectType $parentType, $rootValue, array $path, array $unaliasedPath, \ArrayObject $fields, $contextValue) { $result = $this->promiseReduce( \array_keys($fields->getArrayCopy()), - function ($results, $responseName) use ($contextValue, $path, $parentType, $rootValue, $fields) { + function ($results, $responseName) use ($contextValue, $path, $unaliasedPath, $parentType, $rootValue, $fields) { $fieldNodes = $fields[$responseName]; assert($fieldNodes instanceof \ArrayObject, 'The keys of $fields populate $responseName'); $fieldPath = $path; $fieldPath[] = $responseName; - $result = $this->resolveField($parentType, $rootValue, $fieldNodes, $fieldPath, $this->maybeScopeContext($contextValue)); + + $fieldNode = $fieldNodes[0]; + assert($fieldNode instanceof FieldNode, '$fieldNodes is non-empty'); + + $fieldUnaliasedPath = $unaliasedPath; + $fieldUnaliasedPath[] = $fieldNode->name->value; + + $result = $this->resolveField($parentType, $rootValue, $fieldNodes, $fieldPath, $fieldUnaliasedPath, $this->maybeScopeContext($contextValue)); if ($result === static::$UNDEFINED) { return $results; } @@ -577,10 +586,12 @@ function ($results, $responseName) use ($contextValue, $path, $parentType, $root * serialize scalars, or execute the sub-selection-set for objects. * * @param mixed $rootValue - * @param array $path + * @param list $path + * @param list $unaliasedPath * @param mixed $contextValue * * @phpstan-param Path $path + * @phpstan-param Path $unaliasedPath * * @param \ArrayObject $fieldNodes * @@ -589,7 +600,7 @@ function ($results, $responseName) use ($contextValue, $path, $parentType, $root * * @return array|\Throwable|mixed|null */ - protected function resolveField(ObjectType $parentType, $rootValue, \ArrayObject $fieldNodes, array $path, $contextValue) + protected function resolveField(ObjectType $parentType, $rootValue, \ArrayObject $fieldNodes, array $path, array $unaliasedPath, $contextValue) { $exeContext = $this->exeContext; $fieldNode = $fieldNodes[0]; @@ -616,7 +627,8 @@ protected function resolveField(ObjectType $parentType, $rootValue, \ArrayObject $exeContext->fragments, $exeContext->rootValue, $exeContext->operation, - $exeContext->variableValues + $exeContext->variableValues, + $unaliasedPath ); if ($fieldDef->resolveFn !== null) { $resolveFn = $fieldDef->resolveFn; @@ -642,6 +654,7 @@ protected function resolveField(ObjectType $parentType, $rootValue, \ArrayObject $fieldNodes, $info, $path, + $unaliasedPath, $result, $contextValue ); @@ -722,10 +735,12 @@ protected function resolveFieldValueOrError( * in the execution context. * * @param \ArrayObject $fieldNodes - * @param array $path + * @param list $path + * @param list $unaliasedPath * @param mixed $contextValue * * @phpstan-param Path $path + * @phpstan-param Path $unaliasedPath * * @param mixed $result * @@ -738,6 +753,7 @@ protected function completeValueCatchingError( \ArrayObject $fieldNodes, ResolveInfo $info, array $path, + array $unaliasedPath, $result, $contextValue ) { @@ -746,23 +762,23 @@ protected function completeValueCatchingError( try { $promise = $this->getPromise($result); if ($promise !== null) { - $completed = $promise->then(function (&$resolved) use ($contextValue, $returnType, $fieldNodes, $info, $path) { - return $this->completeValue($returnType, $fieldNodes, $info, $path, $resolved, $contextValue); + $completed = $promise->then(function (&$resolved) use ($contextValue, $returnType, $fieldNodes, $info, $path, $unaliasedPath) { + return $this->completeValue($returnType, $fieldNodes, $info, $path, $unaliasedPath, $resolved, $contextValue); }); } else { - $completed = $this->completeValue($returnType, $fieldNodes, $info, $path, $result, $contextValue); + $completed = $this->completeValue($returnType, $fieldNodes, $info, $path, $unaliasedPath, $result, $contextValue); } $promise = $this->getPromise($completed); if ($promise !== null) { - return $promise->then(null, function ($error) use ($fieldNodes, $path, $returnType): void { - $this->handleFieldError($error, $fieldNodes, $path, $returnType); + return $promise->then(null, function ($error) use ($fieldNodes, $path, $unaliasedPath, $returnType): void { + $this->handleFieldError($error, $fieldNodes, $path, $unaliasedPath, $returnType); }); } return $completed; } catch (\Throwable $err) { - $this->handleFieldError($err, $fieldNodes, $path, $returnType); + $this->handleFieldError($err, $fieldNodes, $path, $unaliasedPath, $returnType); return null; } @@ -771,16 +787,18 @@ protected function completeValueCatchingError( /** * @param mixed $rawError * @param \ArrayObject $fieldNodes - * @param array $path + * @param list $path + * @param list $unaliasedPath * * @throws Error */ - protected function handleFieldError($rawError, \ArrayObject $fieldNodes, array $path, Type $returnType): void + protected function handleFieldError($rawError, \ArrayObject $fieldNodes, array $path, array $unaliasedPath, Type $returnType): void { $error = Error::createLocatedError( $rawError, $fieldNodes, - $path + $path, + $unaliasedPath ); // If the field type is non-nullable, then it is resolved without any @@ -816,7 +834,8 @@ protected function handleFieldError($rawError, \ArrayObject $fieldNodes, array $ * value by evaluating all sub-selections. * * @param \ArrayObject $fieldNodes - * @param array $path + * @param list $path + * @param list $unaliasedPath * @param mixed $result * @param mixed $contextValue * @@ -830,6 +849,7 @@ protected function completeValue( \ArrayObject $fieldNodes, ResolveInfo $info, array $path, + array $unaliasedPath, &$result, $contextValue ) { @@ -846,6 +866,7 @@ protected function completeValue( $fieldNodes, $info, $path, + $unaliasedPath, $result, $contextValue ); @@ -868,7 +889,7 @@ protected function completeValue( throw new InvariantViolation("Expected field {$info->parentType}.{$info->fieldName} to return iterable, but got: {$resultType}."); } - return $this->completeListValue($returnType, $fieldNodes, $info, $path, $result, $contextValue); + return $this->completeListValue($returnType, $fieldNodes, $info, $path, $unaliasedPath, $result, $contextValue); } assert($returnType instanceof NamedType, 'Wrapping types should return early'); @@ -885,12 +906,12 @@ protected function completeValue( } if ($returnType instanceof AbstractType) { - return $this->completeAbstractValue($returnType, $fieldNodes, $info, $path, $result, $contextValue); + return $this->completeAbstractValue($returnType, $fieldNodes, $info, $path, $unaliasedPath, $result, $contextValue); } // Field type must be and Object, Interface or Union and expect sub-selections. if ($returnType instanceof ObjectType) { - return $this->completeObjectValue($returnType, $fieldNodes, $info, $path, $result, $contextValue); + return $this->completeObjectValue($returnType, $fieldNodes, $info, $path, $unaliasedPath, $result, $contextValue); } $safeReturnType = Utils::printSafe($returnType); @@ -958,6 +979,7 @@ function ($previous, $value) use ($callback) { * @param ListOfType $returnType * @param \ArrayObject $fieldNodes * @param list $path + * @param list $unaliasedPath * @param iterable $results * @param mixed $contextValue * @@ -970,6 +992,7 @@ protected function completeListValue( \ArrayObject $fieldNodes, ResolveInfo $info, array $path, + array $unaliasedPath, iterable &$results, $contextValue ) { @@ -979,10 +1002,13 @@ protected function completeListValue( $containsPromise = false; $completedItems = []; foreach ($results as $item) { - $fieldPath = [...$path, $i++]; + $fieldPath = [...$path, $i]; $info->path = $fieldPath; + $unaliasedPath = [...$unaliasedPath, $i]; + $info->unaliasedPath = $unaliasedPath; + ++$i; - $completedItem = $this->completeValueCatchingError($itemType, $fieldNodes, $info, $fieldPath, $item, $contextValue); + $completedItem = $this->completeValueCatchingError($itemType, $fieldNodes, $info, $fieldPath, $unaliasedPath, $item, $contextValue); if (! $containsPromise && $this->getPromise($completedItem) !== null) { $containsPromise = true; @@ -1026,7 +1052,8 @@ protected function completeLeafValue(LeafType $returnType, &$result) * * @param AbstractType&Type $returnType * @param \ArrayObject $fieldNodes - * @param array $path + * @param list $path + * @param list $unaliasedPath * @param array $result * @param mixed $contextValue * @@ -1041,6 +1068,7 @@ protected function completeAbstractValue( \ArrayObject $fieldNodes, ResolveInfo $info, array $path, + array $unaliasedPath, &$result, $contextValue ) { @@ -1066,6 +1094,7 @@ protected function completeAbstractValue( $fieldNodes, $info, $path, + $unaliasedPath, $result, $contextValue )); @@ -1081,6 +1110,7 @@ protected function completeAbstractValue( $fieldNodes, $info, $path, + $unaliasedPath, $result, $contextValue ); @@ -1156,7 +1186,8 @@ protected function defaultTypeResolver($value, $contextValue, ResolveInfo $info, * Complete an Object value by executing all sub-selections. * * @param \ArrayObject $fieldNodes - * @param array $path + * @param list $path + * @param list $unaliasedPath * @param mixed $result * @param mixed $contextValue * @@ -1170,6 +1201,7 @@ protected function completeObjectValue( \ArrayObject $fieldNodes, ResolveInfo $info, array $path, + array $unaliasedPath, &$result, $contextValue ) { @@ -1185,6 +1217,7 @@ protected function completeObjectValue( $returnType, $fieldNodes, $path, + $unaliasedPath, &$result ) { if (! $isTypeOfResult) { @@ -1195,6 +1228,7 @@ protected function completeObjectValue( $returnType, $fieldNodes, $path, + $unaliasedPath, $result, $contextValue ); @@ -1211,6 +1245,7 @@ protected function completeObjectValue( $returnType, $fieldNodes, $path, + $unaliasedPath, $result, $contextValue ); @@ -1235,7 +1270,8 @@ protected function invalidReturnTypeError( /** * @param \ArrayObject $fieldNodes - * @param array $path + * @param list $path + * @param list $unaliasedPath * @param mixed $result * @param mixed $contextValue * @@ -1248,12 +1284,13 @@ protected function collectAndExecuteSubfields( ObjectType $returnType, \ArrayObject $fieldNodes, array $path, + array $unaliasedPath, &$result, $contextValue ) { $subFieldNodes = $this->collectSubFields($returnType, $fieldNodes); - return $this->executeFields($returnType, $result, $path, $subFieldNodes, $contextValue); + return $this->executeFields($returnType, $result, $path, $unaliasedPath, $subFieldNodes, $contextValue); } /** @@ -1297,7 +1334,8 @@ protected function collectSubFields(ObjectType $returnType, \ArrayObject $fieldN * Implements the "Evaluating selection sets" section of the spec for "read" mode. * * @param mixed $rootValue - * @param array $path + * @param list $path + * @param list $unaliasedPath * @param mixed $contextValue * * @phpstan-param Fields $fields @@ -1307,14 +1345,21 @@ protected function collectSubFields(ObjectType $returnType, \ArrayObject $fieldN * * @return Promise|\stdClass|array */ - protected function executeFields(ObjectType $parentType, $rootValue, array $path, \ArrayObject $fields, $contextValue) + protected function executeFields(ObjectType $parentType, $rootValue, array $path, array $unaliasedPath, \ArrayObject $fields, $contextValue) { $containsPromise = false; $results = []; foreach ($fields as $responseName => $fieldNodes) { $fieldPath = $path; $fieldPath[] = $responseName; - $result = $this->resolveField($parentType, $rootValue, $fieldNodes, $fieldPath, $this->maybeScopeContext($contextValue)); + + $fieldNode = $fieldNodes[0]; + assert($fieldNode instanceof FieldNode, '$fieldNodes is non-empty'); + + $fieldUnaliasedPath = $unaliasedPath; + $fieldUnaliasedPath[] = $fieldNode->name->value; + + $result = $this->resolveField($parentType, $rootValue, $fieldNodes, $fieldPath, $fieldUnaliasedPath, $this->maybeScopeContext($contextValue)); if ($result === static::$UNDEFINED) { continue; } diff --git a/src/Type/Definition/ResolveInfo.php b/src/Type/Definition/ResolveInfo.php index 1d3ea8a00..dcae8eecf 100644 --- a/src/Type/Definition/ResolveInfo.php +++ b/src/Type/Definition/ResolveInfo.php @@ -19,7 +19,7 @@ * * @phpstan-import-type QueryPlanOptions from QueryPlan * - * @phpstan-type Path array + * @phpstan-type Path list */ class ResolveInfo { @@ -61,16 +61,27 @@ class ResolveInfo public ObjectType $parentType; /** - * Path to this field from the very root value. + * Path to this field from the very root value. When fields are aliased, the path includes aliases. * * @api * - * @var array + * @var list * * @phpstan-var Path */ public array $path; + /** + * Path to this field from the very root value. This will never include aliases. + * + * @api + * + * @var list + * + * @phpstan-var Path + */ + public array $unaliasedPath; + /** * Instance of a schema used for execution. * @@ -114,9 +125,11 @@ class ResolveInfo /** * @param \ArrayObject $fieldNodes - * @param array $path + * @param list $path + * @param list $unaliasedPath * * @phpstan-param Path $path + * @phpstan-param Path $unaliasedPath * * @param array $fragments * @param mixed|null $rootValue @@ -131,7 +144,8 @@ public function __construct( array $fragments, $rootValue, OperationDefinitionNode $operation, - array $variableValues + array $variableValues, + array $unaliasedPath = [] ) { $this->fieldDefinition = $fieldDefinition; $this->fieldName = $fieldDefinition->name; @@ -139,6 +153,7 @@ public function __construct( $this->fieldNodes = $fieldNodes; $this->parentType = $parentType; $this->path = $path; + $this->unaliasedPath = $unaliasedPath; $this->schema = $schema; $this->fragments = $fragments; $this->rootValue = $rootValue; diff --git a/tests/Type/ResolveInfoTest.php b/tests/Type/ResolveInfoTest.php index e640354c4..a62033bfc 100644 --- a/tests/Type/ResolveInfoTest.php +++ b/tests/Type/ResolveInfoTest.php @@ -445,4 +445,77 @@ public function testDeepFieldSelectionOnDuplicatedFields(): void self::assertEquals(['data' => ['level1' => null]], $result); self::assertEquals($expectedDeepSelection, $actualDeepSelection); } + + public function testPathAndUnaliasedPath(): void + { + $level2 = new ObjectType([ + 'name' => 'level2', + 'fields' => [ + 'scalar1' => [ + 'type' => Type::string(), + 'resolve' => static function ($value, array $args, $context, ResolveInfo $info) { + return 'path: ' . implode('.', $info->path) . ', unaliasedPath: ' . implode('.', $info->unaliasedPath); + }, + ], + 'scalar2' => [ + 'type' => Type::string(), + 'resolve' => static function ($value, array $args, $context, ResolveInfo $info) { + return 'path: ' . implode('.', $info->path) . ', unaliasedPath: ' . implode('.', $info->unaliasedPath); + }, + ], + ], + ]); + $level1 = new ObjectType([ + 'name' => 'level1', + 'fields' => [ + 'level2' => [ + 'type' => $level2, + 'resolve' => function () { + return true; + }, + ], + ], + ]); + + $query = new ObjectType([ + 'name' => 'Query', + 'fields' => [ + 'level1' => [ + 'type' => $level1, + 'resolve' => function () { + return true; + }, + ], + ], + ]); + + $result = GraphQL::executeQuery( + new Schema(['query' => $query]), + <<toArray(); + + self::assertEquals([ + 'data' => [ + 'level1' => [ + 'level2' => [ + 'scalar1' => 'path: level1.level2.scalar1, unaliasedPath: level1.level2.scalar1', + ], + 'level1000' => [ + 'scalar2' => 'path: level1.level1000.scalar2, unaliasedPath: level1.level2.scalar2', + ], + ], + ], + ], $result); + } }