From 8733152f417086d494c3e9997ee35106c2115e22 Mon Sep 17 00:00:00 2001 From: Fabio Capucci Date: Thu, 23 Nov 2023 15:14:52 +0100 Subject: [PATCH] fixes --- src/Federation/FederationHelper.php | 46 +++++++++++++++++++ src/Federation/FederationPrinter.php | 29 +----------- src/Federation/SchemaPrinter.php | 31 +++---------- src/Schema/AST/ASTHelper.php | 42 +++++++++++++++-- src/Schema/AST/DocumentAST.php | 1 - .../Federation/FederationSchemaTest.php | 18 ++++---- tests/Unit/Schema/AST/DocumentASTTest.php | 6 +-- tests/Unit/Schema/DirectiveLocatorTest.php | 2 +- tests/Utils/Directives/BarDirective.php | 18 ++++++++ 9 files changed, 122 insertions(+), 71 deletions(-) create mode 100644 src/Federation/FederationHelper.php create mode 100644 tests/Utils/Directives/BarDirective.php diff --git a/src/Federation/FederationHelper.php b/src/Federation/FederationHelper.php new file mode 100644 index 000000000..29b57acdc --- /dev/null +++ b/src/Federation/FederationHelper.php @@ -0,0 +1,46 @@ + */ + public static function composedDirectives(Schema $schema): array + { + $composedDirectives = []; + + foreach ($schema->extensionASTNodes as $extension) { + foreach (ASTHelper::directiveDefinitions($extension, 'composeDirective') as $directive) { + $name = ASTHelper::directiveArgValue($directive, 'name'); + + if (! is_string($name)) { + continue; + } + + $composedDirectives[] = ltrim($name, '@'); + } + } + + return $composedDirectives; + } + + /** @return array */ + public static function schemaDirectives(Schema $schema): array + { + $schemaDirectives = []; + + foreach ($schema->extensionASTNodes as $extension) { + foreach ($extension->directives as $directive) { + assert($directive instanceof DirectiveNode); + + $schemaDirectives[] = $directive; + } + } + + return $schemaDirectives; + } +} diff --git a/src/Federation/FederationPrinter.php b/src/Federation/FederationPrinter.php index 5a813f8fb..0c6868cb4 100644 --- a/src/Federation/FederationPrinter.php +++ b/src/Federation/FederationPrinter.php @@ -2,11 +2,8 @@ namespace Nuwave\Lighthouse\Federation; -use GraphQL\Language\AST\ArgumentNode; -use GraphQL\Language\AST\DirectiveNode; use GraphQL\Language\AST\FieldDefinitionNode; use GraphQL\Language\AST\ObjectTypeDefinitionNode; -use GraphQL\Language\AST\StringValueNode; use GraphQL\Type\Definition\Argument; use GraphQL\Type\Definition\Directive; use GraphQL\Type\Definition\EnumType; @@ -97,7 +94,7 @@ public static function print(Schema $schema): string $federationDirectives = array_merge( static::FEDERATION_DIRECTIVES, - static::getComposedDirectives($schema), + FederationHelper::composedDirectives($schema), ); $config->setDirectives(array_filter( @@ -157,28 +154,4 @@ public static function print(Schema $schema): string ['printDirectives' => $printDirectives], ); } - - /** @return array */ - protected static function getComposedDirectives(Schema $schema): array - { - $composedDirectives = []; - - foreach ($schema->extensionASTNodes as $extension) { - foreach ($extension->directives as $directive) { - assert($directive instanceof DirectiveNode); - - if ($directive->name->value === 'composeDirective') { - foreach ($directive->arguments as $argument) { - assert($argument instanceof ArgumentNode); - - if ($argument->name->value === 'name' && $argument->value instanceof StringValueNode) { - $composedDirectives[] = ltrim($argument->value->value, '@'); - } - } - } - } - } - - return $composedDirectives; - } } diff --git a/src/Federation/SchemaPrinter.php b/src/Federation/SchemaPrinter.php index 331dfce27..65c783637 100644 --- a/src/Federation/SchemaPrinter.php +++ b/src/Federation/SchemaPrinter.php @@ -2,9 +2,7 @@ namespace Nuwave\Lighthouse\Federation; -use GraphQL\Language\AST\ArgumentNode; use GraphQL\Language\AST\DirectiveNode; -use GraphQL\Language\AST\StringValueNode; use GraphQL\Language\Printer; use GraphQL\Type\Definition\FieldDefinition; use GraphQL\Type\Definition\InterfaceType; @@ -18,35 +16,18 @@ class SchemaPrinter extends GraphQLSchemaPrinter { protected static function printSchemaDefinition(Schema $schema): string { - $composedDirectives = []; - $schemaDirectives = []; - - foreach ($schema->extensionASTNodes as $extension) { - foreach ($extension->directives as $directive) { - assert($directive instanceof DirectiveNode); - - $schemaDirectives[] = $directive; - - if ($directive->name->value === 'composeDirective') { - foreach ($directive->arguments as $argument) { - assert($argument instanceof ArgumentNode); - - if ($argument->name->value === 'name' && $argument->value instanceof StringValueNode) { - $composedDirectives[] = ltrim($argument->value->value, '@'); - } - } - } - } - } + $schemaDirectives = FederationHelper::schemaDirectives($schema); $result = 'extend schema' . self::printDirectives($schemaDirectives) . "\n"; - if ($composedDirectives !== []) { + $directivesToPreserve = FederationHelper::composedDirectives($schema); + + if ($directivesToPreserve !== []) { $directiveLocator = Container::getInstance()->make(DirectiveLocator::class); $result .= "\n" . implode("\n", array_map( - static fn (string $directive): string => $directiveLocator->create($composedDirectives[0])->definition(), - $composedDirectives, + static fn (string $directive): string => $directiveLocator->create($directive)->definition(), + $directivesToPreserve, )); } diff --git a/src/Schema/AST/ASTHelper.php b/src/Schema/AST/ASTHelper.php index 71f729da4..69dafc0c9 100644 --- a/src/Schema/AST/ASTHelper.php +++ b/src/Schema/AST/ASTHelper.php @@ -178,6 +178,22 @@ public static function defaultValueForArgument(ValueNode $defaultValue, Type $ar * As of now, directives may only be used once per location. */ public static function directiveDefinition(Node $definitionNode, string $name): ?DirectiveNode + { + foreach (static::directiveDefinitions($definitionNode, $name) as $directive) { + return $directive; + } + + return null; + } + + /** + * Get a directive with the given name if it is defined upon the node. + * + * As of now, directives may only be used once per location. + * + * @return iterable<\GraphQL\Language\AST\DirectiveNode> + */ + public static function directiveDefinitions(Node $definitionNode, string $name): iterable { if (! property_exists($definitionNode, 'directives')) { throw new \Exception('Expected Node class with property `directives`, got: ' . $definitionNode::class); @@ -186,7 +202,7 @@ public static function directiveDefinition(Node $definitionNode, string $name): /** @var \GraphQL\Language\AST\NodeList<\GraphQL\Language\AST\DirectiveNode> $directives */ $directives = $definitionNode->directives; - return static::firstByName($directives, $name); + return static::filterByName($directives, $name); } /** Check if a node has a directive with the given name on it. */ @@ -196,15 +212,15 @@ public static function hasDirective(Node $definitionNode, string $name): bool } /** - * Out of a list of nodes, get the first that matches the given name. + * Out of a list of nodes, get the ones that matches the given name. * * @template TNode of \GraphQL\Language\AST\Node * * @param iterable $nodes * - * @return TNode|null + * @return iterable */ - public static function firstByName(iterable $nodes, string $name): ?Node + public static function filterByName(iterable $nodes, string $name): iterable { foreach ($nodes as $node) { if (! property_exists($node, 'name')) { @@ -212,9 +228,25 @@ public static function firstByName(iterable $nodes, string $name): ?Node } if ($node->name->value === $name) { - return $node; + yield $node; } } + } + + /** + * Out of a list of nodes, get the first that matches the given name. + * + * @template TNode of \GraphQL\Language\AST\Node + * + * @param iterable $nodes + * + * @return TNode|null + */ + public static function firstByName(iterable $nodes, string $name): ?Node + { + foreach (static::filterByName($nodes, $name) as $node) { + return $node; + } return null; } diff --git a/src/Schema/AST/DocumentAST.php b/src/Schema/AST/DocumentAST.php index 0b7319e6a..ad8cb6dbc 100644 --- a/src/Schema/AST/DocumentAST.php +++ b/src/Schema/AST/DocumentAST.php @@ -243,7 +243,6 @@ protected function hydrateFromArray(array $ast): void // @phpstan-ignore-next-line Since we start from the array form, the generic type does not match $this->directives = new NodeList($directives); // @phpstan-ignore-next-line Since we start from the array form, the generic type does not match - $this->schemaExtensions = array_map(fn (array $node): SchemaExtensionNode => $this->hydrateSchemaExtension($node), $schemaExtensions); } diff --git a/tests/Integration/Federation/FederationSchemaTest.php b/tests/Integration/Federation/FederationSchemaTest.php index a2fbe03a4..779ae5ba2 100644 --- a/tests/Integration/Federation/FederationSchemaTest.php +++ b/tests/Integration/Federation/FederationSchemaTest.php @@ -18,8 +18,7 @@ protected function getPackageProviders($app): array public function testServiceQueryShouldReturnValidSdl(): void { - $foo /** @lang GraphQL */ - = <<<'GRAPHQL' + $foo = /** @lang GraphQL */ <<<'GRAPHQL' type Foo @key(fields: "id") { id: ID! @external foo: String! @@ -27,8 +26,7 @@ public function testServiceQueryShouldReturnValidSdl(): void GRAPHQL; - $query /** @lang GraphQL */ - = <<<'GRAPHQL' + $query = /** @lang GraphQL */ <<<'GRAPHQL' type Query { foo: Int! } @@ -45,8 +43,7 @@ public function testServiceQueryShouldReturnValidSdl(): void public function testServiceQueryShouldReturnValidSdlWithoutQuery(): void { - $foo /** @lang GraphQL */ - = <<<'GRAPHQL' + $foo = /** @lang GraphQL */ <<<'GRAPHQL' type Foo @key(fields: "id") { id: ID! @external foo: String! @@ -91,7 +88,9 @@ public function testFederatedSchemaShouldContainCorrectEntityUnion(): void public function testServiceQueryShouldReturnFederationV2SchemaExtension(): void { - $schemaExtension = 'extend schema @link(url: "https:\/\/specs.apollo.dev\/federation\/v2.3", import: ["@composeDirective", "@extends", "@external", "@inaccessible", "@interfaceObject", "@key", "@override", "@provides", "@requires", "@shareable", "@tag"])'; + $schemaExtension = /** @lang GraphQL */ <<<'GRAPHQL' +extend schema @link(url: "https:\/\/specs.apollo.dev\/federation\/v2.3", import: ["@composeDirective", "@extends", "@external", "@inaccessible", "@interfaceObject", "@key", "@override", "@provides", "@requires", "@shareable", "@tag"]) +GRAPHQL; $foo = /** @lang GraphQL */ <<<'GRAPHQL' type Foo @key(fields: "id") { @@ -109,7 +108,9 @@ public function testServiceQueryShouldReturnFederationV2SchemaExtension(): void public function testServiceQueryShouldReturnFederationV2ComposedDirectives(): void { - $schemaExtension = 'extend schema @link(url: "https:\/\/specs.apollo.dev\/federation\/v2.3", import: ["@composeDirective"]) @link(url: "https:\/\/myspecs.dev\/myCustomDirective\/v1.0", import: ["@foo"]) @composeDirective(name: "@foo")'; + $schemaExtension = /** @lang GraphQL */ <<<'GRAPHQL' +extend schema @link(url: "https:\/\/specs.apollo.dev\/federation\/v2.3", import: ["@composeDirective"]) @link(url: "https:\/\/myspecs.dev\/myCustomDirective\/v1.0", import: ["@foo", "@bar"]) @composeDirective(name: "@foo") @composeDirective(name: "@bar") +GRAPHQL; $foo = /** @lang GraphQL */ <<<'GRAPHQL' type Foo @key(fields: "id") { @@ -123,6 +124,7 @@ public function testServiceQueryShouldReturnFederationV2ComposedDirectives(): vo $this->assertStringContainsString($schemaExtension, $sdl); $this->assertStringContainsString('directive @foo on FIELD_DEFINITION', $sdl); + $this->assertStringContainsString('directive @bar on FIELD_DEFINITION', $sdl); $this->assertStringContainsString('type Foo', $sdl); } diff --git a/tests/Unit/Schema/AST/DocumentASTTest.php b/tests/Unit/Schema/AST/DocumentASTTest.php index 0c019a86b..502a3add0 100644 --- a/tests/Unit/Schema/AST/DocumentASTTest.php +++ b/tests/Unit/Schema/AST/DocumentASTTest.php @@ -4,7 +4,6 @@ use GraphQL\Language\AST\DirectiveDefinitionNode; use GraphQL\Language\AST\DirectiveNode; -use GraphQL\Language\AST\NodeList; use GraphQL\Language\AST\ObjectTypeDefinitionNode; use GraphQL\Language\AST\SchemaExtensionNode; use GraphQL\Language\Parser; @@ -109,7 +108,8 @@ public function testBeSerialized(): void $this->assertSame(['Query'], $reserialized->classNameToObjectTypeNames[User::class]); - $this->assertInstanceOf(SchemaExtensionNode::class, $reserialized->schemaExtensions[0]); - $this->assertInstanceOf(DirectiveNode::class, $reserialized->schemaExtensions[0]->directives[0]); + $schemaExtension = $reserialized->schemaExtensions[0]; + $this->assertInstanceOf(SchemaExtensionNode::class, $schemaExtension); + $this->assertInstanceOf(DirectiveNode::class, $schemaExtension->directives[0]); } } diff --git a/tests/Unit/Schema/DirectiveLocatorTest.php b/tests/Unit/Schema/DirectiveLocatorTest.php index 046f12860..84eaa8749 100644 --- a/tests/Unit/Schema/DirectiveLocatorTest.php +++ b/tests/Unit/Schema/DirectiveLocatorTest.php @@ -79,7 +79,7 @@ public function testThrowsIfDirectiveNameCanNotBeResolved(): void { $this->expectException(DirectiveException::class); - $this->directiveLocator->create('bar'); + $this->directiveLocator->create('foobar'); } public function testCreateSingleDirective(): void diff --git a/tests/Utils/Directives/BarDirective.php b/tests/Utils/Directives/BarDirective.php new file mode 100644 index 000000000..af7366d4a --- /dev/null +++ b/tests/Utils/Directives/BarDirective.php @@ -0,0 +1,18 @@ +