Support for resolving exceptions from a subscription

[Munoon]

Closes gh-397

[pivotal-cla]

@Munoon Please sign the Contributor License Agreement!

Click here to manually synchronize the status of this Pull Request.

See the FAQ for frequently asked questions.

[pivotal-cla]

@Munoon Thank you for signing the Contributor License Agreement!

[rstoyanchev]

Thanks for the proposed changes.

My first thought was, there is probably no need for a dedicated contract. We could just expose an additional method on WebSocketGraphQlInterceptor.

However, a dedicated contract could work better, if it was supported for all transports. As it is right now, it is coupled to each transport handler which is what subscribes to the Publisher.

One idea would be to intercept and transform the ExecutionResult in DefaultExecutionGraphQlService, so that if the data is a Publisher, it is composed with Flux.onErrorResume to invoke the SubscriptionExceptionResolver and return something like a SubscriptionHandlingException with the List<GraphQLError>. Each transport handler can then handle this exception and turn it into the appropriate "error" message for the given transport.

Would you like to try and rework this? If not I can also take it from here.

[Munoon]

I will gladly make changes. 🙂

However, I'm not sure if I understand you fully correct. As I understand, I should remove calls to SubscriptionExceptionResolver in GraphQlWebSocketHandler's and move these calls to DefaultExecutionGraphQlService, right?

[rstoyanchev]

Yes your understanding is correct, but on further thought, DefaultExecutionGraphQlService is maybe not the right place. I think it would make sense to configure SubscriptionExceptionResolver via GraphQlSource.Builder just like DataFetcherExceptionResolver, but that means there is no easy way to pass it into DefaultExecutionGraphQlService, not without changing the GraphQlSource contract and it wouldn't make sense to change it for this.

That means a SubscriptionExceptionResolver chain has to be plugged in one way or another while building the GraphQLSchema and graphql.GraphQL. I think we can use ContextDataFetcherDecorator. It already decorates data fetchers, and is aware of subscriptions. All we need to so is pass SubscriptionExceptionResolver components to it and have them invoked via onErrorResume.

[Munoon]

Fine, @rstoyanchev, everything is done! Please, review.

[rstoyanchev]

Thanks very much! Overall headed in the right direction. Some comments below for another iteration.

[rstoyanchev]

I'm not sure how this even works, since it would emit a DataFetcherResult while the transport expects the Publisher to emit ExecutionResult instances.

I think this should return a Mono.error with a new exception (e.g. SubscriptionStreamException) that wraps and exposes the List<GraphQLError> through a getter. This would force the transport to terminate in the very least, but should actually handle this exception to extract the errors and create an error message.

[Munoon]

I just added a handler of SubscriptionStreamException in GraphQlWebSocketHandler (both webflux and webmvc). Should I add it somewhere else?

[rstoyanchev]

Thanks, this looks good!