Repository: webonyx/graphql-php (php)

Issue:
`Schema::getType()` shoud always return `null` for non-existent type
There is an inconsistency in how `Schema::getType()` behaves depending on how the schema is created.

If the schema is created using type definitions, it correctly returns `null` for a type that doesn't exist in the schema:

```php
$schema = new Schema([
    'types' => [
        new ObjectType([
            'name' => 'Foo',
            'fields' => ['id' => Type::id()],
        ]),
    ],
]);

$foo = $schema->getType('Foo');
$bar = $schema->getType('Bar'); // ✅ returns null as expected
```

If the schema is built from SDL, it throws `Unknown type: "Bar".` error instead or returning `null`:

```php
$schema = BuildSchema::build('type Foo { id: ID }');

$foo = $schema->getType('Foo');
$bar = $schema->getType('Bar'); // 💥 throws error
```

This has a negative impact for example on SDL validation rules.

For reference, `graphql-js` always returns `undefined` for a non-existent type, regardless of how the schema was created.

I'm trying to wrap my head around how the lazy type loading works (it seems related, at least partially). Any suggestions on how to fix this without a significant rewrite of `BuildSchema`, `SchemaExtender` and `ASTDefinitionBuilder` are welcome. (However, I'm hitting various problems with schema building & extending from different directions, so I'm more and more convinced that rewriting them in accordance with the reference implementation is actually the only reasonable solution.) 

Pull request description:
Fix schema built from SDL to return null for unknown types
Fixes #997 (and unblocks #998 and #999).

### History background
The implementation of `BuildSchema`, `SchemaExtender` and `ASTDefinitionBuilder` roughly matches their counterparts from `graphql-js` v14.0.

However, during the lifetime of v14, the JS code underwent significant changes. In the final version 14.7, `buildASTSchema()`, `extendSchema()` and `ASTDefinitionBuilder` looks much different then they did in v14.0. For the context of this PR, the most important changes (https://github.com/graphql/graphql-js/commit/afc6b7c04708068e3733d8e85882e4b2e4ef58e5) were that  `ASTDefinitionBuilder`'s constructor didn't accept `typeDefinitionsMap` param any more, and that the original single method `buildType()` was replaced with a new `buildType()` and `getNamedType()`. Each was used in a different context, and only the latter could throw the `"Unknown type"` error.

These changes to the reference implementation have never been reflected in `graphql-php`.

### Implementation notes
The context is everything. Sometimes it's okay for the builder to return `null` instead of throwing `"Unknown type"` error – like in this case when it's requested to build a type that isn't defined in the SDL schema.

To handle this explicitly, I added the new `maybeBuildType(string $name): ?Type`  method to the builder.

`graphql-js` (v14.2+) builds `typeMap` with all types from SDL ahead. Then calling `typeMap[typeName]` returns either a type or `undefined`.

The new `maybeBuildType()` provides similar functionality for the lazy environment where we don't have such a map and where the individual types are built from the SDL schema on demand.

### Additional notes
In v15, `graphql-js` has undergone even more significant changes and `ASTDefinitionBuilder` has been removed completely (https://github.com/graphql/graphql-js/commit/3948aa20d9c38c5d4aff06f1b3af2538b20f909a). 
To ensure future compatibility with the reference implementation, it would be worth considering reimplementing schema builder and extender completely.


Patch:
diff --git a/CHANGELOG.md b/CHANGELOG.md
index 1e4acbe2..492cce8c 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -37,6 +37,7 @@ You can find and compare releases at the [GitHub release page](https://github.co
 - Always convert promises through `PromiseAdapter::convertThenable()` before calling `->then()` on them
 - Use `JSON_THROW_ON_ERROR` in `json_encode()`
 - Validate some internal invariants through `assert()`
+- `PromiseAdapter::all()` accepts `iterable`
 
 ### Added
 
@@ -67,6 +68,7 @@ You can find and compare releases at the [GitHub release page](https://github.co
 - Preserve extended methods from class-based types in `SchemaExtender::extend()`
 - Fix printing of empty types (#940)
 - Clone `NodeList` in `Node::cloneDeep()`
+- Calling `Schema::getType()` on a schema built from SDL returns `null` for unknown types (#1068)
 
 ### Removed
 
diff --git a/phpunit.xml.dist b/phpunit.xml.dist
index c7aaac02..936a6173 100644
--- a/phpunit.xml.dist
+++ b/phpunit.xml.dist
@@ -17,9 +17,4 @@
       <directory>./tests/</directory>
     </testsuite>
   </testsuites>
-  <groups>
-    <exclude>
-      <group>ReactPromise</group>
-    </exclude>
-  </groups>
 </phpunit>
diff --git a/src/Executor/Promise/Adapter/AmpPromiseAdapter.php b/src/Executor/Promise/Adapter/AmpPromiseAdapter.php
index ce3f480b..2f5a7cde 100644
--- a/src/Executor/Promise/Adapter/AmpPromiseAdapter.php
+++ b/src/Executor/Promise/Adapter/AmpPromiseAdapter.php
@@ -8,8 +8,10 @@ use function Amp\Promise\all;
 use Amp\Promise as AmpPromise;
 use Amp\Success;
 use function array_replace;
+use function assert;
 use GraphQL\Executor\Promise\Promise;
 use GraphQL\Executor\Promise\PromiseAdapter;
+use function is_array;
 use Throwable;
 
 class AmpPromiseAdapter implements PromiseAdapter
@@ -77,8 +79,13 @@ class AmpPromiseAdapter implements PromiseAdapter
         return new Promise($promise, $this);
     }
 
-    public function all(array $promisesOrValues): Promise
+    public function all(iterable $promisesOrValues): Promise
     {
+        assert(
+            is_array($promisesOrValues),
+            'AmpPromiseAdapter::all(): Argument #1 ($promisesOrValues) must be of type array'
+        );
+
         /** @var array<AmpPromise<mixed>> $promises */
         $promises = [];
         foreach ($promisesOrValues as $key => $item) {
diff --git a/src/Executor/Promise/Adapter/ReactPromiseAdapter.php b/src/Executor/Promise/Adapter/ReactPromiseAdapter.php
index bb9c5db8..41c408d2 100644
--- a/src/Executor/Promise/Adapter/ReactPromiseAdapter.php
+++ b/src/Executor/Promise/Adapter/ReactPromiseAdapter.php
@@ -52,8 +52,13 @@ class ReactPromiseAdapter implements PromiseAdapter
         return new Promise($promise, $this);
     }
 
-    public function all(array $promisesOrValues): Promise
+    public function all(iterable $promisesOrValues): Promise
     {
+        assert(
+            is_array($promisesOrValues),
+            'ReactPromiseAdapter::all(): Argument #1 ($promisesOrValues) must be of type array'
+        );
+
         // TODO: rework with generators when PHP minimum required version is changed to 5.5+
 
         foreach ($promisesOrValues as &$promiseOrValue) {
diff --git a/src/Executor/Promise/Adapter/SyncPromiseAdapter.php b/src/Executor/Promise/Adapter/SyncPromiseAdapter.php
index b64cc215..2c4f3c5b 100644
--- a/src/Executor/Promise/Adapter/SyncPromiseAdapter.php
+++ b/src/Executor/Promise/Adapter/SyncPromiseAdapter.php
@@ -2,12 +2,14 @@
 
 namespace GraphQL\Executor\Promise\Adapter;
 
+use function assert;
 use function count;
 use GraphQL\Deferred;
 use GraphQL\Error\InvariantViolation;
 use GraphQL\Executor\Promise\Promise;
 use GraphQL\Executor\Promise\PromiseAdapter;
 use GraphQL\Utils\Utils;
+use function is_array;
 use Throwable;
 
 /**
@@ -72,8 +74,13 @@ class SyncPromiseAdapter implements PromiseAdapter
         return new Promise($promise->reject($reason), $this);
     }
 
-    public function all(array $promisesOrValues): Promise
+    public function all(iterable $promisesOrValues): Promise
     {
+        assert(
+            is_array($promisesOrValues),
+            'SyncPromiseAdapter::all(): Argument #1 ($promisesOrValues) must be of type array'
+        );
+
         $all = new SyncPromise();
 
         $total = count($promisesOrValues);
diff --git a/src/Executor/Promise/PromiseAdapter.php b/src/Executor/Promise/PromiseAdapter.php
index c5f931b1..edc6c238 100644
--- a/src/Executor/Promise/PromiseAdapter.php
+++ b/src/Executor/Promise/PromiseAdapter.php
@@ -63,12 +63,12 @@ interface PromiseAdapter
     public function createRejected(Throwable $reason): Promise;
 
     /**
-     * Given an array of promises (or values), returns a promise that is fulfilled when all the
-     * items in the array are fulfilled.
+     * Given an iterable of promises (or values), returns a promise that is fulfilled when all the
+     * items in the iterable are fulfilled.
      *
-     * @param array<Promise|mixed> $promisesOrValues
+     * @param iterable<Promise|mixed> $promisesOrValues
      *
      * @api
      */
-    public function all(array $promisesOrValues): Promise;
+    public function all(iterable $promisesOrValues): Promise;
 }
diff --git a/src/Utils/ASTDefinitionBuilder.php b/src/Utils/ASTDefinitionBuilder.php
index 24495d09..c84230df 100644
--- a/src/Utils/ASTDefinitionBuilder.php
+++ b/src/Utils/ASTDefinitionBuilder.php
@@ -34,7 +34,6 @@ use GraphQL\Type\Definition\OutputType;
 use GraphQL\Type\Definition\Type;
 use GraphQL\Type\Definition\UnionType;
 use function is_array;
-use function is_string;
 use Throwable;
 
 /**
@@ -162,6 +161,19 @@ class ASTDefinitionBuilder
         return $this->internalBuildType($ref->name->value, $ref);
     }
 
+    /**
+     * Calling this method is an equivalent of `typeMap[typeName]` in `graphql-js`.
+     * It is legal to access a type from the map of already-built types that doesn't exist in the map.
+     * Since we build types lazily, and we don't have a such map of built types,
+     * this method provides a way to build a type that may not exist in the SDL definitions and returns null instead.
+     */
+    public function maybeBuildType(string $name): ?Type
+    {
+        return isset($this->typeDefinitionsMap[$name])
+            ? $this->buildType($name)
+            : null
+    }
+
     /**
      * @param (Node&NamedTypeNode)|(Node&TypeDefinitionNode)|null $typeNode
      *
diff --git a/src/Utils/BuildSchema.php b/src/Utils/BuildSchema.php
index a684a56e..ac38caf7 100644
--- a/src/Utils/BuildSchema.php
+++ b/src/Utils/BuildSchema.php
@@ -156,14 +156,14 @@ class BuildSchema
         $operationTypes = null !== $schemaDef
             ? $this->getOperationTypes($schemaDef)
             : [
-                'query' => isset($this->nodeMap['Query']) ? 'Query' : null,
-                'mutation' => isset($this->nodeMap['Mutation']) ? 'Mutation' : null,
-                'subscription' => isset($this->nodeMap['Subscription']) ? 'Subscription' : null,
+                'query' => 'Query',
+                'mutation' => 'Mutation',
+                'subscription' => 'Subscription',
             ];
 
         $definitionBuilder = new ASTDefinitionBuilder(
             $this->nodeMap,
-            static function (string $typeName): void {
+            static function (string $typeName): Type {
                 throw self::unknownType($typeName);
             },
             $this->typeConfigDecorator
@@ -198,15 +198,15 @@ class BuildSchema
 
         return new Schema([
             'query' => isset($operationTypes['query'])
-                ? $definitionBuilder->buildType($operationTypes['query'])
+                ? $definitionBuilder->maybeBuildType($operationTypes['query'])
                 : null,
             'mutation' => isset($operationTypes['mutation'])
-                ? $definitionBuilder->buildType($operationTypes['mutation'])
+                ? $definitionBuilder->maybeBuildType($operationTypes['mutation'])
                 : null,
             'subscription' => isset($operationTypes['subscription'])
-                ? $definitionBuilder->buildType($operationTypes['subscription'])
+                ? $definitionBuilder->maybeBuildType($operationTypes['subscription'])
                 : null,
-            'typeLoader' => static fn (string $name): Type => $definitionBuilder->buildType($name),
+            'typeLoader' => static fn (string $name): ?Type => $definitionBuilder->maybeBuildType($name),
             'directives' => $directives,
             'astNode' => $schemaDef,
             'types' => fn (): array => array_map(
diff --git a/src/Utils/SchemaExtender.php b/src/Utils/SchemaExtender.php
index a1c58c8a..f780b176 100644
--- a/src/Utils/SchemaExtender.php
+++ b/src/Utils/SchemaExtender.php
@@ -580,11 +580,7 @@ class SchemaExtender
                     ? $def->name->value
                     : null;
 
-                try {
-                    $type = $schema->getType($typeName);
-                } catch (Error $error) {
-                    $type = null;
-                }
+                $type = $schema->getType($typeName);
 
                 if (null !== $type) {
                     throw new Error('Type "' . $typeName . '" already exists in the schema. It cannot also be defined in this type definition.', [$def]);


Test patch:
diff --git a/tests/Executor/Promise/ReactPromiseAdapterTest.php b/tests/Executor/Promise/ReactPromiseAdapterTest.php
index 6821ac13..f29cb0f2 100644
--- a/tests/Executor/Promise/ReactPromiseAdapterTest.php
+++ b/tests/Executor/Promise/ReactPromiseAdapterTest.php
@@ -7,10 +7,11 @@ use GraphQL\Executor\Promise\Adapter\ReactPromiseAdapter;
 use PHPUnit\Framework\TestCase;
 use React\Promise\Deferred;
 use React\Promise\FulfilledPromise;
-use React\Promise\LazyPromise;
 use React\Promise\Promise;
 use React\Promise\Promise as ReactPromise;
+use function React\Promise\reject;
 use React\Promise\RejectedPromise;
+use function React\Promise\resolve;
 use stdClass;
 
 /**
@@ -26,12 +27,8 @@ class ReactPromiseAdapterTest extends TestCase
             $reactAdapter->isThenable(new ReactPromise(static function (): void {
             }))
         );
-        self::assertTrue($reactAdapter->isThenable(new FulfilledPromise()));
-        self::assertTrue($reactAdapter->isThenable(new RejectedPromise()));
-        self::assertTrue(
-            $reactAdapter->isThenable(new LazyPromise(static function (): void {
-            }))
-        );
+        self::assertTrue($reactAdapter->isThenable(resolve()));
+        self::assertTrue($reactAdapter->isThenable(reject()));
         self::assertFalse($reactAdapter->isThenable(false));
         self::assertFalse($reactAdapter->isThenable(true));
         self::assertFalse($reactAdapter->isThenable(1));
@@ -45,7 +42,7 @@ class ReactPromiseAdapterTest extends TestCase
     public function testConvertsReactPromisesToGraphQlOnes(): void
     {
         $reactAdapter = new ReactPromiseAdapter();
-        $reactPromise = new FulfilledPromise(1);
+        $reactPromise = resolve(1);
 
         $promise = $reactAdapter->convertThenable($reactPromise);
 
@@ -55,7 +52,7 @@ class ReactPromiseAdapterTest extends TestCase
     public function testThen(): void
     {
         $reactAdapter = new ReactPromiseAdapter();
-        $reactPromise = new FulfilledPromise(1);
+        $reactPromise = resolve(1);
         $promise = $reactAdapter->convertThenable($reactPromise);
 
         $result = null;
@@ -128,7 +125,7 @@ class ReactPromiseAdapterTest extends TestCase
     public function testAll(): void
     {
         $reactAdapter = new ReactPromiseAdapter();
-        $promises = [new FulfilledPromise(1), new FulfilledPromise(2), new FulfilledPromise(3)];
+        $promises = [resolve(1), resolve(2), resolve(3)];
 
         $allPromise = $reactAdapter->all($promises);
 
@@ -147,7 +144,7 @@ class ReactPromiseAdapterTest extends TestCase
     {
         $reactAdapter = new ReactPromiseAdapter();
         $deferred = new Deferred();
-        $promises = [new FulfilledPromise(1), $deferred->promise(), new FulfilledPromise(3)];
+        $promises = [resolve(1), $deferred->promise(), resolve(3)];
         $result = null;
 
         $reactAdapter->all($promises)->then(static function ($values) use (&$result): void {
diff --git a/tests/Type/SchemaTest.php b/tests/Type/SchemaTest.php
index d47d390a..60ceff69 100644
--- a/tests/Type/SchemaTest.php
+++ b/tests/Type/SchemaTest.php
@@ -123,4 +123,12 @@ class SchemaTest extends TestCase
         self::assertArrayHasKey('DirInput', $typeMap);
         self::assertArrayHasKey('WrappedDirInput', $typeMap);
     }
+
+    /**
+     * @see https://github.com/webonyx/graphql-php/issues/997
+     */
+    public function testSchemaReturnsNullForNonexistentType(): void
+    {
+        self::assertNull($this->schema->getType('UnknownType'));
+    }
 }
diff --git a/tests/Utils/BuildSchemaTest.php b/tests/Utils/BuildSchemaTest.php
index b9f5de18..6050ca41 100644
--- a/tests/Utils/BuildSchemaTest.php
+++ b/tests/Utils/BuildSchemaTest.php
@@ -887,6 +887,15 @@ type Query {
 ', null, ['assumeValidSDL' => true])->assertValid();
     }
 
+    /**
+     * @see https://github.com/webonyx/graphql-php/issues/997
+     */
+    public function testBuiltSchemaReturnsNullForNonexistentType(): void
+    {
+        $schema = BuildSchema::build('type KnownType');
+        self::assertNull($schema->getType('UnknownType'));
+    }
+
     public function testSupportsTypeConfigDecorator(): void
     {
         $body = '