Skip to content
This repository has been archived by the owner on Mar 8, 2023. It is now read-only.

Fix handling of errors in mutations #297

Merged
merged 4 commits into from
Oct 8, 2018
Merged

Fix handling of errors in mutations #297

Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
cce6c02
Fix handling of errors in mutations
crisu83 Oct 8, 2018
da1c0d5
Use instanceof to check for promises
crisu83 Oct 8, 2018
50003e9
Fix error handling of \Throwable
crisu83 Oct 8, 2018
362ff30
Fix handling of Throwables in mutations
crisu83 Oct 8, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
83 changes: 48 additions & 35 deletions src/Execution/Executor.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,11 +22,9 @@
use Digia\GraphQL\Type\Definition\SerializableTypeInterface;
use Digia\GraphQL\Type\Definition\TypeInterface;
use Digia\GraphQL\Type\Definition\UnionType;
use InvalidArgumentException;
use React\Promise\ExtendedPromiseInterface;
use React\Promise\FulfilledPromise;
use React\Promise\PromiseInterface;
use Throwable;
use function Digia\GraphQL\Type\SchemaMetaFieldDefinition;
use function Digia\GraphQL\Type\TypeMetaFieldDefinition;
use function Digia\GraphQL\Type\TypeNameMetaFieldDefinition;
Expand Down Expand Up @@ -72,6 +70,7 @@ class Executor

/**
* Executor constructor.
*
* @param ExecutionContext $context
* @param FieldCollector $fieldCollector
* @param ErrorHandlerInterface|null $errorHandler
Expand Down Expand Up @@ -115,7 +114,7 @@ public function execute(): ?array
? $this->executeFieldsSerially($objectType, $rootValue, $path, $fields)
: $this->executeFields($objectType, $rootValue, $path, $fields);
} catch (\Throwable $ex) {
$this->handleError(new ExecutionException($ex->getMessage(), $fields, null, null, null, null, $ex));
$this->handleError(new ExecutionException($ex->getMessage(), null, null, null, null, null, $ex));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@crisu83 not sure why you removed the $fields?

return null;
}

Expand All @@ -125,6 +124,7 @@ public function execute(): ?array
/**
* @param Schema $schema
* @param OperationDefinitionNode $operation
*
* @return ObjectType|null
* @throws ExecutionException
*/
Expand All @@ -141,6 +141,7 @@ public function getOperationType(Schema $schema, OperationDefinitionNode $operat
[$operation]
);
}

return $mutationType;
case 'subscription':
$subscriptionType = $schema->getSubscriptionType();
Expand All @@ -150,6 +151,7 @@ public function getOperationType(Schema $schema, OperationDefinitionNode $operat
[$operation]
);
}

return $subscriptionType;
default:
throw new ExecutionException(
Expand All @@ -166,9 +168,8 @@ public function getOperationType(Schema $schema, OperationDefinitionNode $operat
* @param mixed $rootValue
* @param array $path
* @param array $fields
*
* @return array
* @throws InvalidArgumentException
* @throws Throwable
*/
public function executeFieldsSerially(
ObjectType $objectType,
Expand All @@ -189,10 +190,10 @@ public function executeFieldsSerially(
return null;
}

if ($this->isPromise($result)) {
/** @var ExtendedPromiseInterface $result */
if ($result instanceof ExtendedPromiseInterface) {
return $result->then(function ($resolvedResult) use ($fieldName, $results) {
$results[$fieldName] = $resolvedResult;

return $results;
});
}
Expand All @@ -217,8 +218,11 @@ public function executeFieldsSerially(

$promise->then(function ($resolvedResults) use (&$finalResults) {
$finalResults = $resolvedResults ?? [];
})->otherwise(function ($ex) {
$this->context->addError($ex);
})->otherwise(function (\Throwable $ex) {
if (!$ex instanceof ExecutionException) {
$ex = new ExecutionException($ex->getMessage(), null, null, null, null, null, $ex);
}
$this->handleError($ex);
});

return $finalResults;
Expand All @@ -228,6 +232,7 @@ public function executeFieldsSerially(
* @param Schema $schema
* @param ObjectType $parentType
* @param string $fieldName
*
* @return Field|null
*/
public function getFieldDefinition(
Expand All @@ -251,6 +256,7 @@ public function getFieldDefinition(
return $typeNameMetaFieldDefinition;
}

/** @noinspection PhpUnhandledExceptionInspection */
$fields = $parentType->getFields();

return $fields[$fieldName] ?? null;
Expand All @@ -262,6 +268,7 @@ public function getFieldDefinition(
* @param ResolveInfo $info
* @param array $path
* @param mixed $result
*
* @return array|mixed|null
* @throws \Throwable
*/
Expand Down Expand Up @@ -291,22 +298,22 @@ public function completeValueCatchingError(
$result
);

if ($this->isPromise($completed)) {
$context = $this->context;
/** @var ExtendedPromiseInterface $completed */
return $completed->then(null, function ($error) use ($context, $fieldNodes, $path) {
//@TODO Handle $error better
if ($completed instanceof ExtendedPromiseInterface) {
return $completed->then(null, function ($error) use ($fieldNodes, $path) {
if ($error instanceof \Exception) {
$context->addError($this->buildLocatedError($error, $fieldNodes, $path));
$this->handleError(
$this->buildLocatedError($error, $fieldNodes, $path)
);
} else {
$context->addError(
$this->handleError(
$this->buildLocatedError(
new ExecutionException($error ?? 'An unknown error occurred.'),
$fieldNodes,
$path
)
);
}

return new FulfilledPromise(null);
});
}
Expand All @@ -318,13 +325,13 @@ public function completeValueCatchingError(
}
}


/**
* @param TypeInterface $fieldType
* @param FieldNode[] $fieldNodes
* @param ResolveInfo $info
* @param array $path
* @param mixed $result
*
* @return array|mixed
* @throws \Throwable
*/
Expand Down Expand Up @@ -357,7 +364,9 @@ public function completeValueWithLocatedError(
* @param mixed $rootValue
* @param array $path
* @param array $fields
*
* @return array
* @throws ExecutionException
* @throws \Throwable
*/
protected function executeFields(
Expand All @@ -379,7 +388,7 @@ protected function executeFields(
continue;
}

$doesContainPromise = $doesContainPromise || $this->isPromise($result);
$doesContainPromise = $doesContainPromise || $result instanceof ExtendedPromiseInterface;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@crisu83 Not sure why you don't like the isPromise method, but we use it in ~3 places at least.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because it’s better to use instanceof instead of the @var comments.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@crisu83 If you insist, feel free to change, but we actually don't really use it in the code.


$finalResults[$fieldName] = $result;
}
Expand All @@ -404,9 +413,10 @@ protected function executeFields(
* @param mixed $rootValue
* @param FieldNode[] $fieldNodes
* @param array $path
*
* @return array|mixed|null
* @throws UndefinedException
* @throws Throwable
* @throws \Throwable
*/
protected function resolveField(
ObjectType $parentType,
Expand Down Expand Up @@ -450,6 +460,7 @@ protected function resolveField(
/**
* @param Field $field
* @param ObjectType $objectType
*
* @return callable|mixed|null
*/
protected function determineResolveCallback(Field $field, ObjectType $objectType)
Expand All @@ -471,6 +482,7 @@ protected function determineResolveCallback(Field $field, ObjectType $objectType
* @param ResolveInfo $info
* @param array $path
* @param mixed $result
*
* @return array|mixed
* @throws InvariantException
* @throws InvalidTypeException
Expand All @@ -484,8 +496,7 @@ protected function completeValue(
array $path,
&$result
) {
if ($this->isPromise($result)) {
/** @var ExtendedPromiseInterface $result */
if ($result instanceof ExtendedPromiseInterface) {
return $result->then(function (&$value) use ($returnType, $fieldNodes, $info, $path) {
return $this->completeValue($returnType, $fieldNodes, $info, $path, $value);
});
Expand Down Expand Up @@ -552,6 +563,7 @@ protected function completeValue(
* @param ResolveInfo $info
* @param array $path
* @param mixed $result
*
* @return array|PromiseInterface
* @throws ExecutionException
* @throws InvalidTypeException
Expand All @@ -572,8 +584,7 @@ protected function completeAbstractValue(
$runtimeType = $this->defaultTypeResolver($result, $this->context->getContextValue(), $info, $returnType);
}

if ($this->isPromise($runtimeType)) {
/** @var ExtendedPromiseInterface $runtimeType */
if ($runtimeType instanceof ExtendedPromiseInterface) {
return $runtimeType->then(function ($resolvedRuntimeType) use (
$returnType,
$fieldNodes,
Expand Down Expand Up @@ -605,6 +616,7 @@ protected function completeAbstractValue(
* @param NamedTypeInterface $returnType
* @param ResolveInfo $info
* @param mixed $result
*
* @return TypeInterface|ObjectType|null
* @throws ExecutionException
* @throws InvariantException
Expand Down Expand Up @@ -666,6 +678,7 @@ protected function ensureValidRuntimeType(
* @param mixed $context
* @param ResolveInfo $info
* @param AbstractTypeInterface $abstractType
*
* @return NamedTypeInterface|mixed|null
* @throws InvariantException
*/
Expand All @@ -686,7 +699,7 @@ protected function defaultTypeResolver(
foreach ($possibleTypes as $index => $type) {
$isTypeOfResult = $type->isTypeOf($value, $context, $info);

if ($this->isPromise($isTypeOfResult)) {
if ($isTypeOfResult instanceof ExtendedPromiseInterface) {
$promisedIsTypeOfResults[$index] = $isTypeOfResult;
continue;
}
Expand All @@ -713,6 +726,7 @@ protected function defaultTypeResolver(
return $possibleTypes[$index];
}
}

return null;
});
}
Expand All @@ -726,6 +740,7 @@ protected function defaultTypeResolver(
* @param ResolveInfo $info
* @param array $path
* @param mixed $result
*
* @return array|\React\Promise\Promise
* @throws \Throwable
*/
Expand Down Expand Up @@ -757,7 +772,7 @@ protected function completeListValue(
$fieldPath[] = $key;
$completedItem = $this->completeValueCatchingError($itemType, $fieldNodes, $info, $fieldPath, $item);
$completedItems[] = $completedItem;
$doesContainPromise = $doesContainPromise || $this->isPromise($completedItem);
$doesContainPromise = $doesContainPromise || $completedItem instanceof ExtendedPromiseInterface;
}

return $doesContainPromise
Expand All @@ -768,6 +783,7 @@ protected function completeListValue(
/**
* @param LeafTypeInterface|SerializableTypeInterface $returnType
* @param mixed $result
*
* @return mixed
* @throws ExecutionException
*/
Expand All @@ -791,6 +807,7 @@ protected function completeLeafValue($returnType, &$result)
* @param ResolveInfo $info
* @param array $path
* @param mixed $result
*
* @return array
* @throws ExecutionException
* @throws InvalidTypeException
Expand Down Expand Up @@ -825,6 +842,7 @@ protected function completeObjectValue(
* @param mixed $rootValue
* @param ExecutionContext $context
* @param ResolveInfo $info
*
* @return array|\Throwable
*/
protected function resolveFieldValueOrError(
Expand Down Expand Up @@ -854,11 +872,12 @@ protected function resolveFieldValueOrError(
* @param FieldNode[] $fieldNodes
* @param array $path
* @param mixed $result
*
* @return array
* @throws ExecutionException
* @throws InvalidTypeException
* @throws InvariantException
* @throws Throwable
* @throws \Throwable
*/
protected function executeSubFields(
ObjectType $returnType,
Expand Down Expand Up @@ -887,19 +906,11 @@ protected function executeSubFields(
return $result;
}

/**
* @param mixed $value
* @return bool
*/
protected function isPromise($value): bool
{
return $value instanceof ExtendedPromiseInterface;
}

/**
* @param \Throwable $originalException
* @param array $nodes
* @param array $path
*
* @return ExecutionException
*/
protected function buildLocatedError(
Expand Down Expand Up @@ -933,6 +944,7 @@ protected function buildLocatedError(
* @param ObjectType $parentType
* @param array|null $path
* @param ExecutionContext $context
*
* @return ResolveInfo
*/
protected function createResolveInfo(
Expand Down Expand Up @@ -976,6 +988,7 @@ protected function handleError(ExecutionException $error)
* @param array $arguments
* @param mixed $contextValues
* @param ResolveInfo $info
*
* @return mixed|null
*/
public static function defaultFieldResolver($rootValue, array $arguments, $contextValues, ResolveInfo $info)
Expand Down