fixes

nuwave · Nov 23, 2023 · 8733152 · 8733152
1 parent 1c0c1da
commit 8733152
Show file tree

Hide file tree

Showing 9 changed files with 122 additions and 71 deletions.
diff --git a/src/Federation/FederationHelper.php b/src/Federation/FederationHelper.php
@@ -0,0 +1,46 @@
+<?php declare(strict_types=1);
+
+namespace Nuwave\Lighthouse\Federation;
+
+use GraphQL\Language\AST\DirectiveNode;
+use GraphQL\Type\Schema;
+use Nuwave\Lighthouse\Schema\AST\ASTHelper;
+
+class FederationHelper
+{
+    /** @return array<string> */
+    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<DirectiveNode> */
+    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
@@ -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<string> */
-    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
@@ -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
@@ -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,25 +212,41 @@ 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<TNode>  $nodes
      *
-     * @return TNode|null
+     * @return iterable<TNode>
      */
-    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')) {
                 throw new \Exception('Expected a Node with a name property, got: ' . $node::class);
             }
 
             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<TNode>  $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
@@ -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
@@ -18,17 +18,15 @@ 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!
 }
 
 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
@@ -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
@@ -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
@@ -0,0 +1,18 @@
+<?php declare(strict_types=1);
+
+namespace Tests\Utils\Directives;
+
+use Nuwave\Lighthouse\Schema\Directives\BaseDirective;
+
+final class BarDirective extends BaseDirective
+{
+    public static function definition(): string
+    {
+        return /** @lang GraphQL */ <<<'GRAPHQL'
+"""
+Maximum foo.
+"""
+directive @bar on FIELD_DEFINITION
+GRAPHQL;
+    }
+}