-
Notifications
You must be signed in to change notification settings - Fork 100
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
Comments
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 |
@oojacoboo I'm not sure I understand what you mean with:
Do you mean that the type "Undefined" should show up as the type for the arg I understand that this might present some challenges in certain cases. Like if the type of the field is So, in summary, the (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. |
Understood. I'm following you now. And yes, I was speaking about introspection. Have you looked into writing a custom root type mapper? 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 These can be checked using ReflectionProperty::isInitialized. Obviously this logic could be wrapped into your persistence layer. |
@oojacoboo 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. The reason why Reflection and performanceThe 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 (
|
You started off by talking about Your persistence layer could have a GraphQL adapter. You can abstract away all this reflection logic as well. But why not use |
Not initialized / undefinedYes not initialized "properties" are effectively the same as Persistence layer and GraphQLI'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).
|
Assuming your DTO uses return signatures like When the input type attribute's |
Yes, exactly, a developer will have to check for
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.
Do you know if the Just to confirm: are parameter middlewares not an option here? |
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:
Additionally you have to handle the Symfony validation part of it as well, but it is also resolvable using |
I still believe that this should be a part of GraphQLite, but, at the same time, I'm not sure how I feel about Either way, this can be implemented in userland in the meantime. |
Thank you for the response @oprypkhantc . I haven't read it fully yet but I wanted to thank you for taking the time. |
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:
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 haveEDIT: nevermind. This library can't map a field with onlynull|undefined
as a possibility (even though it sounds weird).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).
Example of desired outcome
An example of an input type:
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
, toUndefined::UNDEFINED
. From what I know, currently, GraphQLite will set the value tonull
if the parameter accepts it and no value is provided.Already looked into
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.
a. Read the documentation but I don't think this can work.
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:
Extra requirements
I don't want to use
#[Input(name: 'MayHaveUndefinedValueObjectUpdateInput', update: true)]
because:TypeError
.isset()
.The text was updated successfully, but these errors were encountered: