Fix NPE in FieldLevelTrackingApproach when type mismatch error

tinnou · bbakerman · commit 653d67f21474 · 2018-08-07T13:13:44.000+10:00
diff --git a/src/main/java/graphql/execution/AsyncExecutionStrategy.java b/src/main/java/graphql/execution/AsyncExecutionStrategy.java
@@ -82,6 +82,12 @@ public CompletableFuture<ExecutionResult> execute(ExecutionContext executionCont
             List<CompletableFuture<ExecutionResult>> executionResultFuture = completeValueInfos.stream().map(FieldValueInfo::getFieldValue).collect(Collectors.toList());
             executionStrategyCtx.onFieldValuesInfo(completeValueInfos);
             Async.each(executionResultFuture).whenComplete(handleResultsConsumer);
+        }).exceptionally((ex) -> {
+            // if there are any issues with combining/handling the field results,
+            // complete the future at all costs and bubble up any thrown exception so
+            // the execution does not hang.
+            overallResult.completeExceptionally(ex);
+            return null;
         });
 
         overallResult.whenComplete(executionStrategyCtx::onCompleted);
diff --git a/src/main/java/graphql/execution/FieldValueInfo.java b/src/main/java/graphql/execution/FieldValueInfo.java
@@ -3,9 +3,12 @@
 import graphql.ExecutionResult;
 import graphql.PublicApi;
 
+import java.util.ArrayList;
 import java.util.List;
 import java.util.concurrent.CompletableFuture;
 
+import static graphql.Assert.assertNotNull;
+
 @PublicApi
 public class FieldValueInfo {
 
@@ -23,6 +26,7 @@ public enum CompleteValueType {
     private final List<FieldValueInfo> fieldValueInfos;
 
     private FieldValueInfo(CompleteValueType completeValueType, CompletableFuture<ExecutionResult> fieldValue, List<FieldValueInfo> fieldValueInfos) {
+        assertNotNull(fieldValueInfos, "fieldValueInfos can't be null");
         this.completeValueType = completeValueType;
         this.fieldValue = fieldValue;
         this.fieldValueInfos = fieldValueInfos;
@@ -57,7 +61,7 @@ public String toString() {
     public static class Builder {
         private CompleteValueType completeValueType;
         private CompletableFuture<ExecutionResult> executionResultFuture;
-        private List<FieldValueInfo> listInfos;
+        private List<FieldValueInfo> listInfos = new ArrayList<>();
 
         public Builder(CompleteValueType completeValueType) {
             this.completeValueType = completeValueType;
@@ -74,6 +78,7 @@ public Builder fieldValue(CompletableFuture<ExecutionResult> executionResultFutu
         }
 
         public Builder fieldValueInfos(List<FieldValueInfo> listInfos) {
+            assertNotNull(listInfos, "fieldValueInfos can't be null");
             this.listInfos = listInfos;
             return this;
         }
diff --git a/src/test/groovy/graphql/execution/AsyncExecutionStrategyTest.groovy b/src/test/groovy/graphql/execution/AsyncExecutionStrategyTest.groovy
@@ -1,7 +1,10 @@
 package graphql.execution
 
 import graphql.ErrorType
+import graphql.ExecutionResult
+import graphql.execution.instrumentation.ExecutionStrategyInstrumentationContext
 import graphql.execution.instrumentation.SimpleInstrumentation
+import graphql.execution.instrumentation.parameters.InstrumentationExecutionStrategyParameters
 import graphql.language.Field
 import graphql.language.OperationDefinition
 import graphql.parser.Parser
@@ -11,6 +14,7 @@ import graphql.schema.GraphQLSchema
 import spock.lang.Specification
 
 import java.util.concurrent.CompletableFuture
+import java.util.concurrent.CompletionException
 import java.util.concurrent.atomic.AtomicInteger
 import java.util.concurrent.locks.ReentrantLock
 
@@ -211,4 +215,67 @@ class AsyncExecutionStrategyTest extends Specification {
         result.get().getErrors().get(0).errorType == ErrorType.DataFetchingException
 
     }
+
+    def "exception in instrumentation while combining data"() {
+        GraphQLSchema schema = schema(
+                { env -> CompletableFuture.completedFuture("world") },
+                { env -> CompletableFuture.completedFuture("world2") }
+        )
+        String query = "{hello, hello2}"
+        def document = new Parser().parseDocument(query)
+        def operation = document.definitions[0] as OperationDefinition
+
+        def typeInfo = ExecutionTypeInfo.newTypeInfo()
+                .type(schema.getQueryType())
+                .build()
+
+        ExecutionContext executionContext = new ExecutionContextBuilder()
+                .graphQLSchema(schema)
+                .executionId(ExecutionId.generate())
+                .operationDefinition(operation)
+                .instrumentation(new SimpleInstrumentation() {
+                    @Override
+                    ExecutionStrategyInstrumentationContext beginExecutionStrategy(InstrumentationExecutionStrategyParameters parameters) {
+                        return new ExecutionStrategyInstrumentationContext() {
+
+                            @Override
+                            void onFieldValuesInfo(List<FieldValueInfo> fieldValueInfoList) {
+                                throw new RuntimeException("Exception raised from instrumentation")
+                            }
+
+                            @Override
+                            public void onDispatched(CompletableFuture<ExecutionResult> result) {
+
+                            }
+
+                            @Override
+                            public void onCompleted(ExecutionResult result, Throwable t) {
+
+                            }
+                        }
+                    }
+                })
+                .build()
+        ExecutionStrategyParameters executionStrategyParameters = ExecutionStrategyParameters
+                .newParameters()
+                .typeInfo(typeInfo)
+                .fields(['hello': [new Field('hello')], 'hello2': [new Field('hello2')]])
+                .build()
+
+        AsyncExecutionStrategy asyncExecutionStrategy = new AsyncExecutionStrategy()
+        when:
+        def result = asyncExecutionStrategy.execute(executionContext, executionStrategyParameters)
+
+        then: "result should be completed"
+        result.isCompletedExceptionally()
+
+        when:
+        result.join()
+
+        then: "exceptions thrown from the instrumentation should be bubbled up"
+        def ex = thrown(CompletionException)
+        ex.cause.message == "Exception raised from instrumentation"
+    }
+
+
 }
diff --git a/src/test/groovy/graphql/execution/FieldValueInfoTest.groovy b/src/test/groovy/graphql/execution/FieldValueInfoTest.groovy
@@ -0,0 +1,30 @@
+package graphql.execution
+
+import graphql.AssertException
+import spock.lang.Specification
+
+
+class FieldValueInfoTest extends Specification{
+
+    def "simple constructor test"() {
+        when:
+        def fieldValueInfo = FieldValueInfo.newFieldValueInfo().build()
+
+        then: "fieldValueInfos to be empty list"
+        fieldValueInfo.fieldValueInfos == [] as List
+
+        and: "other fields to be null "
+        fieldValueInfo.fieldValue == null
+        fieldValueInfo.completeValueType == null
+    }
+
+    def "negative constructor test"() {
+        when:
+        FieldValueInfo.newFieldValueInfo()
+                .fieldValueInfos(null)
+                .build()
+        then:
+        def assEx = thrown(AssertException)
+        assEx.message.contains("fieldValueInfos")
+    }
+}
diff --git a/src/test/groovy/graphql/execution/instrumentation/dataloader/DataLoaderTypeMismatchTest.groovy b/src/test/groovy/graphql/execution/instrumentation/dataloader/DataLoaderTypeMismatchTest.groovy
@@ -0,0 +1,74 @@
+package graphql.execution.instrumentation.dataloader
+
+import graphql.GraphQL
+import graphql.schema.DataFetcher
+import graphql.schema.DataFetchingEnvironment
+import graphql.schema.idl.RuntimeWiring
+import graphql.schema.idl.SchemaGenerator
+import graphql.schema.idl.SchemaParser
+import org.dataloader.BatchLoader
+import org.dataloader.DataLoader
+import org.dataloader.DataLoaderRegistry
+import spock.lang.Specification
+
+import java.util.concurrent.CompletableFuture
+import java.util.concurrent.CompletionStage
+
+import static graphql.schema.idl.TypeRuntimeWiring.newTypeWiring
+
+class DataLoaderTypeMismatchTest extends Specification {
+
+    def "when actual field value return type is different from expected return type, then it should not hang execution"() {
+        setup:
+        def sdl = """
+        type Todo {
+           id: ID!
+        }
+
+        type Query {
+           getTodos: [Todo]
+        }
+
+        schema {
+           query: Query
+        }"""
+
+        def typeDefinitionRegistry = new SchemaParser().parse(sdl)
+
+        def dataLoader = new DataLoader<Object, Object>(new BatchLoader<Object, Object>() {
+            @Override
+            CompletionStage<List<Object>> load(List<Object> keys) {
+                return CompletableFuture.completedFuture([
+                        [a: "map instead of a list of todos"]
+                ])
+            }
+        })
+        def dataLoaderRegistry = new DataLoaderRegistry()
+        dataLoaderRegistry.register("getTodos", dataLoader)
+
+        def todosDef = new DataFetcher<CompletableFuture<Object>>() {
+            @Override
+            CompletableFuture<Object> get(DataFetchingEnvironment environment) {
+                return dataLoader.load(environment)
+            }
+        }
+
+        def wiring = RuntimeWiring.newRuntimeWiring()
+                .type(newTypeWiring("Query")
+                    .dataFetcher("getTodos", todosDef))
+                .build()
+
+        def schema = new SchemaGenerator().makeExecutableSchema(typeDefinitionRegistry, wiring)
+
+        def graphql = GraphQL.newGraphQL(schema)
+                .instrumentation(new DataLoaderDispatcherInstrumentation(dataLoaderRegistry))
+                .build()
+
+        when:
+        def result = graphql.execute("query { getTodos { id } }")
+
+        then: "execution shouldn't hang"
+        !result.errors.empty
+        result.errors[0].message == "Can't resolve value (/getTodos) : type mismatch error, expected type LIST"
+    }
+}
