graphql / graphql-spec

GraphQL is a query language and execution engine tied to any backend service.
https://spec.graphql.org
14.31k stars 1.13k forks source link

[WIP] Non-nullable error boundary - interrobang #1046

Closed benjie closed 1 year ago

benjie commented 1 year ago

Inspired by @captbaritone's True Nullability Schema discussion (https://github.com/graphql/graphql-wg/discussions/1394) and following @fotoetienne's excellent talk at GraphQLConf, and trying to "use the things we already have in new ways" I am proposing that we introduce a new non-nullable variant, the "Non-Null error boundary" type, represented via interrobang mark !?. This will be a Non-Null type in the current sense, but with an extra property of __Type.isErrorBoundary: true

Critically, this type would "evaporate" for legacy clients, appearing the same as a nullable field. (This is enabled via the includeErrorBoundaries argument to the __Field.type field, which defaults to false.)

The main difference in this type is that it will never be null unless there is an error in the errors array. So it's like a hybrid between the nullability default, and Non-Null. A middle ground.

I have not made all the spec edits for this, I'm just feeling out the problem space right now. "Collecting" the field error (rather than throwing it) is going to need some careful wording... but that's a problem for when we come to formalize it.

netlify[bot] commented 1 year ago

Deploy Preview for graphql-spec-draft failed.

Name Link
Latest commit 3311df377efc6eef5438d2670a4baae8c6932ecc
Latest deploy log https://app.netlify.com/sites/graphql-spec-draft/deploys/650cd1b4e6e0f500080d720a
captbaritone commented 1 year ago

First of all, thanks for kicking off the discussion. I think this proposal gets us what we will need for True Nullabilty, and severs as a great starting place for iteration.

Things I like about this proposal:

Things I wish could be better:

Mainly the syntax has some drawbacks ( perhaps we should defer syntax bike shedding for another time, but I’ll list a few concerns for now)

An observation: we’ll also need to consider how this new type variant would affect interface validation and potentially selection validation.

benjie commented 1 year ago

I agree on all your points @captbaritone. From a separate in-person conversation, I think @mjmahone thinks that this new "non-nullable error boundary" type should be the default, but I'm not convinced I agree - I think nullable+null boundary (i.e. the current default) should remain the default.

Also, I'm considering changing it into a different "wrapper type" instead. So these spec edits are far from complete.

martinbonnin commented 1 year ago

Agree that the syntax is a bit weird. Is !? used in any other language? Also the list case is always a bit awkward:

type User {
  friends: [User!?]!?
}

But overall, what bugs my brain the most here is that it introduces some kind of dissymmetry between nullable and non-nullable types. Why would error boundaries be limited to non-nullable types?

I get the technical reason why (null bubbling is only an issue for non-nullable fields). But from a user point of view, I'd expect errors and nullability to be orthogonal concepts. If we're saying !? is an indication that a non-nullable field might error, I would almost expect a symmetrical ??that hints that the given nullable field might error. But maybe it's too much?

benjie commented 1 year ago

Yeah, so what I’m considering is that you’d have e.g. errorBoundary(nonNullable(Int)) which would come out as Int!?. The problem right now is that error boundaries are implicit, so this would have a pretty tough migration story for schemas.

BoD commented 1 year ago

To clarify, with this schema:

type Query {
    book: Book!?
}

type Book {
    author: User!?
}

querying book when the author resolver fails, would return:

a/

{ "errors": {...}, "data": { "book": { "author": null } } }

or b/

{ "errors": {...}, "data": { "book": null } }

?

benjie commented 1 year ago

(A) (there is no bubbling past an interrobang )

benjie commented 1 year ago

Closing this in favour of the much more complete asterisk proposal: https://github.com/graphql/graphql-spec/pull/1048