[RFC] Remove/rework config processors

[murtukov]

Q	A
Bug report?	no
Feature request?	no
BC Break report?	no
RFC?	yes
Version/Branch	1.0 (master)

Whenever you make a request to your Symfony app with GraphQL Bundle installed, it instantiates all of your GraphQL types. To reduce the performance impact, webonyx/graphql-php library made possible to use closures almost at any part of your type config to make them lazy loaded.

This bundle adds so called "config processors" to each of the generated GraphQL types, which modify configs during the instantiating of the classes.

The problem here is, that configs wrapped into closures (for lazy loading) cannot be accessed, unless you call them. And this is exactly what config processor does in order to access and modify configs. This kinda defeats the purpose of wrapping configs into closures. Secondly it makes double work for no particular reason (unwrap configs, modify it and wrap back into a closure).

Every Config Processor wraps the fields closure into another closure to modify it's behaviour.

Currently we have 3 config processors

• AclConfigProcessor
• PublicFieldsFilterConfigProcessor
• WrapArgumentConfigProcessor

That means that after all processors are executed, the fields array is wrapped into 4 closures.

This RFC tries to find out a better way to do tasks these processors perform.

Let's go through all processors.

1. WrapArgumentConfigProcessor

This processors finds all resolve and resolveFiled callbacks in the config array and wraps them into another callback to only modify the $args param. Here is an example of a field:

Before processing:

'myField' => [
    'type' => /* ... */,
    'resolve' => function ($value, $args, $context, $info) use ($globalVariables) {
        return $globalVariables->get('mutationResolver')->resolve(["no_validation", []]);
    },
    'args' => /* ... */,
],

After processing your resolver callback changes:

'myField' => [
    'type' => /* ... */,
    'resolve' => function () {
        $args = func_get_args();
        if (isset($args[1]) && !$args[1] instanceof ArgumentInterface) {
            $args[1] = $argumentFactory->create($args[1]);
        }
        return (function ($value, $args, $context, $info) use ($globalVariables) {
            return $globalVariables->get('mutationResolver')->resolve(["no_validation", []]);
        })(...$args);
    },
    'args' => /* ... */,
],

Wrapping of the resolve happens in runtime.

Possible solution:

Instead of wrapping the resolver, we can use the ArgumentFactory directly in the generated resolver callback. Example:

'myField' => [
    'type' => /* ... */,
    'resolve' => function ($value, $args, $context, $info) use ($globalVariables) {
        $args = $globalVariables->wrapArguments($args);
        return $globalVariables->get('mutationResolver')->resolve(["no_validation", []]);
    },
    'args' => /* ... */,
],

Fortunately, with the new generator library it's easy to change the content of the generated callbacks. With this we can remove the WrapArgumentConfigProcessor.

2. AclConfigProcessor

The closure created by this processor finds the access callback and executes it. If access returns false, the resolve callback is replaced with another one, that throws an exception. Example:
Before processing:

'myField' => [
    'resolve' => function ($value, $args, $context, $info) use ($globalVariables) {
        return $globalVariables->get('mutationResolver')->resolve(["no_validation", []]);
    },
],

After processing (if access returned false):

'myField' => [
    'resolve' => function() {
        throw new UserWarning('Access denied to this field.');
    }},
],

Possible solution:

Similar to the first one. Instead of replacing the resolve callback, we can generate the access check call in the actual resolver. Example:

'myField' => [
    'resolve' => function ($value, $args, $context, $info) use ($globalVariables) {
        $this->checkAccess('myField'); // this line throws `UserWarning` on denied
        return $globalVariables->get('mutationResolver')->resolve(["no_validation", []]);
    },
],

3. PublicFieldsFilterConfigProcessor

Closure created by this processor finds the public callback of the field config and executes it. If result is false the given field is just removed from the config array. This processor must be on the deepest level of closure nesting in order to be able to access the fields array. Otherwise the array will be wrapped into closures created by other processors.
I don't have any solution to this right now.

Why do we need this?

First and most important it should reduce the initialization and request time:

• No unwrapping/wrapping resolvers work, that partly destroyes the lazy loading.
• No additional callback calls.

Secondly it would make generated types less "magical", because users could see the actual type's code in one place.

[Kocal]

Hi,

That's would be a very nice improvments, thanks for this 👍

Just for information, will you be able to make Blackfire traces to see perf diffs when these RFC will be implemented?

Thanks!

[mcg-web]

The described behavior does not seem to reflect the actual behavior.

The problem here is, that configs wrapped into closures (for lazy loading) cannot be accessed, unless you call them. And this is exactly what config processor does in order to access and modify configs. This kinda defeats the purpose of wrapping configs into closures. Secondly it makes double work for no particular reason (unwrap configs, modify it and wrap back into a closure).

This is not true we only wrap the existing closure and configProcessor behavior are process on runtime ( on type load ) so the lazy behavior is keep ( PublicFieldsFilterConfigProcessor, AclConfigProcessor, WrapArgumentConfigProcessor, ArgumentFactory ).

[murtukov]

@mcg-web Yes you are right, it only wraps existing closures into another closure, so it doesn't destroy the lazy loading. My bad, I updated the description. All processors work like I described the WrapArgumentConfigProcessor. But then we still have problems:

1. All fields are wrapped in 4 closures, something like:

   'fields' => function() { // WrapArgumentConfigProcessor
       function() { // AclConfigProcessor
           function() { // PublicFieldsFilterConfigProcessor
               fn() =>[
                   # ...
               ],
           }
       }
   }

   Additionally AclConfigProcessor wraps the resolve closure into another closure.

   The order of processors is also important: the PublicFieldsFilterConfigProcessor must be in the deepest level to be able to
   access and modify the fields array. I can see here in services, that you set the priority to the service definitions.

   I don't know how this affects the performance. Anyways I will do Blackfire traces and Apache Benchmark tests.

2. The second reason to change this behavior still remains: it would make generated types less "magical", because users could see the actual type's code in one place.

[mcg-web]

I agree ConfigProcessor is not perfect but it was designed to permit to add some behavior for all GraphQL types not only generated types. Generating all these behaviors was already the way of doing in v0.10 ( Access, WrapArgument, PublicFieldsFilter ). So this design is not new to me but was abandoned because it remove the ability to use the same code to deal with all the stack. Since the initial need of code generation was to deal with expression language and this will be optional with #708, I'm not totally sure that we will keep this in place, replacing it totally by Symfony DI but we will deal with this part later.