Fix setting DataFetcherExceptionHandler (#2964)

* Fix data fetcher exception handler * Add data fetcher exception handler to standalone * Set on async executor service * Upgrade graphql
yahoo · Apr 30, 2023 · fd6f86b · fd6f86b
1 parent a3ff2d6
commit fd6f86b
Show file tree

Hide file tree

Showing 20 changed files with 143 additions and 31 deletions.
diff --git a/elide-async/src/main/java/com/yahoo/elide/async/service/AsyncExecutorService.java b/elide-async/src/main/java/com/yahoo/elide/async/service/AsyncExecutorService.java
@@ -14,6 +14,7 @@
 import com.yahoo.elide.core.security.User;
 import com.yahoo.elide.graphql.QueryRunner;
 
+import graphql.execution.DataFetcherExceptionHandler;
 import graphql.execution.SimpleDataFetcherExceptionHandler;
 import jakarta.inject.Inject;
 import lombok.Data;
@@ -23,6 +24,7 @@
 import java.util.Date;
 import java.util.HashMap;
 import java.util.Map;
+import java.util.Optional;
 import java.util.concurrent.Callable;
 import java.util.concurrent.ExecutionException;
 import java.util.concurrent.ExecutorService;
@@ -59,16 +61,14 @@ private static class AsyncAPIResultFuture {
     }
 
     @Inject
-    public AsyncExecutorService(Elide elide, ExecutorService executor, ExecutorService updater,
-                                AsyncAPIDAO asyncAPIDao) {
+    public AsyncExecutorService(Elide elide, ExecutorService executor, ExecutorService updater, AsyncAPIDAO asyncAPIDao,
+            Optional<DataFetcherExceptionHandler> optionalDataFetcherExceptionHandler) {
         this.elide = elide;
         runners = new HashMap<>();
 
         for (String apiVersion : elide.getElideSettings().getDictionary().getApiVersions()) {
-
-            //Because the queries are async, there is no need to override the default exception handler to customize
-            //the error response.
-            runners.put(apiVersion, new QueryRunner(elide, apiVersion, new SimpleDataFetcherExceptionHandler()));
+            runners.put(apiVersion, new QueryRunner(elide, apiVersion,
+                    optionalDataFetcherExceptionHandler.orElseGet(SimpleDataFetcherExceptionHandler::new)));
         }
 
         this.executor = executor;

diff --git a/elide-async/src/test/java/com/yahoo/elide/async/service/AsyncExecutorServiceTest.java b/elide-async/src/test/java/com/yahoo/elide/async/service/AsyncExecutorServiceTest.java
@@ -9,6 +9,8 @@
 import static org.junit.jupiter.api.Assertions.assertEquals;
 import static org.junit.jupiter.api.Assertions.assertNotNull;
 import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.reset;
+import static org.mockito.Mockito.spy;
 import static org.mockito.Mockito.times;
 import static org.mockito.Mockito.verify;
 import static org.mockito.Mockito.when;
@@ -30,13 +32,19 @@
 import com.yahoo.elide.core.security.User;
 import com.yahoo.elide.core.security.checks.Check;
 import com.yahoo.elide.core.utils.DefaultClassScanner;
+
 import org.apache.http.NoHttpResponseException;
 import org.junit.jupiter.api.BeforeAll;
+import org.junit.jupiter.api.BeforeEach;
 import org.junit.jupiter.api.Test;
 import org.junit.jupiter.api.TestInstance;
 
+import graphql.execution.DataFetcherExceptionHandler;
+import graphql.execution.SimpleDataFetcherExceptionHandler;
+
 import java.util.HashMap;
 import java.util.Map;
+import java.util.Optional;
 import java.util.concurrent.Callable;
 import java.util.concurrent.Executors;
 
@@ -49,6 +57,12 @@ public class AsyncExecutorServiceTest {
     private User testUser;
     private RequestScope scope;
     private ResultStorageEngine resultStorageEngine;
+    private DataFetcherExceptionHandler dataFetcherExceptionHandler = spy(new SimpleDataFetcherExceptionHandler());
+
+    @BeforeEach
+    void resetMocks() {
+        reset(dataFetcherExceptionHandler);
+    }
 
     @BeforeAll
     public void setupMockElide() {
@@ -66,7 +80,7 @@ public void setupMockElide() {
         scope = mock(RequestScope.class);
         resultStorageEngine = mock(FileResultStorageEngine.class);
         service = new AsyncExecutorService(elide, Executors.newFixedThreadPool(5), Executors.newFixedThreadPool(5),
-                        asyncAPIDao);
+                        asyncAPIDao, Optional.of(dataFetcherExceptionHandler));
 
     }
 

diff --git a/...main/java/com/yahoo/elide/datastores/jms/websocket/SubscriptionWebSocketConfigurator.java b/...main/java/com/yahoo/elide/datastores/jms/websocket/SubscriptionWebSocketConfigurator.java
@@ -26,6 +26,8 @@
 
 import graphql.ExecutionResult;
 import graphql.GraphQLError;
+import graphql.execution.DataFetcherExceptionHandler;
+import graphql.execution.SimpleDataFetcherExceptionHandler;
 import jakarta.jms.ConnectionFactory;
 import jakarta.websocket.server.ServerEndpointConfig;
 import lombok.AccessLevel;
@@ -73,6 +75,9 @@ public class SubscriptionWebSocketConfigurator extends ServerEndpointConfig.Conf
     @Builder.Default
     protected boolean sendPingOnSubscribe = false;
 
+    @Builder.Default
+    protected DataFetcherExceptionHandler dataFetcherExceptionHandler = new SimpleDataFetcherExceptionHandler();
+
     @Override
     public <T> T getEndpointInstance(Class<T> endpointClass) throws InstantiationException {
         if (endpointClass.equals(SubscriptionWebSocket.class)) {
@@ -134,6 +139,7 @@ protected SubscriptionWebSocket buildWebSocket(Elide elide) {
                 .userFactory(userFactory)
                 .sendPingOnSubscribe(sendPingOnSubscribe)
                 .verboseErrors(verboseErrors)
+                .dataFetcherExceptionHandler(dataFetcherExceptionHandler)
                 .build();
     }
 }
diff --git a/elide-graphql/pom.xml b/elide-graphql/pom.xml
@@ -56,15 +56,17 @@
             <artifactId>jackson-databind</artifactId>
             <scope>provided</scope>
         </dependency>
+        <dependency>
+            <groupId>com.apollographql.federation</groupId>
+            <artifactId>federation-graphql-java-support</artifactId>
+        </dependency>
         <dependency>
             <groupId>com.graphql-java</groupId>
             <artifactId>graphql-java</artifactId>
-            <version>19.2</version>
         </dependency>
         <dependency>
             <groupId>com.graphql-java</groupId>
             <artifactId>graphql-java-extended-scalars</artifactId>
-            <version>20.0</version>
         </dependency>
         <dependency>
             <groupId>jakarta.websocket</groupId>
@@ -131,11 +133,6 @@
             <artifactId>jakarta.persistence-api</artifactId>
             <scope>test</scope>
         </dependency>
-        <dependency>
-            <groupId>com.apollographql.federation</groupId>
-            <artifactId>federation-graphql-java-support</artifactId>
-            <version>2.3.1</version>
-        </dependency>
         <dependency>
             <groupId>org.glassfish.jersey.containers</groupId>
             <artifactId>jersey-container-servlet</artifactId>

diff --git a/elide-graphql/src/main/java/com/yahoo/elide/graphql/GraphQLEndpoint.java b/elide-graphql/src/main/java/com/yahoo/elide/graphql/GraphQLEndpoint.java
@@ -16,6 +16,7 @@
 import com.yahoo.elide.utils.ResourceUtils;
 import org.apache.commons.lang3.StringUtils;
 
+import graphql.execution.DataFetcherExceptionHandler;
 import graphql.execution.SimpleDataFetcherExceptionHandler;
 import jakarta.inject.Inject;
 import jakarta.inject.Named;
@@ -35,6 +36,7 @@
 import java.util.HashMap;
 import java.util.List;
 import java.util.Map;
+import java.util.Optional;
 import java.util.UUID;
 
 /**
@@ -50,13 +52,15 @@ public class GraphQLEndpoint {
     private final HeaderUtils.HeaderProcessor headerProcessor;
 
     @Inject
-    public GraphQLEndpoint(@Named("elide") Elide elide) {
+    public GraphQLEndpoint(@Named("elide") Elide elide,
+            Optional<DataFetcherExceptionHandler> optionalDataFetcherExceptionHandler) {
         log.debug("Started ~~");
         this.elide = elide;
         this.headerProcessor = elide.getElideSettings().getHeaderProcessor();
         this.runners = new HashMap<>();
         for (String apiVersion : elide.getElideSettings().getDictionary().getApiVersions()) {
-            runners.put(apiVersion, new QueryRunner(elide, apiVersion, new SimpleDataFetcherExceptionHandler()));
+            runners.put(apiVersion, new QueryRunner(elide, apiVersion,
+                    optionalDataFetcherExceptionHandler.orElseGet(SimpleDataFetcherExceptionHandler::new)));
         }
     }
 

diff --git a/elide-graphql/src/main/java/com/yahoo/elide/graphql/QueryRunner.java b/elide-graphql/src/main/java/com/yahoo/elide/graphql/QueryRunner.java
@@ -100,6 +100,7 @@ public QueryRunner(Elide elide, String apiVersion, DataFetcherExceptionHandler e
                 nonEntityDictionary, elide.getElideSettings(), fetcher, apiVersion);
 
         api = GraphQL.newGraphQL(builder.build())
+                .defaultDataFetcherExceptionHandler(exceptionHandler)
                 .queryExecutionStrategy(new AsyncSerialExecutionStrategy(exceptionHandler))
                 .build();
 

diff --git a/.../src/main/java/com/yahoo/elide/graphql/subscriptions/websocket/SubscriptionWebSocket.java b/.../src/main/java/com/yahoo/elide/graphql/subscriptions/websocket/SubscriptionWebSocket.java
@@ -20,6 +20,8 @@
 
 import graphql.GraphQL;
 import graphql.execution.AsyncSerialExecutionStrategy;
+import graphql.execution.DataFetcherExceptionHandler;
+import graphql.execution.SimpleDataFetcherExceptionHandler;
 import graphql.execution.SubscriptionExecutionStrategy;
 import jakarta.websocket.OnClose;
 import jakarta.websocket.OnError;
@@ -70,6 +72,9 @@ public class SubscriptionWebSocket {
     @Builder.Default
     private boolean verboseErrors = false;
 
+    @Builder.Default
+    private DataFetcherExceptionHandler dataFetcherExceptionHandler = new SimpleDataFetcherExceptionHandler();
+
     private final Map<String, GraphQL> apis = new HashMap<>();
     private final ConcurrentMap<Session, SessionHandler> openSessions = new ConcurrentHashMap<>();
 
@@ -106,7 +111,8 @@ protected SubscriptionWebSocket(
             long maxIdleTimeoutMs,
             int maxMessageSize,
             boolean sendPingOnSubscribe,
-            boolean verboseErrors
+            boolean verboseErrors,
+            DataFetcherExceptionHandler dataFetcherExceptionHandler
     ) {
         this.elide = elide;
         this.executorService = executorService;
@@ -117,6 +123,7 @@ protected SubscriptionWebSocket(
         this.maxIdleTimeoutMs = maxIdleTimeoutMs;
         this.maxMessageSize = maxMessageSize;
         this.verboseErrors = verboseErrors;
+        this.dataFetcherExceptionHandler = dataFetcherExceptionHandler;
 
         EntityDictionary dictionary = elide.getElideSettings().getDictionary();
         for (String apiVersion : dictionary.getApiVersions()) {
@@ -127,8 +134,9 @@ protected SubscriptionWebSocket(
                     new SubscriptionDataFetcher(nonEntityDictionary), NO_VERSION);
 
             GraphQL api = GraphQL.newGraphQL(builder.build())
-                    .queryExecutionStrategy(new AsyncSerialExecutionStrategy())
-                    .subscriptionExecutionStrategy(new SubscriptionExecutionStrategy())
+                    .defaultDataFetcherExceptionHandler(this.dataFetcherExceptionHandler)
+                    .queryExecutionStrategy(new AsyncSerialExecutionStrategy(this.dataFetcherExceptionHandler))
+                    .subscriptionExecutionStrategy(new SubscriptionExecutionStrategy(this.dataFetcherExceptionHandler))
                     .build();
 
             apis.put(apiVersion, api);

diff --git a/elide-graphql/src/test/java/com/yahoo/elide/graphql/FetcherFetchTest.java b/elide-graphql/src/test/java/com/yahoo/elide/graphql/FetcherFetchTest.java
@@ -7,6 +7,8 @@
 package com.yahoo.elide.graphql;
 
 import static org.junit.jupiter.api.Assertions.assertTrue;
+import static org.mockito.ArgumentMatchers.any;
+import static org.mockito.Mockito.verify;
 
 import com.yahoo.elide.ElideResponse;
 import org.junit.jupiter.api.Test;
@@ -37,6 +39,7 @@ public void testMutationInQueryThrowsError() throws Exception {
                 + "}";
 
         assertQueryFailsWith(query, "Exception while fetching data (/book) : Data model writes are only allowed in mutations");
+        verify(dataFetcherExceptionHandler).handleException(any());
     }
 
     @Test

diff --git a/elide-graphql/src/test/java/com/yahoo/elide/graphql/FetcherUpdateTest.java b/elide-graphql/src/test/java/com/yahoo/elide/graphql/FetcherUpdateTest.java
@@ -5,6 +5,9 @@
  */
 package com.yahoo.elide.graphql;
 
+import static org.mockito.ArgumentMatchers.any;
+import static org.mockito.Mockito.verify;
+
 import org.junit.jupiter.api.Disabled;
 import org.junit.jupiter.api.Test;
 
@@ -48,13 +51,15 @@ public void testRootCollectionInvalidIds() throws Exception {
         // Update 1, create for id 42, create new book with title "abc"
         String expectedMessage = "Exception while fetching data (/book) : Unknown identifier [42] for book";
         runErrorComparisonTest("rootCollectionInvalidIds", expectedMessage);
+        verify(dataFetcherExceptionHandler).handleException(any());
     }
 
     @Test
     public void testRootCollectionMissingIds() throws Exception {
         // Update 1, create for id 42, create new book with title "abc"
         String expectedMessage = "Exception while fetching data (/book) : UPDATE data objects must include ids";
         runErrorComparisonTest("rootCollectionMissingIds", expectedMessage);
+        verify(dataFetcherExceptionHandler).handleException(any());
     }
 
     @Test

diff --git a/elide-graphql/src/test/java/com/yahoo/elide/graphql/GraphQLEndpointTest.java b/elide-graphql/src/test/java/com/yahoo/elide/graphql/GraphQLEndpointTest.java
@@ -20,6 +20,7 @@
 import static org.junit.jupiter.api.Assertions.assertEquals;
 import static org.junit.jupiter.api.Assertions.assertTrue;
 import static org.mockito.ArgumentMatchers.any;
+import static org.mockito.Mockito.reset;
 import static org.mockito.Mockito.spy;
 import static org.mockito.Mockito.verify;
 
@@ -31,6 +32,7 @@
 import com.yahoo.elide.core.dictionary.EntityDictionary;
 import com.yahoo.elide.core.security.checks.Check;
 import com.yahoo.elide.core.utils.DefaultClassScanner;
+
 import com.fasterxml.jackson.databind.JsonNode;
 import com.fasterxml.jackson.databind.ObjectMapper;
 import com.fasterxml.jackson.databind.node.JsonNodeFactory;
@@ -44,6 +46,9 @@
 import org.mockito.Mockito;
 import org.skyscreamer.jsonassert.JSONAssert;
 
+import graphql.execution.DataFetcherExceptionHandler;
+import graphql.execution.SimpleDataFetcherExceptionHandler;
+
 import graphqlEndpointTestModels.Author;
 import graphqlEndpointTestModels.Book;
 import graphqlEndpointTestModels.DisallowTransfer;
@@ -64,6 +69,7 @@
 import java.util.HashMap;
 import java.util.Iterator;
 import java.util.Map;
+import java.util.Optional;
 import java.util.stream.Collectors;
 import java.util.stream.Stream;
 
@@ -80,6 +86,7 @@ public class GraphQLEndpointTest {
     private final AuditLogger audit = Mockito.mock(AuditLogger.class);
     private final UriInfo uriInfo = Mockito.mock(UriInfo.class);
     private final HttpHeaders requestHeaders = Mockito.mock(HttpHeaders.class);
+    private final DataFetcherExceptionHandler dataFetcherExceptionHandler = Mockito.spy(new SimpleDataFetcherExceptionHandler());
 
     private Elide elide;
 
@@ -140,7 +147,7 @@ public void setupTest() throws Exception {
                 );
 
         elide.doScans();
-        endpoint = new GraphQLEndpoint(elide);
+        endpoint = new GraphQLEndpoint(elide, Optional.of(dataFetcherExceptionHandler));
 
         DataStoreTransaction tx = inMemoryStore.beginTransaction();
 
@@ -185,6 +192,8 @@ public void setupTest() throws Exception {
         tx.save(noShare, null);
 
         tx.commit(null);
+
+        reset(dataFetcherExceptionHandler);
     }
 
     @Test
@@ -414,6 +423,7 @@ void testFailedMutationAndRead() throws IOException, JSONException {
 
         Response response = endpoint.post(uriInfo, requestHeaders, user2, graphQLRequestToJSON(graphQLRequest));
         assertHasErrors(response);
+        verify(dataFetcherExceptionHandler).handleException(any());
 
         graphQLRequest = document(
                 selection(

diff --git a/elide-graphql/src/test/java/com/yahoo/elide/graphql/PersistentResourceFetcherTest.java b/elide-graphql/src/test/java/com/yahoo/elide/graphql/PersistentResourceFetcherTest.java
@@ -11,6 +11,8 @@
 import static org.junit.jupiter.api.Assertions.assertThrows;
 import static org.junit.jupiter.api.Assertions.assertTrue;
 import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.reset;
+import static org.mockito.Mockito.spy;
 
 import com.yahoo.elide.Elide;
 import com.yahoo.elide.ElideResponse;
@@ -42,6 +44,7 @@
 
 import graphql.GraphQL;
 import graphql.GraphQLError;
+import graphql.execution.DataFetcherExceptionHandler;
 import graphql.execution.SimpleDataFetcherExceptionHandler;
 
 import java.io.IOException;
@@ -74,6 +77,8 @@ public abstract class PersistentResourceFetcherTest extends GraphQLTest {
     protected HashMapDataStore hashMapDataStore;
     protected ElideSettings settings;
 
+    protected DataFetcherExceptionHandler dataFetcherExceptionHandler = spy(new SimpleDataFetcherExceptionHandler());
+
     @BeforeAll
     public void initializeQueryRunner() {
         RSQLFilterDialect filterDialect = RSQLFilterDialect.builder().dictionary(dictionary).build();
@@ -94,7 +99,7 @@ public void initializeQueryRunner() {
         Elide elide = new Elide(settings);
         elide.doScans();
 
-        runner = new QueryRunner(elide, NO_VERSION, new SimpleDataFetcherExceptionHandler());
+        runner = new QueryRunner(elide, NO_VERSION, dataFetcherExceptionHandler);
     }
 
     protected void initializeMocks() {
@@ -183,6 +188,8 @@ public void initTestData() {
         tx.save(publisher1, null);
         tx.save(publisher2, null);
         tx.commit(null);
+
+        reset(dataFetcherExceptionHandler);
     }
 
     protected void assertQueryEquals(String graphQLRequest, String expectedResponse) throws Exception {