Handling update mutations and default property values on input types

[oojacoboo]

This is more of a general questions, but I'm unable to find a good resolution to this issue. Consider the following mutation:

updateProduct(input: {
    id!, 
    name, 
    price 
}) { id }

In this updateProduct mutation you're requiring an UpdateProductInput type which has a non-nullable id property and 2 nullable properties, meaning name and price can be passed with the request or not.

Now, with the factory and UpdateProductInput, I'm unsure of how to handle these in the situation where one or both of, name and price could be nullable. This is because, with PHP, we basically need to make these values default to null, due to the factory method being called. Or so I think. Does GraphQLite, default arguments of the method signature to null if they are not passed in the request?

With PHP 7.4, if we used setters in a factory, we could possibly make use of the uninitialized state with typed properties, or possibly, if the factory used "overloading" on the input type.

At the end of the day, when using input types, which is the recommended way of designing your mutations to add flexibility and mitigate future BC issues, you have no real way of knowing if the property, with a null value, was explicitly set, or defaulted. When taking this to a persistence layer, this clearly presents issues.

I'd love some advice or feedback on this as I'm trying to determine the best way of approaching this issue. Currently I'm thinking it may be best to rely on the uninitialized state for properties within the input types. If the raw request object was accessible, you'd know if the property was passed, and therefore intended to be persisted.

[moufmouf]

Hey Jacob,

Very good question.
Usually, in PHP, it is considered a bad idea to make a difference between "not set" and "null" (the isset function considers "not set" and "null" to be the same thing).

However, it seems to be different in GraphQL. Reading your issue, I started looking what others are doing. I found this: graphql/graphql-spec#83

So clearly, the GraphQL spec was edited to allow making a difference between "setting a value to null" and "not setting a value at all".

At the end of the day, when using input types, which is the recommended way of designing your mutations to add flexibility and mitigate future BC issues, you have no real way of knowing if the property, with a null value, was explicitly set, or defaulted. When taking this to a persistence layer, this clearly presents issues.

Currently, there is indeed no way to make a difference.

/**
 * @Factory
 */
function create(string $id, ?string $name = null, ?string $price = null) {}

In PHP, there is no way to know if "name" is null (because is was explicitly set) or undefined (because it was not set at all).

I see 2 solutions to this problem.

The easiest one: we could add an annotation that injects the variables array passed by the client directly in the method. Code would look like this:

/**
 * @Factory
 * @InjectVariables(for="$variables")
 */
function create(array $variables, string $id, ?string $name = null, ?string $price = null) {
    if (array_key_exists('name', $variables)) {
        $this->setName($name);
    }
}

This is actually easy to do using "custom argument resolving"

The second solution would be to add a new way to create input types. Maybe by adding a new InputType annotation on the class level and a new InputField annotation on the setters.

Something like:

/**
 * @InputType
 */
class Product {
    public function __construct(string $id) {
        // ...
    }

    /**
     * @InputField
     */
    public function setName(string $name) {
        // ...
    }

    /**
     * @InputField
     */
    public function setPrice(string $price) {
        // ...
    }
}

Of course, that would require much more work. And this approach could only work with one input type (if the same class is mapped by several input types, we could not use this as there is only one constructor).

What do you think about those 2 solutions?

[oojacoboo]

@moufmouf So, I had some conversations with other GraphQL developers yesterday about this topic and a few other solutions were presented.

But first, I'd like to point out my objection to adding more annotations to this library. In fact, I'd like to see more of them cleaned up, unified, and possibly incorporated into actual PHP going forward. I chose this library because of the power that annotations can provide, particularly over a domain model. But some of the other annotations provide little or no benefit over composed PHP, especially since the classes on which they're applied, are generally special purpose (read: GraphQLite) classes anyway. The annotation parsing in development is quite slow. We're already using caching in development and clearing cache manually, due to speed concerns. That's not to mention the other issues annotations provide. I realize this is a tangent, but wanted to mention it while the topic was at hand.

That being said, 3 other solutions to this issue were presented that I think can effectively solve the issue, depending on your use case.

• Require token values for "clearing" (nullifying) a particular property. So, instead of passing null on a nullable property to set it to null and "clear" the value, you'd pass a specific token that the server would interpret accordingly. The issue with this approach is that it doesn't work well across all types. It could possibly be handled with a custom "Nullable" input type.
• Add more mutations, such that all values of a mutation are always required. By doing this, whenever you have a null value, you can be assured of it's intention. This is a pretty good approach, in general, but could become very verbose, and with GraphQLite, could mean adding a LOT more boilerplate code.
• Use specific "clearing" mutations. This is my favorite. It's very explicit in intent and client friendly and effectively solves the issue.

Maybe it'd be best to just leave this issue open for now and revisit, and/or allow others to contribute that might be faced with this same design challenge.

[oojacoboo]

@moufmouf I'd really like to resurface this discussion. I feel that this is core enough to the overall functionality of an API that it really shouldn't be ignored. This is presenting issues for us, particularly where someone wants to "delete" an input/string on the client side and persisting that update.

Did you have any thoughts on this?

[moufmouf]

Hey @oojacoboo ,

I do have a lot on my plate lately (especially with all the Covid 19 lockdown going on...)
Your issue is completely valid and I've been thinking a bit about it, but I feel I don't have enough time right now to treat the problem correctly.

I think the idea about adding a @InjectVariables(for="$variables") annotation as exposed in january could be done quite easily, but I understand you are not fond of it (more annotations). Do you have any other suggestion?

[oojacoboo]

@moufmouf yea, I hear you; re: COVID-19. I was hoping we could, at least further this conversation and come to a plan of attack. I might even be able to submit a pull request if we can get something workable.

You're right that I'd much prefer to not rely on another annotation, but if that's the best solution, so be it. I do think it's looking like the best option right now. I don't particularly like the idea of having to put a special parameter into each Factory method signature though. Were you thinking to use the request object directly to inject this param? What about creating a singleton to access the request params which could have some additional helper methods? Or, what if we created a base Factory class that could be extended if someone wants to take advantage of this functionality?

[devmaslov]

Hey guys,
@moufmouf, @oojacoboo

I faced with the same problem. Implemented "$variables" solution, but to my opinion the code looks a bit ugly, especially when my input type has 10+ properties:

• In this case you have too many arguments within one factory function which is not a good practice.
• Every setter should be wrapped with an if statement.

The main question here, why should I do this routine when I just need to basically map arguments array into a class instance?

I think second solution described here could simplify this process a lot: #210 (comment)

I'd allow using InputField on properties as well - setters can be guessed if needed. The reason is that in such cases DTOs are used often and with public properties, thus no setters for adding this annotation to.

It would be useful to add an option for pre-loading entity, before populating it with argument values. For example we could add an option called loadFactory to InputType annotation. So instead of creating a new instance this factory will be used for loading an entity from DB. It can be really useful for update operations without DTOs - updating Doctrine entity for example.

InputType should be allowed to be applied several times for the same class. The following example should generate 2 input types:

/**
 * @InputType(name="CreateUserInput")
 * @InputType(name="UpdateUserInput", loadFactory="loadUser")
 */
class User

Some useful options for InputField:

• should be possible to apply it for all declared types within a class or only for particular ones.
• option to make field required or not.
• change property name.

For example I have the following fields in a User entity:

• username: is required on create, but optional on update
• email: is required on create, but hidden on update - user cannot change email
• birthday: optional for all input types
• plainPassword: required on create, but optional on update. Also should appear as "password" in graphQL.

it can look like this:

/**
 * @InputField(for="CreateUserInput") <-- automatically set as required, because property is not nullable
 * @InputField(for="UpdateUserInput", required=false)
 */
public string $username;

/**
 * @InputField(for="CreateUserInput")
 */
public string $email;

/**
 * @InputField()
 */
public ?DateTime $birthday;

/**
 * @InputField(for="CreateUserInput", name="password")
 * @InputField(for="UpdateUserInput", name="password", required=false)
 */
public string $plainPassword;

Also as an option, instead of creating a new annotation InputField we could just expand existing one - Field.

This is something really missing for me in this lib and it could simplify the app code very much.
What do you think about that? Makes sense?

[oojacoboo]

I guess as long as there is a way to still build a custom factory, if needed, this solution would work as well and maybe save some time having to write factories, which, admittedly, gets tiresome.

P.S. - All these annotations are going to look fabulous with the new PHP8 syntax for attributes <<InputField()>>. I really hope IDE's clean up the way they're going to look.

[moufmouf]

Hey @devmaslov,

Thanks a lot for your detailed proposal.
Actually, I love it!

It requires quite a big amount of work, but it is worthwhile in my opinion. I completely agree that factories do not offer the best developer experience and something needs to be done about it. Your solution solves this problem elegantly. So 👍

I probably won't have any time to look at this until June so if any of you feels brave/fool enough to tackle the problem, do not hesitate! Otherwise, I'll look into it as soon as I can.

[devmaslov]

Thanks @moufmouf,

Feeling brave for now 😄, so this weekend I'll start working in this direction

[oojacoboo]

@devmaslov's amazing work got this done with #269