-
Notifications
You must be signed in to change notification settings - Fork 1.2k
Error behaviors (including onError: "NULL")
#1163
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
Open
benjie
wants to merge
39
commits into
main
Choose a base branch
from
error-behavior2
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
+120
−27
Open
Changes from all commits
Commits
Show all changes
39 commits
Select commit
Hold shift + click to select a range
31c90e7
Detail onError request parameter
benjie f4fab96
Detail introspection changes
benjie 692d811
Define the directive
benjie 94446ab
ABORT -> HALT
benjie 3c63355
Start speccing out the capabilities system
benjie 7056690
Add a number of basic capabilities
benjie 0fa7a33
Move default error behavior to the service
benjie 1f975e4
Rework capabilities
benjie 8c40086
Use a definition
benjie 641a786
Reorder
benjie 026982b
Reword
benjie a7c6ad5
Editorial
benjie fe559ea
More editorial
benjie b5f64ae
More editorial
benjie 1c3f0cd
Update spec/Section 4 -- Introspection.md
benjie 7ab36b8
Merge branch 'main' into error-behavior2
benjie cc50991
Change NO_PROPAGATE to NULL
benjie df977eb
Overhaul and introduce SDL syntax for service capabilities
benjie b1f039c
Update syntax
benjie b28ef2f
Simplify
benjie 144e854
Update Name syntax
benjie dc9315c
Clarification
benjie 6fd7239
Minor tweaks
benjie c44a7ae
Clarify the interaction of onError:HALT and subscriptions
benjie aef7069
Fix grammar
benjie 955acd4
Spelling
benjie 2fc8b0c
Fix incorrect prefix
benjie aef2e2d
Remove unnecessary negative lookahead
benjie 4d2a94b
Lee says empty braces are okay here!
benjie bbe2512
Merge branch 'main' into error-behavior2
benjie 55458b1
Sync syntax with Lee's suggestions
benjie edd5df0
Merge branch 'main' into error-behavior2
benjie 27317f8
Remove service capabilities; now in #1208
benjie 8d08585
Add pathNonNull so clients can reproduce propagation locally
benjie f5e5ffd
Add pathNonNull to examples
benjie 26b4aa5
Better wording
benjie 39084dd
No capabilities
benjie 22e8ea8
Remove the concept of a default error behavior
benjie 67fea0d
Less awkward wording, per Lee
benjie File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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.
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.
In the case of mutation resolution, does
"NULL"mean execution continues?eg if I have a mutation:
and all fields were not nullable, which of these responses would be correct in the presence of an error on resolution of
doThing2:A: response fields deviate from graphql request ask
{ "data": { "doThing1": true, "doThing2": null /* doThing3 never resolved */ }, "errors": [ { /* error from doThing2 */ } ] }B: synthetic field production - propagate null to down-operation resolution
{ "data": { "doThing1": true, "doThing2": null, "doThing3": null /* never resolved but server injects null because field is requested */ }, "errors": [ { /* error path doThing2 */ } ] }C: continue resolving
{ "data": { "doThing1": true, "doThing2": null, "doThing3": null /* server application's responsibility for stopping execution after error */ }, "errors": [ { /* error path doThing2 */ }, { /* error path doThing3 thrown due to response errored flag set, does not indicate a problem with doThing3 */ } ] }or perhaps more confusingly
{ "data": { "doThing1": true, "doThing2": null, "doThing3": true }, "errors": [ { /* error from doThing2 */ } ] }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.
The final “perhaps more confusingly” is the correct behavior according to the current semantics. It’s an interesting question, because if the client changes the onError without the knowledge of the user, the resulting side-effects will differ. Option C is interesting, but also wrong in its own way. In my own schemas, root level fields (even mutations) are nullable so it wouldn’t make a difference to me, I wonder if making the fields non-nullable is done specifically to block follow-up mutations currently…? Though it does mean you wouldn’t see the result of previously completed mutations so it seems like a weird choice.
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.
Hot Chocolate's mutation convention makes mutations not nullable and it is quite annoying to change: https://chillicream.com/docs/hotchocolate/v16/building-a-schema/mutations/#mutation-conventions and I think semantically it is correct (the mutation always has a result or there is an error).
I agree the current wording suggests option C as well (both of the last 2 samples are option C). The difference between them is if the server implementer (or framework they use) flag that an error occurred via some mechanism such that if inside
ExecuteMutation()the flag is set, every followingExecuteSelectionSet()immediately raises an error.I suppose my point is that either application authors (both clients and servers) will need to beware of of the possibility that this could happen (and frameworks could take an opinionated stance or provide a default) or the spec force A or B and clients (and potentially client libraries) would need to be able to handle such responses. I don't think there is a satisfying answer here.
Uh oh!
There was an error while loading. Please reload this page.
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.
@bbarry can you ellaborate why C. is not satisfying?
onError: NULLmakes it possible to resolve more data, whether for queries or subscriptions. Forcing A. or B. would be very surprising to me. With C. the server can decide how it wants to handle things. If anything, we could recommend to have only a single root field in mutations. I think this was discussed at some point but can't find it anymore.Edit: found it! It's "batched mutations" article: https://medium.com/@xuorig/graphql-mutation-design-batch-updates-ca2452f92833
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 it is a very hard problem and every solution I've seen so far is unsatisfying in some way. In each case there is room for interpretation and miscommunication between client and server. If clients could just send valid data and servers could avoid flaky systems the world would be a much simpler place.
What I find unsatisfying about C is that a client a client sending such a mutation operation could intend that the 3 mutation fields are causality related. If the second doesn't happen and the 3rd does, there could be a logic bug in the client. If the server author chooses to avoid this potential by raising down-field errors the server is adding unnecessary response content and falsely stating there is a problem with a field when in fact the problem is outside the field.