Merge pull request #290 from graphql-java-kickstart/bugfix/response-from-cache

oliemansm · web-flow · commit 7b58f8809f4e · 2020-12-26T19:01:39.000+01:00
Bugfix/response from cache
diff --git a/.github/workflows/pull-request.yml b/.github/workflows/pull-request.yml
@@ -1,8 +1,5 @@
 name: "Pull request"
 on:
-  push:
-    branches-ignore:
-      - master
   pull_request:
     types: [opened, synchronize, reopened]
 jobs:
diff --git a/.gitignore b/.gitignore
@@ -3,11 +3,12 @@ build/
 *.iml
 *.ipr
 *.iws
-.idea/
+**/.idea/
 target/
 /out/
 .classpath
 .project
 .settings
 bin
 .DS_Store
+/**/out/
diff --git a/examples/osgi/apache-karaf-feature/pom.xml b/examples/osgi/apache-karaf-feature/pom.xml
@@ -14,7 +14,7 @@
         </configuration>
         <extensions>true</extensions>
         <groupId>org.apache.karaf.tooling</groupId>
-        <version>4.0.8</version>
+        <version>4.2.10</version>
       </plugin>
 
       <plugin>
@@ -108,7 +108,7 @@
   <parent>
     <artifactId>graphql-java-servlet-osgi-examples</artifactId>
     <groupId>com.graphql-java-kickstart</groupId>
-    <version>7.3.4-SNAPSHOT</version>
+    <version>10.1.0</version>
   </parent>
 
   <properties>
diff --git a/examples/osgi/apache-karaf-feature/src/main/feature/feature.xml b/examples/osgi/apache-karaf-feature/src/main/feature/feature.xml
@@ -5,5 +5,6 @@
     name="graphql-java-servlet-osgi-examples-karaf-feature">
     <feature dependency="false" prerequisite="true">scr</feature>
     <feature dependency="false" prerequisite="true">war</feature>
+    <feature dependency="false" prerequisite="true">http</feature>
   </feature>
 </features>
diff --git a/examples/osgi/apache-karaf-package/pom.xml b/examples/osgi/apache-karaf-package/pom.xml
@@ -110,6 +110,6 @@
   <parent>
     <artifactId>graphql-java-servlet-osgi-examples</artifactId>
     <groupId>com.graphql-java-kickstart</groupId>
-    <version>7.3.4-SNAPSHOT</version>
+    <version>10.1.0</version>
   </parent>
 </project>
diff --git a/examples/osgi/buildAndRun.sh b/examples/osgi/buildAndRun.sh
@@ -1,7 +1,7 @@
 #!/usr/bin/env bash
 mvn clean install
-pushd apache-karaf-package/target
-tar zxvf graphql-java-servlet-osgi-examples-apache-karaf-package-7.3.4-SNAPSHOT.tar.gz
-cd graphql-java-servlet-osgi-examples-apache-karaf-package-7.3.4-SNAPSHOT/bin
+pushd apache-karaf-package/target || exit 1
+tar zxvf graphql-java-servlet-osgi-examples-apache-karaf-package-10.1.0.tar.gz
+cd graphql-java-servlet-osgi-examples-apache-karaf-package-10.1.0/bin || exit 1
 ./karaf debug
-popd
+popd || exit 1
diff --git a/examples/osgi/pom.xml b/examples/osgi/pom.xml
@@ -14,11 +14,13 @@
   <packaging>pom</packaging>
 
   <properties>
-    <graphql.java.servlet.version>7.3.4-SNAPSHOT</graphql.java.servlet.version>
-    <graphql.java.version>11.0</graphql.java.version>
-    <karaf.version>4.2.4</karaf.version>
+    <graphql.java.servlet.version>11.0.0-SNAPSHOT</graphql.java.servlet.version>
+    <graphql.java.version>16.1</graphql.java.version>
+    <karaf.version>4.2.10</karaf.version>
+    <maven.compiler.source>1.8</maven.compiler.source>
+    <maven.compiler.target>1.8</maven.compiler.target>
   </properties>
 
-  <version>7.3.4-SNAPSHOT</version>
+  <version>10.1.0</version>
 
 </project>
diff --git a/examples/osgi/providers/pom.xml b/examples/osgi/providers/pom.xml
@@ -8,8 +8,6 @@
       <plugin>
         <artifactId>maven-bundle-plugin</artifactId>
         <configuration>
-          <instructions>
-          </instructions>
         </configuration>
         <extensions>true</extensions>
         <groupId>org.apache.felix</groupId>
@@ -51,7 +49,7 @@
   <parent>
     <artifactId>graphql-java-servlet-osgi-examples</artifactId>
     <groupId>com.graphql-java-kickstart</groupId>
-    <version>7.3.4-SNAPSHOT</version>
+    <version>10.1.0</version>
   </parent>
 
 </project>
diff --git a/examples/osgi/providers/src/main/java/graphql/servlet/examples/osgi/ExampleGraphQLProvider.java b/examples/osgi/providers/src/main/java/graphql/servlet/examples/osgi/ExampleGraphQLProvider.java
@@ -1,10 +1,10 @@
-package graphql.kickstart.servlet.examples.osgi;
+package graphql.servlet.examples.osgi;
 
 import graphql.schema.GraphQLFieldDefinition;
 import graphql.schema.GraphQLType;
-import graphql.servlet.GraphQLMutationProvider;
-import graphql.servlet.GraphQLQueryProvider;
-import graphql.servlet.GraphQLTypesProvider;
+import graphql.kickstart.servlet.osgi.GraphQLMutationProvider;
+import graphql.kickstart.servlet.osgi.GraphQLQueryProvider;
+import graphql.kickstart.servlet.osgi.GraphQLTypesProvider;
 import org.osgi.service.component.annotations.Component;
 
 import java.util.ArrayList;
@@ -38,8 +38,6 @@ public Collection<GraphQLFieldDefinition> getMutations() {
   }
 
   public Collection<GraphQLType> getTypes() {
-
-    List<GraphQLType> types = new ArrayList<GraphQLType>();
-    return types;
+    return new ArrayList<GraphQLType>();
   }
 }
diff --git a/graphql-java-servlet/src/main/java/graphql/kickstart/servlet/OsgiGraphQLHttpServlet.java b/graphql-java-servlet/src/main/java/graphql/kickstart/servlet/OsgiGraphQLHttpServlet.java
@@ -131,12 +131,7 @@ 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);
     }
   }
 
@@ -147,17 +142,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<>();
diff --git a/graphql-java-servlet/src/main/java/graphql/kickstart/servlet/cache/CacheReader.java b/graphql-java-servlet/src/main/java/graphql/kickstart/servlet/cache/CacheReader.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,26 +18,24 @@ 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 {
-    CachedResponse cachedResponse = null;
     try {
-      cachedResponse = cacheManager.get(request, invocationInput);
+      CachedResponse cachedResponse = cacheManager.get(request, invocationInput);
+      if (cachedResponse != null) {
+        write(response, cachedResponse);
+        return true;
+      }
     } catch (Exception t) {
       log.warn("Ignore read from cache, unexpected error happened", t);
     }
 
-    if (cachedResponse != null) {
-      write(response, cachedResponse);
-      return true;
-    }
-
     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());
diff --git a/graphql-java-servlet/src/main/java/graphql/kickstart/servlet/cache/CachingHttpRequestInvoker.java b/graphql-java-servlet/src/main/java/graphql/kickstart/servlet/cache/CachingHttpRequestInvoker.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);
     }
   }
 
diff --git a/graphql-java-servlet/src/test/groovy/graphql/kickstart/servlet/cache/CacheReaderTest.groovy b/graphql-java-servlet/src/test/groovy/graphql/kickstart/servlet/cache/CacheReaderTest.groovy
@@ -0,0 +1,84 @@
+package graphql.kickstart.servlet.cache
+
+import graphql.kickstart.execution.input.GraphQLInvocationInput
+import spock.lang.Specification
+
+import javax.servlet.ServletOutputStream
+import javax.servlet.http.HttpServletRequest
+import javax.servlet.http.HttpServletResponse
+
+class CacheReaderTest extends Specification {
+
+    def cacheManager
+    def invocationInput
+    def request
+    def response
+    def cacheReader
+    def cachedResponse
+
+    def setup() {
+        cacheManager = Mock(GraphQLResponseCacheManager)
+        invocationInput = Mock(GraphQLInvocationInput)
+        request = Mock(HttpServletRequest)
+        response = Mock(HttpServletResponse)
+        cacheReader = new CacheReader()
+        cachedResponse = Mock(CachedResponse)
+    }
+
+    def "should return false if no cached response"() {
+        given:
+        cacheManager.get(request, invocationInput) >> null
+
+        when:
+        def result = cacheReader.responseFromCache(invocationInput, request, response, cacheManager)
+
+        then:
+        !result
+    }
+
+    def "should send error response if cached response is error"() {
+        given:
+        cachedResponse.isError() >> true
+        cachedResponse.getErrorStatusCode() >> 10
+        cachedResponse.getErrorMessage() >> "some error"
+        cacheManager.get(request, invocationInput) >> cachedResponse
+
+        when:
+        def result = cacheReader.responseFromCache(invocationInput, request, response, cacheManager)
+
+        then:
+        result
+        1 * response.sendError(10, "some error")
+    }
+
+    def "should send success response if cached response is ok"() {
+        given:
+        def outputStream = Mock(ServletOutputStream)
+        cachedResponse.isError() >> false
+        cachedResponse.getContentBytes() >> [ 00, 01, 02 ]
+        response.getOutputStream() >> outputStream
+        cacheManager.get(request, invocationInput) >> cachedResponse
+
+        when:
+        def result = cacheReader.responseFromCache(invocationInput, request, response, cacheManager)
+
+        then:
+        result
+        1 * response.setContentType("application/json;charset=UTF-8")
+        1 * response.setStatus(200)
+        1 * response.setCharacterEncoding("UTF-8")
+        1 * response.setContentLength(3)
+        1 * outputStream.write([ 00, 01, 02 ])
+    }
+
+    def "should return false if exception is thrown"() {
+        given:
+        cacheManager.get(request, invocationInput) >> {throw new RuntimeException()}
+
+        when:
+        def result = cacheReader.responseFromCache(invocationInput, request, response, cacheManager)
+
+        then:
+        !result
+    }
+}
diff --git a/graphql-java-servlet/src/test/groovy/graphql/kickstart/servlet/cache/CachingHttpRequestInvokerTest.groovy b/graphql-java-servlet/src/test/groovy/graphql/kickstart/servlet/cache/CachingHttpRequestInvokerTest.groovy

Original file line number	Diff line number	Diff line change
`@@ -131,12 +131,7 @@ protected void updateSchema() {`
`131`	`131`	`updateFuture.cancel(true);`
`132`	`132`	`}`
`133`	`133`
`134`		`- updateFuture = executor.schedule(new Runnable() {`
`135`		`- @Override`
`136`		`- public void run() {`
`137`		`- doUpdateSchema();`
`138`		`- }`
`139`		`- }, schemaUpdateDelay, TimeUnit.MILLISECONDS);`
	`134`	`+ updateFuture = executor.schedule(this::doUpdateSchema, schemaUpdateDelay, TimeUnit.MILLISECONDS);`
`140`	`135`	`}`
`141`	`136`	`}`
`142`	`137`
`@@ -147,17 +142,17 @@ private void doUpdateSchema() {`
`147`	`142`	`if (!queryProviders.isEmpty()) {`
`148`	`143`	`for (GraphQLQueryProvider provider : queryProviders) {`
`149`	`144`	`if (provider.getQueries() != null && !provider.getQueries().isEmpty()) {`
`150`		`- provider.getQueries().forEach(queryTypeBuilder::field);`
	`145`	`+ provider.getQueries().forEach(queryTypeBuilder::field);`
`151`	`146`	`}`
`152`	`147`	`}`
`153`	`148`	`} else {`
`154`	`149`	`// graphql-java enforces Query type to be there with at least some field.`
`155`	`150`	`queryTypeBuilder.field(`
`156`		`- GraphQLFieldDefinition`
`157`		`- .newFieldDefinition()`
`158`		`- .name("_empty")`
`159`		`- .type(Scalars.GraphQLBoolean)`
`160`		`- .build());`
	`151`	`+ GraphQLFieldDefinition`
	`152`	`+ .newFieldDefinition()`
	`153`	`+ .name("_empty")`
	`154`	`+ .type(Scalars.GraphQLBoolean)`
	`155`	`+ .build());`
`161`	`156`	`}`
`162`	`157`
`163`	`158`	`final Set<GraphQLType> types = new HashSet<>();`