graphql-go
graphql-go copied to clipboard
graph-gophers
Cannot Represent Optional Fields with Defaults
Overview
We encountered a bug in production that has uncovered a mismatch in the relationship between optional fields with defaults and the Go types used to represent them. The parser should be corrected to add support for setting defaults on optional InputValues.
Current State
Consider this simple schema with one object:
schema {
query: Query
}
type Query {
buyIceCream(args: QueryInput): Boolean!
}
input QueryInput {
flavor: String = "VANILLA"
}
and the corresponding resolver:
func (r *resolver) BuyIceCream(args struct{ Args *struct{ Flavor *string } }) bool {
return true
}
The parsing method will reject this schema with this error could not unmarshal "VANILLA" (string) into *string: incompatible type.
Expected Behaviour
The expectation is that the parser is able to represent this schema and resolver combination and that all users of this library are able to set defaults on optional InputValues.
Further Notes & Workarounds
The common way to work around this is to use a non-pointer type to represent the optional argument. While the parser allows this, it can lead to runtime errors if a client supplies a (legal) null value for the field. This error message in this case is graphql: got null for non-null.
This also hopefully explains why we'd want optional fields with defaults in the first place. Including null as a possible input is sometimes the best way to represent our business domain.
I hope this write-up is sufficient. I can open a PR with some failing tests as a next step if that would be helpful. I'm going to continue working on solving the issue, but I thought that having the issue described here might help someone else who is seeing the same problem.
@rudle Did you tried to use string instead of *string in your corresponding resolver?
I tested this behavior now and it works perfectly with following setup:
input QueryInput { flavor: String = "VANILLA" }
func (r *Resolver) BuyIceCream(args struct{ Args *struct{ Flavor string } }) bool { if args.Args.Flavor == "VANILLA" { return true } return false }
I think there is no bug in this library and the issue can be closed.
The issue here is that the flavor input field is defined as a nullable String type, which means null or nil is a valid value for this field, but is not representable with the current implementation of the library.
{ buyIceCream(flavor: "CHOCOLATE") }
In this query, the default value is overridden with "CHOCOLATE".
{ buyIceCream }
In this query, the default value "VANILLA" is used because no value is specified.
{ buyIceCream(flavor: null) }
Here, an explicit null value is passed as the value of flavor.
This should be representable via the schema definition, but does not parse correctly. The library assumes that a nullable input field with default value becomes non-nullable.
This behavior is not fixed, re-opening the issue.
@pavelnikolov Could you point me to the lines where you parse the schema and check if the field can be a nullable or a non-nullable?
Is packer similar to a serializer?
A packer deserialized incoming arguments into type-safe Go struct fields/structs. I'll get back to you shortly.
The schema parsing is done by the lexer and by default it parses a nullable type, however, if it encounters an ! symbol, then it wraps the type in an ast.NonNull struct. Check here: https://github.com/graph-gophers/graphql-go/blob/master/internal/common/types.go#L10
You can find all possible types in the ast package. Then during query execution/validation the library validates the incoming executable definition (i.e. GraphQL query) against the AST schema which is usually parsed only once during application start time.
Just leaving a comment that this issue is still occurring in production for another consumer of the library. We'll be switching away from schema declared default values for optional arguments in our API for the time being, but less transparent API design to support a parsing bug is obviously less than ideal, and the PR to fix this seems like a straightforward fix, including updating the starwars example code to cover it.
After carefully reading the issue description I don't believe that this is an issue with the library. It's rather inappropriate use of the optional arguments. The GraphQL spec states re Arguments:
## Arguments
> Arguments can be either required or optional. When an argument is optional, we can define a default value - if the unit argument is not passed, it will be set to METER by default.
On the other side the docs state re Input Types:
> Input object types also can't have arguments on their fields.
The library already supports default values on optional arguments. See the starship for example. But what you request is not documented by the GraphQL spec so I do not plan to include it in the library for now. if the spec changes I'd be happy to work on/review/merge a PR.
It seems that input object's fields can be set to variables and variables in turn can have default values. That would solve your problem.
@pavelnikolov
While the OP's example is more narrowly scoped outside the GraphQL spec, the library actually breaks the spec you just quoted (When an argument is optional, we can define a default value) with this issue more broadly when the variable's type isn't an InputObject too.
For example in a schema:
getThing(count: Int = 10)
This would end up being a pointer to an int32 in the resolver, but then with the current version the resolver would choke trying to assign the default value as a pointer as opposed to as the value pointed to.
In my own breaking usecases, not a single default for optional values was an InputObject, all were basic types (i.e. string, int, float).
This would end up being a pointer to an int32 in the resolver, but then with the current version the resolver would choke trying to assign the default value as a pointer as opposed to as the value pointed to.
Can the nullable types in the library help?
@pavelnikolov Aren't the nullable types solving the opposite problem?
I was under the impression that the resolver for the above schema necessitated a pointer argument for the corresponding optional argument. Such as:
func (r *Resolver) GetThing(ctx context.Context, args *struct { Count *int32 }) (*[]*thingResolver, error) {
If that could instead be:
func (r *Resolver) GetThing(ctx context.Context, args *struct { Count NullInt }) (*[]*thingResolver, error) {
Then I'd suppose there wouldn't be an issue assigning the default value for the optional argument. I was just under the impression the null types were for required inputs that could have null as a value, not for optional inputs that could be nil when not present.
This would end up being a pointer to an int32 in the resolver, but then with the current version the resolver would choke trying to assign the default value as a pointer as opposed to as the value pointed to.
In the below case I am not using pointers since there is a default value specified. I tested with the following code - no pointers used! I tested with default values for Int and String (i.e. 0 and "") respectively and it worked.
package main
import (
"fmt"
"log"
"net/http"
graphql "github.com/graph-gophers/graphql-go"
"github.com/graph-gophers/graphql-go/relay"
)
type query struct{}
func (query) HelloStr(args *struct{ Name string }) string {
return fmt.Sprintf("Hello, %q!", args.Name)
}
func (query) HelloInt(args *struct{ Num int32 }) string {
return fmt.Sprintf("Got, %d!", args.Num)
}
func main() {
s := `
type Query {
helloStr(name: String = ""): String!
helloInt(num: Int = 0): String!
}
`
schema := graphql.MustParseSchema(s, &query{})
http.Handle("/query", &relay.Handler{Schema: schema})
log.Fatal(http.ListenAndServe(":8080", nil))
}
using the curl commands below:
➜ curl -XPOST -d '{"query": "{ helloStr() }"}' localhost:8080/query
{"data":{"helloStr":"Hello, \"\"!"}}
➜ curl -XPOST -d '{"query": "{ helloInt() }"}' localhost:8080/query
{"data":{"helloInt":"Got, 0!"}}
I'd appreciate a failing unit test or a piece of code that I can run and execute requests against to better understand the problem.
I tested with one more example:
package main
import (
"fmt"
"log"
"net/http"
graphql "github.com/graph-gophers/graphql-go"
"github.com/graph-gophers/graphql-go/relay"
)
type query struct{}
func (query) Test(args struct{ Num graphql.NullInt }) string {
if args.Num.Set && args.Num.Value == nil {
return "value set to NULL"
}
var num int32 = *args.Num.Value
return fmt.Sprintf("set=%v, val=%d", args.Num.Set, num)
}
func main() {
s := `
type Query {
test(num: Int = null): String!
}
`
schema := graphql.MustParseSchema(s, &query{})
http.Handle("/query", &relay.Handler{Schema: schema})
log.Fatal(http.ListenAndServe(":8080", nil))
}
Then, after go run main.go I tried with different queries:
➜ curl -XPOST -d '{"query": "{ test() }"}' localhost:8080/query
{"data":{"test":"value set to NULL"}}
➜ curl -XPOST -d '{"query": "{ test(num: 5) }"}' localhost:8080/query
{"data":{"test":"set=true, val=5"}}
➜ curl -XPOST -d '{"query": "{ test(num: null) }"}' localhost:8080/query
{"data":{"test":"value set to NULL"}}
If the resolver doesn't require a pointer receiver given there's a default value, then there's no issue at all!
Thanks for the heads up!!