Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Type-safe solution for "undefined" input values #726

Open
Eliasyoussef47 opened this issue Jan 28, 2025 · 11 comments
Open

Type-safe solution for "undefined" input values #726

Eliasyoussef47 opened this issue Jan 28, 2025 · 11 comments

Comments

@Eliasyoussef47
Copy link

Eliasyoussef47 commented Jan 28, 2025

I'm looking for a type-safe way to represent "undefined" (similar to TypeScript) values in input types. I define "undefined" values as the lack of provided value (by the client) for a specific class field (PHP calls these properties) in an input type (class).

I've read some issues on this matter already. An example of those is: #210. But none of the proposed solution is good enough.

Desired behavior

I want GraphQLite to resolve/map/set the value of an input class field (or constructor parameter) to a custom type made by me, when no value was provided for that field by the client.

Type-safe

With "type-safe" I mean a that the type system of PHP communicates to and guarantees the developer of:

  1. The type of value that is saved in a class field. That is, the PHP type annotation communicates that a field might not have been provided.
  2. The value that will be returned when the getter is called. That is, no other type than the declared types is returned.
  3. That no obscure or unintended behavior will happen. That is, no TypeError should be thrown because a developer simply called a getter.

No hackiness

I don't want to use isset() or other hacky things to first check if a field is initialized.

Nullability

The type of the field in the input class can still be nullable and undefined-able (not a word but you understand what I mean).
In TypeScript, this would be expressed as string|null|undefined.
For completeness sake, it would be nice to have null|undefined as a possibility (even though it sounds weird). EDIT: nevermind. This library can't map a field with only null as the type, so this shouldn't be possible either.

"Undefined" type

After some research, I have determined that it would be best to use a PHP enum as a custom type to represent undefined values (I can elaborate on this conclusion if needed).

enum Undefined
{
    case UNDEFINED;
}

Example of desired outcome

An example of an input type:

#[Input]
readonly class MayHaveUndefinedValueObject
{
    #[Field]
    private string $name;

    #[Field]
    private string|null|Undefined $description;

    #[Field]
    private bool|Undefined $important;

    /**
     * @param string $name
     * @param string|null|Undefined $description
     * @param bool|Undefined $important
     */
    public function __construct(
        string $name,
        string|null|Undefined $description,
        bool|Undefined $important
    ) {
        $this->name = $name;
        $this->description = $description;
        $this->important = $important;
    }

    public function getName(): string
    {
        return $this->name;
    }

    public function getDescription(): string|null|Undefined
    {
        return $this->description;
    }

    public function getImportant(): bool|Undefined
    {
        return $this->important;
    }
}

As you see, it is also desired that this works with private fields that only have getters and no setters. It would also be nice to have the ability to use readonly classes.

Notice how the return type of the getter clearly communicates that the field's value might not have been set by client. In my project, currently, calling the getter might throw TypeError which the developer might not expect with no clear knowledge of how this class is being used in different places.

I would like a way for GraphQLite to automatically set the value of constructor parameters, that accept Undefined, to Undefined::UNDEFINED. From what I know, currently, GraphQLite will set the value to null if the parameter accepts it and no value is provided.

Already looked into

  1. Root type mappers.
    a. I tried to implement this but couldn't figure out how to map from something that doesn't exist (because no value is provided). This was a couple of months ago and I don't have the code anymore.
  2. (Class) Type mappers.
    a. Read the documentation but I don't think this can work.
  3. Parameter middleware.
    a. Haven't tried it and I'm not sure that this could work.

🥅 What I wish to achieve with this issue

I would like to get answers to these questions:

  1. Is it currently possible to make GraphQLite set the value of a parameter or field (that has no provided value) to a custom type? And how?
  2. If question 1 is not possible, could you at least provide me with hints to which parts of the GraphQLite code base need to be changed to make this possible? And will the maintainers be open for a pull request for this feature?
  3. Are there any work-arounds or existing solutions that you might recommend for my goals?

Extra requirements

I don't want to use #[Input(name: 'MayHaveUndefinedValueObjectUpdateInput', update: true)] because:

  1. It is not type-safe.
  2. The getters don't communicate that the field might be undefined.
  3. The getters might unexpectedly throw a TypeError.
  4. I don't want to use hacky things like isset().
@oojacoboo
Copy link
Collaborator

oojacoboo commented Jan 31, 2025

I'd need to think about this a bit more. But that typing is going to need to be exposed over GraphQL, the way things are currently designed. So MayHaveUndefinedValueObject.description might be Undefined. GraphQL does not support "undefined", only null. So, exposing this concept over GraphQL isn't really ideal. In GraphQL world, a value is never "undefined".

@Eliasyoussef47
Copy link
Author

Eliasyoussef47 commented Feb 3, 2025

@oojacoboo
Thank you for taking the time to reply.

I'm not sure I understand what you mean with:

But that typing is going to need to be exposed over GraphQL, the way things are currently designed.

Do you mean that the type "Undefined" should show up as the type for the arg MayHaveUndefinedValueObject.description in introspection?
If that's what you mean, then that's not needed for us. It should show up as null.
The only change we (my work) need is a way to detect when the client hasn't sent a value for the field and then set the value of that field to Undefined::UNDEFINED.

I understand that this might present some challenges in certain cases. Like if the type of the field is string|Undefined, the type shown during introspection should be String (nullable/optional string). I just discovered that this library can't map the type if the field's type is just null (makes sense). So in the case of the field's type being null|Undefiend or Undefined, the same thing should happen (a CannotMapTypeException is thrown).

So, in summary, the Undefiend type should behave just as if it is null for the client. The only thing I want to achieve is intercept (when an input value is sent to the server) the step where this library checks and maps to null values and instead of just setting the value of the field (whether that is via a setter, constructor parameter or directly to the field) to null, do the next:
check if a value was provided for this field (possible with a parameter middleware), if no value was provided, then check if the field accepts Undefiened and if it does accept it, then set the value to Undefined::UNDEFINED.

(If currently possible) I would appreciate it if you could give me some guidance to how I could implement this currently.

I hope I've cleared everything up. If there is something that I haven't explained well or if I'm overlooking something, please let me know.

@oojacoboo
Copy link
Collaborator

oojacoboo commented Feb 3, 2025

Understood. I'm following you now. And yes, I was speaking about introspection.

Have you looked into writing a custom root type mapper?
https://graphqlite.thecodingmachine.io/docs/custom-types

Is there a reason why the non-initialized state of a property on a class/DTO isn't sufficient enough to serve the purposes of an undefined-like type? That is, using the #[Input(update: true)] argument.

These can be checked using ReflectionProperty::isInitialized. Obviously this logic could be wrapped into your persistence layer.

@Eliasyoussef47
Copy link
Author

Eliasyoussef47 commented Feb 3, 2025

@oojacoboo
Thank you for the quick response.

Yes I did try to write a custom root type mapper. The only thing I couldn't figure out is how to map from a value that doesn't exist (the client didn't send anything). If I remember correctly, my mapper wasn't called when the client didn't supply a value for the field or I wasn't able to detect whether or not the client had sent a value for the field (I think the values were already set to null or something. No sure.). I only spent like 3 hours on this though, so I might have missed something.
Are you sure that this would be possible with a custom root type mapper? Can parameter middlewares achieve my goal perhaps?

The reason why #[Input(update: true)] isn't sufficient comes down to it being hacky (the fields are in an invalid state) and requiring extra code, slower performance and extra developer knowledge (mental overhead) about this library.

Reflection and performance

The biggest reason why we don't want to use reflection is performance. Reflection is quite a bit slower than simply accessing the field. This can be substantial if we need to call reflection methods for a lot of fields.

That's why reflection is a big no-no for DTO's/value objects in this context.

Examples (#[Input(update: true)]) (these examples are not complete in some minor details):

Example Alpha

#[Input(update: true)]
readonly class MayHaveUndefinedValueObject
{
    #[Field]
    private string $name;

    #[Field]
    private string|null|Undefined $description;

    #[Field]
    private bool|Undefined $important;

    /**
     * @param string $name
     * @param string|null|Undefined $description
     * @param bool|Undefined $important
     */
    public function __construct(
        string $name,
        string|null|Undefined $description,
        bool|Undefined $important
    ) {
        $this->name = $name;
        $this->description = $description;
        $this->important = $important;
    }

    public function getName(): string
    {
        return $this->name;
    }

    public function getDescription(): string|null|Undefined
    {
        if (!isset($this->description)) { // Polluting a DTO getter with extra hacky stuff. A DTO needs to be dead simple.
            $this->description = Undefined::UNDEFINED; // NOT POSSIBLE WITH READONLY FIELDS AND CLASSES!!!
        }
        return $this->description;
    }

    public function getImportant(): bool|Undefined
    {
        if (!isset($this->important)) {
            return Undefined::UNDEFINED; // Hacky. We are lying about the field's type and the state of this object.
        }
        return $this->important;
    }
}

Example Bravo

DTO/Value object

#[Input(update: true)] // This fields can be uninitialized. The type system doesn't communicate this at all. The developer must HAPPEN to know this. That's an invalid state!
readonly class MayHaveUndefinedValueObject
{
    #[Field]
    private string $name;

    #[Field]
    private string|null $description;

    #[Field]
    private bool $important;

    /**
     * @param string $name
     * @param string|null $description
     * @param bool $important
     */
    public function __construct(
        string $name,
        string|null $description,
        bool $important
    ) {
        $this->name = $name;
        $this->description = $description;
        $this->important = $important;
    }

    public function getName(): string
    {
        return $this->name;
    }

    public function getDescription(): string|null|Undefined
    {
        if (!isset($this->description)) {
            $this->description = Undefined::UNDEFINED; // NOT POSSIBLE WITH READONLY FIELDS AND CLASSES!!!
        }
        return $this->description;
    }

    public function getImportant(): bool|Undefined
    {
        if (!isset($this->important)) {
            return Undefined::UNDEFINED; // Hacky. We are lying about the field's type and the state of this object.
        }
        return $this->important;
    }
}

Controller

readonly class MayHaveUndefinedController
{
    #[Mutation]
    public function updateMayHaveUndefinedBravo(MayHaveUndefinedValueObject $mayHaveUndefinedValueObject): MayHaveUndefinedEntity
    {
        /** @var MayHaveUndefinedEntity $mayHaveUndefinedEntity */
        $mayHaveUndefinedEntity = $this->mayHaveUndefinedRepository->find(1);

        $reflectionPropertyName = new ReflectionProperty(MayHaveUndefinedValueObject::class, 'name');
        if ($reflectionPropertyName->isInitialized($mayHaveUndefinedValueObject)) {
            $mayHaveUndefinedEntity->setName($mayHaveUndefinedValueObject->getName());
        }

        $reflectionPropertyDescription = new ReflectionProperty(MayHaveUndefinedValueObject::class, 'description');
        if ($reflectionPropertyDescription->isInitialized($mayHaveUndefinedValueObject)) {
            $mayHaveUndefinedEntity->setDescription($mayHaveUndefinedValueObject->getDescription());
        }

        $reflectionPropertyImportant = new ReflectionProperty(MayHaveUndefinedValueObject::class, 'important');
        if ($reflectionPropertyImportant->isInitialized($mayHaveUndefinedValueObject)) {
            $mayHaveUndefinedEntity->setImportant($mayHaveUndefinedValueObject->getImportant());
        }

        $this->entityManager->flush();
    }
}

Examples conclusion

As you can see a lot of extra things need to happen to make this work. The developers must also happen to know this unusual behavior in these cases.
In an exception-safe and type-safe solution, a developer can easily and quickly understand what will happen when calling the getter by simply reading the signature of the getter or the field's type. No obscure or unusual behaviors.

isInitialized logic in persistence layer

I'm not completely sure what you mean with:

Obviously this logic could be wrapped into your persistence layer.

But I don't want to introduce things that are specifically done for GraphQL or GraphQLite to my persistence or service classes. That would cause pollution and a stronger dependence on GraphQL and GraphQLite in my persistence layer.

@oojacoboo
Copy link
Collaborator

You started off by talking about undefined. I'm making the comparison that not initialized properties are effectively the same as undefined. I realize there are obvious differences. But they're still as close as we can get in PHP.

Your persistence layer could have a GraphQL adapter. You can abstract away all this reflection logic as well. But why not use isset instead for performance purposes.

https://3v4l.org/aXQGh

@Eliasyoussef47
Copy link
Author

@oojacoboo

Not initialized / undefined

Yes not initialized "properties" are effectively the same as undefined. But it is not communicated by the type system. Which is exactly what I'm trying to achieve.

Persistence layer and GraphQL

I'm not 100% sure what you mean with this but if I understand it correctly, it's not something we would like to implement. The main reason for that is because it introduces logic and code deep in our architecture that is only needed because of an issue at a much higher level (which is communication between client and server).

isset

Again, this doesn't communicate the state of the field with a type. A developer would need to HAPPEN to know that a field is possibly "undefined"/not initialized in order to use this method correctly. Which is not good.

Furthermore, the example you provided uses public fields. Which is something we don't want to use. I want private (possibly readonly) fields that have corresponding getters.
Other solutions with private fields might be possible. See "Example Alpha" for reasons why it's not preferable: #726 (comment).

My goal

To keep the focus on the original goal of this issue, can you please answer the question that I had asked earlier? These were:

Yes I did try to write a custom root type mapper. The only thing I couldn't figure out is how to map from a value that doesn't exist (the client didn't send anything). If I remember correctly, my mapper wasn't called when the client didn't supply a value for the field or I wasn't able to detect whether or not the client had sent a value for the field (I think the values were already set to null or something. No sure.). I only spent like 3 hours on this though, so I might have missed something.
Are you sure that this would be possible with a custom root type mapper? Can parameter middlewares achieve my goal perhaps?

Or is there any other way currently to achieve my goal? In summary my goal is:

The only thing I want to achieve is intercept (when an input value is sent to the server) the step where this library checks and maps to null values and instead of just setting the value of the field (whether that is via a setter, constructor parameter or directly to the field) to null, do the next:
check if a value was provided for this field (possible with a parameter middleware), if no value was provided, then check if the field accepts Undefiened and if it does accept it, then set the value to Undefined::UNDEFINED.

@oojacoboo
Copy link
Collaborator

Assuming your DTO uses return signatures like string|null|Undefined, you're still going to have to check to see if it's Undefined before passing that over to your model. And while I get what you mean by having it properly typed, I'm not exactly sure what you accomplish here from a DX, which seems to be your main objective. Again though, much of this DX stuff could be abstracted away. But it seems like your mind is set.

When the input type attribute's update argument is false (#[Input(update: false)]), it will set the default value (null) for the nullable fields. That would mean that the type mapper is processing these fields and you should be able to override that to whatever you like. But you'll have to dig in to be sure. I'm not 100%.

@Eliasyoussef47
Copy link
Author

Eliasyoussef47 commented Feb 11, 2025

@oojacoboo

Assuming your DTO uses return signatures like string|null|Undefined, you're still going to have to check to see if it's Undefined before passing that over to your model.

I'm not exactly sure what you accomplish here from a DX

Yes, exactly, a developer will have to check for Undefined (will happen in our controllers). That's actually one of the benefits I want to achieve. It forces the developer to check the value and think about what to do if no value was passed. Our static analysis tools will also force the developer to handle all possible cases (string, null or Undefined).
This can also be used to communicate intent. In the case of an update mutation, if a getter returns Undefined then the developer knows that the client doesn't want to change the value of that field. In the same way, if the getter returns null, then the developer knows that the client wants to set the value to null in the database. Of course this is just an example of how we plan to use the Undefined type, and I am aware that this deviates a bit from how GraphQL works.

much of this DX stuff could be abstracted away.

That's the thing. I don't want to abstract it away (although it might be needed in some places, but I want to possibility of both ways). It can be often useful to force the developer to think about what they're doing.
I understand and appreciate that you are trying to offer alternatives and trying to warn me about possible faults in my idea. But I want to first see what is possible and how far I can get with my idea.

That would mean that the type mapper is processing these fields and you should be able to override that to whatever you like.

Do you know if the NullableTypeMapperAdapter is responsible for setting not-provided values to null (docs)? If so, will my custom root type mapper be called and somehow be able to detect that no value was passed for specific field?

Just to confirm: are parameter middlewares not an option here?

@oprypkhantc
Copy link
Contributor

Just fyi, the same thing has been discussed before here: #565 (comment)

I ended up implementing a custom solution:

/**
 * This is a special type that represents an absence of key-value pair in the payload.
 *
 * I.e. for this set of properties:
 *   public int $foo;
 *   public int|MissingValue $bar = MissingValue::INSTANCE;
 *
 * it would then be possible to differentiate between these two different payloads:
 *   {"foo": 123}
 *   {"foo": 123, "bar": 123}
 */
enum MissingValue
{
	/**
	 * This is a hack to allow setting the default value of properties to an instance of this type.
	 *
	 * It could have been a regular class with a static instance() method, but then it would be an error
	 * doing this: public int|MissingValue $bar = MissingValue::instance();
	 */
	case INSTANCE;
}

class MissingValueInputFieldMiddleware implements InputFieldMiddlewareInterface
{
	public function process(InputFieldDescriptor $inputFieldDescriptor, InputFieldHandlerInterface $inputFieldHandler): ?InputField
	{
		// Unset the default value for properties with MissingValue::INSTANCE as default value, otherwise it
		// gets mapped to a string "INSTANCE" by GraphQLite and throws errors when trying to use the property.
		if ($inputFieldDescriptor->hasDefaultValue() && $inputFieldDescriptor->getDefaultValue() === MissingValue::INSTANCE) {
			$inputFieldDescriptor = $inputFieldDescriptor
				->withHasDefaultValue(false)
				->withDefaultValue(null);
		}

		return $inputFieldHandler->handle($inputFieldDescriptor);
	}
}

class MissingValueTypeMapper implements RootTypeMapperInterface
{
	public function __construct(
		private readonly RootTypeMapperInterface $next,
	) {}

	public function toGraphQLOutputType(Type $type, ?OutputType $subType, ReflectionMethod|ReflectionProperty $reflector, DocBlock $docBlockObj): OutputType&GraphQLType
	{
		return $this->next->toGraphQLOutputType($type, $subType, $reflector, $docBlockObj);
	}

	public function toGraphQLInputType(Type $type, ?InputType $subType, string $argumentName, ReflectionMethod|ReflectionProperty $reflector, DocBlock $docBlockObj): InputType&GraphQLType
	{
		$type = $this->replaceMissingValueWithNull($type);

		return $this->next->toGraphQLInputType($type, $subType, $argumentName, $reflector, $docBlockObj);
	}

	public function mapNameToType(string $typeName): NamedType&GraphQLType
	{
		return $this->next->mapNameToType($typeName);
	}

	/**
	 * Replaces types like this: `int|MissingValue` with `int|null`
	 */
	private function replaceMissingValueWithNull(Type $type): ?Type
	{
		if ($type instanceof Object_ && PhpDocTypes::className($type) === MissingValue::class) {
			return new Null_();
		}

		if ($type instanceof Nullable) {
			return new Nullable($this->replaceMissingValueWithNull($type->getActualType()));
		}

		if ($type instanceof Compound) {
			$types = array_map([$this, 'replaceMissingValueWithNull'], iterator_to_array($type));
			$types = array_values($types);

			if (count($types) > 1) {
				return new Compound($types);
			}

			return $types[0] ?? null;
		}

		return $type;
	}
}

which then can be used like so:

#[Input]
class UpdateUserData
{
	#[Field]
	#[ID]
	public string           $id;

	#[Field]
	#[Length(min: 1, max: 255)]
	#[PersonName]
	public string|MissingValue           $name = MissingValue::INSTANCE;

	#[Field]
	public CarbonInterval|MissingValue|null  $somethingAfter = MissingValue::INSTANCE;

	public function fillable(): array
	{
		return collect([
			'name' => $this->name,
			'something_after'  => $this->somethingAfter,
		])->filter(fn ($value) => $value !== MissingValue::INSTANCE)->all();
	}
}

class UserController
{
	#[Mutation]
	public function updateUser(UpdateUserData $data): User
	{
		$user = User::dummy();

		if ($data->name !== MissingValue::INSTANCE) {
			$user = $user->with(name: $data->name);
		}

		if ($data->somethingAfter !== MissingValue::INSTANCE) {
			$user = $user->with(somethingAfter: $data->somethingAfter);
		}

		return $user->with(fileIds: $data->fileIds);
	}
}

The benefit is that:

  • this can be used with constructor hydration, not just property hydration
  • it is trivial to check for a missing (undefined) value (unlike isset(), which has it's own problems with static analysis and null), and you never have to worry about accessing an uninitialized property
  • it can be easily converted into "fillable" attributes for a model (using fillable() method)

Additionally you have to handle the Symfony validation part of it as well, but it is also resolvable using ConstraintValidatorFactoryInterface and ConstraintValidatorInterface which skips MissingValue during validation.

@oprypkhantc
Copy link
Contributor

I still believe that this should be a part of GraphQLite, but, at the same time, I'm not sure how I feel about MissingValue::INSTANCE hack :/

Either way, this can be implemented in userland in the meantime.

@Eliasyoussef47
Copy link
Author

Thank you for the response @oprypkhantc . I haven't read it fully yet but I wanted to thank you for taking the time.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants