Spec compliance: errors in buildExecutionContext() are caught and included in result rather than thrown

Author: vladar

src/Executor/Executor.php

@@ -115,17 +115,26 @@ public static function execute(
             );
         }
 
-        $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 @@ private function executeOperation(OperationDefinitionNode $operation, $rootValue
         $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;
+        }
     }
 
 

tests/Executor/ExecutorTest.php

@@ -589,9 +589,9 @@ public function testUsesTheNamedOperationIfOperationNameIsProvided()
     }
 
     /**
-     * @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 @@ public function testThrowsIfNoOperationIsProvided()
             ])
         ]);
 
-        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 @@ public function testFailsToExecuteQueryContainingTypeDefinition()
             '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());
     }
 
     /**