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

Feature Request: Directive Visitors / Resolvers #299

Closed
zimzat opened this issue Jul 2, 2018 · 13 comments
Closed

Feature Request: Directive Visitors / Resolvers #299

zimzat opened this issue Jul 2, 2018 · 13 comments

Comments

@zimzat
Copy link
Contributor

zimzat commented Jul 2, 2018

There's currently a lack of examples showing how to check for directives in resolvers for each location, whether the Schema or Query type. There doesn't seem to be any way to hook into schema directives, such as during the build step, and query directives seem to require manually implementing into each defaultResolver/fieldResolver/resolve function with no way to easily chain functionality or creating a global resolver to intercept every other resolver first, which seems a bit cumbersome. Am I missing something?

It would be nice to have a way to implement directives as visitors, promises, or other helpers. Implementing @skip, @include, and @deprecated in this way would be a good first step and provide a guide for implementing custom directives.

For reference the Apollo Server Directive implementation uses visitors and chaining resolver promises.

If a schema visitor during the build process and query resolve chain for the execution process sounds reasonable then I could put some time into implementing those (e.g. like Apollo), but if you have another idea on how to implement those or don't feel like it should be part of the library I'm open to alternates.

For my current usage I'm trying to implement a Query / Mutation directive to set the account context to be used for that execution. I've considered putting in the URL (which would have to be done for multiple graph endpoints, public/admin/employee for every account, but might be arguably easier for the client), adding an argument to every relevant field / object, putting it in filter parameters (which could get messy / disconnected for child/sibling filters), or a secondary request Header (this might be the way I end up going until I can get a better way to handle it with directives).

I'd also like to consider using schema directives for excluding/including specific objects definitions or fields based on the endpoint used (merging public/admin/employee type language files), but currently that's not an option.

@vladar
Copy link
Member

vladar commented Jul 7, 2018

We follow graphql-js reference implementation which doesn't provide such tools at the moment. Following both Apollo tools and graphql-js is a bit too much for us in terms of maintenance.

Such extensions should be possible to implement separately, I encourage you to do so in a separate project. Will be happy to link to it in our complementary tools docs.

@vladar vladar closed this as completed Jul 7, 2018
@vladar
Copy link
Member

vladar commented Jul 12, 2018

Actually, we do miss the documentation on how to implement custom directives. I created a separate issue to track it: #308 which may be useful for you once complete.

@robertvansteen
Copy link

@vladar how would something like that be implemented? It seems like we need to override the Executor but I don't see any way to do that.

@vladar
Copy link
Member

vladar commented Aug 29, 2018

@rovansteen Please explain a bit more what you are trying to achieve, preferably with an example.

@robertvansteen
Copy link

@vladar I was trying to implement a custom directive, called @ability similar to @skip but rather than passing in a boolean you pass in a string which is internally used to check if the currently authenticated user has the ability (is allowed) to read that field. So the directive needs to drop the field from the request if the user is not allowed to read it.

I wanted to do that by extending the functionality of the shouldIncludeNode method on the Executor class but I did not find any way to override that class or method.

@vladar
Copy link
Member

vladar commented Aug 29, 2018

Yes, this type of custom directive is not an easy one at the moment, since it requires execution flow adjustment. There are various hacks, but no out-of-the-box solution yet. I believe the same applies to the reference implementation.

What hacks you can apply at the moment:

  1. Composing resolvers. The common outer resolver can read directives from $info (4th argument of the resolver) and return null conditionally.

But it requires the field to be marked as nullable. It may be actually better than stripping the whole field out (as skip and include do) because at the moment all GraphQL tools expect a value (or null) for a field if it was requested. And only @skip and @include directives are allowed by the spec to exclude fields. So custom directive which removes a part of the response may be confusing to other GraphQL tools.

  1. Pre-processing of the query AST. You can modify query AST and replace your directive with @include or @skip prior to execution. But if your logic requires the runtime value of the object (1st argument of the resolver) this method won't work, since it happens before the execution.

  2. Post-processing of response. With this method, the field is executed but stripped out later in another pass on the response data.

One problem with your approach is that directives should not break spec and the directive you describe will do so. So the solution described in my first example may be even more appropriate.

As for the ability to override parts of the execution flow... we will follow the reference implementation decisions here.

@nagledb
Copy link
Contributor

nagledb commented Nov 1, 2018

I'm very interested in using schema directives.

If I'm understanding the graphql-tools code right, it looks like Schema Visitor is applied on the schema prior to execution. So that approach wouldn't work with lazy-loaded types.

One possibility I'm thinking about is wrapping resolvers with code that checks for schema directives and applies them during execution. However, it seems like this approach would only work for field definition directives.

For other kinds of schema directives, the only way I can think of to get at them with lazy loading would be in the Executor. It looks like that would be pretty complicated and may involve a lot of duplication and modification to get the right changes in the right places, which doesn't sound ideal.

Am I wrong on anything above? Is there a better approach I'm not seeing?

@nagledb
Copy link
Contributor

nagledb commented Nov 2, 2018

Actually I just spent some time digging into BuildSchema::build's $typeConfigDecorator and it looks like it might be able to support handling all the schema directive locations (except for on the schema node itself, but I can't think of any use cases for that). Is that what its intended purpose is? I couldn't find any documentation for it.

@vladar
Copy link
Member

vladar commented Nov 2, 2018

Looks like documentation was deleted here. I missed this, but will restore it as soon as I have a moment. Or you can send a PR to restore it.

As for other options - you can try wrapping typeLoader and enhance all type's fields on load. This is still not ideal, but can work.

@diasfs
Copy link

diasfs commented Feb 12, 2019

An easy way to implement custom directive
https://packagist.org/packages/diasfs/graphql-php-resolvers

@sebastienbarre
Copy link

sebastienbarre commented Aug 13, 2020

@rovansteen @nagledb I'd be very interested (and thankful) to learn how you ended up resolving this issue. I definitely need to skip fields too based on complex permissions. Any field in our schema can be subject to a possible permission, so I don't see myself making all of them nullable, and in many cases the null value could be interpreted the wrong way. Thank you.

Looking at ReferenceExecutor it looked as if a private static $UNDEFINED is set to Utils::undefined(); (a singleton I believe), and used to skip fields internally, so I tried to return Utils::undefined(); from my own resolver as well, to no avail, I'm only getting errors such as "Return value of TimerType::resolveEmployee() must be an instance of Employee, instance of stdClass returned".

@robertvansteen
Copy link

We ended up passing “abilities” (Booleans indicating if the user can do or see certain things) down to the frontend which we could use to skip or include. We needed them there anyways to show or hide UI elements so for us that was the easiest implementation.

@lethak
Copy link

lethak commented Jan 27, 2021

This is a must have for me, and frankly this is one of graphql-js main selling point imho.

unfortunately @diasfs solution cannot work in my case since I have no default resolveField nor graphql string schema definition (all php objects)

I am looking for a low level solution to implement a directive visitor using current graphql-php visitor if possible.

Basically I want to be able to wrap fields and types resolver from a directive.

Is it possible to create a custom validation rule to do this ? how do I best access the type config from the visitor ?

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

No branches or pull requests

7 participants