-
Notifications
You must be signed in to change notification settings - Fork 1.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Adding support for making the exception handlers async #2371
Conversation
* | ||
* @return a result that can contain custom formatted {@link graphql.GraphQLError}s | ||
*/ | ||
default CompletionStage<DataFetcherExceptionHandlerResult> handleException(DataFetcherExceptionHandlerParameters handlerParameters) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we use CompletableFuture
everywhere, no?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
changed
default CompletionStage<DataFetcherExceptionHandlerResult> handleException(DataFetcherExceptionHandlerParameters handlerParameters) { | ||
DataFetcherExceptionHandlerResult result = onException(handlerParameters); | ||
return CompletableFuture.completedFuture(result); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The consuming code only calls this method. It delegates back to the old sync method. So any current implementations will continue to work.
Later at some point we can remove the deprecated version for a breaking change
} | ||
}) | ||
.thenCompose(errorOrValue -> errorOrValue) | ||
.thenApply(result -> unboxPossibleDataFetcherResult(executionContext, parameters, result)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we need to flat map the above CF<T>
back to a
I made it use that. I am confused as to whether CompletionStage is every
worth it and when to use it
…On Fri, 4 Jun 2021 at 18:43, Andreas Marek ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In src/main/java/graphql/execution/DataFetcherExceptionHandler.java
<#2371 (comment)>
:
> DataFetcherExceptionHandlerResult onException(DataFetcherExceptionHandlerParameters handlerParameters);
+
+ /**
+ * When an exception occurs during a call to a ***@***.*** DataFetcher} then this handler
+ * is called to shape the errors that should be placed in the ***@***.*** ExecutionResult#getErrors()}
+ * list of errors.
+ *
+ * @param handlerParameters the parameters to this callback
+ *
+ * @return a result that can contain custom formatted ***@***.*** graphql.GraphQLError}s
+ */
+ default CompletionStage<DataFetcherExceptionHandlerResult> handleException(DataFetcherExceptionHandlerParameters handlerParameters) {
I think we use CompletableFuture everywhere, no?
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#2371 (review)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAMVWSOORI5QR6WOVCRFOT3TRCG4BANCNFSM46CLNQFQ>
.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM from an API perspective.
@bbakerman would it be possible to merge this PR? or at least the commit e7f5a91 ? |
@dfa1 - done |
This makes the data fetching exception handlers async.
The old method is left in place for backwards compatibiility reasons but the new async version is the one called.
It will delegate back to the old sync method.
New implementations can use proper async processing in making fetching errros