Skip to content

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

Closed
wants to merge 3 commits into from
Closed

Add onError RFC #1755

wants to merge 3 commits into from

Conversation

martinbonnin
Copy link
Contributor

@martinbonnin martinbonnin commented May 28, 2025

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


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.
Copy link
Contributor

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?)

Copy link
Contributor Author

@martinbonnin martinbonnin May 28, 2025

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.

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.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
* `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.

Copy link
Contributor Author

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):

Suggested change
* `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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants