Resolve function behaviour identical to Controller

[marvinrabe]

Is your feature request related to a problem? Please describe.

Currently the resolve function looks always like this:

public function resolve($rootValue, array $args, GraphQLContext $context, ResolveInfo $resolveInfo)
    {
        // ...
    }

Most of the time I don't need $rootValue, $context and $resolveInfo. I spend some time thinking about how to improve the resolve functions to feel more Laravel-ish.

Describe the solution you'd like

Resolve function should behave in the same way as controller methods do.

1. Instead of passing 4 attributes to the resolve function maybe create some kind of "Request" object. It might even implement some methods that are already available in the default HTTP Request class provided by Laravel. That way I don't have to write my own logic for simple things (filled, all, input, except, only, validate …).

2. Instead of always passing this Request object, maybe determine the attributes through reflection. So I can have resolve functions based on my needs. function resolve(Request $request) gives me the request object. resolve($id, $name) gives me only id and name from $args. And so on…

3. Resolve Eloquent Models based on reflection. If I have a id or some named attribute. Then I might use the resolve function this way: resolve(Flight $flight) (either id is defined or flight attribute is used as an id), resolve(Flight $flight, Passenger $passenger) (I have flight and passenger as attributes). That way I dont have to manually write $flight = Flight::find($args['id']); every time.

4. Service Injection. Instead of relying on the constructor for passing in Services from the IoC container, maybe resolve them automatically when they are an attribute of the function. In my opinion this does not serve any purpose than to have consistency with Http Controller. I am totally fine with passing services through the constructor.

When the resolve function become more like the controller methods I could imagine that I could reuse the same Controller/Resolver class for Http Controller and GraphQl purposes. Especially when the "Lighthouse" Request object is implemented more or less identical to the default Http Request.

Describe alternatives you've considered

I will probably create an abstract wrapper Resolver class in the next days. That way I can create resolvers in the style described above ~~(except 4.)~~, without relying on you to change the resolver logic. This might even become a part of Lighthouse if you wish. But then all resolvers have to extend this class if they wish to have the same flexibility. Note that default Controllers do not require any Class extensions or interface.

[marvinrabe]

This is the result I have come up with:

https://gist.github.com/marvinrabe/85f9ac19a052137fbcf7ee446b77878a

If you are interested I might prepare a Pull Request with Tests. Before I need to know how I should include it (automatically for every Resolver or - like it currently is - as an optional extension to resolver classes).

[olivernybroe]

I like the idea of changing it to a custom request object. I do not like the idea of resolving based on the names of the args. In Lighthouse we generally typehint as much as possible, so doing stuff based on the name of the arg seems like a bad principle.

I also agree with service injection on method, makes a lot of sense. I think the eloquent model reflection is a cool feature also, if we can find a simple and nice way to actually write it up.

A very small but cool change would be to make the request object Macroable.

[marvinrabe]

@olivernybroe

Nothing stops you from type hinting. And in my opinion this approach is better for this than using the $args array.

Currently it is:

resolve($rootValue, array $args, GraphQLContext $context, ResolveInfo $resolveInfo) {
  return $args['x'] + $args['y']
}

But this is much cleaner:

resolve(int $x, int $y): int { return $x + $y}

Maybe it is worth to also provide a 'Input' interface. So this:

input MyInput {
  foo: Int!
}

type Mutation {
  myAction(input: MyInput!): Int!
}

With this Input class:

class MyInput implements Input {
  public int $foo; // Possible in PHP 7.4
}

Can be resolved like this:

resolve(MyInput $input): int { return $input->foo; }

For the eloquent model reflection have a look at my provided Gist for a working example. It is inspired by the actual Route Substituting that is happening in a default Laravel Route.

[olivernybroe]

I get what you mean, still don't think we should do it by parameter name, only do it by parameter typehint.

Taking your example from above, I find this much cleaner

resolve(Request $request) {
    return $request->argument('x') + $request->argument('y');
}

I actually like the idea of having a php input class which corresponds to the input class in graphql. We could even create a command to automatically generate this class from the graphql. This will basically work like a FormRequest object.

I like the eloquent model reflection, it looks nice.

[marvinrabe]

@olivernybroe

But there is not always a clear mapping by only looking at the type hinting. Take this for an example:

input MyInput {
  foo: Int!
}

type Mutation {
  myAction(first: MyInput!, last: MyInput!): Int!
}

When resolving:

resolve(MyInput $first, MyInput $last) {
  return $first->foo + $last->foo;
}

Then the only way to resolve this is by looking at the parameter name as well.

Probably it's best to allow both. Both cases are already possible. The developer can decide which way he prefers.

resolve(Request $request) {
    return $request->argument('x') + $request->argument('y');
}

resolve(int $x, int $y): int { return $x + $y}

[olivernybroe]

I think we might have misunderstood each other. What I meant was that in the case of the mutation you showed there, it will generate the following class.

class MyActionRequest {
    public MyInput first;
    public MyInput last;
}

And then you would do

resolve(MyActionRequest $request) {
    return $request->first->foo + $request->last->foo;
}

[spawnia]

There is an issue with optional/nullable arguments when using classes to represent arguments/input types. I see no way to discern an argument that is not given vs. an explicit null.

>>> $foo = new class { public $argThatIsNotPassed; }
=> @anonymous {#2327                                
     +argThatIsNotPassed: null,                     
   }                                                
>>> $foo->argThatIsNotPassed                        
=> null                                             

I think we have to stick with some flavour of dynamic property definitions, either an array or a class with ArrayAccess or magic properties. Especially since we have directives like @spread that mess with the defined structure of input arguments.

[spawnia]

I looked at the gist. Generally i like the idea, it takes a proven concept from Laravel and transfers it to Lighthouse - nice!

We should really find another class name than Request since that already has a well-defined meaning within Laravel/PHP. Frequently, you might need access to the actual HTTP request, which would clash and require aliasing. How about Operation, Field, Query or possibly ResolverArguments?

Given we want to add this without breaking everyone's existing resolvers, let's consider our options that enable the user to opt-in to this functionality.

I think the call-site should be responsible, so i would not make it a part of the schema definition/a directive. An abstract class might add useful helpers, but would prevent the user from extending their own base class.

How about we use an interface (without any methods) to mark a class that contains the new style resolvers? Or should we even do something on a per-method basis, like an annotation?

[olivernybroe]

I think we should take this one up again.

Looking at my files related to Lighthouse, resolvers are the ones that looks the worst tbh.

I think calling it Query would be the best, as that name will be readable and still make sense.

In regards to making this non-breaking, I think we should do solve "magic" here. So if the resolver method takes one parameter and that parameter is typehinted with Query we supply that in. In next major version, we should deprecate the old method of doing it.

[spawnia]

Terminology is not the most important part of the functionality, but will be hard to change later if we get it wrong. Let's make sure the name fits in with the established naming of GraphQL.

As a quick outline of http://spec.graphql.org/, some of the proposed terms fall within this hierarchical structure:

• Request is the topmost thing. Composed of a GraphQL Document, which may contain many Operation
• Operation is a named section of a GraphQL document, one must be specified for execution
• Query is a specific kind of Operation, alternative to Mutation and Subscription, may contain many Field
• Field is the smallest unit in a Request and is backed by a Resolver
• Resolver is a function that produces a value for a Field

Since we are implementing a data structure that is passed to resolvers, which are responsible for executing a single field, we certainly should not name it Query or anything higher up in the hierarchy.

So having thought this over for almost two years now, i revise my previous statement and now propose something akin to FieldArguments, ResolverArguments, perhaps a shortened ResolverArgs.

[olivernybroe]

Hmm then I vote for ResolverArgs.

What about just calling it Resolver though?

[spawnia]

What about just calling it Resolver though?

That would make Resolver have a weird double meaning, being a function and the thing that is passed to the function.

[olivernybroe]

Hmm, right. I just like the idea of it being a one Word class as it looks cleaner. But I think we should just go with ResolverArgs then 👍 Can't really think of a better and more accurate name.

[spawnia]

Some gems from https://relatedwords.org/relatedto/resolve:

• Conflict
• Negotiation
• Question
• Matters

[olivernybroe]

Hmm, then ResolverArgs seems better.

What about Execution?

From the docs

Each field on each type is backed by a function called the resolver which is provided by the GraphQL server developer. When a field is executed, the corresponding resolver is called to produce the next value.

[LastDragon-ru]

Proof of concept

How to try:

1. composer require lastdragon-ru/lara-asp-graphql:dev-master
2. Add LastDragon_ru\LaraASP\GraphQL\Resolver\Provider into applications providers

How to use:

public function __invoke($_, array $args, ?Root $root, GraphQLContext $c, Args $a): array {
    // ...
}

Features:

• Compatible with existing code, you can use the following names (todo: should be disableable because may conflict with typed arguments):
  • _, root ;
  • array $args;
  • context instance of GraphQLContext;
  • resolveInfo instance of ResolveInfo;
• Root - value wrapper for root, also possible to use actual root class name;
• Args - value wrapper for $args;
• GraphQLContext and ResolveInfo can be injected by class names;
• Possible to implement validation like FormRequest - just make subclass of ArgsValidated and inject it (added more as workaround for #1780)

Pitfalls:

• Seems it work only for parentIsRootType() and I don't see how it can be added for nested nodes without refactoring.

How it work work

Actually it is very simple:

public function resolve(
        string $callback,
        mixed $root,
        array $args,
        GraphQLContext $context,
        ResolveInfo $resolveInfo,
    ): mixed {
        try {
            // Bind our Value Objects (needed for ArgsValidated and similar).
            $this->container->bind(Root::class, static function () use ($root) {
                return new Root($root);
            });
            $this->container->bind(Args::class, static function () use ($args) {
                return new Args($args);
            });

            // Parameters that will be mapped by class name (probably will be better use `->bind`)
            $parameters = [
                GraphQLContext::class => $context, 
                ResolveInfo::class    => $resolveInfo,
            ];

            // Just for case, needed only for nested queries that doesn't supported yet
            if (is_object($root)) {
                $parameters[$root::class] = $root;
            }

            // Old names, only for old code.
            $parameters += [
                '_'           => $root,
                'root'        => $root,
                'args'        => $args,
                'context'     => $context,
                'resolveInfo' => $resolveInfo,
            ];

            // Call
            return $this->container->call($callback, $parameters);
        } catch (IlluminateValidationException $exception) {
            // Convert validation error to Lighthouse error
            throw new ValidationException(sprintf(
                'Validation failed for the field [%s].',
                implode('.', $resolveInfo->path),
            ), $exception->validator);
        } finally {
            // Remove our bindings
            unset($this->container[Root::class]);
            unset($this->container[Args::class]);
        }
    }