Skip to content

Adding support for making the exception handlers async #2371

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.

Sign up for GitHub

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

Merged
bbakerman merged 6 commits into from
Jun 23, 2021
Merged

Adding support for making the exception handlers async #2371

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
271bf3a
Adding support for making the exception handlers async
bbakerman Jun 4, 2021
86f1b21
Better function naming here
bbakerman Jun 4, 2021
cf3cb83
MAde it return completable future
bbakerman Jun 4, 2021
9a71b50
Addressing contention on error list - moving away from synchronised list
bbakerman Jun 5, 2021
0d54737
ImmutableList handles empty list efficiently
bbakerman Jun 5, 2021
e7f5a91
moved to AtomicRef + ImmutableList
bbakerman Jun 7, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/main/java/graphql/ExecutionResultImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@ private ExecutionResultImpl(boolean dataPresent, Object data, List<? extends Gra
this.dataPresent = dataPresent;
this.data = data;

if (errors != null && !errors.isEmpty()) {
if (errors != null) {
this.errors = ImmutableList.copyOf(errors);
} else {
this.errors = ImmutableKit.emptyList();
Expand Down
26 changes: 25 additions & 1 deletion src/main/java/graphql/collect/ImmutableKit.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@

import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableMap;
import com.google.common.collect.ImmutableSet;
import graphql.Internal;

import java.util.Collection;
Expand Down Expand Up @@ -94,7 +95,7 @@ public static <T, R> ImmutableList<R> map(Iterable<? extends T> iterable, Functi
* @param extraValues more values to add
* @param <T> for two
*
* @return an Immutable list with the extra effort.
* @return an Immutable list with the extra items.
*/
public static <T> ImmutableList<T> addToList(Collection<? extends T> existing, T newValue, T... extraValues) {
assertNotNull(existing);
Expand All @@ -109,4 +110,27 @@ public static <T> ImmutableList<T> addToList(Collection<? extends T> existing, T
return newList.build();
}

/**
* This constructs a new Immutable set from an existing collection and adds a new element to it.
*
* @param existing the existing collection
* @param newValue the new value to add
* @param extraValues more values to add
* @param <T> for two
*
* @return an Immutable Set with the extra itens.
*/
public static <T> ImmutableSet<T> addToSet(Collection<? extends T> existing, T newValue, T... extraValues) {
assertNotNull(existing);
assertNotNull(newValue);
int expectedSize = existing.size() + 1 + extraValues.length;
ImmutableSet.Builder<T> newSet = ImmutableSet.builderWithExpectedSize(expectedSize);
newSet.addAll(existing);
newSet.add(newValue);
for (T extraValue : extraValues) {
newSet.add(extraValue);
}
return newSet.build();
}

}
27 changes: 25 additions & 2 deletions src/main/java/graphql/execution/DataFetcherExceptionHandler.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,22 +1,45 @@
package graphql.execution;

import graphql.ExecutionResult;
import graphql.PublicSpi;
import graphql.schema.DataFetcher;
import graphql.schema.DataFetchingEnvironment;

import java.util.concurrent.CompletableFuture;
import java.util.concurrent.CompletionStage;

/**
* This is called when an exception is thrown during {@link graphql.schema.DataFetcher#get(DataFetchingEnvironment)} execution
*/
@PublicSpi
public interface DataFetcherExceptionHandler {

/**
* When an exception during a call to a {@link DataFetcher} then this handler
* is called back to shape the error that should be placed in the list of errors
* When an exception occurs during a call to a {@link DataFetcher} then this handler
* is called to shape the errors that should be placed in the {@link ExecutionResult#getErrors()}
* list of errors.
*
* @param handlerParameters the parameters to this callback
*
* @return a result that can contain custom formatted {@link graphql.GraphQLError}s
*
* @deprecated use {@link #handleException(DataFetcherExceptionHandlerParameters)} instead which as an asynchronous
* version
*/
@Deprecated
DataFetcherExceptionHandlerResult onException(DataFetcherExceptionHandlerParameters handlerParameters);

/**
* When an exception occurs during a call to a {@link DataFetcher} then this handler
* is called to shape the errors that should be placed in the {@link ExecutionResult#getErrors()}
* list of errors.
*
* @param handlerParameters the parameters to this callback
*
* @return a result that can contain custom formatted {@link graphql.GraphQLError}s
*/
default CompletableFuture<DataFetcherExceptionHandlerResult> handleException(DataFetcherExceptionHandlerParameters handlerParameters) {
DataFetcherExceptionHandlerResult result = onException(handlerParameters);
return CompletableFuture.completedFuture(result);
}
Copy link
Member Author

@bbakerman bbakerman Jun 4, 2021

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

}
71 changes: 52 additions & 19 deletions src/main/java/graphql/execution/ExecutionContext.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
import graphql.GraphQLError;
import graphql.PublicApi;
import graphql.cachecontrol.CacheControl;
import graphql.collect.ImmutableKit;
import graphql.collect.ImmutableMapWithNullValues;
import graphql.execution.instrumentation.Instrumentation;
import graphql.execution.instrumentation.InstrumentationState;
Expand All @@ -19,13 +20,12 @@
import graphql.util.FpKit;
import org.dataloader.DataLoaderRegistry;

import java.util.ArrayList;
import java.util.Collections;
import java.util.HashSet;
import java.util.List;
import java.util.Locale;
import java.util.Map;
import java.util.Set;
import java.util.concurrent.atomic.AtomicReference;
import java.util.function.Consumer;
import java.util.function.Supplier;

Expand All @@ -47,7 +47,7 @@ public class ExecutionContext {
private final Object context;
private final Object localContext;
private final Instrumentation instrumentation;
private final List<GraphQLError> errors = Collections.synchronizedList(new ArrayList<>());
private final AtomicReference<ImmutableList<GraphQLError>> errors = new AtomicReference<>(ImmutableKit.emptyList());
private final Set<ResultPath> errorPaths = new HashSet<>();
private final DataLoaderRegistry dataLoaderRegistry;
private final CacheControl cacheControl;
Expand All @@ -74,7 +74,7 @@ public class ExecutionContext {
this.cacheControl = builder.cacheControl;
this.locale = builder.locale;
this.valueUnboxer = builder.valueUnboxer;
this.errors.addAll(builder.errors);
this.errors.set(builder.errors);
this.localContext = builder.localContext;
this.executionInput = builder.executionInput;
queryTree = FpKit.interThreadMemoize(() -> NormalizedQueryFactory.createNormalizedQuery(graphQLSchema, operationDefinition, fragmentsByName, variables));
Expand Down Expand Up @@ -159,15 +159,17 @@ public ValueUnboxer getValueUnboxer() {
* @param fieldPath the field path to put it under
*/
public void addError(GraphQLError error, ResultPath fieldPath) {
//
// see http://facebook.github.io/graphql/#sec-Errors-and-Non-Nullability about how per
// field errors should be handled - ie only once per field if its already there for nullability
// but unclear if its not that error path
//
if (!errorPaths.add(fieldPath)) {
return;
synchronized (this) {
//
// see http://facebook.github.io/graphql/#sec-Errors-and-Non-Nullability about how per
// field errors should be handled - ie only once per field if its already there for nullability
// but unclear if its not that error path
//
if (!errorPaths.add(fieldPath)) {
return;
}
this.errors.set(ImmutableKit.addToList(this.errors.get(), error));
}
this.errors.add(error);
}

/**
Expand All @@ -177,20 +179,51 @@ public void addError(GraphQLError error, ResultPath fieldPath) {
* @param error the error to add
*/
public void addError(GraphQLError error) {
// see https://github.com/graphql-java/graphql-java/issues/888 on how the spec is unclear
// on how exactly multiple errors should be handled - ie only once per field or not outside the nullability
// aspect.
if (error.getPath() != null) {
this.errorPaths.add(ResultPath.fromList(error.getPath()));
synchronized (this) {
// see https://github.com/graphql-java/graphql-java/issues/888 on how the spec is unclear
// on how exactly multiple errors should be handled - ie only once per field or not outside the nullability
// aspect.
if (error.getPath() != null) {
ResultPath path = ResultPath.fromList(error.getPath());
this.errorPaths.add(path);
}
this.errors.set(ImmutableKit.addToList(this.errors.get(), error));
}
}

/**
* This method will allow you to add errors into the running execution context, without a check
* for per field unique-ness
*
* @param errors the errors to add
*/
public void addErrors(List<GraphQLError> errors) {
if (errors.isEmpty()) {
return;
}
// we are synchronised because we set two fields at once - but we only ever read one of them later
// in getErrors so no need for synchronised there.
synchronized (this) {
Set<ResultPath> newErrorPaths = new HashSet<>();
for (GraphQLError error : errors) {
// see https://github.com/graphql-java/graphql-java/issues/888 on how the spec is unclear
// on how exactly multiple errors should be handled - ie only once per field or not outside the nullability
// aspect.
if (error.getPath() != null) {
ResultPath path = ResultPath.fromList(error.getPath());
newErrorPaths.add(path);
}
}
this.errorPaths.addAll(newErrorPaths);
this.errors.set(ImmutableKit.concatLists(this.errors.get(), errors));
}
this.errors.add(error);
}

/**
* @return the total list of errors for this execution context
*/
public List<GraphQLError> getErrors() {
return ImmutableList.copyOf(errors);
return errors.get();
}

public ExecutionStrategy getQueryStrategy() {
Expand Down
36 changes: 22 additions & 14 deletions src/main/java/graphql/execution/ExecutionStrategy.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,7 @@
import java.util.OptionalInt;
import java.util.concurrent.CompletableFuture;
import java.util.concurrent.CompletionException;
import java.util.function.Function;
import java.util.function.Supplier;

import static graphql.execution.Async.exceptionallyCompletedFuture;
Expand Down Expand Up @@ -290,12 +291,12 @@ protected CompletableFuture<FetchedValue> fetchField(ExecutionContext executionC
.handle((result, exception) -> {
fetchCtx.onCompleted(result, exception);
if (exception != null) {
handleFetchingException(executionContext, environment, exception);
return null;
return handleFetchingException(executionContext, environment, exception);
} else {
return result;
return CompletableFuture.completedFuture(result);
}
})
.thenCompose(Function.identity())
.thenApply(result -> unboxPossibleDataFetcherResult(executionContext, parameters, result));
Copy link
Member Author

@bbakerman bbakerman Jun 4, 2021

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

}

Expand All @@ -309,9 +310,8 @@ protected FetchedValue unboxPossibleDataFetcherResult(ExecutionContext execution
Object result) {

if (result instanceof DataFetcherResult) {
//noinspection unchecked
DataFetcherResult<?> dataFetcherResult = (DataFetcherResult) result;
dataFetcherResult.getErrors().forEach(executionContext::addError);
DataFetcherResult<?> dataFetcherResult = (DataFetcherResult<?>) result;
executionContext.addErrors(dataFetcherResult.getErrors());

Object localContext = dataFetcherResult.getLocalContext();
if (localContext == null) {
Expand All @@ -333,26 +333,35 @@ protected FetchedValue unboxPossibleDataFetcherResult(ExecutionContext execution
}
}

protected void handleFetchingException(ExecutionContext executionContext,
DataFetchingEnvironment environment,
Throwable e) {
protected <T> CompletableFuture<T> handleFetchingException(ExecutionContext executionContext,
DataFetchingEnvironment environment,
Throwable e) {
DataFetcherExceptionHandlerParameters handlerParameters = DataFetcherExceptionHandlerParameters.newExceptionParameters()
.dataFetchingEnvironment(environment)
.exception(e)
.build();

DataFetcherExceptionHandlerResult handlerResult;
try {
handlerResult = dataFetcherExceptionHandler.onException(handlerParameters);
return asyncHandleException(dataFetcherExceptionHandler, handlerParameters, executionContext);
} catch (Exception handlerException) {
handlerParameters = DataFetcherExceptionHandlerParameters.newExceptionParameters()
.dataFetchingEnvironment(environment)
.exception(handlerException)
.build();
handlerResult = new SimpleDataFetcherExceptionHandler().onException(handlerParameters);
return asyncHandleException(new SimpleDataFetcherExceptionHandler(), handlerParameters, executionContext);
}
handlerResult.getErrors().forEach(executionContext::addError);
}

private <T> CompletableFuture<T> asyncHandleException(DataFetcherExceptionHandler handler, DataFetcherExceptionHandlerParameters handlerParameters, ExecutionContext executionContext) {
return handler.handleException(handlerParameters)
.thenApply(handlerResult -> {
// the side effect is that we added the returned errors to the execution context
// here
executionContext.addErrors(handlerResult.getErrors());
// and we return null because there is no data for the executed field
return null;
}
);
}

/**
Expand Down Expand Up @@ -706,7 +715,6 @@ private void handleTypeMismatchProblem(ExecutionContext context, ExecutionStrate
TypeMismatchError error = new TypeMismatchError(parameters.getPath(), parameters.getExecutionStepInfo().getUnwrappedNonNullType());
logNotSafe.warn("{} got {}", error.getMessage(), result.getClass());
context.addError(error);

}


Expand Down
19 changes: 19 additions & 0 deletions src/test/groovy/graphql/collect/ImmutableKitTest.groovy
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,4 +42,23 @@ class ImmutableKitTest extends Specification {
then:
list == ["a", "b", "c", "d", "e"]
}

def "can add to sets"() {
def set = new HashSet(["a"])

when:
set = ImmutableKit.addToSet(set, "b")
then:
set == ["a", "b"] as Set

when:
set = ImmutableKit.addToSet(set, "c", "d", "e")
then:
set == ["a", "b", "c", "d", "e"] as Set

when:
set = ImmutableKit.addToSet(set, "c", "d", "e", "f")
then:
set == ["a", "b", "c", "d", "e", "f"] as Set
}
}
Loading