Closed rudle closed 1 year ago
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.
@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.
@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?
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.
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.
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.
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.
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:
@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.~listScalar(limit: Int): String @complexity(value: 2, multipliers: ["limit"])
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.
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 https://github.com/graph-gophers/graphql-go/issues/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.
@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:
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.traceCtx
and leave it to the user to start a span inside of these methods."data": null
in my result. But this is the correct behavior according to the spec so no problems there.Before
and After
methods? I would like to be able to get the field @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.
I just pushed a commit which passes the traceCtx
to the directive visitors. The users can opt-in to use that.
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
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.
based on graph-gophers/graphql-go#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.