Add support for @experimental_disableErrorPropagation

[martinbonnin]

This pull request adds support for @experimental_disableErrorPropagation to disable error propagation for error aware clients:

type User {
  email: String!
  name: String!
}

query GiveMeAllThePartialData @experimental_disableErrorPropagation {
  user {
    email #this field can fail without blowing up the whole user
    name # this one too 🙌
  }
}

Fixes #3756

See graphql/nullability-wg#85
See graphql/graphql-spec#1050
See graphql/graphql-js#4348

[bbakerman]

The graphql-java team @andimarek and @dondonz discussed this today.

We decided that we would accept this feature with the idea that is very much opt in.

So rather than use src/main/java/graphql/schema/idl/SchemaGenerator.java and its Options to "auto put" the directive in place we would require that people copy the directive into their schema themselves. if some one was programmatically creating a schema say then they would have to add Directives.ErrorHandlingDirective themselves into the schema say.

This is in line with how the experimental @defer is also required to be added to the SDL in text form or programmatically added to the schema say

Also we should adopt the same "opt in" mechanism as defer uses eg graphql.ExperimentalApi#ENABLE_INCREMENTAL_SUPPORT

We should create another String constant like this and if its present in the graphql context, then this code is enabled

eg something like this

var b = Optional.ofNullable(executionContext.getGraphQLContext())
                        .map(graphqlContext -> graphqlContext.getBoolean(ExperimentalApi.ENABLE_ERROR_PROPAGATION))
                        .orElse(false)

We have to think of a name of this "feature" and I dont think "error handling" is a great name because its so general versus this very specific "dont bubble non null errors up" form of error handling.

So this also calls into question the directive name of "errorHandling" - again its a general term but in practice this is actually a very specific thing (non null exception propogration or not )

[martinbonnin]

@bbakerman makes a lot of sense, this is very similar to @defer 👍 .

I pushed some changes here. Let me know what you think.

Re: naming, it's hard 😅 . I went with ENABLE_CUSTOM_ERROR_HANDLING but no strong opinion there. If anyone has an idea, please feel free to share.

[bbakerman]

Thanks for the update and removing the SchemaGenerator code.

I think the final part here is the naming.

Right now its

enum OnError {
              ALLOW_NULL
              PROPAGATE
            }
directive @errorHandling(onError: OnError) on QUERY | MUTATION | SUBSCRIPTION

My problem with the name is errorHandling and onError is very generic - eg it sounds a little bit more like perhaps "all the error handling" on this operation. Whereas in truth its a very specific bit of error handling - it prevents the propagation of exceptions when a non null type field returns null.

I think we should name this directive more specifically about this "non null type field returning null" behavior.

Naming is hard - lets go shopping!!

Here are my suggestions (with a matching directive argument and enum type)

• onNonNullFieldReturningNull
• onNonNullFieldContractFailure
• onFieldContractFailure
• onContractFailure

The last one is my favourite name because we could extend it for further contract failures in the future (but probably not)

enum NonNullFieldIsNull {
              ALLOW_NULL
              PROPAGATE
            }

directive @onContractFailure(nonNullFieldIsNull: NonNullFieldIsNull) on QUERY | MUTATION | SUBSCRIPTION

Thoughts?

@andimarek / @dondonz @martinbonnin

[benjie]

I read @nullOnNonNullError and thought you were implying that only coercion error caused by a null in a non-null position would result in null; but I realise that's not what was intended - instead you meant "non-null positions may be null when there is an error". Perhaps @ignoreNonNullOnError or @allowNullOnError or just @nullOnError might be a closer description? No matter where an error occurs (a non-nullable position or otherwise), in this mode, the position will become null if an error occurs there. An alternative tact could be: @disableErrorPropagation

FYI: we decided to put this on hold for now because we were informed that the WG works on a new RFC for this topic.

This makes sense:

• one nullability proposal has two wrapper types around a default nullable type: (strictly) Non-Null and Semantic-Non-Null; disabling null bubbling then can be thought of as treating every Non-Null wrapper as if it were a Semantic-Non-Null wrapper instead (and this may be expressed through introspection in this mode)
• the other nullability proposal may try and leverage this same work and only change things at a syntax level, but that feels wrong. Instead it will likely have two wrapper types around a default "semantically non-nullable" type: Nullable and (strictly) Non-Null; disabling null bubbling then would be equivalent to stripping all Non-Null wrappers, leaving only Nullable and the unwrapped type which is now "semantically non-nullable"

If you want to implement the introspection changes at the same time then I think you're probably right to hold off implementing it until the latter solution has the spec changes fully written out 👍

If you're happy only implementing the non-introspection features, then both proposals work in the same way (error propagation is disabled - errors (including nulls in non-null positions) never bubble) and there's no need to wait.

[martinbonnin]

we decided to put this on hold for now because we were informed that the WG works on a new RFC for this topic.

For the record, I believe we shouldn't always wait for an RFC to be complete before adding (experimental) features.

The WG discussion have been going on for a while now and getting something relatively simple out so that people can toy around with has lots of value IMO.

That being said, alignment is also good. Let's discuss!

In the meantime, I'll revert to @nullOnError. This was our first hunch and I'll trust that the simplicity of it outweights the other drawbacks.

[andimarek]

@michaelstaib can you elaborate on your plans here? thanks!

[martinbonnin]

Matching pull request there for graphql-js: graphql/graphql-js#4348

[martinbonnin]

I've synced this PR with graphql/graphql-js#4348 and @experimental_disableErrorPropagation

There was a consensus to make the directive experimental by prefixing it with experimental_ and move forward even though the ultimate solution might look different (probably request parameter or document directive?). In all cases, the experimental_ prefix should make it relatively clear that things can change.

The name disableErrorPropagation reuses the spec terminology and was deemed the most precise choice. In all cases, this can be changed down the road if we decide to.

I have also removed the GraphQLContext flag to enable the parsing of the directive. This is to remove friction with potential integrators/users. Users can test without having to wait for a release of their framework of choice by "just" bumping the transitive version of graphql-java. Also keeps things in sync with graphql-js. Let me know if this is an issue and I'll add it back.

[bbakerman]

Sorry can we get rid of these.

I dont want "outside opnions" on how we set up our IDEA in tis PR.

Make another PR if this is something we should do

[martinbonnin]

Sure thing! 535a620

[bbakerman]

We want to make another change so that @experimental_disableErrorPropagation is auto added to SDL like all the other directives from graphql-java are. We want this for consistency with the other directives

BUT this does not have to be this PR (it will break one test here - which is good - TDD)

[martinbonnin]

Thanks for the review!

We want to make another change so that @experimental_disableErrorPropagation is auto added to SDL like all the other directives from graphql-java are.

Are you talking about GraphQLSchema.builder here?

FWIW, in graphql-js, the user has to add the directive definition manually for it to be recognized. This how the "opt-in" is done. Of course, it's perfectly fine for different projects to have different conventions, just surfacing that here so we're aligned!

In all cases, let me know if this PR needs anything else.

[dondonz]

Thanks for contributing this PR and for your work championing the spec @martinbonnin

[martinbonnin]

Thanks all for the reviews! Looking forward to trying it out!