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 16 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
179 changes: 179 additions & 0 deletions rfcs/QueryLevelNullability.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,179 @@
# RFC: Query Level Nullability

**Proposed by:**
- [Liz Jakubowski](https://github.com/lizjakubowski) - Yelp iOS
- [Sanae Rosen](<social or github link here>) - Yelp Android
- [Mark Larah](https://github.com/magicmark) - Yelp Web
- [Alex Reilly](<social or github link here>) - Yelp iOS
twof marked this conversation as resolved.
Show resolved Hide resolved
- [Wei Xue](<social or github link here>) - Yelp iOS
twof marked this conversation as resolved.
Show resolved Hide resolved

This RFC proposes a syntactical construct for GraphQL clients to express the **nullability** of schema fields requested in a query.

## Definitions

Nullability: A concept that exists across 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 express when users can be certain that a value can or can never be `null`
twof marked this conversation as resolved.
Show resolved Hide resolved
(or the language equivalent). Nullability language constructs (eg `?` in Swift/Kotlin)
have become popular due to their ability to solve ergonomic problems in languages
such as those surrounding `NullPointerException` in Java.

Codegen: Short for Code Generation, tools that exist for working with GraphQL which 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. GraphQL codegen tools exist for many platforms. As an example, [here's some info about Apollo's offerings](https://github.com/apollographql/apollo-tooling#code-generation).

## 📜 Problem Statement

GraphQL is a "nullable by default" language meaning that all properties are allowed to be `null`.
This is in contrast to the two modern languages used on mobile clients, Swift and Kotlin,
which are both non-null by default. In Swift and Kotlin, unless developers otherwise
specify it, properties cannot be `null`.

This mismatch creates some dissonance for developers who are currently forced to deal with
problems that commonly surround nullablility. This makes large numbers of null fields tedious and
time-consuming to work with.

It is often unclear how to handle partial results.
- What elements are essential to having an adequate user 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 app
might look and work, including transitions to other screens.
twof marked this conversation as resolved.
Show resolved Hide resolved

In Yelp's GraphQL schema, almost all object fields are nullable except for those with ID type.
This adheres to what seems to be the [official best practice](https://graphql.org/learn/best-practices/#nullability).
twof marked this conversation as resolved.
Show resolved Hide resolved

This poses a problem for GraphQL clients that use codegen. The codegen provides
Swift/Kotlin types to represent queries used in the app. Nullable fields in the schema are represented
as nullable properties on the resulting type, represented by `?` following the type name:
```graphql
query GetBusinessName($encid: String!) {
business(encid: $encid) {
name
}
}
```
```swift
struct GetBusinessNameQuery.Data.Business {
let name: String?
}
```
In many cases, the client should error if the business `name` is null. If codegen were out of the picture,
lizjakubowski marked this conversation as resolved.
Show resolved Hide resolved
we would be able to throw an error at JSON response-parsing time if it's missing, or otherwise instantiate
a hand-written business object with a non-nullable `name` property. From that point on, all feature code
can happily assume that it has a non-null business name to work with.

## 🧑‍💻 Proposed syntax

It would make more sense if the client could express that `name` must be non-null _in the query itself_:
```graphql
query GetBusinessName($encid: String!) {
business(encid: $encid) {
name! <-- this!
}
}
```
Semantically the GraphQL `!` operator is nearly identical to its counterpart in Swift (also represented by `!`) which is
referred to as the "force unwrap operator". In Swift, for example, you can cast a string to an integer with `Int("5")`
but the string being cast may not be a valid number, so that statement will return `null` rather than an integer if the
string cannot be turned into an integer. If you want to ensure that the statement does not return `null` you can instead
write `Int("5")!`. If you do that, an exception will be thrown if the statement would return `null`.

In GraphQL, the `!` operator will act similarly. In the case that `name` does not exist, the query will return an
error to the client rather than any data.
lizjakubowski marked this conversation as resolved.
Show resolved Hide resolved

On web where codegen is not used, the client no longer needs to handle the case where expected fields are missing.
twof marked this conversation as resolved.
Show resolved Hide resolved
On mobile platforms where codegen is used, clients have full control over the nullability of the properties on the
generated types. Since nullability is expressed in the query rather than the schema, it's flexible enough to accommodate
various use-cases (e.g., where the business `name` _is_ allowed to be nullable).
twof marked this conversation as resolved.
Show resolved Hide resolved

### `!`

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
There are times where a field can be `null`, but a feature that queries for 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.
twof marked this conversation as resolved.
Show resolved Hide resolved

### ✨ Examples

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

```swift
struct GetBusinessNameQuery.Data.Business {
let name: String // lack of `?` indicates that `name` will never be `null`
}
```

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

## ✅ RFC Goals
- Non-nullable syntax that is based off of syntax that developers will already be familiar with

## 🚫 RFC Non-goals

## 🗳️ Alternatives considered

### Make Schema Fields Non-Nullable Instead
Discussion on [this topic can be found here](https://medium.com/@calebmer/when-to-use-graphql-non-null-fields-4059337f6fc8)
Copy link
Owner Author

Choose a reason for hiding this comment

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

TODO: Flesh this out with things about versioning, breaking changes, and the impact it has on partial results

// We definitely want to flesh this out

### Use a directive such as `@non-null` instead
This is the alternative being used at some of the companies represented in this proposal for the time being.
However this feels like a common enough need to call for a language feature. A single language feature
rather than using directives as a workaround would enable more unified public tooling around GraphQL.
twof marked this conversation as resolved.
Show resolved Hide resolved

### 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

// TODO: This was rejected when it was proposed. Looking for more info as to why
twof marked this conversation as resolved.
Show resolved Hide resolved

## 🙅 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.

## Implementation
GraphQL.js: https://github.yelpcorp.com/wxue/graphql-js