Fix returning response from cache fix #289

Author: oliemansm

examples/osgi/providers/src/main/java/graphql/servlet/examples/osgi/ExampleServlet.java

@@ -52,7 +52,7 @@
 
 @Component(
     service = {javax.servlet.http.HttpServlet.class, javax.servlet.Servlet.class},
-    property = {"alias=/graphql", "service.description=GraphQL HTTP Servlet"},
+    property = {"service.description=GraphQL HTTP Servlet"},
     immediate = true
 )
 @Designate(ocd = OsgiGraphQLHttpServletConfiguration.class, factory = true)
@@ -132,12 +132,8 @@ protected void updateSchema() {
         updateFuture.cancel(true);
       }
 
-      updateFuture = executor.schedule(new Runnable() {
-        @Override
-        public void run() {
-          doUpdateSchema();
-        }
-      }, schemaUpdateDelay, TimeUnit.MILLISECONDS);
+      updateFuture = executor
+          .schedule(this::doUpdateSchema, schemaUpdateDelay, TimeUnit.MILLISECONDS);
     }
   }
 
@@ -148,17 +144,17 @@ private void doUpdateSchema() {
     if (!queryProviders.isEmpty()) {
       for (GraphQLQueryProvider provider : queryProviders) {
         if (provider.getQueries() != null && !provider.getQueries().isEmpty()) {
-            provider.getQueries().forEach(queryTypeBuilder::field);
+          provider.getQueries().forEach(queryTypeBuilder::field);
         }
       }
     } else {
       // graphql-java enforces Query type to be there with at least some field.
       queryTypeBuilder.field(
-        GraphQLFieldDefinition
-          .newFieldDefinition()
-          .name("_empty")
-          .type(Scalars.GraphQLBoolean)
-          .build());
+          GraphQLFieldDefinition
+              .newFieldDefinition()
+              .name("_empty")
+              .type(Scalars.GraphQLBoolean)
+              .build());
     }
 
     final Set<GraphQLType> types = new HashSet<>();

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

@@ -9,10 +9,7 @@
 import lombok.extern.slf4j.Slf4j;
 
 @Slf4j
-public final class CacheReader {
-
-  private CacheReader() {
-  }
+public class CacheReader {
 
   /**
    * Response from cache if possible, if nothing in cache will not produce any response
@@ -21,7 +18,7 @@ private CacheReader() {
    * found or an error occurred while reading value from cache
    * @throws IOException if can not read value from the cache
    */
-  public static boolean responseFromCache(GraphQLInvocationInput invocationInput,
+  public boolean responseFromCache(GraphQLInvocationInput invocationInput,
       HttpServletRequest request,
       HttpServletResponse response,
       GraphQLResponseCacheManager cacheManager) throws IOException {
@@ -40,7 +37,7 @@ public static boolean responseFromCache(GraphQLInvocationInput invocationInput,
     return false;
   }
 
-  private static void write(HttpServletResponse response, CachedResponse cachedResponse)
+  private void write(HttpServletResponse response, CachedResponse cachedResponse)
       throws IOException {
     if (cachedResponse.isError()) {
       response.sendError(cachedResponse.getErrorStatusCode(), cachedResponse.getErrorMessage());

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

@@ -9,38 +9,38 @@
 import java.io.IOException;
 import javax.servlet.http.HttpServletRequest;
 import javax.servlet.http.HttpServletResponse;
+import lombok.AccessLevel;
+import lombok.RequiredArgsConstructor;
 import lombok.extern.slf4j.Slf4j;
 
 @Slf4j
+@RequiredArgsConstructor(access = AccessLevel.PROTECTED)
 public class CachingHttpRequestInvoker implements HttpRequestInvoker {
 
   private final GraphQLConfiguration configuration;
   private final HttpRequestInvoker requestInvoker;
+  private final CacheReader cacheReader;
 
   public CachingHttpRequestInvoker(GraphQLConfiguration configuration) {
-    this.configuration = configuration;
-    requestInvoker = new HttpRequestInvokerImpl(configuration, configuration.getGraphQLInvoker(),
-        new CachingQueryResponseWriterFactory());
+    this(configuration, new HttpRequestInvokerImpl(configuration, configuration.getGraphQLInvoker(),
+        new CachingQueryResponseWriterFactory()), new CacheReader());
   }
 
+  /**
+   * Try to return value from cache if cache exists, otherwise process the query normally
+   */
   @Override
   public 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(
+      if (!cacheReader.responseFromCache(
           invocationInput, request, response, configuration.getResponseCacheManager()
-      );
+      )) {
+        requestInvoker.execute(invocationInput, request, response);
+      }
     } catch (IOException e) {
       response.setStatus(STATUS_BAD_REQUEST);
       log.warn("Unexpected error happened during response from cache", e);
-      return;
-    }
-
-    if (!returnedFromCache) {
-      requestInvoker.execute(invocationInput, request, response);
     }
   }
 

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

@@ -0,0 +1,56 @@
+package graphql.kickstart.servlet.cache
+
+import graphql.kickstart.execution.input.GraphQLInvocationInput
+import graphql.kickstart.servlet.GraphQLConfiguration
+import graphql.kickstart.servlet.HttpRequestInvoker
+import spock.lang.Specification
+
+import javax.servlet.http.HttpServletRequest
+import javax.servlet.http.HttpServletResponse
+
+class CachingHttpRequestInvokerTest extends Specification {
+
+    def cacheReaderMock
+    def cachingInvoker
+    def invocationInputMock
+    def requestMock
+    def responseMock
+    def responseCacheManagerMock
+    def httpRequestInvokerMock
+
+    def setup() {
+        cacheReaderMock = Mock(CacheReader)
+        invocationInputMock = Mock(GraphQLInvocationInput)
+        requestMock = Mock(HttpServletRequest)
+        responseMock = Mock(HttpServletResponse)
+        responseCacheManagerMock = Mock(GraphQLResponseCacheManager)
+        def configuration = Mock(GraphQLConfiguration)
+        httpRequestInvokerMock = Mock(HttpRequestInvoker)
+        cachingInvoker = new CachingHttpRequestInvoker(configuration, httpRequestInvokerMock, cacheReaderMock)
+
+        configuration.getResponseCacheManager() >> responseCacheManagerMock
+    }
+
+    def "should execute regular invoker if cache not exists"() {
+        given:
+        cacheReaderMock.responseFromCache(invocationInputMock, requestMock, responseMock, responseCacheManagerMock) >> false
+
+        when:
+        cachingInvoker.execute(invocationInputMock, requestMock, responseMock)
+
+        then:
+        1 * httpRequestInvokerMock.execute(invocationInputMock, requestMock, responseMock)
+    }
+
+    def "should not execute regular invoker if cache exists"() {
+        given:
+        cacheReaderMock.responseFromCache(invocationInputMock, requestMock, responseMock, responseCacheManagerMock) >> true
+
+        when:
+        cachingInvoker.execute(invocationInputMock, requestMock, responseMock)
+
+        then:
+        0 * httpRequestInvokerMock.execute(invocationInputMock, requestMock, responseMock)
+    }
+
+}