-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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 #895
Conversation
|
✅ Deploy Preview for graphql-spec-draft ready!
To edit notification comments on pull requests, go to your Netlify site settings. |
647f703
to
849c7ac
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think these changes are more in line with the GraphQL grammar
What's the effect on the execution section? |
|
Ok, I took a look at Execution: I believe the key parts are
|
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) |
7df9360
to
991bd1c
Compare
@leebyron Thanks for the info! I've added a step to 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 |
spec/Section 5 -- Validation.md
Outdated
@@ -429,6 +429,10 @@ SameResponseShape(fieldA, fieldB): | |||
|
|||
* Let {typeA} be the return type of {fieldA}. | |||
* Let {typeB} be the return type of {fieldB}. | |||
* Let {fieldARequiredStatus} be the required status of {fieldA} | |||
* Let {fieldBRequiredStatus} be the required status of {fieldB} | |||
* Let {typeA} be the result of {ModifiedOutputType(typeA, fieldARequiredStatus)} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Am I allowed to reference functions defined in other documents? ModifiedOutputType
is defined in Execution.md
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes! that's okay.
However you should not redefine a variable. In spec-pseudo-programming "Let {x} be" means const x =
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah I was pulling this pattern from line 438 in the same file. I'll create a new variable.
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: Even if we don't implement list element nullability controls now, we should consider them more thoroughly. before moving forward. Proposed SolutionsFor 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.
Sequence of ModifiersIn 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 SuggestionsHere 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 TypeThis 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 Container Representation - Agnostic to Internal Type SyntaxSince 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 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 NameI 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 SummaryOf 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
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 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 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. |
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:
|
6e54eb7
to
06819e3
Compare
The graphql-js branch with execution has been integrated into Apollo-iOS, and released in an Alpha! https://github.com/apollographql/apollo-ios/releases/tag/1.0.0-alpha.4 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TODO list to bring things up to date with the decisions that have been made
spec/Section 2 -- Language.md
Outdated
An error is added to the `errors` array identical to if the field had been | ||
`Non-Nullable` in the schema. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Update this once we land on error behavior.
spec/Section 6 -- Execution.md
Outdated
to {null}, otherwise if its {ModifiedOutputType} is a `Non-Null` type, the field | ||
error is further propagated to its parent field. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If folks are cool with it, I think we should be more precise here and say that nulls are propagated rather than errors. I think the language about error propagation likely stems from the GraphQL-JS implementation which centers its logic for propagation decisions around errors, but the errors themselves technically have no impact on any fields beyond causing the originating field to become null, which may in turn violate the spec and cause propagation. The only way parent fields can "handle" an error propagated from a child field is by being Nullable
.
If the intent is to provide fields other means of error handling in a future version of the spec, then I could see a reason to keep the current language.
Spec now includes
|
For posterity and to assist future newcomers entering discussions surrounding this proposal, I've added a decision log to the original RFC. Let me know if updating RFCs once they've already been merged isn't standard practice and if there's somewhere else I can put the decision log. |
@twof PRs to RFCs pretty much get merged straight away, it allows us to keep the RFC document up to date 👍 |
Syntax naming discussion and poll: graphql/graphql-wg#994 (comment) |
I mentioned during the WG meeting that we can't merge fields with differing CCN operators. Here's more about that: |
- ListNullability NullabilityDesignator? | ||
- NullabilityDesignator | ||
|
||
ListNullability : `[` Nullability? `]` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a good reason for the ?
here?
This means that something like field[[]]
is allowed, but doesn't add any value, right? Can it be argued that this may be confusing so it would make more sense to disallow it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep. field[[]]
would technically be allowed but do nothing. The ?
is there because the nullability operator is optional. If it was not optional then [[!]]?
would not be valid because the second dimension of the list doesn't have a nullability operator.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To clarify, I am talking about the ?
in ListNullability : [ Nullability? ]
If it were ListNullability : [ Nullability ]
instead, then [[!]]?
would be valid, right? But [[]]?
wouldn't, which I think is the preferable behavior.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doesn't it make sense to allow [[]?]
though? It could be coded as [?]
but I think it's better to be more explicit (here there's a list of lists, and the inner list is nullable) as that helps people reading the query. I even imagine I'd add lint rules to enforce this for clarity.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doesn't it make sense to allow [[]?] though?
Thanks @benjie, you are right!
So ListNullability : `[` Nullability? `]`
does make sense.
- If {typeDepth} equals {designatorDepth} or {designatorDepth} equals 0 return | ||
true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about this:
- If {typeDepth} equals {designatorDepth} or {designatorDepth} equals 0 return | |
true | |
- If {typeDepth} is greater or equal to {designatorDepth} return true |
?
This way, 0
is just a particular case of a more generic rule that allows "skipping over the list items you want unchanged"
With this schema:
type Query {
field: [[String]]
}
This would all be valid:
{
# make the list required
a: field!
# make the items of the list required
a: field[!]
# make the items of the items of the list required
a: field[[!]]
}
I don't think it's going to be used that much but it's a more generic rule and avoids "0" as an outlier: everything that's not mentioned explicitely is untouched
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Someone will need to go back through the notes, but IIRC the concern with this was that it's not clear how to read a list operator with fewer dimensions than the list type it's being applied to. ie
Does [!] == [[!]]
or does [!] == [[]!]
? @calvincestari will be leading a CCN discussion at the next working group meeting on August 3rd, and we'll also be having a CCN working group meeting on the 26th if you'd like to join that to discuss more: https://github.com/graphql/client-controlled-nullability-wg/blob/main/agendas/2023/2023-07-26.md
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does
[!] == [[!]]
or does[!] == [[]!]
?
I think it would work like for a single dimension list.
For this field definition list1: [String]
, list1!
is the equivalent of list1[]!
, not list1[!]
so it's just a generalisation of the "single list" case. Just like ! == []!
, I would expect [!] == [[]!]
(modulo the caveat from above about [] but that's something else).
@calvincestari will be leading a CCN discussion at the next working group meeting on August 3rd, and we'll also be having a CCN working group meeting on the 26th if you'd like to join that to discuss more
I won't be able to join unfortunately but I'm working with Calvin so I'll discuss this with him :)!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I also think [!] == [[]!]
makes sense.
But If {typeDepth} equals {designatorDepth} or {designatorDepth} equals 0 return
allows future relaxing the rule to be If {typeDepth} is greater or equal to {designatorDepth} return true
without breaking queries. So would it be possible to advance as is and wait for the feedback from the community?
Hello! It's been a while. The Client Controlled Nullability RFC is now winding down in favor of True Schema Nullability, so I'll be closing CCN related issues, PRs, and discussions. All further discussion should be rerouted to one of the following: Additionally, we're in the process of removing the CCN experimental flag from graphql-js. TL;DR of the new proposal(s) is that instead of allowing clients to adapt to imprecise schema definitions, our new approach is to find ways to allow schema authors to provide the precision that's been missing. There are a few different approaches to that which you can read about in the discussions above. Apollo and a couple other schema authors are in the process of putting together experiments, so if you'd like to have input or beta test the new proposal(s), reach out to us either in the Nullability Working Group, or in the #nullability-wg channel on Discord. |
Rendered proposal: https://github.com/graphql/graphql-wg/blob/main/rfcs/ClientControlledNullability.md
Spec Implementation: graphql/graphql-js#3281
Working Group Action Item: graphql/graphql-wg#694
Known issues: https://docs.google.com/document/d/10MmZbzfPtk8GoAeSVm_m9ng8Aj10LBXVIQwkpjti6CQ/edit?usp=drivesdk