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

Implement FieldMiddleware Stack #221

Merged
merged 10 commits into from
Jul 26, 2018
Merged

Implement FieldMiddleware Stack #221

merged 10 commits into from
Jul 26, 2018

Conversation

mathewbyrne
Copy link
Contributor

@mathewbyrne mathewbyrne commented Jul 24, 2018

Following on from #217 this change introduces a the concept of DirectiveMiddleware to generated code.

A DirectiveRoot struct is now created and can be passed in when creating an executable schema. If a directive is encountered during codegen from the source schema, this DirectiveRoot will contain a field for each that the user can assign. Each field has the same signature as a ResolverMiddleware, and is run on fields with this directive at the end of the resolver middleware chain.

Within the directive middleware, users can access the current request and resolver contexts, and can either call the next middleware in the chain, or return a value early.

Limitations

  • Currently there is no direct support for directive arguments. This is coming soon but may depend on some refactoring around coerced values both in gqlgen and gqlparser.
  • If returning a new value, middlewares need to take care to return a value of the expected type, as this will be cast back after the middleware stack has resolved.
  • There is probably a refactor to happen around config now required to pass into NewExecutableSchema, and the config that can be provided to GraphQL.
  • This only supports schema-directives on fields definitions.
  • Currently DirectiveRoot is implemented as a struct, but this could easily also be an interface instead. Current thinking is that a struct allows a user to get up and running without having to implement their directives immediately; they are noops until set.

@mathewbyrne mathewbyrne changed the base branch from master to next July 24, 2018 04:45
}

func (d *Directive) Name() string {
return strings.Title(d.name)
func (d *Directive) GoName() string {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe this should be NameUpper? where is this used?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mathewbyrne mathewbyrne changed the title [WIP] Implement FieldMiddleware Stack Implement FieldMiddleware Stack Jul 25, 2018
Mathew Byrne added 10 commits July 26, 2018 10:45
This will allow us to include DirectiveMiddleware in the same middleware
setup, that will run after Resolver middlewares.
We need the Field Definition so that we can run directive middlewares
for this field.
Moves it off of RequestContext and into generated land.  This change
has a basic implementation of how directive middlewares might work.
@mathewbyrne mathewbyrne merged commit 803711e into next Jul 26, 2018
Copy link
Collaborator

@vektah vektah left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@vektah vektah deleted the middleware-stack branch July 26, 2018 02:58
@rajzru
Copy link

rajzru commented Aug 3, 2018

@mathewbyrne @vektah can we get HTTP headers value in field middlewares as mostly auth details are passed as HTTP Headers?

@mathewbyrne
Copy link
Contributor Author

@ridhamzuru you have access to Context in middleware, so depending on your authentication scheme, you probably want to do authentication in some middleware before graphql and pass through related data.

@rajzru
Copy link

rajzru commented Aug 6, 2018

@mathewbyrne Are we talking about graphql handler.RequestMiddleware as I have only specific mutation and queries on which I need auth so I can not use HTTP middleware as I will need the name of mutation or query on this.

This is what I get in the context variable so still can't access the HTTP variables
image

@mathewbyrne
Copy link
Contributor Author

Right, context doesn't have the HTTP data you're after on it directly. What I was getting at was that you should do authentication as a middleware, then use context to pass down relevant data (e.g. authenticated user id) to resolvers throughout your graph.

This has come up a few times now (#262), so we will look at getting some examples together for how we recommend doing this.

@rajzru
Copy link

rajzru commented Aug 6, 2018

But how can I get the name of query or mutation in authentication as a middleware approach? as this is something we need to decide if this request need auth or not. I have tried parsing the body and get the name by duplicating your lib code but this approach is completely redundant.

@mathewbyrne
Copy link
Contributor Author

It sounds like you might be mixing up your authentication and authorisation logic. Authentication should not need information about the query to happen. Once you have authenticated through whatever scheme you're using, you can make authorisation decisions at the resolver level using a directive.

@rajzru
Copy link

rajzru commented Aug 6, 2018

Can we one simple example for JWT with recommended way? Even small gist will do the work with server file and handler.
Thanks for the help it's highly appreciated 😄

@rajzru
Copy link

rajzru commented Aug 6, 2018

@mathewbyrne I have implemented with context suggestion and commented on #262
Thanks for the help anyways. You guys are doing the awesome job.

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.

3 participants