Reconsider ArgumentsTransformer

[murtukov]

I think that the ArgumentsTransformer class and the associtated expression function arguments() should be reworked/removed and here I explain why:

I will speak about the current transformation and validation parts separately.

Why should we remove the current validation mechanism from the ArgumentsTransformer in favor of the new validation feature:

1. It's not flexible: we can't disable it, can't use validation groups, group sequences, can't reuse constraints from another classes (code dublication), can't control cascade validation.

2. It happens in the transformation phase (inside the ArgumentsTransformer), which breaks the single responsibility principle.
   Transformation and validation should be 2 completely separated concepts.

3. It works only with types created by annotations.

Why should we remove/rework the current transformation mechanist:

1. It requires GraphQL types declared with annotations to hydrate data. That means I cannot transform data into a random object - it must certainly be a GraphQL type and this is unflexible.
   For example I would like to have a possibility to hydrate data directly into Doctrine entities (without or with validation).

2. The mapping is declared in expressions, which is not good. Expression language is a nice feature and we should use it, when it's necessary, but we shouldn't abuse the usage of expression language, because the configuration is not a place for logic.

Take a look at this 2 examples (php and yaml):

/**
 * @GQL\Type
 */
class RootMutation {
    /**
     * @GQL\Field(
     *   type="User",
     *   args={
     *     @GQL\Arg(name="input", type="UserRegisterInput")
     *   },
     *   resolve="@=call(service('UserRepository').createUser, arguments({input: 'UserRegisterInput'}, arg))"
     * )
     */
    public $createUser;
}

Mutation:
    type: object
    config:
        fields:
            register:
                type: User
                resolve: "@=mutation('register', [args, arguments({input: 'UserRegisterInput'}, args)])"
                # ...

As you can see there is too much logic inside strings (expressions) and this is definitely NOT a good practice. Configuration must be declarative and not imperative. The logic must be written in php, not in configuration. Plus expression language is hard to read, because of no highlighting.

Additionally the name arguments is confusing, because we already have a variable called args. It should be named transformer at least.

What I suggest:

First we need to distinguish 2 concepts: Transformer and Hydrator .

Transformer converts a given value into another data type or format. These can be scalars and objects.
Hydrator only populates a given object with data. It cannot convert values.

Lets follow the approach of Symfony Forms. Forms can do 3 things:
1) Hydrate entities with form data.
2) Transform field values with DataTransformers.
3) Validate data using entity constraints.
4) Validate raw data without entities (no object mapping).

I suggest to follow this approach and integrate our bundle with Doctrine entities as tight as possible, without any drawbacks, because most of the time people work with Doctrine.

3 and 4 are already implemented in the opened PR. Now we only need to implement a Hydrator and a DataTransformer.

Suppose we have the following yaml config:

Mutation:
    type: object
    config:
        fields:
            createUser:
                type: Post
                resolve: "@=mut('create_user', [args, hydrator])"
                entity: App\Entity\User
                args:
                    username:
                        type: String!
                    type: 
                        type: String!
                        transformer: App\Transformer\StringToInt # using a service
                    dateOfBirth: 
                        type: String!
                        transformer: App\Transformer\Generic::stringToDate # using a static method

We have here new entries: entity and transformer. When the resolver is called system will call transformers first. Then the entity will be populated with the transformed data with a default hydrator, using property accessor. args won't be changed:

public function createUser(ArgumentInterface $args, HydratorInterface $hydrator): User
{
    // First validates arguments and then 
    // creates and populates an object.
    $user = $hydrator->process();
	
    // At this point the $user is 100% valid.
    $this->em->persist($user);
    $this->em->flush();

    return $user;
}

Of course we need to decide in what moment the validation will be done, maybe create different validation strategies (e.g. first transform, then validate or first validate, then transform).

The form component normally takes validation constraints from the entity, but you always can add extra constraints directly to a form.
We can do the same, but in the example above I didn't use constraits directly in the config.

Let's discuss here this feature. Any suggestions?

[Vincz]

Hi @murtukov !

I agree that the ArgumentsTransformer is far from ideal. The main idea was to be able to manipulate PHP objects instead of data arrays.
You can still disable it by not using it (just don't use the arguments expression in your resolvers). And it can still works without annotations as long as you feed it with a map GraphQL type => PHP Class.
Still, I agree with you on this:

• The transformation and validation should be separate
• Their is place for improvement in the validation process
• It shouldn't be in the expression language

The thing I like about the ArgumentsTranformer is that I don't need to think about it. I only need to think in "PHP" and manipulate PHP objects and with the auto-guessed arguments / types feature it's really simple to use as the only thing I need to do when I need a new query / mutation is to annotate a provider method. I don't need to write a single line of YAML, and I like it. I write my PHP and the GraphQL is generated for me. It's the same PHP class used to declare the GraphQL type and receive data to validate it. For example:

/**
 * @GQL\Input
 */
class HeroInput
{
    /**
     * @GQL\Field(type="String!")
     * @Assert\Regex(
     *     pattern="/Dark/",
     *     match=false,
     *     message="A hero name can't contain 'Dark'"
     * )
     */
    public $name;

    public function isSkywalker()
    {
        return strstr($this->name, 'Skywalker') !== false;
    }
}

It's all I need to have a generated HeroInput GraphQL type, and to receive a validated instance of this object each time I have a type hinted argument with the class HeroInput in one of my method.
And if I use this class in a method like this for example:

/**
 * @GQL\Provider
 */
 class HeroRepository 
 {
     /**
      * @GQL\Mutation
      */
     public function createHero(HeroInput $input) : int
     {
         ...
     }
 }

It's all I need to have a generated createUser mutation with the correct argument generated for me. As you can see, I don't need to define the resolver or the args part.
With the YAML, you define all your GraphQL stuff, then, you tell wich method should be called and which argument should be used.
With the annotations and auto-guessed, it's your PHP code telling what it needs and the GraphQL is generated accordingly. It's the PHP method signature that define the GraphQL. It's great because, it's all in one place. If I need an new argument on the createHero mutation, I just need to add this argument to the PHP method and that's it. I don't need to first modify the YAML to declare my new argument, then modify the resolver expression signature to handle the new arg, then modify the PHP method signature to receive the new argument. 95% of the time you don't need to define the resolver or the args definition.

For the transformation part, your solution could work, and we could have a way of defining a transformer for every arg. But maybe the default should be an AutoGuessed transformer (The same way we used to inject entities in controller methods), we could check for the argument GraphQL type, and if we found a corresponding PHP Class, instanciate and populate it. So if it finds a argument of type HeroInput and it does have a corresponding PHP class for it, it will use it. This way, we could keep the auto-guessing features and stuff. We should also in the process, be able to inject other kind of arguments (following this discussion).

For the validation part, it's the same. Your solution is to add constraints to arguments and I think it would be a great addition. But we should also keep the automated part as well. As long as a transformer output an object, we should validate it (if there is no constraints attached to it, it will not trigger any validation).

So in the end, I think it's great to add new features as long as we keep the simplicity of the auto-guessed stuff and at the moment, I don't know how we could do it without the Arguments Transformer.

We should discuss about a service to handle this arguments transformation. @mcg-web is working on a service to inject contextual arg and it should be part of the process. I think it should be something as automatic as possible, a bit like the Symfony service auto-configuration.

[murtukov]

Hi @Vincz,

first of all I want to say, that I am NOT against annotations or any good features related to it.

• Declaring GraphQL types with annotations - is good
• Auto guessing - is good
• Using PHP classes as declaration and as DTOs - is good.

I like it all and I'm not saying that I want to remove something of it. We are all programmers here and we want things to be done as easy as possible. Thats why I love so much new features of Symfony 4 (service autowiring, service autoconfiguration, dependency injection by type-hints).

I only want to enhance this features, but to do so, much needs to be changed. For now it's an experimental feature for me and is not yet ready to be used in big projects.

The problem is that the current implementation of the annotations feature is too limited. The examples that you showed are too simple and will work only in small projects. In real life many people will have more complex projects (as I do). Imagine if you have an already working big project, where you use REST API, Doctrine entities and DTOs for validation purposes. And then you want to add a GraphQL API, without changig the current architecture of your project. In this case it would be very hard to use annotations for graphql schemas, because:

1. We already have entities and DTOs. If we create GraphQL types with annotations it will be a code dublication, becase we already have DTOs with validation constraints in it and managers, that persist data from this DTOs. In this case the simpliest way would be to use a yaml configuration, where you can say: here is my DTO, hydrate it with this data and use constraints from it. So you don't repeat yourself.
2. We can't use validation groups (but it's a question related to the validator)
3. What about complex data relations? How good does the ArgumentsTransformer work with nested types? one-to-one? many-to-one?

When we create a new feature, we should ask ourselves: How would it work with annotations? How would it work with yaml configs? How should I design this feature, so that its API is same in both?

New features should be generic, but the ArgumentsTransformer was clearly designed with annotations in mind: it's good and easy to use with annotations, but it's pretty useless with yaml. I don't see any use cases, where I would use it in the yaml approach. With other words ArgumentsTransformer is annotations-centric.

I suggest to rework the ArgumentsTransformer without losing any of its functionality, step-by-step.

1. First we need to decouple the transformation and validation logic. Let me work on it, as I have written the validation feature, it will be easier for me to integrate it. I will consult with you to keep things convenient.
2. Second we need to decouple concepts of Transformator and Hydrator. What does the ArgumentsTransformer actually do? It takes an object and populates it with data. Therefore it should be renamed to Hydrator, because the purpose of a transformer is different: it converts a single value to another value, inclusive scalars to scalars. When we rename it, we could then use it also as an entity hydrator in yaml configs as suggested above.

Another reason to rename it is the need to free the name transformer for the data transformer feature.

Everything in short:

1. Remove current validator from ArgumentsTransformer
2. Integrate the new validator
3. Reconsider the name ArgumentsTransformer. I suggest the name Hydrator or TypeHydrator or ObjectHydrator.

What do you think?

[murtukov]

@Vincz

With the YAML, you define all your GraphQL stuff, then, you tell wich method should be called and which argument should be used.

With php annotations you do the same: you define a class, then you tell which method should be called and which arguments should be used. It's pretty much the same.
The only differences are:

• you don't have to synchronize the argument list in yaml and in the php.
• you can use the php class as a DTO.

While it's a nice idea to create GraphQL types with only php and annotations it has it's disadvantages:

• you lose the overview of your GraphQL types. When you have yaml, you see everything there explicitly.
• you mix the concepts of configuration and a logic. You want to have all the configuration in the php classes, what can be considered as a bad practice by some developers.

Anyways it's all a matter of taste and we should let the users decide, what they want to use.

---

For the "argument list synchronization" problem I have an idea:

The idea is, that you dont declare the argument list in the yaml and don't use an expression language. Instead you only point to the method:

resolve: App\GraphQL\Resolver\UserResolver::findOne

The arguments list will be auto guessed by type hints in the resolver method:

public function findOne(ArgumentInterface $args, ArgumentsValidator $validator): User
{
    // do your stuff
}

This also eliminates the problem with double slashes in the expressions. The only drawback is, that you can't use expression functions inside it. Anyway you can always fallback to expressions if you want to.

[akomm]

@murtukov I am totally with you. Just recently had a discussion about exactly the same things you pointed out. Kind of funny. Loosing overview of files with annotations. To much expression, etc., FQCN in config/expressions (also backslash "issue", sync). I actually would only want to use annotations for DTOs. In other cases, annotations would lead to dead classes/shells with props and no other purpose than annotate to produce graphql types. I always think like this: when I remove annotation, does the class make any sense afterwards?

And having this:

<?php
/** @GQL\Type(name="Query") */
class QueryType
{
}

Just to create the query type, that is than later populated via @Resolver does not make any sense to me. But annotating DTO does make sense to me, because I would still be able to use them as DTO.

So yes, being general annotation-centric is not optimal.

[akomm]

As to the proposal part: do you think it is useful to be so open-minded about validation, hydration and transformation, as to make them exchangeable services? This might need to consider not hard-coding in the internal types configuration (the type extension) in regards to validation. Because it current proposed form makes assumption about validation mechanics used (AFAIK).

Why I ask:
I like the idea of not having to use expression at all for arguments-insertion. Like the example you provided @murtukov . Maybe we can have a service for resolving typed arguments, which has a list of resolvers you can enhance that resolve different types of arguments? For example instead of passing args via expression, there might be a ArgumentsResolver, which can handle \Overblog\GraphQLBundle\ExpressionLanguage\ExpressionFunction\GraphQL\Arguments typed arguments and populate them from query/mutation args. Or there might be an "instantiable" resolver, or specific ones that inject the ResolveInfo, when in signature. Those resolvers would need different sets of validation/hydration/transformation.

[murtukov]

@akomm

do you think it is useful to be so open-minded about validation, hydration and transformation, as to make them exchangeable services?

I am not sure I understand what you mean with "being open-minded" and "exchangeable services" 😄
But I'll try to explain, why a would like to have a transformer, validator and hydrator as separate features.

The Symfony Form component is a powerfull tool, but it's designed to be used with HTML, so when we decide to use GraphQL for our APIs, we automatically reject all these nice features. My proposal here is an attempt to recreate (reimplement) these features, but with GraphQL.

What is a Symfony form and what can we do with it?

With forms you can define a set of data, that you expect from a client. When a request comes from the client, the form handles this request and holds the input data. It's similar to GraphQL types, but form can also map the incoming data to an entity (or any other object), if you want. This is what I mean under hydration and I want to recreate this feature in our bundle. If you have some additional fields in your GraphQL type that you don't want to hydrate - no problem, you can mark these fields as unmapped, exatly the same way the forms do.

Forms also have data transformers, which make possible to convert data, before it is hydrated to an entity. A simpliest example is String <-> DateTime conversation.

Now about validation. When form handles a request, it automatically performs a validation. First it hydrates an object (entity) with data and then validates this object. This approach is considered wrong, because if validation fails you have an instantiated entity with invalid data inside it, which can still be persisted in DB. My implementation of validator creates temporary DTOs and validates them. If input data is valid we could then hydrate our entities.

I have no doubt, that the 3 features should be separate (validation, hydration, transformation), because they can be used separately:

1. Validation - if you want just validate a raw data, without any transformation or object hydration.
2. Hydration - if you know, that the incoming data is 100% valid and you don't want to validate it, just hydrate an onbject directly.
3. Transformation - the same as 2, but you dont use objects, just raw data (DateTime transformation).

With Symfony Forms you can do all of it, but it can be done easely with only one call: handleRequest(), which performs all 3 operations behind the scenes.

My question is: How can we implement the handleRequest approach with GraphQL types?
My code example with hydrator being injected into a resolver is only a draft, a starting point for this discussion.

So how can we implement all 3 features so, that they could be used separately and, if you want, could be performed with one call?

[murtukov]

@akomm

I didn't understand this part:

Maybe we can have a service for resolving typed arguments, which has a list of resolvers you can enhance that resolve different types of arguments? For example instead of passing args via expression, there might be a ArgumentsResolver, which can handle \Overblog\GraphQLBundle\ExpressionLanguage\ExpressionFunction\GraphQL\Arguments typed arguments and populate them from query/mutation args. Or there might be an "instantiable" resolver, or specific ones that inject the ResolveInfo, when in signature. Those resolvers would need different sets of validation/hydration/transformation.

Maybe you provide some examples?