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

Added ability to use your own directives usage #446

Closed
wants to merge 1 commit into from

Conversation

eko
Copy link
Contributor

@eko eko commented Apr 8, 2021

Hi,

Description

This PR goal is to bring ability to use so custom directives, as mentioned in issue #151.

I would be interested in having feedbacks, mainly on the way that I am currently re-using the internal/common package exported to be used publicly in pkg/common: maybe I could just copy objects I need (but I need a lot of these defined in package ;-)).

Example usage

Assuming that I have the following schema:

directive @customDirective(
	customAttribute: String!
) on FIELD_DEFINITION

schema {
	query: Query
}

type Query {
	myTestQuery: String! @customDirective(customAttribute: hi)
}

I can use the directive in my resolver by adding a new argument:

func (r *customDirectiveResolver) MyTestQuery(ctx context.Context, directives common.DirectiveList) string {
	customDirective := directives.Get("customDirective")
	customAttribute, _ := customDirective.Args.Get("customAttribute")

	return fmt.Sprintf(
		"Hello custom directive '%s' with attribute value '%s'!",
		customDirective.Name.Name,
		customAttribute.String(),
	)
}

Thank you!

@eko eko changed the title Added support of custom directives Added ability to use your own directives usage Apr 8, 2021
@pavelnikolov
Copy link
Member

Thank you for your contribution. I really want custom directives to be supported in the library! There are few things to discuss, though.

  1. Does your implementation support only directives on FIELD_DEFINITIONs? Do you support directives on objects, fragments and inline fragments?
  2. Regarding the private types that are made public - take a look at this PR add types package #437 it already exports some types and it will be merged soon.
  3. The final thing that I want to get resolved before merging this is refactor the signature of the resolvers. (Opt-in and backwards compatible, of course). Now we support no args, ctx and ctx + args. With this implementation we now have one more optional argument - directives. But in another PR that I want to get in soon(ish) is selected fields that allows people to prefetch the data of fields beforehand. So I was thinking about an optional signature that has a Request argument, for example:
func (r *Resolver) Something(ctx context.Context, req *graphql.Request) (*SomethingResponse, error) {
    // req contains input args, directives and selected fields. Potentially we can add more things in future without changing the signature.
}

@eko
Copy link
Contributor Author

eko commented Apr 9, 2021

@pavelnikolov,

Thanks for your quick & clear reply!

  1. You're right, actually it only supports FIELD_DEFINITION, maybe we could add support for OBJECT and FRAGMENT too in a second time? I've tried to take a look and I don't think I am too smart with reflection to do this actually, or maybe you can point me to the right direction?

  2. Thanks for pointing me to this PR add types package #437, this is exactly what I need so I will wait for it to be merged and will use it afterwards.

  3. I understand your arguments and what you want but I find it pity to loose the typing of the arguments because we will retrieve a common.Request object so users will have to do casting like this in order to retrieve their arguments and we will not benefit of schema validation anymore?

type PaginationArg struct {
    Offset int32
}

func (r *Resolver) Something(ctx context.Context, req *graphql.Request) (*SomethingResponse, error) {
    myDirective := req.Directives.Get("myCustomDirective") // No problem here, directives are graphql-go types

    myPaginationArg, ok := req.Args.Get("Pagination").(*PaginationArg)
    if ok {
        offset := myPaginationArg.Offset
    }
}

Anyway, maybe it's acceptable, I will try to implement this in the coming days and will keep you informed.

I just seen this PR #428 that adds selected fields into the contex, maybe it is a good solution to put the directives into the context too and provide a method to retrieve them? What is your point of view?

@eko
Copy link
Contributor Author

eko commented Apr 12, 2021

@pavelnikolov After thinking about your suggestions, a colleague @iotafr 👋 point me to what Apollo is doing and I think this is a good way: https://www.apollographql.com/docs/apollo-server/schema/creating-directives/

I've updated the PR to use this kind of Visitor pattern. I think it takes the following advantages:

  • We don't touch to the current resolvers objects,
  • We can easily create a single directive visitor to act on multiple fields,
  • We could easily support objects & fragments directives (I will try to take a look at this now that I have something working).

Directive visitor must implement the following interface and can act before the resolver to be called or after:

type DirectiveVisitor interface {
	Before(directive *Directive, input interface{}) error
	After(directive *Directive, output interface{}) (interface{}, error)
}

What do you think? I've kept the old previous logic on a branch in case you don't like this one ;)

@eko eko force-pushed the custom-directives branch 3 times, most recently from 4a97316 to ada73c5 Compare April 12, 2021 13:14
@@ -1,5 +1,10 @@
package common

type DirectiveVisitor interface {

Choose a reason for hiding this comment

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

Maybe in some cases we need to access to the context at the directive level. What do you think to add ctx context.Context in After and Before definitions ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You're right, I just added the context

Copy link
Member

Choose a reason for hiding this comment

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

One such case would be with a caching directive, since caching info can be stored in the context.

@pavelnikolov
Copy link
Member

@eko I really like the visitor pattern!

@pavelnikolov
Copy link
Member

@eko #437 has been merged

@eko
Copy link
Contributor Author

eko commented Apr 23, 2021

@pavelnikolov Great thanks! I've just updated the PR with types exported by #437.

I've tried to take a look at how I can retrieve the object directives but without success actually, I've to spend more time to understand the internal workflow.

@pavelnikolov
Copy link
Member

There are some conflicts. @eko can I help you to get this working?

@eko
Copy link
Contributor Author

eko commented Dec 28, 2021

Hi @pavelnikolov,

Sorry for the delay, I was off for a couple of days.

I just rebased the pull request in case you want to merge the first part. Unfortunately, I will not have any much time to work on other directives implementations.

Thank you

graphql_test.go Outdated Show resolved Hide resolved
@pavelnikolov
Copy link
Member

@eko thank you very much for rebasing this code. I left one comment re where to pass the directive visitors. I like this pattern but it will require some more work.
Regarding your comment about "on other directives implementations" what do you mean with "other"? Do you mean object, interface, union and argument directives (anything other than fields)?

@eko
Copy link
Contributor Author

eko commented Jan 7, 2022

Hello @pavelnikolov,

Thank you, you're right, this is so much better to have them at the schema parse section, I just updated the PR in order to add a new DirectiveVisitors() Schema option to pass them.

Concerning the other directives implementations: yes, this implementation works with FIELD_DEFINITION type. That's a first step but other types have to also be implemented later.

@pavelnikolov
Copy link
Member

@eko OK, this is much better now that the directive visitors are not passed with Exec. I was thinking about adding DirectiveVisitors() method to the root resolver, but maybe your idea is better to be passed as a schema option. I like it.

@eko
Copy link
Contributor Author

eko commented Jan 10, 2022

I also found it was interesting to have them declared as schema options but let me know if you prefer to have them on root resolver.

@pavelnikolov
Copy link
Member

pavelnikolov commented Jan 10, 2022

Merci beaucoup, @eko! I was thinking the same thing. Then I thought to myself "What if there is a resolver field named 'DirectiveVisotrs'". I know that this is highly unlikely. The notion of Schema options is that they are optional and we can turn the options on and off. However, in the case of directives, is it possible that they are included in the schema, but not included in the options? Then what do we do? Do we error out or do we ignore them? If we ignore them, some people might be confused in the other case other people will get confused. If we leave them in the resolver, how do we pass them around so that the method for directives doesn't interfere with resolver fields/methods? 🤔

@pavelnikolov
Copy link
Member

These before/after visitors are only going to be invoked if the fields have method resolvers. How about field-resolvers?
https://github.com/graph-gophers/graphql-go/pull/446/files#diff-87ea4cfcdd6bb8062908727b36ef05d305ddfbd5ed89322e2276f315ac63dd1bR204

}
}
}
}
Copy link
Member

Choose a reason for hiding this comment

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

These visitors will only be invoked if we use method resolvers. How about when we use struct fields as resolvers?

Copy link
Member

@pavelnikolov pavelnikolov Jan 11, 2022

Choose a reason for hiding this comment

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

I'm talking about lines 263 to 269

values = append(values, inValue.Interface())
}

if visitorErr := visitor.Before(ctx, directive, values); err != nil {

Choose a reason for hiding this comment

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

this should be
if visitorErr := visitor.Before(ctx, directive, values); visitorErr != nil {

for _, directive := range f.field.Directives {
if visitor, ok := r.Visitors[directive.Name.Name]; ok {
returned, visitorErr := visitor.After(ctx, directive, result.Interface())
if err != nil {

Choose a reason for hiding this comment

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

this should be if visitorErr != nil

kopancek added a commit to sourcegraph/sourcegraph-public-snapshot that referenced this pull request Sep 29, 2022
This only works on token based authorization, when using non-internal
tokens. Existing functionality was kept backwards compatible on this
POC, so if you have a token with "user:all" or "site-admin:sudo" everything
works like before. Similarly session based auth still works the same.

Added a directive `@authz(scopes: ["some_scope"])` which controls
scopes that are required when calling the graphql API with a token
based authentication. When this directive is present on a field,
query, mutation or type, it is required that the token has scopes
that are listed. The beauty of this approach is, that we can add
different scopes to different queries/fields/mutations and single source
of truth is our graphql schema.

- unit tests
- failing authorization on queries/mutations/fields without the directive
- adding scopes to all needed graphql entities
- UI changes to create tokens with more scopes
- making backwards incompatible changes
- authorizing internal API access

Tested locally.

To test locally, you need to modify `go.mod` to point to your own
local fork of graph-gophers/graphql-go#446 It is also needed to apply
the patch suggested in this comment: https://github.com/graph-gophers/graphql-go/pull/446/files#r914374506

To create a token with different scopes, create a normal token as you would
usually by going to Settings -> Access tokens. You need to modify
the scopes directly in the database (yikes!). Search the `schema.graphql` file
for `@authz` directive scope definitions that are required with these changes.

You then need to use the token directly with curl or similar.

When using the token without proper scopes, you should see graphql errors
instead of data.
kopancek added a commit to sourcegraph/sourcegraph-public-snapshot that referenced this pull request Oct 12, 2022
This only works on token based authorization, when using non-internal
tokens. Existing functionality was kept backwards compatible on this
POC, so if you have a token with "user:all" or "site-admin:sudo" everything
works like before. Similarly session based auth still works the same.

Added a directive `@authz(scopes: ["some_scope"])` which controls
scopes that are required when calling the graphql API with a token
based authentication. When this directive is present on a field,
query, mutation or type, it is required that the token has scopes
that are listed. The beauty of this approach is, that we can add
different scopes to different queries/fields/mutations and single source
of truth is our graphql schema.

- unit tests
- failing authorization on queries/mutations/fields without the directive
- adding scopes to all needed graphql entities
- UI changes to create tokens with more scopes
- making backwards incompatible changes
- authorizing internal API access

Tested locally.

To test locally, you need to modify `go.mod` to point to your own
local fork of graph-gophers/graphql-go#446 It is also needed to apply
the patch suggested in this comment: https://github.com/graph-gophers/graphql-go/pull/446/files#r914374506

To create a token with different scopes, create a normal token as you would
usually by going to Settings -> Access tokens. You need to modify
the scopes directly in the database (yikes!). Search the `schema.graphql` file
for `@authz` directive scope definitions that are required with these changes.

You then need to use the token directly with curl or similar.

When using the token without proper scopes, you should see graphql errors
instead of data.
rudle pushed a commit to rudle/graphql-go that referenced this pull request Dec 22, 2022
rudle pushed a commit to rudle/graphql-go that referenced this pull request Dec 22, 2022
rudle pushed a commit to rudle/graphql-go that referenced this pull request Dec 22, 2022
@pavelnikolov
Copy link
Member

I am closing this PR in favor of #543

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