GraphQLBundle
GraphQLBundle copied to clipboard
overblog
Override serialization/parse logic of built-in ID scalar
| Q | A |
|---|---|
| Bug report? | no |
| Feature request? | yes |
| BC Break report? | no |
| RFC? | no |
| Version/Branch | * |
Let's say I want to use the built-in ID type, but I want to parse it into an uuid. Redefining the ID scalar with alternative ScalarType implementation has no effect (also no errors).
~~The only way currently is via resolve & expression in field definition. Builder are an option, but kind of messy for a simple ID field. Decorator? Possible, but also messy in my opinion.~~
Actually, since the moment when you want to parse/parseLiteral is in input context and you can not define resolve on input-object, the above is not really possible.
I do not think that would make sense for other built-in scalar types, or does it?
What do you think?
ID is actually spec'd so changing the behavior of it seems broken to me.
What's wrong with a custom UUID scalar type?
@ooflorent From spec's perspective ID has no behavior at all. It is (for the client) an opaque identifier.
The ID scalar type represents a unique identifier, often used to refetch an object or as key for a cache. The ID type appears in a JSON response as a String; however, it is not intended to be human-readable. When expected as an input type, any string (such as "4") or integer (such as 4) input value will be accepted as an ID.
How the ID value is parsed server side has nothing to do with spec or altering behavior. Whats wrong with a custom UUID scalar type is, that UUID is not opaque and breaks out-of-the-box conpatibility of libraries that work with ID (including relayjs). So you will be forced to adjust client code to work with different type and care about something, the client should not care by design.
I currently have indeed an UUID scalar on the input side as workaround. However its typing-wise incompatible with the ID type returned on the out side. At least if you do not want to change the relay defaults.
Parsing UUID string in the resolvers is as bad as parsing date(-time) string in the resolver.
Also when you provide an invalid ID (UUID internally) you would get an graphql error the same way you would get when you provide an invalid date(-time) or any other scalar in an invalid form (because its part of the parse logic for the scalar type).
Given spec: https://graphql.github.io/graphql-spec/June2018/#sec-ID Quote:
Input Coercion:
When expected as an input type, any string (such as "4") or integer (such as 4) input value should be coerced to ID as appropriate for the ID formats a given GraphQL server expects. Any other input value, including float input values (such as 4.0), must raise a query error indicating an incorrect type.
Note this:
- [...] coerced to ID as appropriate for the ID formats a given GraphQL server expects
- `any other input value [...] must raise a query error indicating an incorrect type.
On the Output side of the spec, we are currently lucky, when (in this case the ramsey/uuid) library uses __toString() to coerce to string, as per spec. If you use something different, you might be less lucky.
@ooflorent Does it make sense now? What makes up a scalar type is its name (nominal type) and evtl. expected format. When you create a custom scalar type, the type system of graphql does not know whether it can be int, float, bool, string, [...]. You never specify it in the schema either. You decide it serverside what it has to be. This includes the format expected, for example a string date, datetime, uuid, etc.
Yes, actually the ID type is in the graphql library itself. So I think I should start there. However, it might be needed, when it becomes possible in the library, that we might have to adjust it here. I will post an issue there as soon as I have the time. If it is then fully resolved there (if they agree on this needs a change ;P), then I will close this if nothing needs to be done.
Note: It it had no real-life usage, I would say "not too important". But it actually has. Like said, not having to hydrate and manually create graphql errors each time I receive an ID input. Sometimes it might even be multiple. Imagine you receive a list of IDs.
@akomm at the first look it seem a good idea, being able to override scalar system types but I now get the reason why @ooflorent or the main lib are against that. ID should be the same for all GraphQL implementation (node js, php, ruby, ...), if the scalar system types does not feet your need not a big deal just create your own scalar UUID for example and use it or open an issue on the main lib (webonyx/graphql-php). I'm not against adding customs scalars to the bundle (UUID, DateTime, Date, Email, Url, ...).
@mcg-web I don't quite understand, how this makes ID any different. As a solution you propose to make it different. I want to avoid it. The only way to make the implementation be same for all is to behave according to spec. Spec tells us, as I quoted:
- On the output side, the server must coerce its internal
IDformat to string, so that it can be used uniformly across different clients. - On the input side, the server must coerce from string or int to
IDformat as needed internally by the server. The server must raise an error, if given input is invalid (can not be coerced from string or int).
The current implementation takes away the logic how the coercion is happening both on input and output sides.
Example interface to implement the coerce logic for ID. This way the ID type (library) itself does not have to be overriden, it just has to use such an interface. It might have its (BC) default implementation. And you can provide your custom. Whats important is, its spec conform as on the output side you return string and on input side you expect string or int.
interface CoerceID
{
/**
* @var string|int $id
* @return mixed internal ID type
*/
public function fromInput($id);
/**
* @param ValueNode $id
* @return mixed internal ID type
*/
public function fromInputLiteral(ValueNode $id);
/**
* @param mixed $id internal ID type
* @return string
*/
public function toOutput($id): string;
}
That what I says if you want to fix the behavior of ID the best place to do so is webonyx/graphql-php , override it will only fix it for you? Maybe I'm missing something ?
Correct, this is why I wrote this: https://github.com/overblog/GraphQLBundle/issues/541#issuecomment-538912602
Evtl. it will be needed to have an ability to specify the custom CoerceID in extension config however, this is what I mean:
Yes, actually the ID type is in the graphql library itself. So I think I should start there. [...] If it is then fully resolved there (if they agree on this needs a change ;P), then I will close this if nothing needs to be done.
A solution where we do not need to do anything here, but might not be desired by lib maintainers, is setting the used strategy via some static method, like IDType::setCoerceStrategy(CoerceID $strategy).
There we go: https://github.com/webonyx/graphql-php/issues/555
Found also this: https://github.com/webonyx/graphql-php/issues/184#issuecomment-336933107
Note the except maybe ID type at the end of: I don't think that replacing standard types with custom types is a good idea because their parsing behavior is defined in spec and clients rely on it (except maybe ID type)