Skip to content

Commit

Permalink
RFC: Fix ambiguity with null variable values and default values
Browse files Browse the repository at this point in the history 
> This is a **behavioral change** which changes how explicit `null` values interact with variable and argument default values. This also changes a validation rule which makes the rule more strict.

There is currently ambiguity and inconsistency in how `null` values are coerced and resolved as part of variable values, default values, and argument values. This inconsistency and ambiguity can allow for `null` values to appear at non-null arguments, which might result in unforseen null-pointer-errors.

This appears in three distinct but related issues:

**Validation: All Variable Usages are Allowed**

The explicit value `null` may be used as a default value for a variable with a nullable type, however this rule asks to treat a variable's type as non-null if it has a default value. Instead this rule should specifically only treat the variable's type as non-null if the default value is not `null`.

Additionally, the `AreTypesCompatible` algorithm is underspecificied, which could lead to further misinterpretation of this validation rule.

**Coercing Variable Values**

`CoerceVariableValues()` allows the explicit `null` value to be used instead of a default value. This can result in a null value flowing to a non-null argument due to the validation rule mentioned above. Instead a default value must be used even when an explicit `null` value is provided. This is also more consistent with the explanation for validation rule "Variable Default Value Is Allowed"

Also, how to treat an explicit `null` value is currently underspecified. While an input object explains that a `null` value should result in an explicit `null` value at the input object field, there is no similar explaination for typical scalar input types. Instead, `CoerceVariableValues()` should explicitly handle the `null` value to make it clear a `null` is the resulting value in the `coercedValues` Map.

**Coercing Argument Values**

The `CoerceArgumentValues()` algorithm is intentionally similar to `CoerceVariableValues()` and suffers from the same inconsistency. Explicit `null` values should not take precedence over default values, and should also be explicitly handled rather than left to underspecified input scalar coercion.
  • Loading branch information
@leebyron
leebyron committed Mar 6, 2018
1 parent 54df48d commit ef26891
Show file tree
Hide file tree
Showing 2 changed files with 75 additions and 46 deletions.
42 changes: 29 additions & 13 deletions spec/Section 5 -- Validation.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1836,17 +1836,33 @@ an extraneous variable.
* For each {operation} in {document}
* Let {variableUsages} be all usages transitively included in the {operation}
* For each {variableUsage} in {variableUsages}
* Let {variableType} be the type of variable definition in the {operation}
* Let {argumentType} be the type of the argument the variable is passed to.
* Let {hasDefault} be true if the variable definition defines a default.
* AreTypesCompatible({argumentType}, {variableType}, {hasDefault}) must be true

* AreTypesCompatible({argumentType}, {variableType}, {hasDefault}):
* If {hasDefault} is true, treat the {variableType} as non-null.
* If inner type of {argumentType} and {variableType} are different, return false
* If {argumentType} and {variableType} have different list dimensions, return false
* If any list level of {variableType} is not non-null, and the corresponding level
in {argument} is non-null, the types are not compatible.
* Let {variableName} be the name of {variableUsage}
* Let {variableDefinition} be the {VariableDefinition} in {operation}.
* Let {variableType} be the type of {variableDefinition}.
* Let {defaultValue} be the default value of {variableDefinition}.
* If {variableType} is not a non-null and {defaultValue} is provided and not {null}:
* Let {variableType} be the non-null of {variableType}.
* Let {argumentType} be the type of the argument {variableUsage} is passed to.
* AreTypesCompatible({argumentType}, {variableType}) must be {true}.

AreTypesCompatible(argumentType, variableType):
* If {argumentType} is a non-null type:
* If {variableType} is a non-null type:
* Let {nullableArgumentType} be the nullable type of {argumentType}.
* Let {nullableVariableType} be the nullable type of {variableType}.
* Return {AreTypesCompatible(nullableArgumentType, nullableVariableType)}.
* Otherwise return {false}.
* If {variableType} is a non-null type:
* Let {nullableVariableType} be the nullable type of {variableType}.
* Return {AreTypesCompatible(argumentType, nullableVariableType)}.
* If {argumentType} is a list type:
* If {variableType} is a list type:
* Let {itemArgumentType} be the item type of {argumentType}.
* Let {itemVariableType} be the item type of {variableType}.
* Return {AreTypesCompatible(itemArgumentType, itemVariableType)}.
* Otherwise return {false}.
* If {variableType} is a list type return {false}.
* Return if {variableType} and {argumentType} are identical.

**Explanatory Text**

Expand Down Expand Up @@ -1890,8 +1906,8 @@ query booleanArgQuery($booleanArg: Boolean) {
}
```

A notable exception is when default arguments are provided. They are, in effect,
treated as non-nulls.
A notable exception is when a default value are provided. They are, in effect,
treated as non-nulls as long as the default value is not {null}.

```graphql example
query booleanArgQueryWithDefault($booleanArg: Boolean = true) {
Expand Down
79 changes: 46 additions & 33 deletions spec/Section 6 -- Execution.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -82,20 +82,27 @@ CoerceVariableValues(schema, operation, variableValues):
* For each {variableDefinition} in {variableDefinitions}:
* Let {variableName} be the name of {variableDefinition}.
* Let {variableType} be the expected type of {variableDefinition}.
* Assert: {variableType} must be an input type.
* Let {defaultValue} be the default value for {variableDefinition}.
* Let {value} be the value provided in {variableValues} for the name {variableName}.
* If {value} does not exist (was not provided in {variableValues}):
* If {defaultValue} exists (including {null}):
* Let {hasValue} be {true} if {variableValues} provides a value for the
name {variableName}.
* Let {value} be the value provided in {variableValues} for the
name {variableName}.
* If {hasValue} is not {true} or if {value} is {null}:
* If {variableType} is a Non-Nullable type, throw a query error.
* Otherwise if {defaultValue} exists (including {null}):
* Add an entry to {coercedValues} named {variableName} with the
value {defaultValue}.
* Otherwise if {variableType} is a Non-Nullable type, throw a query error.
* Otherwise, continue to the next variable definition.
* Otherwise, if {value} cannot be coerced according to the input coercion
rules of {variableType}, throw a query error.
* Let {coercedValue} be the result of coercing {value} according to the
input coercion rules of {variableType}.
* Add an entry to {coercedValues} named {variableName} with the
value {coercedValue}.
* Otherwise if {hasValue} is {true} and {value} is {null}:
* Add an entry to {coercedValues} named {variableName} with the
value {null}.
* Otherwise, {value} must not be {null}:
* If {value} cannot be coerced according to the input coercion
rules of {variableType}, throw a query error.
* Let {coercedValue} be the result of coercing {value} according to the
input coercion rules of {variableType}.
* Add an entry to {coercedValues} named {variableName} with the
value {coercedValue}.
* Return {coercedValues}.

Note: This algorithm is very similar to {CoerceArgumentValues()}.
Expand Down Expand Up @@ -530,8 +537,8 @@ order to correctly produce a value. These arguments are defined by the field in
the type system to have a specific input type: Scalars, Enum, Input Object, or
List or Non-Null wrapped variations of these three.

At each argument position in a query may be a literal value or a variable to be
provided at runtime.
At each argument position in a query may be a literal {Value} or {Variable} to
be provided at runtime.

CoerceArgumentValues(objectType, field, variableValues):
* Let {coercedValues} be an empty unordered Map.
Expand All @@ -543,30 +550,36 @@ CoerceArgumentValues(objectType, field, variableValues):
* Let {argumentName} be the name of {argumentDefinition}.
* Let {argumentType} be the expected type of {argumentDefinition}.
* Let {defaultValue} be the default value for {argumentDefinition}.
* Let {value} be the value provided in {argumentValues} for the name {argumentName}.
* If {value} is a Variable:
* Let {variableName} be the name of Variable {value}.
* Let {variableValue} be the value provided in {variableValues} for the name {variableName}.
* If {variableValue} exists (including {null}):
* Add an entry to {coercedValues} named {argName} with the
value {variableValue}.
* Let {hasValue} be {true} if {argumentValues} provides a value for the
name {argumentName}.
* Let {value} be the value provided in {argumentValues} for the
name {argumentName}.
* If {hasValue} is not {true} or if {value} is {null}:
* If {argumentType} is a Non-Nullable type, throw a field error.
* Otherwise, if {defaultValue} exists (including {null}):
* Add an entry to {coercedValues} named {argName} with the
* Add an entry to {coercedValues} named {argumentName} with the
value {defaultValue}.
* Otherwise, if {hasValue} is {true} and {value} is {null}:
* Add an entry to {coercedValues} named {argumentName} with the
value {null}.
* Otherwise, if {value} is a {Variable}:
* Let {variableName} be the name of {value}.
* If {variableValues} provides a value for the name {variableName}:
* Let {variableValue} be the value provided in {variableValues} for the
name {variableName}.
* Add an entry to {coercedValues} named {argumentName} with the
value {variableValue}.
* Otherwise, if {argumentType} is a Non-Nullable type, throw a field error.
* Otherwise, continue to the next argument definition.
* Otherwise, if {value} does not exist (was not provided in {argumentValues}:
* If {defaultValue} exists (including {null}):
* Add an entry to {coercedValues} named {argName} with the
* Otherwise, if {defaultValue} exists (including {null}):
* Add an entry to {coercedValues} named {argumentName} with the
value {defaultValue}.
* Otherwise, if {argumentType} is a Non-Nullable type, throw a field error.
* Otherwise, continue to the next argument definition.
* Otherwise, if {value} cannot be coerced according to the input coercion
rules of {argType}, throw a field error.
* Let {coercedValue} be the result of coercing {value} according to the
input coercion rules of {argType}.
* Add an entry to {coercedValues} named {argName} with the
value {coercedValue}.
* Otherwise, {value} must be a {Value} and not {null}:
* If {value} cannot be coerced according to the input coercion
rules of {argType}, throw a field error.
* Let {coercedValue} be the result of coercing {value} according to the
input coercion rules of {argType}.
* Add an entry to {coercedValues} named {argumentName} with the
value {coercedValue}.
* Return {coercedValues}.

Note: Variable values are not coerced because they are expected to be coerced
Expand Down

0 comments on commit ef26891

Please sign in to comment.