Added ability to use your own directives usage

[eko]

Hi,

Description

This PR goal is to bring ability to use so custom directives, as mentioned in issue https://github.com/graph-gophers/graphql-go/issues/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!

[pavelnikolov]

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 #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]

@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 #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 https://github.com/graph-gophers/graphql-go/pull/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]

@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 ;)

[pavelnikolov]

@eko I really like the visitor pattern!

[pavelnikolov]

@eko #437 has been merged

[eko]

@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]

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

[eko]

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

[pavelnikolov]

@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]

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]

@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]

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]

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]

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

[pavelnikolov]

I am closing this PR in favor of https://github.com/graph-gophers/graphql-go/pull/543