Feature Request: We should be unsetting default values from "update = true" input types

[MattBred]

According to the docs (https://graphqlite.thecodingmachine.io/docs/input-types) when you use an update=true input type, it removes default parameters:

Note that update: true argument for UpdateUserInput. It should be used when input type is used for a partial update, It makes all fields optional and removes all default values

Inspecting the GraphQL schema proves that this is the case - default parameters exist for the "create" input type, and do not exist for the "update" input type.

However, the actual PHP "update" input object still has the default parameter.

Full example:

/**
 * @GraphQL\Input(name="CreateBlogPostInput", default=true, update=false)
 * @GraphQL\Input(name="UpdateBlogPostInput", update=true)
 */
class BlogPostInput {
	/**
	 * @GraphQL\Field
	 */
	public string $title;

	/**
	 * @GraphQL\Field
	 */
	public string $content;

	/**
	 * @GraphQL\Field(inputType="Boolean")
	 */
	public bool $published = true;
}

The GraphQL schema for CreatePostInput is generated as:

title: String!
content: String!
published: Boolean = true

The PHP object will default $input->published to true if it's not passed in.

---

The GraphQL schema for UpdatePostInput is generated as:

title: String
content: String
published: Boolean

However, the PHP object will still default $input->published to true if it's not passed in.

I believe that for the Update input, we should unset($input->published) in this case, which would make it null/not initialised depending on the PHP version.

[MattBred]

If people agree, and can point me to the area, I can have a poke at implementing this.

[oojacoboo]

@MattBred absolutely. I don't think this functionality is widely used yet. There were many things holding people back from really implementing the input types using #[Input].

That said, I'm confused as to why published would be false for the CreatePostInput. I'm assuming you meant true?

Regarding an "update" input type, can we never hydrate the property in the first place? Also, I'm not sure how property hydration is handled when there is a constructor for "update" inputs. That's a scenario we should look into. The type resolver typically attempts to construct the object with field/arguments first, then hydrates the remaining properties.

At any rate, I agree that we shouldn't be defaulting to false and if the field isn't provided with an "update" input type, it shouldn't be initialized.

[MattBred]

@oojacoboo

Whoops, yeah I meant true - just edited my post to fix that.

Totally makes sense to not even hydrate the field in the first place - I'm not sure how the whole flow of the input class gets created, but that would be the most ideal solution for sure.

Kind of a side note, but we have been using the pattern of public typed properties on our input types with no constructors quite well. The only downside we've come across is that if you can have a legitimate null value, it's difficult to check if it's been passed in with PHP as it doesn't have a native is_initialized type of function, and will throw an error if you check an un-initialized property.

To get around this, we've developed an ugly is_initialized function ourselves that takes in a closure, tries executing it, and catches the exception & returns true/false. e.g.

function is_initialized(\Closure $check): bool
{
	try
	{
		$check();
	}
	catch (\Error $e)
	{
		// str_contains() not available until PHP8+ use strpos() instead.
		if (strpos($e->getMessage(), 'must not be accessed before initialization') !== false)
		{
			return false;
		}
		throw $e;
	}
	return true;
}


$fieldSet = is_initialized(fn () => $input->published);

The reason we don't use the reflection (https://www.php.net/manual/en/reflectionproperty.isinitialized.php) is for proper type hinting/auto-completion. I dislike passing string names for methods/properties etc.

[oojacoboo]

@MattBred checkout TheCodingMachine\GraphQLite\FieldsBuilder.

[Lappihuan]

I've completely changed this code in #466 i think i should be able to fix this if i have a clear idea of what is actually expected instead.

[Lappihuan]

@MattBred the update feature is inspired by this: graphql-dotnet/conventions#128
And some additional toughts of the contributor of this feature: #269 (comment)

[oojacoboo]

@Lappihuan the issue is that bool fields are being hydrated as false by default. Ideally, these fields are never hydrated/initialized. We can't modify the nullable state of a property in the class, so a not-initialized state is about the best we can do on an optional update for the field, if not provided.

In my previous comment, I was saying we also need to check the constructor and if this bool is provided and determine how best to handle this as well. Overall, a review on bool behavior needs to be done with regards to update input types.

[MattBred]

@oojacoboo In my reply I clarified that it's not actually hydrating false by default, its hydrating the default value set in the class (in my case true as public bool $published = true;)

But otherwise, correct, we shouldn't even initialize them, or if we can't control that (as it's a default PHP property) we should unset them.

[Lappihuan]

I'm wondering why you even set it in the first place then.

[MattBred]

@Lappihuan Default values for the "Create" input types.

When updating, if a user doesn't pass in a value, it shouldn't be set so we can detect if they even passed something in. It doesn't make sense to default values on "Update" input types.

[Lappihuan]

@MattBred i doubt this is viable in any way.
Imagine a private property with a default value.

It doesn't make sense to default values on "Update" input types.

graphqlite has not full controll over the objects its working with and it's making sure its not touching the properties in update mode. There are enought ways to solve this without messing with the properties in a unnatural way.

Why not have two properties for this kind of case?

	 /**
	 * @GraphQL\Field(for="CreateBlogPostInput", name="published")
	 */
	public bool $createPublished = true;

	 /**
	 * @GraphQL\Field(for=UpdateBlogPostInput", name="published")
	 */
	public ?bool $updatePublished;

or with #466 you can use two diffrent setter methods for the same internal property.
actuallly, simple setters are supported already.

[MattBred]

@Lappihuan A private property can't be directly used as a GraphQL field in my example - it would need a constructor/setter in order to be used as a field, which I'm trying to avoid.

My example is for public properties where you directly attach a field annotation, and GraphQLite automatically fills it in.

Your workaround would work, but its ugly, and the application code now needs to determine which PHP property to use. It's not a viable answer IMO.

This request is purely for public properties with field annotations, avoiding the use of constructor/setters entirely.

I don't think it's unnatural at all. I think its wrong that it doesnt unset them, yet shows the Schema as not having a default value for the Update inputs.

[oojacoboo]

@MattBred I'm kind of falling in the middle on this one. I thought you were saying that for update input types the FieldsBuilder was defaulting tofalse for bool properties. I now understand that you meant true for both input types.

I think, first and foremost, we should maintain the native PHP handling of objects and not have GraphQLite do any funny business. Unsetting this property goes against the expected result from native PHP instantiation.

That said, I can see the need for having a default for create and not update. Attempting to avoid getters/setters and constructors, while cool, isn't a valid enough reason to do something else. Input type classes should try to use native PHP functionality to solve the problem whenever possible.

@Lappihuan's suggestion of using a different setter for each input type seems like the most ideal way of handling this. You think you could checkout #466 and give it a shot?

@Lappihuan if I'm understanding you correctly, the following will be supported?

/**
 * @GraphQL\Input(name="CreateBlogPostInput", default=true, update=false)
 * @GraphQL\Input(name="UpdateBlogPostInput", update=true)
 */
class BlogPostInput {

    /**
     * @GraphQL\Field
     */
    public string $title;

    /**
     * @GraphQL\Field
     */
    public string $content;

    protected ?bool $published;

    /**
     * @GraphQL\Field(for="CreateBlogPostInput", name="published")
     */
    public function setPublishedForCreateBlogPostInput(bool $published = true) {
        $this->published = published;
    }

    /**
     * @GraphQL\Field(for="UpdateBlogPostInput", name="published")
     */
    public function setPublishedForUpdateBlogPostInput(?bool $published) {
        $this->published = published;
    }
}

[Lappihuan]

Jep, it is in #466
Currently setter Methods ignore the Field annotation.

@MattBred you can try #466 by changing your require in the composer.json to:

"thecodingmachine/graphqlite": "dev-bring_input_setters_on_par_with_type_getters as 5.0.x-dev"

and adding this to the repositories field:

        "graphqlite": {
            "type": "git",
            "url": "https://github.com/Lappihuan/graphqlite"
        }

[MattBred]

I can appreciate not wanting to change PHP class properties as that might be too "magic".

The dedicated setter per input type might be a valid compromise, I will need to experiment, thanks!