Rework all builders

[murtukov]

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

@overblog/graphql

I strongly believe that we should rework all builders in the bundle and here I'll try to explain why.

This is just a draft with my thoughts on how builders could be changed. Use examples as a starting point to come to a common conclusion of how field/fields/args builders should be used/changed/enhaced.

What is wrong with builders?

1. Currently all builders are working with raw arrays and simply return them. Working with PHP arrays is not the best experience, so I suggest we create new builder classes to get rid of using arrays. This would lead to less errors, more readable code, autosuggestions by IDEs + opens perspectives for some other features in the future, for example using one builder inside another. Examples will be shown below a little later.

2. The configuration flow is pretty messy right now. Take a look at the following example:

   User:
       type: object
       config:
           fields:
               rawId:
                   # Field builder
                   builder: "RawId"
                   builderConfig:
                         name: photoID
                         type: String
                   # Args builder
                   argsBuilder:
                       builder: "Pager"
                       config:
                           defaultLimit: 50

   We have builder, builderConfig, argsBuilder, another builder one level deeper and config, which actually should be also named builderConfig. This is not the most readable type configuration and should be also reworked.

3. Lack of flexibility when it comes to the question Which part of my GraphQL Type do I want to be generated by a builder? We have 3 types of builders: args, field and fields builders. So what is the difference? Take a look at this picture:

   
   As you can see the main difference between builders is that they are responsible for different parts of GraphQL types.
   Important is that some builders are supersets of other builders, which means, that they can do the same as their subsets, for example we can replace multiple field and args builders with one fields builder, becase field and args builders are its subsets.

   Fields Builder > Field Builder > Args Builder
   

   The problem here is, that a user can't use one builder in which he could decide what he wants to be generated. Instead he needs to choose between 3 predefined builders with static limited responsibility (green, blue and red areas on the pic). Maybe we can combine all 3 builders into 1 (Type Builder)?

These are 3 points on why we should rework builders.

1. Getting rid of arrays

FieldBuilder

Current implementation:

The method gets a single argument with all configs and returns an array.

class RawIdField implements MappingInterface
{
    public function toMappingDefinition(array $config): array
    {
        $name = $config['name'] ?? 'ID';
        $type = $config['type'] ?? 'Int!';
​
        return [
            'description' => 'The raw ID of an object',
            'type' => $type,
            'resolve' => "@=value.$name",
        ];
    }
}

Suggestion:

The method gets all configs separately and doesn't return anything. The class extends a base class instead of implementing an interface.

class RawIdField extends FieldBuilder
{
    public function build(string $name, string $type): void
    {
        $this
            ->setType($type)
            ->setDescription('The raw ID of an object')
            ->setResolverExpression("value.$name");

        # if without expression
        $this->setResolver('my_resolver');
    }
}

Adding validation:

Passing constraint objects to the builder is not possible:

$this->setConstraints([
    new Assert\Uuid()
    new Assert\Length(['min' => 1, 'max' => 15]),
]);

because eventually a type class will be generated where the constraints will be actually instantiated. So we should pass class names instead of objects and optionally their params one by one:

$this->addConstraint(Uuid::class);
$this->addConstraint(Length::class, ['min' => 1, 'max' => 15]);

There might be different approaches to add constraints all at once:

1. Passing array pairs

->setConstraints([
    [Uuid::class]
    [Length::class, ['min' => 1, 'max' => 15]],
]);

2. Using an associative array, which has it's drawback, that constraints cannot be added more than once:

->setConstraints([
    Uuid::class => null,
    Length::class => ['min' => 1, 'max' => 15],
]);

This is a pretty rare case when someone needs to add a constraint more than once, but in case it's required it could be done by using addConstraint().

Entries without params could omit null:

->setConstraints([
    Uuid::class,
    Length::class => ['min' => 1, 'max' => 15],
]);

Example:

class RawIdField extends FieldBuilder
{
    public function build(): void
    {
        $this
            ->setType(Type::ID)
            ->isNullable(false)
            ->setResolveExpression("value.$name")
            ->setConstraints([
                Assert\Uuid::class,
                Assert\Length::class => ['min' => 1, 'max' => 15],
            ]);

        # or set a link
        $this->setLinkToProperty(User::class, 'id');
    
        # or set cascade validation
        $this->setCascade(true);       
    }
}

Args Builder

Pretty much the same as field builder.

Current Implementation:

class Pager implements MappingInterface
{
     public function toMappingDefinition(array $config): array
     {
         $defaultLimit = isset($config['defaultLimit']) ? (int)$config['defaultLimit'] : 20;
         return [
             'limit' => [
                 'type' => 'Int!',
                 'defaultValue' => $defaultLimit,
             ],
             'offset' => [
                 'type' => 'Int!',
                 'defaultValue' => 0,
             ],
         ];
     }
}

Suggestion:

class Pager extends ArgumentsBuilder
{
     public function build(int $defaultLimit = 20): void
     {
        $limit = $this->addArgument('limit', Type::INT, $defaultLimit)->isNullable(true);
        $offset = $this->addArgument('offset', 'Int!', 0);
     }
 }

Some extra examples:

class Pager extends ArgumentsBuilder
{
    public function build(): void
    {
        // ...
        
        # Change some options
        $limit->isNullable(false);
        $limit->setType(Type::STRING);
    
        # Add validation
        $limit->addConstraint(Length::class, ['min' => 1, 'max' => 10]);
        
        $this->removeArgument($offset);
        # or
        $this->removeArgument('offset');
    }
}

Fields Builder

The same approach as in field and args builders: each part of the type should be buildable with predefined methods.

2. Changing the configuration

I don't have any idea to drastically change the current approach to insert builders into GraphQL types. It just feels somehow messy. Here is an example using all 3 builders:

User:
    type: object
    config:
        # Fields builder
        builders:
            - builder: Timestamped
              builderConfig:
                  propertyCreated: dateCreated
        fields:
            rawId:
                # Field builder
                builder: "RawId"
                builderConfig:
                      name: photoID
                      type: String
                # Args builder
                argsBuilder:
                    builder: "Pager"
                    config:
                        defaultLimit: 50

The first thing I don't like about this approach is mixing of concepts of yaml and php configurations. The configuration gets sparsed, because part of the type lies in a yaml file and another part in a php file.

The second thing I don't like is that builders can be defined at many places, which looks kind of messy and confusing.

Maybe we can fix both problems with a Type Builder (new builder type), which I will talk about later.

And even if we don't change the approach above, we could at least make it a little bit nicer. Here is the same example changed:

User:
    type: object
    config:
        # Fields builder
        builders:
            - name: Timestamped
              params: [dateCreated]
        fields:
            rawId:
                # Field builder
                builder: 
                    name: "RawId"
                    params: [photoId, String]
                # Args builder
                argsBuilder:
                    name: "Pager"
                    params: [50]

• builderConfig and config are renamed to params and moved under the builder option, because builderConfig makes no sense without builder.
• params can be defined without names (collection) and they will be injected into builder separately. If user wants to use named params he can do it and in this case params will be injected into the builder using same argument names in the PHP.

short syntax (without params):

User:
    type: object
    config:
        builders: [Timestamped]
        fields:
            rawId:
                builder: RawId
                argsBuilder: Pager

Btw I don't know if field builder and args builder can be mixed.

3. Type Builder

A Type Builder is the new type of builders and it's responsible for generating of any part of a type:

Here is an example:

class UserBuilder extends TypeBuilder  
{  
    public function build(string $name, string $type, ?string $resolver = null): void  
    {  
        $this->setName($name);  
        $this->setType($type);  
    
        $usernameField = $this->addField("username", "String!")->setResolver($resolver);  
        
        if("User" === $type) {
            $usernameField->addArgument('limit', 'Int');
        }
        
        // ...
    }  
	
    public function configProvider()  
    {  
        yield ['User', 'object', 'user_resolver'];  
        yield ['UserCreateInput', 'input-object'];  
        yield ['UserUpdateInput', 'input-object'];  
        yield ['UserPayload', 'object'];  
    }  
}

This builder is used to create 4 different GraphQL types. The build method will be called 4 times with different arguments received from configProvider (like in PHPUnit tests). Technically a single type builder would be enough to build the entire GraphQL schema, but it would make sense to create separate builders for groups of related types.

With this we don't need yaml types at all, the entire schema could be defined with type builders. It could even be possible to use arg and field builders inside the type builder (don't know if it could be useful).

Maybe with type builders the need in other builders will disappear?

[Vincz]

Hi @murtukov !
I agree that PHP arrays are not really sexy.... but no matter what you do, at the end of the day, it's what you get. I don't really like to extend class, I prefer to implement interfaces, because my "builder" can be anything (a service or whatever) already extending something.
The builders are just config generator. You could add a object to it, like, instead of returning an array in the toMappingDefinition you could return objects. But your objects will be turn into PHP array just after.
So adding objects into all of this, would just be a new way to express configuration array.

A PHP like that, you already know that it's a GraphQL config :

return [
    'description' => 'The raw ID of an object',
    'type' => $type,
    'resolve' => "@=value.$name",
];

With this, you need to learn something new, and understand that it's the same as the above:

$this
    ->setType($type)
    ->setDescription('The raw ID of an object')
    ->setResolverExpression("value.$name");

# if without expression
$this->setResolver('my_resolver');

So, for me, it's just "adding" a new step with new stuff to learn. Not really necessary I think.
And I don't really understand the problem with the constraints as you just need to do as usual, like:

return [
    'description' => 'The raw ID of an object',
    'type' => $type,
    'resolve' => "@=value.$name",
    'validation' => ...
];

I let the others express themselves on the subject.

[murtukov]

Hi @Vincz

What do you mean by that?

but no matter what you do, at the end of the day, it's what you get.

If we implement the builders I suggested, we would have no arrays at all, so at the end of the day we would have objects 😀.

---

I don't really like to extend class, I prefer to implement interfaces, because my "builder" can be anything (a service or whatever) already extending something.

Currently we have just one interface MappingInterface for all 3 builder types, that means when you open any builder you can't say of which type it is unless you read the code inside the toMappingDefinition method.

But with object builders on other hand we would have 3 different classes: ArgumentBuilder, FieldBuilder and FieldsBuilder predefined by the bundle, so you would always know of which type it is and what $this means. Your builder couldn't be anything else, but only one of those 3 classes.

---

But your objects will be turn into PHP array just after.
So adding objects into all of this, would just be a new way to express configuration array.

It doesn't matter what happens afterwards, we are talking now about a convenient API for users of this bundle, not for developers. The users don't have to think about what happens after. From their point of view they will never get into touch with arrays.

---

A PHP like that, you already know that it's a GraphQL config :

return [
    'description' => 'The raw ID of an object',
   'type' => $type,
   'resolve' => "@=value.$name",
];

How do you know it's a GraphQL config? It's just a regular PHP array, it doesn't contain any additional information.

But if it were an object:

1. You could know what options you can add to the builder just by typing CTRL+SPACE (in PHPStorm, could be different in other IDEs)
2. You could read the description of every method again with the help of your IDE.

---

With this, you need to learn something new, and understand that it's the same as the above:

Let's be honest, when you use this bundle for the first time do you know what a builder is? Surely not. So you have to read the documentation and learn this feature anyways to know, what it is. But with objects it's easier to use them, because of the autosuggestions and descriptions of all methods. You don't have to return to the docs every time to look, what keys your array is allowed to contain. And what if you make a mistake? It would be hard to find it, especially if you have something like that (from documentation):

class MutationField implements MappingInterface
{
    public function toMappingDefinition(array $config): array
    {
        $name = $config['name'] ?? null;
        $resolver = $config['resolver'] ?? null;
        $inputFields = $config['inputFields'] ?? [];

        $successPayloadFields = $config['payloadFields'] ?? null;
        $failurePayloadFields = [
            '_error' => ['type' => 'String'],
        ];

        foreach (\array_keys($inputFields) as $fieldName) {
            $failurePayloadFields[$fieldName] = ['type' => 'String'];
        }

        $payloadTypeName = $name.'Payload';
        $payloadSuccessTypeName = $name.'SuccessPayload';
        $payloadFailureTypeName = $name.'FailurePayload';
        $inputTypeName = $name.'Input';

        $field = [
            'type' => $payloadTypeName.'!',
            'resolve' => \sprintf('@=mutation("%s", [args["input"]])', $resolver),
            'args' => [
                'input' => $inputTypeName.'!',
            ],
        ];

        $types = [
            $inputTypeName => [
                'type' => 'input-object',
                'config' => [
                    'fields' => $inputFields,
                ],
            ],
            $payloadTypeName => [
                'type' => 'union',
                'config' => [
                    'types' => [$payloadSuccessTypeName, $payloadFailureTypeName],
                    'resolveType' => \sprintf(
                        '@=resolver("PayloadTypeResolver", [value, "%s", "%s"])',
                        $payloadSuccessTypeName,
                        $payloadFailureTypeName
                    ),
                ],
            ],
            $payloadSuccessTypeName => [
                'type' => 'object',
                'config' => [
                    'fields' => $successPayloadFields,
                ],
            ],
            $payloadFailureTypeName => [
                'type' => 'object',
                'config' => [
                    'fields' => $failurePayloadFields,
                ],
            ],
        ];

        return ['field' => $field, 'types' => $types];
    }
}

---

With this, you need to learn something new, and understand that it's the same as the above:

With this you don't have do understand it's the same as the above, because the above would not exist.

---

And I don't really understand the problem with the constraints as you just need to do as usual, like

When you add constraint into a yaml file you write string names of the constraints:

username:
    type: String!
    validation:
        - Json: ~
        - Length: 
              min: 6
              max: 32

If you want to repeat the above in the builder with arrays we will get the following:

'username' => [
    'type' => 'String!',
    'validation' => [
        [
            'Json' => null
        ],
        [
            'Length' => ['min' => 6, 'max' => 32]
        ]
    ]
]

As you can see I don't use objects there.

[Vincz]

I understand your point, and you have solid arguments, but what I meant is that... no matter what kind of parser you use (Annotations, Yaml, GraphQL, ...), they all end up dumping a config array for Webonyx GraphQL lib (check the ParserInterface). And yes, even with your implementation, your objects will be turned and merge into a PHP array (check the BuilderProcessor). Currently, there is no object anywhere else in the bundle to describe GraphQL types/fields and this is what you want to implement with your objects and it would be very specific to builders.
With the current builders implementation, we are low level. I agree, it's less sexy than using objects with a fluent interface but there is no extra step.
And about the learning curve, if you use builders, it means that you have already understand the type configuration (as there is anyway only 3 or 4 keys to know).
About the configuration part, I agree we could improve it.

[murtukov]

with your objects and it would be very specific to builders

Why would it be "very specific"? It will generate the same webonyx objects at the end, there is no difference, what you use (Annotations, Yaml etc.).

---

With the current builders implementation, we are low level. I agree, it's less sexy than using objects with a fluent interface but there is no extra step.

Low level doesn't mean good, actually I suggest to introduce object builders to avoid low level interactions. And why is an extra step even an issue if it happens in the background and only once in the generation time?

---

And about the learning curve, if you use builders, it means that you have already understand the type configuration (as there is anyway only 3 or 4 keys to know).

Learning curve? These needs max 5-10 minutes to understand. As you said, if you are working with builders, you already know types and their keys, therefore you will understand builders too. It is not something big so that you need to sit and learn it all day long.

---

I don't see any serious reason to NOT to implement this. But I do see reasons to DO it.

[akomm]

I agree, that replacing deep arrays is a good thing. I also see the point of @Vincz . What if we could achieve both things and stay BC?

@murtukov
What if your builders (except type, which is a new thing) would be simply implementations of the MappingInterface? I see there one fixable issue though: If you use $this to provide API for building, it means you either have to instantiate a new builder instance each build operation or the builder must track state and reset it. My proposal would be instead of using the builder as API, provide some object injected into the build method, which has the desired API.

I agree with you, that things feel partially redundant, especially args were never useful to me. And the field builder only became useful after I have added the type emission feature

The types emission features above is in some way related to the type builder you propose:

Actually, the initial Idea I had way back in version 0.7, was to have a type building system, where having type: object actually means you use a builder object, so that the builder is kind of first-class concept.
In fact the bundle uses internal MappingInterface exactly like that for those built-in relay types. You use the type: relay-connection and it goes through a MappingInterface to generate the corresponding relay types (connection, edge).

Quote:

friendConnection:
    type: relay-connection # this is the relevant part
    config:
        nodeType: User
        resolveNode: '@=resolver("node", [value])'
        edgeFields:
            friendshipTime:
                type: String
                resolve: "Yesterday"
        connectionFields:
            totalCount:
                type: Int
                resolve: '@=resolver("connection")'

[murtukov]

@akomm

First of all, can we agree that the performance is not so important here, as builders are called only in the generation time and only once?

---

If you use $this to provide API for building, it means you either have to instantiate a new builder instance each build operation or the builder must track state and reset it.

The second option would do. No need to track the state, just need to call the reset method on the builder before system uses it's build.

---

My proposal would be instead of using the builder as API, provide some object injected into the build method, which has the desired API.

My first idea was to inject a builder object as you said, but I found some disadvantages of this approach:

• If we inject a builder object, then how we inject the params? There are 2 options (neither of which I like):

  • We injects params as a second argument: build(Builder $builder, $params)
    The problem is, that you can't see which params do you have and don't know their types. Plus you need to extract them first.
  • We can inject params separately after the builder: build(Builder $builder, $name, $type)
    This is not intuitive for the users and just doesn't feel right, because it's not typical for PHP.

  It also feels wrong to inject a builder into a builder:

  class Pager extends ArgumentsBuilder # this class is a builder
  {
      public function build(Builder $builder): void # and we inject a builder
      { }
  }

• You can't use dependency injection. My idea is to make all builders as services, so that you can inject any dependency you want. You can even build your entire schema depending on DB data (I already saw an issue with such request).

---

And the field builder only became useful after I have added the type emission feature

I read about this feature and found it weird. Why would you generate new GraphQL types in a Field Builder? The name speaks for itself, it should build fields, not new types. Of course all this unless I misunderstood something in this featur.