[RFC] Resolver definition enhancement

[murtukov]

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

These are the next enhancements I would like to work on next.

Expression language

Currently this bundle heavily (if not completely) relies on Expression Language which is a powerful, yet pretty exotic feature. I don't believe it should be the main configuration tool, because it:

• requires to learn a new pseudo-language to start
• has no support of its syntax
• introduces too much logic into config files

Now it became so ubiquitous in the bundle, that we even use it inside annotations, which itself is a pseudo-language. Consider this piece of code:

/**
 * @GQL\Field(resolve="resolver('hero_friends', [value, args['page']])")
 */
public $friends;

the expression inside the annotation looks just ridiculous.

The thing is, that we can achieve same things with more traditional methods and turn the Expression Language from the "must have" to the "nice to have" feature, whaе it actually was invented for. This part is an attempt to rethink the way we define resolvers in our GraphQL schema (one of the ways).

Changing the resolve to resolver

Example:

Query:
    type: object
    config:
        fields:
            user:
                type: "User!"
                description: "Something, that talks"
                resolver: App\GraphQL\UserResolver::getUser

Just a FQC and method name, no @= prefix, no quotes, no double or quadro slashes, no code-eye-parsing. Pretty clean, right? And this syntax is well knows to IDEs, you can just ctrl+click on the link and teleport right to the resolver method... even if it's on the Moon 🚀🌑

The name resolve is a verb, which makes sense with Expression Language, but in the new configuration it just points to a method and calling it resolver is more suitable here (@akomm's idea btw.). In addition it will allow us to mark the resolve field deprecated.

Jumping to the resolver method

Now it comes to the resolver configuration. This could be done with the new @Resolver annotation. First question that comes into my mind is "How do we define the order of the resolver params?".

Well, first it will try to guess params. We can guess ArgumentInterface, ResolveInfo, InputValidator, Models by the type-hints. The parameters without type-hints will be guessed by names: $value, $object, $context, $user.

If non-standart names are used, they could be mapped with the @Resolver annotation:

/**
 * Configuring all variables. Only predefined values are available.
 *
 * @Resolver("args", "context", "info", "value")
 */
public function getUser(ArgumentInterface $args, $context, ResolveInfo $info, $value) 
{
    # ...
}

/**
 * Configure only specific params
 *
 * @Resolver({"var" = "context", "parent" = "value"})
 */
public function getUser(ArgumentInterface $args, $var, ResolveInfo $info, $parent) 
{
    # ...
}

Services can be simply injected via constructor and that should be enough. But if the user is desperate enough, and wants
to inject services or values from expressions, he can use @Param annotation, designed for extended configuration of a specific param:

/**
 * @Param("post", expr="repository.find(args.postId)")
 * @Param("options", expr="json_decode(args.options)")
 */
public function getUser(Post $post, ArgumentInterface $args, array $options) 
{
    # ...
}

Noticed that I access args items with the dot symbol? We can make it possible by adding the magical method __get to the Argument class.

There is also no @Resolver annotation, because it's not necessary, the class already implements ResolverInterface. The @Resolver annotation can be used for simple configs like order of predefined variables, or setting the resolver alias, which will be described below.

Aliases

We all love the old good aliases, expecially those, who are too lazy, to type long FQCNs (wink to @Vincz 😉).

If the FQCN is too long and it bothers you, then you can use the short alias:

Query:
    type: object
    config:
        fields:
            user:
                type: "User!"
                description: "Something, that talks"
                resolver: UserResolver::getUser

Usually you don't have resolvers with same names and methods, so that shouldn't create any collisions in 99.99999% cases, unless you are insane. If it's the case, you can use the @Resolver annotation to define an alias either on the class or on the method (or both): @Resolver(alias="my_alias").

With this however you can't ctrl+click on the alias.

Access

What if we make possible to restrict access on resolver level? It can be applied on the whole class to restrict all the resolvers inside, or only on a specific method (like access control in Symfony controllers):

/**
 * @IsGranted("ROLE_EMPEROR")
 */
class NewsResolver implements ResolverInterface
{
    /**
     * @Resolver(alias="my_resolver")
     * @IsGranted("ROLE_PRESIDENT")
     */
    public function test(ArgumentInterface $a, ResolveInfo $info, UserInterface $user)
    {

    }
}

The @IsGranted annotation is pretty powerful, users can define their voters and all access logic will happen there.

The getAliases method

This method won't be necessary anymore, all required information is already provided with annotations.

Using __invoke method

If method is not defined in the resolver path, it will simply use the __invoke function.

That's it for now. Please tell me, what you think @mcg-web @Vincz @akomm @mavimo

[Vincz]

Hey @murtukov !

I agree with you about the expression language, but...
On my current big project, I didn't use it a single time.

Here is why:

I use the @Provider, the @Query and the @Mutation annotations all the time.
So I don't have this intermediate step of defining the query/mutation with a resolver.

When I need a new query or mutation, all I have to do is tag a method as query or mutation.

Here is a really simple example:

/** @GQL\Provider **/
class MailerService {
    /**
     * @Mutation
     */
    public function sendEmailTest(string $type, string $email = '[email protected]') : bool
    {
        return $this->sendEmailTo($this->getEmail($type), $email);
    }
}

And.... that's it.
Now I have a new mutation, name sendEmailTest with two args : type: String! and email: String, default: [email protected] and his type is Boolean.
Once again, I go from PHP to GraphQL, and not "here is my GraphQL query, and to resolve it, here is what you must do".
With the annotation approach, you define all of your query related stuff in the same place and it's really convenient.
If I need a new argument, I just add it to my method / function. If I need to control access, same.

With the arguments transformer, I don't bother using expression language as all is automatic (and the expression language is then only used in the AnnotationParser and we even could remove it from here if we replace it with your new code generator.
The current limitation of this system is : The MailerService class must be a public service accessible by it's FQN.

Hope it helps !

[murtukov]

@Vincz how is it supposed to help if I talk about improvements in yaml configs only. Sounds more like another annotations advertising 🙂

[mavimo]

@murtukov a lot of improvements 🎉

I appreciate most of them, on some one i had no clue to have a strong opinion; I think should be super useful if this issue will be subdivided in many issues so we can discuss them without mixing, WDYT?

I'll try to add my points if we split it I'll report each one in the appropriate issue :)

Expression language

I'm not a big fan of expression language & annotations (at least until we have it in PHP8 and become part of the language :D ), actually I don't see too much coupling between bundle and expression-language, is not something strictly needed (we use this bundle and we do not use expression language, so not big issue on that) but I see is a dependency that is required also if not used and -maybe- something that a new developer that use the bundle see as "suggested solution".

I'll like to see contributions that will allow us to add support for expression-language only if explicitly required from developer and remove it from requirement (maybe moving to suggest section in the composer.json) and if developers will include it is something that will be "enabled".

Changing the resolve to resolver

LGTM, i think is more clear and will create less confusion to newcomer (also if not semantically "exact")

Jumping to the resolver method

I can try to understand why we are considering that but I'm not sure is something that is good practice, I mean, we have a "definition" on how params sorting should be, why we need to allow them to change order of args? Should we consider the args to be ALWAYS defined in the same order and no matter what should be the far name? Maybe I'm missing some points here... the only reason I see is to allow devs to re-use a service without writing an adapter that I think is a bad practice

Aliases

We all love the old good aliases ...

On that part maybe I'm a bit extreme but I'll like to consider to drop the alias ✂️

Access
I'm fighting with myself every time I consider the ACL as annotation. I think that should be part of the business logic and need to be handled via "code" without relay on some infra (non domain library + annotation), but the speed up that using annotation give us can't be ignored... so no strong opinion on how to proceed from my side.

Using __invoke method

Absolutely agree 😄

[Vincz]

@murtukov Sorry mate, I didn't mean to be rude. As your initial example was about annotations, I thought that was the subject. And yes, let's be honest, I'm definitely biased about annotations 😺

[murtukov]

@mavimo

actually I don't see too much coupling between bundle and expression-language, is not something strictly needed (we use this bundle and we do not use expression language

How did you define your yaml configs? If you look at the documentation it's literally all about expression language.

Example:

Character:
    type: interface
    config:
        resolveType: "@=resolver('character_type', [value, typeResolver])"
        description: "A character in the Star Wars Trilogy"
        fields:
            id: 'ID!'
            name: 
                type: 'String'
                access: '@=isAuthenticated()'
            friends: '[Character]'
            appearsIn:
                type: '[Episode]'
                description: 'Which movies this character appears in.'

Human:
    type: object
    config:
        description: "A humanoid creature in the Star Wars universe."
        fields:
            id: "String!"
            name: "String"
            friends:
                type: "[Character]"
                resolve: "@=resolver('character_friends', [value])"
            appearsIn:  "[Episode]"
            homePlanet:
                type: "String"
                description: "The home planet of the human, or null if unknown."
        interfaces: [Character]

resolve, resolveType, access - this fields use expression language. Without expression language types defined in yaml become useless. I wonder how do you define your schema?

---

I mean, we have a "definition" on how params sorting should be.

I don't really understand what you mean. Resolvers get only that, what user defines:

Query:
    type: object
    config:
        fields:
            posts:
                type: "[Post]"
                # Here I define what params and in which order (args, info)
                resolve: "@=res('App\GraphQL\PostResolver::getPosts', [args, info])"

// Getting exact required params
public function getPosts($args, $info)
{
     // ...
}

In case of a schema fully defined with annotations, the params are auto-guessed.
So this line is not about the order, but about what params do you want in your resolver:

@Resolver("args", "context")

---

I'm fighting with myself every time I consider the ACL as annotation. I think that should be part of the business logic

Actually @IsGranted("ROLE_ADIMN") doesnt contain any logic. It just says "This method should be checked for the acces rights" and calls voters, where the actual logic happens.
But if you want to do it fully in php, then we could create a special abstract resolver, something like AccessResolver and extend your resovler from it, then you can call inside your resolver $this->denyAccessUnlessGranted('ROLE_ADMIN');. This is identical to @IsGranted("ROLE_ADMIN")

[murtukov]

@Vincz Don't get me wrong, I love annotations and it's always my #1 choise for configuration (doctrine, routes, asserts, etc). And I know, that I will use only annotations in the future for GraphQL definitions, but not yet. I think it's not ready. It's not universal: validation is limited, it doesn't support groups, doesn't support group sequences, I don't know, in what form validation errors are returned, whether they are in the same form, as I wrote in the documentation, I don't know, whether argument transformation errors are mapped to validation errors and how they are mapped, i don't know if errors are translated etc.
Some aspects are too annotations-specific (ArgumentsTransformer for example).

It just happened, that when I started to use this bundle (since version 0.11) there were no annotations, so I dived in yaml and started to work on improvements in that direction. Then you implemented annotations. When I finish with yaml improvements I would dig in annotations direction.

I am trying to create features as universal as possible, so that they could be applied to any schema definition, whether it's yaml, annotations, GraphQL Schema language or xml. For example the Hydrator feature is supposed to be used with any method. It actually makes the same thing as ArgumentsTransformer, the only difference is that ArgumentsTransformer is leaned towards annotations and it's not conviniet to use it with yaml (I tried) or even with Graphql Schema Language (God save us all from this😀).

When I finish with Hydrator I will study the ArgumentsTransformer deeply and will try to "merge" them into one, leaving the best from both. And this child will be called "ArgumentsHydratorModelTransformerOptimusPrime". Just kidding.
The same applies to validation.

Btw, do you maybe have some playground project with a complex schema with annotations?

[mcg-web]

I can't answer for the annotation points because my main project using this bundle used yaml configuration. But expression language is the foundation of this bundle indeed.The bundle was first place made to be used by ANY dev (not only php or Symfony) , this point is very important to understand the concept under the creation of this bundle. Here in my team any frontend dev can make simple changes without requiring a php / Symfony expert.
Here an example:

User:
    type: object
    config:
        fields:
            username:
                type: "String!"
                description: "Something, that talks"
                resolve: '@=value.username'

if tomorrow for some reason we want to use the firstname as username instead, no need to understand PHP to make this simple change.

Expression was also introduce for a performance level up. If we take example above

this @=value.username can simple become $value->username after compilation so no method are call here multiply by the 1000 of resolvers calls this can make a difference.

Also expression can be used for almost all config entry:

User:
    type: object
    config:
        fields:
            username:
                type: "String!"
                description: '@=value.description'
                resolve: '@=value.username'

I can understand the need of making this more modern using directly services and I think this is a good change but I need to keep the concept based on expression.

We can introduce resolver without breaking the current behavior since expression trigger is not a must. I think that both concepts can live together.

[mcg-web]

I agree with @mavimo comment it look very close of my point of view on this.

[murtukov]

@mcg-web well my suggesting doesn't really remove or break anything. It just adds a new functionality, in which you can simply move all resolver-related configurations in the resolvers itself. So at the end of the day everyone is happy and can use whatever approach he wants.

Okay then, i can work on this, right?

[mcg-web]

move all resolver-related configurations in the resolvers itself

I don't really understand this point since you say that you don't break anything?

[murtukov]

Then I'll just show an example. I am not trying to remove the expression functionality, but simply trying to allow users to have an option. The following config should be valid after the implementation:

My YAML:

Query:
    type: object
    config:
        fields:
            user:
                type: "User!"
                resolver: App\GraphQL\UserResolver::getUser
                args:
                    userId: Int!
            userPosts:
                type: "[Post]"
                resolver: App\GraphQL\UserResolver::findUserPosts
                args:
                    userId: Int!
                    orderBy: String!

Resolver:

class UserResolver implements ResolverInterface
{
    public function getUser(ArgumentInterface $args, ResolveInfo $info)
    {
        // ...
    }

    /**
     * @Param("myService", expr="service('MyService')")
     */
    public function findUserPosts(ArgumentInterface $args, $myService)
    {
        // ...
    }
}

As you can see I don't configure resolver params in the yaml, because they are auto-guessed. But in the second resolver findUserPosts the param $myService cannot be auto-guessed, thats why I added annotation @Param. You can use expressions in the @Param and you can use as many @Param as you want. That's it basically, it doesn't break anything, you can still use expressions in your yaml or use old resolve option instead of resolver.

[mcg-web]

you don't get what I mean I think. I'm not using annotations in my main project so your solution does not answer to my needs.
I don't understand what the problem with supporting both

Query:
    type: object
    config:
        fields:
            user:
                type: "User!"
                resolver: '@=getUser()'
                args:
                    userId: Int!
            userPosts:
                type: "[Post]"
                resolver: App\GraphQL\UserResolver::findUserPosts
                args:
                    userId: Int!
                    orderBy: String!

@= is a trigger to enable expression language so this is not a requirement. To enable it, we can suggest installing expression language, then this become totally optional.

Next question for App\GraphQL\UserResolver::findUserPosts do you guess arguments to inject on Runtime for this service?

[murtukov]

@mcg-web No, I get what you mean. I didn't say you can't use both. You can, both will be valid in YAML:

this "@=res(...)" and this "App\GraphQL\UserResolver::getUser"

I only suggested to define them under different keys:

# Both correct
# Note the difference in suffix

# old one
resolve: "@=resolver('find_user_posts', [args])"
# new one
resolver: App\GraphQL\UserResolver::findUserPosts

Here are reasons, why it makes sense to use different keys:

• The resolver key can be validated in the TreeBuilder during the compilation and raise an error if method is not found

• If we use resolve for both, following will be syntactically incorrect:

  resolve: App\GraphQL\UserResolver::findUserPosts

  Because resolve is a verb. It's the same if you write: get: App\GraphQL\UserResolver::findUserPosts
  but correct word is: getter: ...

• If we use resolver for both, following will be syntactically incorrect:

  resolver: "@=value.username"
  resolver: true
  resolver: 15
  resolver: "some string"

  Because scalars are not resolvers.

The only minus is, that we have 2 keys: resolve and resolver, which can confuse people.

If we still decide to use 1 key, then I suggest it to be resolver.

[murtukov]

@mcg-web

Next question for App\GraphQL\UserResolver::findUserPosts do you guess arguments to inject on Runtime for this service?

No. It doesn't "inject" anything. It's generated 100% the same way as this:

resolve: "@=resolver('App\\GraphQL\\UserResolver::findUserPosts', [args, service('MyService')])"

We just don't define arguments in the yaml, they will be auto-guessed from the method signature during the type generation. Annotations will be used only if arguments cannot be auto-guessed, to provide additional information.

So the answer is no.

[mcg-web]

Ok thank you @murtukov for answers! its clearer to me now.

I prefer a solution totally base on config like this

Query:
    type: object
    config:
        fields:
            userPosts:
                type: "[Post]"
                # short syntax to guess from 
                # resolver: App\GraphQL\UserResolver::findUserPosts 
                resolver:
                     method: App\GraphQL\UserResolver::findUserPosts
                     bind:
                          $object: $value

Can I work on this feature ? I maybe have a solution to completely remove old resolve without loosing it flexibility.

[murtukov]

@mcg-web Interesting. I like it, very similar to service arguments binding.

But I have 2 questions:

1. How will the short syntax guessing work?
2. How do you use expression language?

---

Can I work on this feature ? I maybe have a solution to completely remove old resolve without loosing it flexibility.

Yeah, sure, I have another features to work on.

[mcg-web]

How will the short syntax guessing work?

short syntax will used Reflection on compile time to add information to bind using var names or typehint for ResolveInfo for example.

How do you use expression language?

expression language we become a service create on container compile time and will replace resolve by expression.

Query:
    type: object
    config:
        fields:
            user:
                type: "User!"
                expression: 'getUser(value)'

will be equivalent to something like that

Query:
    type: object
    config:
        fields:
            user:
                type: "User!"
                resolver: 'Overblog\GraphQLBundle\GeneratedExpression\Resolver_{sha1 expression string}' # ::__invoke

I will be smart enough to generate a resolver only if it doesn't already exist

[murtukov]

@mcg-web Take into account, that if we also make possible to configure bindings with annotations, then it would be conviniet to use it with GraphQL Schema Language:

type Query {
    user: User!     @resolver(method="App\GraphQL\UserResolver::getUser")
    posts: [Post!]! @resolver(method="App\GraphQL\PostResolver::findAllPosts")
}

Of course we could make possible to make bindings inside GraphQL Schema language as well:

type Query {
    user: User!  @resolver(
                     method="App\GraphQL\UserResolver::getUser",
                     bind: {object: "$value"}
                 )
    posts: [Post!]!  @expression(code="getUser(value)")
}

but it doesn't look very clean and you can't use the $ symbol as the binding var name ($object in this case).

[mcg-web]

I'm working on the feature using yaml in the first place, we'll break this feature in small PRs to make this easy to review so we can start using it and improve it before release.