Add context throughout graphql

[tallstreet]

It would be useful to have a context that was maintained through the graphql library so when resolving fields we can change what we want to return based on the context of the request.

See https://blog.golang.org/context for use cases.

[bbuck]

I like the concept you're going after. But I don't agree with using a context. I think it would better if the request were attached to the GQLFRParams struct then one could implement whatever form of context they wanted. Such as loading session data from a database based on a session cookie or something.

Essentially what I"m getting at is that I think the use case of this change is far too small to suit a general purpose library, where as providing the http.Request opens you up to any standard options, really.

[tallstreet]

The only issue I can see with that is if there were several resolve functions that needed to be run to return a request, wouldn't each of them need to independently lookup the session data from the database (sure there could be a caching layer to make it that efficient but that is just one use-case).

Having a context would allow you to store and share any state between all functions needed to resolve an http request. Context also has the other benefits such as being able to manage a request deadline and kill slow resolve functions which you couldn't do by just passing the http_request round.

The context could be put on GQLFRParams instead and that would be a more transparent change, passing it as a first argument is just a convention that google adopted and mentioned in the blog article, so I applied it here.

[bsr203]

+1. Love to have this. Needed for GAE apps where all the operations are request bounded.

[sogko]

Hi guys,

@tallstreet Thanks for the PR! Really appreciate you putting your time to contribute to this project.

I agree that having a per-request context would be really useful, so the question now is how do we allow user to store context for them to use.

Currently, GraphQLFieldResolveFn, GraphQLResolveInfo and GQLFRParams are defined as the following

type GQLFRParams struct {
    Source interface{}
    Args   map[string]interface{}
    Info   GraphQLResolveInfo
    Schema GraphQLSchema
}
type GraphQLFieldResolveFn func(p GQLFRParams) interface{}
type GraphQLResolveInfo struct {
    FieldName      string
    FieldASTs      []*ast.Field
    ReturnType     GraphQLOutputType
    ParentType     GraphQLCompositeType
    Schema         GraphQLSchema
    Fragments      map[string]ast.Definition
    RootValue      interface{}
    Operation      ast.Definition
    VariableValues map[string]interface{}
}

So right now we have the following possibilities:

• Proposal 1: Add context to GraphQLFieldResolveFn args
  As proposed by @tallstreet. This conforms with convention set by google as mentioned.
• Proposal 2: Add context to GQLFRParams struct
  As proposed by @bbuck.

May I proposed that we look into using root params to store context?

• Proposal 3: Use root to store context

This was brought up by an issue in graphql/graphql-js#56 and it seems that it was designed for this particular use-case.

The issue for graphql-go here though is that there is lack of documentation for this and unclear/inconsistent API in passing root value.

package gql
type GraphqlParams struct {
    ...
    RootObject     map[string]interface{}
}

package executor
type ExecuteParams struct {
    ...
    Root          interface{}
}

package types
type GraphQLResolveInfo struct {
    ...
    RootValue      interface{}
}

One benefit is that it does not force user to use a particular context library.

What you guys think?

/cc @chris-ramon

[bsr203]

It indeed work now, but involves unnecessary casting.

at the handler

    ctx, err := NewContext(r)
    if err != nil {
        panic(err) //TODO
    }

    // execute graphql query
    resultChannel := make(chan *types.GraphQLResult)
    params := graphql.GraphqlParams{
        Schema:         *h.Schema,
        RequestString:  opts.Query,
        RootObject:     map[string]interface{}{"ctx": ctx},
        VariableValues: opts.Variables,
        OperationName:  opts.OperationName,
    }
    go graphql.Graphql(params, resultChannel)
    result := <-resultChannel

and in the field config

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

like @sogko suggested, the API should be consistent and should retain the name and the type map[string]interface{} throughout. This will at least avoid the need for rv := p.Info.RootValue.(map[string]interface{}) casting.

At the same time, like the issue graphql/graphql-js#56 , we almost always need a request context, and like it to be explicit and convenient. So, I back this PR or a field ctx (context.Context) as part of RootValue .

For the reference, see a different implementation here also by @tallstreet which uses context throughout.

https://github.com/tallstreet/graphql
https://github.com/tallstreet/todographqlgo

[bbuck]

My original comments were uneducated. After I remarked here I went looking for how graphql-js handled sessions (for authentication purposes) and from there attempted to use the RootValue piece as demonstrated for graphql-js but with little success (mostly probably my own fault but I haven't had a significant amount of time to look at it again recently).

So I'm totally on board with @sogko's suggestion of doing it through the RootValue and I think I agree with @bsr203, if I understand his post correctly. Keeping that as a general purpose map[string]interface{} opens it up to any implementation chosen by consumers - not just Context, or Request, etc...

[pyrossh]

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.

[F21]

Any update on this one?

[pkieltyka]

I think it would be great to see this. context.Conext is quickly becoming the standard for controlling goroutine cancellation and passing data down a chain of handlers. It's easy to use and implement :)

I built GitHub.com/pressly/chi which shows you those benefits as an http service. You can picture how the http endpoint to graphql would pass it's context to graphql for timing out, passing some state by composition, graceful shutdowns, or invoking a cancellation of a running goroutine.

Please :)

On Nov 24, 2015, at 2:58 AM, Francis Chuang notifications@github.com wrote:

Any update on this one?

—
Reply to this email directly or view it on GitHub.

[bsr203]

I think having this and Resolve returns an error in addition to the response will improve the API. see the dicussion at #44

[pkieltyka]

@bsr203 I saw that issue too. I agree an error should be returned instead of panic`ing.

[augustoroman]

Well, I totally missed this PR and went off and did the same thing. Now that I'm all caught up, my 2 cents:

• We should use context.Context directly instead of providing a generic map. The context.Context provides both an arbitrary metadata mechanism and a goroutine cancellation/management mechanism that is quickly becoming the standard. We will almost certainly want to use the goroutine mechanisms in the future. If we also provide a generic map, we will have redundant mechanisms for arbitrary metadata. Boo.
• Root appears to be an appropriate place to put this, but should be renamed to 'Context' or something to be clear that it's request-scoped, not global. I would normally expect Root* to be a single entity shared amongst all in-flight request handlers, which is explicitly NOT what the context is.

(minor side note: humorously, of the three possible locations to insert a Context (func signature, Params, or Info), I happened to pick the the third one that nobody else did. d'oh)

[chris-ramon]

closing this one in favor of #98 - thanks a lot for great feedback 👍