Add support for field directives #860

sforward · 2021-11-24T11:57:27Z

Summary

This PR starts where #703 was left off. It is a non-breaking implementation of the field directive, by overriding the default field resolver.

Locations
There are two types of directive locations: type system (or schema) directive locations and executable directive locations. The type system directives are not supported by webonyx/graphql-php, see here. This isn't a real problem though, since similar functionality can be achieved via the types, queries, middleware, etc of this package. I have added a warning for this in the readme.

I decided to make a PR for the field location only, since the other locations will act on different parts of the query execution.

Built-in directives
The include and skip directives already worked, so I added these to the readme as well.

Type of change:

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to not work as expected)
This change requires a documentation update
Misc. change (internal, infrastructure, maintenance, etc.)

Checklist:

Existing tests have been adapted and/or new tests have been added
Add a CHANGELOG.md entry
Update the README.md
Code style has been fixed via composer fix-style

illambo · 2021-11-24T12:49:25Z

Hi! it is a pleasure that someone has had the desire to pick up the thing again.
Has nothing else been introduced compared to the old PR?

sforward · 2021-11-24T14:41:34Z

The differences with the previous PR are:

Directives work with arguments
Internal directives are still available if you have custom directives registered
Definition of the custom directive classes are made similar to Type classes (with $attributes and args method)
The logic needed to apply the directives is separated from the field resolver code

@illambo I hope this answers your question!

illambo · 2021-11-24T14:45:52Z

Yes! thanks for the quick reply and for continuing development in this.

mfn · 2021-12-16T18:49:27Z

Just a quick no-update update: I've not forgotten, still on my list, but it's a "bad time of the year" timing and it's not that I just have to review handful lines of code. Thanks for you patience 🙏🏼

mfn

Wow, really nice PR! This is a preliminary review until I better understand it, please see my feedback below and inline!

I was able to follow some of the parts but not yet all of it.

I'm surprised to understand (or: maybe I got it wrong) that the graphql-php default field resolver is responsible for resoling the field directives. I would have though this would be built-in somehow.
Because if I got this right, this would mean directive support depends on the default field resolver and if a custom one is provided and does not include the support for it, it won't work.

Can you explain this in more detail?

Also, do you have an idea what it would need to support other kind of directives?

Thank you!

Please import all FQCN (I saw it multiple times but only gave feedback in one place)
I noticed that the FieldResolver and DirectiveHandler are not registered in the service provider. This means if running under a different architecture, like in Laravel Octane via e.r. roadrunner, we create multiple instances but by and large, they never really change?
TL;DR: my question here is: can they be made singletons?

mfn · 2021-12-20T07:15:45Z

README.md

@@ -2521,6 +2522,96 @@ class AppServiceProvider extends ServiceProvider

 The `macro` function accepts a name as its first argument, and a `Closure` as its second.

+### Directives


Thanks for the detailed explanation here in the README!

However I see that in the three parts of the example how to add a custom directive, you used three different functionalities and none completes the other: CapitalizeDirective::class, TrimDirective and finally @upper in the query example.

Is there a reason for this or can we harmonize this, so the example is one working thing?

src/GraphQL.php

mfn · 2021-12-20T07:23:23Z

src/Support/Directive.php

+declare(strict_types = 1);
+namespace Rebing\GraphQL\Support;
+
+abstract class Directive extends \GraphQL\Type\Definition\Directive


Please import:

Suggested change

abstract class Directive extends \GraphQL\Type\Definition\Directive

abstract class Directive extends BaseDirective

mfn · 2021-12-20T07:25:17Z

src/Support/Directive.php

+    /**
+     * Specify the locations where this directive can be applied.
+     *
+     * @return array<string>


I think this would be a good place to document which one we exactly support, in a way static analyzers can also make use if it.

Can we express (only) the supported constants from \GraphQL\Language\DirectiveLocation here?

mfn · 2021-12-20T07:32:05Z

tests/Support/Directives/LowerCaseDirective.php

+    public function handle($value, array $args = []): ?string
+    {
+        if (\is_string($value)) {
+            return strtolower($value);


mb_strtolower?

mfn · 2021-12-20T07:32:45Z

tests/Support/Directives/TrimDirective.php

+        return [
+            'chars' => [
+                'type' => Type::string(),
+                'description' => 'Trim field by given characters.',


It would probably make sense to document the defaults of the trim function here, too

mfn · 2021-12-20T07:38:48Z

src/Support/ExecutionMiddleware/GraphqlExecutionMiddleware.php

+
+    private function getDefaultFieldResolver(Schema $schema): callable
+    {
+        return $this->config->get('graphql.defaultFieldResolver') ?? app(FieldResolver::class)->setSchema($schema);


How does the directive support work in case a custom field resolver is provided?

I get the impression the "location \GraphQL\Language\DirectiveLocation::FIELD" support depends on the field resolver doing the actual work of applying the directives (looking at \Rebing\GraphQL\Support\FieldResolver\FieldResolver::__invoke), which means a custom field resolver would disable this?

mfn · 2021-12-20T07:39:24Z

src/Support/FieldResolver/DirectiveHandler.php

+    public function getDirective(string $name): ?Directive
+    {
+        return $this->directives[$name] ?? null;
+    }


Why would it be a good idea to silence a directive which isn't present? Shouldn't we throw an exception instead?

With the designed usage you can never run into the situation that the directive isn't present, because the query validation will be executed before. But since the method is public I added it to prevent the "undefined index" exception from being thrown, and anyone can decide how to handle the null value in its application.

mfn · 2021-12-20T07:47:02Z

src/Support/FieldResolver/FieldResolver.php

+use GraphQL\Type\Definition\ResolveInfo;
+use GraphQL\Type\Schema;
+
+class FieldResolver extends Executor


I don't like the design we're extending the Executor just so we can call self::defaultFieldResolver.

This seems rather "heavy" and also misleading, as the actual Executor being invoked is (seems to me) unrelated to this resolver implementation which feels confusing.

I think just calling \GraphQL\Executor\Executor::defaultFieldResolver directly would make more sense. If we're concerned about extending/customizing the FieldResolver itself, we could always make a protected function callDefaultFieldResolver so the user can override it.

This actually brings me back to my other question from \Rebing\GraphQL\Support\ExecutionMiddleware\GraphqlExecutionMiddleware::getDefaultFieldResolver:

Wouldn't it make sense to actually invoke any custom field resolver (from config->get('graphql.defaultFieldResolver')) in __invoke here, if present, and otherwise call Executor::defaultFieldResolver?

Maybe not though, might be confusing: if the custom field resolver is responsible itself for applying the directives, what if someone wants to provide their own implementation for this. This would require adapting the FieldResolver class and not the defaultFieldResolver

mfn · 2021-12-20T07:50:08Z

src/Support/FieldResolver/FieldResolver.php

+        $this->directiveHandler = $directiveHandler;
+    }
+
+    public function setSchema(Schema $schema): self


The naming is kind of weird, as we're not "setting the schema" anywhere.

Co-authored-by: Markus Podar <markus@fischer.name>

sforward · 2022-01-04T20:48:53Z

I'm surprised to understand (or: maybe I got it wrong) that the graphql-php default field resolver is responsible for resoling the field directives. I would have though this would be built-in somehow.
Because if I got this right, this would mean directive support depends on the default field resolver and if a custom one is provided and does not include the support for it, it won't work.

While working on this, and digging into webonyx/graphl-php, I was surprised as well. The built-in directives skip and include are hard-coded in graphql-php, by leaving out fields before they are resolved. No other directive support is available, and a related issue is open for a long time. The field resolver is the suggested approach according their documentation. That's the reason that the possible directive locations are limited currently.

If someone defines a custom field resolver, the directive support will indeed no longer be available. I will add a warning for this in de readme, if you want to continue with this PR. I can understand if you see too many objections.

mfn · 2022-01-30T20:23:10Z

If someone defines a custom field resolver, the directive support will indeed no longer be available.

If I understand it correctly, it would boil down to having to delegate only one call to your architecture to support them as I see in

graphql-laravel/src/Support/FieldResolver/FieldResolver.php

Line 38 in 07056b6

return $this->directiveHandler->applyDirectives($property, $info);

This would be acceptable to me although I'm not yet sure how a final solution regarding the default field resolver should work:

as it does right now: shift responsibility to dev if they use a custom resolver, but then we should make integration of directives easier
always wrap it as part of this framework and callback to the user-defined one, so directives are always applied / controllable via the framework

if you want to continue with this PR. I can understand if you see too many objections.

Quite some time has passed and even I had to re-read all my feedback to get the picture again. My gut feeling is that we might need some more iteration(s) to nail it, but otherwise
I would gladly have this PR and have its support in the framework.

mfn · 2022-05-07T18:56:07Z

@sforward are you still interested in continuing this PR?

sforward · 2022-06-10T12:46:30Z

Sorry it's been a while. The linked issue at webonyx/graphql-php was closed yesterday, so I don't expect we can achieve full support for directives here. I will close the PR.

sforward added 2 commits November 23, 2021 22:29

Add support for field directives

f47e7ce

Add feature to changelog

2416c2e

sforward added 2 commits November 25, 2021 08:36

Add more docblocks to make phpstan happy

07056b6

Cs

d2df1ac

mfn self-requested a review December 20, 2021 06:28

mfn self-assigned this Dec 20, 2021

mfn reviewed Dec 20, 2021

View reviewed changes

Update src/GraphQL.php

8a865b5

Co-authored-by: Markus Podar <markus@fischer.name>

mfn added the waiting:feedback label May 7, 2022

sforward closed this Jun 10, 2022

mfn removed the waiting:feedback label Jun 11, 2022

mfn mentioned this pull request Sep 26, 2022

Register directives via schema config #947

Merged

9 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for field directives #860

Add support for field directives #860

sforward commented Nov 24, 2021 •

edited

illambo commented Nov 24, 2021

sforward commented Nov 24, 2021

illambo commented Nov 24, 2021

mfn commented Dec 16, 2021

mfn left a comment

mfn Dec 20, 2021

mfn Dec 20, 2021

mfn Dec 20, 2021

mfn Dec 20, 2021

mfn Dec 20, 2021

mfn Dec 20, 2021

mfn Dec 20, 2021

sforward Jan 4, 2022

mfn Dec 20, 2021

mfn Dec 20, 2021

sforward commented Jan 4, 2022

mfn commented Jan 30, 2022

mfn commented May 7, 2022

sforward commented Jun 10, 2022

		@@ -2521,6 +2522,96 @@ class AppServiceProvider extends ServiceProvider

		The `macro` function accepts a name as its first argument, and a `Closure` as its second.

		### Directives

	abstract class Directive extends \GraphQL\Type\Definition\Directive
	abstract class Directive extends BaseDirective

Add support for field directives #860

Add support for field directives #860

Conversation

sforward commented Nov 24, 2021 • edited

Summary

illambo commented Nov 24, 2021

sforward commented Nov 24, 2021

illambo commented Nov 24, 2021

mfn commented Dec 16, 2021

mfn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sforward commented Jan 4, 2022

mfn commented Jan 30, 2022

mfn commented May 7, 2022

sforward commented Jun 10, 2022

sforward commented Nov 24, 2021 •

edited