Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

RFC: Fix ambiguity with null variable values and default values #418

Merged
merged 7 commits into from
Apr 18, 2018
Merged

RFC: Fix ambiguity with null variable values and default values #418

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
ef26891
RFC: Fix ambiguity with null variable values and default values
leebyron Mar 6, 2018
3dc6d1b
Updated based on feedback.
leebyron Mar 30, 2018
2fbeb5d
One step further towards the idealized "from scratch" proposal, this …
leebyron Apr 3, 2018
ee6220b
Editing AreTypesCompatible() to avoid trailing "Otherwise return fals…
leebyron Apr 5, 2018
429bf2b
Update "All Variable Usages are Allowed" to remove breaking change.
leebyron Apr 6, 2018
3147689
Make related changes to input object coercion rules
leebyron Apr 6, 2018
03197f8
Final review edits
leebyron Apr 18, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
134 changes: 71 additions & 63 deletions spec/Section 5 -- Validation.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -710,25 +710,26 @@ and invalid.
* {arguments} must be the set containing only {argument}.


#### Required Non-Null Arguments
#### Required Arguments

* For each Field or Directive in the document.
* Let {arguments} be the arguments provided by the Field or Directive.
* Let {argumentDefinitions} be the set of argument definitions of that Field or Directive.
* For each {definition} in {argumentDefinitions}:
* Let {type} be the expected type of {definition}.
* If {type} is Non-Null:
* Let {argumentName} be the name of {definition}.
* For each {argumentDefinition} in {argumentDefinitions}:
* Let {type} be the expected type of {argumentDefinition}.
* Let {defaultValue} be the default value of {argumentDefinition}.
* If {type} is Non-Null and {defaultValue} does not exist:
* Let {argumentName} be the name of {argumentDefinition}.
* Let {argument} be the argument in {arguments} named {argumentName}
* {argument} must exist.
* Let {value} be the value of {argument}.
* {value} must not be the {null} literal.

**Explanatory Text**

Arguments can be required. If the argument type is non-null the argument is
required and furthermore the explicit value {null} may not be provided.
Otherwise, the argument is optional.
Arguments can be required. If the argument type is non-null and does not have a
default value, the argument is required and furthermore the explicit value
{null} may not be provided. Otherwise, the argument is optional.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I read this to imply (probably unintentionally) that a non-null argument with a default value is allowed to be passed an explicit null?


For example the following are valid:

Expand All @@ -738,7 +739,7 @@ fragment goodBooleanArg on Arguments {
}

fragment goodNonNullArg on Arguments {
nonNullBooleanArgField(nonNullBooleanArg: true)
requiredBooleanArgField(requiredBooleanArg: true)
}
```

Expand All @@ -752,19 +753,19 @@ fragment goodBooleanArgDefault on Arguments {
}
```

but this is not valid on a non-null argument.
but this is not valid on a required argument.

```graphql counter-example
fragment missingRequiredArg on Arguments {
nonNullBooleanArgField
requiredBooleanArgField
}
```

Providing the explicit value {null} is also not valid.

```graphql counter-example
fragment missingRequiredArg on Arguments {
notNullBooleanArgField(nonNullBooleanArg: null)
requiredBooleanArgField(requiredBooleanArg: null)
}
```

Expand Down Expand Up @@ -1358,6 +1359,31 @@ For example the following query will not pass validation.
```


### Input Object Required Fields
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This rule exists as part of GraphQL.js's ValuesOfCorrectType but was missing from the spec


**Formal Specification**

* For each Input Object in the document.
* Let {fields} be the fields provided by that Input Object.
* Let {fieldDefinitions} be the set of input field definitions of that Input Object.
* For each {fieldDefinition} in {fieldDefinitions}:
* Let {type} be the expected type of {fieldDefinition}.
* Let {defaultValue} be the default value of {fieldDefinition}.
* If {type} is Non-Null and {defaultValue} does not exist:
* Let {fieldName} be the name of {fieldDefinition}.
* Let {field} be the input field in {fields} named {fieldName}
* {field} must exist.
* Let {value} be the value of {field}.
* {value} must not be the {null} literal.

**Explanatory Text**

Input object fields can be required. If the input object field type is non-null
and does not have a default value, the input object field is required and
furthermore the explicit value {null} may not be provided. Otherwise, the input
object field is optional.


## Directives


Expand Down Expand Up @@ -1494,44 +1520,6 @@ fragment HouseTrainedFragment {
```


### Variable Default Value Is Allowed
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This rule is completely removed since the queries it prohibited we explicitly wish to support.


**Formal Specification**

* For every Variable Definition {variableDefinition} in a document
* Let {variableType} be the type of {variableDefinition}
* Let {defaultValue} be the default value of {variableDefinition}
* If {variableType} is Non-null:
* {defaultValue} must be undefined.

**Explanatory Text**

Variables defined by operations are allowed to define default values
if the type of that variable is not non-null.

For example the following query will pass validation.

```graphql example
query houseTrainedQuery($atOtherHomes: Boolean = true) {
dog {
isHousetrained(atOtherHomes: $atOtherHomes)
}
}
```

However if the variable is defined as non-null, default values
are unreachable. Therefore queries such as the following fail
validation

```graphql counter-example
query houseTrainedQuery($atOtherHomes: Boolean! = true) {
dog {
isHousetrained(atOtherHomes: $atOtherHomes)
}
}
```


### Variables Are Input Types

**Formal Specification**
Expand Down Expand Up @@ -1836,17 +1824,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}.
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Most of this change is for clarity, however the default value is now checked for {null} first


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.
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Other than the checking default values being removed from this algorithm, this is all for clarity and doesn't signify any other behavioral change


**Explanatory Text**

Expand Down Expand Up @@ -1890,8 +1894,12 @@ 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 is provided. Variables which include
a default value may be provided to a non-null argument as long as the default
value is not {null}.

Note: The value {null} could still be provided to a variable at runtime, and
a non-null argument must produce a field error if provided such a variable.

```graphql example
query booleanArgQueryWithDefault($booleanArg: Boolean = true) {
Expand Down
91 changes: 53 additions & 38 deletions spec/Section 6 -- Execution.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,20 +82,28 @@ 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} and {defaultValue} exists (including {null}):
* Add an entry to {coercedValues} named {variableName} with the
value {defaultValue}.
* Otherwise if {variableType} is a Non-Nullable type, and either {hasValue}
is not {true} or {value} is {null}, throw a query error.
* Otherwise if {hasValue} is true:
* If {value} is {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}.
value {null}.
* 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}.
* Return {coercedValues}.

Note: This algorithm is very similar to {CoerceArgumentValues()}.
Expand Down Expand Up @@ -530,8 +538,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 +551,37 @@ 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}.
* Otherwise, if {defaultValue} exists (including {null}):
* Add an entry to {coercedValues} named {argName} 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} does not exist (was not provided in {argumentValues}:
* If {defaultValue} exists (including {null}):
* Add an entry to {coercedValues} named {argName} 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}.
* Let {hasValue} be {true} if {argumentValues} provides a value for the
name {argumentName}.
* Let {argumentValue} be the value provided in {argumentValues} for the
name {argumentName}.
* If {argumentValue} is a {Variable}:
* Let {variableName} be the name of {argumentValue}.
* 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}.
* Otherwise:
* Let {value} be {argumentValue}.
* If {hasValue} is not {true} and {defaultValue} exists (including {null}):
* Add an entry to {coercedValues} named {argumentName} with the
value {defaultValue}.
* Otherwise if {argumentType} is a Non-Nullable type, and either {hasValue}
is not {true} or {value} is {null}, throw a field error.
* Otherwise if {hasValue} is true:
* If {value} is {null}:
* Add an entry to {coercedValues} named {argumentName} with the
value {null}.
* Otherwise, if {argumentValue} is a {Variable}:
* Add an entry to {coercedValues} named {argumentName} with the
value {value}.
* Otherwise:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You probably don't want this "otherwise" here, since coercion and resulting failures need to happen if {argumentValue} is a {Variable}.

Copy link
Collaborator Author

@leebyron leebyron Apr 12, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is intentional. Variables are coerced during CoerceVariableValues before execution of a query at which point and resulting failures are also reported before execution, so a second coercion is not necessary.

There's an existing note below this algorithm capturing this point

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah I see – I missed that note below. 👍

* If {value} cannot be coerced according to the input coercion
rules of {variableType}, throw a field error.
* Let {coercedValue} be the result of coercing {value} according to the
input coercion rules of {variableType}.
* 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