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

[RFC] Client controlled nullability operator #867

Open
twof opened this issue May 8, 2021 · 16 comments
Open

[RFC] Client controlled nullability operator #867

twof opened this issue May 8, 2021 · 16 comments
Labels
💡 Proposal (RFC 1) RFC Stage 1 (See CONTRIBUTING.md)

Comments

@twof
Copy link
Contributor

twof commented May 8, 2021

You can read a fuller version of the RFC here.

TL;DR:

This RFC proposes new syntax for GraphQL clients to express that fields in an operation are non-null.

Problem Statement

The official best practice for schema design is to make all fields nullable by default. If “null” is not an appropriate value for a failed field, the guidance recommends making it nonNull in the SDL.

The problem with the SDL nonNull (!) is that it eliminates the possibility of partial failure on a given type. This forces the schema 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 every possible use case.

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.

🧑‍💻 Proposed Solution

Three new operators are introduced to the query language:

  1. A "required designator" represented by ! which marks a field Non-Nullable
  2. An "optional designator" represented by ? which marks a field Nullable
  3. A "list operator" represented by [] which allows the above two operators to be applied to the elements of a list, or the list as a whole.

🎬 Behavior

The proposed query-side Non-Null designator and SDL-defined Non-Null would have slightly different behavior. When null is resolved for a SDL Non-Nullable field, the executor propagates until it finds the nearest Nullable parent, and sets that to null.

This proposal also introduces a second "optional designator", ?, which designates that the field is Nullable, even if it is Non-Nullable in the schema. When null is resolved for a field marked with a required designator (!) the executor instead propagates null to the nearest field marked with an optional designator (?). If there is no optional designator to propagate to, then the response's data field will become null ie the entire response will be lost. Because of that, in most cases we expect that required designators will be paired with optional designators to preserve partial responses.

✏️ Proposed syntax - !

The client can express that a schema field is required by using the ! syntax in the query definition:

query GetAllBusinesses {
  businesses [!]? {
    name!
  }
}

We have chosen ! because ! is already being used in the GraphQL spec to indicate that a field in the schema is non-nullable. ? chosen because other languages use it to indicate optionality. [] was used because it's traditionally been used to indicate a list.

If a field is a list, designators can either be applied directly to the field ie twoDListField! or to the elements of the list with list operators matching the number of dimensions of the list ie twoDListField[[!]]!.

✨ Use cases

Improve the developer experience using GraphQL client codegen types

Handling nullable values on the client is a major source of frustration for engineers, especially when using GraphQL codegen types in strongly-typed languages.

The proposed nonNull designator would allow GraphQL clients to generate codegen types with more precise nullability requirements for a particular feature. For example, using a GraphQL client like Apollo GraphQL on mobile, the following query

query GetBusinessName($id: String!) {
  business(id: $id) {
    name!
  }
}

would codegen to the following type in 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
    }
  }
}

If a null business name is not acceptable for the feature executing this query, this generated type is more ergonomic to use since the developer does not need to unwrap the value each time it’s accessed.

@leebyron
Copy link
Collaborator

I love this, and to elaborate a bit and try to apply some definitions and following thoughts:

This is client control to transform a Nullable to a Non-Null, but I would like to see the opposite appear in this same RFC - converting a Non-Null to a Nullable. Terms for those:

  • Client controlled nullability - overall name for both behaviors
  • "Required field" - A field which is modified with ! such that a non-null value is required on a Nullable type
  • "Optional field" - A field which is modified with ? such that a null value is allowed on a Non-Null type. Alternatively I've talked about this idea as "Error Boundary"

Some thoughts on validation:

  • Should we require ? only be allowed to use on a Non-Null field? It's currently "not safe" to convert a Non-Null field type to a nullable type, since that could result in null values being sent to a client which wasn't designed to receive them, but I don't think we have a case where such a change would result in a previously valid query becoming invalid.
  • Currently it's considered safe to replace a nullable type with a non-null type, since it's a subset of the possible return values. Existing clients should continue to work. That implies that we might want to allow ! on a Non-Null type, since otherwise a query with a required field could become invalid after this currently-considered-safe change.
  • When performing "Overlapping Fields Can Be Merged" validation, we'll need to consider these modifiers. Is it safe or confusing to allow different uses? Is it burdensome to not allow? Is { field, field! } ok? What about { field, field? } or {field?, field!}?
  • When performing "CollectFields()" at execution, we'll need to consider these modifiers and the validation rule above. If we require the same modifiers (most strict interpretation) then likely no changes are necessary, otherwise some caution should be taken to select the appropriately modified field.

@leebyron leebyron added the 💡 Proposal (RFC 1) RFC Stage 1 (See CONTRIBUTING.md) label May 13, 2021
@AnthonyMDev
Copy link

Maintainer of the Apollo GraphQL iOS library here. I am very much in favor of this proposal. As @twof stated, in Swift, dealing with the nullability of most fields becomes very cumbersome. Most developers end up explicitly (ie. unsafely) unwrapping fields all over the place or having to wrapping our generated models in their own view models, where they do nullability checks and validation.

We've been mulling this idea around internally for a while now, but never got around to making a proposal. I'm glad someone else got around to making the proposal.

@leebyron I'm not sold on the idea of converting Non-Null fields to Nullable. As you describe, it adds a lot of complexity and edge cases. But I have a hard time seeing compelling uses cases that would warrant providing the functionality. If you have example use cases, I'd love to hear them!

@benjie
Copy link
Member

benjie commented May 14, 2021

Regarding making a Non-Nullable field nullable (e.g. via ?), what effect would this have on errors? Currently we have a situation where for a non-null field, if the resolver throws an error or if the field returns null then an error will be raised and will bubble up to the next nullable ancestor field. If we add ? it seems clear that if the field would have returned null it should now be allowed to return null without error, but what happens if the resolver does throw? Should it be smooshed to null and the error swallowed, or should the error still be returned in the errors field on the response?

I imagine a use case for ? is if the schema was incorrectly defined with a field marked as non-nullable that then ends up returning null or throwing errors more often than anticipated. Changing this field to be nullable would be a breaking change, but allowing clients to opt in via ? gets around this. That said, I'm not particularly keen on ?.

@benjie
Copy link
Member

benjie commented May 14, 2021

Since making a nullable field non-nullable is a non-breaking change, we must allow ! to be used on nullable and non-nullable fields alike (otherwise making the field non-nullable could invalidate previously valid operations).

Strictly speaking the same situation cannot arise for ? assuming ? only has effect on non-nullable fields (non-nullable fields cannot become nullable safely so there's no concerns about invalidating previous operations if ? were to be restricted to only non-nullable fields). However, given the above for !, it seems that ? should also be allowed on nullable and non-nullable fields alike for consistency. This raises the question: does putting ? on a nullable field have any effect? Maybe it "swallows" any error the field might raise?

@benjie
Copy link
Member

benjie commented May 14, 2021

(To be clear, I don't currently think ? should swallow errors. Just trying to test the edges 😄)

@leebyron
Copy link
Collaborator

Non-Null fields to Nullable. As you describe, it adds a lot of complexity and edge cases. But I have a hard time seeing compelling uses cases that would warrant providing the functionality. If you have example use cases, I'd love to hear them!

Same motivations as marking a field as required with ! just flipped around. This completes the story of control for where errors are caught and what result shapes are possible from execution. Since ! can cause an error at a field to bubble up, then ? gives you a location to catch that bubbling error.

Excuse the poor data modeling for this entirely contrived example, but hopefully this makes the design intent clear. In this schema every hero has a craft (non-null), ...but not every craft is capable of a top speed (nullable)

type Hero {
  name: String
  craft: StarShip!
}

type StarShip {
  topSpeed: String
}

# Queries our Hero, but only includes its craft if it has a topSpeed. Includes the hero's name regardless.
fragment Test on Hero {
  name
  craft? {
    topSpeed!
  }
}

You can see how in that example if our required topSpeed! field were to raise an error for encountering a null/error during resolution, without craft? being optional it would bubble up and remove the entire Hero object, if not more, which might not be the client's desired outcome.

Regarding making a Non-Nullable field nullable (e.g. via ?), what effect would this have on errors? Currently we have a situation where for a non-null field, if the resolver throws an error or if the field returns null then an error will be raised and will bubble up to the next nullable ancestor field. If we add ? it seems clear that if the field would have returned null it should now be allowed to return null without error, but what happens if the resolver does throw? Should it be smooshed to null and the error swallowed, or should the error still be returned in the errors field on the response?

I imagine the exact same errors would exist in the errors response, it would just alter where the null-replacement behavior applies. An error below is of course captured in the errors response, and would null at the optional field if caught there. An error at the optional field is also captured in the errors response and replaced with null as it would for a typical nullable field. Also a null value at the optional field should still be considered an error since the actual type is non-null and its unexpected to encounter a null, at which point it should use the same behavior as any other error - capture it in the errors response and apply error bubbling (replacing the optional field with null).

TL;DR: No error swallowing, all existing errors continue to report, just null replacement behavior is now controllable.

I imagine a use case for ? is if the schema was incorrectly defined with a field marked as non-nullable that then ends up returning null or throwing errors more often than anticipated. Changing this field to be nullable would be a breaking change, but allowing clients to opt in via ? gets around this.

IMHO that's a secondary use case, but still a valid one. As I mentioned before, I think a primary use case is determining where to stop error bubbling in the case an ancestor field uses ! (or otherwise has this same problem of throwing errors more than anticipated)

This raises the question: does putting ? on a nullable field have any effect?

Seems likely not. I think putting ! on a non-null field probably also should have no effect.

I think the validation rules around these are open questions which tradeoff between improved clarity of intent and back-compat resiliency. Though after thinking about this a bit, it seems weird if ! and ? had slightly different validation rules without good reason. This might be another good case for "lint" rules vs "validation". We might want to restrict ! on Non-null and ? on nullable at author time (clarity of intent), but not at runtime (back compat)


I don't think I'd advocate for introducing error boundary optional fields (?) alone, it really only makes sense along side required fields (!) since it changes the story around errors from "errors should be exceptional" to using ! to state "I'm taking an otherwise schema legal response and knowingly converting it into an error".

Obviously imperfect corollary, but IMHO introducing ! without ? is like introducing throw without catch

@benjie
Copy link
Member

benjie commented May 14, 2021

That example really clears things up; I see your intent now and it definitely seems to make sense 👍 (I didn’t think about it before because all my relations tend to be nullable anyway.)

Interestingly this does add a reason to allow (without even a lint error) ? on nullable fields: saying catch the null here even if the field is made non-nullable in future versions of the GraphQL schema. Without this, the behaviour of the operation would change if you were to make the nullable parent field non-nullable (a non-breaking change) because the non-null error would have to bubble further.

@martinbonnin
Copy link
Contributor

Could the non-nullability information somehow be made available as schema extensions (schema level) in addition to query fields (query level)? From my understanding, a lot of the nullability friction comes from:

  1. servers defaulting to nullable types for more fine-grained error control and partial responses
  2. strongly typed clients like Swift or Kotlin preferring non-nullable because handling all possible error cases is a lot of work and sometimes doesn't even make sense compared to displaying a generic error to the user.

For an example, the star wars API has nullable node in PeopleEdge:

type PeopleEdge {
  cursor: String!
  node: Person
}

This comes up super confusing to an app developer as there's not really much to be done if edge.node is null. Displaying a generic error is certainly the thing to do if the UI cannot accommodate null. But having to make this decision in app code feels a bit awkward as this is something that could be handled more generically during parsing.

I think there would be some value in allowing codegen clients to augment the schema on their side and do some "impedance matching" with what the server exposes. This could be done using the current grammar with a directive and type extensions only known to the client:

# client/schema-extensions.graphqls
extend type PeopleEdge @nonnull(fields: ["node"])

Or using new syntax elements and validation rules:

# client/schema-extensions.graphqls
extend type PeopleEdge {
  node: Person!
}

In larger apps, this would allow to manage nullability in a central place without each developer having to opt-in the non-nullability manually.

@aprilrd
Copy link

aprilrd commented May 17, 2021

One more consideration: The currently proposed syntax does not have an obvious answer for the nullable list items. @martinbonnin's second suggested syntax (copied below) lets you mark the list items not nullable. But it would be too aggressive for components with different nullability requirements as suggested by comments like this: twof#1 (comment)

# @martinbonnin's second suggested syntax
extend type PeopleEdge {
  node: Person!
}
# a way to mark list items non-nullable
extend type PeopleConnection {
  edges: [PeopleEdge!]!
}

@aprilrd
Copy link

aprilrd commented May 21, 2021

To solve the nullable list items, what do folks think about this syntax? To extend the example above:

query {
  peopleConnection {
    node {
      name
    }
  }! # Notice "!" here. "!" makes list items non-nullable.
}

! after } will be only valid if the type is a list type. ? can be applied similarly. Do you see any pitfalls or drawbacks?

@martinbonnin
Copy link
Contributor

In theory, you could have an arbitrary number of nested Lists where each item type could be nullable:

type Transformation {
  # here everything is nullable
  rotationMatrix: [[Int]]
  # here everything is non-nullable
  translationMatrix: [[Int!]!]!
}

Adding a single ! will work in the vast majority of use cases but not in the general case.

@leebyron
Copy link
Collaborator

leebyron commented Jul 2, 2021

To solve the nullable list items, what do folks think about this syntax? To extend the example above:
Do you see any pitfalls or drawbacks?

Two major drawbacks:

  • Expressivity: This syntax would only allow you to control one level of List items.
  • Legibility: This syntax moves the field modifier far away from the field itself. In the case that you have a large selection set, it would be very challenging to read where the ! modifier actually applies and would be easy to miss.

I'm not entirely convinced we need to extend this concept down to items within Lists, but if we do, I'd suggest just allowing the modifier to be a sequence of modifiers:

fragment T on Transformation {
  # just requires that the matrix itself exist, inner item nullability still applies
  a: rotationMatrix!
  # requires that the matrix exist and items of the first list are non-null
  b: rotationMatrix!!
  # requires that the matrix, items, and nested items are non-null
  c: rotationMatrix!!!
  # requires that items and nested items are non-null, but catches that error at the field level (doesnt bubble the error)
  d: rotationMatrix?!!
}

@benjie
Copy link
Member

benjie commented Jul 4, 2021

I like Lee’s solution above, but worry that it may complicate us adding another wrapping type (in addition to List and NonNull) in future. No idea what such a wrapping type may be though, and we haven’t needed one yet…

@twof
Copy link
Contributor Author

twof commented Jul 7, 2021

Hey there! Quick update for everyone. I got pulled into another project at work and haven't been able to focus on this, but I'm hoping to wrap it up in the next couple of weeks, and then I'll be able to focus on this full time. We've started on an implementation, and we're looking to make sure we've got all of our corner cases covered next. You can follow our progress here: aprilrd/graphql-js#5

We're currently implementing both the non-null operator (!) and its inverse (?). We haven't dealt with list items yet.

kitten added a commit to 0no-co/graphql-web-lite that referenced this issue Oct 8, 2021
The ["Nullability RFC" for GraphQL](graphql/graphql-wg#694)
allows fields to individually be marked as optional or required in a query by the
client-side. ([See Strawman Proposal](graphql/graphql-spec#867))

If a field is marked as optional then it's allowed to be missing and `null`, which
can control where missing values cascade to:

```graphql
query {
  me {
    name?
  }
}
```

If a field is marked as required it may never be allowed to become `null` and must
cascade if it otherwise would have been set to `null`:

```graphql
query {
  me {
    name!
  }
}
```
kitten added a commit to urql-graphql/urql that referenced this issue Oct 8, 2021
The ["Nullability RFC" for GraphQL](graphql/graphql-wg#694)
allows fields to individually be marked as optional or required in a query by the
client-side. ([See Strawman Proposal](graphql/graphql-spec#867))

If a field is marked as optional then it's allowed to be missing and `null`, which
can control where missing values cascade to:

```graphql
query {
  me {
    name?
  }
}
```

If a field is marked as required it may never be allowed to become `null` and must
cascade if it otherwise would have been set to `null`:

```graphql
query {
  me {
    name!
  }
}
```

In Graphcache, we imagine that the nullable field — which would be marked with
`required: 'optional'` — can allow Graphcache to make more data nullable and
hence partial, which enhances schema awareness, even if it's not actively used.

The required fields — which would be marked with `required: 'required'` — would
force Graphcache to include this data, regardless of what schema awareness may
say, which also enhances partial data in the presence of schema awareness, since
it increases what the cache may deliver.

In other words, it guarantees a "forced outcome" in both cases, without having to
look up whether a field is nullable in the schema.
In the future, we may even derive the `RequiredStatus` of a `FieldNode` in an
external place and never call `isFieldNullable` with a schema in the query
traversal.
@leebyron leebyron changed the title [Strawman] Provide a non-null designator in GraphQL operations Client controlled nullability operator Nov 4, 2021
@leebyron leebyron changed the title Client controlled nullability operator [RFC] Client controlled nullability operator Nov 4, 2021
@TomPeas
Copy link

TomPeas commented Apr 11, 2024

@twof Whats the update on this, there has been no discussion for years and I cannot find a reason the PR was closed?

@twof
Copy link
Contributor Author

twof commented Apr 11, 2024

@twof Whats the update on this, there has been no discussion for years and I cannot find a reason the PR was closed?

#895 (comment)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
💡 Proposal (RFC 1) RFC Stage 1 (See CONTRIBUTING.md)
Projects
None yet
Development

No branches or pull requests

7 participants