Skip to content

Adds Context support. #98

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

Merged
merged 2 commits into from
Jan 7, 2016
Merged

Adds Context support. #98

merged 2 commits into from
Jan 7, 2016

Conversation

chris-ramon
Copy link
Member

Details

  • Adds support for Context available on each field's Resolve function, eg:

    graphq.Do({
      // ...
      Context: context.WithValue(context.Background(), "currentUser", user)
    })
    // ...
    Resolve: func(p graphql.ResolveParams) {
      u := p.Context.Value("currentUser")
    }
    • On previous related pr's, Context was exposed:
      • A new param of Resolve function - func(ctx context.Context, p ResolveParams)
      • Through graphql.ResolveParams.Info.Context
    • I think it's more concise to expose context as field of graphql.ResolveParams.Context like to example showed above, if any thoughts on this please do share 👍
  • Thanks a lot to @tallstreet & @augustoroman who initially proposed and worked on this on Add context throughout graphql #25 and Add context parameter that passes through the API to resolvers. #82.

augustoroman and others added 2 commits January 5, 2016 03:09
This adds a net/context.Context parameter that is threaded through from
the calling API to any resolver functions.  This allows an application
to provide custom, per-request handling when resolving queries.

For example, when working on App Engine, all interactions with the
datastore require a per-request context.  Other examples include
authentication, logging, or auditing of graphql operations.

An alternative that was considered was to use an arbitrary, application-
provided interface{} value -- that is, the application could stick
anything in that field and it would be up to the app to handle it.  This
is fairly reasonable, however using context.Context has a few other
advantages:
  - It provides a clean way for the graphql execution system to handle
    parallelizing and deadlining/cancelling requests.  Doing so would
    provide a consistent API to developers to also hook into such
    operations.
  - It fits with a potentially upcoming trend of using context.Context
    for most HTTP handlers.

Going with an arbitrary interface{} now, but later using context.Context
for its other uses as well would result in redundant mechanisms to provide
external (application) metadata to requests.

Another potential alternative is to specifically provide just the
*http.Request pointer.  Many libraries do this and use a global,
synchronized map[*http.Request]metadata lookup table.  This would satisfy
the AppEngine requirements and provide a minimal mechanism to provide
additional metadata, but the global LUT is clumsy and, again, if
context.Context were later used to manage subprocessing it would provide
a redundant metadata mechanism.
@chris-ramon chris-ramon self-assigned this Jan 5, 2016
@chris-ramon chris-ramon added this to the alpha-0.1 milestone Jan 5, 2016
@augustoroman
Copy link
Contributor

Cool! I like it.

  • Augusto

This was referenced Jan 6, 2016
chris-ramon added a commit that referenced this pull request Jan 7, 2016
@chris-ramon chris-ramon merged commit 14c8c6d into master Jan 7, 2016
@chris-ramon chris-ramon deleted the context branch January 7, 2016 21:52
@andreyvital
Copy link

Finally! 🍻

augustoroman added a commit to augustoroman/handler that referenced this pull request Jan 16, 2016
…ext.

Following graphql-go/graphql#98, it's now possible
to provide a Context to the graphql resolve functions.  This exposes that
capability in the handler API.

This follows the convention of putting the Context as the first arg, and
the apparently evolving context-aware http handler func signature.
@ghost
Copy link

ghost commented Feb 11, 2016

Nice now I can use https://github.com/labstack/echo context instead of a rootValue

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants