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

[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!