Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Query level nullability review and drafting thread #1
Query level nullability review and drafting thread #1
Changes from 30 commits
53c3d6e
ff04f2d
b823d7c
20aa350
7f12742
acf4d00
486b23f
37d78b3
438bcac
b625197
4829dfd
70762e3
1bc2272
f859034
ad40331
c9a925d
7871906
c6723ce
2e31037
c7764d5
1c20e26
e9cb076
7255a8d
254fd85
3bd4f32
96d0bee
e3b91d1
e0a572b
4aab23b
3c45346
d891d96
54a06eb
e6fc0ed
550ffeb
8515128
c60e590
e1e363a
5d5a519
6e54093
6d5d16c
458ee4e
e32b1f2
b265238
da52795
27069c9
844248c
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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 would consider reducing the focus on specific client languages.
The crux of the problem is that the SDL nonNull (!) eliminates the possibility of partial failure on a given type. This forces the SDL 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 a given canvas. 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.
The pains are more noticeable in languages with first-class null safety, but are relevant to any client.
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.
Can you expand here? My reading of this is that if the value of a field with
!
is null at runtime, then the GraphQL executor must treat that identically to the case that the field resolver raised an exception, including returning an entry in the top-levelerrors
list and nulling out the parent field (which may need to then bubble up an error if that field was non-nullable in the schema, or also had!
applied).This would mean that in a query such as the following,
other_media_field
being null at runtime would cause the entire query to return effectively no data (just{data: {media: null}, errors: [...]}
). This would prevent any part of the UI from rendering because a single field was missing for one part:How are you thinking about handling cases such as this?
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 read this as any field in the response that would be
null
and has a!
on the field in the operation would cause the whole operation to return an error and no data. Even if that field is in some named fragment.I think this would limit the usefulness extremely, basically to very simple queries. To take Yelp as an example, I don't think it'd be a good idea to error a search query if a single search result item failed to generate a star rating (assuming that's required to render a search result item).
It would be good to clarify what the behavior is if the
!
is in a named fragment.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.
At Netflix, we have implemented this on the server (using a directive syntax) and defined it to have identical semantics as the existing SDL Non-Null behavior. This has worked well.
I've proposed a clarification to specify this.
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.
@fotoetienne Thanks for adding the clarification, that confirms my interpretation and brings us back to my original question:
How are you thinking about the fact that a single fragment with a
!
field can cause data for other fragments (or in the extreme, an entire request) to be nulled out? As @captbaritone mentioned above, one of our goals / requirements when designing@required
was that a "missing" required value for one part of a UI cannot cause data for other parts of the UI to be nulled out. The design here violates that constraint (it breaks fragment modularity).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.
@josephsavona That's a great question. Our current server logic will make a field act as Non-Null is there any Non-Null designation for that field in the query. This query example:
would essentially be interpreted by the server as:
It is at least non-ambiguous, but I can see how it is a challenge for fragment modularity.
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.
@fotoetienne Thanks for clarifying. That seems like a severe limitation that would make applications significantly less robust than they can and arguably should be. How do you manage this in practice? Do you encourage fields with
!
to be given unique aliases so that you prevent removing data for other parts of a UI?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.
Short answer - it hasn't come up for us yet, but it's an important edge-case to consider.
Long answer - Fragment modularity is particularly crucial for clients that rely heavily on components tied to individually defined Fragments that are compiled into large queries. We don't currently have many teams using that approach.
I agree that this is an important edge-case that we should think through some more. We want a design that will lead UI developers to fall into "the pit of success" rather than allowing surprising behavior like you've described.
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.
An initial thought, probably flawed: reversing the server-defined logic might help. Only honor nonNull if all references to that field specify it. This would, however, require client codegen to be aware of an entire query and only make types nonNull if all fragments in the query specify nonNull. Thoughts?
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.
Making conflicting definitions between fragments be a validation error (like you did with @requires) is the safest approach. I like that. It's probably an acceptable limitation that you just can't have differing nullability for the same 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.
@joesavona even unique aliases would still null out the object by bubbling up to the parent. It's really the same as any specific field being non-nullable in the server schema and ending up null which bubbles up.
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.
Here's one of the cases where this would be needed.
GatsbyJS projects use GraphQL to handle build-time data dependencies, and provide a file system route API that can automatically build pages from specific types.
Creating a file called
src/pages/{BlogPost.slug}.jsx
creates pages from all BlogPost nodes and injects page context.{ id: string, slug: string }
user can use the page context as an argument to fetch additional data.
This can be done with additional static analysis in plugin layer. But the proposal can be a simple solution.
Even if It's not clear in production cases that can customize schema, but it seems a huge advantage for ecosystem players using SDL and code-generation.
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.
Thank you for sharing another use case!
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.
This will be true even with custom syntax. For composability of fragments, it would be worrisome if this new syntax made it impossible to have these two fragments in the same application:
With consistency, I could end up re-rendering a component built with
UserMustHaveName
based on a response fetched using onlyUserOkWithoutName
.And in fact it gets worse:
If my best friend, when I fetched
BestFriendWithName
, isUser:1
, and then suddenly, after fetchingBestFriendWithoutName
isUser:2
, I no longer am capable of rendering the component relying onBestFriendWithName
(becausename
is unset onUser:2
). If the client library doesn't know what to do in that case, you can end up with consistency updates causing crashes.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.
As you said, the client library will need to understand this new syntax/directive. I am not familiar with GraphQL client implementations, so I am not sure how difficult my imagined behavior will be to implement. Doesn't this behavior solve the issue you outlined?
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.
That's how Apollo solves it, but that strategy isn't true universally. For instance that's not how Relay solves it. The client I work on can't solve it this way, as our consistency subscriptions are at the response level, for queries that include hundreds of fragments (whether that's a good choice or not is irrelevant: it's how things work for the largest GraphQL client at Facebook): if missing
name
meant the entire consistency update should not be served, whenever a product chose to mark their field with!
, they could cause entire unrelated surfaces to lose consistency.Another valid strategy would also be to return the data, but with
best_friend
being null: all the other data in the fragment/consistency subscription would still be delivered (this is Relay's strategy for@required(action: NONE)
). Or requiring the product to null check every piece of data, always, regardless of its nullability annotation.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.
Do you mean that Relay requires field selections on the same id and type to behave the same globally? If so, I can see how my suggested behavior won't work. If not:
This is what I imagined. The client would nullify up until the most immediate nullable parent.
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: Flesh this out with things about versioning, breaking changes, and the impact it has on partial results
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.
@wxue Do you remember why we didn't want this?
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 we should remove this as an alternative, I don't think we understand it well enough to vouch for 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.
Defaults can be quite useful from a resilience standpoint. It is relevant to this proposal in that the
!
syntax would't be as amenable to specifying a default, whereas a directive (e.g.@nonNull(default: false)
) would open us up to that.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.
Though replacing
!
with! = “default”
like in variable defaults could work. This would either have to be evaluated client side (like in Relay) or the same default would have to apply to all references at the same location (whether from fragments or directly).