From f1ee6a8e8813b7f01b6f957e222e5f0d6141d472 Mon Sep 17 00:00:00 2001 From: abdullahseba Date: Wed, 8 Sep 2021 13:31:50 +0100 Subject: [PATCH 01/13] Add class-defined scalar test case for SchemaExtenderTest --- tests/Utils/SchemaExtenderTest.php | 32 ++++++++++++++++++++++++++++- tests/Utils/SomeScalarClassType.php | 28 +++++++++++++++++++++++++ 2 files changed, 59 insertions(+), 1 deletion(-) create mode 100644 tests/Utils/SomeScalarClassType.php diff --git a/tests/Utils/SchemaExtenderTest.php b/tests/Utils/SchemaExtenderTest.php index 6f7c09109..b5d660dc3 100644 --- a/tests/Utils/SchemaExtenderTest.php +++ b/tests/Utils/SchemaExtenderTest.php @@ -60,6 +60,7 @@ public function setUp(): void { parent::setUp(); + //Inline definition. $SomeScalarType = new CustomScalarType([ 'name' => 'SomeScalar', 'serialize' => static function ($x) { @@ -67,6 +68,9 @@ public function setUp(): void }, ]); + //Class definition. + $SomeClassScalarType = new SomeScalarClassType(); + $SomeInterfaceType = new InterfaceType([ 'name' => 'SomeInterface', 'fields' => static function () use (&$SomeInterfaceType): array { @@ -167,10 +171,11 @@ public function setUp(): void $this->testSchema = new Schema([ 'query' => new ObjectType([ 'name' => 'Query', - 'fields' => static function () use ($FooType, $SomeScalarType, $SomeUnionType, $SomeEnumType, $SomeInterfaceType, $SomeInputType): array { + 'fields' => static function () use ($FooType, $SomeScalarType, $SomeClassScalarType, $SomeUnionType, $SomeEnumType, $SomeInterfaceType, $SomeInputType): array { return [ 'foo' => [ 'type' => $FooType ], 'someScalar' => [ 'type' => $SomeScalarType ], + 'SomeScalarClass' => [ 'type' => $SomeClassScalarType ], 'someUnion' => [ 'type' => $SomeUnionType ], 'someEnum' => [ 'type' => $SomeEnumType ], 'someInterface' => [ @@ -2106,4 +2111,29 @@ public function testSupportsTypeConfigDecorator(): void self::assertSame(['data' => ['hello' => 'Hello World!', 'foo' => ['value' => 'bar']]], $result->toArray()); } + + /** + * Tests both custom inline and class scalar definitions. + * + * Ensures the correct instance is maintained before and after schema extension. + * Should probably be incorporated into testExtendsWithoutAlteringOriginalSchema(). + */ + public function testExtendsWithoutAlteringOriginalScalarTypes(): void + { + $extendedSchema = $this->extendTestSchema(' + extend type Foo { + bar: Bar + } + '); + + $someScalar = $this->testSchema->getType('SomeScalar'); + $someScalarClass = $this->testSchema->getType('SomeScalarClass'); + $extendedSomeScalar = $extendedSchema->getType('SomeScalar'); + $extendedSomeScalarClass = $extendedSchema->getType('SomeScalarClass'); + + self::assertInstanceOf(CustomScalarType::class, $someScalar); + self::assertInstanceOf(SomeScalarClassType::class, $someScalarClass); + self::assertInstanceOf(CustomScalarType::class, $extendedSomeScalar); + self::assertInstanceOf(SomeScalarClassType::class, $extendedSomeScalarClass); + } } diff --git a/tests/Utils/SomeScalarClassType.php b/tests/Utils/SomeScalarClassType.php new file mode 100644 index 000000000..e1846e8f9 --- /dev/null +++ b/tests/Utils/SomeScalarClassType.php @@ -0,0 +1,28 @@ +value; + } +} From c6aa3101e8225dabab21edd5a3f9049a74cc5fc4 Mon Sep 17 00:00:00 2001 From: Benedikt Franke Date: Wed, 8 Sep 2021 17:51:23 +0200 Subject: [PATCH 02/13] Fix formatting --- src/Language/AST/NodeList.php | 6 ++++-- src/Type/Schema.php | 3 +-- tests/Utils/SomeScalarClassType.php | 1 + 3 files changed, 6 insertions(+), 4 deletions(-) diff --git a/src/Language/AST/NodeList.php b/src/Language/AST/NodeList.php index 7ecef7a1d..30a886d8a 100644 --- a/src/Language/AST/NodeList.php +++ b/src/Language/AST/NodeList.php @@ -32,8 +32,8 @@ class NodeList implements ArrayAccess, IteratorAggregate, Countable /** * @param array> $nodes - * @phpstan-param array> $nodes * + * @phpstan-param array> $nodes * @phpstan-return self */ public static function create(array $nodes): self @@ -43,6 +43,7 @@ public static function create(array $nodes): self /** * @param array $nodes + * * @phpstan-param array> $nodes */ public function __construct(array $nodes) @@ -90,6 +91,7 @@ public function offsetGet($offset)// : Node /** * @param int|string|null $offset * @param Node|array $value + * * @phpstan-param T|array $value */ #[ReturnTypeWillChange] @@ -134,8 +136,8 @@ public function splice(int $offset, int $length, $replacement = null): NodeList /** * @param NodeList|array> $list - * @phpstan-param NodeList|array $list * + * @phpstan-param NodeList|array $list * @phpstan-return NodeList */ public function merge($list): NodeList diff --git a/src/Type/Schema.php b/src/Type/Schema.php index 5fbb0b224..9328509b5 100644 --- a/src/Type/Schema.php +++ b/src/Type/Schema.php @@ -395,10 +395,9 @@ private function defaultTypeLoader(string $typeName): ?Type /** * @param Type|callable $type - * @phpstan-param T|callable():T $type * + * @phpstan-param T|callable():T $type * @phpstan-return T - * * @template T of Type */ public static function resolveType($type): Type diff --git a/tests/Utils/SomeScalarClassType.php b/tests/Utils/SomeScalarClassType.php index e1846e8f9..a4587362f 100644 --- a/tests/Utils/SomeScalarClassType.php +++ b/tests/Utils/SomeScalarClassType.php @@ -16,6 +16,7 @@ public function serialize($value): string { return $value; } + public function parseValue($value): string { return $value; From 42215cc5b3f4d3832d75bb13dee7ad72728fbe8d Mon Sep 17 00:00:00 2001 From: Benedikt Franke Date: Wed, 8 Sep 2021 17:51:34 +0200 Subject: [PATCH 03/13] Clarify types --- src/Utils/SchemaExtender.php | 122 +++++++++++++++-------------- tests/Utils/SchemaExtenderTest.php | 31 ++++---- 2 files changed, 79 insertions(+), 74 deletions(-) diff --git a/src/Utils/SchemaExtender.php b/src/Utils/SchemaExtender.php index f8c0132dc..f738c832d 100644 --- a/src/Utils/SchemaExtender.php +++ b/src/Utils/SchemaExtender.php @@ -45,17 +45,16 @@ class SchemaExtender { public const SCHEMA_EXTENSION = 'SchemaExtension'; - /** @var Type[] */ - protected static $extendTypeCache; + /** @var array */ + protected static array $extendTypeCache; - /** @var mixed[] */ - protected static $typeExtensionsMap; + /** @var array> */ + protected static array $typeExtensionsMap; - /** @var ASTDefinitionBuilder */ - protected static $astBuilder; + protected static ASTDefinitionBuilder $astBuilder; /** - * @return TypeExtensionNode[]|null + * @return array|null */ protected static function getExtensionASTNodes(NamedType $type): ?array { @@ -147,9 +146,7 @@ protected static function extendUnionType(UnionType $type): UnionType return new UnionType([ 'name' => $type->name, 'description' => $type->description, - 'types' => static function () use ($type): array { - return static::extendPossibleTypes($type); - }, + 'types' => static fn (): array => static::extendPossibleTypes($type), 'astNode' => $type->astNode, 'resolveType' => $type->config['resolveType'] ?? null, 'extensionASTNodes' => static::getExtensionASTNodes($type), @@ -172,16 +169,14 @@ protected static function extendInputObjectType(InputObjectType $type): InputObj return new InputObjectType([ 'name' => $type->name, 'description' => $type->description, - 'fields' => static function () use ($type): array { - return static::extendInputFieldMap($type); - }, + 'fields' => static fn (): array => static::extendInputFieldMap($type), 'astNode' => $type->astNode, 'extensionASTNodes' => static::getExtensionASTNodes($type), ]); } /** - * @return mixed[] + * @return array> */ protected static function extendInputFieldMap(InputObjectType $type): array { @@ -203,6 +198,7 @@ protected static function extendInputFieldMap(InputObjectType $type): array $extensions = static::$typeExtensionsMap[$type->name] ?? null; if ($extensions !== null) { + /** @var InputObjectTypeExtensionNode $extension */ foreach ($extensions as $extension) { foreach ($extension->fields as $field) { $fieldName = $field->name->value; @@ -219,7 +215,7 @@ protected static function extendInputFieldMap(InputObjectType $type): array } /** - * @return mixed[] + * @return array> */ protected static function extendValueMap(EnumType $type): array { @@ -242,6 +238,7 @@ protected static function extendValueMap(EnumType $type): array $extensions = static::$typeExtensionsMap[$type->name] ?? null; if ($extensions !== null) { + /** @var EnumTypeExtensionNode $extension */ foreach ($extensions as $extension) { foreach ($extension->values as $value) { $valueName = $value->name->value; @@ -258,7 +255,7 @@ protected static function extendValueMap(EnumType $type): array } /** - * @return ObjectType[] + * @return array Should be ObjectType, will be caught in schema validation */ protected static function extendPossibleTypes(UnionType $type): array { @@ -269,6 +266,7 @@ protected static function extendPossibleTypes(UnionType $type): array $extensions = static::$typeExtensionsMap[$type->name] ?? null; if ($extensions !== null) { + /** @var UnionTypeExtensionNode $extension */ foreach ($extensions as $extension) { foreach ($extension->types as $namedType) { $possibleTypes[] = static::$astBuilder->buildType($namedType); @@ -282,7 +280,7 @@ protected static function extendPossibleTypes(UnionType $type): array /** * @param ObjectType|InterfaceType $type * - * @return array + * @return array Should be InterfaceType, will be caught in schema validation */ protected static function extendImplementedInterfaces(ImplementingType $type): array { @@ -296,7 +294,9 @@ protected static function extendImplementedInterfaces(ImplementingType $type): a /** @var ObjectTypeExtensionNode|InterfaceTypeExtensionNode $extension */ foreach ($extensions as $extension) { foreach ($extension->interfaces as $namedType) { - $interfaces[] = static::$astBuilder->buildType($namedType); + /** @var InterfaceType $interface */ + $interface = static::$astBuilder->buildType($namedType); + $interfaces[] = $interface; } } } @@ -304,7 +304,12 @@ protected static function extendImplementedInterfaces(ImplementingType $type): a return $interfaces; } - protected static function extendType($typeDef) + /** + * @param ListOfType|NonNull|(Type &NamedType) $typeDef + * + * @return ListOfType|NonNull|(Type&NamedType) + */ + protected static function extendType(Type $typeDef): Type { if ($typeDef instanceof ListOfType) { return Type::listOf(static::extendType($typeDef->getOfType())); @@ -314,45 +319,43 @@ protected static function extendType($typeDef) return Type::nonNull(static::extendType($typeDef->getWrappedType())); } + // @phpstan-ignore-next-line generic is not properly caught return static::extendNamedType($typeDef); } /** - * @param FieldArgument[] $args + * @param array $args * - * @return mixed[] + * @return array> */ protected static function extendArgs(array $args): array { - return Utils::keyValMap( - $args, - static function (FieldArgument $arg): string { - return $arg->name; - }, - static function (FieldArgument $arg): array { - $def = [ - 'type' => static::extendType($arg->getType()), - 'description' => $arg->description, - 'astNode' => $arg->astNode, - ]; - - if ($arg->defaultValueExists()) { - $def['defaultValue'] = $arg->defaultValue; - } + $extended = []; + foreach ($args as $arg) { + $def = [ + 'type' => static::extendType($arg->getType()), + 'description' => $arg->description, + 'astNode' => $arg->astNode, + ]; - return $def; + if ($arg->defaultValueExists()) { + $def['defaultValue'] = $arg->defaultValue; } - ); + + $extended[$arg->name] = $def; + } + + return $extended; } /** * @param InterfaceType|ObjectType $type * - * @return mixed[] + * @return array> * * @throws Error */ - protected static function extendFieldMap($type): array + protected static function extendFieldMap(Type $type): array { $newFieldMap = []; $oldFieldMap = $type->getFields(); @@ -373,6 +376,7 @@ protected static function extendFieldMap($type): array $extensions = static::$typeExtensionsMap[$type->name] ?? null; if ($extensions !== null) { + /** @var ObjectTypeExtensionNode|InputObjectTypeExtensionNode $extension */ foreach ($extensions as $extension) { foreach ($extension->fields as $field) { $fieldName = $field->name->value; @@ -393,12 +397,8 @@ protected static function extendObjectType(ObjectType $type): ObjectType return new ObjectType([ 'name' => $type->name, 'description' => $type->description, - 'interfaces' => static function () use ($type): array { - return static::extendImplementedInterfaces($type); - }, - 'fields' => static function () use ($type): array { - return static::extendFieldMap($type); - }, + 'interfaces' => static fn (): array => static::extendImplementedInterfaces($type), + 'fields' => static fn (): array => static::extendFieldMap($type), 'astNode' => $type->astNode, 'extensionASTNodes' => static::getExtensionASTNodes($type), 'isTypeOf' => $type->config['isTypeOf'] ?? null, @@ -411,12 +411,8 @@ protected static function extendInterfaceType(InterfaceType $type): InterfaceTyp return new InterfaceType([ 'name' => $type->name, 'description' => $type->description, - 'interfaces' => static function () use ($type): array { - return static::extendImplementedInterfaces($type); - }, - 'fields' => static function () use ($type): array { - return static::extendFieldMap($type); - }, + 'interfaces' => static fn (): array => static::extendImplementedInterfaces($type), + 'fields' => static fn (): array => static::extendFieldMap($type), 'astNode' => $type->astNode, 'extensionASTNodes' => static::getExtensionASTNodes($type), 'resolveType' => $type->config['resolveType'] ?? null, @@ -435,7 +431,14 @@ protected static function isSpecifiedScalarType(Type $type): bool ); } - protected static function extendNamedType(Type $type) + /** + * @param T $type + * + * @return T + * + * @template T of Type + */ + protected static function extendNamedType($type) { if (Introspection::isIntrospectionType($type) || static::isSpecifiedScalarType($type)) { return $type; @@ -458,13 +461,18 @@ protected static function extendNamedType(Type $type) } } + // @phpstan-ignore-next-line the lines above ensure only matching subtypes get in here return static::$extendTypeCache[$name]; } /** - * @return mixed|null + * @param T|null $type + * + * @return T|null + * + * @template T of Type */ - protected static function extendMaybeNamedType(?NamedType $type = null) + protected static function extendMaybeNamedType(?Type $type = null): ?Type { if ($type !== null) { return static::extendNamedType($type); @@ -474,9 +482,9 @@ protected static function extendMaybeNamedType(?NamedType $type = null) } /** - * @param DirectiveDefinitionNode[] $directiveDefinitions + * @param array $directiveDefinitions * - * @return Directive[] + * @return array */ protected static function getMergedDirectives(Schema $schema, array $directiveDefinitions): array { diff --git a/tests/Utils/SchemaExtenderTest.php b/tests/Utils/SchemaExtenderTest.php index b5d660dc3..b2bcb13a0 100644 --- a/tests/Utils/SchemaExtenderTest.php +++ b/tests/Utils/SchemaExtenderTest.php @@ -44,17 +44,14 @@ class SchemaExtenderTest extends TestCase { - /** @var Schema */ - protected $testSchema; + protected Schema $testSchema; - /** @var string[] */ - protected $testSchemaDefinitions; + /** @var array */ + protected array $testSchemaDefinitions; - /** @var ObjectType */ - protected $FooType; + protected ObjectType $FooType; - /** @var Directive */ - protected $FooDirective; + protected Directive $FooDirective; public function setUp(): void { @@ -69,7 +66,7 @@ public function setUp(): void ]); //Class definition. - $SomeClassScalarType = new SomeScalarClassType(); + $SomeScalarClassType = new SomeScalarClassType(); $SomeInterfaceType = new InterfaceType([ 'name' => 'SomeInterface', @@ -171,11 +168,11 @@ public function setUp(): void $this->testSchema = new Schema([ 'query' => new ObjectType([ 'name' => 'Query', - 'fields' => static function () use ($FooType, $SomeScalarType, $SomeClassScalarType, $SomeUnionType, $SomeEnumType, $SomeInterfaceType, $SomeInputType): array { + 'fields' => static function () use ($FooType, $SomeScalarType, $SomeScalarClassType, $SomeUnionType, $SomeEnumType, $SomeInterfaceType, $SomeInputType): array { return [ 'foo' => [ 'type' => $FooType ], 'someScalar' => [ 'type' => $SomeScalarType ], - 'SomeScalarClass' => [ 'type' => $SomeClassScalarType ], + 'someScalarClass' => [ 'type' => $SomeScalarClassType ], 'someUnion' => [ 'type' => $SomeUnionType ], 'someEnum' => [ 'type' => $SomeEnumType ], 'someInterface' => [ @@ -2114,21 +2111,21 @@ public function testSupportsTypeConfigDecorator(): void /** * Tests both custom inline and class scalar definitions. - * + * * Ensures the correct instance is maintained before and after schema extension. - * Should probably be incorporated into testExtendsWithoutAlteringOriginalSchema(). + * Should probably be incorporated into testExtendsWithoutAlteringOriginalSchema(). */ public function testExtendsWithoutAlteringOriginalScalarTypes(): void { - $extendedSchema = $this->extendTestSchema(' + $extendedSchema = $this->extendTestSchema(/** @lang GraphQL */ ' extend type Foo { bar: Bar } '); - $someScalar = $this->testSchema->getType('SomeScalar'); - $someScalarClass = $this->testSchema->getType('SomeScalarClass'); - $extendedSomeScalar = $extendedSchema->getType('SomeScalar'); + $someScalar = $this->testSchema->getType('SomeScalar'); + $someScalarClass = $this->testSchema->getType('SomeScalarClass'); + $extendedSomeScalar = $extendedSchema->getType('SomeScalar'); $extendedSomeScalarClass = $extendedSchema->getType('SomeScalarClass'); self::assertInstanceOf(CustomScalarType::class, $someScalar); From 162919b91ab9cd0f2051d84447aa3f6b78b6df59 Mon Sep 17 00:00:00 2001 From: abdullahseba Date: Thu, 9 Sep 2021 00:32:21 +0100 Subject: [PATCH 04/13] Implement possible fix for #933 and rewrite test --- src/Utils/SchemaExtender.php | 6 +-- tests/Utils/SchemaExtenderTest.php | 82 +++++++++++++++++++++++------- 2 files changed, 68 insertions(+), 20 deletions(-) diff --git a/src/Utils/SchemaExtender.php b/src/Utils/SchemaExtender.php index f738c832d..688bae97c 100644 --- a/src/Utils/SchemaExtender.php +++ b/src/Utils/SchemaExtender.php @@ -134,9 +134,9 @@ protected static function extendScalarType(ScalarType $type): CustomScalarType 'name' => $type->name, 'description' => $type->description, 'astNode' => $type->astNode, - 'serialize' => $type->config['serialize'] ?? null, - 'parseValue' => $type->config['parseValue'] ?? null, - 'parseLiteral' => $type->config['parseLiteral'] ?? null, + 'serialize' => static fn (...$args) => $type->serialize(...$args) ?? null, + 'parseValue' => static fn (...$args) => $type->parseValue(...$args) ?? null, + 'parseLiteral' => static fn (...$args) => $type->parseLiteral(...$args) ?? null, 'extensionASTNodes' => static::getExtensionASTNodes($type), ]); } diff --git a/tests/Utils/SchemaExtenderTest.php b/tests/Utils/SchemaExtenderTest.php index b2bcb13a0..4970c55d2 100644 --- a/tests/Utils/SchemaExtenderTest.php +++ b/tests/Utils/SchemaExtenderTest.php @@ -52,6 +52,13 @@ class SchemaExtenderTest extends TestCase protected ObjectType $FooType; protected Directive $FooDirective; + + /** @var CustomScalarType */ + protected $SomeScalarType; + + /** @var SomeScalarClassType */ + protected $SomeClassScalarType; + public function setUp(): void { @@ -200,8 +207,10 @@ public function setUp(): void return Printer::doPrint($node); }, iterator_to_array($testSchemaAst->definitions->getIterator())); - $this->FooDirective = $FooDirective; - $this->FooType = $FooType; + $this->FooDirective = $FooDirective; + $this->FooType = $FooType; + $this->SomeClassScalarType = $SomeClassScalarType; + $this->SomeScalarType = $SomeScalarType; } protected function dedent(string $str): string @@ -2112,25 +2121,64 @@ public function testSupportsTypeConfigDecorator(): void /** * Tests both custom inline and class scalar definitions. * - * Ensures the correct instance is maintained before and after schema extension. - * Should probably be incorporated into testExtendsWithoutAlteringOriginalSchema(). */ - public function testExtendsWithoutAlteringOriginalScalarTypes(): void + public function testUseOriginalScalarTypes(): void { - $extendedSchema = $this->extendTestSchema(/** @lang GraphQL */ ' - extend type Foo { - bar: Bar - } + + $queryType = new ObjectType([ + 'name' => 'Query', + 'fields' => [ + 'someScalar' => [ 'type' => $this->SomeScalarType ], + 'someScalarClass' => [ 'type' => $this->SomeClassScalarType ], + ], + 'resolveField' => static function (): string { + return ''; + }, + ]); + + $schema = new Schema(['query' => $queryType]); + + $documentNode = Parser::parse(' + type Foo { + someScalar: SomeScalar + someScalarClass: SomeScalarClass + } + extend type Query { + foo:Foo + } + '); - $someScalar = $this->testSchema->getType('SomeScalar'); - $someScalarClass = $this->testSchema->getType('SomeScalarClass'); - $extendedSomeScalar = $extendedSchema->getType('SomeScalar'); - $extendedSomeScalarClass = $extendedSchema->getType('SomeScalarClass'); + $typeConfigDecorator = static function ($typeConfig) { + switch ($typeConfig['name']) { + case 'Foo': + $typeConfig['resolveField'] = function ($user, $args, $context, $info) { + switch ($info->fieldName) { + case 'someScalar': + return 'someScalar'; + break; + case 'someScalarClass': + return 'someScalarClass'; + break; + } + }; + break; + } + + return $typeConfig; + }; + + $extendedSchema = SchemaExtender::extend($schema, $documentNode, [],$typeConfigDecorator); + + $query = '{ + foo { + someScalar + someScalarClass + } + }'; + + $result = GraphQL::executeQuery($extendedSchema, $query); - self::assertInstanceOf(CustomScalarType::class, $someScalar); - self::assertInstanceOf(SomeScalarClassType::class, $someScalarClass); - self::assertInstanceOf(CustomScalarType::class, $extendedSomeScalar); - self::assertInstanceOf(SomeScalarClassType::class, $extendedSomeScalarClass); + self::assertSame(['data' => [ 'foo' => ['someScalar' => 'someScalar', 'someScalarClass' => 'someScalarClass']]], $result->toArray()); } } From 8420b3b01e8bcca283029fac0c117144708aa218 Mon Sep 17 00:00:00 2001 From: Benedikt Franke Date: Thu, 9 Sep 2021 09:23:26 +0200 Subject: [PATCH 05/13] Fix codestyle --- tests/Utils/SchemaExtenderTest.php | 106 ++++++++++++++++------------- 1 file changed, 58 insertions(+), 48 deletions(-) diff --git a/tests/Utils/SchemaExtenderTest.php b/tests/Utils/SchemaExtenderTest.php index 4970c55d2..055bd6fc0 100644 --- a/tests/Utils/SchemaExtenderTest.php +++ b/tests/Utils/SchemaExtenderTest.php @@ -4,6 +4,7 @@ namespace GraphQL\Tests\Utils; +use Exception; use GraphQL\Error\Error; use GraphQL\GraphQL; use GraphQL\Language\AST\DefinitionNode; @@ -21,6 +22,7 @@ use GraphQL\Type\Definition\InterfaceType; use GraphQL\Type\Definition\NonNull; use GraphQL\Type\Definition\ObjectType; +use GraphQL\Type\Definition\ResolveInfo; use GraphQL\Type\Definition\ScalarType; use GraphQL\Type\Definition\Type; use GraphQL\Type\Definition\UnionType; @@ -52,13 +54,12 @@ class SchemaExtenderTest extends TestCase protected ObjectType $FooType; protected Directive $FooDirective; - + /** @var CustomScalarType */ protected $SomeScalarType; /** @var SomeScalarClassType */ - protected $SomeClassScalarType; - + protected $SomeScalarClassType; public function setUp(): void { @@ -207,10 +208,10 @@ public function setUp(): void return Printer::doPrint($node); }, iterator_to_array($testSchemaAst->definitions->getIterator())); - $this->FooDirective = $FooDirective; - $this->FooType = $FooType; - $this->SomeClassScalarType = $SomeClassScalarType; - $this->SomeScalarType = $SomeScalarType; + $this->FooDirective = $FooDirective; + $this->FooType = $FooType; + $this->SomeScalarClassType = $SomeScalarClassType; + $this->SomeScalarType = $SomeScalarType; } protected function dedent(string $str): string @@ -2120,65 +2121,74 @@ public function testSupportsTypeConfigDecorator(): void /** * Tests both custom inline and class scalar definitions. - * */ public function testUseOriginalScalarTypes(): void { - $queryType = new ObjectType([ - 'name' => 'Query', - 'fields' => [ - 'someScalar' => [ 'type' => $this->SomeScalarType ], - 'someScalarClass' => [ 'type' => $this->SomeClassScalarType ], - ], - 'resolveField' => static function (): string { - return ''; - }, - ]); - - $schema = new Schema(['query' => $queryType]); + 'name' => 'Query', + 'fields' => [ + 'someScalar' => [ 'type' => $this->SomeScalarType ], + 'someScalarClass' => [ 'type' => $this->SomeScalarClassType ], + ], + 'resolveField' => static fn (): string => '', + ]); - $documentNode = Parser::parse(' - type Foo { - someScalar: SomeScalar - someScalarClass: SomeScalarClass + $schema = new Schema(['query' => $queryType]); + + $documentNode = Parser::parse(/** @lang GraphQL */ ' + type Foo { + someScalar: SomeScalar + someScalarClass: SomeScalarClass + } + + extend type Query { + foo: Foo } - extend type Query { - foo:Foo - } - '); - $typeConfigDecorator = static function ($typeConfig) { - switch ($typeConfig['name']) { - case 'Foo': - $typeConfig['resolveField'] = function ($user, $args, $context, $info) { - switch ($info->fieldName) { - case 'someScalar': - return 'someScalar'; - break; - case 'someScalarClass': - return 'someScalarClass'; - break; - } - }; - break; - } + $typeConfigDecorator = static function (array $typeConfig): array { + switch ($typeConfig['name']) { + case 'Foo': + $typeConfig['resolveField'] = static function ($user, array $args, $context, ResolveInfo $info): string { + switch ($info->fieldName) { + case 'someScalar': + return 'someScalar'; + + case 'someScalarClass': + return 'someScalarClass'; - return $typeConfig; + default: + throw new Exception('Unexpected field: ' . $info->fieldName); + } + }; + } + + return $typeConfig; }; - $extendedSchema = SchemaExtender::extend($schema, $documentNode, [],$typeConfigDecorator); + $extendedSchema = SchemaExtender::extend($schema, $documentNode, [], $typeConfigDecorator); - $query = '{ + $query = /** @lang GraphQL */' + { foo { someScalar someScalarClass } - }'; + } + '; $result = GraphQL::executeQuery($extendedSchema, $query); - self::assertSame(['data' => [ 'foo' => ['someScalar' => 'someScalar', 'someScalarClass' => 'someScalarClass']]], $result->toArray()); + self::assertSame( + [ + 'data' => [ + 'foo' => [ + 'someScalar' => 'someScalar', + 'someScalarClass' => 'someScalarClass', + ], + ], + ], + $result->toArray() + ); } } From bcd73c348d4aa43bf7f85381ccf75e4bb858db23 Mon Sep 17 00:00:00 2001 From: Benedikt Franke Date: Thu, 9 Sep 2021 09:40:22 +0200 Subject: [PATCH 06/13] Make the test laser-focused on the actual problem --- src/Utils/SchemaExtender.php | 6 +- tests/Utils/SchemaExtenderTest.php | 92 +++++------------------------ tests/Utils/SomeScalarClassType.php | 12 ++-- 3 files changed, 26 insertions(+), 84 deletions(-) diff --git a/src/Utils/SchemaExtender.php b/src/Utils/SchemaExtender.php index 688bae97c..0be0a5a73 100644 --- a/src/Utils/SchemaExtender.php +++ b/src/Utils/SchemaExtender.php @@ -134,9 +134,9 @@ protected static function extendScalarType(ScalarType $type): CustomScalarType 'name' => $type->name, 'description' => $type->description, 'astNode' => $type->astNode, - 'serialize' => static fn (...$args) => $type->serialize(...$args) ?? null, - 'parseValue' => static fn (...$args) => $type->parseValue(...$args) ?? null, - 'parseLiteral' => static fn (...$args) => $type->parseLiteral(...$args) ?? null, + 'serialize' => [$type, 'serialize'], + 'parseValue' => [$type, 'parseValue'], + 'parseLiteral' => [$type, 'parseLiteral'], 'extensionASTNodes' => static::getExtensionASTNodes($type), ]); } diff --git a/tests/Utils/SchemaExtenderTest.php b/tests/Utils/SchemaExtenderTest.php index 055bd6fc0..fb6686e35 100644 --- a/tests/Utils/SchemaExtenderTest.php +++ b/tests/Utils/SchemaExtenderTest.php @@ -4,7 +4,6 @@ namespace GraphQL\Tests\Utils; -use Exception; use GraphQL\Error\Error; use GraphQL\GraphQL; use GraphQL\Language\AST\DefinitionNode; @@ -22,7 +21,6 @@ use GraphQL\Type\Definition\InterfaceType; use GraphQL\Type\Definition\NonNull; use GraphQL\Type\Definition\ObjectType; -use GraphQL\Type\Definition\ResolveInfo; use GraphQL\Type\Definition\ScalarType; use GraphQL\Type\Definition\Type; use GraphQL\Type\Definition\UnionType; @@ -55,27 +53,15 @@ class SchemaExtenderTest extends TestCase protected Directive $FooDirective; - /** @var CustomScalarType */ - protected $SomeScalarType; - - /** @var SomeScalarClassType */ - protected $SomeScalarClassType; - public function setUp(): void { parent::setUp(); - //Inline definition. $SomeScalarType = new CustomScalarType([ 'name' => 'SomeScalar', - 'serialize' => static function ($x) { - return $x; - }, + 'serialize' => static fn ($x) => $x, ]); - //Class definition. - $SomeScalarClassType = new SomeScalarClassType(); - $SomeInterfaceType = new InterfaceType([ 'name' => 'SomeInterface', 'fields' => static function () use (&$SomeInterfaceType): array { @@ -176,11 +162,10 @@ public function setUp(): void $this->testSchema = new Schema([ 'query' => new ObjectType([ 'name' => 'Query', - 'fields' => static function () use ($FooType, $SomeScalarType, $SomeScalarClassType, $SomeUnionType, $SomeEnumType, $SomeInterfaceType, $SomeInputType): array { + 'fields' => static function () use ($FooType, $SomeScalarType, $SomeUnionType, $SomeEnumType, $SomeInterfaceType, $SomeInputType): array { return [ 'foo' => [ 'type' => $FooType ], 'someScalar' => [ 'type' => $SomeScalarType ], - 'someScalarClass' => [ 'type' => $SomeScalarClassType ], 'someUnion' => [ 'type' => $SomeUnionType ], 'someEnum' => [ 'type' => $SomeEnumType ], 'someInterface' => [ @@ -208,10 +193,8 @@ public function setUp(): void return Printer::doPrint($node); }, iterator_to_array($testSchemaAst->definitions->getIterator())); - $this->FooDirective = $FooDirective; - $this->FooType = $FooType; - $this->SomeScalarClassType = $SomeScalarClassType; - $this->SomeScalarType = $SomeScalarType; + $this->FooDirective = $FooDirective; + $this->FooType = $FooType; } protected function dedent(string $str): string @@ -2119,76 +2102,31 @@ public function testSupportsTypeConfigDecorator(): void self::assertSame(['data' => ['hello' => 'Hello World!', 'foo' => ['value' => 'bar']]], $result->toArray()); } - /** - * Tests both custom inline and class scalar definitions. - */ - public function testUseOriginalScalarTypes(): void + public function testPreservesScalarClassMethods(): void { + $SomeScalarClassType = new SomeScalarClassType(); + $queryType = new ObjectType([ 'name' => 'Query', 'fields' => [ - 'someScalar' => [ 'type' => $this->SomeScalarType ], - 'someScalarClass' => [ 'type' => $this->SomeScalarClassType ], + 'someScalarClass' => ['type' => $SomeScalarClassType], ], - 'resolveField' => static fn (): string => '', ]); $schema = new Schema(['query' => $queryType]); $documentNode = Parser::parse(/** @lang GraphQL */ ' - type Foo { - someScalar: SomeScalar - someScalarClass: SomeScalarClass - } - extend type Query { - foo: Foo + foo: ID } '); - $typeConfigDecorator = static function (array $typeConfig): array { - switch ($typeConfig['name']) { - case 'Foo': - $typeConfig['resolveField'] = static function ($user, array $args, $context, ResolveInfo $info): string { - switch ($info->fieldName) { - case 'someScalar': - return 'someScalar'; - - case 'someScalarClass': - return 'someScalarClass'; - - default: - throw new Exception('Unexpected field: ' . $info->fieldName); - } - }; - } - - return $typeConfig; - }; - - $extendedSchema = SchemaExtender::extend($schema, $documentNode, [], $typeConfigDecorator); - - $query = /** @lang GraphQL */' - { - foo { - someScalar - someScalarClass - } - } - '; - - $result = GraphQL::executeQuery($extendedSchema, $query); + $extendedSchema = SchemaExtender::extend($schema, $documentNode); + $extendedScalar = $extendedSchema->getType('SomeScalarClass'); - self::assertSame( - [ - 'data' => [ - 'foo' => [ - 'someScalar' => 'someScalar', - 'someScalarClass' => 'someScalarClass', - ], - ], - ], - $result->toArray() - ); + self::assertInstanceOf(CustomScalarType::class, $extendedScalar); + self::assertSame(SomeScalarClassType::SERIALIZE_RETURN, $extendedScalar->serialize(null)); + self::assertSame(SomeScalarClassType::PARSE_VALUE_RETURN, $extendedScalar->parseValue(null)); + self::assertSame(SomeScalarClassType::PARSE_LITERAL_RETURN, $extendedScalar->parseLiteral($documentNode)); } } diff --git a/tests/Utils/SomeScalarClassType.php b/tests/Utils/SomeScalarClassType.php index a4587362f..6c520cae2 100644 --- a/tests/Utils/SomeScalarClassType.php +++ b/tests/Utils/SomeScalarClassType.php @@ -10,20 +10,24 @@ /** * Custom class-based scalar type for testing. */ -class SomeScalarClassType extends ScalarType +final class SomeScalarClassType extends ScalarType { + public const SERIALIZE_RETURN = 'a constant value that is always returned from serialize'; + public const PARSE_VALUE_RETURN = 'a constant value that is always returned from parseValue'; + public const PARSE_LITERAL_RETURN = 'a constant value that is always returned from parseLiteral'; + public function serialize($value): string { - return $value; + return self::SERIALIZE_RETURN; } public function parseValue($value): string { - return $value; + return self::PARSE_VALUE_RETURN; } public function parseLiteral(Node $valueNode, ?array $variables = null): string { - return $valueNode->value; + return self::PARSE_LITERAL_RETURN; } } From a80b4bfa55f0b12ba85584e85a0c1dfd31c9d668 Mon Sep 17 00:00:00 2001 From: Benedikt Franke Date: Thu, 9 Sep 2021 09:43:28 +0200 Subject: [PATCH 07/13] fix codestyle --- src/Language/AST/NodeList.php | 6 ++---- src/Type/Schema.php | 3 ++- 2 files changed, 4 insertions(+), 5 deletions(-) diff --git a/src/Language/AST/NodeList.php b/src/Language/AST/NodeList.php index 30a886d8a..7ecef7a1d 100644 --- a/src/Language/AST/NodeList.php +++ b/src/Language/AST/NodeList.php @@ -32,8 +32,8 @@ class NodeList implements ArrayAccess, IteratorAggregate, Countable /** * @param array> $nodes - * * @phpstan-param array> $nodes + * * @phpstan-return self */ public static function create(array $nodes): self @@ -43,7 +43,6 @@ public static function create(array $nodes): self /** * @param array $nodes - * * @phpstan-param array> $nodes */ public function __construct(array $nodes) @@ -91,7 +90,6 @@ public function offsetGet($offset)// : Node /** * @param int|string|null $offset * @param Node|array $value - * * @phpstan-param T|array $value */ #[ReturnTypeWillChange] @@ -136,8 +134,8 @@ public function splice(int $offset, int $length, $replacement = null): NodeList /** * @param NodeList|array> $list - * * @phpstan-param NodeList|array $list + * * @phpstan-return NodeList */ public function merge($list): NodeList diff --git a/src/Type/Schema.php b/src/Type/Schema.php index 9328509b5..5fbb0b224 100644 --- a/src/Type/Schema.php +++ b/src/Type/Schema.php @@ -395,9 +395,10 @@ private function defaultTypeLoader(string $typeName): ?Type /** * @param Type|callable $type - * * @phpstan-param T|callable():T $type + * * @phpstan-return T + * * @template T of Type */ public static function resolveType($type): Type From d97d7545c5c1dd67a32076e55a0d68fbda9fae06 Mon Sep 17 00:00:00 2001 From: Benedikt Franke Date: Thu, 9 Sep 2021 10:43:37 +0200 Subject: [PATCH 08/13] Fix other method references --- UPGRADE.md | 2 +- src/Type/Definition/AbstractType.php | 8 +- src/Type/Definition/InterfaceType.php | 8 -- src/Type/Definition/ObjectType.php | 8 +- src/Type/Definition/UnionType.php | 8 -- src/Utils/SchemaExtender.php | 22 ++-- tests/Type/SchemaTest.php | 18 +-- tests/Utils/SchemaExtenderTest.php | 168 ++++++++++++++++++++++--- tests/Utils/SomeInterfaceClassType.php | 22 ++++ tests/Utils/SomeObjectClassType.php | 19 +++ tests/Utils/SomeUnionClassType.php | 22 ++++ 11 files changed, 237 insertions(+), 68 deletions(-) create mode 100644 tests/Utils/SomeInterfaceClassType.php create mode 100644 tests/Utils/SomeObjectClassType.php create mode 100644 tests/Utils/SomeUnionClassType.php diff --git a/UPGRADE.md b/UPGRADE.md index 612565e15..796d16571 100644 --- a/UPGRADE.md +++ b/UPGRADE.md @@ -810,7 +810,7 @@ Starting from v0.7.0 the signature has changed to (note the new `$context` argum /** * @param mixed $object The parent resolved object * @param array $args Input arguments - * @param mixed $context The context object hat was passed to GraphQL::execute + * @param mixed $context The context object that was passed to GraphQL::execute * @param ResolveInfo $info ResolveInfo object * @return mixed */ diff --git a/src/Type/Definition/AbstractType.php b/src/Type/Definition/AbstractType.php index e02571549..1baf341c7 100644 --- a/src/Type/Definition/AbstractType.php +++ b/src/Type/Definition/AbstractType.php @@ -12,12 +12,12 @@ interface AbstractType { /** - * Resolves concrete ObjectType for given object value + * Resolves the concrete ObjectType for the given value. * - * @param object $objectValue - * @param mixed[] $context + * @param mixed $objectValue The resolved value for the object type + * @param mixed $context The context that was passed to GraphQL::execute * - * @return mixed + * @return ObjectType|callable(): ObjectType|null */ public function resolveType($objectValue, $context, ResolveInfo $info); } diff --git a/src/Type/Definition/InterfaceType.php b/src/Type/Definition/InterfaceType.php index 59413b762..1e92df348 100644 --- a/src/Type/Definition/InterfaceType.php +++ b/src/Type/Definition/InterfaceType.php @@ -116,14 +116,6 @@ public function getInterfaces(): array return $this->interfaces; } - /** - * Resolves concrete ObjectType for given object value - * - * @param object $objectValue - * @param mixed $context - * - * @return Type|mixed|null - */ public function resolveType($objectValue, $context, ResolveInfo $info) { if (isset($this->config['resolveType'])) { diff --git a/src/Type/Definition/ObjectType.php b/src/Type/Definition/ObjectType.php index 73cda6cf3..ec52dc7fa 100644 --- a/src/Type/Definition/ObjectType.php +++ b/src/Type/Definition/ObjectType.php @@ -161,16 +161,16 @@ public function getInterfaces(): array } /** - * @param mixed $value - * @param mixed $context + * @param mixed $objectValue The resolved value for the object type + * @param mixed $context The context that was passed to GraphQL::execute * * @return bool|Deferred|null */ - public function isTypeOf($value, $context, ResolveInfo $info) + public function isTypeOf($objectValue, $context, ResolveInfo $info) { return isset($this->config['isTypeOf']) ? $this->config['isTypeOf']( - $value, + $objectValue, $context, $info ) diff --git a/src/Type/Definition/UnionType.php b/src/Type/Definition/UnionType.php index 60ab553f5..63a5befed 100644 --- a/src/Type/Definition/UnionType.php +++ b/src/Type/Definition/UnionType.php @@ -110,14 +110,6 @@ public function getTypes(): array return $this->types; } - /** - * Resolves concrete ObjectType for given object value - * - * @param object $objectValue - * @param mixed $context - * - * @return callable|mixed|null - */ public function resolveType($objectValue, $context, ResolveInfo $info) { if (isset($this->config['resolveType'])) { diff --git a/src/Utils/SchemaExtender.php b/src/Utils/SchemaExtender.php index 0be0a5a73..eafb6eadc 100644 --- a/src/Utils/SchemaExtender.php +++ b/src/Utils/SchemaExtender.php @@ -133,10 +133,10 @@ protected static function extendScalarType(ScalarType $type): CustomScalarType return new CustomScalarType([ 'name' => $type->name, 'description' => $type->description, - 'astNode' => $type->astNode, 'serialize' => [$type, 'serialize'], 'parseValue' => [$type, 'parseValue'], 'parseLiteral' => [$type, 'parseLiteral'], + 'astNode' => $type->astNode, 'extensionASTNodes' => static::getExtensionASTNodes($type), ]); } @@ -146,9 +146,9 @@ protected static function extendUnionType(UnionType $type): UnionType return new UnionType([ 'name' => $type->name, 'description' => $type->description, - 'types' => static fn (): array => static::extendPossibleTypes($type), + 'types' => static fn (): array => static::extendUnionPossibleTypes($type), + 'resolveType' => [$type, 'resolveType'], 'astNode' => $type->astNode, - 'resolveType' => $type->config['resolveType'] ?? null, 'extensionASTNodes' => static::getExtensionASTNodes($type), ]); } @@ -158,7 +158,7 @@ protected static function extendEnumType(EnumType $type): EnumType return new EnumType([ 'name' => $type->name, 'description' => $type->description, - 'values' => static::extendValueMap($type), + 'values' => static::extendEnumValueMap($type), 'astNode' => $type->astNode, 'extensionASTNodes' => static::getExtensionASTNodes($type), ]); @@ -217,7 +217,7 @@ protected static function extendInputFieldMap(InputObjectType $type): array /** * @return array> */ - protected static function extendValueMap(EnumType $type): array + protected static function extendEnumValueMap(EnumType $type): array { $newValueMap = []; /** @var EnumValueDefinition[] $oldValueMap */ @@ -257,7 +257,7 @@ protected static function extendValueMap(EnumType $type): array /** * @return array Should be ObjectType, will be caught in schema validation */ - protected static function extendPossibleTypes(UnionType $type): array + protected static function extendUnionPossibleTypes(UnionType $type): array { $possibleTypes = array_map( [static::class, 'extendNamedType'], @@ -369,8 +369,8 @@ protected static function extendFieldMap(Type $type): array 'deprecationReason' => $field->deprecationReason, 'type' => static::extendType($field->getType()), 'args' => static::extendArgs($field->args), - 'astNode' => $field->astNode, 'resolve' => $field->resolveFn, + 'astNode' => $field->astNode, ]; } @@ -399,10 +399,10 @@ protected static function extendObjectType(ObjectType $type): ObjectType 'description' => $type->description, 'interfaces' => static fn (): array => static::extendImplementedInterfaces($type), 'fields' => static fn (): array => static::extendFieldMap($type), + 'isTypeOf' => [$type, 'isTypeOf'], + 'resolveField' => $type->resolveFieldFn ?? null, 'astNode' => $type->astNode, 'extensionASTNodes' => static::getExtensionASTNodes($type), - 'isTypeOf' => $type->config['isTypeOf'] ?? null, - 'resolveField' => $type->resolveFieldFn ?? null, ]); } @@ -413,9 +413,9 @@ protected static function extendInterfaceType(InterfaceType $type): InterfaceTyp 'description' => $type->description, 'interfaces' => static fn (): array => static::extendImplementedInterfaces($type), 'fields' => static fn (): array => static::extendFieldMap($type), + 'resolveType' => [$type, 'resolveType'], 'astNode' => $type->astNode, 'extensionASTNodes' => static::getExtensionASTNodes($type), - 'resolveType' => $type->config['resolveType'] ?? null, ]); } @@ -509,8 +509,8 @@ protected static function extendDirective(Directive $directive): Directive 'description' => $directive->description, 'locations' => $directive->locations, 'args' => static::extendArgs($directive->args), - 'astNode' => $directive->astNode, 'isRepeatable' => $directive->isRepeatable, + 'astNode' => $directive->astNode, ]); } diff --git a/tests/Type/SchemaTest.php b/tests/Type/SchemaTest.php index a58173077..e653d55f8 100644 --- a/tests/Type/SchemaTest.php +++ b/tests/Type/SchemaTest.php @@ -18,23 +18,17 @@ class SchemaTest extends TestCase { - /** @var InterfaceType */ - private $interfaceType; + private InterfaceType $interfaceType; - /** @var ObjectType */ - private $implementingType; + private ObjectType $implementingType; - /** @var InputObjectType */ - private $directiveInputType; + private InputObjectType $directiveInputType; - /** @var InputObjectType */ - private $wrappedDirectiveInputType; + private InputObjectType $wrappedDirectiveInputType; - /** @var Directive */ - private $directive; + private Directive $directive; - /** @var Schema */ - private $schema; + private Schema $schema; public function setUp(): void { diff --git a/tests/Utils/SchemaExtenderTest.php b/tests/Utils/SchemaExtenderTest.php index fb6686e35..5bed37d28 100644 --- a/tests/Utils/SchemaExtenderTest.php +++ b/tests/Utils/SchemaExtenderTest.php @@ -4,6 +4,7 @@ namespace GraphQL\Tests\Utils; +use GraphQL\Error\DebugFlag; use GraphQL\Error\Error; use GraphQL\GraphQL; use GraphQL\Language\AST\DefinitionNode; @@ -29,6 +30,7 @@ use GraphQL\Utils\SchemaExtender; use GraphQL\Utils\SchemaPrinter; use PHPUnit\Framework\TestCase; +use stdClass; use function array_filter; use function array_map; @@ -1935,16 +1937,14 @@ public function testOriginalResolversArePreserved(): void 'fields' => [ 'hello' => [ 'type' => Type::string(), - 'resolve' => static function (): string { - return 'Hello World!'; - }, + 'resolve' => static fn (): string => 'Hello World!', ], ], ]); $schema = new Schema(['query' => $queryType]); - $documentNode = Parser::parse(' + $documentNode = Parser::parse(/** @lang GraphQL */ ' extend type Query { misc: String } @@ -1955,7 +1955,7 @@ public function testOriginalResolversArePreserved(): void self::assertIsCallable($helloResolveFn); - $query = '{ hello }'; + $query = /** @lang GraphQL */ '{ hello }'; $result = GraphQL::executeQuery($extendedSchema, $query); self::assertSame(['data' => ['hello' => 'Hello World!']], $result->toArray()); } @@ -1969,14 +1969,12 @@ public function testOriginalResolveFieldIsPreserved(): void 'type' => Type::string(), ], ], - 'resolveField' => static function (): string { - return 'Hello World!'; - }, + 'resolveField' => static fn (): string => 'Hello World!', ]); $schema = new Schema(['query' => $queryType]); - $documentNode = Parser::parse(' + $documentNode = Parser::parse(/** @lang GraphQL */ ' extend type Query { misc: String } @@ -1987,7 +1985,7 @@ public function testOriginalResolveFieldIsPreserved(): void self::assertIsCallable($queryResolveFieldFn); - $query = '{ hello }'; + $query = /** @lang GraphQL */ '{ hello }'; $result = GraphQL::executeQuery($extendedSchema, $query); self::assertSame(['data' => ['hello' => 'Hello World!']], $result->toArray()); } @@ -1997,10 +1995,11 @@ public function testOriginalResolveFieldIsPreserved(): void */ public function testShouldBeAbleToIntroduceNewTypesThroughExtension(): void { - $sdl = ' + $sdl = /** @lang GraphQL */' type Query { defaultValue: String } + type Foo { value: Int } @@ -2009,7 +2008,7 @@ public function testShouldBeAbleToIntroduceNewTypesThroughExtension(): void $documentNode = Parser::parse($sdl); $schema = BuildSchema::build($documentNode); - $extensionSdl = ' + $extensionSdl = /** @lang GraphQL */ ' type Bar { foo: Foo } @@ -2018,7 +2017,7 @@ public function testShouldBeAbleToIntroduceNewTypesThroughExtension(): void $extendedDocumentNode = Parser::parse($extensionSdl); $extendedSchema = SchemaExtender::extend($schema, $extendedDocumentNode); - $expected = ' + $expected = /** @lang GraphQL */' type Bar { foo: Foo } @@ -2040,7 +2039,7 @@ public function testShouldBeAbleToIntroduceNewTypesThroughExtension(): void */ public function testPreservesRepeatableInDirective(): void { - $schema = BuildSchema::build(' + $schema = BuildSchema::build(/** @lang GraphQL */ ' directive @test(arg: Int) repeatable on FIELD | SCALAR '); @@ -2060,17 +2059,16 @@ public function testSupportsTypeConfigDecorator(): void 'type' => Type::string(), ], ], - 'resolveField' => static function (): string { - return 'Hello World!'; - }, + 'resolveField' => static fn (): string => 'Hello World!', ]); $schema = new Schema(['query' => $queryType]); - $documentNode = Parser::parse(' + $documentNode = Parser::parse(/** @lang GraphQL */ ' type Foo { value: String } + extend type Query { defaultValue: String foo: Foo @@ -2091,12 +2089,14 @@ public function testSupportsTypeConfigDecorator(): void $extendedSchema = SchemaExtender::extend($schema, $documentNode, [], $typeConfigDecorator); - $query = '{ + $query = /** @lang GraphQL */' + { hello foo { value } - }'; + } + '; $result = GraphQL::executeQuery($extendedSchema, $query); self::assertSame(['data' => ['hello' => 'Hello World!', 'foo' => ['value' => 'bar']]], $result->toArray()); @@ -2129,4 +2129,132 @@ public function testPreservesScalarClassMethods(): void self::assertSame(SomeScalarClassType::PARSE_VALUE_RETURN, $extendedScalar->parseValue(null)); self::assertSame(SomeScalarClassType::PARSE_LITERAL_RETURN, $extendedScalar->parseLiteral($documentNode)); } + + public function testPreservesResolveTypeMethod(): void + { + $SomeInterfaceClassType = new SomeInterfaceClassType([ + 'name' => 'SomeInterface', + 'fields' => [ + 'foo' => [ 'type' => Type::string() ], + ], + ]); + + $FooType = new ObjectType([ + 'name' => 'Foo', + 'interfaces' => [$SomeInterfaceClassType], + 'fields' => [ + 'foo' => [ 'type' => Type::string() ], + ], + ]); + + $BarType = new ObjectType([ + 'name' => 'Bar', + 'fields' => [ + 'bar' => [ 'type' => Type::string() ], + ], + ]); + + $SomeUnionClassType = new SomeUnionClassType([ + 'name' => 'SomeUnion', + 'types' => [$FooType, $BarType], + ]); + + $QueryType = new ObjectType([ + 'name' => 'Query', + 'fields' => [ + 'someUnion' => ['type' => $SomeUnionClassType], + 'someInterface' => ['type' => $SomeInterfaceClassType], + ], + 'resolveField' => static fn (): stdClass => new stdClass(), + ]); + + $schema = new Schema(['query' => $QueryType]); + + $documentNode = Parser::parse(/** @lang GraphQL */ ' + extend type Query { + foo: ID + } + '); + + $extendedSchema = SchemaExtender::extend($schema, $documentNode); + + $ExtendedFooType = $extendedSchema->getType('Foo'); + self::assertInstanceOf(ObjectType::class, $ExtendedFooType); + + $SomeInterfaceClassType->concrete = $ExtendedFooType; + $SomeUnionClassType->concrete = $ExtendedFooType; + + $query = /** @lang GraphQL */' + { + someUnion { + __typename + } + someInterface { + __typename + } + } + '; + $result = GraphQL::executeQuery($extendedSchema, $query); + + self::assertSame([ + 'data' => [ + 'someUnion' => ['__typename' => 'Foo'], + 'someInterface' => ['__typename' => 'Foo'], + ], + ], $result->toArray(DebugFlag::RETHROW_INTERNAL_EXCEPTIONS)); + } + + public function testPreservesIsTypeOfMethod(): void + { + $SomeInterfaceType = new InterfaceType([ + 'name' => 'SomeInterface', + 'fields' => [ + 'foo' => [ 'type' => Type::string() ], + ], + ]); + + $FooClassType = new SomeObjectClassType([ + 'name' => 'Foo', + 'interfaces' => [$SomeInterfaceType], + 'fields' => [ + 'foo' => [ 'type' => Type::string() ], + ], + ]); + + $QueryType = new ObjectType([ + 'name' => 'Query', + 'fields' => [ + 'someInterface' => ['type' => $SomeInterfaceType], + ], + 'resolveField' => static fn (): stdClass => new stdClass(), + ]); + + $schema = new Schema([ + 'query' => $QueryType, + 'types' => [$FooClassType], + ]); + + $documentNode = Parser::parse(/** @lang GraphQL */ ' + extend type Query { + foo: ID + } + '); + + $extendedSchema = SchemaExtender::extend($schema, $documentNode); + + $query = /** @lang GraphQL */' + { + someInterface { + __typename + } + } + '; + $result = GraphQL::executeQuery($extendedSchema, $query); + + self::assertSame([ + 'data' => [ + 'someInterface' => ['__typename' => 'Foo'], + ], + ], $result->toArray(DebugFlag::RETHROW_INTERNAL_EXCEPTIONS)); + } } diff --git a/tests/Utils/SomeInterfaceClassType.php b/tests/Utils/SomeInterfaceClassType.php new file mode 100644 index 000000000..741bd8618 --- /dev/null +++ b/tests/Utils/SomeInterfaceClassType.php @@ -0,0 +1,22 @@ +concrete; + } +} diff --git a/tests/Utils/SomeObjectClassType.php b/tests/Utils/SomeObjectClassType.php new file mode 100644 index 000000000..9cfa464b5 --- /dev/null +++ b/tests/Utils/SomeObjectClassType.php @@ -0,0 +1,19 @@ +concrete; + } +} From c10c6050023f193bea17aacda8f0881d59190c90 Mon Sep 17 00:00:00 2001 From: Benedikt Franke Date: Thu, 9 Sep 2021 10:45:06 +0200 Subject: [PATCH 09/13] changelog --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 66a6c93c4..4549c2861 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -41,6 +41,7 @@ You can find and compare releases at the [GitHub release page](https://github.co - Handle `null` parent of list in `ValuesOfCorrectType::getVisitor` - Allow sending both `query` and `queryId`, ignore `queryId` in that case - Fix `extend()` to preserve `repeatable` (#931) +- Preserve extended methods from class-based types in `SchemaExtender::extend()` ### Removed From 62cc470f1394f2d189cdf85c2a5c5561db614a04 Mon Sep 17 00:00:00 2001 From: Benedikt Franke Date: Fri, 10 Sep 2021 10:13:00 +0200 Subject: [PATCH 10/13] Reference method with () --- src/Type/Definition/AbstractType.php | 2 +- src/Type/Definition/ObjectType.php | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/Type/Definition/AbstractType.php b/src/Type/Definition/AbstractType.php index 1baf341c7..ba5c6581c 100644 --- a/src/Type/Definition/AbstractType.php +++ b/src/Type/Definition/AbstractType.php @@ -15,7 +15,7 @@ interface AbstractType * Resolves the concrete ObjectType for the given value. * * @param mixed $objectValue The resolved value for the object type - * @param mixed $context The context that was passed to GraphQL::execute + * @param mixed $context The context that was passed to GraphQL::execute() * * @return ObjectType|callable(): ObjectType|null */ diff --git a/src/Type/Definition/ObjectType.php b/src/Type/Definition/ObjectType.php index ec52dc7fa..13a4b32ac 100644 --- a/src/Type/Definition/ObjectType.php +++ b/src/Type/Definition/ObjectType.php @@ -162,7 +162,7 @@ public function getInterfaces(): array /** * @param mixed $objectValue The resolved value for the object type - * @param mixed $context The context that was passed to GraphQL::execute + * @param mixed $context The context that was passed to GraphQL::execute() * * @return bool|Deferred|null */ From 7b4a03bfc9c2995695c520f5456d8d5e1490578b Mon Sep 17 00:00:00 2001 From: Benedikt Franke Date: Fri, 10 Sep 2021 10:13:11 +0200 Subject: [PATCH 11/13] baseline over ignore --- phpstan-baseline.neon | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/phpstan-baseline.neon b/phpstan-baseline.neon index 8e7fed45c..dbc05649a 100644 --- a/phpstan-baseline.neon +++ b/phpstan-baseline.neon @@ -56,12 +56,12 @@ parameters: path: src/Language/AST/Node.php - - message: "#^Variable property access on GraphQL\\\\Language\\\\AST\\\\Node\\|null\\.$#" + message: "#^Variable property access on GraphQL\\\\Language\\\\AST\\\\Node\\.$#" count: 1 path: src/Language/Visitor.php - - message: "#^Variable property access on GraphQL\\\\Language\\\\AST\\\\Node\\.$#" + message: "#^Variable property access on GraphQL\\\\Language\\\\AST\\\\Node\\|null\\.$#" count: 1 path: src/Language/Visitor.php @@ -80,6 +80,11 @@ parameters: count: 2 path: src/Utils/AST.php + - + message: "#^Method GraphQL\\\\Utils\\\\SchemaExtender\\:\\:extendType\\(\\) should return GraphQL\\\\Type\\\\Definition\\\\ListOfType\\|\\(GraphQL\\\\Type\\\\Definition\\\\NamedType&GraphQL\\\\Type\\\\Definition\\\\Type\\)\\|GraphQL\\\\Type\\\\Definition\\\\NonNull but returns GraphQL\\\\Type\\\\Definition\\\\Type\\.$#" + count: 1 + path: src/Utils/SchemaExtender.php + - message: "#^Variable property access on object\\.$#" count: 1 From dce75faa2dadf7dad6a1e33171d88d5d13b25d60 Mon Sep 17 00:00:00 2001 From: Benedikt Franke Date: Fri, 10 Sep 2021 10:13:59 +0200 Subject: [PATCH 12/13] justify @var hints --- src/Utils/SchemaExtender.php | 97 ++++++++++++++++++------------------ 1 file changed, 48 insertions(+), 49 deletions(-) diff --git a/src/Utils/SchemaExtender.php b/src/Utils/SchemaExtender.php index eafb6eadc..55379ecfe 100644 --- a/src/Utils/SchemaExtender.php +++ b/src/Utils/SchemaExtender.php @@ -54,30 +54,22 @@ class SchemaExtender protected static ASTDefinitionBuilder $astBuilder; /** + * @param Type &NamedType $type + * * @return array|null */ protected static function getExtensionASTNodes(NamedType $type): ?array { - if (! $type instanceof Type) { - return null; - } - - $name = $type->name; - if ($type->extensionASTNodes !== null) { - if (isset(static::$typeExtensionsMap[$name])) { - return array_merge($type->extensionASTNodes, static::$typeExtensionsMap[$name]); - } - - return $type->extensionASTNodes; - } - - return static::$typeExtensionsMap[$name] ?? null; + return array_merge( + $type->extensionASTNodes ?? [], + static::$typeExtensionsMap[$type->name] ?? [] + ); } /** * @throws Error */ - protected static function checkExtensionNode(Type $type, Node $node): void + protected static function assertTypeMatchesExtension(Type $type, Node $node): void { switch (true) { case $node instanceof ObjectTypeExtensionNode: @@ -196,10 +188,13 @@ protected static function extendInputFieldMap(InputObjectType $type): array $newFieldMap[$fieldName]['defaultValue'] = $field->defaultValue; } - $extensions = static::$typeExtensionsMap[$type->name] ?? null; - if ($extensions !== null) { - /** @var InputObjectTypeExtensionNode $extension */ - foreach ($extensions as $extension) { + if (isset(static::$typeExtensionsMap[$type->name])) { + /** + * Proven by @see assertTypeMatchesExtension(). + * + * @var InputObjectTypeExtensionNode $extension + */ + foreach (static::$typeExtensionsMap[$type->name] as $extension) { foreach ($extension->fields as $field) { $fieldName = $field->name->value; if (isset($oldFieldMap[$fieldName])) { @@ -220,7 +215,7 @@ protected static function extendInputFieldMap(InputObjectType $type): array protected static function extendEnumValueMap(EnumType $type): array { $newValueMap = []; - /** @var EnumValueDefinition[] $oldValueMap */ + /** @var array $oldValueMap */ $oldValueMap = []; foreach ($type->getValues() as $value) { $oldValueMap[$value->name] = $value; @@ -236,10 +231,13 @@ protected static function extendEnumValueMap(EnumType $type): array ]; } - $extensions = static::$typeExtensionsMap[$type->name] ?? null; - if ($extensions !== null) { - /** @var EnumTypeExtensionNode $extension */ - foreach ($extensions as $extension) { + if (isset(static::$typeExtensionsMap[$type->name])) { + /** + * Proven by @see assertTypeMatchesExtension(). + * + * @var EnumTypeExtensionNode $extension + */ + foreach (static::$typeExtensionsMap[$type->name] as $extension) { foreach ($extension->values as $value) { $valueName = $value->name->value; if (isset($oldValueMap[$valueName])) { @@ -264,10 +262,13 @@ protected static function extendUnionPossibleTypes(UnionType $type): array $type->getTypes() ); - $extensions = static::$typeExtensionsMap[$type->name] ?? null; - if ($extensions !== null) { - /** @var UnionTypeExtensionNode $extension */ - foreach ($extensions as $extension) { + if (isset(static::$typeExtensionsMap[$type->name])) { + /** + * Proven by @see assertTypeMatchesExtension(). + * + * @var UnionTypeExtensionNode $extension + */ + foreach (static::$typeExtensionsMap[$type->name] as $extension) { foreach ($extension->types as $namedType) { $possibleTypes[] = static::$astBuilder->buildType($namedType); } @@ -289,12 +290,15 @@ protected static function extendImplementedInterfaces(ImplementingType $type): a $type->getInterfaces() ); - $extensions = static::$typeExtensionsMap[$type->name] ?? null; - if ($extensions !== null) { - /** @var ObjectTypeExtensionNode|InterfaceTypeExtensionNode $extension */ - foreach ($extensions as $extension) { + if (isset(static::$typeExtensionsMap[$type->name])) { + /** + * Proven by @see assertTypeMatchesExtension(). + * + * @var ObjectTypeExtensionNode|InterfaceTypeExtensionNode $extension + */ + foreach (static::$typeExtensionsMap[$type->name] as $extension) { foreach ($extension->interfaces as $namedType) { - /** @var InterfaceType $interface */ + /** @var InterfaceType $interface we know this, but PHP templates cannot express it */ $interface = static::$astBuilder->buildType($namedType); $interfaces[] = $interface; } @@ -319,7 +323,6 @@ protected static function extendType(Type $typeDef): Type return Type::nonNull(static::extendType($typeDef->getWrappedType())); } - // @phpstan-ignore-next-line generic is not properly caught return static::extendNamedType($typeDef); } @@ -374,10 +377,13 @@ protected static function extendFieldMap(Type $type): array ]; } - $extensions = static::$typeExtensionsMap[$type->name] ?? null; - if ($extensions !== null) { - /** @var ObjectTypeExtensionNode|InputObjectTypeExtensionNode $extension */ - foreach ($extensions as $extension) { + if (isset(static::$typeExtensionsMap[$type->name])) { + /** + * Proven by @see assertTypeMatchesExtension(). + * + * @var ObjectTypeExtensionNode|InputObjectTypeExtensionNode $extension + */ + foreach (static::$typeExtensionsMap[$type->name] as $extension) { foreach ($extension->fields as $field) { $fieldName = $field->name->value; if (isset($oldFieldMap[$fieldName])) { @@ -536,12 +542,7 @@ public static function extend( /** @var array $schemaExtensions */ $schemaExtensions = []; - $definitionsCount = count($documentAST->definitions); - for ($i = 0; $i < $definitionsCount; $i++) { - - /** @var Node $def */ - $def = $documentAST->definitions[$i]; - + foreach ($documentAST->definitions as $def) { if ($def instanceof SchemaDefinitionNode) { $schemaDef = $def; } elseif ($def instanceof SchemaTypeExtensionNode) { @@ -561,16 +562,14 @@ public static function extend( $typeDefinitionMap[$typeName] = $def; } elseif ($def instanceof TypeExtensionNode) { - $extendedTypeName = isset($def->name) ? $def->name->value : null; + $extendedTypeName = $def->name->value; $existingType = $schema->getType($extendedTypeName); if ($existingType === null) { throw new Error('Cannot extend type "' . $extendedTypeName . '" because it does not exist in the existing schema.', [$def]); } - static::checkExtensionNode($existingType, $def); - - $existingTypeExtensions = static::$typeExtensionsMap[$extendedTypeName] ?? null; - static::$typeExtensionsMap[$extendedTypeName] = $existingTypeExtensions !== null ? array_merge($existingTypeExtensions, [$def]) : [$def]; + static::assertTypeMatchesExtension($existingType, $def); + static::$typeExtensionsMap[$extendedTypeName][] = $def; } elseif ($def instanceof DirectiveDefinitionNode) { $directiveName = $def->name->value; $existingDirective = $schema->getDirective($directiveName); From 472d0f6d8b8df274700327c535f5bf317bd80d61 Mon Sep 17 00:00:00 2001 From: Benedikt Franke Date: Fri, 10 Sep 2021 10:16:00 +0200 Subject: [PATCH 13/13] namespace test-specific classes --- tests/Utils/SchemaExtenderTest.php | 4 ++++ .../Utils/{ => SchemaExtenderTest}/SomeInterfaceClassType.php | 2 +- tests/Utils/{ => SchemaExtenderTest}/SomeObjectClassType.php | 2 +- tests/Utils/{ => SchemaExtenderTest}/SomeScalarClassType.php | 2 +- tests/Utils/{ => SchemaExtenderTest}/SomeUnionClassType.php | 2 +- 5 files changed, 8 insertions(+), 4 deletions(-) rename tests/Utils/{ => SchemaExtenderTest}/SomeInterfaceClassType.php (89%) rename tests/Utils/{ => SchemaExtenderTest}/SomeObjectClassType.php (86%) rename tests/Utils/{ => SchemaExtenderTest}/SomeScalarClassType.php (94%) rename tests/Utils/{ => SchemaExtenderTest}/SomeUnionClassType.php (89%) diff --git a/tests/Utils/SchemaExtenderTest.php b/tests/Utils/SchemaExtenderTest.php index 5bed37d28..c1be0933d 100644 --- a/tests/Utils/SchemaExtenderTest.php +++ b/tests/Utils/SchemaExtenderTest.php @@ -14,6 +14,10 @@ use GraphQL\Language\DirectiveLocation; use GraphQL\Language\Parser; use GraphQL\Language\Printer; +use GraphQL\Tests\Utils\SchemaExtenderTest\SomeInterfaceClassType; +use GraphQL\Tests\Utils\SchemaExtenderTest\SomeObjectClassType; +use GraphQL\Tests\Utils\SchemaExtenderTest\SomeScalarClassType; +use GraphQL\Tests\Utils\SchemaExtenderTest\SomeUnionClassType; use GraphQL\Type\Definition\CustomScalarType; use GraphQL\Type\Definition\Directive; use GraphQL\Type\Definition\EnumType; diff --git a/tests/Utils/SomeInterfaceClassType.php b/tests/Utils/SchemaExtenderTest/SomeInterfaceClassType.php similarity index 89% rename from tests/Utils/SomeInterfaceClassType.php rename to tests/Utils/SchemaExtenderTest/SomeInterfaceClassType.php index 741bd8618..60c3dd0d5 100644 --- a/tests/Utils/SomeInterfaceClassType.php +++ b/tests/Utils/SchemaExtenderTest/SomeInterfaceClassType.php @@ -2,7 +2,7 @@ declare(strict_types=1); -namespace GraphQL\Tests\Utils; +namespace GraphQL\Tests\Utils\SchemaExtenderTest; use GraphQL\Type\Definition\InterfaceType; use GraphQL\Type\Definition\ObjectType; diff --git a/tests/Utils/SomeObjectClassType.php b/tests/Utils/SchemaExtenderTest/SomeObjectClassType.php similarity index 86% rename from tests/Utils/SomeObjectClassType.php rename to tests/Utils/SchemaExtenderTest/SomeObjectClassType.php index 9cfa464b5..0003b9d47 100644 --- a/tests/Utils/SomeObjectClassType.php +++ b/tests/Utils/SchemaExtenderTest/SomeObjectClassType.php @@ -2,7 +2,7 @@ declare(strict_types=1); -namespace GraphQL\Tests\Utils; +namespace GraphQL\Tests\Utils\SchemaExtenderTest; use GraphQL\Type\Definition\ObjectType; use GraphQL\Type\Definition\ResolveInfo; diff --git a/tests/Utils/SomeScalarClassType.php b/tests/Utils/SchemaExtenderTest/SomeScalarClassType.php similarity index 94% rename from tests/Utils/SomeScalarClassType.php rename to tests/Utils/SchemaExtenderTest/SomeScalarClassType.php index 6c520cae2..b92739e99 100644 --- a/tests/Utils/SomeScalarClassType.php +++ b/tests/Utils/SchemaExtenderTest/SomeScalarClassType.php @@ -2,7 +2,7 @@ declare(strict_types=1); -namespace GraphQL\Tests\Utils; +namespace GraphQL\Tests\Utils\SchemaExtenderTest; use GraphQL\Language\AST\Node; use GraphQL\Type\Definition\ScalarType; diff --git a/tests/Utils/SomeUnionClassType.php b/tests/Utils/SchemaExtenderTest/SomeUnionClassType.php similarity index 89% rename from tests/Utils/SomeUnionClassType.php rename to tests/Utils/SchemaExtenderTest/SomeUnionClassType.php index 6a92e3a35..36840ccad 100644 --- a/tests/Utils/SomeUnionClassType.php +++ b/tests/Utils/SchemaExtenderTest/SomeUnionClassType.php @@ -2,7 +2,7 @@ declare(strict_types=1); -namespace GraphQL\Tests\Utils; +namespace GraphQL\Tests\Utils\SchemaExtenderTest; use GraphQL\Type\Definition\ObjectType; use GraphQL\Type\Definition\ResolveInfo;