Added ability to use your own directives usage #446

eko · 2021-04-08T15:53:53Z

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!

pavelnikolov · 2021-04-08T21:49:30Z

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

Does your implementation support only directives on FIELD_DEFINITIONs? Do you support directives on objects, fragments and inline fragments?
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.
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 · 2021-04-09T08:13:46Z

@pavelnikolov,

Thanks for your quick & clear reply!

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?
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.
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 · 2021-04-12T12:05:36Z

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

ldechoux · 2021-04-12T16:42:51Z

pkg/common/directive.go

@@ -1,5 +1,10 @@
 package common

+type DirectiveVisitor interface {


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 ?

You're right, I just added the context

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

pavelnikolov · 2021-04-13T08:42:21Z

@eko I really like the visitor pattern!

pavelnikolov · 2021-04-23T11:12:00Z

@eko #437 has been merged

eko · 2021-04-23T11:35:12Z

@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 · 2021-12-16T15:57:10Z

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

eko · 2021-12-28T13:01:46Z

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

pavelnikolov · 2022-01-06T17:57:23Z

@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 · 2022-01-07T21:38:42Z

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 · 2022-01-08T10:18:06Z

@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 · 2022-01-10T07:49:32Z

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 · 2022-01-10T12:14:39Z

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 · 2022-01-10T13:28:27Z

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 · 2022-01-11T08:24:20Z

internal/exec/exec.go

+						}
+					}
+				}
+			}


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

I'm talking about lines 263 to 269

fajrifernanda · 2022-07-06T03:09:05Z

internal/exec/exec.go

+							values = append(values, inValue.Interface())
+						}
+
+						if visitorErr := visitor.Before(ctx, directive, values); err != nil {


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

fajrifernanda · 2022-07-06T03:09:37Z

internal/exec/exec.go

+				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 {


this should be if visitorErr != nil

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.

@eko

based on graph-gophers#446 and work by @eko

@eko

based on graph-gophers#446 and work by @eko

@eko

based on graph-gophers#446 and work by @eko

pavelnikolov · 2022-12-22T23:43:52Z

I am closing this PR in favor of #543

eko force-pushed the custom-directives branch from 5f38024 to 3e2729d Compare April 8, 2021 16:00

eko changed the title ~~Added support of custom directives~~ Added ability to use your own directives usage Apr 8, 2021

eko force-pushed the custom-directives branch from 3e2729d to 26d6863 Compare April 8, 2021 19:25

eko force-pushed the custom-directives branch from 26d6863 to fafbdd2 Compare April 12, 2021 11:54

eko force-pushed the custom-directives branch 3 times, most recently from 4a97316 to ada73c5 Compare April 12, 2021 13:14

ldechoux reviewed Apr 12, 2021

View reviewed changes

eko force-pushed the custom-directives branch from ada73c5 to 4de6df2 Compare April 12, 2021 18:22

eko force-pushed the custom-directives branch from 4de6df2 to 5f8e662 Compare April 23, 2021 11:33

eko force-pushed the custom-directives branch from 5f8e662 to a030af0 Compare December 28, 2021 13:00

pavelnikolov reviewed Jan 6, 2022

View reviewed changes

graphql_test.go Outdated Show resolved Hide resolved

eko force-pushed the custom-directives branch from a030af0 to 8eb238d Compare January 7, 2022 21:27

Added support of custom directives

e64165c

eko force-pushed the custom-directives branch from 8eb238d to e64165c Compare January 7, 2022 21:40

pavelnikolov reviewed Jan 11, 2022

View reviewed changes

pavelnikolov mentioned this pull request Mar 17, 2022

Apollo Federation Spec: Fetch service capabilities #507

Merged

fajrifernanda suggested changes Jul 6, 2022

View reviewed changes

kopancek mentioned this pull request Sep 29, 2022

[DO NOT MERGE][feat] Add access token scopes authorization based on graphql directives sourcegraph/sourcegraph-public-snapshot#42284

Closed

rudle pushed a commit to rudle/graphql-go that referenced this pull request Dec 22, 2022

Added support of custom directives

856e336

based on graph-gophers#446 and work by @eko

rudle pushed a commit to rudle/graphql-go that referenced this pull request Dec 22, 2022

Added support of custom directives

c2aa41b

based on graph-gophers#446 and work by @eko

rudle pushed a commit to rudle/graphql-go that referenced this pull request Dec 22, 2022

Added support of custom directives

5e217c2

based on graph-gophers#446 and work by @eko

rudle mentioned this pull request Dec 22, 2022

Add Custom Directive Support for Fields #543

Merged

pavelnikolov closed this Dec 22, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added ability to use your own directives usage #446

Added ability to use your own directives usage #446

eko commented Apr 8, 2021 •

edited

Loading

pavelnikolov commented Apr 8, 2021

eko commented Apr 9, 2021 •

edited

Loading

eko commented Apr 12, 2021

ldechoux Apr 12, 2021

eko Apr 12, 2021

pavelnikolov Apr 13, 2021

pavelnikolov commented Apr 13, 2021

pavelnikolov commented Apr 23, 2021

eko commented Apr 23, 2021 •

edited

Loading

pavelnikolov commented Dec 16, 2021

eko commented Dec 28, 2021

pavelnikolov commented Jan 6, 2022

eko commented Jan 7, 2022

pavelnikolov commented Jan 8, 2022

eko commented Jan 10, 2022

pavelnikolov commented Jan 10, 2022 •

edited

Loading

pavelnikolov commented Jan 10, 2022

pavelnikolov Jan 11, 2022

pavelnikolov Jan 11, 2022 •

edited

Loading

fajrifernanda Jul 6, 2022

fajrifernanda Jul 6, 2022

pavelnikolov commented Dec 22, 2022

		@@ -1,5 +1,10 @@
		package common

		type DirectiveVisitor interface {

Added ability to use your own directives usage #446

Added ability to use your own directives usage #446

Conversation

eko commented Apr 8, 2021 • edited Loading

Description

Example usage

pavelnikolov commented Apr 8, 2021

eko commented Apr 9, 2021 • edited Loading

eko commented Apr 12, 2021

ldechoux Apr 12, 2021

Choose a reason for hiding this comment

eko Apr 12, 2021

Choose a reason for hiding this comment

pavelnikolov Apr 13, 2021

Choose a reason for hiding this comment

pavelnikolov commented Apr 13, 2021

pavelnikolov commented Apr 23, 2021

eko commented Apr 23, 2021 • edited Loading

pavelnikolov commented Dec 16, 2021

eko commented Dec 28, 2021

pavelnikolov commented Jan 6, 2022

eko commented Jan 7, 2022

pavelnikolov commented Jan 8, 2022

eko commented Jan 10, 2022

pavelnikolov commented Jan 10, 2022 • edited Loading

pavelnikolov commented Jan 10, 2022

pavelnikolov Jan 11, 2022

Choose a reason for hiding this comment

pavelnikolov Jan 11, 2022 • edited Loading

Choose a reason for hiding this comment

fajrifernanda Jul 6, 2022

Choose a reason for hiding this comment

fajrifernanda Jul 6, 2022

Choose a reason for hiding this comment

pavelnikolov commented Dec 22, 2022

eko commented Apr 8, 2021 •

edited

Loading

eko commented Apr 9, 2021 •

edited

Loading

eko commented Apr 23, 2021 •

edited

Loading

pavelnikolov commented Jan 10, 2022 •

edited

Loading

pavelnikolov Jan 11, 2022 •

edited

Loading