-
Notifications
You must be signed in to change notification settings - Fork 231
Add onError
RFC
#1755
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
Add onError
RFC
#1755
Conversation
|
||
Servers not supporting `onError` must execute in the `PROPAGATE` mode. | ||
|
||
Servers supporting `onError` must decide on a sensible default. Existing servers will most likely use `PROPAGATE`, new servers will most likely use `NULL`. `HALT` is also possible as a default albeit it's expected to be used less frequently. |
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.
Would it be simpler if the default is always PROPAGATE
? That automatically makes this compatible with old clients, and removes the need to surface the default in introspection (unless there's another reason to do so that I missed?)
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.
From what I understand, we want clients to be able to omit onError
in the end state. This was discussed in the May wg around here.
If you create a new schema now, it's reasonable to create it with a modern onError: NULL
default so that clients do not have to send extra useless bytes for each and every request.
This reverts commit 5d6248b.
Possible values for the `onError` property are: | ||
|
||
* `NULL`: disables error propagation. The executor emits `null` in the `data` part of the response and a GraphQL [execution error](https://spec.graphql.org/draft/#execution-error) in the `errors` part of the response. | ||
* `PROPAGATE`: propagates the error to the nearest nullable parent field. This is the current behaviour. |
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.
* `PROPAGATE`: propagates the error to the nearest nullable parent field. This is the current behaviour. | |
* `PROPAGATE`: propagates `null` and the error to the nearest nullable parent field. This is the current behaviour. |
I think having only "the error" could be misinterpreted and it's clearer to state that the null
value is propagated too.
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 spec text is currently only mentioning propagating "errors"
:
If during ExecuteSelectionSetwith a non-null type raises an execution error then that error must propagate
to the parent response position (the entire selection set in the case of a field, or the entire list in the case
of a list position), either resolving to null if allowed or being further propagated to a parent response position.
Alternative suggestion (also with position
instead of field
):
* `PROPAGATE`: propagates the error to the nearest nullable parent field. This is the current behaviour. | |
* `PROPAGATE`: propagates the error to the nearest nullable parent response position where the error then resolves to null. This is the current behaviour. |
This is a RFC that aims to extract the
onError
bits from the semantic nullability discussion, breaking down the initial problem in smaller, hopefully more manageable, problems.Read the rendered RTC