Allow clients to disable error propagation via request parameter (take 2)

[benjie]

This is a rewrite of

• Allow clients to disable error propagation via request parameter #1153

It is re-implemented on top of the recent editorial work (e.g. renaming _field error_ to _execution error_) and also makes a significant change in that it does not require onError to be included in the response, instead an introspection field is used to indicate:

1. that onError is supported
2. what the behavior will be if onError is not present

---

Replaces:

• [RFC] Directive proposal for opting out of null bubbling #1050
• Introduce @disableErrorPropagation #1145
• Allow clients to disable error propagation via request parameter #1153

GraphQL.js implementation:

• Implement onError proposal graphql-js#4364

---

Please see this 60 second video on the motivation for this PR (the last few seconds of the video also covers "transitional non-null" which is a separate concern).

As agreed at the nullability working group, disabling error propagation is the future of error handling in GraphQL. Error propagation causes a number of issues, but chief among them are:

1. It destroys useful data in the response.
2. It makes it unsafe to store resulting data in normalized stores.

Clients such as Relay do not want error propagation to be a thing.

This has traditionally resulted in schema design best practices advising using nullable in positions where errors were expected, even if null was never a semantically valid value for that position. And since errors can happen everywhere, this has lead to an explosion of nullability and significant pain on the client side with developers having to do seemingly unnecessary null checks in loads of positions, or worse - unsafely bypassing the type safety.

The reason that GraphQL does error propagation is to keep it's "not null" promise in the event that an error occurs (and is replaced with null due to the way GraphQL responses are structured and limitations in JSON) in a non-nullable position.

It doesn't take much code on the client to prevent the client reading a null that relates to an error, graphql-toe can be used with almost any JavaScript or TypeScript based GraphQL client (not Relay, but it has @throwOnFieldError that you can use instead) and achieves this in 512 bytes gzipped - and that's with a focus on performance rather than bundle size.

This PR allows the client to take responsibility for error handling by specifying onError: "NO_PROPAGATE" as part of the GraphQL request, and thereby turns off error propagation behavior. This is also set as the recommended default for future schemas.

With clients responsible for error handling, we no longer need to factor the possibility of whether something can error or not into its nullability, meaning we can use the not-null ! to indicate all the positions in the schema for which null is not a semantically valid value - i.e. the underlying resource will never be a legitimate null.

The end result:

• true nullability indicated in schema - no more thinking about where errors are likely
• fewer null checks on clients
• clients can leverage their native error handling capabilities such as try/catch or <ErrorBoundary />
• safe to store errored responses into normalized stores

---

I've also included onError: "ABORT" in this proposal. We've discovered that there's a small but significant class of clients out there, mostly ad-hoc scripts, that throw away the entire response when any error occurs. By codifying this into the spec we make it easier to implement these clients, and we allow the server to abort processing the rest of the request unnecessarily.

As noted by @revangen in this comment:

I've also included onError: "ABORT" in this proposal.

Appreciate this being included. For Shopify's public Admin GraphQL API, we have a mix of scenarios that result in a partial success response and only error response. Having been around for 8+ years, we are reluctant at times to change its behaviour to favour partial responses as we don't control majority of clients. Providing clients a way to specify the server's behaviour provides a migration path should clients care about partial responses.

[netlify]

✅ Deploy Preview for graphql-spec-draft ready!

Name	Link
🔨 Latest commit	692d811
🔍 Latest deploy log	https://app.netlify.com/sites/graphql-spec-draft/deploys/68120b69d49afb0008926f5c
😎 Deploy Preview	https://deploy-preview-1163--graphql-spec-draft.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

---

To edit notification comments on pull requests, go to your Netlify site configuration.

[benjie]

I have applied the RFC1 label to this as inherited from the PR it replaces:

• Allow clients to disable error propagation via request parameter #1153

[benjie]

A full, CI-passing, implementation of this is available in GraphQL.js now; check it out: graphql@canary-pr-4364

[martinbonnin]

Moving my previous comment here, love this proposal overall but I would love it even more with NULL

enum __ErrorBehavior {
  NULL
  PROPAGATE
  ABORT
}

tldr: I believe this is going to be read and discussed a lot more than it's going to be written by users and I would optimize for the conciseness, pronouncability and memorability of NULL.

[benjie]

To summarize my thoughts on it from the previous thread: NULL is what I originally wrote, I agree it looks and initially feels nicer; but ultimately I discounted it for a number of reasons:

Communication issues

If you tell someone on the phone to set onError: NULL, they're very likely to instead set onError: null.

Ambiguity issues

onError: null and onError: NULL are very similar, but the first means PROPAGATE and the latter means NO_PROPAGATE. This is likely to be the source of many user errors and much frustration (both for newbies, and for the people supporting them including library authors).

Errors already become null in the spec:

[an error] raised [...] is handled as though the response position [...] resolved to null
-- https://spec.graphql.org/draft/#sel-FANVNJCAACGW-qR

onError: NULL doesn't implicitly seem to mean something different from this.

Please suggest other alternatives!

I'm very open to changing the word NO_PROPAGATE to something else, I really don't like NO_PROPAGATE, not least because I always want to spell it propOgate rather than propAgate. But I think changing it to NULL would be a mistake.

I've gone through about 30 alternatives when brainstorming it, and none are sufficiently clear whilst also being shorter than NO_PROPAGATE. My favourite alternatives are things like LOCAL/LOCALIZE/ISOLATE/CONTAIN, but I'm not sure they convey the meaning quite as well as NO_PROPAGATE does.

[benjie]

Ooo... Maybe LOCAL_NULL actually is quite compelling. Not thought of that one before.

[fotoetienne]

What about onError (or behavior) as a query directive rather than a new field on the request? Adding a field to the request payload might require changes to lots of GraphQL middleware. also a directive mirrors the behavior schema directive.

[martinbonnin]

This is where we initially started with @disableErrorPropagation. It's very handy that no middleware is impacted at all but @benjie argumented that it should be a request parameter.

The argument that won me is that the directive would most likely be automatically added by the Apollo/Relay/... compilers. Then it means you're modifying the user query and if the user types the same query in GraphiQL it won't behave the same as what they initially typed, which is surprising.

[fotoetienne]

That's a good point that you might want to be able to add it without modifying the query document

[benjie]

Also keep in mind that it being a GraphQL request parameter does not necessarily mean it’s an HTTP request parameter; for example you could use a different endpoint in HTTP to set the param: /graphql_nobubblesplz ;)

[fotoetienne]

Thoughts on directive @onError(behavior: __ErrorBehavior!) on SCHEMA to mirror the naming of the client onError field?

[benjie]

Here’s why I think it should be a request parameter and not a directive. (Further, I don’t think we should allow both because that opens up questions around what happens if the client sets one and the user sets the other: who wins?)

[fotoetienne]

What about defining the server default onError behavior only in introspection and not in the SDL? We can instead define abstract "execution configuration" parameters that informs execution behavior. Implementations can then choose to have this be configured via a schema directive or as a separate configuration file.