Closed brainlag closed 4 months ago
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.
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.
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.
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.
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?
Yeah I guess that would work too.
close?
I have the case where the
afterExecute
of my custom EventingService may throw an error and it is impossible to add this error to theExecutionResult
. Only option is to throw an Exception which is not catched and handled anywhere and leads to an 500 error.