Annotated input type

[devmaslov]

The initial goal of this PR is to solve the issue about handling update without default values and with only properties passed by a user into the request. You can find my initial proposal here: #210.

This update brings the following features and changes:

• New annotation @Input is introduced on classes. It works the same way as @Type, but for input types. In combination with @Field annotation on properties you can describe input object type. When this input type is injected into a mutation during the request new instance of the class is created with only properties passed by a user. See examples below. The @input annotation has the following attributes:
  • name: Input name. If it's omitted it will be generated based on the class name. For example for class name User, UserInput type name will be generated.
  • default: Determines whether this type is default. The same idea as in @Factory annotation.
  • description: Just a description for documentation.
  • update: Determines if this input should be in "update" mode. For such input types all properties are optional and without default values.
• @Field annotation now can be used on class properties Feature Request: Field for property #246. If property is private or protected setter (for input) or getter (for output) is required. The type is guessed from property type (>= php 7.4) or from PHPDoc @var tag. Also description is extracted from PHPDoc and applied to the field. This annotation has a couple of new attributes:
  • for: @Field annotation can be now applied several times to the same method or property. Property for allows to determine to which type it should belong to. For example: @Field(for="User") or @Field(for={"CreateUserInput", "UpdateUserInput"})
  • description: If you require a different description per different type for the same field, you can now use a new property description of @Field annotation.
  • inputType: allows to set a specific input type on a field - similar as it works with outputType.
• Field middlewares such as @Security, @Right, @Logged etc. can be used on properties alongside with @Field.

In the initial proposal there was an idea about pre-loading an entity on update, so it would be possible to inject into mutation a loaded from DB User for example with populated data from the request. I tried to follow the idea where you put ID (or another field) as first param and payload as the second one. For example: updateUser(id: ID!, user: UserInput!), updateUserByEmail(email: String!, user: UserInput) or updateMe(user: UserInput) (for logged it user). Implementing this might be an overhead, because at least we need to dynamically add a property (if necessary) to a mutation based on which entity should be loaded and also loaders which would handle the pre-loading. I think this kind of thing can be done as a separate feature I'll gladly discuss any ideas about it. For now I suggest using this feature for describing DTOs as input types and inject them into mutations. Mutation can use a custom service or handle DTO to Entity transformation by themself.

Here's an example how it all works together.

UserInput - DTO for user entity with public fields. Totally the same can be achieved for private/protected fields with setters. Also you can omit it and describe inputs directly on User class.

/**
 * @GQL\Input(name="CreateUserInput")
 * @GQL\Input(name="UpdateUserInput", update=true)
 */
final class UserInput
{

    /**
     * @var string Username or login
     *
     * @GQL\Field()
     */
    public string $username;

    /**
     * @var string User email
     *
     * @GQL\Field(for="CreateUserInput") <-- Required for create and cannot be updated
     */
    public string $email;

    /**
     * @var string User's password
     *
     * @GQL\Field(name="password") <-- In DTO you would probably just name this field "password", but I just want to show here that the name of the input field also can be different from the property name. 
     */
    public string $plainPassword;

    /**
     * @var DateTimeInterface|null Birthday
     *
     * @GQL\Field(inputType="Date") <-- For example we have a custom scalar type Date without time.
     */
    public ?DateTimeInterface $birthday; <-- This field is nullable, so it's optional even for CreateUserInput.
}

User - actual entity.

/**
 * User entity.
 *
 * @GQL\Type()
 */
class User
{
    /**
     * @var string Username or login
     *
     * @GQL\Field()
     */
    private string $username;

    /**
     * @var string User email
     *
     * @GQL\Field()
     */
    private string $email;

    /**
     * @var DateTimeInterface|null Birthday
     *
     * @GQL\Field(outputType="Date")
     */
    private ?DateTimeInterface $birthday = null;
}

And mutations. Instance of UserInput will be created with populated data from a request and injected for "$user" argument. With @Assertion annotation those inputs can be validated in Symfony.

    /**
     * @GQL\Mutation()
     * @GQL\UseInputType(for="$user", inputType="CreateUserInput!")
     *
     * @Assertion(for="$user", constraint=@Assert\Valid)
     *
     * @param UserInput $user
     *
     * @return User
     */
    public function createUser(UserInput $user): User
    {
        // Create a new entity from a UserInput object, save and return it
    }

    /**
     * @GQL\Mutation()
     * @GQL\UseInputType(for="$user", inputType="UpdateUserInput!")
     *
     * @Assertion(for="$user", constraint=@Assert\Valid)
     *
     * @param string    $id
     * @param UserInput $user
     *
     * @return User
     */
    public function updateUser(string $id, UserInput $user): User
    {
        // Load user by id and populate it with data in UserInput.
        // You can create some custom mechanism of tracking what properties were set to the $user
        // or check which properties are initialized (PHP >= 7.4)
        // or just loop through $user - only initialized properties will be iterated.
        // In such case if user just wants to update one field, email for example - only it will be updated, without default values on other ones.
    }

I must say that the code is not covered with unit tests yet. First I'd like to hear some opinions about the implementation and the concept in general.

Also there'are few not critical but still Breaking changes.
$refMethod argument has been replaced without a type hint so it would be possible to pass either ReflectionMethod or ReflectionProperty in such public methods:

• AnnotationReader::getMiddlewareAnnotations
• InvalidPrefetchMethodRuntimeException::methodNotFound
• CannotMapTypeException::createForMissingPhpDoc
• CachedDocBlockFactory::getDocBlock
• RootTypeMapperInterface methods and all implementing it classes - this is my biggest concern

[moufmouf]

Woah, this is a huge amount of work! Thanks so much!

I'm a bit busy right now, but I'll definitely try to do an extensive code review by the end of the week.

[gulien]

This looks amazing 👍

I wonder if we could add a "partial update" mechanism in the mix (see graphql-dotnet/conventions#128 (comment)).

[devmaslov]

@moufmouf, thanks, looking forward to it.

@gulien the partial update is possible with this feature. The idea is that input object (such as UserInput DTO) is populated only with data that user passes to the graphQL request. Let's say I'm sending this query:

mutation {
  updateUser(id: "user-id", user: {
    username: "NewName"
  }) {
    id
    username
  }
}

When it's executed only username will be set to the instance of UserInput. It means that you can implement a way of tracking which properties has been set and if it's a DTO, properly transform it to a needed entity. For instance the easiest way if you're using PHP 7.4 and your DTO has all properties public and typed (as in my example above) - simply loop through the object with foreach and set needed properties into a new object. Here's an example for update user mutation:

    /**
     * @GQL\Mutation()
     * @GQL\UseInputType(for="$user", inputType="UpdateUserInput!")
     *
     * @Assertion(for="$user", constraint=@Assert\Valid)
     *
     * @param string    $id
     * @param UserInput $user
     *
     * @return User
     */
    public function updateUser(string $id, UserInput $user): User
    {
        $entity = // some logic to load existing user by $id
        foreach ($user as $key => $value) {
            // Symfony has property accessor service which allows to set values into objects. Let's use that for example:
            $this->propertyAccessor->setValue($entity, $key, $value)
        }
       
        // Save and return $entity
    }

It won't work though if you property doesn't have a type. In this case such property has null as a default value by PHP. I'm sure there're plenty of other ways to achieve this as well.

[moufmouf]

First of all, thanks very very much. This is by far the most extensive PR I received on GraphQLite and it is overall very good!

I wish I could spend more time reviewing this in depth but I really won't have time to seriously work on in before next Tuesday. That being said, here is a first review.

I really like the direction you are taking and I'm definitely looking forward to merging this when it is ready (and helping you working on it as time permits)

Regarding your concerns about breaking changes:

I was pretty sure I had applied a @unstable annotation to all extensions point (https://graphqlite.thecodingmachine.io/docs/semver.html)

It seems I forgot to add it on RootTypeMapperInterface :(

Yet, the documentation states:

As a rule of thumb:

If you are a GraphQLite user (using GraphQLite mainly through its annotations), we guarantee strict semantic versioning
If you are extending GraphQLite features (if you are developing custom annotations, or if you are developing a GraphQlite integration with a framework...), be sure to check the tags.

Which is kind of a warning that if you are implementing GraphQLite interfaces, you should probably target a specific minor version of GraphQLite.

My gut feeling is that it is ok to introduce those breaking changes. Implementing RootTypeMapperInterface if probably not that common.

I put other comments directly in the code.

I did not try yet to run your code :)

We will definitely have to add unit/integration tests (and documentation) before merging this, but this looks very very promising. Thanks a lot!

[moufmouf]

Open question: You could have a @field annotation on a @type class with an "inputType" property declared.
Did you consider splitting @field into 2 classes: @Field and @InputField ?

[devmaslov]

I thought about this and came to a decision that it would be more convenient to describe fields for both type and input within one annotation. You can apply @Input and @Type annotation for the same class and in most cases you'd want to mark the same properties as fields.

With two annotations you'd need to do this twice which is kind of redundant:

/**
 * User entity.
 *
 * @GQL\Type()
 * @GQL\Input()
 */
class User
{
    /**
     * @var string Username or login
     *
     * @GQL\Field()
     * @GQL\InputField()
     */
    private string $username;

    /**
     * @var string User email
     *
     * @GQL\Field()
     * @GQL\InputField()
     */
    private string $email;

    /**
     * @var DateTimeInterface|null Birthday
     *
     * @GQL\Field()
     * @GQL\InputField()
     */
    private ?DateTimeInterface $birthday = null;
}

With one @Field annotation it looks more simple and clean:

/**
 * User entity.
 *
 * @GQL\Type()
 * @GQL\Input()
 */
class User
{
    /**
     * @var string Username or login
     *
     * @GQL\Field()
     */
    private string $username;

    /**
     * @var string User email
     *
     * @GQL\Field()
     */
    private string $email;

    /**
     * @var DateTimeInterface|null Birthday
     *
     * @GQL\Field()
     */
    private ?DateTimeInterface $birthday = null;
}

You need to separate input fields from output you can use for attribute.

But here it is the problem with inputType and outputType attributes. If I want to set the same type for both cases and the same field - which is 99% use case I think, I'd need to do this: @GQL\Field(inputType="Date", outputType="Date").

I considered alternative option - deprecate outputType and add another attribute type - which would suit for both input and output types. @GQL\Field(type="Date") looks much better. What do you think about that?

[moufmouf]

Ok, I understand your will to not repeat Field and InputField annotations. It makes sense.

I'm trying to assess if there are any corner-cases that might bother us in the future.

Let's imagine this:

/**
 * User entity.
 *
 * @GQL\Type()
 * @GQL\Input()
 */
class User
{
    // ...

    /**
     * @var string Username or login
     *
     * @GQL\Field()
     */
    private string $password;

    public function setPassword(string $password): void {
    	$this->password = $password;
    }
}

In this case, we have a @field annotation on the property, a setter, but no getter.

What should happen in your opinion? Should GraphQLite throw an error? (something like "Output field "password" is not reachable via a getter, please add a "getPassword" method). Or should we silently put only an input field and no output field?

(I'm trying to put as much attention as possible to all the error messages a user can have and to have meaningful error messages)

[devmaslov]

It's a good question.

Or should we silently put only an input field and no output field?

I don't think silently hiding fields is the way to go. At some point it might confuse developers, especially with models containing a lot of properties, why their field is not working even though there's a @Field annotation.

I like the idea of throwing an error. At the moment if you try accessing such field PHP will throw an error like Cannot access private property .... It would be much better to display a message as you suggested to give more accurate explanation of the problem.

[moufmouf]

Since fields can be set in the constructor, wouldn't it make sense if the @Field annotation could be declared in the constructor too? (the problem being that in @Field(for=...), the for is referring a typename right now and not a variable name (as in the rest of the application).

Also, something is coming to my mind: with the @Factory annotation, you can use many additional annotations like @Autowire, @UseInputType, etc... since a constructor is a kind of default factory, wouldn't it make sense to reuse a part of the code used to instantiate objects declared with the @Factory annotation?

[devmaslov]

I'm not sure if we should overload constructor with such logic. It's job is only to instantiate an object with a given parameters. Current implementation assumes that required constructor parameters have defined fields on properties with the same name in the class. When object is instantiated users payload is mapped to the constructor parameters.

For example:

/**
 * @GQL\Input()
 */
class User
{
    /**
     * @var string Username or login
     *
     * @GQL\Field()
     */
    private string $username;

    /**
     * @param string $username
     */
    public function __construct(string $username)
    {
        $this->username = $username;
    }
}

In this case we definitely know that input type has a field called username and it's required. There's a constructor parameter with the same name so when the object is instantiated value from the request payload will be passed to it. For this createInstance() function is responsible - this is kind of default factory, which knows how to map data to a constructor and instantiate an object.

I agree that it would be nice to provide a custom and flexible way of creating instances before they are populated with new data. At the beginning I thought of reusing @Factory. We could add a new attribute @Input(init="FactoryName"). But after that I realized that it's not very flexible. For example it would be nice to reuse the same input type in different mutations, but pre-load should be done with different identificators. For example:

updateUser(id: ID!, user: UserInput!)
updateUserByEmail(email: String!, user: UserInput)
updateMe(user: UserInput)

Considering this we'd need to create 3 different input types with the same fields but different init attributes.

As an alternative this init attribute could be added to @UseInputType annotation. This way we choose what type should be used and how the object should be initialized for each particular mutation. Also extra parameters such as "id: ID!" or "email: String!" described in factories could be mapped not to input types, but to mutations directly. I hope it won't be confusing. Here's an example of what I mean:

/**
 * @GQL\Type()
 * @GQL\Input()
 */
class User
{

    /**
     * @var string
     *
     * @GQL\Field(for="User")
     */
    private string $id;
    
    /**
     * @var string
     *
     * @GQL\Field()
     */
    private string $username;

    ...
}

...

class UserFactory
{

    /**
     * @Factory(name="LoadUser")
     * 
     * @param string $id
     * 
     * @return User
     */
    public function loadUser(string $id): User {
       // ... Load user from DB
    }
}

...

class UserController
{

    /**
     * @Mutation(name="UpdateUser")
     * @UseInputType(for="$user", inputType="UserInput!", init="LoadUser")
     * 
     * @param User $user
     * 
     * @return User
     */
    public function updateUser(User $user): User {
       // ... $user is loaded from DB and populated with given data. Just validate and save it. 
    }
}

Because @UseInputType points to "LoadUser" factory with init attribute, another field id will be added to the mutation as loadUser() function requires it as a parameter. So the request mapping will be like this:

updateUserByEmail(id: String!, user: UserInput)

My only concern is that it might be not obvious that by adding init attribute your mutation signature is split across different methods - actual mutation method and supportive "init" factory. It might be a bit confusing or not obvious...
What do you think about this idea? Do you see other ways to achieve this?

[moufmouf]

About the "init" idea: I would even go one step further. The concept of @factory is hard to explain and I'm getting a lot of people confused with it.

Since quite often, a factory is used by only one mutation, we could even imagine something like this:

class UserController
{

    /**
     * @Mutation(name="UpdateUser")
     * @UseInputType(for="$user", factory="UserFactory::loadUser", inputName="UpdateUserInput")
     * 
     * @param User $user
     * 
     * @return User
     */
    public function updateUser(User $user): User {
       // ... $user is loaded from DB and populated with given data. Just validate and save it. 
    }
}

and this might actually be easier to understand for many people.

[devmaslov]

Is UserFactory just some service with loadUser function and has nothing to do with @Factory annotation?

I considered similar approach, but in this case you'd need to specify an absolute class name. For example: @UseInputType(for="$user", factory="App\Services\UserFactory::loadUser", inputName="UpdateUserInput"). It will be easier to understand and use. Not sure only if it looks good in the code though 😄

[devmaslov]

@moufmouf
Thanks a lot, I am happy to see you feel that way 😃
About the review - no worries, take your time and thank you for the first feedback. I'll respond shortly 😊

My gut feeling is that it is ok to introduce those breaking changes. Implementing RootTypeMapperInterface if probably not that common.

I also think that it's be fine. For the ones who currently have this this interface implemented, it should be easy to spot that something is wrong, since PHP will throw the exception at compilation time.

[moufmouf]

@devmaslov just a quick message to let you know I started applying some changes in your code (right now, it's only applying cs-fixes to have a green CI) Be sure to pull the code from your branch before working on it again!

[moufmouf]

@devmaslov I started adding tests here: bb69621

It is just a simple test to see if @field annotation works correctly in properties.
The result is quite suprising. The new test works, but adding the @field annotation on properties seems to have a side-effect on inheritance. In my test, I'm adding fields on the Contact class and suddenly, I can't access fields on the User class (that extends Contact) anymore.

See:

https://travis-ci.com/github/thecodingmachine/graphqlite/jobs/353951488#L361

[devmaslov]

@moufmouf Looks like this issue with inheritance appears when you property is defined as private in parent class. For public and protected it should work fine. I'll take a look into it. And thanks for adding unit tests, I'll make sure they pass after the fix.

[devmaslov]

@moufmouf Fixed the problem. The reason for it was different, but still I noticed interesting behavior with private properties.
Consider the following example:

/**
 * @Type()
 */
class Contact {
    /**
     * @Field()
     * @var int
     */
    private $age = 42;

    /**
     * @return int
     */
    public function getAge(): int
    {
        return $this->age;
    }
}

/**
 * @Type()
 */
class User extends Contact {
}

In this case field name appears in User type without any problems. But if you remove @Type annotation from Contact it will disappear from User - because there's no GraphQL type to inherit it from and $refClass->getProperties() doesn't return parent's private properties. I think it should be fixed and field should appear without parent being @Type - they still can be available via getters.

[oojacoboo]

@devmaslov just a friendly ping to let you know that this looks awesome and can't wait to get it merged in :)

[oojacoboo]

@devmaslov @moufmouf any chance we can get this over the finish line soon before it becomes too stale?

[devmaslov]

I'd like to move this forward as well. @moufmouf, what should be our next steps here? Is there anything that should be adjusted before we dive into covering all of this with unit tests? There're a couple open threads above ⬆️

[devmaslov]

@moufmouf it's done. CI is passing 🎉

[devmaslov]

Hey @moufmouf,
Just a kind reminder to review this PR 🙂

[moufmouf]

Hey @devmaslov ,

I'm definitely not forgetting this PR, I've just been overwhelmed lately, but I'll try to have a proper look at it and validate it this week!

[moufmouf]

Hey @devmaslov ,

The amount of work you put into this PR is astounding. The doc is perfect, unit tests are also great. I have nothing to add. Let's merge this! 👍

Also, I realize I have been very long to give feedback. I've had an awful lot of work lately (not directly related to GraphQLite alas) and I certainly need some help maintaining this library. Since this PR clearly shows you understood GraphQLite, would you like to be promoted as a maintainer of GraphQLite?

[oojacoboo]

@devmaslov Say yes :)

[moufmouf]

@oojacoboo the offer also extends to you ❤️
You have been giving many feedbacks to the community in general and if you want to be granted maintainer rights, just tell me 👍

[devmaslov]

@moufmouf, @oojacoboo Thanks a lot for your input and feedback 😄 Happy to see this branch merged 🥳
I'll be happy to help maintaining the library 😃

[moufmouf]

Cool!
@devmaslov Do you happen to have a Twitter account? (so I can mention you in the upcoming Tweet?)

[devmaslov]

@moufmouf no, I don't have one 🙂

[oojacoboo]

@moufmouf I'm happy to continue to help out - discuss issue tickets, help with some pull requests, etc. I'm fairly familiar with some of the internals. We do use this lib pretty heavily and will be using it much more so going forward.

[moufmouf]

@oojacoboo I'll take that for a yes :D

[TamasSzigeti]

Sorry to intrude here, really glad that the responsibility is now shared, thanking each of you. May I call attention to the graphqlite-bundle as well, I have a tiny humble PR waiting there (:

[Lappihuan]

@devmaslov this looks awesome! may i ask what is holding back the release of this?

[oojacoboo]

@Lappihuan we're mostly waiting for release instructions from @moufmouf

[MattBred]

Any movement on 4.2 / instructions @moufmouf ?