-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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.
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
[RFC] List coercion algorithm #1058
base: main
Are you sure you want to change the base?
Conversation
✅ Deploy Preview for graphql-spec-draft ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This feels like it brings list coercion to the same (currently buggy due to nullable-with-default-values) state as all the other variable coercion so this feels right to me
spec/Section 3 -- Type System.md
Outdated
- Otherwise, if {itemValue} is a Variable: | ||
- If the variable provides a runtime value: | ||
- Let {coercedItemValue} be the runtime value of the variable. | ||
- Otherwise, if the variable definition provides a default value: | ||
- Let {coercedItemValue} be this default value. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a specific reason this doesn't use the "pre-coerced" variable values? (from CoerceVariableValues
)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The reason was that this is essentially copied from the input coercion for input objects (but via an algorithm to make it clearer): https://spec.graphql.org/draft/#sec-Input-Objects.Input-Coercion
But it's a good question. I guess the reason is that coercedVariableValues
is not explicitly made available in section 3 of the spec.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see... It may be an issue:
Ultra synthetic example:
type Query {
a(b: [[Int]]): Int
}
Operation:
query Foo($c: [Int]) {
a(b: [$c])
}
Runtime Variables:
{
"c": 42
}
If we're saying the runtime value is the "value that is sent over the wire", we end up with b = [42]
(incompatible) instead of b = [[42]]
if we coerce the variable to a list first (I think?)
If we're saying the runtime value is the "pre-coerced" value, then there's no need to mention defaultValue
76bec1c
to
6aed5a9
Compare
Fixes #1002.
Previously, list coercion does not detail what to do with variables at all, and that could lead to either a null pointer exception, or to double-coercion of the variable value if you're only following the spec.
Consider the following valid schema:
and the query that is valid against this schema:
NOTE: We're using the variable in a list item position!
If you issue this to the GraphQL server with variables
{"number": null}
thenCoerceVariableValues
will give you{"number": null}
and when you fast-forward toCoerceArgumentValues
you'll go in to 5.j.iii.1:https://spec.graphql.org/draft/#sel-NANTHHCJFTDFBBCAACGB0yS
coercedValues = {}
argumentValues = { numbers: [1, $number, 3] }
fieldName = 'sum'
field named {fieldName}.
argumentDefinitions = { numbers: ... }
argumentName = 'numbers'
argumentType = [Int!]!
defaultValue = undefined
{argumentName}.
hasValue = true
{argumentName}.
argumentValue = [1, $number, 3]
NOPE
{variableName}.
{variableName}.
value = [1, $number, 3]
NOT TRIGGERED
{defaultValue}.
not {true} or {value} is {null}, raise a field error.
NOT TRIGGERED
Yes, it is
It is not, it is a list
{null}.
It is not, it is a list
{value}.
YES
{argumentType}, raise a field error. TIME TO VISIT LIST COERCION
input coercion rules of {argumentType}.
{coercedValue}.
Time to visit list coercion
We need to coerce the value
[1, $number, 3]
to the non-nullable type[Int!]!
.Step 1: handle the non-null. It's not null. Great!
Now we need to coerce the value
[1, $number, 3]
to the list type[Int!]
.Here's what the spec says about input coercion for lists:
We have a list, so we only care about the bold line.
This line seems to miss a bunch of situations.
For example: if we were coercing to
[Int]
the value[1, $number, 3]
with variables{}
then is $number (which is undefined, since it wasn't provided in the variables) "accepted by the list's item type"? Really we must coerce this tonull
, but that doesn't seem to be detailed. In fact this entire section doesn't mention variables at all.We're actually coercing to
[Int!]
, so the question is: is$number
accepted by the list's item type?$number
itself is a variable, so...I've attempted to solve this problem by being much more explicit about the input coercion for lists, inspired by the input coercion for input objects. I've also added a non-normative note highlighting the risk of a null variable being fed through into a non-nullable position, why that can occur (validation) and what we do about it (field error). I've also expanded the table with both variables and many more examples to cover many more edge cases.