Skip to content

Commit 4815b3d

Browse files
ruudkruudk
authored
Fix parseLiteral not called on per-schema scalar overrides for inline arguments (#1880)
AST::valueFromAST() called parseLiteral() on the built-in scalar singleton from field definitions, not the custom override. This meant inline literal arguments like `node(id: 123)` silently skipped custom parsing logic. Pass the Schema into valueFromAST() and resolve built-in scalars from it before calling parseLiteral(), mirroring the parseValue() fix in Value::coerceInputValue().
1 parent 7d870b9 commit 4815b3d

File tree

7 files changed

+133
-16
lines changed

7 files changed

+133
-16
lines changed

‎docs/class-reference.md‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2955,7 +2955,8 @@ static function astFromValue($value, GraphQL\Type\Definition\InputType $type): ?
29552955
static function valueFromAST(
29562956
?GraphQL\Language\AST\ValueNode $valueNode,
29572957
GraphQL\Type\Definition\Type $type,
2958-
?array $variables = null
2958+
?array $variables = null,
2959+
?GraphQL\Type\Schema $schema = null
29592960
)
29602961
```
29612962

‎src/Executor/ReferenceExecutor.php‎

Lines changed: 8 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -466,10 +466,13 @@ protected function shouldIncludeNode(SelectionNode $node): bool
466466
{
467467
$variableValues = $this->exeContext->variableValues;
468468

469+
$schema = $this->exeContext->schema;
470+
469471
$skip = Values::getDirectiveValues(
470472
Directive::skipDirective(),
471473
$node,
472-
$variableValues
474+
$variableValues,
475+
$schema,
473476
);
474477
if (isset($skip['if']) && $skip['if'] === true) {
475478
return false;
@@ -478,7 +481,8 @@ protected function shouldIncludeNode(SelectionNode $node): bool
478481
$include = Values::getDirectiveValues(
479482
Directive::includeDirective(),
480483
$node,
481-
$variableValues
484+
$variableValues,
485+
$schema,
482486
);
483487

484488
return ! isset($include['if']) || $include['if'] !== false;
@@ -738,7 +742,8 @@ protected function resolveFieldValueOrError(
738742
$args = $this->fieldArgsCache[$fieldDef][$fieldNode] ??= $argsMapper(Values::getArgumentValues(
739743
$fieldDef,
740744
$fieldNode,
741-
$this->exeContext->variableValues
745+
$this->exeContext->variableValues,
746+
$this->exeContext->schema,
742747
), $fieldDef, $fieldNode, $contextValue);
743748

744749
return $resolveFn($rootValue, $args, $contextValue, $info);

‎src/Executor/Values.php‎

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -154,13 +154,13 @@ public static function getVariableValues(Schema $schema, NodeList $varDefNodes,
154154
*
155155
* @return array<string, mixed>|null
156156
*/
157-
public static function getDirectiveValues(Directive $directiveDef, Node $node, ?array $variableValues = null): ?array
157+
public static function getDirectiveValues(Directive $directiveDef, Node $node, ?array $variableValues = null, ?Schema $schema = null): ?array
158158
{
159159
$directiveDefName = $directiveDef->name;
160160

161161
foreach ($node->directives as $directive) {
162162
if ($directive->name->value === $directiveDefName) {
163-
return self::getArgumentValues($directiveDef, $directive, $variableValues);
163+
return self::getArgumentValues($directiveDef, $directive, $variableValues, $schema);
164164
}
165165
}
166166

@@ -180,7 +180,7 @@ public static function getDirectiveValues(Directive $directiveDef, Node $node, ?
180180
*
181181
* @return array<string, mixed>
182182
*/
183-
public static function getArgumentValues($def, Node $node, ?array $variableValues = null): array
183+
public static function getArgumentValues($def, Node $node, ?array $variableValues = null, ?Schema $schema = null): array
184184
{
185185
if ($def->args === []) {
186186
return [];
@@ -196,7 +196,7 @@ public static function getArgumentValues($def, Node $node, ?array $variableValue
196196
}
197197
}
198198

199-
return static::getArgumentValuesForMap($def, $argumentValueMap, $variableValues, $node);
199+
return static::getArgumentValuesForMap($def, $argumentValueMap, $variableValues, $node, $schema);
200200
}
201201

202202
/**
@@ -209,7 +209,7 @@ public static function getArgumentValues($def, Node $node, ?array $variableValue
209209
*
210210
* @return array<string, mixed>
211211
*/
212-
public static function getArgumentValuesForMap($def, array $argumentValueMap, ?array $variableValues = null, ?Node $referenceNode = null): array
212+
public static function getArgumentValuesForMap($def, array $argumentValueMap, ?array $variableValues = null, ?Node $referenceNode = null, ?Schema $schema = null): array
213213
{
214214
/** @var array<string, mixed> $coercedValues */
215215
$coercedValues = [];
@@ -260,7 +260,7 @@ public static function getArgumentValuesForMap($def, array $argumentValueMap, ?a
260260
// usage here is of the correct type.
261261
$coercedValues[$name] = $variableValues[$variableName] ?? null;
262262
} else {
263-
$coercedValue = AST::valueFromAST($argumentValueNode, $argType, $variableValues);
263+
$coercedValue = AST::valueFromAST($argumentValueNode, $argType, $variableValues, $schema);
264264
if (Utils::undefined() === $coercedValue) {
265265
// Note: ValuesOfCorrectType validation should catch this before
266266
// execution. This is a runtime check to ensure execution does not

‎src/Type/Definition/Type.php‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -228,7 +228,7 @@ public static function overrideStandardTypes(array $types): void
228228
public static function isBuiltInScalar($type): bool
229229
{
230230
return $type instanceof ScalarType
231-
&& in_array($type->name, self::BUILT_IN_SCALAR_NAMES, true);
231+
&& self::isBuiltInScalarName($type->name);
232232
}
233233

234234
/** Checks if the given name is one of the built-in scalar type names (ID, String, Int, Float, Boolean). */

‎src/Utils/AST.php‎

Lines changed: 17 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -37,6 +37,7 @@
3737
use GraphQL\Type\Definition\NullableType;
3838
use GraphQL\Type\Definition\ScalarType;
3939
use GraphQL\Type\Definition\Type;
40+
use GraphQL\Type\Schema;
4041

4142
/**
4243
* Various utilities dealing with AST.
@@ -307,7 +308,7 @@ public static function astFromValue($value, InputType $type): ?ValueNode
307308
*
308309
* @api
309310
*/
310-
public static function valueFromAST(?ValueNode $valueNode, Type $type, ?array $variables = null)
311+
public static function valueFromAST(?ValueNode $valueNode, Type $type, ?array $variables = null, ?Schema $schema = null)
311312
{
312313
$undefined = Utils::undefined();
313314

@@ -323,7 +324,7 @@ public static function valueFromAST(?ValueNode $valueNode, Type $type, ?array $v
323324
return $undefined;
324325
}
325326

326-
return self::valueFromAST($valueNode, $type->getWrappedType(), $variables);
327+
return self::valueFromAST($valueNode, $type->getWrappedType(), $variables, $schema);
327328
}
328329

329330
if ($valueNode instanceof NullValueNode) {
@@ -362,7 +363,7 @@ public static function valueFromAST(?ValueNode $valueNode, Type $type, ?array $v
362363

363364
$coercedValues[] = null;
364365
} else {
365-
$itemValue = self::valueFromAST($itemNode, $itemType, $variables);
366+
$itemValue = self::valueFromAST($itemNode, $itemType, $variables, $schema);
366367
if ($undefined === $itemValue) {
367368
// Invalid: intentionally return no value.
368369
return $undefined;
@@ -375,7 +376,7 @@ public static function valueFromAST(?ValueNode $valueNode, Type $type, ?array $v
375376
return $coercedValues;
376377
}
377378

378-
$coercedValue = self::valueFromAST($valueNode, $itemType, $variables);
379+
$coercedValue = self::valueFromAST($valueNode, $itemType, $variables, $schema);
379380
if ($undefined === $coercedValue) {
380381
// Invalid: intentionally return no value.
381382
return $undefined;
@@ -416,7 +417,8 @@ public static function valueFromAST(?ValueNode $valueNode, Type $type, ?array $v
416417
$fieldValue = self::valueFromAST(
417418
$fieldNode->value,
418419
$field->getType(),
419-
$variables
420+
$variables,
421+
$schema,
420422
);
421423

422424
if ($undefined === $fieldValue) {
@@ -439,6 +441,16 @@ public static function valueFromAST(?ValueNode $valueNode, Type $type, ?array $v
439441
}
440442

441443
assert($type instanceof ScalarType, 'only remaining option');
444+
$typeName = $type->name;
445+
446+
// Account for type loader returning a different scalar instance than
447+
// the built-in singleton used in field definitions. Resolve the actual
448+
// type from the schema to ensure the correct parseLiteral() is called.
449+
if ($schema !== null && Type::isBuiltInScalarName($typeName)) {
450+
$schemaType = $schema->getType($typeName);
451+
assert($schemaType instanceof ScalarType, "Schema must provide a ScalarType for built-in scalar \"{$typeName}\".");
452+
$type = $schemaType;
453+
}
442454

443455
// Scalars fulfill parsing a literal value via parseLiteral().
444456
// Invalid values represent a failure to parse correctly, in which case

‎tests/Type/Definition/TypeTest.php‎

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -48,4 +48,20 @@ public function testIsBuiltInScalarReturnsFalseForCustomScalarWithNonBuiltInName
4848
{
4949
self::assertFalse(Type::isBuiltInScalar(new CustomScalarType(['name' => 'MyScalar']))); // @phpstan-ignore staticMethod.alreadyNarrowedType
5050
}
51+
52+
public function testIsBuiltInScalarNameReturnsTrueForBuiltInNames(): void
53+
{
54+
self::assertTrue(Type::isBuiltInScalarName(Type::STRING));
55+
self::assertTrue(Type::isBuiltInScalarName(Type::INT));
56+
self::assertTrue(Type::isBuiltInScalarName(Type::FLOAT));
57+
self::assertTrue(Type::isBuiltInScalarName(Type::BOOLEAN));
58+
self::assertTrue(Type::isBuiltInScalarName(Type::ID));
59+
}
60+
61+
public function testIsBuiltInScalarNameReturnsFalseForNonBuiltInNames(): void
62+
{
63+
self::assertFalse(Type::isBuiltInScalarName('MyScalar'));
64+
self::assertFalse(Type::isBuiltInScalarName('Query'));
65+
self::assertFalse(Type::isBuiltInScalarName(''));
66+
}
5167
}

‎tests/Type/ScalarOverridesTest.php‎

Lines changed: 83 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,8 @@
44

55
use GraphQL\Error\InvariantViolation;
66
use GraphQL\GraphQL;
7+
use GraphQL\Language\AST\BooleanValueNode;
8+
use GraphQL\Language\AST\IntValueNode;
79
use GraphQL\Language\AST\StringValueNode;
810
use GraphQL\Type\Definition\CustomScalarType;
911
use GraphQL\Type\Definition\InputObjectType;
@@ -298,6 +300,87 @@ public function testTypesOverrideWithInputObjectFieldOfOverriddenBuiltInScalarTy
298300
self::assertSame(['data' => ['node' => 'custom-abc-123:test']], $result->toArray());
299301
}
300302

303+
public function testTypesOverrideCallsParseLiteralForInlineArgument(): void
304+
{
305+
$parseLiteralCalled = false;
306+
307+
$customID = new CustomScalarType([
308+
'name' => Type::ID,
309+
'serialize' => static fn ($value): string => (string) $value,
310+
'parseValue' => static fn ($value): string => 'parsed-' . $value,
311+
'parseLiteral' => static function ($node) use (&$parseLiteralCalled): string {
312+
$parseLiteralCalled = true;
313+
314+
assert($node instanceof IntValueNode || $node instanceof StringValueNode);
315+
316+
return 'literal-' . $node->value;
317+
},
318+
]);
319+
320+
$queryType = new ObjectType([
321+
'name' => 'Query',
322+
'fields' => [
323+
'node' => [
324+
'type' => Type::string(),
325+
'args' => [
326+
'id' => Type::nonNull(Type::id()),
327+
],
328+
'resolve' => static fn ($root, array $args): string => 'node-' . $args['id'],
329+
],
330+
],
331+
]);
332+
333+
$schema = new Schema([
334+
'query' => $queryType,
335+
'types' => [$customID],
336+
]);
337+
338+
$result = GraphQL::executeQuery($schema, '{ node(id: 123) }');
339+
340+
self::assertEmpty($result->errors, isset($result->errors[0]) ? $result->errors[0]->getMessage() : '');
341+
self::assertTrue($parseLiteralCalled, 'Expected custom parseLiteral to be called for inline literal argument');
342+
self::assertSame(['data' => ['node' => 'node-literal-123']], $result->toArray());
343+
}
344+
345+
public function testTypesOverrideCallsParseLiteralForDirectiveArgument(): void
346+
{
347+
$parseLiteralCalled = false;
348+
349+
$customBoolean = new CustomScalarType([
350+
'name' => Type::BOOLEAN,
351+
'serialize' => static fn ($value): bool => (bool) $value,
352+
'parseValue' => static fn ($value): bool => (bool) $value,
353+
'parseLiteral' => static function ($node) use (&$parseLiteralCalled): bool {
354+
$parseLiteralCalled = true;
355+
356+
self::assertInstanceOf(BooleanValueNode::class, $node);
357+
358+
return $node->value;
359+
},
360+
]);
361+
362+
$queryType = new ObjectType([
363+
'name' => 'Query',
364+
'fields' => [
365+
'greeting' => [
366+
'type' => Type::string(),
367+
'resolve' => static fn (): string => 'hello',
368+
],
369+
],
370+
]);
371+
372+
$schema = new Schema([
373+
'query' => $queryType,
374+
'types' => [$customBoolean],
375+
]);
376+
377+
$result = GraphQL::executeQuery($schema, '{ greeting @include(if: true) }');
378+
379+
self::assertEmpty($result->errors, isset($result->errors[0]) ? $result->errors[0]->getMessage() : '');
380+
self::assertTrue($parseLiteralCalled, 'Expected custom parseLiteral to be called for directive argument');
381+
self::assertSame(['data' => ['greeting' => 'hello']], $result->toArray());
382+
}
383+
301384
public function testTypesOverrideWorksWithTypeLoader(): void
302385
{
303386
$uppercaseString = self::createUppercaseString();

0 commit comments

Comments
 (0)