From 34bd378c7ee59644148644b432977f7ea43b48e2 Mon Sep 17 00:00:00 2001 From: Vladimir Razuvaev Date: Tue, 4 Jul 2017 00:09:32 +0700 Subject: [PATCH] Refactored executor logic related to isTypeOf --- src/Executor/Executor.php | 167 ++++++++++++++++++-------------- tests/Executor/ExecutorTest.php | 7 +- tests/Type/ValidationTest.php | 10 +- 3 files changed, 105 insertions(+), 79 deletions(-) diff --git a/src/Executor/Executor.php b/src/Executor/Executor.php index 011e7cf..89ed1f3 100644 --- a/src/Executor/Executor.php +++ b/src/Executor/Executor.php @@ -968,81 +968,86 @@ class Executor $exeContext = $this->exeContext; $runtimeType = $returnType->resolveType($result, $exeContext->contextValue, $info); - if (null === $runtimeType) { - $runtimeType = self::defaultTypeResolver($result, $exeContext->contextValue, $info, $returnType); - } - if ($this->promises->isThenable($runtimeType)) { $runtimeType = $this->promises->convertThenable($runtimeType); Utils::invariant($runtimeType instanceof Promise); } + if (null === $runtimeType) { + $runtimeType = self::defaultTypeResolver($result, $exeContext->contextValue, $info, $returnType); + } + if ($runtimeType instanceof Promise) { return $runtimeType->then(function($resolvedRuntimeType) use ($returnType, $fieldNodes, $info, $path, &$result) { - return $this->validateRuntimeTypeAndCompleteObjectValue( - $returnType, + return $this->completeObjectValue( + $this->ensureValidRuntimeType( + $resolvedRuntimeType, + $returnType, + $fieldNodes, + $info, + $result + ), $fieldNodes, $info, $path, - $resolvedRuntimeType, $result ); }); } - return $this->validateRuntimeTypeAndCompleteObjectValue( - $returnType, + return $this->completeObjectValue( + $this->ensureValidRuntimeType( + $runtimeType, + $returnType, + $fieldNodes, + $info, + $result + ), $fieldNodes, $info, $path, - $runtimeType, $result ); } /** + * @param string|ObjectType|null $runtimeTypeOrName * @param AbstractType $returnType - * @param FieldNode[] $fieldNodes + * @param $fieldNodes * @param ResolveInfo $info - * @param array $path - * @param mixed $returnedRuntimeType * @param $result - * @return array|Promise|\stdClass + * @return ObjectType * @throws Error */ - private function validateRuntimeTypeAndCompleteObjectValue( + private function ensureValidRuntimeType( + $runtimeTypeOrName, AbstractType $returnType, $fieldNodes, ResolveInfo $info, - $path, - $returnedRuntimeType, &$result ) { - $exeContext = $this->exeContext; - $runtimeType = $returnedRuntimeType; + $runtimeType = is_string($runtimeTypeOrName) ? + $this->exeContext->schema->getType($runtimeTypeOrName) : + $runtimeTypeOrName; - // If resolveType returns a string, we assume it's a ObjectType name. - if (is_string($runtimeType)) { - $runtimeType = $exeContext->schema->getType($runtimeType); - } - - if (!($runtimeType instanceof ObjectType)) { + if (!$runtimeType instanceof ObjectType) { throw new Error( "Abstract type {$returnType} must resolve to an Object type at runtime " . - "for field {$info->parentType}.{$info->fieldName} with value: " . Utils::printSafe($result) . "," . - "received \"$runtimeType\".", + "for field {$info->parentType}.{$info->fieldName} with " . + 'value "' . Utils::printSafe($result) . '", received "'. Utils::printSafe($runtimeType) . '".', $fieldNodes ); } - if (!$exeContext->schema->isPossibleType($returnType, $runtimeType)) { + if (!$this->exeContext->schema->isPossibleType($returnType, $runtimeType)) { throw new Error( "Runtime Object type \"$runtimeType\" is not a possible type for \"$returnType\".", $fieldNodes ); } - return $this->completeObjectValue($runtimeType, $fieldNodes, $info, $path, $result); + + return $runtimeType; } /** @@ -1114,67 +1119,81 @@ class Executor */ private function completeObjectValue(ObjectType $returnType, $fieldNodes, ResolveInfo $info, $path, &$result) { + // If there is an isTypeOf predicate function, call it with the + // current result. If isTypeOf returns false, then raise an error rather + // than continuing execution. $isTypeOf = $returnType->isTypeOf($result, $this->exeContext->contextValue, $info); - if (null === $isTypeOf) { - return $this->validateResultTypeAndExecuteFields( - $returnType, - $fieldNodes, - $info, - $path, - $result, - true - ); + if (null !== $isTypeOf) { + if ($this->promises->isThenable($isTypeOf)) { + /** @var Promise $isTypeOf */ + $isTypeOf = $this->promises->convertThenable($isTypeOf); + Utils::invariant($isTypeOf instanceof Promise); + + return $isTypeOf->then(function($isTypeOfResult) use ($returnType, $fieldNodes, $info, $path, &$result) { + if (!$isTypeOfResult) { + throw $this->invalidReturnTypeError($returnType, $result, $fieldNodes); + } + + return $this->collectAndExecuteSubfields( + $returnType, + $fieldNodes, + $info, + $path, + $result + ); + }); + } + if (!$isTypeOf) { + throw $this->invalidReturnTypeError($returnType, $result, $fieldNodes); + } } - if ($this->promises->isThenable($isTypeOf)) { - /** @var Promise $isTypeOf */ - $isTypeOf = $this->promises->convertThenable($isTypeOf); - Utils::invariant($isTypeOf instanceof Promise); - - return $isTypeOf->then(function($isTypeOfResult) use ($returnType, $fieldNodes, $info, $path, &$result) { - return $this->validateResultTypeAndExecuteFields( - $returnType, - $fieldNodes, - $info, - $path, - $result, - $isTypeOfResult - ); - }); - } - - return $this->validateResultTypeAndExecuteFields($returnType, $fieldNodes, $info, $path, $result, $isTypeOf); + return $this->collectAndExecuteSubfields( + $returnType, + $fieldNodes, + $info, + $path, + $result + ); } + /** + * @param ObjectType $returnType + * @param array $result + * @param FieldNode[] $fieldNodes + * @return Error + */ + private function invalidReturnTypeError( + ObjectType $returnType, + $result, + $fieldNodes + ) + { + return new Error( + 'Expected value of type "' . $returnType->name . '" but got: ' . Utils::printSafe($result) . '.', + $fieldNodes + ); + } + + /** * @param ObjectType $returnType * @param FieldNode[] $fieldNodes * @param ResolveInfo $info * @param array $path * @param array $result - * @param bool $isTypeOfResult * @return array|Promise|\stdClass * @throws Error */ - private function validateResultTypeAndExecuteFields( + private function collectAndExecuteSubfields( ObjectType $returnType, $fieldNodes, ResolveInfo $info, $path, - &$result, - $isTypeOfResult + &$result ) { - // If isTypeOf returns false, then raise an error - // rather than continuing execution. - if (false === $isTypeOfResult) { - throw new Error( - "Expected value of type $returnType but got: " . Utils::getVariableType($result), - $fieldNodes - ); - } - // Collect sub-fields to execute to complete this value. $subFieldNodes = new \ArrayObject(); $visitedFragmentNames = new \ArrayObject(); @@ -1208,15 +1227,13 @@ class Executor { $possibleTypes = $info->schema->getPossibleTypes($abstractType); $promisedIsTypeOfResults = []; - $promisedIsTypeOfResultTypes = []; - foreach ($possibleTypes as $type) { + foreach ($possibleTypes as $index => $type) { $isTypeOfResult = $type->isTypeOf($value, $context, $info); if (null !== $isTypeOfResult) { if ($this->promises->isThenable($isTypeOfResult)) { - $promisedIsTypeOfResults[] = $this->promises->convertThenable($isTypeOfResult); - $promisedIsTypeOfResultTypes[] = $type; + $promisedIsTypeOfResults[$index] = $this->promises->convertThenable($isTypeOfResult); } else if ($isTypeOfResult) { return $type; } @@ -1225,10 +1242,10 @@ class Executor if (!empty($promisedIsTypeOfResults)) { return $this->promises->all($promisedIsTypeOfResults) - ->then(function($isTypeOfResults) use ($promisedIsTypeOfResultTypes) { + ->then(function($isTypeOfResults) use ($possibleTypes) { foreach ($isTypeOfResults as $index => $result) { if ($result) { - return $promisedIsTypeOfResultTypes[$index]; + return $possibleTypes[$index]; } } return null; diff --git a/tests/Executor/ExecutorTest.php b/tests/Executor/ExecutorTest.php index ba7e5ff..fb30aab 100644 --- a/tests/Executor/ExecutorTest.php +++ b/tests/Executor/ExecutorTest.php @@ -934,9 +934,10 @@ class ExecutorTest extends \PHPUnit_Framework_TestCase ], $result->data); $this->assertEquals(1, count($result->errors)); - $this->assertArraySubset([ - 'message' => 'Expected value of type SpecialType but got: GraphQL\Tests\Executor\NotSpecial', - 'locations' => [['line' => 1, 'column' => 3]] + $this->assertEquals([ + 'message' => 'Expected value of type "SpecialType" but got: instance of GraphQL\Tests\Executor\NotSpecial.', + 'locations' => [['line' => 1, 'column' => 3]], + 'path' => ['specials', 1] ], $result->errors[0]->toSerializableArray()); } diff --git a/tests/Type/ValidationTest.php b/tests/Type/ValidationTest.php index 7f3094e..79fbead 100644 --- a/tests/Type/ValidationTest.php +++ b/tests/Type/ValidationTest.php @@ -243,11 +243,19 @@ class ValidationTest extends \PHPUnit_Framework_TestCase // TODO: does not allow isDeprecated without deprecationReason on field // TODO: does not allow isDeprecated without deprecationReason on enum - // Type System: Object fields must have valid resolve values + // DESCRIBE: Type System: Object fields must have valid resolve values // TODO: accepts a lambda as an Object field resolver // TODO: rejects an empty Object field resolver // TODO: rejects a constant scalar value resolver + // DESCRIBE: Type System: Input Object fields must not have resolvers + // TODO: accepts an Input Object type with no resolver + // TODO: accepts an Input Object type with null resolver + // TODO: accepts an Input Object type with undefined resolver + // TODO: rejects an Input Object type with resolver function + // TODO: rejects an Input Object type with resolver constant + + private function assertEachCallableThrows($closures, $expectedError) { foreach ($closures as $index => $factory) {