From b9d3a117850f1a7093f411186b1eb48981e0fa9b Mon Sep 17 00:00:00 2001 From: Vladimir Razuvaev Date: Mon, 14 Aug 2017 20:41:08 +0700 Subject: [PATCH] Extracted lazy schema test; minor related refactoring --- src/Error/Warning.php | 2 +- src/Executor/Executor.php | 6 +- src/Type/Schema.php | 59 ++++++----- tests/Executor/AbstractPromiseTest.php | 8 +- tests/Executor/AbstractTest.php | 18 +--- tests/Executor/ExecutorLazySchemaTest.php | 116 ++++++++++++++++++++++ tests/Executor/UnionInterfaceTest.php | 12 +-- tests/Type/TypeLoaderTest.php | 23 +++++ 8 files changed, 186 insertions(+), 58 deletions(-) create mode 100644 tests/Executor/ExecutorLazySchemaTest.php diff --git a/src/Error/Warning.php b/src/Error/Warning.php index 5ccf04b..c27e7b5 100644 --- a/src/Error/Warning.php +++ b/src/Error/Warning.php @@ -6,7 +6,7 @@ final class Warning const NAME_WARNING = 1; const ASSIGN_WARNING = 2; const CONFIG_WARNING = 4; - const RESOLVE_TYPE_WARNING = 8; + const FULL_SCHEMA_SCAN_WARNING = 8; const CONFIG_DEPRECATION_WARNING = 16; const NOT_A_TYPE = 32; diff --git a/src/Executor/Executor.php b/src/Executor/Executor.php index 5ac9733..cd660ea 100644 --- a/src/Executor/Executor.php +++ b/src/Executor/Executor.php @@ -1062,13 +1062,13 @@ class Executor $runtimeType = $returnType->resolveType($result, $exeContext->contextValue, $info); if (null === $runtimeType) { - if ($returnType instanceof InterfaceType) { + if ($returnType instanceof InterfaceType && $info->schema->getConfig()->typeLoader) { Warning::warnOnce( "GraphQL Interface Type `{$returnType->name}` returned `null` from it`s `resolveType` function ". 'for value: ' . Utils::printSafe($result) . '. Switching to slow resolution method using `isTypeOf` ' . - 'of all possible implementations. It degrades query performance significantly. '. + 'of all possible implementations. It requires full schema scan and degrades query performance significantly. '. ' Make sure your `resolveType` always returns valid implementation or throws.', - Warning::RESOLVE_TYPE_WARNING + Warning::FULL_SCHEMA_SCAN_WARNING ); } $runtimeType = self::defaultTypeResolver($result, $exeContext->contextValue, $info, $returnType); diff --git a/src/Type/Schema.php b/src/Type/Schema.php index 92dbd94..8505557 100644 --- a/src/Type/Schema.php +++ b/src/Type/Schema.php @@ -123,6 +123,7 @@ class Schema $this->resolvedTypes[$config->subscription->name] = $config->subscription; } if (!$this->config->typeLoader) { + // Perform full scan of the schema $this->getTypeMap(); } } @@ -199,40 +200,44 @@ class Schema */ private function collectAllTypes() { - $initialTypes = $this->resolvedTypes; - $typeMap = []; - foreach ($initialTypes as $type) { + foreach ($this->resolvedTypes as $type) { $typeMap = TypeInfo::extractTypes($type, $typeMap); } + foreach ($this->resolveAdditionalTypes() as $type) { + $typeMap = TypeInfo::extractTypes($type, $typeMap); + } + return $typeMap + Type::getInternalTypes() + Introspection::getTypes(); + } - if ($this->config->types) { - $types = $this->config->types; + /** + * @return \Generator + */ + private function resolveAdditionalTypes() + { + $types = $this->config->types ?: []; - if (is_callable($types)) { - $types = $types(); - } - - if (!is_array($types) && !$types instanceof \Traversable) { - throw new InvariantViolation(sprintf( - 'Schema types callable must return array or instance of Traversable but got: %s', - Utils::getVariableType($types) - )); - } - - foreach ($types as $index => $type) { - if (!$type instanceof Type) { - throw new InvariantViolation( - 'Each entry of schema types must be instance of GraphQL\Type\Definition\Type but entry at %s is %s', - $index, - Utils::printSafe($type) - ); - } - $typeMap = TypeInfo::extractTypes($type, $typeMap); - } + if (is_callable($types)) { + $types = $types(); } - return $typeMap + Type::getInternalTypes() + Introspection::getTypes(); + if (!is_array($types) && !$types instanceof \Traversable) { + throw new InvariantViolation(sprintf( + 'Schema types callable must return array or instance of Traversable but got: %s', + Utils::getVariableType($types) + )); + } + + foreach ($types as $index => $type) { + if (!$type instanceof Type) { + throw new InvariantViolation( + 'Each entry of schema types must be instance of GraphQL\Type\Definition\Type but entry at %s is %s', + $index, + Utils::printSafe($type) + ); + } + yield $type; + } } /** diff --git a/tests/Executor/AbstractPromiseTest.php b/tests/Executor/AbstractPromiseTest.php index 8925b05..363c80c 100644 --- a/tests/Executor/AbstractPromiseTest.php +++ b/tests/Executor/AbstractPromiseTest.php @@ -87,9 +87,9 @@ class AbstractPromiseTest extends \PHPUnit_Framework_TestCase } }'; - Warning::suppress(Warning::RESOLVE_TYPE_WARNING); + Warning::suppress(Warning::FULL_SCHEMA_SCAN_WARNING); $result = GraphQL::execute($schema, $query); - Warning::enable(Warning::RESOLVE_TYPE_WARNING); + Warning::enable(Warning::FULL_SCHEMA_SCAN_WARNING); $expected = [ 'data' => [ @@ -174,9 +174,9 @@ class AbstractPromiseTest extends \PHPUnit_Framework_TestCase } }'; - Warning::suppress(Warning::RESOLVE_TYPE_WARNING); + Warning::suppress(Warning::FULL_SCHEMA_SCAN_WARNING); $result = GraphQL::execute($schema, $query); - Warning::enable(Warning::RESOLVE_TYPE_WARNING); + Warning::enable(Warning::FULL_SCHEMA_SCAN_WARNING); $expected = [ 'data' => [ diff --git a/tests/Executor/AbstractTest.php b/tests/Executor/AbstractTest.php index 024a5bf..dcc9129 100644 --- a/tests/Executor/AbstractTest.php +++ b/tests/Executor/AbstractTest.php @@ -3,14 +3,11 @@ namespace GraphQL\Tests\Executor; require_once __DIR__ . '/TestClasses.php'; -use GraphQL\Error\Warning; use GraphQL\Executor\ExecutionResult; use GraphQL\Executor\Executor; -use GraphQL\Error\FormattedError; use GraphQL\GraphQL; use GraphQL\Language\Parser; -use GraphQL\Language\SourceLocation; -use GraphQL\Schema; +use GraphQL\Type\Schema; use GraphQL\Type\Definition\InterfaceType; use GraphQL\Type\Definition\ObjectType; use GraphQL\Type\Definition\Type; @@ -90,21 +87,8 @@ class AbstractTest extends \PHPUnit_Framework_TestCase ] ]); - Warning::suppress(Warning::RESOLVE_TYPE_WARNING); $result = Executor::execute($schema, Parser::parse($query)); $this->assertEquals($expected, $result); - - Warning::enable(Warning::RESOLVE_TYPE_WARNING); - $result = Executor::execute($schema, Parser::parse($query)); - $this->assertEquals(1, count($result->errors)); - $this->assertInstanceOf('PHPUnit_Framework_Error_Warning', $result->errors[0]->getPrevious()); - - $this->assertEquals( - 'GraphQL Interface Type `Pet` returned `null` from it`s `resolveType` function for value: '. - 'instance of GraphQL\Tests\Executor\Dog. Switching to slow resolution method using `isTypeOf` of '. - 'all possible implementations. It degrades query performance significantly. '. - 'Make sure your `resolveType` always returns valid implementation or throws.', - $result->errors[0]->getMessage()); } /** diff --git a/tests/Executor/ExecutorLazySchemaTest.php b/tests/Executor/ExecutorLazySchemaTest.php new file mode 100644 index 0000000..5e8b97c --- /dev/null +++ b/tests/Executor/ExecutorLazySchemaTest.php @@ -0,0 +1,116 @@ + 'Pet', + 'fields' => function() { + return [ + 'name' => ['type' => Type::string()] + ]; + } + ]); + + // Added to interface type when defined + $dogType = new ObjectType([ + 'name' => 'Dog', + 'interfaces' => [$petType], + 'isTypeOf' => function($obj) { return $obj instanceof Dog; }, + 'fields' => function() { + return [ + 'name' => ['type' => Type::string()], + 'woofs' => ['type' => Type::boolean()] + ]; + } + ]); + + $catType = new ObjectType([ + 'name' => 'Cat', + 'interfaces' => [$petType], + 'isTypeOf' => function ($obj) { + return $obj instanceof Cat; + }, + 'fields' => function() { + return [ + 'name' => ['type' => Type::string()], + 'meows' => ['type' => Type::boolean()], + ]; + } + ]); + + $schema = new Schema([ + 'query' => new ObjectType([ + 'name' => 'Query', + 'fields' => [ + 'pets' => [ + 'type' => Type::listOf($petType), + 'resolve' => function () { + return [new Dog('Odie', true), new Cat('Garfield', false)]; + } + ] + ] + ]), + 'types' => [$catType, $dogType], + 'typeLoader' => function($name) use ($dogType, $petType, $catType) { + switch ($name) { + case 'Dog': + return $dogType; + case 'Pet': + return $petType; + case 'Cat': + return $catType; + } + } + ]); + + $query = '{ + pets { + name + ... on Dog { + woofs + } + ... on Cat { + meows + } + } + }'; + + $expected = new ExecutionResult([ + 'pets' => [ + ['name' => 'Odie', 'woofs' => true], + ['name' => 'Garfield', 'meows' => false] + ], + ]); + + Warning::suppress(Warning::FULL_SCHEMA_SCAN_WARNING); + $result = Executor::execute($schema, Parser::parse($query)); + $this->assertEquals($expected, $result); + + Warning::enable(Warning::FULL_SCHEMA_SCAN_WARNING); + $result = Executor::execute($schema, Parser::parse($query)); + $this->assertEquals(1, count($result->errors)); + $this->assertInstanceOf('PHPUnit_Framework_Error_Warning', $result->errors[0]->getPrevious()); + + $this->assertEquals( + 'GraphQL Interface Type `Pet` returned `null` from it`s `resolveType` function for value: instance of '. + 'GraphQL\Tests\Executor\Dog. Switching to slow resolution method using `isTypeOf` of all possible '. + 'implementations. It requires full schema scan and degrades query performance significantly. '. + 'Make sure your `resolveType` always returns valid implementation or throws.', + $result->errors[0]->getMessage()); + } +} diff --git a/tests/Executor/UnionInterfaceTest.php b/tests/Executor/UnionInterfaceTest.php index 5c8894e..827ea9e 100644 --- a/tests/Executor/UnionInterfaceTest.php +++ b/tests/Executor/UnionInterfaceTest.php @@ -256,9 +256,9 @@ class UnionInterfaceTest extends \PHPUnit_Framework_TestCase ] ]; - Warning::suppress(Warning::RESOLVE_TYPE_WARNING); + Warning::suppress(Warning::FULL_SCHEMA_SCAN_WARNING); $this->assertEquals($expected, Executor::execute($this->schema, $ast, $this->john)->toArray()); - Warning::enable(Warning::RESOLVE_TYPE_WARNING); + Warning::enable(Warning::FULL_SCHEMA_SCAN_WARNING); } /** @@ -294,9 +294,9 @@ class UnionInterfaceTest extends \PHPUnit_Framework_TestCase ] ]; - Warning::suppress(Warning::RESOLVE_TYPE_WARNING); + Warning::suppress(Warning::FULL_SCHEMA_SCAN_WARNING); $this->assertEquals($expected, Executor::execute($this->schema, $ast, $this->john)->toArray()); - Warning::enable(Warning::RESOLVE_TYPE_WARNING); + Warning::enable(Warning::FULL_SCHEMA_SCAN_WARNING); } /** @@ -351,9 +351,9 @@ class UnionInterfaceTest extends \PHPUnit_Framework_TestCase ] ]; - Warning::suppress(Warning::RESOLVE_TYPE_WARNING); + Warning::suppress(Warning::FULL_SCHEMA_SCAN_WARNING); $this->assertEquals($expected, Executor::execute($this->schema, $ast, $this->john)->toArray()); - Warning::enable(Warning::RESOLVE_TYPE_WARNING); + Warning::enable(Warning::FULL_SCHEMA_SCAN_WARNING); } /** diff --git a/tests/Type/TypeLoaderTest.php b/tests/Type/TypeLoaderTest.php index 14787b4..5521123 100644 --- a/tests/Type/TypeLoaderTest.php +++ b/tests/Type/TypeLoaderTest.php @@ -235,6 +235,10 @@ class TypeLoaderTest extends \PHPUnit_Framework_TestCase $input = $schema->getType('PostStoryMutationInput'); $this->assertSame($this->postStoryMutationInput, $input); $this->assertEquals(['Node', 'Content', 'PostStoryMutationInput'], $this->calls); + + $result = $schema->isPossibleType($this->node, $this->blogStory); + $this->assertTrue($result); + $this->assertEquals(['Node', 'Content', 'PostStoryMutationInput'], $this->calls); } public function testOnlyCallsLoaderOnce() @@ -316,4 +320,23 @@ class TypeLoaderTest extends \PHPUnit_Framework_TestCase $schema->getType('Node'); } + + public function testReturnsIdenticalResults() + { + $withoutLoader = new Schema([ + 'query' => $this->query, + 'mutation' => $this->mutation + ]); + + $withLoader = new Schema([ + 'query' => $this->query, + 'mutation' => $this->mutation, + 'typeLoader' => $this->typeLoader + ]); + + $this->assertSame($withoutLoader->getQueryType(), $withLoader->getQueryType()); + $this->assertSame($withoutLoader->getMutationType(), $withLoader->getMutationType()); + $this->assertSame($withoutLoader->getType('BlogStory'), $withLoader->getType('BlogStory')); + $this->assertSame($withoutLoader->getDirectives(), $withLoader->getDirectives()); + } }