From a7e0f7e53a3ccbecfbb79b7f63a27daa1f4320b8 Mon Sep 17 00:00:00 2001 From: Ruud Kamphuis Date: Mon, 16 Mar 2026 11:58:29 +0100 Subject: [PATCH] Fix type loader scalar overrides breaking validation and input coercion When a type loader returns a custom instance for a built-in scalar (e.g. ID, String), two problems occur because field definitions still reference the built-in singletons (e.g. Type::id()): 1. Variable validation fails VariablesInAllowedPosition resolves variable types via Schema::getType(), which returns the type loader's instance. TypeComparators::isEqualType() and isTypeSubTypeOf() compare this against field argument types using strict identity (===). Since the type loader instance differs from the built-in singleton, the check fails with errors like: Variable \"$id\" of type \"ID!\" used in position expecting type \"ID!\". Fix: add a name-based equality check for NamedType instances in both isEqualType() and isTypeSubTypeOf(). In a valid schema, each type name is unique, so two named types with the same name are semantically equivalent even if they are different object instances. 2. Input coercion uses wrong parseValue() Value::coerceInputValue() calls parseValue() on the type from the field definition (the built-in singleton), not the custom override from the type loader. This means custom parsing logic (e.g. converting a base64 global ID string into a domain object) is skipped, causing input validation to fail. This is the input-side equivalent of the output fix already present in ReferenceExecutor::completeValue() (lines 920-923). Fix: add an optional Schema parameter to coerceInputValue(). When provided and the type is a ScalarType, resolve the actual type from the schema to ensure the correct parseValue() is called. The schema is passed from Values::getVariableValues() and propagated through all recursive calls. Ref #1869 Fixes #1874 --- src/Executor/Values.php | 2 +- src/Utils/TypeComparators.php | 17 ++++ src/Utils/Value.php | 21 ++++- tests/Type/ScalarOverridesTest.php | 137 ++++++++++++++++++++++++++++ tests/Utils/TypeComparatorsTest.php | 76 +++++++++++++++ 5 files changed, 248 insertions(+), 5 deletions(-) create mode 100644 tests/Utils/TypeComparatorsTest.php diff --git a/src/Executor/Values.php b/src/Executor/Values.php index 862049fbd..e63737e79 100644 --- a/src/Executor/Values.php +++ b/src/Executor/Values.php @@ -104,7 +104,7 @@ public static function getVariableValues(Schema $schema, NodeList $varDefNodes, } else { // Otherwise, a non-null value was provided, coerce it to the expected // type or report an error if coercion fails. - $coerced = Value::coerceInputValue($value, $varType); + $coerced = Value::coerceInputValue($value, $varType, null, $schema); $coercionErrors = $coerced['errors']; if ($coercionErrors !== null) { diff --git a/src/Utils/TypeComparators.php b/src/Utils/TypeComparators.php index 716de3189..26bec38a8 100644 --- a/src/Utils/TypeComparators.php +++ b/src/Utils/TypeComparators.php @@ -5,6 +5,7 @@ use GraphQL\Error\InvariantViolation; use GraphQL\Type\Definition\ImplementingType; use GraphQL\Type\Definition\ListOfType; +use GraphQL\Type\Definition\NamedType; use GraphQL\Type\Definition\NonNull; use GraphQL\Type\Definition\Type; use GraphQL\Type\Schema; @@ -19,6 +20,14 @@ public static function isEqualType(Type $typeA, Type $typeB): bool return true; } + // Named types with the same name are equal, even if they are different + // instances (e.g. a type loader override vs the built-in singleton). + if ($typeA instanceof NamedType && $typeB instanceof NamedType + && $typeA->name() === $typeB->name() + ) { + return true; + } + // If either type is non-null, the other must also be non-null. if ($typeA instanceof NonNull && $typeB instanceof NonNull) { return self::isEqualType($typeA->getWrappedType(), $typeB->getWrappedType()); @@ -46,6 +55,14 @@ public static function isTypeSubTypeOf(Schema $schema, Type $maybeSubType, Type return true; } + // Named types with the same name are equivalent, even if they are different + // instances (e.g. a type loader override vs the built-in singleton). + if ($maybeSubType instanceof NamedType && $superType instanceof NamedType + && $maybeSubType->name() === $superType->name() + ) { + return true; + } + // If superType is non-null, maybeSubType must also be nullable. if ($superType instanceof NonNull) { if ($maybeSubType instanceof NonNull) { diff --git a/src/Utils/Value.php b/src/Utils/Value.php index a39cecb3e..6e69436b1 100644 --- a/src/Utils/Value.php +++ b/src/Utils/Value.php @@ -13,6 +13,7 @@ use GraphQL\Type\Definition\NonNull; use GraphQL\Type\Definition\ScalarType; use GraphQL\Type\Definition\Type; +use GraphQL\Type\Schema; /** * @phpstan-type CoercedValue array{errors: null, value: mixed} @@ -37,7 +38,7 @@ class Value * * @phpstan-return CoercedValue|CoercedErrors */ - public static function coerceInputValue($value, InputType $type, ?array $path = null): array + public static function coerceInputValue($value, InputType $type, ?array $path = null, ?Schema $schema = null): array { if ($type instanceof NonNull) { if ($value === null) { @@ -47,7 +48,7 @@ public static function coerceInputValue($value, InputType $type, ?array $path = } // @phpstan-ignore-next-line wrapped type is known to be input type after schema validation - return self::coerceInputValue($value, $type->getWrappedType(), $path); + return self::coerceInputValue($value, $type->getWrappedType(), $path, $schema); } if ($value === null) { @@ -56,6 +57,16 @@ public static function coerceInputValue($value, InputType $type, ?array $path = } if ($type instanceof ScalarType || $type instanceof EnumType) { + // Account for type loader returning a different instance than the + // built-in singleton used in field definitions. Resolve the actual + // type from the schema to ensure the correct parseValue() is called. + if ($schema !== null && $type instanceof ScalarType) { + $schemaType = $schema->getType($type->name); + if ($schemaType instanceof ScalarType) { + $type = $schemaType; + } + } + // Scalars and Enums determine if a input value is valid via parseValue(), which can // throw to indicate failure. If it throws, maintain a reference to // the original error. @@ -88,7 +99,8 @@ public static function coerceInputValue($value, InputType $type, ?array $path = $coercedItem = self::coerceInputValue( $itemValue, $itemType, - [...$path ?? [], $index] + [...$path ?? [], $index], + $schema, ); if (isset($coercedItem['errors'])) { @@ -104,7 +116,7 @@ public static function coerceInputValue($value, InputType $type, ?array $path = } // Lists accept a non-list value as a list of one. - $coercedItem = self::coerceInputValue($value, $itemType); + $coercedItem = self::coerceInputValue($value, $itemType, null, $schema); return isset($coercedItem['errors']) ? $coercedItem @@ -133,6 +145,7 @@ public static function coerceInputValue($value, InputType $type, ?array $path = $fieldValue, $field->getType(), [...$path ?? [], $fieldName], + $schema, ); if (isset($coercedField['errors'])) { diff --git a/tests/Type/ScalarOverridesTest.php b/tests/Type/ScalarOverridesTest.php index d93f48000..ebee4ca88 100644 --- a/tests/Type/ScalarOverridesTest.php +++ b/tests/Type/ScalarOverridesTest.php @@ -4,7 +4,9 @@ use GraphQL\Error\InvariantViolation; use GraphQL\GraphQL; +use GraphQL\Language\AST\StringValueNode; use GraphQL\Type\Definition\CustomScalarType; +use GraphQL\Type\Definition\InputObjectType; use GraphQL\Type\Definition\ObjectType; use GraphQL\Type\Definition\ScalarType; use GraphQL\Type\Definition\Type; @@ -204,12 +206,147 @@ public function testNonOverriddenScalarsAreUnaffected(): void self::assertSame('abc-123', $data['identifier']); } + public function testTypeLoaderOverrideWithVariableOfOverriddenBuiltInScalarType(): void + { + $customID = self::createCustomID(static fn ($value): string => (string) $value); + + $queryType = new ObjectType([ + 'name' => 'Query', + 'fields' => [ + 'node' => [ + 'type' => Type::string(), + 'args' => [ + 'id' => Type::nonNull(Type::id()), + ], + 'resolve' => static fn ($root, array $args): string => 'node-' . $args['id'], + ], + ], + ]); + + $types = ['Query' => $queryType, 'ID' => $customID]; + + $schema = new Schema([ + 'query' => $queryType, + 'typeLoader' => static fn (string $name): ?Type => $types[$name] ?? null, + ]); + + $schema->assertValid(); + + $result = GraphQL::executeQuery($schema, 'query ($id: ID!) { node(id: $id) }', null, null, ['id' => 'abc-123']); + + self::assertEmpty($result->errors, isset($result->errors[0]) ? $result->errors[0]->getMessage() : ''); + self::assertSame(['data' => ['node' => 'node-abc-123']], $result->toArray()); + } + + public function testTypeLoaderOverrideWithNullableVariableOfOverriddenBuiltInScalarType(): void + { + $customString = self::createUppercaseString(); + + $queryType = new ObjectType([ + 'name' => 'Query', + 'fields' => [ + 'echo' => [ + 'type' => Type::string(), + 'args' => [ + 'text' => Type::string(), + ], + 'resolve' => static fn ($root, array $args): ?string => $args['text'] ?? null, + ], + ], + ]); + + $types = ['Query' => $queryType, 'String' => $customString]; + + $schema = new Schema([ + 'query' => $queryType, + 'typeLoader' => static fn (string $name): ?Type => $types[$name] ?? null, + ]); + + $schema->assertValid(); + + $result = GraphQL::executeQuery($schema, 'query ($text: String) { echo(text: $text) }', null, null, ['text' => 'hello']); + + self::assertEmpty($result->errors, isset($result->errors[0]) ? $result->errors[0]->getMessage() : ''); + self::assertSame(['data' => ['echo' => 'HELLO']], $result->toArray()); + } + + public function testTypeLoaderOverrideWithInputObjectFieldOfOverriddenBuiltInScalarType(): void + { + $customID = self::createCustomID(static fn ($value): string => 'custom-' . $value); + + $inputType = new InputObjectType([ + 'name' => 'NodeInput', + 'fields' => [ + 'id' => Type::nonNull(Type::id()), + 'label' => Type::string(), + ], + ]); + + $queryType = new ObjectType([ + 'name' => 'Query', + 'fields' => [ + 'node' => [ + 'type' => Type::string(), + 'args' => [ + 'input' => Type::nonNull($inputType), + ], + 'resolve' => static fn ($root, array $args): string => $args['input']['id'] . ':' . ($args['input']['label'] ?? ''), + ], + ], + ]); + + $types = ['Query' => $queryType, 'ID' => $customID, 'NodeInput' => $inputType]; + + $schema = new Schema([ + 'query' => $queryType, + 'typeLoader' => static fn (string $name): ?Type => $types[$name] ?? null, + ]); + + $schema->assertValid(); + + $result = GraphQL::executeQuery( + $schema, + 'query ($input: NodeInput!) { node(input: $input) }', + null, + null, + ['input' => ['id' => 'abc-123', 'label' => 'test']], + ); + + self::assertEmpty($result->errors, isset($result->errors[0]) ? $result->errors[0]->getMessage() : ''); + self::assertSame(['data' => ['node' => 'custom-abc-123:test']], $result->toArray()); + } + + /** @throws InvariantViolation */ + private static function createCustomID(\Closure $parseValue): CustomScalarType + { + return new CustomScalarType([ + 'name' => Type::ID, + 'serialize' => static fn ($value): string => (string) $value, + 'parseValue' => $parseValue, + 'parseLiteral' => static function ($node): string { + if (! $node instanceof StringValueNode) { + throw new \Exception('Expected a string literal for ID.'); + } + + return $node->value; + }, + ]); + } + /** @throws InvariantViolation */ private static function createUppercaseString(): CustomScalarType { return new CustomScalarType([ 'name' => Type::STRING, 'serialize' => static fn ($value): string => strtoupper((string) $value), + 'parseValue' => static fn ($value): string => (string) $value, + 'parseLiteral' => static function ($node): string { + if (! $node instanceof StringValueNode) { + throw new \Exception('Expected a string literal for String.'); + } + + return $node->value; + }, ]); } diff --git a/tests/Utils/TypeComparatorsTest.php b/tests/Utils/TypeComparatorsTest.php new file mode 100644 index 000000000..b7afb05cf --- /dev/null +++ b/tests/Utils/TypeComparatorsTest.php @@ -0,0 +1,76 @@ + Type::STRING]); + + self::assertTrue(TypeComparators::isEqualType(Type::string(), $customString)); + self::assertTrue(TypeComparators::isEqualType($customString, Type::string())); + } + + public function testIsEqualTypeWithWrappedDifferentInstances(): void + { + $customString = new CustomScalarType(['name' => Type::STRING]); + + self::assertTrue(TypeComparators::isEqualType(Type::nonNull(Type::string()), Type::nonNull($customString))); + self::assertTrue(TypeComparators::isEqualType(Type::listOf(Type::string()), Type::listOf($customString))); + self::assertTrue(TypeComparators::isEqualType( + Type::nonNull(Type::listOf(Type::string())), + Type::nonNull(Type::listOf($customString)), + )); + } + + public function testIsTypeSubTypeOfWithDifferentInstancesOfSameNamedType(): void + { + $schema = $this->createSchemaWithCustomString(); + $customString = new CustomScalarType(['name' => Type::STRING]); + + self::assertTrue(TypeComparators::isTypeSubTypeOf($schema, $customString, Type::string())); + self::assertTrue(TypeComparators::isTypeSubTypeOf($schema, Type::string(), $customString)); + } + + public function testIsTypeSubTypeOfWithWrappedDifferentInstances(): void + { + $schema = $this->createSchemaWithCustomString(); + $customString = new CustomScalarType(['name' => Type::STRING]); + + self::assertTrue(TypeComparators::isTypeSubTypeOf($schema, Type::nonNull($customString), Type::string())); + self::assertTrue(TypeComparators::isTypeSubTypeOf($schema, Type::nonNull($customString), Type::nonNull(Type::string()))); + self::assertTrue(TypeComparators::isTypeSubTypeOf( + $schema, + Type::nonNull(Type::listOf(Type::nonNull($customString))), + Type::listOf(Type::nonNull(Type::string())), + )); + } + + /** @throws InvariantViolation */ + private function createSchemaWithCustomString(): Schema + { + $queryType = new ObjectType([ + 'name' => 'Query', + 'fields' => [ + 'greeting' => [ + 'type' => Type::string(), + 'resolve' => static fn (): string => 'hello', + ], + ], + ]); + + return new Schema([ + 'query' => $queryType, + 'typeLoader' => static fn (string $name): ?Type => ['Query' => $queryType][$name] ?? null, + ]); + } +}