type of ResolveInfo.RootValue should be interface{} about graphql HOT 8 CLOSED

Closing issue due to inactivity. You can find more info on the current ResolveInfo in godoc

from graphql.

Hi @bsr203
Thanks for bring this up and keeping it from getting buried 👍🏻

From @pyros2097 comments in #25

I was just going to post an issue on the inconsistent API passing root value,
In the Executor params you specify Root as an interface{} here https://github.com/chris-ramon/graphql-go/blob/master/executor/executor.go#L15
But in the GraphqlParams you specify it as a map[string]interface{} here https://github.com/chris-ramon/graphql-go/blob/master/graphql.go#L15
Maybe the RootObject should be an interface because anyway we are going to attach structs to it like session data or some other context which we will be typecasting to anyway.

I think the main take-away from that PR is that RootValue can be used to provide context for each GraphQL request (see graphql/graphql-js#56). But currently, RootValue type is inconsistent.

package graphql

// passed into `graphql.Graphql(...)`
type Params struct {
    RootObject     map[string]interface{}
}

type ExecuteParams struct {
    Root          interface{}
}

type BuildExecutionCtxParams struct {
    Root          interface{}
}

// finally available as RootValue in ResolveInfo through GQLFRParams 
type ResolveInfo struct {
    RootValue      interface{}
}

Along the way, RootObject gets renamed as Root, and finally RootValue.
In addition to that, to add to the confusion, in FieldResolveFn, RootValue is made available through GQLFRParams as both p.Source and p.Info.RootValue. (i.e. p.Source === p.Info.RootValue)

It would be great to have a consistent name for RootValue.

While I'm leaning towards updating RootObject/RootValue to map[string]interface{},
I would like to hear if anyone think it should be interface{} instead? (Or another type, perhaps context)?

Changing it map[string]interface may not necessarily remove extra casting; you may still need to cast the interface{} value stored in the map most of the time anyway. But it does allow you to store multiple context values in a map and make it available in FieldResolveFn.

Discussion welcomed!
Cheers guys!

from graphql.

It does avoid one casting/type assertion though

Resolve: func(p gt.GQLFRParams) interface{} {
            rv := p.Info.RootValue.(map[string]interface{})
            ctx := rv["ctx"].(context.Context)
...
}

we know, RootValue is map[string]interface{}, but currently it is defined as interface{}, so to access anything from it, first need to cast like

            rv := p.Info.RootValue.(map[string]interface{})

otherwise above example could have been

Resolve: func(p gt.GQLFRParams) interface{} {
            ctx := p.Info.RootValue["ctx"].(context.Context)

from graphql.

Oh yes you are right, I meant to say it does not remove/eliminate all casting, but like you said, it does avoid one casting; sorry, its wayyyy past my bed time here right now lol 😄,

Again, I'm all for RootValue being map[string]interface{} 👍🏻

As @pyros2097 brought up a suggestion previously that RootObject/RootValue could be constantly be made an interface{}, I want to know if anyone else feel the same, just to hear a different perspective on this.

from graphql.

another consideration being it https://godoc.org/golang.org/x/net/context type, as from the original pull request, if any of the deadlines, cancelation signals, useful. I would definitely use it for request-scoped values. which is also more idiomatic in go land.

from graphql.

@bsr203 The context type seems good since we are going to be passing a context object usually and most of the other http server libs use it for request handling. But I Think this will make sense only if you use graphql within the http server context. I prefer the rootValue as an interface{} type as we are most probably going to attach all our session data and env variables to a struct and that would become the rootValue.

from graphql.

@pyros2097 context has nothing to do with http though. These are the only dependencies.

"   errors"
    "fmt"
    "sync"
    "time"

they are used to store per request data and allow cancellation of goroutines elegantly. Also, you could store any value in it (Value(key interface{}) interface{}) and access easily though the provided interface. They are widely used outside http https://godoc.org/golang.org/x/net/context?importers

Edit:

@pyros2097 thinking about it it may be ok to be consistently interface{} (not map[string]interface{}) as we could easily type assert it.

ctx := val.(net.Context)

so, you guys decide, but not map[string]interface{} :-)

from graphql.

Should RootValue be removed, now that we have Context?

Context appears to satisfy most of the use cases of RootValue, EXCEPT for sharing data between resolvers during a request. That is, two resolve functions (possibly executing in parallel) could, in theory, coordinate via RootValue, right? However, the Context is not meaningfully mutable in the current implementation.

from graphql.