overblog / GraphQLBundle

This bundle provides tools to build a complete GraphQL API server in your Symfony App.
MIT License
784 stars 221 forks source link

feature/hydrator #540

Closed murtukov closed 5 years ago

murtukov commented 5 years ago
Q A
Bug report? no
Feature request? yes
BC Break report? no
RFC? yes
Version/Branch 0.13

I am currently working on this feature. Its purpose is to automatise population of objects (e.g. Doctrine entities or DTOs) and to integratate it with the validator

Here is my view of how it should be implemented (not finished).

Suppose we have 3 entities:

Which are associated with each other: User one-to-one Address User one-to-many Post

We could then create corresponding GraphQL types:

Mutation:
    type: object
    config:
        fields:
            createUser:
                resolve: "@=res('create_user', [args, validator, hydrate])"
                hydration: App\Entity\User
                args:
                    username: String!
                    password: String!
                    address: Address!
                    posts: [Post]!

Address:
    type: input-object
    config:
        hydration: App\Entity\Address
        fields:
            street:
                type: String!
            city:
                type: String!
            zip:
                type: Int!

Post:
    type: input-object
    config:
        hydration: App\Entity\Post
        fields:
            title:
                type: String!
            content:
                type: String!

The line hydration: App\Entity\User is a shortcut. The full config looks like this:

hydration:
    class: App\Entity\User
    recursive: true # true by default
    force: false # false by default
    hydrator: App\MyCustomHydrator # null by default

In resolver:

public function createUser(ArgumentInterface $args, ArgumentsValidator $validator, User $user) 
{
    // The Validator is aware of the hydrator configs and
    // will use constraints defined in the App\Entity\User class
    // and all associated classes recursively.
    $validator->validate();

    // $user is an object created by the Hydrator.
    $this->userManager->save($user);

    return $user;
}

Problems

Naming problem

Variable name in the expression. I considered different names:

All names above have no direct connection with the hydrator and therefore are not intuitive. I decided to use the name hydrate because it has direct connection with the hydration and is short. hydrate is a noun, not a verb. The term hydration was adopted by the Hibernate ORM and wasn't used in IT before. Doctrine ORM, which is mostly a php copy of Hibernate, also adopted this term. So why not to use the term hydrate to denote objects, which were created in the hydration process?

I am open to suggestions.

UPDATE After some thought, I came to the conclusion, that the name hydrate can be confusing due to its similarity to the verb hydrate. What if we allow both entity and dto, which will be aliases to each other and users could choose themselves depending on the use case?

Validation edgy cases

Entities don't always have 100% same properties as their corresponding GraphQL types. Suppose we have an entity User:

class User {
    /** @Assert\...  */
    public $username;
    /** @Assert\...  */
    public $email;
    /** @Assert\...  */
    public $password;
}

And we want to create a mutation GraphQL type with additional arguments to manipulate profile data:

Mutation:
    type: object
    config:
        fields:
            updateProfile:
                resolve: "@=res('update_profile', [args, validator, hydrate])"
                hydration: App\Entity\User
                args:
                    username: String!
                    email: String!
                    password: String!
                    # additional arguments
                    about:
                        type: String!
                        validation:
                            - Length: {min: 3, max: 256}
                    preferences:
                        type: String!
                        validation:
                            - Json: ~

We want to update profile data, which includes user data. We have here 2 additional arguments (about and prefrences) which also have validation constraints. Question: how should validation happen in this case?

The other edgy case by validation is, if we use data transformers to convert input data, before it is hydrated into an object. Should we then validate only data in the object or also the raw value (the value, which was transformed)?

Data transformers are not yet implemented. They would work together with the Hydrator and Validator.

What if we use mixed validation constraints (declared both in the yaml and in entity annotations)?

Answer: We could introduce an option to control the validation strategy. Strategy 1: validates only hydrated entity (DTO), using constraints declared in entity itself. All additional arguments are ignored Strategy 2: validates both, entity and additional arguments, but in this case the validator will use the class ValidationNode to populate it with data, so that everything would be validated in the same context Strategy 3: ignores hydrated object and only validates raw arguments. It is usefull, if users have no DTOs and they want to populate entities directly. They could then just validate arguments and persist hydrated entites.

If not all GraphQL types have hydration configs:

Lets take the very first yaml example from this issue. What if there is no hydration configuration in the Mutation type and only in Address and Post? What should the hydrator return? Should we maybe indroduce a standard object to hydrate data into, so that the resulting tree wouldn't be borken?

akomm commented 5 years ago

Naming problem Variable name in the expression. I considered different names

I am not sure what name you mean. The hydrated args? Can't figure out the relevant location in your example. In your example you pass hydrate and args (raw?) into the resolver. The name also depends on whether you intend to hydrate all args-fields as a single php type or not. For example given relay standard you have 1 field input. You would need a php type (DTO) with a single input property for each mutation with input of different type, otherwise your input property will be incorrectly typed in php.

hydration:
    class: App\Entity\User
    recursive: true # true by default
    force: false # false by default
    hydrator: App\MyCustomHydrator # null by default

The above yaml: Is there a reason to not simply inject the hydrator as dependency into the resolver?

Validation edgy cases Entities don't always have 100% same properties as their corresponding GraphQL types. Suppose we have an entity User

I see rather a design issue, easy solved. The following does make more sense and also fixes the issue:

Mutation:
    type: object
    config:
        fields:
            updateProfile:
                resolve: "@=res('update_profile', [args, validator, hydrate])"
                hydration: App\...\UpdateUserProfile
                args:
                    user:
                        username: String!
                        email: String!
                        password: String!
                       # additional arguments
                    about:
                        type: String!
                        validation:
                            - Length: {min: 3, max: 256}
                    preferences:
                        type: String!
                        validation:
                            - Json: ~

The other edgy case by validation is, if we use data transformers to convert input data, before it is hydrated into an object. Should we then validate only data in the object or also the raw value (the value, which was transformed)?

You only care about validity of the data you get at the end. Every step should be optional: 1. transform 2. hydrate 3. validate 4. resolver This would not make any sense: 1. transform & validate 2. hydrate & validate 3. validate? 4. resolver

What if we use mixed validation constraints (declared both in the yaml and in entity annotations)?

This is a made-up edgy-case in my opinion, because instead of simply injection a validator with its existing configuration methods and having therefore 1 source of truth, we create with this solution two sources of truth and now have to ask the question: what do we do with this two sources of truth.

The only benefit of doing it in the bundle and not own app code via injection is, when we reduce boilerplate, make code more understandable, reduce redundancies in configuration/code.

I think what the bundle should max do is to define a set of interfaces for different services you can tell it to use. Such services could be: 1 guessing params in resolver by different strategies (value, info, args, ...) 2. transforming args, 3. hydrating args 4. validation args and then means of implement how to handle failure of any of this steps. So when you want to use symfony validation, you only need to configure the bundle to use it as validator (if needed with adapter interface so not only restricted to sy validator). You would not have two sources of truth but one. You use your validation solution via injection somewhere, or you let this bundle validate your inputs with it, they all have the same configuration options (annotation, yaml) and they are always sync.

If not all GraphQL types have hydration configs

I think hydration should be easy. You have array, you have object. Try set each array key to each property. No failures, if not writable, just skip. The validity is the job of the validator, not hydrator. Hydrator is kind of stupid. The "hardest" thing in a hydrator is in terms of recursive hydration using some sort of annotation to get the type of the nested php type. For example the symfony denormalizer can do the job easily, without any configuration. You only need types annotated with phpdoc, including arrays like MyNestedType[].

akomm commented 5 years ago

Transformation and hydration should be soft processes. They should simply "try" to do their best.

  1. Should transformer not being able to transform a value, leave value unchanged, go to next.
  2. Should a hydrator not be able to hydrate a prop, skip, go to next.
  3. Validation is where you can make sure everything is okay. If some of previous failed, you will find out data is invalid.

In general I do not recommend directly hydrating entities, but In regards to the implementation of the feature I think it does not matter.

The solution should be, in my opinion, about: how do we safe the need to do ->transform ->hydrate ->validate over and over again in out resolvers. And also how we can save us the expression injection for every field in the definition (in this specific case validator). Otherwise there is no point to make any validator integration, as we can simply inject it into the resolver's constructor and use it.

murtukov commented 5 years ago

A note about the args variable:

As I already told in other issue (this message), $args will NEVER be changed. It doesn't matter what do you use: a validator, a hydrator, a data transformer - the $args will always be the same (raw values). And it will always remain so.


I am not sure what name you mean.

I mean this name: image hydrate is an object (entity or DTO). The word hydrate sounds like a verb, so it's not an optimal name. I think it would be better to use words entity or dto interchangeable, like this:

resolve: "@=res('update_profile', [args, validator, entity])"
# which is equal to
resolve: "@=res('update_profile', [args, validator, dto])"

The name also depends on whether you intend to hydrate all args-fields as a single php type or not.

Hydrated objects will ALWAYS be populated by ALL args-fields.

For example given relay standard you have 1 field input. You would need a php type (DTO) with a single input property for each mutation with input of different type, otherwise your input property will be incorrectly typed in php.

If I understand you correctly you mean this case:

Mutation:
    type: object
    config:
        fields:
            createUser:
                resolve: "@=res('create_user', [args, validator, hydrate])"
                args:
                    input: UserInput!
            updateUser:
                resolve: "@=res('update_user', [args, validator, hydrate])"
                args:
                    input: UserInput!

UserInput:
    type: input-object
    config:
        hydration: App\Entity\UserDTO
        # ...

As you can see there is no hydration config in the Mutation type. But the UserInput type has hydration entry. In this case the resolver will get an array ob objects:

public function createUser($args, $validator, $hydrate)
{
    $user = $hydrate['input'];
}

So there is no need to create a DTO with a single property input.


The above yaml: Is there a reason to not simply inject the hydrator as dependency into the resolver?

Yes, there is a reason. There is y default hydrator, which will be used in the hydration process. It's not meant to be used directly. If you want to change a certain part of this hydrator, you can extend the default hydrator and change some methods, for example:

class MyCustomHydrator extends Hydrator 
{
    // Override the parent method for mapping
    public function mapName($name): string
    {
        // your logic
    }
}

This can be usefull if your entity has a property email, but your GraphQL type argument is called username and you want to map username to email in your entity.

Injecting the hydrator into a resolver? For what?


You only care about validity of the data you get at the end. Every step should be optional:

  1. transform 2. hydrate 3. validate 4. resolver This would not make any sense:
  2. transform & validate 2. hydrate & validate 3. validate? 4. resolver

You didn't get the workflow. The Hydrator makes NO validation. Validation happen ONLY if you inject the validator in your expression and call it in your resolver:

public function createUser($args, $validator, User $user) 
{
    // Validation happens here 
    $validator->validate();

   // ...
}

The default strategy for the validator is: "Validate raw arguments". That's it. BUT, if you use the hydrator, you can tell your validator to use a different validation strategy. I described 3 different validation strategies above, but I changed some points. Here are 2 validation strategies with use cases: 1) Validate raw arguments (by default). This strategy ignores all hydrated objects. It only validates the raw arguments. This approach will use the built in ValidationNode class to create DTOs, as described in the validation documentation.

Use case: you have a clean project with only GraphQL API + you have entities. Your entities have no validation constraints in anotations, yaml or xml. All constraints are located in your GraphQL yaml types. So you can just validate the raw arguments and then safely hydrate them into entities.

2) Validate only DTOs. This strategy ignores any raw arguments and will NOT use the ValidationNode class. It also ignores all constraints declared in the GraphQL yaml type. This strategy can be ONLY used with hydrator. It wil just take the hydrated object and validate it with default validator.

Use case: you have a clean project with only GraphQL API + you have entities. All validation constraints are declared inside entities with annotations. You don't need to type any constraints inside your GraphQL yaml types.

Note, that the validator feature has already a similar feature link, which can read the annotations and just use them for validation. The difference is, that you can't use data transformers with the link. Data transformers work ONLY WITH HYDRATOR.

I think hydration should be easy. You have array, you have object. Try set each array key to each property. No failures, if not writable, just skip.

Hydration is far from a simple operation. I just stop there 😄

murtukov commented 5 years ago

hydration config just associates GraphQL types with entities/DTOs. But it doesn't describe the relations between these entities/DTOs. And shouldn't, because GraphQL types already describe relations between types and this is enough to build your object tree.

murtukov commented 5 years ago

Transformation and hydration should be soft processes. They should simply "try" to do their best.

Why should they be soft? Take as example the doctrine hydrator and data transformers. The hydrator will fail, if something goes wrong. But the transformers logic is fully on users, they write the transformation logic and they will decide, whether it will fail or pass the value.

It should fail, if hydration doesn't work. Otherwise you can get a not fully populated object and will not even know about it.

murtukov commented 5 years ago

Strictness leads to more secure code. A soft behavior creates a large space for errors.

murtukov commented 5 years ago

@akomm

And also how we can save us the expression injection for every field in the definition (in this specific case validator).

You are absolutely right here, the second validation strategy makes no sense, as you can simply inject a validator in the resolver constructor and validate the hydrated object. Need to rethink it.

akomm commented 5 years ago

As I already told in other issue (this message), $args will NEVER be changed. It doesn't matter what do you use: a validator, a hydrator, a data transformer - the $args will always be the same (raw values). And it will always remain so.

I know that. But if you write something that leaves no other logical conclusion, I ask you anyway whether you mean args. As you correctly spoted, hydrate is confusing and was never considered by me as dto or entity. Part of error on my side, I missed the connection you tried to explain in the issue too ^^

Hydrated objects will ALWAYS be populated by ALL args-fields.

Which was exactly my question. So you can not inject two hydrated params into resolver. That means you will have to create additional php type to wrap all args-fields or fiddle with assoc arrays, which in my eyes defeats the purpose of hydration and also makes type-guessed injection harder in the future.

As you can see there is no hydration config in the Mutation type. But the UserInput type has hydration entry. In this case the resolver will get an array ob objects [...] So there is no need to create a DTO with a single property input.

See above.

Yes, there is a reason. There is y default hydrator, which will be used in the hydration process. It's not meant to be used directly. If you want to change a certain part of this hydrator, you can extend the default hydrator and change some methods

Or you simply have DTO and then names do not matter, because you have this:

// in resolver or service ->createUser($createUserInput)
// $createUserInput is the DTO
$entity = new Entity()
$entity->setEmail($createUserInput->username);

The issue starts at doing things one should not do and then building solutions for those made-up issues. In this case we clearly see what happens. In case of the extends Hydrator solution, we have some class somewhere far away from the place where its relevant and there the magic happens.

Injecting the hydrator into a resolver? For what?

First of all, the question was (tbh not obviously) rhetorical. It was based on hydrate being the verb and hance the hydrator injected via expression. The point should then be clear to you :).

You didn't get the workflow. The Hydrator makes NO validation.

I never told hydrator makes validation. I just answered your following question, pointing out possible workflows and what would make sense and what not.

The other edgy case by validation is, if we use data transformers to convert input data, before it is hydrated into an object. Should we then validate only data in the object or also the raw value (the value, which was transformed)?

Hydration is far from a simple operation. I just stop there

If you overload hydrator with responsibilities it should not have, it is indeed not simple. A hydrator should not care about types. A recursive hydrator should only care for object types and array of object types.

hydration config just associates GraphQL types with entities/DTOs. But it doesn't describe the relations between these entities/DTOs. And shouldn't, because GraphQL types already describe relations between types and this is enough to build your object tree.

Making some relations between GraphQL types reflect relations in DTO would be terrible. Two different concept. Otherwise why do you not just genrate DTOs out of GraphQL types, instead of configuring them? This is a rhetorical question. I know why you do, and the same reason is, why you do not want 1:1 relations in GraphQL types to DTO types.

Why should they be soft? Take as example the doctrine hydrator and data transformers. The hydrator will fail, if something goes wrong. But the transformers logic is fully on users, they write the transformation logic and they will decide, whether it will fail or pass the value. It should fail, if hydration doesn't work. Otherwise you can get a not fully populated object and will not even know about it.

Doctrine hydrator is a bad comparison. It populates domain objects. You do not get DTOs out of your ORM, right? This is different when you work with an API that maps (=not a 1:1 projection) to the domain. Exactly where you work with DTOs. Sure, if you populate your domain objects from an public API directly, you need to make a complicated hydrator. Its simply an obstacle course that will require more and more solutions :( .

Strictness leads to more secure code. A soft behaviur creates a large space for errors.

When you transform/hydrate your entities, you are making the code less secure. Your entities should not allow to be created in an invalid state, to be then validated. Validation is theoretically an optional step. If you fail to validate you have created invalid state. Entities should have type checks (signature typing, etc.) and verification of rules and throw on violation. Exactly with the overdoing of the responsibility of the transformation and hydration you potentially planting a bomb. On the other hand, if your model is defensive, hydration+dto+validation is only a layer over this defensive model, that allows to pre-validate and deliver feedback that is more clear to user than exceptions. And if your validation is not correct, the model will throw but not allow you to make something invalid. This is more strict and secure. And allows simple transformation,hydration and validation process :)

murtukov commented 5 years ago

@akomm

Which was exactly my question. So you can not inject two hydrated params into resolver. That means you will have to create additional php type to wrap all args-fields or fiddle with assoc arrays, which in my eyes defeats the purpose of hydration and also makes type-guessed injection harder in the future.

The Hydrator always generates a tree of objects (or object composition in other words), which has only one root object on its top. A tree cannot have more than one top object, therefore your resolver also cannot get more than one hydrated object. A tree generated by the Hydrator is meant to 100% repeat the tree of GraphQL types. Take the args variable as an example. args is an array tree. When you have a relay style arguments, how do you acces this arguments? You do: $user = $args['input'];

The hydrator just creates a copy of the args tree, but instead of array nodes, you have object nodes (where the hydration config is defined).

So what if some of the types (nodes) has no hydration configs?

We can do the following:

We could leave these nodes as arrays or we could fallback to a built in class, defined by the bundle. Something like HydratedObject or HydratedTree or HydratedArgs. This solves following problems:

An example:

Mutation:
    type: object
    config:
        fields:
            createUser:
                resolve: "@=mut('create_user', [args, validator, hydrated])"
                args:
                    input: UserInput!
            updateUser:
                resolve: "@=mut('update_user', [args, validator, hydrated])"
                args:
                    input: UserInput!

UserInput:
    type: input-object
    config:
        hydration: App\Model\UserDTO
        # ...
public function createUser(ArgumentInterface $args, ArgumentsValidator $validator, HydratedArgs $hydrated)
{
    $user = $hydrated->get('input'); /* $user instanceof User */
}
murtukov commented 5 years ago

Or you simply have DTO and then names do not matter, because you have this.

Still don't understand what do you mean by injecting the hydrator into a resolver. Bring some examples.

In case of the extends Hydrator solution, we have some class somewhere far away from the place where its relevant and there the magic happens.

This point makes no sense to me. A hydrator is a class used internally, you should not inject it somewhere, but you can modify its behavior. And then you can reuse the modified hydrator wherever you want. I didn't invent something new here, it's just a standard approach to modify a functionality of a library.


I think it's better to wait untill a do a first implementation, so that you can play with it and see everything yourself. Otherwise, we will discuss it endlessly.

murtukov commented 5 years ago

Making some relations between GraphQL types reflect relations in DTO would be terrible. Two different concept.

Symfony already uses the similar approach with the Form Component. First you configure forms and relations between these forms, then you map each form to an entity. Some fields can be marked as unmapped. When a request comes, the form component handles the request, hydrates mapped entities and performs validation.

What is the difference with GraphQL types? We define types, create relations between them and now I am trying to bring the hydration part, which would work similar to Forms. But it's somehow "terrible" for you. I dont get it.

akomm commented 5 years ago

HydratedArgs is better than array, because it allows auto-guessing between raw args (ArgumentInterface) and hydrated args. However its fields are still untyped, like in an array. I know that args is a tree and tree has a single root. Every tree is divisible into separate trees on per-depth-level basis.

This API would make more sense:

createUser:
  type: 'User!'
  args:
    input: CreateUserInput!
public function createUser(CreateUserInput $input) {}

Or:

users:
  type: '[User!]!'
  args:
    filters: {type: UserCriteria}
    options: {type: OtherOptions}
public function users(?UserCriteria $filters, ?OtherOptions $options) {}

Still don't understand what do you mean by injecting the hydrator into a resolver. Bring some examples.

You named it hydrate, I assumed its hydrator, hence I asked rhetorical, why not inject then in constructor if you inject it in expression. You fixed it, problem solved.

This point makes no sense to me. A hydrator is a class used internally, you should not inject it somewhere, but you can modify its behavior. And then you can reuse the modified hydrator wherever you want. I didn't invent something new here, it's just a standard approach to modify a functionality of a library.

The code example has nothing to do with hydrator injection. It was demonstrating, that the following is not needed:

This can be usefull if your entity has a property email, but your GraphQL type argument is called username and you want to map username to email in your entity.

Just because something is possible does not mean its good. Symfony also has \Symfony\Bundle\FrameworkBundle\Controller\AbstractController. Symfony has a lot of stuff.

murtukov commented 5 years ago

This API would make more sense:

createUser:
  type: 'User!'
  args:
    input: CreateUserInput!
public function createUser(CreateUserInput $input) {}

It's a pretty simple example. What if there is more than 1 argument (input in this case)? What if some arguments are of built in types (String, Boolean, Float etc.)? How (and where) should be the hydration config defined, you didn't show it in your example. This is vital.

akomm commented 5 years ago

Actually there is an example in the same post how it would. And you might have spotted a correlation between field arg names and resolver param names. The primitive types do not need to be hydrated, they can be passed through directly. The others are derived from the signature type annotation. Since resolver is part of graphql api domain its acceptable to have a coupling here (arg name, field name).

akomm commented 5 years ago

Example with primitives:

users:
  type: [User!]!
  args:
    page: {type: Int!}
    itemsPerPage: {type: Int!}
public function users(int $page, int $itemsPerPage) {}
akomm commented 5 years ago

A demonstration, how it could work: Note:

akomm commented 5 years ago

Btw if you desired to be strict in hydration, because you populate entity, you could use HydrateArgsStrict strategy ^^

murtukov commented 5 years ago

Actually there is an example in the same post how it would.

All examples you provided are incomplete. There are no resolve and no hydration configured, which are important.

Let's take this one:

createUser:
  type: 'User!'
  args:
    input: CreateUserInput!
public function createUser(CreateUserInput $input) { }

Where should be the hydration, how do you inject multiple inputs in expression? In all examples I have provided the hydration key is defined on the same level as args. But your examples requires, that i define hydration for each argument I want to be hydrated. What if I dont want to use the relay style?

Please provide a complete example, with an expression, resolver and hydration, with mixed arguments (input arguments and primitives), not separate examples with either only inputs or only primitives.

akomm commented 5 years ago

I have added some code and more examples after the quote. If that does not explain it to you, please tell me ^^

murtukov commented 5 years ago

Your examples still have no resolve and hydration configs.

akomm commented 5 years ago

You do not need any hydration configs. But you are right, you need resolve. I omitted it, but its obviously not self-explaining:

createUser:
  type: 'User!'
  args:
    input: CreateUserInput!
  resolver: "Mutation.createUser" // note resolveR <- expects resolver alias or FQCN

This requires a resolve-wrapper like this:

public function resolve(string $resolverAlias, $value, ArgumentInterface $fieldArgs, ResolveInfo $info)
{
    $resolver = $this->resolvers->getSolution($resolverAlias);
    $resolverOptions = $this->resolvers->getSolutionOptions($resolverAlias);
    $resolverMethod = $resolverOptions['method'] ?? null;

    Assert::notNull($resolver);
    Assert::notNull($resolverMethod);
    Assert::methodExists($resolver, $resolverMethod);

    $resolverParams = (new \ReflectionMethod($resolver, $resolverMethod))->getParameters();
    $resolverArgs = [];

    foreach ($resolverParams as $param) {
        $resolverArgs[] = $this->resolveArg($param, $value, $fieldArgs, $info);
    }

    return \call_user_func_array([$resolver, $resolverMethod], $resolverArgs);
}
murtukov commented 5 years ago

@akomm we are talking here about the hydration feature, right? Why would you provide examples with a new resolver feature which is not even yet implemented? This is a topic of a separate issue.

Before we talk about how it would be done with the new resolver approach, we need to decide how it would work classic way, with expression functions.

akomm commented 5 years ago

If you want to make it work with expression, simply make this possible in expression:

createUser:
  type: User!
  resolve: '@=resolver("Mutation.createUser")' // note, no args passed

Now the resolver args are resolved for the resolver using same method as in the demonstrated code. Where is the problem? Works with expressions.

Also possible with new option for BC: guessResolverParameter: true|false (default: false)