-
Notifications
You must be signed in to change notification settings - Fork 2k
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
GraphQL should allow mutating optional fields to 'null'. #133
Comments
In GraphQL input variables, there is no difference between While passing null as a sentinel value to mean "delete this value" is a pattern sometimes seen in JavaScript mutation APIs, it's not one that GraphQL is well suited to directly mirror at the moment. Does this match what you're trying to do, or is there more information you could provide about your use case? |
Yes, that's what I'm trying to do: delete a pre-existing [optional] value. Of course, deleting a required (i.e., GraphQLNonNull) value should not be permitted. Do you have any recommended workaround for this? |
Since GraphQL does not have this sentinal value, I recommend a more explicit form for deleting things:
|
That would work. However, note that using
whereas just changing
i.e., the presence of a key with a value of null is an explicit intention whereas the absence of key is an implicit one. Any chance this could be supported at some point? |
We do not plan on supporting this since most environments do not have different
I don't personally agree with this. Using I also like terse APIs, and when using only JavaScript often use this mechanism myself, but I fear that if we embraced this concept in GraphQL, the implicit |
It does seem like an omission to have not-null, which is basically equivalent to an option type (Option[T] = Some[T] or None[T]), and not be able to nullify fields. I agree that implicit null seems bad - so how about an explicit sentinel object to indicate explicit nulling? Like iOS with NSNull, for example. Maybe a token |
@leebyron What platforms are you thinking of that do not have this sentinel value? Haskell, OCaml? In these it is easy enough to pass 'None' or 'Nothing' and I would argue that the GraphQL engine shouldn't arbitrarily strip out such values (whether they be |
Er, I see the sentinel object was already covered, but I missed that in reading. Apologies @leebyron – what specifically is the challenge here? @johanatan: I think that people in general are used to |
@toulouse I don't think it is the case that people would expect that using Here's the same in Clojure:
[Note that I would be ok with an explicit GraphQL null type but now do you require GraphQL servers to interpret the end-user request and construct the GraphQL object on the fly to pass into the engine? In my case, a JSON string is the transport from the end user and there's no way for him to construct said sentinel object. |
@johanatan If I'm interpreting you correctly, you're saying that if it's not a native type, then as it would (probably) be a scalar, and as it is not just a type but also a representation, would then require specification of the actual data format, which is out of scope of the GraphQL specification (which as I understand it is a spec for defining the structure of the interface, but not the format by which data marshalling occurs). Is this roughly correct? |
(continuing my previous comment after some further thought) If so, then accidental deletion via setting-to-null seems like an invitation for subtle bugs at scale, because it enables null bugs depending on the quality of whatever graphql implementation is interpreting it. I think that Lee's caution about introducing "null has the semantic meaning of 'delete'" is well-founded, and it's not something that could be added without deeper exploration. Perhaps deletion-by-value could work better as a convention than as a part of the specification? One could use (Also, sanity check – are my comments off-base? I would hate to be derailing the conversation) |
@toulouse My previous post was merely to point out that in every language that I know of that supports both maps and nulls (e.g., Python, Perl, Ruby, Clojure, JavaScript, etc) this is expected behavior. However, your paraphrase above seems to be along the lines of what I was thinking when I said that GraphQL should "not put itself between the user-programmer and himself". |
Let me just say that I'm glad you've brought this topic up and are helping think through it. @dschafer and I just talked through the implications of adding something like this to the spec and I think it's possible. Let me try to talk through these questions you pose and see if we can find something that works.
The concern is not that platforms do not have a concept of
This is a great point, and nicely explains why I'm nervous about this direction. It's pretty easy to accidentally intermix An explicit GraphQL-Null type is interesting, but probably untenable. It would be the only example of a GraphQL-specific value where we otherwise always create native values. It would be really easy to forget about this kind of value and let it leak into your system and cause issues.
Another interesting idea - but expand the abilities of Union type to Scalars in a way that has some negative consequences. |
@toulouse Can you explain the real danger here exactly? If a programmer-user puts a value of null for some key then either: a) they really want that value to be null; i.e., for the key to in essence be deleted-- mutated to nothing from something or b) they made a mistake and put an incorrect 'null' in as the value. However, in the second case, they must have gotten that 'null' value from somewhere and if whereever that is says the value should be null, then it quite likely should be (i.e., then it quite likely was not a mistake at all--the real mistake is further upstream if there is indeed a mistake at all). |
@leebyron Can you explain how that is actually a problem though? I'm not sure how |
Ok, thinking-cap time: @dschafer just reminded me that we don't actually need to differentiate between So I think we can work out a way to do this with the following changes to the spec and reference:
Reservations:
Thoughts? |
I think 'nullable' and 'optional' are not separable without significantly complicating the code necessary to handle both cases. If they're separated, I view a reference-implementation schema -> model code (or FlatBuffer schema) generator as a requirement. The reason I think so is that in order to standardize those notions you have to either punt the complexity of handling the cases you detailed to the developer or do it for them. You also are somewhat opting out of the simplicity of using language-native collections containing plain old $LANGUAGE objects in doing so. |
Supporting the generation of plain old $LANGUAGE objects is a critical thing to maintain. |
It seems that HList would be their friend here-- the "implicit not set" values would just not be added as members of the HList and the "explicit null" values would become members of an HList with value |
Regarding the separation of 'nullable' and 'optional'-- I think it is an unnecessary complication that it is likely not very useful in practice. I prefer the simplicity of 'required' and 'nullable' being logical opposites. |
My proposal would be allowing a schema to anoint one of its member scalar types as a 'null' type. If left undefined, then any explicit null-set could be flagged as an error while statically inspecting the queries/schema (I think). The implementation (what's the right terminology here? I mean whoever is defining the scalar type) could then define the format of that scalar's wire format on their own. |
I'm glad there's agreement on keeping this simple. |
This is no better than a GraphQL supplied sentinel type though-- it would still require a GraphQL server to parse and interpret some placeholder (quite likely 'null' in my case) into the instance of this sentinel object when being passed off to the engine as 'args' or 'params' for the query. |
This is probably not the right granularity for this sort of definition. It's more likely the case that there's one "null" type that makes sense per language. Regardless, this is not something we need to solve here as it doesn't impact the spec nor the JS-impl. |
I'll work on drawing up a more formal proposal to discuss this further. |
Thanks! |
Sounds good, thanks! I think the spec's language should also include some visible |
I have an RFC up at graphql/graphql-spec#83 exploring this further. |
Closing this aging issue since graphql/graphql-spec#83 tracks this further. |
apologies for the thread-necromancy, but for future curious readers/because I didn't see this addressed: Maybe-based approaches can handle this just fine: they'd use |
Unless I'm overlooking something, I'm not seeing any way to pass
null
into a mutation for an 'optional' arg. It seems that the keys withnull
values get stripped out inside GraphQL-JS itself before making it to myresolve
in theobj
parameter.The text was updated successfully, but these errors were encountered: