diff --git a/rfcs/ClientControlledNullability.md b/rfcs/ClientControlledNullability.md index 7ab81c39..6d01781a 100644 --- a/rfcs/ClientControlledNullability.md +++ b/rfcs/ClientControlledNullability.md @@ -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 @@ -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 @@ -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! } } ``` @@ -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! } } @@ -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! } } @@ -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 @@ -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 { @@ -256,7 +250,7 @@ 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 { @@ -264,7 +258,7 @@ Non-Nullable, the response will be: } ``` -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. @@ -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. \ No newline at end of file +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.