graphql / graphql-js

A reference implementation of GraphQL for JavaScript
http://graphql.org/graphql-js/
MIT License
20.05k stars 2.02k forks source link

GraphQL should allow mutating optional fields to 'null'. #133

Closed johanatan closed 8 years ago

johanatan commented 9 years ago

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 with null values get stripped out inside GraphQL-JS itself before making it to my resolve in the obj parameter.

leebyron commented 9 years ago

In GraphQL input variables, there is no difference between null and undefined. The value null means "lack of a value".

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?

johanatan commented 9 years ago

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?

leebyron commented 9 years ago

Since GraphQL does not have this sentinal value, I recommend a more explicit form for deleting things:

mutation myMutation {
  editThing(id: 3, additions: { foo: "foo" }, deletions: [ "bar" ]) { ... }
}
johanatan commented 9 years ago

That would work. However, note that using null is also explicit. The above would be:

mutation x {
  editThing(id: 3, values: {foo: "foo", "bar": null }) { ... }
}

whereas just changing foo and leaving bar (as well as baz or whatever else may be defined) to [their] pre-existing value(s) would be:

mutation x {
   editThing(id: 3, values: {foo: "foo"}) { ... }
}

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?

leebyron commented 9 years ago

We do not plan on supporting this since most environments do not have different null and undefined sentinel values - that's a specific feature of JavaScript and GraphQL is designed to be supported by many more platforms.

However, note that using null is also explicit.

I don't personally agree with this. Using null to implicitly mean "delete this value" is a JavaScript-ism that doesn't translate well to platform-agnostic systems. I personally much prefer the explicit deletions: field.

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 null would be misinterpreted by different servers and result in the accidental deletion of data.

toulouse commented 9 years ago

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 <null> or something.

johanatan commented 9 years ago

@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 None, Nothing or null) [inserting itself between the user and himself].

toulouse commented 9 years ago

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 key: null being the same as nothing at all, rather than an explicit set to null, which might also be a problem when crafting some part of a request, adding information, then stripping it out. I think an explicit GraphQL null type might be better than relying on language-native nulls.

johanatan commented 9 years ago

@toulouse I don't think it is the case that people would expect that using key: null would have no effect at all-- it reads to me like an explicit setting of the value for key to null (just like in JavaScript itself).

Here's the same in Clojure:

> (assoc {:a 9} :a nil)
{:a nil}

[Note that :a is not deleted, it's value is merely set to nil].

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.

toulouse commented 9 years ago

@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?

toulouse commented 9 years ago

(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 union nullableSomeType = someType | userDefinedNullType as a user-defined alternative to !. On the other hand, that might lead to a ton of repetition, and it's really only explicitly acknowledging the custom null type, throwing out the conciseness of !, and passing the buck down the road.

(Also, sanity check – are my comments off-base? I would hate to be derailing the conversation)

johanatan commented 9 years ago

@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?

@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".

leebyron commented 9 years ago

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.

@johanatan

What platforms are you thinking of that do not have this sentinel value?

The concern is not that platforms do not have a concept of null - you correctly point out that nearly all platforms do. The concern is a differentiation between an explicit value null and something else that means "the value was not defined" - in JavaScript: undefined. As far as I know, JavaScript is pretty unique to have this 2nd sentinel value. Other common server environment languages don't have a corollary: PHP, Ruby, Python, C#, C++, Scala, Java, etc. They all have one value akin to null but cannot differentiate between "explicitly null" and "implicitly not set".

@toulouse

I think that people in general are used to key: null being the same as nothing at all, rather than an explicit set to null, which might also be a problem when crafting some part of a request, adding information, then stripping it out. I think an explicit GraphQL null type might be better than relying on language-native nulls.

This is a great point, and nicely explains why I'm nervous about this direction. It's pretty easy to accidentally intermix null and undefined and each providing different semantic behavior is the root cause of a lot of software errors in systems that use mutation APIs like the one proposed.

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.

Perhaps deletion-by-value could work better as a convention than as a part of the specification? One could use union nullableSomeType = someType | userDefinedNullType as a user-defined alternative to !. On the other hand, that might lead to a ton of repetition, and it's really only explicitly acknowledging the custom null type, throwing out the conciseness of !, and passing the buck down the road.

Another interesting idea - but expand the abilities of Union type to Scalars in a way that has some negative consequences.

johanatan commented 9 years ago

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.

@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).

johanatan commented 9 years ago

The concern is not that platforms do not have a concept of null - you correctly point out that nearly all platforms do. The concern is a differentiation between an explicit value null and something else that means "the value was not defined" - in JavaScript: undefined. As far as I know, JavaScript is pretty unique to have this 2nd sentinel value. Other common server environment languages don't have a corollary: PHP, Ruby, Python, C#, C++, Scala, Java, etc. They all have one value akin to null but cannot differentiate between "explicitly null" and "implicitly not set".

@leebyron Can you explain how that is actually a problem though? I'm not sure how undefined got brought into this (and seems like a separate issue entirely to me). Since a JSON object is by definition a 'static' subset of a full-blown JavaScript object, I don't think we have to worry about intricacies involving undefined (or other JavaScriptisms). In fact, I think it would be fine if the engine did strip out or reject [live] structures containing undefineds (and functions or other complex values) (yet that wouldn't apply to or affect my current request to allow nulls to pass through).

leebyron commented 9 years ago

Ok, thinking-cap time:

@dschafer just reminded me that we don't actually need to differentiate between null and undefined values - In the cases that we need to interpret explicit vs implicit null, what we really need to do is treat the containing input object or arguments as a Dictionary/Map and differentiate between "this is set in the Map to the value null" vs "this was not set in the Map". Most languages we care about have a way to represent Map - however some of the strongly-typed environments (e.g. Scala, Haskell) would much prefer a Record type where each field is correctly typed, where Map would imply a uniform type for all arguments / fields which is not going to work out well for them.

So I think we can work out a way to do this with the following changes to the spec and reference:

Reservations:

Thoughts?

toulouse commented 9 years ago

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.

leebyron commented 9 years ago

Supporting the generation of plain old $LANGUAGE objects is a critical thing to maintain.

johanatan commented 9 years ago

Languages which are strongly-typed and wish to strongly-type the fields in an arguments set or input object, but do not have a way to identify explicit null vs implicit not-provided. This was my original primary concern, and remains a problem. For example, a server which uses Scala, or Haskell, or even just Java or C# may sneer at the idea of using Map when they could have defined an explicit type that represents the input, such as { foo: Maybe, bar: Maybe } - but of course once the incoming variable JSON is parsed into a type like that, the explicit null vs implicit not set is lost as Maybe.None or equivalent.

Backends written in these languages may explicitly decide not to interpret the two differently, and not support mutations written in this way.

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 Maybe.None.

johanatan commented 9 years ago

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.

toulouse commented 9 years ago

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.

leebyron commented 9 years ago

I prefer the simplicity of 'required' and 'nullable' being logical opposites.

I'm glad there's agreement on keeping this simple.

johanatan commented 9 years ago

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.

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.

leebyron commented 9 years ago

My proposal would be allowing a schema to anoint one of its member scalar types as a 'null' type.

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.

johanatan commented 9 years ago

So, just to be clear: I do like the proposal @leebyron and @dschafer came up with above: i.e., introduction of 'null', spec language describing the semantics, and ref impl carefully handling the inputs args in light of this new value.

leebyron commented 9 years ago

I'll work on drawing up a more formal proposal to discuss this further.

johanatan commented 9 years ago

Thanks!

toulouse commented 9 years ago

Sounds good, thanks!

I think the spec's language should also include some visible :warning: warning or prominent indication that conforming implementations should pay attention to that.

leebyron commented 9 years ago

I have an RFC up at https://github.com/facebook/graphql/pull/83 exploring this further.

leebyron commented 8 years ago

Closing this aging issue since https://github.com/facebook/graphql/pull/83 tracks this further.

vergenzt commented 7 years ago

Found this issue from Google, and the rabbit hole ended with this being resolved within graphql-js by graphql/graphql-js#544 (released in v0.8.0), so thought I'd mention that here. :)

riwsky commented 4 years ago

Reservations: [...]

  • Languages which are strongly-typed and wish to strongly-type the fields in an arguments set or input object, but do not have a way to identify explicit null vs implicit not-provided. This was my original primary concern, and remains a problem. For example, a server which uses Scala, or Haskell, or even just Java or C# may sneer at the idea of using Map<Any> when they could have defined an explicit type that represents the input, such as { foo: Maybe<String>, bar: Maybe<Int> } - but of course once the incoming variable JSON is parsed into a type like that, the explicit null vs implicit not set is lost as Maybe.None or equivalent.

Thoughts?

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 Maybe<Maybe<Type>>, so an explicitly-set null would be Just (Nothing) and not-set case is Nothing. In fact, if you wanted to be able to explicitly set values to Nothing in a Haskell map, this is exactly how the Map lookup in the standard library will force you to handle this.