From 9b9a74c1d1fbd3281152ef82d6c781b0b2465933 Mon Sep 17 00:00:00 2001 From: Vladimir Razuvaev Date: Wed, 5 Jul 2017 17:31:35 +0700 Subject: [PATCH] Spec compliance: errors in buildExecutionContext() are caught and included in result rather than thrown --- src/Executor/Executor.php | 54 ++++-- tests/Executor/ExecutorTest.php | 102 ++++++---- tests/Executor/VariablesTest.php | 310 ++++++++++++++++--------------- tests/Language/ParserTest.php | 29 ++- 4 files changed, 290 insertions(+), 205 deletions(-) diff --git a/src/Executor/Executor.php b/src/Executor/Executor.php index 061a3ce..5fb1c03 100644 --- a/src/Executor/Executor.php +++ b/src/Executor/Executor.php @@ -115,17 +115,26 @@ class Executor ); } - $exeContext = self::buildExecutionContext( - $schema, - $ast, - $rootValue, - $contextValue, - $variableValues, - $operationName, - $fieldResolver - ); $promiseAdapter = self::getPromiseAdapter(); + try { + $exeContext = self::buildExecutionContext( + $schema, + $ast, + $rootValue, + $contextValue, + $variableValues, + $operationName, + $fieldResolver + ); + } catch (Error $e) { + if ($promiseAdapter instanceof SyncPromiseAdapter) { + return new ExecutionResult(null, [$e]); + } else { + return $promiseAdapter->createFulfilled(new ExecutionResult(null, [$e])); + } + } + $executor = new self($exeContext, $promiseAdapter); $result = $executor->executeQuery(); @@ -283,11 +292,30 @@ class Executor $fields = $this->collectFields($type, $operation->selectionSet, new \ArrayObject(), new \ArrayObject()); $path = []; - if ($operation->operation === 'mutation') { - return $this->executeFieldsSerially($type, $rootValue, $path, $fields); - } - return $this->executeFields($type, $rootValue, $path, $fields); + // 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. + // + // Similar to completeValueCatchingError. + try { + $result = $operation->operation === 'mutation' ? + $this->executeFieldsSerially($type, $rootValue, $path, $fields) : + $this->executeFields($type, $rootValue, $path, $fields); + + $promise = $this->getPromise($result); + if ($promise) { + return $promise->then(null, function($error) { + $this->exeContext->addError($error); + return null; + }); + } + return $result; + + } catch (Error $error) { + $this->exeContext->addError($error); + return null; + } } diff --git a/tests/Executor/ExecutorTest.php b/tests/Executor/ExecutorTest.php index ecc2403..5121234 100644 --- a/tests/Executor/ExecutorTest.php +++ b/tests/Executor/ExecutorTest.php @@ -589,9 +589,9 @@ class ExecutorTest extends \PHPUnit_Framework_TestCase } /** - * @it throws if no operation is provided + * @it provides error if no operation is provided */ - public function testThrowsIfNoOperationIsProvided() + public function testProvidesErrorIfNoOperationIsProvided() { $doc = 'fragment Example on Type { a }'; $data = [ 'a' => 'b' ]; @@ -605,62 +605,84 @@ class ExecutorTest extends \PHPUnit_Framework_TestCase ]) ]); - try { - Executor::execute($schema, $ast, $data); - $this->fail('Expected exception was not thrown'); - } catch (Error $e) { - $this->assertEquals('Must provide an operation.', $e->getMessage()); - } + $result = Executor::execute($schema, $ast, $data); + $expected = [ + 'errors' => [ + [ + 'message' => 'Must provide an operation.', + ] + ] + ]; + + $this->assertEquals($expected, $result->toArray()); } /** - * @it throws if no operation name is provided with multiple operations + * @it errors if no op name is provided with multiple operations */ - public function testThrowsIfNoOperationIsProvidedWithMultipleOperations() + public function testErrorsIfNoOperationIsProvidedWithMultipleOperations() { $doc = 'query Example { a } query OtherExample { a }'; - $data = [ 'a' => 'b' ]; + $data = ['a' => 'b']; $ast = Parser::parse($doc); $schema = new Schema([ 'query' => new ObjectType([ 'name' => 'Type', 'fields' => [ - 'a' => [ 'type' => Type::string() ], + 'a' => ['type' => Type::string()], ] ]) ]); - try { - Executor::execute($schema, $ast, $data); - $this->fail('Expected exception is not thrown'); - } catch (Error $err) { - $this->assertEquals('Must provide operation name if query contains multiple operations.', $err->getMessage()); - } + $result = Executor::execute($schema, $ast, $data); + + $expected = [ + 'errors' => [ + [ + 'message' => 'Must provide operation name if query contains multiple operations.', + ] + ] + ]; + + $this->assertEquals($expected, $result->toArray()); } /** - * @it throws if unknown operation name is provided + * @it errors if unknown operation name is provided */ - public function testThrowsIfUnknownOperationNameIsProvided() + public function testErrorsIfUnknownOperationNameIsProvided() { $doc = 'query Example { a } query OtherExample { a }'; - $data = [ 'a' => 'b' ]; $ast = Parser::parse($doc); $schema = new Schema([ 'query' => new ObjectType([ 'name' => 'Type', 'fields' => [ - 'a' => [ 'type' => Type::string() ], + 'a' => ['type' => Type::string()], ] ]) ]); - try { - Executor::execute($schema, $ast, $data, null, null, 'UnknownExample'); - $this->fail('Expected exception was not thrown'); - } catch (Error $e) { - $this->assertEquals('Unknown operation named "UnknownExample".', $e->getMessage()); - } + + $result = Executor::execute( + $schema, + $ast, + null, + null, + null, + 'UnknownExample' + ); + + $expected = [ + 'errors' => [ + [ + 'message' => 'Unknown operation named "UnknownExample".', + ] + ] + + ]; + + $this->assertEquals($expected, $result->toArray()); } /** @@ -956,20 +978,24 @@ class ExecutorTest extends \PHPUnit_Framework_TestCase 'query' => new ObjectType([ 'name' => 'Query', 'fields' => [ - 'foo' => ['type' => Type::string() ] + 'foo' => ['type' => Type::string()] ] ]) ]); - try { - Executor::execute($schema, $query); - $this->fail('Expected exception was not thrown'); - } catch (Error $e) { - $this->assertEquals([ - 'message' => 'GraphQL cannot execute a request containing a ObjectTypeDefinition.', - 'locations' => [['line' => 4, 'column' => 7]] - ], $e->toSerializableArray()); - } + + $result = Executor::execute($schema, $query); + + $expected = [ + 'errors' => [ + [ + 'message' => 'GraphQL cannot execute a request containing a ObjectTypeDefinition.', + 'locations' => [['line' => 4, 'column' => 7]], + ] + ] + ]; + + $this->assertEquals($expected, $result->toArray()); } /** diff --git a/tests/Executor/VariablesTest.php b/tests/Executor/VariablesTest.php index 25588a0..a8107f9 100644 --- a/tests/Executor/VariablesTest.php +++ b/tests/Executor/VariablesTest.php @@ -155,49 +155,49 @@ class VariablesTest extends \PHPUnit_Framework_TestCase // errors on null for nested non-null: $params = ['input' => ['a' => 'foo', 'b' => 'bar', 'c' => null]]; - $expected = FormattedError::create( - 'Variable "$input" got invalid value {"a":"foo","b":"bar","c":null}.'. "\n". - 'In field "c": Expected "String!", found null.', - [new SourceLocation(2, 17)] - ); - - try { - Executor::execute($schema, $ast, null, null, $params); - $this->fail('Expected exception not thrown'); - } catch (Error $err) { - $this->assertEquals($expected, Error::formatError($err)); - } + $result = Executor::execute($schema, $ast, null, null, $params); + $expected = [ + 'errors' => [ + [ + 'message' => + 'Variable "$input" got invalid value {"a":"foo","b":"bar","c":null}.'. "\n". + 'In field "c": Expected "String!", found null.', + 'locations' => [['line' => 2, 'column' => 17]] + ] + ] + ]; + $this->assertEquals($expected, $result->toArray()); // errors on incorrect type: $params = [ 'input' => 'foo bar' ]; - - try { - Executor::execute($schema, $ast, null, null, $params); - $this->fail('Expected exception not thrown'); - } catch (Error $error) { - $expected = FormattedError::create( - 'Variable "$input" got invalid value "foo bar".'."\n". - 'Expected "TestInputObject", found not an object.', - [new SourceLocation(2, 17)] - ); - $this->assertEquals($expected, Error::formatError($error)); - } + $result = Executor::execute($schema, $ast, null, null, $params); + $expected = [ + 'errors' => [ + [ + 'message' => + 'Variable "$input" got invalid value "foo bar".' . "\n" . + 'Expected "TestInputObject", found not an object.', + 'locations' => [ [ 'line' => 2, 'column' => 17 ] ], + ] + ] + ]; + $this->assertEquals($expected, $result->toArray()); // errors on omission of nested non-null: $params = ['input' => ['a' => 'foo', 'b' => 'bar']]; - try { - Executor::execute($schema, $ast, null, null, $params); - $this->fail('Expected exception not thrown'); - } catch (Error $e) { - $expected = FormattedError::create( - 'Variable "$input" got invalid value {"a":"foo","b":"bar"}.'. "\n". - 'In field "c": Expected "String!", found null.', - [new SourceLocation(2, 17)] - ); - - $this->assertEquals($expected, Error::formatError($e)); - } + $result = Executor::execute($schema, $ast, null, null, $params); + $expected = [ + 'errors' => [ + [ + 'message' => + 'Variable "$input" got invalid value {"a":"foo","b":"bar"}.'. "\n". + 'In field "c": Expected "String!", found null.', + 'locations' => [['line' => 2, 'column' => 17]] + ] + ] + ]; + $this->assertEquals($expected, $result->toArray()); // errors on deep nested errors and with many errors $nestedDoc = ' @@ -208,33 +208,35 @@ class VariablesTest extends \PHPUnit_Framework_TestCase $nestedAst = Parser::parse($nestedDoc); $params = [ 'input' => [ 'na' => [ 'a' => 'foo' ] ] ]; - try { - Executor::execute($schema, $nestedAst, null, null, $params); - $this->fail('Expected exception not thrown'); - } catch (Error $error) { - $expected = FormattedError::create( - 'Variable "$input" got invalid value {"na":{"a":"foo"}}.' . "\n" . - 'In field "na": In field "c": Expected "String!", found null.' . "\n" . - 'In field "nb": Expected "String!", found null.', - [new SourceLocation(2, 19)] - ); - $this->assertEquals($expected, Error::formatError($error)); - } + $result = Executor::execute($schema, $nestedAst, null, null, $params); + $expected = [ + 'errors' => [ + [ + 'message' => + 'Variable "$input" got invalid value {"na":{"a":"foo"}}.' . "\n" . + 'In field "na": In field "c": Expected "String!", found null.' . "\n" . + 'In field "nb": Expected "String!", found null.', + 'locations' => [['line' => 2, 'column' => 19]] + ] + ] + ]; + $this->assertEquals($expected, $result->toArray()); + // errors on addition of unknown input field $params = ['input' => [ 'a' => 'foo', 'b' => 'bar', 'c' => 'baz', 'd' => 'dog' ]]; - - try { - Executor::execute($schema, $ast, null, null, $params); - $this->fail('Expected exception not thrown'); - } catch (Error $e) { - $expected = FormattedError::create( - 'Variable "$input" got invalid value {"a":"foo","b":"bar","c":"baz","d":"dog"}.'."\n". - 'In field "d": Expected type "ComplexScalar", found "dog".', - [new SourceLocation(2, 17)] - ); - $this->assertEquals($expected, Error::formatError($e)); - } + $result = Executor::execute($schema, $ast, null, null, $params); + $expected = [ + 'errors' => [ + [ + 'message' => + 'Variable "$input" got invalid value {"a":"foo","b":"bar","c":"baz","d":"dog"}.'."\n". + 'In field "d": Expected type "ComplexScalar", found "dog".', + 'locations' => [['line' => 2, 'column' => 17]] + ] + ] + ]; + $this->assertEquals($expected, $result->toArray()); } // Describe: Handles nullable scalars @@ -366,16 +368,17 @@ class VariablesTest extends \PHPUnit_Framework_TestCase } '; $ast = Parser::parse($doc); - try { - Executor::execute($this->schema(), $ast); - $this->fail('Expected exception not thrown'); - } catch (Error $e) { - $expected = FormattedError::create( - 'Variable "$value" of required type "String!" was not provided.', - [new SourceLocation(2, 31)] - ); - $this->assertEquals($expected, Error::formatError($e)); - } + $result = Executor::execute($this->schema(), $ast); + + $expected = [ + 'errors' => [ + [ + 'message' => 'Variable "$value" of required type "String!" was not provided.', + 'locations' => [['line' => 2, 'column' => 31]] + ] + ] + ]; + $this->assertEquals($expected, $result->toArray()); } /** @@ -389,19 +392,18 @@ class VariablesTest extends \PHPUnit_Framework_TestCase } '; $ast = Parser::parse($doc); - - try { - Executor::execute($this->schema(), $ast, null, null, ['value' => null]); - $this->fail('Expected exception not thrown'); - } catch (Error $e) { - $expected = [ - 'message' => - 'Variable "$value" got invalid value null.' . "\n". - 'Expected "String!", found null.', - 'locations' => [['line' => 2, 'column' => 31]] - ]; - $this->assertEquals($expected, Error::formatError($e)); - } + $result = Executor::execute($this->schema(), $ast, null, null, ['value' => null]); + $expected = [ + 'errors' => [ + [ + 'message' => + 'Variable "$value" got invalid value null.' . "\n". + 'Expected "String!", found null.', + 'locations' => [['line' => 2, 'column' => 31]] + ] + ] + ]; + $this->assertEquals($expected, $result->toArray()); } /** @@ -546,18 +548,18 @@ class VariablesTest extends \PHPUnit_Framework_TestCase } '; $ast = Parser::parse($doc); - $expected = FormattedError::create( - 'Variable "$input" got invalid value null.' . "\n" . - 'Expected "[String]!", found null.', - [new SourceLocation(2, 17)] - ); - - try { - $this->assertEquals($expected, Executor::execute($this->schema(), $ast, null, null, ['input' => null])->toArray()); - $this->fail('Expected exception not thrown'); - } catch (Error $e) { - $this->assertEquals($expected, Error::formatError($e)); - } + $result = Executor::execute($this->schema(), $ast, null, null, ['input' => null]); + $expected = [ + 'errors' => [ + [ + 'message' => + 'Variable "$input" got invalid value null.' . "\n" . + 'Expected "[String]!", found null.', + 'locations' => [['line' => 2, 'column' => 17]] + ] + ] + ]; + $this->assertEquals($expected, $result->toArray()); } /** @@ -633,18 +635,18 @@ class VariablesTest extends \PHPUnit_Framework_TestCase } '; $ast = Parser::parse($doc); - $expected = FormattedError::create( - 'Variable "$input" got invalid value ["A",null,"B"].' . "\n" . - 'In element #1: Expected "String!", found null.', - [new SourceLocation(2, 17)] - ); - - try { - Executor::execute($this->schema(), $ast, null, null, ['input' => ['A', null, 'B']]); - $this->fail('Expected exception not thrown'); - } catch (Error $e) { - $this->assertEquals($expected, Error::formatError($e)); - } + $result = Executor::execute($this->schema(), $ast, null, null, ['input' => ['A', null, 'B']]); + $expected = [ + 'errors' => [ + [ + 'message' => + 'Variable "$input" got invalid value ["A",null,"B"].' . "\n" . + 'In element #1: Expected "String!", found null.', + 'locations' => [ ['line' => 2, 'column' => 17] ] + ] + ] + ]; + $this->assertEquals($expected, $result->toArray()); } /** @@ -658,17 +660,18 @@ class VariablesTest extends \PHPUnit_Framework_TestCase } '; $ast = Parser::parse($doc); - $expected = FormattedError::create( - 'Variable "$input" got invalid value null.' . "\n" . - 'Expected "[String!]!", found null.', - [new SourceLocation(2, 17)] - ); - try { - Executor::execute($this->schema(), $ast, null, null, ['input' => null]); - $this->fail('Expected exception not thrown'); - } catch (Error $e) { - $this->assertEquals($expected, Error::formatError($e)); - } + $result = Executor::execute($this->schema(), $ast, null, null, ['input' => null]); + $expected = [ + 'errors' => [ + [ + 'message' => + 'Variable "$input" got invalid value null.' . "\n" . + 'Expected "[String!]!", found null.', + 'locations' => [ ['line' => 2, 'column' => 17] ] + ] + ] + ]; + $this->assertEquals($expected, $result->toArray()); } /** @@ -697,17 +700,18 @@ class VariablesTest extends \PHPUnit_Framework_TestCase } '; $ast = Parser::parse($doc); - $expected = FormattedError::create( - 'Variable "$input" got invalid value ["A",null,"B"].'."\n". - 'In element #1: Expected "String!", found null.', - [new SourceLocation(2, 17)] - ); - try { - Executor::execute($this->schema(), $ast, null, null, ['input' => ['A', null, 'B']]); - $this->fail('Expected exception not thrown'); - } catch (Error $e) { - $this->assertEquals($expected, Error::formatError($e)); - } + $result = Executor::execute($this->schema(), $ast, null, null, ['input' => ['A', null, 'B']]); + $expected = [ + 'errors' => [ + [ + 'message' => + 'Variable "$input" got invalid value ["A",null,"B"].'."\n". + 'In element #1: Expected "String!", found null.', + 'locations' => [ ['line' => 2, 'column' => 17] ] + ] + ] + ]; + $this->assertEquals($expected, $result->toArray()); } /** @@ -722,19 +726,18 @@ class VariablesTest extends \PHPUnit_Framework_TestCase '; $ast = Parser::parse($doc); $vars = [ 'input' => [ 'list' => [ 'A', 'B' ] ] ]; - - try { - Executor::execute($this->schema(), $ast, null, null, $vars); - $this->fail('Expected exception not thrown'); - } catch (Error $error) { - $expected = [ - 'message' => - 'Variable "$input" expected value of type "TestType!" which cannot ' . - 'be used as an input type.', - 'locations' => [['line' => 2, 'column' => 25]] - ]; - $this->assertEquals($expected, Error::formatError($error)); - } + $result = Executor::execute($this->schema(), $ast, null, null, $vars); + $expected = [ + 'errors' => [ + [ + 'message' => + 'Variable "$input" expected value of type "TestType!" which cannot ' . + 'be used as an input type.', + 'locations' => [['line' => 2, 'column' => 25]] + ] + ] + ]; + $this->assertEquals($expected, $result->toArray()); } /** @@ -750,17 +753,18 @@ class VariablesTest extends \PHPUnit_Framework_TestCase $ast = Parser::parse($doc); $vars = ['input' => 'whoknows']; - try { - Executor::execute($this->schema(), $ast, null, null, $vars); - $this->fail('Expected exception not thrown'); - } catch (Error $error) { - $expected = FormattedError::create( - 'Variable "$input" expected value of type "UnknownType!" which ' . - 'cannot be used as an input type.', - [new SourceLocation(2, 25)] - ); - $this->assertEquals($expected, Error::formatError($error)); - } + $result = Executor::execute($this->schema(), $ast, null, null, $vars); + $expected = [ + 'errors' => [ + [ + 'message' => + 'Variable "$input" expected value of type "UnknownType!" which ' . + 'cannot be used as an input type.', + 'locations' => [['line' => 2, 'column' => 25]] + ] + ] + ]; + $this->assertEquals($expected, $result->toArray()); } // Describe: Execute: Uses argument default values diff --git a/tests/Language/ParserTest.php b/tests/Language/ParserTest.php index 8c7f1d4..b63b263 100644 --- a/tests/Language/ParserTest.php +++ b/tests/Language/ParserTest.php @@ -1,12 +1,12 @@ fail('Expected exception was not thrown'); + } catch (InvariantViolation $e) { + $this->assertEquals('GraphQL query body is expected to be string, but got NULL', $e->getMessage()); + } + + try { + Parser::parse(['a' => 'b']); + $this->fail('Expected exception was not thrown'); + } catch (InvariantViolation $e) { + $this->assertEquals('GraphQL query body is expected to be string, but got array', $e->getMessage()); + } + + try { + Parser::parse(new \stdClass()); + $this->fail('Expected exception was not thrown'); + } catch (InvariantViolation $e) { + $this->assertEquals('GraphQL query body is expected to be string, but got stdClass', $e->getMessage()); + } + } + /** * @it parse provides useful errors */