Support readonly input classes with constructors

[oprypkhantc]

Currently, the best you can do when defining an update #[Input(update: true)] is to define regular properties without readonly:

#[Input(default: true, update: true)]
class UpdateDTO
{
	#[Field] public Layout $layout;
}

Then, if you try to

• mark the class/property as readonly, it fails trying to set it: Cannot initialize readonly property UpdateDTO::$layout from scope TheCodingMachine\\GraphQLite\\Utils\\PropertyAccessor. This can be fixed by using reflection to set the value instead of setting it directly using $object->$field = $value;
• mark the property as private (in case you instead want to provide a getter for it), it fails again with a similar error
• lastly, use promoted properties (constructor hydration) - it fails again with Parameter 'layout' is missing for class 'UpdateDTO' constructor. It should be mapped as required field..

Modern deserialization (Java - Moshi, GSON) libraries default to creating an instance without constructor and then hydrating the properties rather than calling the constructor, even if it exists. This makes sense because deserialization usually requires more complex logic than just create an instance, as is the case with #[Input(update: true)].

The reason I want to use promoted properties is because those inputs classes are used as value objects throughout the project, so it makes perfect sense for them to have a regular constructor. One more thing is unit testing - with a constructor I can do this:

$service->doSomething(new UpdateDTO(layout: Layout::BASIC))

So what would sound better to me is always using field-based hydration:

#[Input(default: true, update: true)]
readonly class UpdateDTO
{
	public function __construct() {
		#[Field] public Layout $layout,
	}
}

If constructor hydration is deemed important (at the very least for compatibility with older code that could break), we can instead make it opt-in:

#[Input(default: true, update: true)]
class UpdateDTO
{
	#[Hydrate]
	public function __construct() {
		#[Field] public Layout $layout,
	}
}

Thoughts on using reflection to set the property? And the hydration?

[oojacoboo]

I've run into this issue as well, as we use getters on input type classes for some minor data massaging, and would prefer that the properties are protected, even readonly.

We use the constructor for some specialized logic around validation in some input type classes. I think constructor hydration needs to remain for sure, especially for BC support.

Overall, the input type classes and hydration logic is sub-optimal IMO and didn't receive enough design planning. I'm certainly in agreement that reflection would be a better way to handle this and allow the PHP classes to be designed to work in a desired fashion, not in a way to satisfy GraphQLite.

I'm opposed to trying to add more annotations to this lib unless absolutely necessary though. I think we can do without the #[Hydrate] annotation.

[Lappihuan]

@oprypkhantc check this discussion arround constructor hydration for input types:
#466 (comment)

i think this should actually work but might be prevented by something else in the class. for example if there is something publicly writable constructor hydration is ignored iirc.

[Lappihuan]

additionally, using update marks all your types nullable for GQL but that does not translate to the PHP world which can cause issues especially around the constructor.
I think whats happening is you don't provide $layout in your mutation which ends up trying to instanciate your UpdateDTO which as a non-nullable layout property in the constructor with null.

i would assume this works:

#[Input(default: true, update: true)]
readonly class UpdateDTO
{
	public function __construct(
		#[Field] public ?Layout $layout = null
	) {
	}
}

uhm, editing this comment as it seems your whole constructor property promotion is wrongly set up.
the layout in not even part of the constructor parameters.

[oprypkhantc]

@Lappihuan Well yeah, publicly writable constructor is the problem for me, but I want it to remain, just not use it for deserialization. It doesn't make much sense.

Yes, the above should work, but for my use case Layout should not be nullable nor have a default value. graphqlite's docs say that I can check whether a property is initialized to check if it's been passed in the payload and that's exactly what I want: only update fields that have been passed in the payload. Otherwise with your solution I'll just end up updating $layout to null every time my mutation is triggered, even if $layout parameter wasn't passed.

And speaking of that, you've pointed out another issue with update: true - if you pass a null to non-nullable field for an update: true input, it throws an error: Cannot assign null to property UpdateDTO::$layout of type Layout. This makes sense from the Schema standpoint - the field is nullable, but from PHP's standpoint it doesn't. In this case it'd rather have graphqlite treat null value as a missing key.

I.e. separate optional and nullable concepts:

• nullable on the PHP side means that type is marked nullable and can accept null at all. update: true should not assume all fields are nullable on PHP side
• nullable on the Schema side means that type is marked GraphQL nullable and can accept null also. update: true should mark all fields as nullable on GraphQL side
• optional on the PHP side means that property is not always initialized with a value; sometimes, it's left in uninitialized state so that developers knows whether a value was passed or not. update: true should mark all fields as optional
• optional on the Schema side means is the same as nullable, because GraphQL doesn't separate these

This way we can have both nullable and non-nullable optional fields for update requests, similar to HTTP's spec for PATCH requests - i.e. only update fields that were passed in the payload, don't touch the rest. This would allow the following:

#[Input(default: true, update: true)]
class UpdateDTO
{
	// Appears as "Theme" in GraphQL scheme - nullable
	//   1. if value is passed in the payload, it's deserialized and assigned - property initialized
	//   2. if key isn't present in the payload, property is not assigned - property unitialized
	//   3. if `null` is passed, `null` is assigned to this property - property initialized
	#[Field] public ?Theme $theme;

	// Appears as "Layout" in GraphQL schema - also nullable
	//   4. if value is passed, it's deserialized and assigned - property initialized
	//   5. if key isn't present, property is not assigned - property uninitialized
	//   6. if `null` is passed, since we can't assign `null`, it's treated as if the key isn't present: property not assigned and uninitalized
	#[Field] public Layout $layout;
}

I believe I can already fix that for myself in userland, but I think this makes sense for graphqlite too.

Another approach would be to use wrapper Optional objects, but I'm not sure whether this is user-friendly enough. For our app we'll likely be using a combination of both:

#[Input(default: true)]
class UpdateDTO
{
	// Appears as "Theme" in GraphQL scheme - nullable
	/** @var Optional<Theme|null> */
	#[Field] public Optional $theme;

	// Appears as "Layout" in GraphQL schema - also nullable
	/** @var Optional<Layout> */
	#[Field] public Optional $layout;
}

This is very similar to what graphqlite does with uninitialized properties, but wraps values in Optional objects that have two methods:

public function isInitialized(): bool;
public function value(): mixed;

which allows developers to replace isset($data->layout) and (new ReflectionProperty($data, 'layout'))->isInitialized($data) checks with simpler $data->layout->isInitialized(), and get the value through $data->layout->value().

[oprypkhantc]

@oojacoboo As for the hydration/deserialization, maybe extract that to an interface and allow developers provide their own implementation? I have a serialization library in mind and I would have just used that.

[Lappihuan]

@oojacoboo As for the hydration/deserialization, maybe extract that to an interface and allow developers provide their own implementation? I have a serialization library in mind and I would have just used that.

I'm pretty sure that would make the whole middleware implementation much more difficult. but overall not a bad idea.

as for the update nullability discussion, we had this before: #474
and there is also a way to overwrite nullability with update: true describde here: #371

but overall, the constructor is complete PHP land, so you need to obay the PHP typesystem.

[oprypkhantc]

I was able to partly implement Optional in userland with a root type mapper and an input field middleware. I still have to init it with an "empty" optional as a default value because ->resolver callback that I set in my input field middleware is not getting called when value is missing due to this check.

A custom hydrator will solve this one too.

[Lappihuan]

i'm not sure but wouldn't setting the property readonly prevent you from using anything else but the constructor for hydration?

[oprypkhantc]

It shouldn't? I'm not a 100% sure either but I think you can set readonly properties through reflection just fine.

[oojacoboo]

Can we break this down into any actionable improvements?

@Lappihuan can we use reflection for setting the class properties? I think this could help with a few different issues.

[oprypkhantc]

Two issues need to be solved here:

• support setting readonly and/or private properties through reflection
• force using property-hydration even if constructor is present

The former is obvious and I can PR that change. The latter I'm not sure how to approach to both avoid a BC and avoid a new annotation being added. Maybe a parameter for #[Type(constructorHydration: false)]? This can then become false by default in the next major version and constructorHydration: true be deprecated if needed.

[Lappihuan]

What confuses me is, how would the instance be created if the constructor requires parameters.
How does Doctrine handle this?

[oprypkhantc]

The same way as unserialize() - they create an instance through reflection without calling the constructor at all: https://github.com/doctrine/instantiator/blob/d6eef505a6c46e963e54bf73bb9de43cdea70821/src/Doctrine/Instantiator/Instantiator.php#L116

This is normal practise for deserialization. On that note, maybe use doctrine/instrantiator to instantiate objects? It seems like a tiny dependency but it does the job.

[oojacoboo]

@oprypkhantc are you suggesting that you'd have a property defined with a #[Field] attribute, as well as being included in the constructor, and you'd want to avoid the constructor altogether? How would this play out with property promotion?

[oprypkhantc]

@oojacoboo Yes, I want to avoid constructor when deserializing data, but keep it for my tests & other needs. And based off the experience of other serializers this seems to be the default or even the only option.

Promoted parameters (i.e. property promotion) exist in reflection as both parameters (with isPromoted() reported as true) and class properties (also isPromoted() reported as true). So existing code that maps properties will "just work", aside from default values. This is where it gets a little tricky: default value of a promoted parameter is only accessible on the parameter, not on the property:

class Test {
	public function __construct(
		public readonly int $int = 123
	) {}
}

$property = (new \ReflectionClass(Test::class))->getProperty('int');

var_dump(
	$property->hasDefaultValue(),
	$property->getDefaultValue()
);

//

bool(false)
NULL

But this is trivial to fix - when a property was promoted, fallback to getting a default value from the parameter: https://github.com/good-php/serialization/blob/b145fbf644510e98ae18988e8d4aa5355ec12a18/src/GoodPhp/Serialization/TypeAdapter/Primitive/ClassProperties/Property/DefaultValueSkippingBoundClassProperty.php#L24

The rest will play nicely with existing code.