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

Make non-null the default #63

Closed
matthewcheok opened this issue Jul 20, 2015 · 6 comments
Closed

Make non-null the default #63

matthewcheok opened this issue Jul 20, 2015 · 6 comments

Comments

@matthewcheok
Copy link

I can understand why making types nullable by default might be a reasonable decision for growing systems adding new features but I think there are reasons why this might not be ideal:

  1. This conflates the idea of nullability with providing reasonable default values. By allowing new features to be nullable by default, we push more responsibility to the application logic for determining what might be the right behavior.

  2. Because making types nullable is so easy in practice, it promotes not considering if data should even be nullable or not. This could escalate the system's entropy more quickly making it hard to reason about the system's intended behaviour.

I'd love to hear other thoughts or ideas on this!

@dschafer
Copy link
Contributor

This was the subject of considerable internal debate prior to the release of the spec, so it's definitely a discussion worth having, thanks for raising it! A few things came up when we were considering this that convinced us nullability by default was a reasonable call.

Because the type system on a GraphQL server is application-specific, not feature-specific, it can be difficult to provide universally valid default values. For example, what's the right behavior if someone's profile picture fails to load? In some features, it might be to replace it with a default silhouette; in others it might be to omit the person from the list. The correct behavior in the presence of an error is dependent not on the field, but on the feature, and only the client knows about the feature. By making the field be nullable and providing an error indicating that we nulled this field out because of the error, the feature can handle it as it sees fit.

Especially as queries and type systems grow, the probability that something will go wrong in the backend increases, and we need some way of handling that and expressing that in the query. The entirety of news feed for Facebook is powered by one GraphQL query, and there are a number of places where a backend system might (for any number of reasons) not be able to provide a result for a field. In a system where things are non-null by default, this could mean that a backend failure at a leaf node deep in the tree causes the entire query to fail.

To be clear, though: while nullable fields is a feature of GraphQL (and one that we think is valuable), it's absolutely not a requirement of it. Non-null fields and nullable fields are both first class citizens of GraphQL. We had to choose one to be the default, though, and based on the above, we chose nullable.

@OlegIlyenko
Copy link
Contributor

Just wanted to add my 2 cents. This topic also concerned me quite a bit , so I glad that @matthewcheok started this discussion.

I definitely can see the advantages of nullable as a default. Backwards-compatibility and error-handling are good and valid arguments in favor of it. Still I feel that in many cases not-null default can be advantageous. In addition to @matthewcheok arguments (which I agree with), I also would like to point out these two aspects of it:

  1. In many programming languages this concept is implemented as not-null by default. I'm coming from scala background where nullable values are represented with Option type which serves as a container for nullable value. But scala is not the only example of it, languages like kotlin, groovy, c#, haskell, java, etc. all share the same approach to this issue. Even the flow type checker, which is heavily used in graphql-js itself, also takes this approach. I feel that nullable as a default can potentially confuse developers working with these languages (which on itself can case frustration or even hard-to-find bugs because of this mismatch)
  2. As a consequence of the first point, I noticed, that it can be pretty challenging to implement this concept in statically-typed languages, which natively support "maybe" types like scala. I currently working on scala implementation of graphql. First I tried to implement not-null type for schema definition DSL, but at some point I gave up on this idea and decided to introduce OptionType which replaces NotNullType in schema DSL (it still work in progress, so things can change dramatically in future, but this what current state of implementation is). Of course it still will produce correct introspection results (with nullable default and not-null type, according to the spec)

@leebyron
Copy link
Collaborator

In addition to @dschafer's points, another fairly significant reason we chose to default to nullable values is the relative cost of changing from one to the other once you have deployed a schema and have shipped clients which are consuming that schema.

Consider the case that non-null is the default, and you set up a schema for a comment discussion feature:

type Comment {
  author: User 
  message: String
}

Then, you build Android and iOS apps, and launch them. A week later, you get bug reports that comments is not working for some users. You dig in and realize that if a user deleted their account, the comments they left behind are still there but there is no longer an author to load, so your servers are returning an error and as a consequence entire comments views don't load if they include a comment from a deleted user.

Let's fix this by changing the schema so author can return null.

type Comment {
  author: User?
  message: String
}

This gets deployed and you check back in with your users who are reporting this issue. They should see comments now, just without the author, right? Nope. Your users are now reporting that when they load a comment view, their app crashes. What? Null-pointer exception. You were indexing directly into that comment.author.picture.url in your app code, and at the time, author was non-nullable so that was safe. The server change you made was a breaking change. Yikes!

If the default had been nullable, then your original schema as defined would have Just Worked™.

For clients that understand nullability (Swift, recent Java and ObjC to a degree), they will warn you about accessing values without first checking for null. If you're checking null for something that you think should never be null, then you can have that conversation and go alter the schema. Perhaps you decide that there's always a message, an empty string can always suffice, so you change the schema to improve developer ergonomics on the client:

type Comment {
  author: User
  message: String!
}

Now we've ended up at the same result, but via the incremental application of becoming more specific rather than the late realization that you were too specific from the start and had to add breaking changes to correct. In fact, this change is totally safe. Any client that was previously checking for null will continue to work just fine.

This tale is slightly contrived, but we've suffered through versions of it at Facebook and it's a big part of why the default is nullable. As @dschafer pointed out, the probability that something will go wrong in the backend is more than 0 when loading data from storage, which is what GraphQL is designed to help you do.

In practice (perhaps unfortunately), most people do not think about nullability when designing their type systems. It's why most languages with strong types have no concept of non-null vs nullable with the obvious exception of those inspired by functional programming. And of course nearly all dynamically typed languages have no concept of this either. If most people do not think about nullability, we wanted to define the one that required less typing to be the safer of the two options, and let the incremental application of knowledge to allow for improvements from that position.

@leebyron
Copy link
Collaborator

In many programming languages this concept is implemented as not-null by default. ... I feel that nullable as a default can potentially confuse developers working with these languages

Many, but not the majority. I would contend that a non-null default would cause more confusion from programmers of more mainstream imperative languages and not used to thinking about this concept at all than a nullable default would cause confusion from those who use languages where they must think about nullability when defining types all the type because they have access to Option/Either/Maybe/?.

As a consequence of the first point, I noticed, that it can be pretty challenging to implement this concept in statically-typed languages, which natively support "maybe" types like Scala. ... and decided to introduce OptionType which replaces NotNullType in schema DSL ... Of course it still will produce correct introspection results

I strongly support this approach. Ultimately, the default for nullability is a moot-point when you have good GraphQL core libraries which respect the ergonomics of the particular language or environment. For strong typed languages that replace the concept of null with Maybe/Option/Either, I would expect to use a GraphQL library in exactly the way you define.

@leebyron
Copy link
Collaborator

Closing this task now.

@unkind
Copy link

unkind commented Mar 23, 2019

The server change you made was a breaking change.
we've suffered through versions of it at Facebook and it's a big part of why the default is nullable

It looks like the real problem was lack of automated BC breaking detector.

By making nullable fields by default, you introduce another problem. In real life examples, nullable is a special case ("server with images is down", "user has no referrer", etc.). If "special case" becomes a default one, then API users just won't pay that much attention. They always see data in the response, UI designer doesn't describe what happens if this field nullable, so it's very likely that client devs will start to just ignore nullable case.

If you put "the floor is slippery" sign everywhere and even when floor is dry, you don't make your place safer.

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

5 participants