Annotated exception handler methods

[Sam-Kruglov]

Hi, would be a nice addition to support @ExceptionHandler methods and @ControllerAdvice. Those could be injected into the exception resolver chain. This feature is available here (at least to some extend) https://github.com/graphql-java-kickstart/graphql-spring-boot

[bclozel]

Thanks for raising this point @Sam-Kruglov

We can mix and match MVC, WebFlux and GraphQL handler methods in the same @Controller class - but our engine can sort them out because they're using different annotations: @RequestMapping, @SchemaMapping and their variants.

It looks like graphql-java-kickstart accepts the following signatures for @ExceptionHandler annotated methods:

• List<GraphQLError> handle(MyException exc)
• ThrowableGraphQLError handle(MyException exc)

I'm surprised the choice was done here to support only synchronous calls.

Right now @ExceptionHandler methods accept a wide variety of arguments and return types.

With that in mind, I think that supporting @ExceptionHandler can cause issues for several reasons:

1. the @ExceptionHandler method signatures is really flexible and we're likely to run into conflicts
2. developers might think that somehow a single @ExceptionHandler method could be used for both MVC and GraphQL, which doesn't seem likely or even possible
3. @ExceptionHandler in itself is confusing, since we're never handling an exception but rather translating an "internal" exception into user-facing GraphQL errors
4. The @ExceptionHandler contract can be localized to a controller or @ControllerAdvice and called only when a close matching handler was in charge of processing the request. With Spring GraphQL, the DataFetcherExceptionResolver is global as exceptions can happen at any point during the data fetching process

Maybe we could instead with an @ExceptionResolver contract supporting the following.

Method Argument	Description
Exception Type	Exception raised during data fetching
DataFetchingEnvironment	Environment for the invoked DataFetcher
GraphqlErrorBuilder	New GraphqlErrorBuilder instance built with the current environment

Return Value	Description
GraphQLError, List<GraphQLError>	Error(s) to add to the response
Mono<GraphQLError>, Flux<GraphQLError>	Error(s) to add to the response
null	Indicates that the Exception remains unresolved

As we can see, dealing with reactive types for return values can lead to subtle behaviors: the exception remains unresolved vs. no error should be added to the response. The org.springframework.graphql.execution.DataFetcherExceptionResolver#resolveException contract is quite clear about that, but developer expectations might not be the same for annotated methods.

[Sam-Kruglov]

Yeah, you're right, I like @ExceptionResolver suggestion!

Also, GraphQL can return errors and data at the same time, so when an error occurs, it occurs inside a field resolver (is it called data fetcher here?). So, when a resolver fails, the exception translates via this annotation method, and gets added to the "errors" field of the graphql response. Other resolvers should be able to proceed their work and populate the "data" field of the graphql response. Not sure if this is the current behaviour already but from the documentation I got the impression that a single error would fail everything.

[bclozel]

Also, GraphQL can return errors and data at the same time, so when an error occurs, it occurs inside a field resolver (is it called data fetcher here?).

Yes, that's right. In Spring GraphQL, we're using GraphQL Java concepts such as DataFetcher.

So, when a resolver fails, the exception translates via this annotation method, and gets added to the "errors" field of the graphql response. Other resolvers should be able to proceed their work and populate the "data" field of the graphql response.

Yes, this is the current behavior - and IMO expected behavior for GraphQL applications.

[rstoyanchev]

The underlying mechanism is still that of GraphQL Java, so when a controller raises an exception, it has to propagate at first. Then GraphQL Java calls the registered DataFetcherExceptionHandler and we can check which controller raised the exception, look for @ExceptionResolver methods, go through @ControllerAdvice, etc.

What we have currently, as explained in the docs, is that you can register one or more DataFetcherExceptionResolver beans which apply globally like a ControllerAdvice. For example:

@Bean
public DataFetcherExceptionResolver exceptionResolver() {
	return DataFetcherExceptionResolverAdapter.from((ex, env) -> {
		if (ex instanceof SomeException) {
			return GraphqlErrorBuilder.newError(env).message("...").errorType(...).build();
		}
		else if (ex instanceof AnotherException) {
			return GraphqlErrorBuilder.newError(env).message("...").errorType(...).build();
		}
		else {
			return null;
		}
	});
}

I'm wondering whether this is sufficient for GraphQL? For once the inputs are simpler and for output there is no need to write to a response, which reduces the amount of benefit from such methods. At the same time there is extra overhead in GraphQL where there may be multiple exceptions per request for different parts of the data graph and the exception resolution steps are repeated multiple times. The other aspect could be the need for localized handling. It would be useful to hear more feedback on all this.

[Sam-Kruglov]

@rstoyanchev I think it's just nicer to have separate @ExceptionResolver methods instead of a single method with a bunch of if (ex instanceof .... Other than that, DataFetcherExceptionResolver seems sufficient. But why can we supply multiple instances of it if it can handle everything?

Some exceptions will be in response for the client to see, some will be internal errors that the client should not see but we still want to recover from or at least log.

[hanrw]

It's good way than using if (ex instanceof ....

@ExceptionHandler(
      EntityNotFoundException::class,
      NoSuchElementException::class,
      EmptyResultDataAccessException::class,
      IndexOutOfBoundsException::class,
      KotlinNullPointerException::class
  )
  fun notFoundException(e: Exception): GraphQLError {
      return GraphqlErrorBuilder.newError().message(e.message).build()
  }