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 1 commit
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
43 changes: 34 additions & 9 deletions src/Execution/Executor.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,6 @@
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 +71,7 @@ class Executor

/**
* Executor constructor.
*
* @param ExecutionContext $context
* @param FieldCollector $fieldCollector
* @param ErrorHandlerInterface|null $errorHandler
Expand Down Expand Up @@ -114,8 +114,8 @@ public function execute(): ?array
$result = $operation->getOperation() === 'mutation'
? $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));
} catch (ExecutionException $ex) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Now \Throwable and other exceptions won't ever be caught?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This should be correct, \Throwables are already elsewhere wrapped after ExecutionExceptions.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What we can do is add another catch clause, just to be sure though. Good point.

$this->handleError($ex);
return null;
}

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

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

return $subscriptionType;
default:
throw new ExecutionException(
Expand All @@ -166,9 +169,9 @@ 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 @@ -193,6 +196,7 @@ public function executeFieldsSerially(
/** @var ExtendedPromiseInterface $result */
return $result->then(function ($resolvedResult) use ($fieldName, $results) {
$results[$fieldName] = $resolvedResult;

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

$promise->then(function ($resolvedResults) use (&$finalResults) {
$finalResults = $resolvedResults ?? [];
})->otherwise(function ($ex) {
$this->context->addError($ex);
})->otherwise(function (ExecutionException $ex) {
Copy link
Contributor

Choose a reason for hiding this comment

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

What if $ex is not ExecutionException? Do we lose that exception?

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry, clicked the wrong button. I'm also wondering if this is correct?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You're right, I've fixed this now.

$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 Down Expand Up @@ -262,6 +267,7 @@ public function getFieldDefinition(
* @param ResolveInfo $info
* @param array $path
* @param mixed $result
*
* @return array|mixed|null
* @throws \Throwable
*/
Expand Down Expand Up @@ -293,6 +299,7 @@ public function completeValueCatchingError(

if ($this->isPromise($completed)) {
$context = $this->context;

/** @var ExtendedPromiseInterface $completed */
return $completed->then(null, function ($error) use ($context, $fieldNodes, $path) {
//@TODO Handle $error better
Expand All @@ -307,6 +314,7 @@ public function completeValueCatchingError(
)
);
}

return new FulfilledPromise(null);
});
}
Expand All @@ -318,13 +326,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,6 +365,7 @@ public function completeValueWithLocatedError(
* @param mixed $rootValue
* @param array $path
* @param array $fields
*
* @return array
* @throws \Throwable
*/
Expand Down Expand Up @@ -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 Down Expand Up @@ -552,6 +564,7 @@ protected function completeValue(
* @param ResolveInfo $info
* @param array $path
* @param mixed $result
*
* @return array|PromiseInterface
* @throws ExecutionException
* @throws InvalidTypeException
Expand Down Expand Up @@ -605,6 +618,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 +680,7 @@ protected function ensureValidRuntimeType(
* @param mixed $context
* @param ResolveInfo $info
* @param AbstractTypeInterface $abstractType
*
* @return NamedTypeInterface|mixed|null
* @throws InvariantException
*/
Expand Down Expand Up @@ -713,6 +728,7 @@ protected function defaultTypeResolver(
return $possibleTypes[$index];
}
}

return null;
});
}
Expand All @@ -726,6 +742,7 @@ protected function defaultTypeResolver(
* @param ResolveInfo $info
* @param array $path
* @param mixed $result
*
* @return array|\React\Promise\Promise
* @throws \Throwable
*/
Expand Down Expand Up @@ -768,6 +785,7 @@ protected function completeListValue(
/**
* @param LeafTypeInterface|SerializableTypeInterface $returnType
* @param mixed $result
*
* @return mixed
* @throws ExecutionException
*/
Expand All @@ -791,6 +809,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 +844,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 +874,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 @@ -889,6 +910,7 @@ protected function executeSubFields(

/**
* @param mixed $value
*
* @return bool
*/
protected function isPromise($value): bool
Expand All @@ -900,6 +922,7 @@ protected function isPromise($value): bool
* @param \Throwable $originalException
* @param array $nodes
* @param array $path
*
* @return ExecutionException
*/
protected function buildLocatedError(
Expand Down Expand Up @@ -933,6 +956,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 +1000,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