Skip to content

Commit bf4e7d4

Browse files
vladarvladar
committed
SPEC/BUG: Ambiguity with null variable values and default values (fixes #274)
1 parent 953178f commit bf4e7d4

18 files changed

+659
-395
lines changed

‎CHANGELOG.md‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,8 @@
11
# Changelog
22

33
## Unreleased
4+
- **BREAKING:** Removal of `VariablesDefaultValueAllowed` validation rule. All variables may now specify a default value.
5+
- **BREAKING:** renamed `ProvidedNonNullArguments` to `ProvidedRequiredArguments` (no longer require values to be provided to non-null arguments which provide a default value).
46
- Add schema validation: Input Objects must not contain non-nullable circular references (#492)
57
- Added retrieving query complexity once query has been completed (#316)
68
- Allow input types to be passed in from variables using \stdClass instead of associative arrays (#535)

‎src/Executor/Values.php‎

Lines changed: 110 additions & 70 deletions
Original file line numberDiff line numberDiff line change
@@ -67,48 +67,9 @@ public static function getVariableValues(Schema $schema, $varDefNodes, array $in
6767
/** @var InputType|Type $varType */
6868
$varType = TypeInfo::typeFromAST($schema, $varDefNode->type);
6969

70-
if (Type::isInputType($varType)) {
71-
if (array_key_exists($varName, $inputs)) {
72-
$value = $inputs[$varName];
73-
$coerced = Value::coerceValue($value, $varType, $varDefNode);
74-
/** @var Error[] $coercionErrors */
75-
$coercionErrors = $coerced['errors'];
76-
if (empty($coercionErrors)) {
77-
$coercedValues[$varName] = $coerced['value'];
78-
} else {
79-
$messagePrelude = sprintf(
80-
'Variable "$%s" got invalid value %s; ',
81-
$varName,
82-
Utils::printSafeJson($value)
83-
);
84-
85-
foreach ($coercionErrors as $error) {
86-
$errors[] = new Error(
87-
$messagePrelude . $error->getMessage(),
88-
$error->getNodes(),
89-
$error->getSource(),
90-
$error->getPositions(),
91-
$error->getPath(),
92-
$error,
93-
$error->getExtensions()
94-
);
95-
}
96-
}
97-
} else {
98-
if ($varType instanceof NonNull) {
99-
$errors[] = new Error(
100-
sprintf(
101-
'Variable "$%s" of required type "%s" was not provided.',
102-
$varName,
103-
$varType
104-
),
105-
[$varDefNode]
106-
);
107-
} elseif ($varDefNode->defaultValue !== null) {
108-
$coercedValues[$varName] = AST::valueFromAST($varDefNode->defaultValue, $varType);
109-
}
110-
}
111-
} else {
70+
if (! Type::isInputType($varType)) {
71+
// Must use input types for variables. This should be caught during
72+
// validation, however is checked again here for safety.
11273
$errors[] = new Error(
11374
sprintf(
11475
'Variable "$%s" expected value of type "%s" which cannot be used as an input type.',
@@ -117,6 +78,61 @@ public static function getVariableValues(Schema $schema, $varDefNodes, array $in
11778
),
11879
[$varDefNode->type]
11980
);
81+
} else {
82+
$hasValue = array_key_exists($varName, $inputs);
83+
$value = $hasValue ? $inputs[$varName] : Utils::undefined();
84+
85+
if (! $hasValue && $varDefNode->defaultValue) {
86+
// If no value was provided to a variable with a default value,
87+
// use the default value.
88+
$coercedValues[$varName] = AST::valueFromAST($varDefNode->defaultValue, $varType);
89+
} elseif ((! $hasValue || $value === null) && ($varType instanceof NonNull)) {
90+
// If no value or a nullish value was provided to a variable with a
91+
// non-null type (required), produce an error.
92+
$errors[] = new Error(
93+
sprintf(
94+
$hasValue
95+
? 'Variable "$%s" of non-null type "%s" must not be null.'
96+
: 'Variable "$%s" of required type "%s" was not provided.',
97+
$varName,
98+
Utils::printSafe($varType)
99+
),
100+
[$varDefNode]
101+
);
102+
} elseif ($hasValue) {
103+
if ($value === null) {
104+
// If the explicit value `null` was provided, an entry in the coerced
105+
// values must exist as the value `null`.
106+
$coercedValues[$varName] = null;
107+
} else {
108+
// Otherwise, a non-null value was provided, coerce it to the expected
109+
// type or report an error if coercion fails.
110+
$coerced = Value::coerceValue($value, $varType, $varDefNode);
111+
/** @var Error[] $coercionErrors */
112+
$coercionErrors = $coerced['errors'];
113+
if ($coercionErrors) {
114+
$messagePrelude = sprintf(
115+
'Variable "$%s" got invalid value %s; ',
116+
$varName,
117+
Utils::printSafeJson($value)
118+
);
119+
120+
foreach ($coercionErrors as $error) {
121+
$errors[] = new Error(
122+
$messagePrelude . $error->getMessage(),
123+
$error->getNodes(),
124+
$error->getSource(),
125+
$error->getPositions(),
126+
$error->getPath(),
127+
$error,
128+
$error->getExtensions()
129+
);
130+
}
131+
} else {
132+
$coercedValues[$varName] = $coerced['value'];
133+
}
134+
}
135+
}
120136
}
121137
}
122138

@@ -208,47 +224,71 @@ public static function getArgumentValuesForMap($fieldDefinition, $argumentValueM
208224
$argType = $argumentDefinition->getType();
209225
$argumentValueNode = $argumentValueMap[$name] ?? null;
210226

211-
if ($argumentValueNode === null) {
212-
if ($argumentDefinition->defaultValueExists()) {
213-
$coercedValues[$name] = $argumentDefinition->defaultValue;
214-
} elseif ($argType instanceof NonNull) {
227+
if ($argumentValueNode instanceof VariableNode) {
228+
$variableName = $argumentValueNode->name->value;
229+
$hasValue = $variableValues ? array_key_exists($variableName, $variableValues) : false;
230+
$isNull = $hasValue ? $variableValues[$variableName] === null : false;
231+
} else {
232+
$hasValue = $argumentValueNode !== null;
233+
$isNull = $argumentValueNode instanceof NullValueNode;
234+
}
235+
236+
if (! $hasValue && $argumentDefinition->defaultValueExists()) {
237+
// If no argument was provided where the definition has a default value,
238+
// use the default value.
239+
$coercedValues[$name] = $argumentDefinition->defaultValue;
240+
} elseif ((! $hasValue || $isNull) && ($argType instanceof NonNull)) {
241+
// If no argument or a null value was provided to an argument with a
242+
// non-null type (required), produce a field error.
243+
if ($isNull) {
215244
throw new Error(
216-
'Argument "' . $name . '" of required type ' .
217-
'"' . Utils::printSafe($argType) . '" was not provided.',
245+
'Argument "' . $name . '" of non-null type ' .
246+
'"' . Utils::printSafe($argType) . '" must not be null.',
218247
$referenceNode
219248
);
220249
}
221-
} elseif ($argumentValueNode instanceof VariableNode) {
222-
$variableName = $argumentValueNode->name->value;
223250

224-
if ($variableValues !== null && array_key_exists($variableName, $variableValues)) {
225-
// Note: this does not check that this variable value is correct.
226-
// This assumes that this query has been validated and the variable
227-
// usage here is of the correct type.
228-
$coercedValues[$name] = $variableValues[$variableName];
229-
} elseif ($argumentDefinition->defaultValueExists()) {
230-
$coercedValues[$name] = $argumentDefinition->defaultValue;
231-
} elseif ($argType instanceof NonNull) {
251+
if ($argumentValueNode instanceof VariableNode) {
252+
$variableName = $argumentValueNode->name->value;
232253
throw new Error(
233254
'Argument "' . $name . '" of required type "' . Utils::printSafe($argType) . '" was ' .
234255
'provided the variable "$' . $variableName . '" which was not provided ' .
235256
'a runtime value.',
236257
[$argumentValueNode]
237258
);
238259
}
239-
} else {
240-
$valueNode = $argumentValueNode;
241-
$coercedValue = AST::valueFromAST($valueNode, $argType, $variableValues);
242-
if (Utils::isInvalid($coercedValue)) {
243-
// Note: ValuesOfCorrectType validation should catch this before
244-
// execution. This is a runtime check to ensure execution does not
245-
// continue with an invalid argument value.
246-
throw new Error(
247-
'Argument "' . $name . '" has invalid value ' . Printer::doPrint($valueNode) . '.',
248-
[$argumentValueNode]
249-
);
260+
261+
throw new Error(
262+
'Argument "' . $name . '" of required type ' .
263+
'"' . Utils::printSafe($argType) . '" was not provided.',
264+
$referenceNode
265+
);
266+
} elseif ($hasValue) {
267+
if ($argumentValueNode instanceof NullValueNode) {
268+
// If the explicit value `null` was provided, an entry in the coerced
269+
// values must exist as the value `null`.
270+
$coercedValues[$name] = null;
271+
} elseif ($argumentValueNode instanceof VariableNode) {
272+
$variableName = $argumentValueNode->name->value;
273+
Utils::invariant($variableValues !== null, 'Must exist for hasValue to be true.');
274+
// Note: This does no further checking that this variable is correct.
275+
// This assumes that this query has been validated and the variable
276+
// usage here is of the correct type.
277+
$coercedValues[$name] = $variableValues[$variableName] ?? null;
278+
} else {
279+
$valueNode = $argumentValueNode;
280+
$coercedValue = AST::valueFromAST($valueNode, $argType, $variableValues);
281+
if (Utils::isInvalid($coercedValue)) {
282+
// Note: ValuesOfCorrectType validation should catch this before
283+
// execution. This is a runtime check to ensure execution does not
284+
// continue with an invalid argument value.
285+
throw new Error(
286+
'Argument "' . $name . '" has invalid value ' . Printer::doPrint($valueNode) . '.',
287+
[$argumentValueNode]
288+
);
289+
}
290+
$coercedValues[$name] = $coercedValue;
250291
}
251-
$coercedValues[$name] = $coercedValue;
252292
}
253293
}
254294

‎src/Utils/AST.php‎

Lines changed: 8 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -354,9 +354,14 @@ public static function valueFromAST(?ValueNode $valueNode, Type $type, ?array $v
354354
return $undefined;
355355
}
356356

357-
// Note: we're not doing any checking that this variable is correct. We're
358-
// assuming that this query has been validated and the variable usage here
359-
// is of the correct type.
357+
$variableValue = $variables[$variableName] ?? null;
358+
if ($variableValue === null && $type instanceof NonNull) {
359+
return $undefined; // Invalid: intentionally return no value.
360+
}
361+
362+
// Note: This does no further checking that this variable is correct.
363+
// This assumes that this query has been validated and the variable
364+
// usage here is of the correct type.
360365
return $variables[$variableName];
361366
}
362367

‎src/Utils/TypeInfo.php‎

Lines changed: 36 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -64,6 +64,9 @@ class TypeInfo
6464
/** @var SplStack<FieldDefinition> */
6565
private $fieldDefStack;
6666

67+
/** @var SplStack<mixed> */
68+
private $defaultValueStack;
69+
6770
/** @var Directive */
6871
private $directive;
6972

@@ -78,11 +81,13 @@ class TypeInfo
7881
*/
7982
public function __construct(Schema $schema, $initialType = null)
8083
{
81-
$this->schema = $schema;
82-
$this->typeStack = [];
83-
$this->parentTypeStack = [];
84-
$this->inputTypeStack = [];
85-
$this->fieldDefStack = [];
84+
$this->schema = $schema;
85+
$this->typeStack = [];
86+
$this->parentTypeStack = [];
87+
$this->inputTypeStack = [];
88+
$this->fieldDefStack = [];
89+
$this->defaultValueStack = [];
90+
8691
if (! $initialType) {
8792
return;
8893
}
@@ -322,6 +327,7 @@ public function enter(Node $node)
322327
$fieldOrDirective = $this->getDirective() ?: $this->getFieldDef();
323328
$argDef = $argType = null;
324329
if ($fieldOrDirective) {
330+
/** @var FieldArgument $argDef */
325331
$argDef = Utils::find(
326332
$fieldOrDirective->args,
327333
static function ($arg) use ($node) {
@@ -332,28 +338,33 @@ static function ($arg) use ($node) {
332338
$argType = $argDef->getType();
333339
}
334340
}
335-
$this->argument = $argDef;
336-
$this->inputTypeStack[] = Type::isInputType($argType) ? $argType : null;
341+
$this->argument = $argDef;
342+
$this->defaultValueStack[] = $argDef && $argDef->defaultValueExists() ? $argDef->defaultValue : Utils::undefined();
343+
$this->inputTypeStack[] = Type::isInputType($argType) ? $argType : null;
337344
break;
338345

339346
case $node instanceof ListValueNode:
340-
$listType = Type::getNullableType($this->getInputType());
341-
$itemType = $listType instanceof ListOfType
347+
$listType = Type::getNullableType($this->getInputType());
348+
$itemType = $listType instanceof ListOfType
342349
? $listType->getWrappedType()
343350
: $listType;
344-
$this->inputTypeStack[] = Type::isInputType($itemType) ? $itemType : null;
351+
// List positions never have a default value.
352+
$this->defaultValueStack[] = Utils::undefined();
353+
$this->inputTypeStack[] = Type::isInputType($itemType) ? $itemType : null;
345354
break;
346355

347356
case $node instanceof ObjectFieldNode:
348357
$objectType = Type::getNamedType($this->getInputType());
349358
$fieldType = null;
359+
$inputField = null;
350360
$inputFieldType = null;
351361
if ($objectType instanceof InputObjectType) {
352362
$tmp = $objectType->getFields();
353363
$inputField = $tmp[$node->name->value] ?? null;
354364
$inputFieldType = $inputField ? $inputField->getType() : null;
355365
}
356-
$this->inputTypeStack[] = Type::isInputType($inputFieldType) ? $inputFieldType : null;
366+
$this->defaultValueStack[] = $inputField && $inputField->defaultValueExists() ? $inputField->defaultValue : Utils::undefined();
367+
$this->inputTypeStack[] = Type::isInputType($inputFieldType) ? $inputFieldType : null;
357368
break;
358369

359370
case $node instanceof EnumValueNode:
@@ -456,6 +467,18 @@ public function getFieldDef()
456467
return null;
457468
}
458469

470+
/**
471+
* @return mixed|null
472+
*/
473+
public function getDefaultValue()
474+
{
475+
if (! empty($this->defaultValueStack)) {
476+
return $this->defaultValueStack[count($this->defaultValueStack) - 1];
477+
}
478+
479+
return null;
480+
}
481+
459482
/**
460483
* @return ScalarType|EnumType|InputObjectType|ListOfType|NonNull|null
461484
*/
@@ -494,10 +517,12 @@ public function leave(Node $node)
494517
break;
495518
case $node instanceof ArgumentNode:
496519
$this->argument = null;
520+
array_pop($this->defaultValueStack);
497521
array_pop($this->inputTypeStack);
498522
break;
499523
case $node instanceof ListValueNode:
500524
case $node instanceof ObjectFieldNode:
525+
array_pop($this->defaultValueStack);
501526
array_pop($this->inputTypeStack);
502527
break;
503528
case $node instanceof EnumValueNode:

‎src/Validator/DocumentValidator.php‎

Lines changed: 2 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -28,7 +28,7 @@
2828
use GraphQL\Validator\Rules\NoUnusedVariables;
2929
use GraphQL\Validator\Rules\OverlappingFieldsCanBeMerged;
3030
use GraphQL\Validator\Rules\PossibleFragmentSpreads;
31-
use GraphQL\Validator\Rules\ProvidedNonNullArguments;
31+
use GraphQL\Validator\Rules\ProvidedRequiredArguments;
3232
use GraphQL\Validator\Rules\ProvidedRequiredArgumentsOnDirectives;
3333
use GraphQL\Validator\Rules\QueryComplexity;
3434
use GraphQL\Validator\Rules\QueryDepth;
@@ -43,7 +43,6 @@
4343
use GraphQL\Validator\Rules\ValidationRule;
4444
use GraphQL\Validator\Rules\ValuesOfCorrectType;
4545
use GraphQL\Validator\Rules\VariablesAreInputTypes;
46-
use GraphQL\Validator\Rules\VariablesDefaultValueAllowed;
4746
use GraphQL\Validator\Rules\VariablesInAllowedPosition;
4847
use Throwable;
4948
use function array_filter;
@@ -160,8 +159,7 @@ public static function defaultRules()
160159
KnownArgumentNames::class => new KnownArgumentNames(),
161160
UniqueArgumentNames::class => new UniqueArgumentNames(),
162161
ValuesOfCorrectType::class => new ValuesOfCorrectType(),
163-
ProvidedNonNullArguments::class => new ProvidedNonNullArguments(),
164-
VariablesDefaultValueAllowed::class => new VariablesDefaultValueAllowed(),
162+
ProvidedRequiredArguments::class => new ProvidedRequiredArguments(),
165163
VariablesInAllowedPosition::class => new VariablesInAllowedPosition(),
166164
OverlappingFieldsCanBeMerged::class => new OverlappingFieldsCanBeMerged(),
167165
UniqueInputFieldNames::class => new UniqueInputFieldNames(),

0 commit comments

Comments
 (0)