How I'm supposed to add errors to the response in an EventingServices

[brainlag]

I have the case where the afterExecute of my custom EventingService may throw an error and it is impossible to add this error
to the ExecutionResult. Only option is to throw an Exception which is not catched and handled anywhere and leads to an 500 error.

[t1]

Two options come to my mind (and we could do both):

1. We could add the ExecutionResult to the Context, so it would be possible to add errors.
2. We could catch the exception and add it to the response.

I think option 1) could have a bigger impact in the long run, and 2) seems the minimum we should do.
@brainlag: you could provide a PR to move this forward.

[brainlag]

ExecutionResult is already in the context and I can get it with context.unwrap(ExecutionResult.class) but there is
no way to add a new error because the error list is immutable.

[jmartisk]

At that point, the ExecutionResult is already built and immutable and besides, it wouldn't seem proper to override results for particular fields by replacing them with an error. An error thrown in an afterExecute probably isn't specific to one of the fields in the response, so I would suggest adding it via an extension - context.addExtension() - would that work for you? The regular errors key is meant for errors specific to some GraphQL fields.

[brainlag]

Shouldn't the ExecutionResult only be built after the afterExecute hooks are executed? Of course I can push it over the extension mechanism but now have some errors in errors and some more errors in extension.errors. Then on the client I concat both arrays and go from there. Not sure what the motivation for this artificial separation is.

[jmartisk]

Ok I'm checking it and... it seems that it is possible to overwrite the execution result by casting the Context to SmallRyeContext and calling setExecutionResult, but that won't help, because we use the original ExecutionResult from graphql-java to build the response, and don't expect that the afterExecute method will overwrite it (see https://github.com/smallrye/smallrye-graphql/blob/2.8.4/server/implementation/src/main/java/io/smallrye/graphql/execution/ExecutionService.java#L269)

We could change that line to re-retrieve the ExecutionResult from the context, thus allowing the afterExecute event to change the reference to it.

Would that be ok for you?

[brainlag]

Yeah I guess that would work too.

[mskacelik]

close?