Skip to content

remove absolute graphql error #1941

Merged
bbakerman merged 3 commits into
masterfrom
remove-absolute-error
Nov 10, 2020
Merged

remove absolute graphql error #1941
bbakerman merged 3 commits into
masterfrom
remove-absolute-error

Conversation

@andimarek

Copy link
Copy Markdown
Member

and special relative error flag in DataFetcherResult

@andimarek andimarek added the breaking change requires a new major version to be relased label Jun 6, 2020
@andimarek andimarek added this to the 16.0 milestone Jun 6, 2020
@andimarek andimarek requested a review from bbakerman June 6, 2020 05:50
@juriad

juriad commented Jun 6, 2020

Copy link
Copy Markdown

This PR will break my use-case. Since the PR is not connected to any issue, I will describe my use-case here and then probably move it to a better place...

I have a fetcher which returns a list of objects from database which serve as breadcrumbs on frontend. This is modeled as non-nullable list of nullable objects; the order matters in this list .
Fetching any of the objects can fail (user does not have permissions, problems with communication between microservices, ...), yet the list as whole is always well defined (I know the number of items).

My data fetcher is actually internally working with List<CompletableFuture<X>>, but its return type is CompletableFuture<List<DataFetchingResult<X>>> because I did not find any other way how to return a list which has errors on some items. An inner completed future is converted to DFR with data, an inner failed future is converted to DFR with an error.

I am using the relative flag to allow myself the comfort of not setting locations and path on the GraphQLError. The GraphQLError contains the original exception. After the query is finished, I post-process ExceptionWhileDataFetching and AbsoluteGraphQLError using the wrapped exception to properly format the error. (Some frameworks are involved, but this is roughly what is happening.)

I know that I am misusing the DataFetchingResult and AbsoluteGraphQLError, but I don't have a better solution yet. I would like to use DataFetcherExceptionHandler to handle errors, but that only works when the fetcher throws an exception (ExecutionStrategy#fetchField) but not later (`ExecutionStrategy#completeValueForList).

An ideal solution to my use-case would allow returning exceptions from items of a list without using DFR because DFR requires GraphQLError which is already formatted (more-or-less). I am playing with an idea to extend ValueUnboxer (introduced in 14.0) to allow throwing exceptions which would then be processed in a similar fashion as exceptions thrown by a fetcher. That way processing of all errors can be concentrated at one place. CompletableFuture might then be handled just as another boxing type. This extension would then allow returning monadic types such as Kotlin's Result and Scala's Try not just Optional and its variations.

I was just considering whether to discuss my idea on Spektrum chat or create a Feature Request issue, but your PR made me act faster. :-) Even if you merge the PR, I can just pass something like ExecutionStepInfo to errorResult (see below) to handle the error details myself instead of delegating it to the AbsoluteGraphQLError.

A piece of code I use to handle exceptions on list items:

@UtilityClass
public class ExceptionUtils {
    public static <T> DataFetcherResult<T> errorResult(Exception exception, Object... localPath) {
        return DataFetcherResult.<T>newResult().error(new GraphQLError() {
            @Override
            public String getMessage() {
                return exception.getMessage();
            }

            @Override
            public List<SourceLocation> getLocations() {
                return Collections.emptyList();
            }

            @Override
            public ErrorClassification getErrorType() {
                return ErrorType.DataFetchingException;
            }

            @Override
            public List<Object> getPath() {
                return List.of(localPath);
            }

            @Override
            public Map<String, Object> getExtensions() {
                return Map.of("exception", exception);
            }
        }).mapRelativeErrors(true).build();
    }

    public static <T> DataFetcherResult<T> okResult(T data) {
        return DataFetcherResult.<T>newResult().data(data).build();
    }

    public static Exception getExceptionFromAbsoluteGraphQLError(AbsoluteGraphQLError error) {
        return (Exception) error.getExtensions().get("exception");
    }
}

@bbakerman

bbakerman commented Jun 8, 2020

Copy link
Copy Markdown
Member

My data fetcher is actually internally working with List<CompletableFuture>, but its return type is CompletableFuture<List<DataFetchingResult>> because I did not find any other way how to return a list which has errors on some items. An inner completed future is converted to DFR with data, an inner failed future is converted to DFR with an error.

You can ;post process the results of a list of CFs. You need to wait on all the results to come in to give out a single CF

See java.util.concurrent.CompletableFuture#allOf


List<CompletableFuture<X>> couldHaveFailures = callSomeCode(env)
CompletableFuture<Void> allDone  = CompletableFuture.allOf(couldHaveFailures)

CompleteableFuture<DataFetcherResult> finalCF = allDone.whenComplete( ignored, throwable) {
     
     for (cf : couldHaveFailures) {
        if (cf.completeedExceptionally()) {
              /// do you error handling
       else {
             // make your list
       }

   return DataFetcherResult.newResult().data(...your list).errors(...your errors).build();
}

return finalCF;




# Conflicts:
#	src/main/java/graphql/execution/AbsoluteGraphQLError.java
#	src/main/java/graphql/execution/ExecutionStrategy.java
#	src/main/java/graphql/execution/nextgen/ValueFetcher.java
#	src/test/groovy/graphql/execution/AbsoluteGraphQLErrorTest.groovy
@bbakerman bbakerman merged commit fccbe35 into master Nov 10, 2020
@andimarek andimarek deleted the remove-absolute-error branch May 4, 2021 21:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

breaking change requires a new major version to be relased

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants