Skip to content

Commit b868a6f

Browse files
spawniaspawnia
authored
Move runtime checks to schema validation (#1047)
1 parent 712e525 commit b868a6f

File tree

11 files changed

+154
-60
lines changed

11 files changed

+154
-60
lines changed

‎.github/workflows/test.yml‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -36,7 +36,7 @@ jobs:
3636
with:
3737
php-version: ${{ matrix.php-version }}
3838
coverage: pcov
39-
ini-values: zend.assertions=1
39+
ini-values: zend.assertions=1, assert.exception=1
4040

4141
- name: Install dependencies with Composer
4242
uses: ramsey/composer-install@v2

‎CHANGELOG.md‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -51,6 +51,7 @@ You can find and compare releases at the [GitHub release page](https://github.co
5151
- Sync input value coercion with `graphql-js` reference implementation
5252
- Store rules exclusively by class name in `DocumentValidator`
5353
- Reorder standard types as described in the GraphQL specification
54+
- Improve runtime performance by moving checks for duplicate/mismatching type instances to `assert()` or schema validation
5455

5556
### Added
5657

‎src/Executor/ReferenceExecutor.php‎

Lines changed: 13 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -36,6 +36,7 @@
3636
use GraphQL\Type\Definition\Type;
3737
use GraphQL\Type\Introspection;
3838
use GraphQL\Type\Schema;
39+
use GraphQL\Type\SchemaValidationContext;
3940
use GraphQL\Utils\AST;
4041
use GraphQL\Utils\Utils;
4142
use SplObjectStorage;
@@ -848,14 +849,10 @@ protected function completeValue(
848849

849850
// Account for invalid schema definition when typeLoader returns different
850851
// instance than `resolveType` or $field->getType() or $arg->getType()
851-
$schema = $this->exeContext->schema;
852-
if ($returnType !== $schema->getType($returnType->name)) {
853-
$hint = $schema->getConfig()->typeLoader !== null
854-
? "Ensure the type loader returns the same instance as defined in {$info->parentType}.{$info->fieldName}. "
855-
: '';
856-
857-
throw new InvariantViolation("Found duplicate type in schema: {$returnType}. {$hint}See https://webonyx.github.io/graphql-php/type-definitions/#type-registry.");
858-
}
852+
assert(
853+
$returnType === $this->exeContext->schema->getType($returnType->name),
854+
SchemaValidationContext::duplicateType($this->exeContext->schema, "{$info->parentType}.{$info->fieldName}", $returnType->name)
855+
);
859856

860857
if ($returnType instanceof LeafType) {
861858
return $this->completeLeafValue($returnType, $result);
@@ -1356,13 +1353,15 @@ protected function ensureValidRuntimeType(
13561353
throw new InvariantViolation("Runtime Object type \"{$runtimeType}\" is not a possible type for \"{$returnType}\".");
13571354
}
13581355

1359-
if ($this->exeContext->schema->getType($runtimeType->name) === null) {
1360-
throw new InvariantViolation("Schema does not contain type \"{$runtimeType}\". This can happen when an object type is only referenced indirectly through abstract types and never directly through fields.List the type in the option \"types\" during schema construction, see https://webonyx.github.io/graphql-php/schema-definition/#configuration-options.");
1361-
}
1356+
assert(
1357+
$this->exeContext->schema->getType($runtimeType->name) !== null,
1358+
"Schema does not contain type \"{$runtimeType}\". This can happen when an object type is only referenced indirectly through abstract types and never directly through fields.List the type in the option \"types\" during schema construction, see https://webonyx.github.io/graphql-php/schema-definition/#configuration-options."
1359+
);
13621360

1363-
if ($runtimeType !== $this->exeContext->schema->getType($runtimeType->name)) {
1364-
throw new InvariantViolation("Schema must contain unique named types but contains multiple types named \"{$runtimeType}\". Make sure that `resolveType` function of abstract type \"{$returnType}\" returns the same type instance as referenced anywhere else within the schema (see https://webonyx.github.io/graphql-php/type-definitions/#type-registry).");
1365-
}
1361+
assert(
1362+
$runtimeType === $this->exeContext->schema->getType($runtimeType->name),
1363+
"Schema must contain unique named types but contains multiple types named \"{$runtimeType}\". Make sure that `resolveType` function of abstract type \"{$returnType}\" returns the same type instance as referenced anywhere else within the schema (see https://webonyx.github.io/graphql-php/type-definitions/#type-registry)."
1364+
);
13661365

13671366
return $runtimeType;
13681367
}

‎src/Type/Schema.php‎

Lines changed: 4 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -125,11 +125,10 @@ public function getTypeMap(): array
125125
assert($type instanceof NamedType);
126126

127127
$typeName = $type->name;
128-
if (isset($this->resolvedTypes[$typeName])) {
129-
if ($type !== $this->resolvedTypes[$typeName]) {
130-
throw new InvariantViolation("Schema must contain unique named types but contains multiple types named \"{$type}\" (see https://webonyx.github.io/graphql-php/type-definitions/#type-registry).");
131-
}
132-
}
128+
assert(
129+
! isset($this->resolvedTypes[$typeName]) || $type === $this->resolvedTypes[$typeName],
130+
"Schema must contain unique named types but contains multiple types named \"{$type}\" (see https://webonyx.github.io/graphql-php/type-definitions/#type-registry).",
131+
);
133132

134133
$this->resolvedTypes[$typeName] = $type;
135134
}

‎src/Type/SchemaValidationContext.php‎

Lines changed: 33 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@
33
namespace GraphQL\Type;
44

55
use GraphQL\Error\Error;
6+
use GraphQL\Error\InvariantViolation;
67
use GraphQL\Language\AST\DirectiveDefinitionNode;
78
use GraphQL\Language\AST\DirectiveNode;
89
use GraphQL\Language\AST\EnumTypeDefinitionNode;
@@ -343,6 +344,8 @@ private function validateFields(Type $type): void
343344
);
344345
}
345346

347+
$this->validateTypeIsSingleton($fieldType, "{$type->name}.{$fieldName}");
348+
346349
$argNames = [];
347350
foreach ($field->args as $arg) {
348351
$argName = $arg->name;
@@ -370,6 +373,8 @@ private function validateFields(Type $type): void
370373
);
371374
}
372375

376+
$this->validateTypeIsSingleton($argType, $argPath);
377+
373378
if (isset($arg->astNode->directives)) {
374379
$this->validateDirectivesAtLocation($arg->astNode->directives, DirectiveLocation::ARGUMENT_DEFINITION);
375380
}
@@ -826,4 +831,32 @@ private function validateInputFields(InputObjectType $inputObj): void
826831
}
827832
}
828833
}
834+
835+
private function validateTypeIsSingleton(Type $type, string $path): void
836+
{
837+
$typeLoader = $this->schema->getConfig()->typeLoader;
838+
if ($typeLoader === null) {
839+
return;
840+
}
841+
842+
$namedType = Type::getNamedType($type);
843+
assert($namedType !== null, 'because getNamedType() was called with non-null type');
844+
if ($namedType->isBuiltInType()) {
845+
return;
846+
}
847+
848+
$name = $namedType->name;
849+
if ($namedType !== $typeLoader($name)) {
850+
throw new InvariantViolation(static::duplicateType($this->schema, $path, $name));
851+
}
852+
}
853+
854+
public static function duplicateType(Schema $schema, string $path, string $name): string
855+
{
856+
$hint = $schema->getConfig()->typeLoader !== null
857+
? 'Ensure the type loader returns the same instance. '
858+
: '';
859+
860+
return "Found duplicate type in schema at {$path}: {$name}. {$hint}See https://webonyx.github.io/graphql-php/type-definitions/#type-registry.";
861+
}
829862
}

‎src/Utils/LazyException.php‎

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,17 @@
1+
<?php declare(strict_types=1);
2+
3+
namespace GraphQL\Utils;
4+
5+
/**
6+
* Allows lazy calculation of a complex message when the exception is used in `assert()`.
7+
*/
8+
class LazyException extends \Exception
9+
{
10+
/**
11+
* @param callable(): string $makeMessage
12+
*/
13+
public function __construct(callable $makeMessage)
14+
{
15+
parent::__construct($makeMessage());
16+
}
17+
}

‎tests/Executor/AbstractTest.php‎

Lines changed: 8 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -637,24 +637,12 @@ public function testWarnsAboutOrphanedTypesWhenMissingType(): void
637637

638638
$result = GraphQL::executeQuery($schema, '{ foo { bar } }');
639639

640-
$expected = [
641-
'data' => ['foo' => null],
642-
'errors' => [
643-
[
644-
'message' => 'Internal server error',
645-
'locations' => [['line' => 1, 'column' => 3]],
646-
'path' => ['foo'],
647-
'extensions' => [
648-
'debugMessage' => 'Schema does not contain type "FooObject". '
649-
. 'This can happen when an object type is only referenced indirectly through '
650-
. 'abstract types and never directly through fields.'
651-
. 'List the type in the option "types" during schema construction, '
652-
. 'see https://webonyx.github.io/graphql-php/schema-definition/#configuration-options.',
653-
],
654-
],
655-
],
656-
];
657-
self::assertEquals($expected, $result->toArray(DebugFlag::INCLUDE_DEBUG_MESSAGE));
640+
$error = $result->errors[0] ?? null;
641+
self::assertInstanceOf(Error::class, $error);
642+
self::assertStringContainsString(
643+
'Schema does not contain type "FooObject". This can happen when an object type is only referenced indirectly through abstract types and never directly through fields.List the type in the option "types" during schema construction, see https://webonyx.github.io/graphql-php/schema-definition/#configuration-options.',
644+
$error->getMessage(),
645+
);
658646
}
659647

660648
public function testResolveTypeAllowsResolvingWithTypeName(): void
@@ -784,11 +772,8 @@ public function testHintsOnConflictingTypeInstancesInResolveType(): void
784772
$error = $result->errors[0] ?? null;
785773

786774
self::assertInstanceOf(Error::class, $error);
787-
self::assertEquals(
788-
'Schema must contain unique named types but contains multiple types named "Test". '
789-
. 'Make sure that `resolveType` function of abstract type "Node" returns the same type instance '
790-
. 'as referenced anywhere else within the schema '
791-
. '(see https://webonyx.github.io/graphql-php/type-definitions/#type-registry).',
775+
self::assertStringContainsString(
776+
'Schema must contain unique named types but contains multiple types named "Test". Make sure that `resolveType` function of abstract type "Node" returns the same type instance as referenced anywhere else within the schema (see https://webonyx.github.io/graphql-php/type-definitions/#type-registry).',
792777
$error->getMessage()
793778
);
794779
}

‎tests/Executor/ExecutorLazySchemaTest.php‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -181,8 +181,8 @@ public function testHintsOnConflictingTypeInstancesInDefinitions(): void
181181

182182
$error = $result->errors[0] ?? null;
183183
self::assertInstanceOf(Error::class, $error);
184-
self::assertEquals(
185-
'Found duplicate type in schema: Test. Ensure the type loader returns the same instance as defined in Query.test. See https://webonyx.github.io/graphql-php/type-definitions/#type-registry.',
184+
self::assertStringContainsString(
185+
'Found duplicate type in schema at Query.test: Test. Ensure the type loader returns the same instance. See https://webonyx.github.io/graphql-php/type-definitions/#type-registry.',
186186
$error->getMessage()
187187
);
188188
}

‎tests/Server/QueryExecutionTest.php‎

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -401,8 +401,7 @@ public function testProhibitsInvalidPersistedQueryLoader(): void
401401
$this->config->setPersistedQueryLoader(static fn (): array => ['err' => 'err']);
402402

403403
$this->expectExceptionObject(new InvariantViolation(
404-
'Persisted query loader must return query string or instance of GraphQL\Language\AST\DocumentNode '
405-
. 'but got: {"err":"err"}'
404+
'Persisted query loader must return query string or instance of GraphQL\Language\AST\DocumentNode but got: {"err":"err"}'
406405
));
407406
$this->executePersistedQuery('some-id');
408407
}

‎tests/Type/DefinitionTest.php‎

Lines changed: 6 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -1292,9 +1292,7 @@ public function testAcceptsAScalarTypeDefiningSerialize(): void
12921292
public function testRejectsAScalarTypeDefiningSerializeWithAnIncorrectType(): void
12931293
{
12941294
$this->expectExceptionObject(new InvariantViolation(
1295-
'SomeScalar must provide "serialize" function. If this custom Scalar '
1296-
. 'is also used as an input type, ensure "parseValue" and "parseLiteral" '
1297-
. 'functions are also provided.'
1295+
'SomeScalar must provide "serialize" function. If this custom Scalar is also used as an input type, ensure "parseValue" and "parseLiteral" functions are also provided.'
12981296
));
12991297

13001298
$this->schemaWithFieldType(
@@ -1772,8 +1770,7 @@ public function testRejectsASchemaWhichDefinesAnObjectTypeTwice(): void
17721770
$schema = new Schema(['query' => $QueryType]);
17731771

17741772
$this->expectExceptionObject(new InvariantViolation(
1775-
'Schema must contain unique named types but contains multiple types named "SameName" '
1776-
. '(see https://webonyx.github.io/graphql-php/type-definitions/#type-registry).'
1773+
'Schema must contain unique named types but contains multiple types named "SameName" (see https://webonyx.github.io/graphql-php/type-definitions/#type-registry).'
17771774
));
17781775
$schema->assertValid();
17791776
}
@@ -1810,13 +1807,12 @@ public function testRejectsASchemaWhichDefinesFieldsWithConflictingTypes(): void
18101807
$schema = new Schema(['query' => $QueryType]);
18111808

18121809
$this->expectExceptionObject(new InvariantViolation(
1813-
'Schema must contain unique named types but contains multiple types named "SameName" '
1814-
. '(see https://webonyx.github.io/graphql-php/type-definitions/#type-registry).'
1810+
'Schema must contain unique named types but contains multiple types named "SameName" (see https://webonyx.github.io/graphql-php/type-definitions/#type-registry).'
18151811
));
18161812
$schema->assertValid();
18171813
}
18181814

1819-
public function testRejectsASchemaWhichHaveSameNamedObjectsImplementingAnInterface(): void
1815+
public function testRejectsASchemaWithSameNamedObjectsImplementingAnInterface(): void
18201816
{
18211817
$AnotherInterface = new InterfaceType([
18221818
'name' => 'AnotherInterface',
@@ -1842,15 +1838,12 @@ public function testRejectsASchemaWhichHaveSameNamedObjectsImplementingAnInterfa
18421838
],
18431839
]);
18441840

1845-
$this->expectException(InvariantViolation::class);
1846-
$this->expectExceptionMessage(
1847-
'Schema must contain unique named types but contains multiple types named "BadObject" '
1848-
. '(see https://webonyx.github.io/graphql-php/type-definitions/#type-registry).'
1849-
);
18501841
$schema = new Schema([
18511842
'query' => $QueryType,
18521843
'types' => [$FirstBadObject, $SecondBadObject],
18531844
]);
1845+
1846+
$this->expectExceptionMessageMatches('/Schema must contain unique named types but contains multiple types named "BadObject"/');
18541847
$schema->assertValid();
18551848
}
18561849
}

0 commit comments

Comments
 (0)