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

Query level nullability review and drafting thread #1

Closed
wants to merge 46 commits into from
Closed
Changes from all commits
Commits
Show all changes
46 commits
Select commit Hold shift + click to select a range
53c3d6e
Create QueryLevelNullability.md
twof Apr 7, 2021
ff04f2d
Optional -> Nullable
twof Apr 7, 2021
b823d7c
Update QueryLevelNullability.md
twof Apr 7, 2021
20aa350
Update QueryLevelNullability.md
twof Apr 7, 2021
7f12742
Update QueryLevelNullability.md
twof Apr 7, 2021
acf4d00
Update QueryLevelNullability.md
twof Apr 12, 2021
486b23f
spell check
twof Apr 12, 2021
37d78b3
Update QueryLevelNullability.md
twof Apr 12, 2021
438bcac
Update QueryLevelNullability.md
twof Apr 13, 2021
b625197
Update rfcs/QueryLevelNullability.md
twof Apr 14, 2021
4829dfd
Update rfcs/QueryLevelNullability.md
twof Apr 14, 2021
70762e3
Update rfcs/QueryLevelNullability.md
twof Apr 14, 2021
1bc2272
Update rfcs/QueryLevelNullability.md
twof Apr 14, 2021
f859034
Update rfcs/QueryLevelNullability.md
twof Apr 14, 2021
ad40331
Update rfcs/QueryLevelNullability.md
twof Apr 14, 2021
c9a925d
Update rfcs/QueryLevelNullability.md
twof Apr 14, 2021
7871906
Responded to feedback
twof Apr 14, 2021
c6723ce
Use non-internal PR link
twof Apr 14, 2021
2e31037
Rework some definitions, problem statement, alternatives considered
Apr 16, 2021
c7764d5
fixups
Apr 16, 2021
1c20e26
Add Young Min Kim to the proposal (#3)
aprilrd Apr 19, 2021
e9cb076
Update rfcs/QueryLevelNullability.md
lizjakubowski Apr 19, 2021
7255a8d
Update rfcs/QueryLevelNullability.md
lizjakubowski Apr 19, 2021
254fd85
Update rfcs/QueryLevelNullability.md
lizjakubowski Apr 19, 2021
3bd4f32
Add work items section
twof Apr 19, 2021
96d0bee
PR feedback
Apr 19, 2021
e3b91d1
Merge pull request #2 from twof/lizjakubowski-first-pass
lizjakubowski Apr 19, 2021
e0a572b
Update rfcs/QueryLevelNullability.md
twof Apr 19, 2021
4aab23b
Update QueryLevelNullability.md
twof Apr 21, 2021
3c45346
Merge pull request #4 from twof/work-items
twof Apr 26, 2021
d891d96
Update rfcs/QueryLevelNullability.md
aprilrd May 6, 2021
54a06eb
Formally specify Non-Null behavior
fotoetienne May 6, 2021
e6fc0ed
Added discussion on partial results
fotoetienne May 6, 2021
550ffeb
Update rfcs/QueryLevelNullability.md
fotoetienne May 6, 2021
8515128
Update rfcs/QueryLevelNullability.md
fotoetienne May 6, 2021
c60e590
Update rfcs/QueryLevelNullability.md
fotoetienne May 6, 2021
e1e363a
typos
fotoetienne May 6, 2021
5d5a519
Merge pull request #5 from fotoetienne/QueryLevelNullability
twof May 6, 2021
6e54093
Update QueryLevelNullability.md
twof May 7, 2021
6d5d16c
Update QueryLevelNullability.md
twof May 7, 2021
458ee4e
Merge pull request #6 from twof/3rd-party-apis
twof May 7, 2021
e32b1f2
Cleanup after behavior was clarified
twof May 8, 2021
b265238
Remove a few instances of query when they should be operation
twof May 8, 2021
da52795
revert non-null
twof May 8, 2021
27069c9
Merge pull request #7 from twof/behavior-cleanup
twof May 10, 2021
844248c
Update rfcs/QueryLevelNullability.md
twof May 13, 2021
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
342 changes: 342 additions & 0 deletions rfcs/QueryLevelNullability.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,342 @@
# RFC: Operation Level Nullability

**Proposed by:**

- [Alex Reilly](<social or github link here>) - Yelp iOS
twof marked this conversation as resolved.
Show resolved Hide resolved
twof marked this conversation as resolved.
Show resolved Hide resolved
- [Liz Jakubowski](https://github.com/lizjakubowski) - Yelp iOS
- [Mark Larah](https://github.com/magicmark) - Yelp Web
- [Sanae Rosen](<social or github link here>) - Yelp Android
- [Stephen Spalding](https://github.com/fotoetienne) - Netflix GraphQL Server Infrastructure
- [Wei Xue](https://github.com/xuewei8910) - Yelp iOS
- [Young Min Kim](https://github.com/aprilrd) - Netflix UI

This RFC proposes a syntactical construct for GraphQL clients to express that fields in an operation are **non-null**.

## Definitions

- **Nullability**. A feature of many programming languages (eg [Swift](https://developer.apple.com/documentation/swift/optional),
[Kotlin](https://kotlinlang.org/docs/null-safety.html#nullable-types-and-non-null-types), [SQL](https://www.w3schools.com/sql/sql_notnull.asp))
that is used to indicate whether or not a value can be `null`.

Nullability language constructs (e.g. `?` in Swift/Kotlin) have become popular due to their ability to solve ergonomic
problems in programming languages, such as those surrounding `NullPointerException` in Java.

- **Codegen**. Short for "code generation", in this proposal refers to tools that generate code to facilitate using
GraphQL on the client. GraphQL codegen tooling exists for many platforms:
- [Apollo](https://github.com/apollographql/apollo-tooling#code-generation) has a code generator for Android (Kotlin)
and iOS (Swift) clients
- [The Guild](https://www.graphql-code-generator.com/) has a TypeScript code generator for web clients

GraphQL codegen tools typically accept a schema and a set of documents as input, and output code in a language of
choice that represents the data returned by those operations.

For example, the Apollo iOS codegen tool generates Swift types to represent each operation document, as well as model types
representing the data returned from those queries. Notably, a nullable field in the schema becomes an `Optional`
property on the generated Swift model type, represented by `?` following the type name.

In the example below, the `Business` schema type has a nullable field called `name`.
```graphql
# Schema
type Business {
# The unique identifier for the business (non-nullable)
id: String!

# The name of the business (nullable)
name: String
}

# Document
query GetBusinessName($id: String!) {
business(id: $id) {
name
}
}
```
At build time, Apollo generates the following Swift code (note: the code has been shortened for clarity).
```swift
struct GetBusinessNameQuery {
let id: String

struct Data {
let business: Business?

struct Business {
/// Since the `Business.name` schema field is nullable, the corresponding codegen Swift property is `Optional`
let name: String?
}
}
}
```
The query can then be fetched, and the resulting data handled, as follows:
```swift
GraphQL.fetch(query: GetBusinessNameQuery(id: "foo"), completion: { result in
guard case let .success(gqlResult) = result, let business = gqlResult.data?.business else { return }

// Often, the client needs to provide a default value in case `name` is `null`.
print(business?.name ?? "null")
}
```

## 📜 Problem Statement

The two modern languages used on mobile clients, Swift and Kotlin, are both non-null by default. By contrast, in a

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would consider reducing the focus on specific client languages.

The crux of the problem is that the SDL nonNull (!) eliminates the possibility of partial failure on a given type. This forces the SDL author to decide for which fields partial failure is acceptable. A GraphQL schema author may not be in the best position to decide whether partial failure is acceptable for a given canvas. Additionally, the answer to whether a missing value is acceptable may vary between UI canvases. Specifying nonNull (aka. required) in the query gives the UI developer the power to decide where partial failure is acceptable, and the flexibility for different canvases to have different requirements in this regard.

The pains are more noticeable in languages with first-class null safety, but are relevant to any client.

GraphQL type system, every field is nullable by default. From the [official GraphQL best practice](https://graphql.org/learn/best-practices/#nullability):

> This is because there are many things that can go awry in a networked service backed by databases and other
> services. A database could go down, an asynchronous action could fail, an exception could be thrown. Beyond
> simply system failures, authorization can often be granular, where individual fields within a request can
> have different authorization rules.

This discrepancy means that developers using strongly-typed languages must perform `null` checks anywhere that GraphQL
codegen types are used, significantly decreasing the benefits of codegen. In some cases, the codegen types may be so
complex that a new model type which encapsulates the `null` handling is written manually.

While the schema can have nullable fields for valid reasons (such as federation), in some cases the client wants to decide
if it accepts a `null` value for the result to simplify the client-side logic. In addition, syntax for this concept
would allow codegen tooling to generate model types that are more ergonomic to work with, since the model
type's properties would have the desired nullability.


## 🧑‍💻 Proposed Solution

A client specified Non-Null designator.

## 🎬 Behavior

The proposed query-side Non-Null designator would have identical semantics as the current
SDL-defined [Non-Null](https://spec.graphql.org/June2018/#sec-Errors-and-Non-Nullability). Specifically:

- If the result of resolving a field is null (either because the function to resolve the field returned null
or because an error occurred), and that field is of a Non-Null type,
**or the operation specifies this field as Non-Null**,
then a field error is thrown. The error must be added to the "errors" list in the response.

- Since Non-Null type fields cannot be null, field errors are propagated to be handled by the parent field.
If the parent field may be null then it resolves to null, otherwise the field error
is further propagated to its parent field.

- If all fields from the root of the request to the source of the field error return Non-Null types or are
specified as Non-Null in the operation, then the "data" entry in the response should be null.

## ✏️ Proposed syntax

The client can express that a schema field is required by using the `!` syntax in the operation definition:
```graphql
query GetBusinessName($id: String!) {
business(id: $id) {
name!
}
}
```

### `!`

We have chosen `!` because `!` is already being used in the GraphQL spec to indicate that a field is non-nullable.
Incidentally the same precedent exists in Swift (`!`) and Kotlin (`!!`) which both use similar syntax to indicate
"throw an exception if this value is `null`".

## Use cases

#### When a field is necessary to the function of the client

Expressing nullability in the operation, as opposed to the schema, offers the client more flexibility and control over
whether or not an error is thrown.

There are cases where a field is `nullable`, but a feature that fetches the field will not function if it is `null`.
For example if you are trying to render an information page for a movie, you won't be able to do that if the name field
of the movie is missing. In that case it would be preferable to fail as early as possible.
Comment on lines +145 to +147
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here's one of the cases where this would be needed.

GatsbyJS projects use GraphQL to handle build-time data dependencies, and provide a file system route API that can automatically build pages from specific types.

Creating a file called src/pages/{BlogPost.slug}.jsx creates pages from all BlogPost nodes and injects page context. { id: string, slug: string }

user can use the page context as an argument to fetch additional data.

# $id is automatically injected by Gatsby
query BlogPostQuery($id: String!) {
  # The type of blogPost is nullable, but the result is guaranteed to non-null by Gatsby
  blogPost(id: { eq: $id }) {
    title
    tags
  }
}

This can be done with additional static analysis in plugin layer. But the proposal can be a simple solution.

Even if It's not clear in production cases that can customize schema, but it seems a huge advantage for ecosystem players using SDL and code-generation.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for sharing another use case!


According to the official GraphQL best practice, this field should be non-nullable:

> When designing a GraphQL schema, it's important to keep in mind all the problems that could go wrong and if "null" is
> an appropriate value for a failed field. Typically it is, but occasionally, it's not. In those cases, use non-null
> types to make that guarantee.

However, the same field may not be "vital" for every feature in the application that fetches it. Marking the field as
non-null in the schema would result in those other features erroring unnecessarily whenever the field is "null".

#### When handling partial results

It is often unclear how to handle partial results.

- What elements are essential to having an adequate developer experience? How many elements can fail before you
should just show an error message instead?
- When any combination of elements can fail, it is very hard to anticipate all edge cases and how the UI
might look and work, including transitions to other screens.

Implementing these decisions significantly complicates the client-side logic for handling query results.

### ✨ Examples

#### Non-nullable field
```graphql
query GetBusinessName($id: String!) {
business(id: $id) {
name!
}
}
```
would codegen to the following type on iOS.
```swift
struct GetBusinessNameQuery {
let id: String

struct Data {
let business: Business?

struct Business {
/// Lack of `?` indicates that `name` will never be `null`
let name: String
}
}
}
```

#### Non-nullable object with nullable fields
```graphql
query GetBusinessReviews {
business(id: 4) {
reviews! {
rating
text
}
}
}
```

## ✅ RFC Goals

- Non-nullable syntax that is based off of syntax that developers will already be familiar with
- Enable GraphQL codegen tools to generate more ergonomic types

## 🚫 RFC Non-goals

## 🗳️ Alternatives considered

### A `@nonNull` official directive

This solution offers the same benefits as the proposed solution. Since many GraphQL codegen tools already support the `@skip` and `@include` directives, this solution likely has a faster turnaround.

### A `@nonNull` custom directive

This is an alternative being used at some of the companies represented in this proposal for the time being.

While this solution simplifies some client-side logic, it does not meaningfully improve the developer experience for clients.

* The cache implementations of GraphQL client libraries also need to understand the custom directive to behave correctly. Currently, when a client library caches a null field based on an operation without a directive, it will return the null field for another operation with this directive.
Copy link

@mjmahone mjmahone May 6, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will be true even with custom syntax. For composability of fragments, it would be worrisome if this new syntax made it impossible to have these two fragments in the same application:

fragment UserOkWithoutName on User {
  name
}
fragment UserMustHaveName on User {
  name!
}

With consistency, I could end up re-rendering a component built with UserMustHaveName based on a response fetched using only UserOkWithoutName.

And in fact it gets worse:

fragment BestFriendWithoutName on User {
  best_friend {
    id
  }
}
fragment BestFriendWithName on User {
  best_friend {
    id
    name!
  }
}

If my best friend, when I fetched BestFriendWithName, is User:1, and then suddenly, after fetching BestFriendWithoutName is User:2, I no longer am capable of rendering the component relying on BestFriendWithName (because name is unset on User:2). If the client library doesn't know what to do in that case, you can end up with consistency updates causing crashes.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As you said, the client library will need to understand this new syntax/directive. I am not familiar with GraphQL client implementations, so I am not sure how difficult my imagined behavior will be to implement. Doesn't this behavior solve the issue you outlined?

If a GraphQL client finds that a component relies on BestFriendWithName , but the cached data doesn't have name, it should not return any of the cached data. To the client, it is like a state before fetching started. Depending on fetch-policy (to use Apollo's terminology), the client can start a new request to fetch the missing field data.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doesn't this behavior solve the issue you outlined?

That's how Apollo solves it, but that strategy isn't true universally. For instance that's not how Relay solves it. The client I work on can't solve it this way, as our consistency subscriptions are at the response level, for queries that include hundreds of fragments (whether that's a good choice or not is irrelevant: it's how things work for the largest GraphQL client at Facebook): if missing name meant the entire consistency update should not be served, whenever a product chose to mark their field with !, they could cause entire unrelated surfaces to lose consistency.

Another valid strategy would also be to return the data, but with best_friend being null: all the other data in the fragment/consistency subscription would still be delivered (this is Relay's strategy for @required(action: NONE)). Or requiring the product to null check every piece of data, always, regardless of its nullability annotation.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you mean that Relay requires field selections on the same id and type to behave the same globally? If so, I can see how my suggested behavior won't work. If not:

Another valid strategy would also be to return the data, but with best_friend being null: all the other data in the fragment/consistency subscription would still be delivered (this is Relay's strategy for @required(action: NONE))

This is what I imagined. The client would nullify up until the most immediate nullable parent.

* For clients that rely on codegen, codegen types typically cannot be customized based on a custom directive. See https://github.com/dotansimha/graphql-code-generator/discussions/5676 for an example. As a result, the optional codegen properties still need to be unwrapped in the code.

This feels like a common enough need to call for a language feature. A single language feature would enable more unified public tooling around GraphQL.

### Make Schema Fields Non-Nullable Instead

It is intuitive that one should simply mark fields that are not intended to be null as non-null in the schema.
For example, in the following GraphQL schema:

```graphql
type Business {
name: String
isStarred: Boolean
}
```

If we intend to always have a name and isStarred for a Business, it may be tempting to mark these fields as Non-Null:

```graphql
type Business {
name: String!
isStarred: Boolean!
}
```

Marking Schema fields as non-null can introduce particular problems in a distributed environment where there is a possibility
of partial failure regardless of whether the field is intended to have null as a valid state.

When a non-nullable field results in null, the GraphQL server will recursively step through the field’s ancestors to find the next nullable field. In the following GraphQL response:

```json
{
"data": {
"business": {
"name": "The French Laundry",
"isStarred": false
}
}
}
```

If isStarred is non-nullable but returns null and business is nullable, the result will be:

```json
{
"data": {
"business": null
}
}
```

Even if name returns valid results, the response would no longer provide this data. If business is non-nullable, the response will be:
```json
{
"data": null
}
```

In the case that the service storing user stars is unavailable, the UI may want to go ahead and render the component
without a star (effectively defaulting isStarred to false). Non-Null in the schema makes it impossible for the client
to receive partial results from the server, and thus potentially forces the entire component to fail rendering.

More discussion on [when to use non-null can be found here](https://medium.com/@calebmer/when-to-use-graphql-non-null-fields-4059337f6fc8)

Additionally, marking a field non-null is not possible in every use case. For example, when a developer is using a 3rd-party API such as
[Github's GraphQL API](https://docs.github.com/en/graphql) they won't be able to alter Github's schema, but they may still
want to have certain fields be non-nullable in their application.

### Write wrapper types that null-check fields
This is the alternative being used at some of the companies represented in this proposal for the time being.
It's quite labor intensive and the work is quite rote. It more or less undermines the purpose of
having code generation.

### Alternatives to `!`
#### `!!`
This would follow the precedent set by Kotlin.

### Make non-nullability apply recursively
For example, everything in this tree would be non-nullable
```graphql
query! {
business(id: 4) {
name
}
}
```
Comment on lines +305 to +312
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@wxue Do you remember why we didn't want this?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should remove this as an alternative, I don't think we understand it well enough to vouch for it


## 🙅 Syntax Non-goals

This syntax consciously does not cover the following use cases:

- **Default Values**
Copy link

@fotoetienne fotoetienne May 6, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Defaults can be quite useful from a resilience standpoint. It is relevant to this proposal in that the ! syntax would't be as amenable to specifying a default, whereas a directive (e.g. @nonNull(default: false)) would open us up to that.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Though replacing ! with ! = “default” like in variable defaults could work. This would either have to be evaluated client side (like in Relay) or the same default would have to apply to all references at the same location (whether from fragments or directly).

The syntax being used in this proposal causes queries to error out in the case that
a `null` is found. As an alternative, some languages provide syntax (eg `??` for Swift)
that says "if a value would be `null` make it some other value instead". We are
not interested in covering that in this proposal.

## Work Items
Patches that will need to be made if this proposal is accepted. The
[RFC proposal process](https://github.com/graphql/graphql-spec/blob/main/CONTRIBUTING.md)
requires that a proof of concept is implemented in a GraphQL library. As work items are completed,
PRs will be linked here.
- Spec Changes
- Official Libraries
- GraphQL.js: https://github.com/graphql/graphql-js/pull/2824
- 3rd Party Libraries
- [Apollo Android](https://github.com/apollographql/apollo-android)
- Code Gen
- Cache
- [Apollo iOS](https://github.com/apollographql/apollo-ios)
- Code Gen
- Cache
- [Apollo JS](https://github.com/apollographql/apollo-client)
- Code Gen
- Cache
- [GraphQL Code Generator by The Guild](https://github.com/dotansimha/graphql-code-generator)