Skip to content
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

Parent middleware generated contexts #353

Merged
merged 1 commit into from
Oct 2, 2018
Merged

Conversation

vektah
Copy link
Collaborator

@vektah vektah commented Sep 25, 2018

Currently any context generated by resolver middleware is only passed to the resolver, not to the children. This means tracing middleware like OpenTracing and OpenCensus cant correctly set the parent span info.

This is an alternative to #327 that uses the context that would be passed to the resolver, instead of returning one from the outer most middleware.

Advantages:

  • Correct order when using multiple middleware
  • Not a BC break

I haven't addressed setting context from resolvers, but it's more obvious that you cant manipulate the context from inside a resolver (no next func). If we need it we can add a ChildContext prop to ResolverContext with a method that lets the user change it.

Thoughts @vvakame?

I have:

  • Added tests covering the bug / feature (see testing)
  • Updated any relevant documentation (see docs)

@vektah vektah changed the title Parent resolver generated contexts Parent middleware generated contexts Sep 25, 2018
@vektah vektah force-pushed the resolver-ctx-parenting branch from bb2b8e4 to faf0416 Compare October 2, 2018 00:16
@vektah vektah merged commit 2ab05da into master Oct 2, 2018
@vektah vektah deleted the resolver-ctx-parenting branch October 2, 2018 00:29
@vvakame
Copy link
Collaborator

vvakame commented Oct 15, 2018

ah, looks good!
I'll try this change in my environment 👍

@leebenson
Copy link

I haven't addressed setting context from resolvers, but it's more obvious that you cant manipulate the context from inside a resolver (no next func). If we need it we can add a ChildContext prop to ResolverContext with a method that lets the user change it.

👍 We're running into exactly this issue with auth context that could do with being set inside a resolver.

Would you be open to a PR to add the ChildContext you mentioned?

cgxxv pushed a commit to cgxxv/gqlgen that referenced this pull request Mar 25, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants