graphql-spec

graphql-spec copied to clipboard

Rendered proposal: https://github.com/graphql/graphql-wg/blob/main/rfcs/ClientControlledNullability.md Spec Implementation: https://github.com/graphql/graphql-js/pull/3281 Working Group Action Item: https://github.com/graphql/graphql-wg/issues/694 Known issues: https://docs.google.com/document/d/10MmZbzfPtk8GoAeSVm_m9ng8Aj10LBXVIQwkpjti6CQ/edit?usp=drivesdk

The committers are authorized under a signed CLA.

• :white_check_mark: Alex Reilly (849c7ac63bc6d8da217a371d0223b39d696033d9)

✅ Deploy Preview for graphql-spec-draft ready!

Name	Link
🔨 Latest commit	159d15946bee00c9c4bc2c01016a7b5f77cf47bb
🔍 Latest deploy log	https://app.netlify.com/sites/graphql-spec-draft/deploys/627ec36711a9870008e81b80
😎 Deploy Preview	https://deploy-preview-895--graphql-spec-draft.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

---

To edit notification comments on pull requests, go to your Netlify site settings.

What's the effect on the execution section?

What's the effect on the execution section?

https://github.com/graphql/graphql-spec/pull/895#discussion_r739561291

Ok, I took a look at Execution:

I believe the key parts are ExecuteField(), CompleteValue(), and ExecuteSelectionSet() (which calls ExecuteField())

ExecuteField() is passed both a fieldType and a set of fields, which it passes on to CompleteValue(). CompleteValue() is responsible for coercing and validating the returned value against the provided type. I think this is an ideal place to make a note that the _fieldType_ will reflect the field's nullability qualifiers and explain a bit.

ExecuteSelectionSet() is currently where that fieldType is actually derived as part of it's inner loop. Specifically here you can see this comes directly from the schema. (Ideally this should actually happen in ExecuteField, but there is a checking step that happens within this inner loop)

• We need to introduce a step (perhaps a whole subroutine) that applies a field's nullability qualifiers to result in the expected type.
• We'll need to think a bit about how to name this so that we can make it clear what type is coming from the schema definition, and which type is the result of applying nullability qualifiers.

Also there is the whole section on Handling Field Errors. We'll definitely need to clarify what we mean by "field type" in this section and make an explicit reference to nullability qualifiers.

What's occurring to me is that terminology is going to be really important to avoid confusion. We previously could just simply call something "field type" but now we have two different kinds of field types - those defined in a schema, and those modified by a selection set field.

(Slightly unrelatedly, IMHO we should introduce error returning mechanics into our spec algorithms directly and rewrite some of this to be more clear. Right now my read is that we're still too hand-wavy about how errors work)

@leebyron Thanks for the info! I've added a step to ExecuteField() that describes how we combine the schema field's nullability and requested field's RequiredStatus to alter the field's nullability for the remainder of execution.

I'm still working on Handling Field Errors, and I think I've got some more work to do on validation.

I've got a question about how the schema and GraphQL.js are related. I've made my change in ExecuteField because that's where we take that step in our GraphQL.js implementation, but it looks like the relationship between the spec and GraphQL.js isn't as strict as I thought it was. For example, some of the steps described in ExecuteSelectionSet() happen prior to executeFields() (Let {groupedFieldSet} be the result of {CollectFields(objectType, selectionSet, variableValues)}.), some happen during, and some happen within executeField() instead such as If {fieldType} is defined:. Is GraphQL.js intended to be a 1:1 representation of the spec? If not, is are my changes to the spec in the right place, or should they be in ExecuteSelectionSet() instead?

Is GraphQL.js intended to be a 1:1 representation of the spec? If not, is are my changes to the spec in the right place

The goal is for it to be as close to 1:1 as is reasonable. Spec algorithms should completely describe behavior, just as JavaScript should.

In practice there are some differences because of how JS must handle exception throwing and promise resolution and rejection. Because of that, GraphQL.js has a slightly more complicated inner execution loop that's below the level of abstraction that the spec describes (eg. JS specific).

There have also been some iteration in both places, and so some divergence has probably happened. Ideally if we find that a difference between them is not related to a JS specific detail then we can adjust one or the other to match again.

I really love this proposal, and I'm glad to see it being pushed forward. However it seems that we haven't yet come to a decision on how to handle list types. I feel that we need to address that before adding this to the spec permanently.

Is the current plan to just allow the outermost list type to have client controlled nullability, but not the inner elements (including nested lists)?

While initially that seems alright, as we could incrementally add the functionality for inner lists later, I would caution about finalizing the syntax on this without making a determination on how that might work in the future.

The provided syntax in this proposal, while elegant, does not account for list items, and I don't really see a great way that we can make it make sense in the future. My fear is that this will lead to us either: a) never supporting that feature (and I think we should), or b) having significantly diverging syntax for list nullability (which I think would be a shame, as the language is relatively elegant today).

Even if we don't implement list element nullability controls now, we should consider them more thoroughly. before moving forward.

Proposed Solutions

For posterity, I'd like to give a summary of the proposed solutions that I have seen come up so far.

Nullability Modifier at End Of Selection Set

@aprilrd made the suggestion of putting the nullability modifier at the end of the selection set for list fields, rather than on the name of the field.

# requires that the list itself exists, inner item nullability still applies
query {
  rotationMatrix! {
    node {
      name
    }
  }
}

# requires that the list exist and items of the list are non-null
query {
  rotationMatrix! {
    node {
      name
    }
  }! # Notice "!" here. "!" makes list items non-nullable.
}

# requires that items are non-null, but catches that error at the field level (doesn't bubble the error)
query {
  rotationMatrix {
    node {
      name
    }
  }! # Notice "!" here. "!" makes list items non-nullable.
}

While I like this direction, @martinbonnin & @leebyron have pointed out two problems with this solution.

• 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.

Sequence of Modifiers

In a previous thread, @leebyron suggested using 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 (doesn't bubble the error)
  d: rotationMatrix?!!
}

While this solves the problem with the least modification to the existing RFC, I don't think it's expressive enough. It's not very clear what is going on here and takes a bit of effort to grok.

Full Type Representations

@martinbonnin and @twof have talked about annotating the fields with their full type representations.

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

I certainly prefer this for the expressiveness and clarity. As @benjie pointed out, this would be more flexible for implementations of future container types (such as Dictionary). While the above solution may complicate that.

However this does have its drawbacks. If we did this, I'd think we would want to implement the same syntax for non-list fields as well for syntactical consistency. In that case, we have unneeded verbosity in non-list fields for the sake of consistency.

Additionally, operation definitions have never needed to provide information about the types of fields in the past, and providing for that opens up other questions. What do we do if the internal named type is a mismatch with the schema? That would require additional validation, which does affect performance (albeit minutely).

Container Representation - Omitting Internal Type

@twof Also proposed that we could omit the internal named type, while maintaining the container syntax like so:

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 (doesn't bubble the error)
  d: rotationMatrix: [[!]!]
}

While this removes the problem of adding in unnecessary type information, I think that its still a bit difficult to grok and visualize.

New Suggestions

Here are a couple of alternatives I've come up with that could be considered. I'm not supporting any of them as better than the previously proposed solutions; I just wanted to get them out there as concepts for discussion.

Container Representation - Placeholder for Internal Type

This is the same as the above solution, but uses a placeholder instead of an empty container. I think that a placeholder where the element should be makes it easier to read and understand. Here, I'll show examples using different placeholders, so that you can visualize what each of those would look like, but we would agree on a single valid placeholder.

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 (doesn't bubble the error)
  d: rotationMatrix: [[_!]!]
}

I think that makes it a little more visually appealing and clearer to understand. But it's still not clear or intuitive to me what this is doing without reading about it in the spec or some guide the explains it. (Though that may be the case with all implementations of this feature, list or not).

Of the above suggestions, I think that $ or _ as placeholders are the most reasonable.

Container Representation - Agnostic to Internal Type Syntax

Since the nested type doesn't have any impact on the implementation of this feature, we could theoretically just ignore it. If we just ignored anything in-between the [[]] it would allow consumers to develop their own style guidelines for this and use it as is most expressive for their use case. In this case, all of the following would be legal.

fragment T on Transformation {
  a: rotationMatrix: [[]]!
  b: rotationMatrix: [[_]!]!
  c: rotationMatrix: [[ !]!]!
  d: rotationMatrix: [[element!]!]
  e: rotationMatrix: [[item!]!]
  f: rotationMatrix: [[$!]!]
  g: rotationMatrix: [[Int!]!] # This would work even if the type of the field is not an Int.
}

Container Representation - Wrapping Field Name

I actually think this is not a good choice, but for the sake of being exhaustive, I've included it.

Instead of having an element inside of the container, we could wrap the field name.

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 (doesn't bubble the error)
  d: [[rotationMatrix!]!]
}

I think this is really confusing though. This makes me thing that we have a list of lists of whatever the type of rotationMatrix is. Which is not the case. We are really talking about the elements of the list rotationMatrix, not the field itself. Perhaps something like `a: [[rotationMatrix.element]]! would be more clear, but I just generally wouldn't go down this route.

Summary

Of the proposed solutions thus far, I think that I prefer us selecting a placeholder for the internal type of lists. And I lean towards using $ as that placeholder. I'm not 100% ecstatic about any of these solutions, but this seems like one that is:

• Easy to understand
• Visually appealing
• Not overly verbose
• And most importantly, most easily adaptable to future container types.

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 (doesn't bubble the error)
  d: rotationMatrix: [[$!]!]
}

I think my main issue with the list nullability operators is their use of colon. That makes sense given the idea of providing a direct type annotation, but I find the repeated colon hard to read. I like to think of that recommendation as a "type cast" in which case maybe a keyword would be easier to follow. TS and Swift both use as, so following that precedent:

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

I think that's a quite interesting way to apply additional nullability, but also can see it as a general downcasting tool. Interesting, but I don't necessarily love that this would create multiple ways to do casting:

fragment S on Story {
  # Assuming `author` is an interface, and `Person` is one possible type. Throws an error if `author` is not `Person`
  a: author as Person {
    name
  }
  # Existing supported behavior, but doesn't throw an error, just omits the subfield.
  b: author {
    ... on Person {
      name
    }
  }

I think the major downside here is it's verbosity. It requires you to repeat the typename when you just want to apply the non-null.

---

Riffing on my original idea of repeated operators, and a few of the ideas above of introducing [] syntax directly, perhaps these could be combined in a way that still reads like an operator:

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 (doesn't bubble the error)
  d: rotationMatrix[[!]!]?
  # BONUS, couldn't do these before with my prev syntax proposal:
  # requires that nested items in the matrix exist, but does not alter the rows or matrix type itself
  e: rotationMatrix[[!]]
}

Following up on the discussion today, I was wondering if we could also consider nullability syntax for input types? Like I said, I see the primary benefit of this proposal as a way to help clients prepare for nullability changes in servers, and one big breaking change that can be made by graph implementers is to turn a nullable input into a required one. If you’re reading operations on their own, you can’t really tell if variables are always provided, so having nullability syntax as a way for clients to indicate that they expect a certain input type as required independently of the schema would help with deployment deadlocks.

If this proposal already has syntax for input types, ignore this comment, for I am not always a close reader.

Can you share an example of what that might look like? I thought the case you described is already possible, but I may have misunderstood

Right now I don't believe we have any input specific changes in this RFC.

@leebyron Right so the big bad changes that servers can make are 1. make non-nullable fields nullable, and 2. make nullable inputs non-nullable, just by like basic type theory. I was thinking that there would be syntax that allowed developers to mark field arguments as non-nullable, in preparation for a type 2 change, but as it turns out, this is already easily discoverable because you can find the variable an argument is referencing and check its nullability. So false alarm, my bad, and this proposal wouldn’t be necessary for allowing easier type 2 changes.

It’s exciting though! Maybe this is the start of semantic analysis of GraphQL schemas and operations.

Riffing on my original idea of repeated operators, and a few of the ideas above of introducing [] syntax directly, perhaps these could be combined in a way that still reads like an operator:

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 (doesn't bubble the error)
  d: rotationMatrix[[!]!]?
  # BONUS, couldn't do these before with my prev syntax proposal:
  # requires that nested items in the matrix exist, but does not alter the rows or matrix type itself
  e: rotationMatrix[[!]]
}

I actually really like this syntax. This is now my top choice for list nullability.

Grammar for that might be something like:

Nullability :
  - ListNullability NullabilityModifier?
  - NullabilityModifier

ListNullability : `[` Nullability `]`

NullabilityModifier : one of `!` or `?`

Riffing on my original idea of repeated operators, and a few of the ideas above of introducing [] syntax directly, perhaps these could be combined in a way that still reads like an operator:

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 (doesn't bubble the error)
  d: rotationMatrix[[!]!]?
  # BONUS, couldn't do these before with my prev syntax proposal:
  # requires that nested items in the matrix exist, but does not alter the rows or matrix type itself
  e: rotationMatrix[[!]]
}

I actually really like this syntax. This is now my top choice for list nullability.

If nobody has any objections to this syntax, I'm down to start implementing it and adding it to this spec. This is also my favorite out of all of the options that have been put forth.

I've added some additional notes to the Known Problems Doc based on what was discussed in the November Working Group meeting. Check it out and let me know if anything I have there is inaccurate. If you'd like to talk more about your thoughts, particularly on what should be done with the ? operator, let's keep discussion to this PR so it's centralized in one place.

A couple follow ups: @benjie During the meeting you had some thoughts that you said you wanted to formulate and write down. I'd love to hear them if you're still interested in doing that!

@leebyron For our integration into a code generation tool, is it important that we get it to a point where we can merge to main, or is it okay to just have a demo-able proof of concept? Here's a link to our working branch.

@AnthonyMDev Any thoughts on what ought to happen if a non-list field is marked with the [!] operator? Should it be a validation error? A type cast?

Elsewhere we do use validation to define clearly incorrect statements as invalid, so that seems like it would make sense for that operator as well

I agree with @leebyron, that should probably be a validation error. I think considering this any type of type cast would open a lot of doors we probably don’t want to open right now.

@AnthonyMDev Schema:

type Lists {
  list: [Int]
  threeDList: [[[Int]]]
}

Operation:

fragment foo on Lists {
  list!
}

This feels like it should be valid, and would make the list required, but leave the list elements optional. The final type of list would be [Int]!

fragment foo on Lists {
  threeDList[!]!
}

Should this also be valid? Should any required query syntax at or less than the depth of the field definition be valid? If it is valid, I feel that the final type of threeDList should be [[[Int]]!]!.

The other option is that we could require that the query syntax match the field definition list depth, so the only valid operators would look something like

fragment foo on Lists {
  list[!]!
  threeDList[[[!]!]!]!
}

The implementation is is up for review: https://github.com/graphql/graphql-js/pull/3281#issuecomment-973118901 I'd like a review on that PR before I write the spec since I'm a little more unsure about it now than prior to the list syntax being introduced.

The list syntax has the following behaviour.

Schema:

type Lists {
  list: [Int]
  threeDList: [[[Int]]]
}

Operation:

fragment foo on Lists {
  list!
}

This feels like it should be valid, and would make the list required, but leave the list elements optional. The final type of list would be [Int]!

fragment foo on Lists {
  threeDList[!]!
}

Should this also be valid? Should any required query syntax at or less than the depth of the field definition be valid? If it is valid, I feel that the final type of threeDList should be [[[Int]]!]!.

I think we should consider being more explicit to start, and require matching depth of actual type in schema.

I'm going back and forth on this, but I think we should allow the single ! syntax at all times (only affecting the outermost element). But if any nested depth is included, it must match the depth of the type. It's also important to note that you don't always need ! or ?. If you omit any operator at a depth level, it would inherit the default nullability. So here are a number of examples:

Schema:

type Lists {
  list: [Int]
  listOfRequired: [Int!]
  threeDList: [[[Int]]]
}

fragment foo on Lists {
  list!                    // [Int]!
  threeDList!              // [[[Int]]]!

  list[!]!                 // -> [Int!]!
  list[?]!                 // -> [Int]!

  list[]!                  // -> [Int]! (Int is optional as inherited from the default value)
  listOfRequired[]!        // -> [Int!]! (Int is required as inherited from the default value)

  threeDList[[[!]!]!]!     // -> [[[Int!]!]!]!
  threeDList[[[]!]!]!      // -> [[[Int]!]!]! (Int is optional as inherited from the default value)
  threeDList[[[!]!]!]      // -> [[[Int!]!]!]
  threeDList[[[!]]!]!      // -> [[[Int!]]!]!
}

I'm unsure about if this should be valid:

fragment foo on Lists {
  listOfRequired[?]        // -> [Int]! 
}

Can you make the required element nullable on client? What value does that provide? listOfRequired[]? makes sense for error propagation.

Non-expert opinion: I would also go with matching schema depths for lists, being explicit is one of GraphQL’s core strengths, and having edge cases only causes more cognitive strain and library complexity

I want to try to get discussion started on what to do with the ?. I'm going to copy in everyone's opinions from the working group meeting.

I'm going to copy each of them in as individual comments to avoid a wall of text, and tag the people who expressed them. It would be awesome if we could land on something before the next working group meeting. Apologies for the temporary spam!

[@twof @mjmahone]: We could defer the conversation. It’s currently unclear what a counterpart to a non-nullable designator should do, so we could leave it out until the non-nullable designator gets some community use, and we can see what issues people run into with it. Problems could be solved in a later proposal.

[@leebyron]

• Override Non-Nullable fields, and make them Nullable. As a side effect, fields marked with a question mark act as a boundary and stop null propagation from their children going any higher in the tree.
  • This is what the current implementation does.

[@leebyron]

• The RobinHood use case. (I wasn’t clear on the details here. It would be helpful if you could write more about this if you want it discussed.)
  • ‘It's not even about “hey this could be null on the server and I want to enforce that I have data on the client” it's “what happens if something blows up and a part of the the response is is gone?” I want the ability to say “that's okay like please stop blowing things up at this point” and it's okay if that requires my client side to be required to handle the null case and so it's actually it's like the opposite of the the intent for this original proposal.’

[@IvanGoncharov ]

• In the case that something blows up because it’s been marked required, we could include the partial result in the error itself. That way you can access the same fields if you want. It’s not total data loss. Maybe this is something that the question mark/error boundary can do.