Additional checks and tests for situations when user-land code returns duplicate type instances

src/Executor/Executor.php

@@ -960,6 +960,18 @@ class Executor
             return $this->completeListValue($returnType, $fieldNodes, $info, $path, $result);
         }
 
+        // Account for invalid schema definition when typeLoader returns different
+        // instance than `resolveType` or $field->getType() or $arg->getType()
+        if ($returnType !== $this->exeContext->schema->getType($returnType->name)) {
+            $hint = "";
+            if ($this->exeContext->schema->getConfig()->typeLoader) {
+                $hint = "Make sure that type loader returns the same instance as defined in {$info->parentType}.{$info->fieldName}";
+            }
+            throw new InvariantViolation(
+                "Schema must contain unique named types but contains multiple types named \"$returnType\". $hint"
+            );
+        }
+
         // If field type is Scalar or Enum, serialize to a valid value, returning
         // null if serialization is not possible.
         if ($returnType instanceof LeafType) {
@@ -1130,18 +1142,24 @@ class Executor
             $runtimeTypeOrName;
 
         if (!$runtimeType instanceof ObjectType) {
-            throw new Error(
+            throw new InvariantViolation(
                 "Abstract type {$returnType} must resolve to an Object type at runtime " .
                 "for field {$info->parentType}.{$info->fieldName} with " .
-                'value "' . Utils::printSafe($result) . '", received "'. Utils::printSafe($runtimeType) . '".',
+                'value "' . Utils::printSafe($result) . '", received "'. Utils::printSafe($runtimeType) . '".'
-                $fieldNodes
             );
         }
 
         if (!$this->exeContext->schema->isPossibleType($returnType, $runtimeType)) {
-            throw new Error(
+            throw new InvariantViolation(
-                "Runtime Object type \"$runtimeType\" is not a possible type for \"$returnType\".",
+                "Runtime Object type \"$runtimeType\" is not a possible type for \"$returnType\"."
-                $fieldNodes
+            );
+        }
+
+        if ($runtimeType !== $this->exeContext->schema->getType($runtimeType->name)) {
+            throw new InvariantViolation(
+                "Schema must contain unique named types but contains multiple types named \"$runtimeType\". ".
+                "Make sure that `resolveType` function of abstract type \"{$returnType}\" returns the same ".
+                "type instance as referenced anywhere else within the schema."
             );
         }
 

tests/Executor/AbstractPromiseTest.php

@@ -361,7 +361,7 @@ class AbstractPromiseTest extends \PHPUnit_Framework_TestCase
       }
     }';
 
-        $result = GraphQL::execute($schema, $query);
+        $result = GraphQL::executeAndReturnResult($schema, $query)->toArray(true);
 
         $expected = [
             'data' => [
@@ -373,7 +373,7 @@ class AbstractPromiseTest extends \PHPUnit_Framework_TestCase
             ],
             'errors' => [
                 [
-                    'message' => 'Runtime Object type "Human" is not a possible type for "Pet".',
+                    'debugMessage' => 'Runtime Object type "Human" is not a possible type for "Pet".',
                     'locations' => [['line' => 2, 'column' => 7]],
                     'path' => ['pets', 2]
                 ],
@@ -462,7 +462,7 @@ class AbstractPromiseTest extends \PHPUnit_Framework_TestCase
       }
     }';
 
-        $result = GraphQL::execute($schema, $query);
+        $result = GraphQL::executeAndReturnResult($schema, $query)->toArray(true);
 
         $expected = [
             'data' => [
@@ -474,7 +474,7 @@ class AbstractPromiseTest extends \PHPUnit_Framework_TestCase
             ],
             'errors' => [
                 [
-                    'message' => 'Runtime Object type "Human" is not a possible type for "Pet".',
+                    'debugMessage' => 'Runtime Object type "Human" is not a possible type for "Pet".',
                     'locations' => [['line' => 2, 'column' => 7]],
                     'path' => ['pets', 2]
                 ]

tests/Executor/AbstractTest.php

@@ -251,12 +251,12 @@ class AbstractTest extends \PHPUnit_Framework_TestCase
                 ]
             ],
             'errors' => [[
-                'message' => 'Runtime Object type "Human" is not a possible type for "Pet".',
+                'debugMessage' => 'Runtime Object type "Human" is not a possible type for "Pet".',
                 'locations' => [['line' => 2, 'column' => 11]],
                 'path' => ['pets', 2]
             ]]
         ];
-        $actual = GraphQL::execute($schema, $query);
+        $actual = GraphQL::executeAndReturnResult($schema, $query)->toArray(true);
 
         $this->assertArraySubset($expected, $actual);
     }
@@ -336,7 +336,7 @@ class AbstractTest extends \PHPUnit_Framework_TestCase
           }
         }';
 
-        $result = GraphQL::execute($schema, $query);
+        $result = GraphQL::executeAndReturnResult($schema, $query)->toArray(true);
         $expected = [
             'data' => [
                 'pets' => [
@@ -348,7 +348,7 @@ class AbstractTest extends \PHPUnit_Framework_TestCase
                 ]
             ],
             'errors' => [[
-                'message' => 'Runtime Object type "Human" is not a possible type for "Pet".',
+                'debugMessage' => 'Runtime Object type "Human" is not a possible type for "Pet".',
                 'locations' => [['line' => 2, 'column' => 11]],
                 'path' => ['pets', 2]
             ]]
@@ -432,4 +432,59 @@ class AbstractTest extends \PHPUnit_Framework_TestCase
             ]
         ], $result);
     }
+
+    public function testHintsOnConflictingTypeInstancesInResolveType()
+    {
+        $createTest = function() use (&$iface) {
+            return new ObjectType([
+                'name' => 'Test',
+                'fields' => [
+                    'a' => Type::string()
+                ],
+                'interfaces' => function() use ($iface) {
+                    return [$iface];
+                }
+            ]);
+        };
+
+        $iface = new InterfaceType([
+            'name' => 'Node',
+            'fields' => [
+                'a' => Type::string()
+            ],
+            'resolveType' => function() use (&$createTest) {
+                return $createTest();
+            }
+        ]);
+
+        $query = new ObjectType([
+            'name' => 'Query',
+            'fields' => [
+                'node' => $iface,
+                'test' => $createTest()
+            ]
+        ]);
+
+        $schema = new Schema([
+            'query' => $query,
+        ]);
+        $schema->assertValid();
+
+        $query = '
+            {
+                node {
+                    a
+                }
+            }
+        ';
+
+        $result = Executor::execute($schema, Parser::parse($query), ['node' => ['a' => 'value']]);
+
+        $this->assertEquals(
+            'Schema must contain unique named types but contains multiple types named "Test". '.
+            'Make sure that `resolveType` function of abstract type "Node" returns the same type instance '.
+            'as referenced anywhere else within the schema.',
+            $result->errors[0]->getMessage()
+        );
+    }
 }

tests/Executor/ExecutorLazySchemaTest.php

@@ -3,6 +3,7 @@ namespace GraphQL\Tests\Executor;
 
 require_once __DIR__ . '/TestClasses.php';
 
+use GraphQL\Error\InvariantViolation;
 use GraphQL\Error\Warning;
 use GraphQL\Executor\ExecutionResult;
 use GraphQL\Executor\Executor;
@@ -113,4 +114,59 @@ class ExecutorLazySchemaTest extends \PHPUnit_Framework_TestCase
             'Make sure your `resolveType` always returns valid implementation or throws.',
             $result->errors[0]->getMessage());
     }
+
+    public function testHintsOnConflictingTypeInstancesInDefinitions()
+    {
+        $calls = [];
+        $typeLoader = function($name) use (&$calls) {
+            $calls[] = $name;
+            switch ($name) {
+                case 'Test':
+                    return new ObjectType([
+                        'name' => 'Test',
+                        'fields' => function() {
+                            return [
+                                'test' => Type::string(),
+                            ];
+                        }
+                    ]);
+                default:
+                    return null;
+            }
+        };
+        $query = new ObjectType([
+            'name' => 'Query',
+            'fields' => function() use ($typeLoader) {
+                return [
+                    'test' => $typeLoader('Test')
+                ];
+            }
+        ]);
+        $schema = new Schema([
+            'query' => $query,
+            'typeLoader' => $typeLoader
+        ]);
+
+        $query = '
+            {
+                test {
+                    test
+                }
+            }
+        ';
+
+        $this->assertEquals([], $calls);
+        $result = Executor::execute($schema, Parser::parse($query), ['test' => ['test' => 'value']]);
+        $this->assertEquals(['Test', 'Test'], $calls);
+
+        $this->assertEquals(
+            'Schema must contain unique named types but contains multiple types named "Test". '.
+            'Make sure that type loader returns the same instance as defined in Query.test',
+            $result->errors[0]->getMessage()
+        );
+        $this->assertInstanceOf(
+            InvariantViolation::class,
+            $result->errors[0]->getPrevious()
+        );
+    }
 }