Skip to content

Update CCN RFC, and introduce a decision log #966

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.

Sign up for GitHub

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

Merged
merged 7 commits into from
May 16, 2022
Merged

Update CCN RFC, and introduce a decision log #966

Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
3d1afd5
list syntax decision detailed
twof May 14, 2022
fba9063
loosened restrictions described
twof May 14, 2022
320c6a7
more decision logs
twof May 14, 2022
d3252e4
more decision logs
twof May 15, 2022
3887c0f
null propagation rules updates
twof May 15, 2022
7d580ab
reduced max line length
twof May 15, 2022
507d1f9
spelling fixes
twof May 15, 2022
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
172 changes: 144 additions & 28 deletions rfcs/ClientControlledNullability.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,11 +15,9 @@ nullability of fields for individual operations.

## Definitions

- **Required field** - A field which is modified with `!` such that a non-null value is required on
a Nullable or Non-Nullable type.
- **Required field** - A field which is marked with `!`.

- **Optional field** - A field which is modified with `?` such that a null value is allowed on a
Non-Nullable or Nullable type.
- **Optional field** - A field which is marked with `?`.

## 📜 Problem Statement

Expand All @@ -36,11 +34,10 @@ thrown. Beyond
a request can
> have different authorization rules.

The problem with the SDL Non-Nullable (!) is that it eliminates the possibility of partial failure
The problem with the SDL Non-Nullable (`!`) is that it eliminates the possibility of partial failure
on a given type. This forces schema authors to decide for which fields partial failure is
acceptable. A GraphQL schema author
may not be in the best position to predict whether partial failure will be acceptable or
unacceptable for every canvas that makes use of a field.
acceptable. A GraphQL schema author may not be in the best position to predict whether partial failure
will be acceptable or unacceptable for every canvas that makes use of a field.

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
Expand All @@ -56,34 +53,31 @@ Each client controlled nullability designator overrides the schema-defined nulla
it's attached to for the duration of the operation.

### `!`
The proposed client-controlled required designator would have identical semantics to the current
schema-defined
[Non-Null](https://spec.graphql.org/draft/#sec-Executing-Selection-Sets.Errors-and-Non-Null-Fields).
Specifically:

- If during ExecuteSelectionSet() a field **designated required by the operation or** with a
non-null fieldType raises a field error then that error must propagate to this entire selection
set, either resolving to null if allowed or further propagated to a parent field.
The proposed client-controlled required designator would have similar, but not identical semantics to the current
schema-defined [Non-Null](https://spec.graphql.org/draft/#sec-Executing-Selection-Sets.Errors-and-Non-Null-Fields).
Specifically if a required field resolves to `null`, then `null` propagation extends to the nearest optional
parent rather than the nearest nullable parent. In the event that no optional parent exists, the `data` field
of the response will be `null`.

### `?`
The proposed client-controlled optional designator would have identical semantics to the current
schema-defined default behavior. Fields that resolve to `null` return `null` for that field with no
additional side effects.
schema-defined default behavior. Fields that resolve to `null` return `null` for that field. Additionally,
fields marked with `?` act as a stopping point for `null` propagation caused by required fields.

## ✅ Validation

If a developer executed an operation with two fields name `foo`, one a `String` and the other an
`Int`, the operation would be declared invalid by the server. The same is true if one of the fields
is designated required but both are otherwise the same type. In this example, `someValue` could be
is designated required but both are otherwise the same type. In this example, `nickname` could be
either a `String` or a `String!` which are two different types and therefor can not be merged:

```graphql
fragment conflictingDifferingResponses on Pet {
... on Dog {
someValue: nickname
nickname
}
... on Cat {
someValue: nickname!
nickname!
}
}
```
Expand All @@ -94,7 +88,7 @@ The client can express that a schema field is required by using the `!` syntax i
definition:
```graphql
query GetBusinessName($id: String!) {
business(id: $id) {
business(id: $id)? {
name!
}
}
Expand All @@ -120,7 +114,7 @@ requirements for a particular feature. For example, using a GraphQL client like
mobile, the following query
```graphql
query GetBusinessName($id: String!) {
business(id: $id) {
business(id: $id)? {
name!
}
}
Expand All @@ -143,7 +137,7 @@ is more ergonomic to use since the developer does not need to unwrap the value e
accessed.

### 3rd-party GraphQL APIs
Marking field Non-Nullable in schema is not possible in every use case. For example, when a
Marking a field Non-Nullable in schema 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 required in their application. Even within
Expand Down Expand Up @@ -246,7 +240,7 @@ field’s ancestors to find the next nullable field. In the following GraphQL re
}
```

If isStarred is Non-Nullable but returns `null` and business is nullable, the result will be:
If `isStarred` is Non-Nullable but resolves to `null` and business is nullable, the result will be:

```json
{
Expand All @@ -256,15 +250,15 @@ If isStarred is Non-Nullable but returns `null` and business is nullable, the re
}
```

Even if `name` returns valid results, the response would no longer provide this data. If business is
Even if `isStarred` resolves to a valid result, 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
In the case that the service storing business stars is unavailable, the UI may want to go ahead and
render the component without a star (effectively defaulting `isStarred` to `false`). A Non-Nullable
field 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 to render.
Expand All @@ -282,4 +276,126 @@ generation.
### Alternatives to `!`
#### `!!`
This would follow the precedent set by Kotlin. It's more verbose and diverges from GraphQL's SDL
precedent.
precedent.

## Decision Log

This proposal started out with a very simple premise and implementation, and has gotten more complex as
the community has explored edge cases and facets about how GraphQL is actually used in practice. For
example this proposal starts out by talking about accommodating the "best practices" that are recommended
by the GraphQL documentation and the community, but we discovered pretty early on that there are
legitimate use cases where the "best practices" are rightfully ignored. Some of those use cases are
covered in "`?` as a counterpart to `!`".

In order to cover instances like that, we've needed to justify additional complexity which can be
difficult to understand for newcomers without (at this point a full year) of context. This decision
log was written with newcomers in mind to avoid rediscussing issues that have already been hashed out,
and to make it easier to understand why certain decisions have been made. At the time of writing,
the decisions here aren't set in stone, so any future discussions can use this log as a starting point.

### `?` as a counterpart to `!`

Lee was the first person [to suggest](https://github.com/graphql/graphql-spec/issues/867#issuecomment-840807186)
that the inverse of `!` should exist and that it should be represented by `?`. The
[reasoning](https://github.com/graphql/graphql-spec/issues/867#issuecomment-841372320) was that it "completes
the story of control" and provides a guaranteed stopping point for `null` propagation if we're using the existing
`null` propagation rules. The feeling was that "introducing ! without ? is like introducing `throw` without `catch`".

Lee also surfaced that there are some use cases like his own at Robinhood where they're trying to balance
developer experience and data preservation, and have opted to mark quite a few fields `Non-Null`. Data
preservation is very important because Robinhood is working with financial data, so they have the opposite
problem where they sometimes want to be able to halt `null` propagation, rather than the inverse use case which
this proposal originally supported.

Developers from Apollo indicated that many of their customers face problems around schema breaking where the
solution to developer experience gripes is to make a breaking change and swap a field from nullable to `Non-Nullable`
or vice versa, which can be a labor intensive process.

Since there seemed to be general consensus that `?` was a good addition to the proposal, it was adopted without a vote.

Subsequently there was discussion around whether `?` could be introduced in a later proposal, and there was general
agreement that the usability of `!` is limited without `?`, and the selected `null` propagation behavior described
below solidifies the decision to introduce both additions in a single proposal.

### List syntax
Developers from Apollo [suggested](https://github.com/graphql/graphql-spec/pull/895#issuecomment-961442966) early
on that users would want to apply CCN syntax to list elements. The possibility had been suggested earlier than
that as well, but it was put off because neither Netflix nor Relay's CCN counterparts had the feature, and it
hadn't been a problem yet. However there was enough interest during community feedback sessions to adopt it
into the proposal. [Discussions](https://github.com/graphql/graphql-wg/discussions/864) around which specific
syntax to adopt happened over the following months.

Options other than the one that was landed on included the following:

```graphql
twoDimensionalList!!?
```
The folks that voted against this option felt that it was unclear how it should be interpreted, whether operators
should be applied from the outside-in, or inside-out.

```graphql
twoDimensionalList as [[Int!]!]
twoDimensionalList <= [[Int!]!]
```
The folks that voted against this option felt that this option read like a type-cast, and that the inclusion
of a type placed an undue burden on client developers. Validation would fail if the type was incorrect, and
didn't provide much additional value.

```graphql
twoDimensionalList[[!]!]?
```
This syntax, called "the bracket syntax" during discussions was selected for adoption by majority vote at the
[March 3rd, 2022 GraphQL Working Group Meeting](https://github.com/graphql/graphql-wg/blob/main/agendas/2022/2022-03-03.md).
9 out of 10 participants voted for this option with the final vote going to the `<= [[Int!]!]` option.

Initially there was a restriction on the bracket syntax where the depth of the syntax needed to match depth of
the field's list type, but participants at the same meeting felt that restriction should be loosened so
that developers could opt to apply the syntax to only the field itself and ignore the elements of the
list. Under that new rule the following would also be valid, and the two examples would be equivalent.

```graphql
twoDimensionalList!
twoDimensionalList[[]]!
```

There are however some open concerns that the first of the two examples could be ambiguous as to whether
the `!` applies to the field as a whole or to the list elements.

### `!` propagates `null` to nearest `?` rather than nearest nullable field
The selected mechanics were most requested by the folks at Meta working on [Relay](https://relay.dev/).
Relay wanted this behavior for a few reasons
- Relay presents a facade of fragment isolation for its own
[`@required` directive](https://relay.dev/docs/next/guides/required-directive/). If a field is `null`,
rather than merging fragments and propagating `null` to all sibling fields on selection sets that use
that fragment, the most popular option as chosen by ~90% of developers is to have the request throw
and utilize React's error boundaries. It also has the option to do `null` propagation, but that too
is bound to a single fragment. Because of this, it likely won't be able to use CCN at first, but
developers would like to be able to use it in the future once
[Fragment Modularity](https://github.com/graphql/graphql-wg/blob/main/rfcs/FragmentModularity.md)
makes it into the spec. The selected option preserves the possibility of Relay and other clients that utilize
fragments heavily using CCN in the future.
- The throwing option that Relay provides on their `@required` directive effectively allows developers
to indicate "If some required field is missing, throw out everything from this field to the fragment boundary"
which is tighter client control than was offered by the initial iteration of this proposal where `!` only
transformed a field into a `Non-Null`. In that case, the server still had control over where `null`
propagation halted with which fields the schema said were nullable. The selected `null` propagation option
is slightly closer to the most popular `@required` option in that way.

With the initial iteration of this proposal if users wanted to guarantee that all fields through multiple
parents should be lost in the event that a child is `null`, they would need to mark each level with a `!`,
but the selected option avoids that.

With the selected option, forgetting to include a `?` is potentially dangerous because it would result
in more fields being lost than intended, all the way up to the `data` field in the worst case scenario.
There were concerns that that was a blocker, but arguments were made that because most queries are relatively
small, it wasn't actually that dangerous. Clients have also often been treating the existence of any errors
as a failed request and thrown out entire responses, so in effect, clients been choosing the "worst case
scenario" when given the option.

The behavior where `!` propagates `null` to nearest `?` was selected for adoption by majority vote at the
[March 3rd, 2022 GraphQL Working Group Meeting](https://github.com/graphql/graphql-wg/blob/main/agendas/2022/2022-03-03.md).
7 out of 8 participants voted for this option with the final vote going to behavior where the `!` would be non-destructive.

The non-destructive option was turned down because having different behavior per-client wasn't desirable, and
it provided no benefits to naive clients (like a bash script) because extra processing would be required
for it to be a value-add.