Update CCN RFC, and introduce a decision log (#966)

* list syntax decision detailed

* loosened restrictions described

* more decision logs

* more decision logs

* null propagation rules updates

* reduced max line length

* spelling fixes

Author: twof

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,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.
@@ -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.