Add Custom Directive Support for Fields

[rudle]

based on #446 and work by @eko

I intend to revive the stalled conversation from #446 with this PR. I addressed the open comments on that change.

[rudle]

I added a commit that addresses your comment. I will keep my changes in a separate commit and then we can squash the whole thing down when we are ready merge.

[rudle]

I don't think that struct resolver fields can ever produce an error but I'm not an expert. Shall we delete this, Pavel?

Suggested change

	if !f.field.HasError && len(f.field.Directives) > 0 {
	if len(f.field.Directives) > 0 {

[pavelnikolov]

Yes, struct fields cannot produce an error. It doesn't make sense to me to check for one.

[rudle]

@pavelnikolov thanks for the review feedback so far. I have some questions about how we should handle errors from directive visitors. Wondering if you've thought this through before.

[pavelnikolov]

@rudle I think that it is fine to stop execution if we have an error in the Before method of a directive. It makes sense to prevent further execution. We can also make it configurable.
On a separate note, I was thinking that we should make method resolver execution configurable based on logic in the Before directive method. Imagine a @cache directive which would cache fields for a certain amount of time. And if the value is present in the cache the code should not produce a query to the backend storage. Before merging we should think of practical examples, like @auth(hasRole="editor"), @cache(for="10m") (such a caching directive would be different to the commonly used @cacheControl(maxAge="300", scope="PUBLIC") which is used to set cache-control headers on the response - if something is found in the cache we don't hit the DB). Does that make sense or is it too much to think about for now?

[rudle]

Does that make sense or is it too much to think about for now?

I'm not opposed to adding on to this PR nor am I in a rush to have it merge it, but I don't have the time to do the requirements gathering you're describing here.

What I will do as a next action is experiment with different options for how we can pass state between Before(), the Method Resolver and After() and report back here. I said above that the context.Context may be our only choice, but I'd like to convince myself of that.

[pavelnikolov]

What I will do as a next action is experiment with different options for how we can pass state between Before(), the Method Resolver and After() and report back here. I said above that the context.Context may be our only choice, but I'd like to convince myself of that.

Maybe we can have a 3rd method which returns bool value indicating whether we need to execute the resolver method or no. Because obviously the directive is a struct and the Before and After methods can shared state either through the context or using struct fields. However, the library needs to agree on some special method, which indicates whether we need to execute the method resolver between the Before and After 🤔
Tomorrow I'll checkout this branch and try to implement some commonly used directives and will give feedback.

[rudle]

OK. A 3rd return value from Before makes sense to me. I wonder if it needs to be a whole method? I guess that's more flexible, but at the cost of a broader API.

I just did a quick code sketch of a directive implements server-side caching via a struct field set in Before and a return of that value as the result of resolution in After and it felt pretty good to me.

Earlier today, I tried to find a nice way for Before to pass data into a resolver, but I didn't like any of the options I came up with. I hope we can avoid this requirement.

[rudle]

above commit adds a new test case that demonstrates how to implement server-side caching. It takes the approach of adding a second return value to Before(). I'm open to adding a 3rd method to the Visitor interface also.

[pavelnikolov]

Thank you @rudle! Happy New Year! I like the 3rd return value! This makes sense to me and looks nice.
I was looking at the connections specification and this example looks interesting to me. Imagine this query:

{
  user {
    id
    name
    friends(first: 10, after: "opaqueCursor") {
      edges {
        cursor
        node {
          id
          name
        }
      }
      pageInfo {
        hasNextPage
      }
    }
  }
}

So imagine we want to cache the friends of a user and when we open their profile get the first 10 friends from cache. The schema declaration would look like this:

    ...
    friends(first: Int = 10, after: ID) @cache(ttl: Int = 300, key: String) FriendsConnection!
    ...

The interesting thing there is the key. This would be our cache key. Since we have many users and the directive is applied to each of them, how do we know which user's friends are we picking up from cache? We need to cache based on the user ID, right? So our "key" would be something like parent.id or something similar. I'm not sure how to resolve that problem. I found this directive and it seems that they accept parent, args and vars cacheKeys prefixes, as in:

parent.<field-name>
args.<argument-name>
vars.<variable-name>

see their "understanding cacheKey argument section".
So my final issue before merging this PR is: do we need to give the directive access to the parent object and query context (like arguments and variables). The field (method) arguments should be accessible I think, however, that is not enough since often times we should cache based on the parent ID. Chances are we have that ID in the Before method already. However, in order to create a fully field agnostic directive, we need to work with any type of parent ID/key. Do you think this is important?

Other than that, I am happy with this PR. I have few other thoughts:

• Maybe I should state that the directives API is not stable yet. This would give us an option to iterate on it before we call it final? Thoughts?
  * I was thinking about checking out what's needed for something like Apollo Federation (e.g. @external and @override). This relies heavily on custom directives. I might take a look. I don't mean fully implementing it. I mean just checking if the current implementation would support such a use-case. Ideally, I don't want to bake such features in the library with special directives that we treat in some different way. It would be nice if the library would allow users to extend it and build anything they want.
• Another directive to consider is a sample query complexity directive. Such a directive would need access to the field args, for example:

listScalar(limit: Int): String @complexity(value: 2, multipliers: ["limit"])

[rudle]

Hello! Happy new year. Thanks for all the feedback so far. Excited that this is maybe getting close!

Maybe I should state that the directives API is not stable yet. This would give us an option to iterate on it before we call it final? Thoughts?

Definitely agree with this. I appreciate the legwork you have already done to find some specific directives to help guide the API. The graphql-go community will no doubt have more specific feedback for this initial implementation as they try it out on their own applications. I also suspect that once this PR lands, more small contributions and improvements from the community will quickly follow. We should plan to iterate on this API and document that things are unstable.

Another directive to consider is a sample query complexity directive. Such a directive would need access to the field args, for example:

This is supported in this PR, but in an untyped way. The 3rd argument to Before contains the traceCtx and also all args to the field. The input isn't typed, so the resolver would need to know the type of the field resolver method in order to use them. A less sophisticated approach would be: splatting them all together and calling .String(). Could the former approach be as easy as passing in res.Method(f.field.MethodIndex).Type or something like that?

Regarding the parent object

This makes some sense to me but it seems hard to support. I don't know of any prior art for this type of thing within graphql-go. It seems to me that each field resolves in isolation of other fields and that walking up and down the graph isn't supported. It seems a bit related to this very old issue #17.

[pavelnikolov]

This makes some sense to me but it seems hard to support. I don't know of any prior art for this type of thing within graphql-go. It seems to me that each field resolves in isolation of other fields and that walking up and down the graph isn't supported. It seems a bit related to this very old issue #17.

Indeed it is similar issue, however, I can imagine people opening issues about it. The selected fields issue is easy to solve, but it is difficult to implement in a type-safe way without changing the existing resolver API. I don't want selected fields extraction to happen through the context if possible. Therefore, this would require some other argument to the resolvers containing information about the GraphQL request such as the selected fields and maybe some other info in future.

[pavelnikolov]

It would be cool if this key can come from the resolved entity's ID somehow. In a real-world situation we would need to use the ID of that entity or the ID of its parent, for example, the products of a category or the comments of a post. In both of these examples we need to cache by the ID of the parent entity. I wish we could somehow do that.

[pavelnikolov]

@rudle I added a few commits to your branch. I added an example @hasRole directive as one of the most popular examples from the community for declarative authorization. I love the custom directive feature overall but here are some topics which might need polishing:

• We pass ctx to the Before and After methods. Instead I believe we need to pass the traceCtx this way the activity of the directive visitors will be added to the trace graphs. However, in order to do this in a protocol-agnostic way (e.g. OTEL, OpenTelemetry etc.) we need to create some kind of directive tracer interface. Then check if the provided tracer implements the directive visitor interface and wrap the Before and After methods with a trace span.
• Alternatively, since we could just pass to them the traceCtx and leave it to the user to start a span inside of these methods.
• Another thing, that I just realized is that if your directive returns an error, then that error will bubble up the query to the nearest nullable field up the query hierarchy. Since my query had only non-nullable fields, when my directive was erroring out I was getting "data": null in my result. But this is the correct behavior according to the spec so no problems there.
• Is there an easy way to retrieve the field arguments inside the Before and After methods? I would like to be able to get the field
• Imagine that I am creating an @isOwner directive which is supposed to limit access to mutations in a multi-tenanted system to only owners of an entity (e.g. a project). So only if the currently logged in user is the owner of the current project, then mutations would be allowed. In order to check that I need to have access to the ID of the current entity. How would that happen? Maybe in the After method when I have the entity I can check if the user has access to it and decide what to do based on that, however, by this time, the resolver method has already been executed. How can check in the Before method so that the resolver never executes if the user is not the owner of the entity? Imagine this query:

  mutation {
      updatePost(id: ID, post: Post!): UpdatePostResult! @isAuthorOrAdmin
      ...
  }

  In the above example I need access to the id argument of the field inside of the Before method in order to achieve my goal.

I believe that this code is ready to be merged. I'll probably mention in the README.md file that the directives API is experimental and is potentially subject to change in future.

[pavelnikolov]

I just pushed a commit which passes the traceCtx to the directive visitors. The users can opt-in to use that.

[pavelnikolov]

I'm going to merge this PR the way it is. It already covers basic usage of field directives. I put a note in the readme that the directives API is potentially subject to change in future versions. You are correct that some of my ideas are outside of the scope of this PR. When in future it is possible to check selected fields in a resolver it will be introduced retrospectively into directives as well. Then potentially we will be able to access field arguments as well. That is why I stated that potentially the directives resolver API is subject to change.
Thank you @rudle

[rudle]

Thanks @pavelnikolov! I was just checking in on this PR and am happy to see that it's merged.

I'm excited for the next steps that will make this feature more powerful, especially checking for selected fields.

[pavelnikolov]

1. There is one thing that I'm not completely happy about in this PR and it's the arguments of the directives. I don't like the way we extract them now. I wish we could assign directly to a struct in a type-safe way similar to the way resolvers do it and also get validated during schema parsing. However, this would be tricky.
2. The selected fields in resolvers/directives/context is the next thing on my list. It has been the most requested feature. I've implemented it a few times already. I'm just going to start a new issue with requirements and try to make it user-friendly and without breaking changes. If you are interested - feedback/contributions would be more than appreciated.