Allow undefined variable in query #70
Conversation
| @@ -43,7 +43,7 @@ type InputValue struct { | |||
| Name string | |||
| Description string | |||
| Type Type | |||
| DefaultValue string | |||
| DefaultValue *string | |||
There was a problem hiding this comment.
can you reference the spec doc in the commit message?
http://facebook.github.io/graphql/October2016/#sec-The-__InputValue-Type
| @@ -36,7 +36,7 @@ func valueToJson(value ast.Value, vars map[string]interface{}) (interface{}, err | |||
| case *ast.Variable: | |||
| actual, ok := vars[value.Name.Value] | |||
| if !ok { | |||
| return nil, NewClientError("unknown var: %s", value.Name.Value) | |||
| return nil, nil | |||
There was a problem hiding this comment.
let's add tests for the 2x2:
- argument is optional, and argument is not specified in variables
- argument is optional, and argument is specified in variables
- argument is not optional, and argument is not specified in variables
- argument is not optional, and argument is specified in variables
From the spec: http://facebook.github.io/graphql/October2016/#sec-The-__InputValue-Type `null` is the correct default value for a variable that is not provided and does not have its own default value. This will allow clients to pass in arguments as `undefined` (i.e. omitted from the json variables block) without subtlely breaking queries.
oliversong
force-pushed
the
oliver/thunder-fix-default-types
branch
from
c943689
to
d50e086
Compare
graphql/end_to_end_test.go
Outdated
| } | ||
|
|
||
| if !reflect.DeepEqual(internal.AsJSON(result), internal.ParseJSON(` | ||
| {"optional": -1}`)) { |
There was a problem hiding this comment.
I think I expect an error here, since we passed in nothing, but the variable is required
There was a problem hiding this comment.
Oh you're right. I think I need to make a "mandatory" field that has a matching go function definition
There was a problem hiding this comment.
I believe the fact that this test passes right now implies that we are doing something wrong though - your variable is declared as non null, but its null-ness is never checked before being passed down to the executor.
There was a problem hiding this comment.
(i.e. even though none of the users of the argument require it, the query itself should be requiring it.)
graphql/end_to_end_test.go
Outdated
| builtSchema := schema.MustBuild() | ||
|
|
||
| // argument is optional and not passed in | ||
| q := graphql.MustParse(` |
There was a problem hiding this comment.
can we clean up the copy/paste in an inner function to make this easier to read?
oliversong
force-pushed
the
oliver/thunder-fix-default-types
branch
3 times, most recently
from
d660127
to
d71d6fd
Compare
|
Smoke tested. Nothing blew up. |
graphql/end_to_end_test.go
Outdated
| "testArg": float64(5), | ||
| } | ||
|
|
||
| // arg is optional and passed in |
There was a problem hiding this comment.
nit: prefer full sentences (capital/period) in go comments
| @@ -36,7 +36,7 @@ func valueToJson(value ast.Value, vars map[string]interface{}) (interface{}, err | |||
| case *ast.Variable: | |||
| actual, ok := vars[value.Name.Value] | |||
| if !ok { | |||
There was a problem hiding this comment.
As we discussed irl, there is a new bug introduced in this PR that we think we are okay with ignoring for now.
That is, if a variable is marked optional in the operation definition, we do not check for whether or not it is passed in, which goes against the spec. We think this is okay because if the variable was actually required (i.e. used at some location), then the executor will get unhappy about it downstream.
There was a problem hiding this comment.
Awesome. Thanks for writing the issue.
oliversong
force-pushed
the
oliver/thunder-fix-default-types
branch
from
d71d6fd
to
4dd9dc5
Compare
Before:
After:
