Code review fixes.

Author: donbeave

graphql-java-servlet/src/main/java/graphql/kickstart/servlet/AbstractGraphQLHttpServlet.java

@@ -7,6 +7,7 @@
 import graphql.kickstart.execution.GraphQLQueryInvoker;
 import graphql.kickstart.execution.GraphQLRequest;
 import graphql.kickstart.execution.input.GraphQLSingleInvocationInput;
+import graphql.kickstart.servlet.cache.CachingHttpRequestHandlerImpl;
 import graphql.schema.GraphQLFieldDefinition;
 import graphql.kickstart.servlet.core.GraphQLMBean;
 import graphql.kickstart.servlet.core.GraphQLServletListener;
@@ -83,7 +84,11 @@ protected GraphQLConfiguration getConfiguration() {
   public void init() {
     if (configuration == null) {
       this.configuration = getConfiguration();
-      this.requestHandler = new HttpRequestHandlerImpl(configuration);
+      if (configuration.getResponseCacheManager() != null) {
+        this.requestHandler = new CachingHttpRequestHandlerImpl(configuration);
+      } else {
+        this.requestHandler = new HttpRequestHandlerImpl(configuration);
+      }
     }
   }
 

graphql-java-servlet/src/main/java/graphql/kickstart/servlet/HttpRequestHandlerImpl.java

@@ -3,25 +3,23 @@
 import graphql.GraphQLException;
 import graphql.kickstart.execution.GraphQLInvoker;
 import graphql.kickstart.execution.GraphQLQueryResult;
+import graphql.kickstart.servlet.input.BatchInputPreProcessResult;
+import graphql.kickstart.servlet.input.BatchInputPreProcessor;
 import graphql.kickstart.execution.input.GraphQLBatchedInvocationInput;
 import graphql.kickstart.execution.input.GraphQLInvocationInput;
 import graphql.kickstart.execution.input.GraphQLSingleInvocationInput;
-import graphql.kickstart.servlet.cache.CacheReader;
-import graphql.kickstart.servlet.input.BatchInputPreProcessResult;
-import graphql.kickstart.servlet.input.BatchInputPreProcessor;
-import lombok.extern.slf4j.Slf4j;
-
+import java.io.IOException;
 import javax.servlet.http.HttpServletRequest;
 import javax.servlet.http.HttpServletResponse;
-import java.io.IOException;
+import lombok.extern.slf4j.Slf4j;
 
 @Slf4j
-class HttpRequestHandlerImpl implements HttpRequestHandler {
+public class HttpRequestHandlerImpl implements HttpRequestHandler {
 
   private final GraphQLConfiguration configuration;
   private final GraphQLInvoker graphQLInvoker;
 
-  HttpRequestHandlerImpl(GraphQLConfiguration configuration) {
+  public HttpRequestHandlerImpl(GraphQLConfiguration configuration) {
     this.configuration = configuration;
     graphQLInvoker = configuration.getGraphQLInvoker();
   }
@@ -48,32 +46,25 @@ public void handle(HttpServletRequest request, HttpServletResponse response) thr
     }
   }
 
-  private void execute(GraphQLInvocationInput invocationInput, HttpServletRequest request,
+  protected void execute(GraphQLInvocationInput invocationInput, HttpServletRequest request,
       HttpServletResponse response) {
     try {
-      // try to return value from cache if cache manager was set, otherwise processed the query
-      boolean returnedFromCache = configuration.getResponseCacheManager() != null &&
-              !CacheReader.responseFromCache(invocationInput, request, response, configuration.getResponseCacheManager());
+      GraphQLQueryResult queryResult = invoke(invocationInput, request, response);
 
-      if (!returnedFromCache) {
-        GraphQLQueryResult queryResult = invoke(invocationInput, request, response);
-
-        QueryResponseWriter queryResponseWriter = QueryResponseWriter.createWriter(
-                queryResult,
-                configuration.getObjectMapper(),
-                configuration.getSubscriptionTimeout(),
-                invocationInput,
-                configuration.getResponseCacheManager()
-        );
-        queryResponseWriter.write(request, response);
-      }
+      QueryResponseWriter queryResponseWriter = createWriter(invocationInput, queryResult);
+      queryResponseWriter.write(request, response);
     } catch (Throwable t) {
       response.setStatus(STATUS_BAD_REQUEST);
       log.info("Bad GET request: path was not \"/schema.json\" or no query variable named \"query\" given");
       log.debug("Possibly due to exception: ", t);
     }
   }
 
+  protected QueryResponseWriter createWriter(GraphQLInvocationInput invocationInput, GraphQLQueryResult queryResult) {
+    return QueryResponseWriter.createWriter(queryResult, configuration.getObjectMapper(),
+            configuration.getSubscriptionTimeout());
+  }
+
   private GraphQLQueryResult invoke(GraphQLInvocationInput invocationInput, HttpServletRequest request,
       HttpServletResponse response) {
     if (invocationInput instanceof GraphQLSingleInvocationInput) {

graphql-java-servlet/src/main/java/graphql/kickstart/servlet/QueryResponseWriter.java

@@ -1,41 +1,45 @@
 package graphql.kickstart.servlet;
 
-import graphql.kickstart.execution.GraphQLQueryResult;
 import graphql.kickstart.execution.GraphQLObjectMapper;
+import graphql.kickstart.execution.GraphQLQueryResult;
 import graphql.kickstart.execution.input.GraphQLInvocationInput;
 import graphql.kickstart.servlet.cache.CachingQueryResponseWriter;
 import graphql.kickstart.servlet.cache.GraphQLResponseCacheManager;
 
-import java.io.IOException;
-import java.util.Objects;
 import javax.servlet.http.HttpServletRequest;
 import javax.servlet.http.HttpServletResponse;
+import java.io.IOException;
+import java.util.Objects;
 
 public interface QueryResponseWriter {
 
   static QueryResponseWriter createWriter(
       GraphQLQueryResult result,
       GraphQLObjectMapper graphQLObjectMapper,
-      long subscriptionTimeout,
-      GraphQLInvocationInput invocationInput,
-      GraphQLResponseCacheManager responseCache
+      long subscriptionTimeout
   ) {
     Objects.requireNonNull(result, "GraphQL query result cannot be null");
 
-    QueryResponseWriter writer;
-
     if (result.isBatched()) {
-      writer = new BatchedQueryResponseWriter(result.getResults(), graphQLObjectMapper);
+      return new BatchedQueryResponseWriter(result.getResults(), graphQLObjectMapper);
     } else if (result.isAsynchronous()) {
-      writer = new SingleAsynchronousQueryResponseWriter(result.getResult(), graphQLObjectMapper, subscriptionTimeout);
+      return new SingleAsynchronousQueryResponseWriter(result.getResult(), graphQLObjectMapper, subscriptionTimeout);
     } else if (result.isError()) {
-      writer = new ErrorQueryResponseWriter(result.getStatusCode(), result.getMessage());
-    } else {
-      writer = new SingleQueryResponseWriter(result.getResult(), graphQLObjectMapper);
+      return new ErrorQueryResponseWriter(result.getStatusCode(), result.getMessage());
     }
+    return new SingleQueryResponseWriter(result.getResult(), graphQLObjectMapper);
+  }
 
+  static QueryResponseWriter createCacheWriter(
+      GraphQLQueryResult result,
+      GraphQLObjectMapper graphQLObjectMapper,
+      long subscriptionTimeout,
+      GraphQLInvocationInput invocationInput,
+      GraphQLResponseCacheManager responseCache
+  ) {
+    QueryResponseWriter writer = createWriter(result, graphQLObjectMapper, subscriptionTimeout);
     if (responseCache != null) {
-      writer = new CachingQueryResponseWriter(writer, responseCache, invocationInput, result.isError());
+      return new CachingQueryResponseWriter(writer, responseCache, invocationInput, result.isError());
     }
     return writer;
   }

graphql-java-servlet/src/main/java/graphql/kickstart/servlet/cache/CacheReader.java

@@ -23,9 +23,9 @@ private CacheReader() {
    * @throws IOException if can not read value from the cache
    */
   public static boolean responseFromCache(GraphQLInvocationInput invocationInput,
-                      HttpServletRequest request,
-                      HttpServletResponse response,
-                      GraphQLResponseCacheManager cacheManager) throws IOException {
+                                          HttpServletRequest request,
+                                          HttpServletResponse response,
+                                          GraphQLResponseCacheManager cacheManager) throws IOException {
     CachedResponse cachedResponse = null;
     try {
       cachedResponse = cacheManager.get(request, invocationInput);
@@ -42,7 +42,7 @@ public static boolean responseFromCache(GraphQLInvocationInput invocationInput,
   }
 
   private static void write(HttpServletResponse response, CachedResponse cachedResponse)
-          throws IOException {
+      throws IOException {
     if (cachedResponse.isError()) {
       response.sendError(cachedResponse.getErrorStatusCode(), cachedResponse.getErrorMessage());
     } else {

graphql-java-servlet/src/main/java/graphql/kickstart/servlet/cache/CachingHttpRequestHandlerImpl.java

@@ -0,0 +1,56 @@
+package graphql.kickstart.servlet.cache;
+
+import graphql.kickstart.execution.GraphQLQueryResult;
+import graphql.kickstart.execution.input.GraphQLInvocationInput;
+import graphql.kickstart.servlet.GraphQLConfiguration;
+import graphql.kickstart.servlet.HttpRequestHandlerImpl;
+import graphql.kickstart.servlet.QueryResponseWriter;
+import lombok.extern.slf4j.Slf4j;
+
+import javax.servlet.http.HttpServletRequest;
+import javax.servlet.http.HttpServletResponse;
+import java.io.IOException;
+import java.util.Objects;
+
+@Slf4j
+public class CachingHttpRequestHandlerImpl extends HttpRequestHandlerImpl {
+
+  private final GraphQLConfiguration configuration;
+
+  public CachingHttpRequestHandlerImpl(GraphQLConfiguration configuration) {
+    super(configuration);
+    Objects.requireNonNull(configuration.getResponseCacheManager(), "Response Cache Manager cannot be null");
+    this.configuration = configuration;
+  }
+
+  protected void execute(GraphQLInvocationInput invocationInput, HttpServletRequest request,
+               HttpServletResponse response) {
+    // try to return value from cache if cache exists, otherwise processed the query
+    boolean returnedFromCache;
+
+    try {
+      returnedFromCache = !CacheReader.responseFromCache(
+          invocationInput, request, response, configuration.getResponseCacheManager()
+      );
+    } catch (IOException e) {
+      response.setStatus(STATUS_BAD_REQUEST);
+      log.warn("unexpected error happened during response from cache", e);
+      return;
+    }
+
+    if (!returnedFromCache) {
+      super.execute(invocationInput, request, response);
+    }
+  }
+
+  protected QueryResponseWriter createWriter(GraphQLInvocationInput invocationInput, GraphQLQueryResult queryResult) {
+    return QueryResponseWriter.createCacheWriter(
+        queryResult,
+        configuration.getObjectMapper(),
+        configuration.getSubscriptionTimeout(),
+        invocationInput,
+        configuration.getResponseCacheManager()
+    );
+  }
+
+}