From fdd8cb5a6c226af3d56be85ee601f1089e5bff26 Mon Sep 17 00:00:00 2001 From: oliemansm Date: Thu, 13 May 2021 11:08:07 +0200 Subject: [PATCH 1/6] fix(#469): support async timeout with proper error --- build.gradle | 4 - .../kickstart/execution/GraphQLInvoker.java | 2 + .../servlet/AsyncTimeoutListener.java | 14 ++++ .../servlet/HttpRequestInvokerImpl.java | 80 +++++++++++++------ .../servlet/SingleQueryResponseWriter.java | 1 + 5 files changed, 73 insertions(+), 28 deletions(-) create mode 100644 graphql-java-servlet/src/main/java/graphql/kickstart/servlet/AsyncTimeoutListener.java diff --git a/build.gradle b/build.gradle index a0936f5c..9639c85f 100644 --- a/build.gradle +++ b/build.gradle @@ -31,7 +31,6 @@ plugins { id "org.sonarqube" version "3.2.0" id "jacoco" id 'io.codearte.nexus-staging' version '0.30.0' - id 'com.github.sherter.google-java-format' version '0.9' apply false } sonarqube { @@ -49,7 +48,6 @@ subprojects { apply plugin: 'java' apply plugin: 'maven-publish' apply plugin: 'signing' - apply plugin: 'com.github.sherter.google-java-format' repositories { mavenLocal() @@ -80,8 +78,6 @@ subprojects { compileJava.dependsOn(processResources) - compileJava.mustRunAfter verifyGoogleJavaFormat - test { useJUnitPlatform() diff --git a/graphql-java-kickstart/src/main/java/graphql/kickstart/execution/GraphQLInvoker.java b/graphql-java-kickstart/src/main/java/graphql/kickstart/execution/GraphQLInvoker.java index 435fb260..ca8fb832 100644 --- a/graphql-java-kickstart/src/main/java/graphql/kickstart/execution/GraphQLInvoker.java +++ b/graphql-java-kickstart/src/main/java/graphql/kickstart/execution/GraphQLInvoker.java @@ -12,7 +12,9 @@ import java.util.List; import java.util.concurrent.CompletableFuture; import lombok.RequiredArgsConstructor; +import lombok.extern.slf4j.Slf4j; +@Slf4j @RequiredArgsConstructor public class GraphQLInvoker { diff --git a/graphql-java-servlet/src/main/java/graphql/kickstart/servlet/AsyncTimeoutListener.java b/graphql-java-servlet/src/main/java/graphql/kickstart/servlet/AsyncTimeoutListener.java new file mode 100644 index 00000000..7d39ad89 --- /dev/null +++ b/graphql-java-servlet/src/main/java/graphql/kickstart/servlet/AsyncTimeoutListener.java @@ -0,0 +1,14 @@ +package graphql.kickstart.servlet; + +import java.io.IOException; +import javax.servlet.AsyncEvent; +import javax.servlet.AsyncListener; + +interface AsyncTimeoutListener extends AsyncListener { + + default void onComplete(AsyncEvent event) throws IOException {} + + default void onError(AsyncEvent event) throws IOException {} + + default void onStartAsync(AsyncEvent event) throws IOException {} +} diff --git a/graphql-java-servlet/src/main/java/graphql/kickstart/servlet/HttpRequestInvokerImpl.java b/graphql-java-servlet/src/main/java/graphql/kickstart/servlet/HttpRequestInvokerImpl.java index cc1b39ec..4316827a 100644 --- a/graphql-java-servlet/src/main/java/graphql/kickstart/servlet/HttpRequestInvokerImpl.java +++ b/graphql-java-servlet/src/main/java/graphql/kickstart/servlet/HttpRequestInvokerImpl.java @@ -2,8 +2,10 @@ import static graphql.kickstart.servlet.HttpRequestHandler.STATUS_BAD_REQUEST; +import graphql.ExecutionResultImpl; import graphql.kickstart.execution.GraphQLInvoker; import graphql.kickstart.execution.GraphQLQueryResult; +import graphql.kickstart.execution.error.GenericGraphQLError; import graphql.kickstart.execution.input.GraphQLBatchedInvocationInput; import graphql.kickstart.execution.input.GraphQLInvocationInput; import graphql.kickstart.execution.input.GraphQLSingleInvocationInput; @@ -11,7 +13,9 @@ import graphql.kickstart.servlet.input.BatchInputPreProcessor; import java.io.IOException; import java.io.UncheckedIOException; +import java.util.Optional; import java.util.concurrent.CompletableFuture; +import java.util.concurrent.atomic.AtomicReference; import javax.servlet.AsyncContext; import javax.servlet.http.HttpServletRequest; import javax.servlet.http.HttpServletResponse; @@ -31,43 +35,65 @@ public void execute( GraphQLInvocationInput invocationInput, HttpServletRequest request, HttpServletResponse response) { + ListenerHandler listenerHandler = + ListenerHandler.start(request, response, configuration.getListeners()); if (request.isAsyncSupported()) { - AsyncContext asyncContext = - request.isAsyncStarted() - ? request.getAsyncContext() - : request.startAsync(request, response); - asyncContext.setTimeout(configuration.getAsyncTimeout()); - invokeAndHandle(invocationInput, request, response) - .thenAccept(aVoid -> asyncContext.complete()); + invokeAndHandleAsync(invocationInput, request, response, listenerHandler); } else { - invokeAndHandle(invocationInput, request, response).join(); + invokeAndHandle(invocationInput, request, response, listenerHandler).join(); } } + private void invokeAndHandleAsync( + GraphQLInvocationInput invocationInput, + HttpServletRequest request, + HttpServletResponse response, + ListenerHandler listenerHandler) { + AsyncContext asyncContext = + request.isAsyncStarted() + ? request.getAsyncContext() + : request.startAsync(request, response); + asyncContext.setTimeout(configuration.getAsyncTimeout()); + AtomicReference> futureHolder = new AtomicReference<>(); + AsyncTimeoutListener timeoutListener = + event -> { + Optional.ofNullable(futureHolder.get()).ifPresent(it -> it.cancel(true)); + writeResultResponse( + invocationInput, + GraphQLQueryResult.create( + new ExecutionResultImpl(new GenericGraphQLError("Timeout"))), + (HttpServletRequest) event.getAsyncContext().getRequest(), + (HttpServletResponse) event.getAsyncContext().getResponse()); + listenerHandler.onError(event.getThrowable()); + }; + asyncContext.addListener(timeoutListener); + asyncContext.start( + () -> + futureHolder.set( + invokeAndHandle(invocationInput, request, response, listenerHandler) + .thenAccept(aVoid -> asyncContext.complete()))); + } + private CompletableFuture invokeAndHandle( GraphQLInvocationInput invocationInput, HttpServletRequest request, - HttpServletResponse response) { - ListenerHandler listenerHandler = - ListenerHandler.start(request, response, configuration.getListeners()); + HttpServletResponse response, + ListenerHandler listenerHandler) { return invoke(invocationInput, request, response) - .thenAccept( - result -> - writeResultResponse(invocationInput, result, request, response, listenerHandler)) - .exceptionally(t -> writeErrorResponse(t, response, listenerHandler)) - .thenAccept(aVoid -> listenerHandler.onFinally()); + .thenAccept(it -> writeResultResponse(invocationInput, it, request, response)) + .thenAccept(it -> listenerHandler.onSuccess()) + .exceptionally(t -> writeBadRequestError(t, response, listenerHandler)) + .thenAccept(it -> listenerHandler.onFinally()); } private void writeResultResponse( GraphQLInvocationInput invocationInput, GraphQLQueryResult queryResult, HttpServletRequest request, - HttpServletResponse response, - ListenerHandler listenerHandler) { + HttpServletResponse response) { QueryResponseWriter queryResponseWriter = createWriter(invocationInput, queryResult); try { queryResponseWriter.write(request, response); - listenerHandler.onSuccess(); } catch (IOException e) { throw new UncheckedIOException(e); } @@ -78,12 +104,18 @@ protected QueryResponseWriter createWriter( return queryResponseWriterFactory.createWriter(invocationInput, queryResult, configuration); } - private Void writeErrorResponse( + private Void writeBadRequestError( Throwable t, HttpServletResponse response, ListenerHandler listenerHandler) { - response.setStatus(STATUS_BAD_REQUEST); - log.info( - "Bad request: path was not \"/schema.json\" or no query variable named \"query\" given", t); - listenerHandler.onError(t); + if (!response.isCommitted()) { + response.setStatus(STATUS_BAD_REQUEST); + log.info( + "Bad request: path was not \"/schema.json\" or no query variable named \"query\" given", + t); + listenerHandler.onError(t); + } else { + log.warn( + "Cannot write GraphQL response, because the HTTP response is already committed. It most likely timed out."); + } return null; } diff --git a/graphql-java-servlet/src/main/java/graphql/kickstart/servlet/SingleQueryResponseWriter.java b/graphql-java-servlet/src/main/java/graphql/kickstart/servlet/SingleQueryResponseWriter.java index 86af54df..86568d29 100644 --- a/graphql-java-servlet/src/main/java/graphql/kickstart/servlet/SingleQueryResponseWriter.java +++ b/graphql-java-servlet/src/main/java/graphql/kickstart/servlet/SingleQueryResponseWriter.java @@ -23,5 +23,6 @@ public void write(HttpServletRequest request, HttpServletResponse response) thro byte[] contentBytes = graphQLObjectMapper.serializeResultAsBytes(result); response.setContentLength(contentBytes.length); response.getOutputStream().write(contentBytes); + response.getOutputStream().flush(); } } From 9de944bf5e25cc0e2768d06d5a2e8899addca260 Mon Sep 17 00:00:00 2001 From: oliemansm Date: Thu, 13 May 2021 11:09:15 +0200 Subject: [PATCH 2/6] chore: remove google code format from build --- .github/workflows/pull-request.yml | 33 +++--------------------------- .github/workflows/release.yml | 2 +- .github/workflows/snapshot.yml | 33 +++--------------------------- github-build.sh | 2 +- 4 files changed, 8 insertions(+), 62 deletions(-) diff --git a/.github/workflows/pull-request.yml b/.github/workflows/pull-request.yml index dd09d092..6d2cae3b 100644 --- a/.github/workflows/pull-request.yml +++ b/.github/workflows/pull-request.yml @@ -11,33 +11,6 @@ jobs: - uses: actions/checkout@v2 - uses: gradle/wrapper-validation-action@v1 - verify-google-java-format: - name: Google Java Format Verification - runs-on: ubuntu-latest - needs: validation - steps: - - name: Checkout - uses: actions/checkout@v2 - - name: Setup Java - uses: actions/setup-java@v2 - with: - distribution: 'zulu' - java-version: 15 - - name: Cache Gradle - uses: actions/cache@v2 - env: - java-version: 15 - with: - path: | - ~/.gradle/caches - ~/.gradle/wrapper - key: ${{ runner.os }}-${{ env.java-version }}-gradle-${{ hashFiles('**/*.gradle*') }} - restore-keys: ${{ runner.os }}-${{ env.java-version }}-gradle- - - name: Make gradlew executable - run: chmod +x ./gradlew - - name: Gradle Check - run: ./gradlew --info build -x test - test: name: Test run strategy: @@ -70,11 +43,11 @@ jobs: run: chmod +x ./gradlew - name: Gradle Check (non-Windows) if: matrix.os != 'windows-latest' - run: ./gradlew --info check -x verifyGoogleJavaFormat + run: ./gradlew --info check - name: Gradle Check (Windows) if: matrix.os == 'windows-latest' shell: cmd - run: gradlew --info check -x verifyGoogleJavaFormat + run: gradlew --info check build: name: Sonar analysis @@ -112,4 +85,4 @@ jobs: env: GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }} # Needed to get PR information, if any SONAR_TOKEN: ${{ secrets.SONAR_TOKEN }} - run: ./gradlew build jacocoTestReport sonarqube --info -x verifyGoogleJavaFormat + run: ./gradlew build jacocoTestReport sonarqube --info diff --git a/.github/workflows/release.yml b/.github/workflows/release.yml index 407451bf..f2fd3bde 100644 --- a/.github/workflows/release.yml +++ b/.github/workflows/release.yml @@ -35,7 +35,7 @@ jobs: - name: Make gradlew executable run: chmod +x ./gradlew - name: Gradle Check - run: ./gradlew --info check -x verifyGoogleJavaFormat + run: ./gradlew --info check build: name: Publish release diff --git a/.github/workflows/snapshot.yml b/.github/workflows/snapshot.yml index d89b53ae..b08ccb1c 100644 --- a/.github/workflows/snapshot.yml +++ b/.github/workflows/snapshot.yml @@ -12,33 +12,6 @@ jobs: - uses: actions/checkout@v2 - uses: gradle/wrapper-validation-action@v1 - verify-google-java-format: - name: Google Java Format Verification - runs-on: ubuntu-latest - needs: validation - steps: - - name: Checkout - uses: actions/checkout@v2 - - name: Setup Java - uses: actions/setup-java@v2 - with: - distribution: 'zulu' - java-version: 15 - - name: Cache Gradle - uses: actions/cache@v2 - env: - java-version: 15 - with: - path: | - ~/.gradle/caches - ~/.gradle/wrapper - key: ${{ runner.os }}-${{ env.java-version }}-gradle-${{ hashFiles('**/*.gradle*') }} - restore-keys: ${{ runner.os }}-${{ env.java-version }}-gradle- - - name: Make gradlew executable - run: chmod +x ./gradlew - - name: Gradle Check - run: ./gradlew --info build -x test - test: name: Test run needs: validation @@ -65,7 +38,7 @@ jobs: - name: Make gradlew executable run: chmod +x ./gradlew - name: Gradle Check - run: ./gradlew --info check -x verifyGoogleJavaFormat + run: ./gradlew --info check build: name: Publish snapshot @@ -97,7 +70,7 @@ jobs: env: OSS_USER_TOKEN_KEY: ${{ secrets.OSS_USER_TOKEN_KEY }} OSS_USER_TOKEN_PASS: ${{ secrets.OSS_USER_TOKEN_PASS }} - run: ./gradlew clean build publish -x test -x verifyGoogleJavaFormat + run: ./gradlew clean build publish -x test sonar: name: Sonar analysis @@ -129,4 +102,4 @@ jobs: GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }} # Needed to get PR information, if any SONAR_TOKEN: ${{ secrets.SONAR_TOKEN }} if: env.SONAR_TOKEN != null - run: ./gradlew build jacocoTestReport sonarqube --info -x verifyGoogleJavaFormat + run: ./gradlew build jacocoTestReport sonarqube --info diff --git a/github-build.sh b/github-build.sh index 6e9d498f..cfec06b2 100644 --- a/github-build.sh +++ b/github-build.sh @@ -45,7 +45,7 @@ git config --global user.name "GitHub Actions" echo "Deploying release to Maven Central" removeSnapshots -./gradlew clean build publish closeAndReleaseRepository -x verifyGoogleJavaFormat +./gradlew clean build publish closeAndReleaseRepository commitRelease bumpVersion From dfaee2d241b38c1cdb61ac8f0e99524a0b0bf21b Mon Sep 17 00:00:00 2001 From: oliemansm Date: Thu, 13 May 2021 12:03:41 +0200 Subject: [PATCH 3/6] fix(#298): convert exception to graphql error --- .../servlet/HttpRequestInvokerImpl.java | 43 +++++++++++++------ .../CachingHttpRequestInvokerTest.groovy | 11 +++++ 2 files changed, 40 insertions(+), 14 deletions(-) diff --git a/graphql-java-servlet/src/main/java/graphql/kickstart/servlet/HttpRequestInvokerImpl.java b/graphql-java-servlet/src/main/java/graphql/kickstart/servlet/HttpRequestInvokerImpl.java index 4316827a..210d2558 100644 --- a/graphql-java-servlet/src/main/java/graphql/kickstart/servlet/HttpRequestInvokerImpl.java +++ b/graphql-java-servlet/src/main/java/graphql/kickstart/servlet/HttpRequestInvokerImpl.java @@ -1,8 +1,8 @@ package graphql.kickstart.servlet; -import static graphql.kickstart.servlet.HttpRequestHandler.STATUS_BAD_REQUEST; - +import graphql.ExecutionResult; import graphql.ExecutionResultImpl; +import graphql.GraphQLException; import graphql.kickstart.execution.GraphQLInvoker; import graphql.kickstart.execution.GraphQLQueryResult; import graphql.kickstart.execution.error.GenericGraphQLError; @@ -15,6 +15,7 @@ import java.io.UncheckedIOException; import java.util.Optional; import java.util.concurrent.CompletableFuture; +import java.util.concurrent.CompletionException; import java.util.concurrent.atomic.AtomicReference; import javax.servlet.AsyncContext; import javax.servlet.http.HttpServletRequest; @@ -82,7 +83,8 @@ private CompletableFuture invokeAndHandle( return invoke(invocationInput, request, response) .thenAccept(it -> writeResultResponse(invocationInput, it, request, response)) .thenAccept(it -> listenerHandler.onSuccess()) - .exceptionally(t -> writeBadRequestError(t, response, listenerHandler)) + .exceptionally( + t -> writeErrorResponse(invocationInput, request, response, listenerHandler, t)) .thenAccept(it -> listenerHandler.onFinally()); } @@ -99,18 +101,18 @@ private void writeResultResponse( } } - protected QueryResponseWriter createWriter( - GraphQLInvocationInput invocationInput, GraphQLQueryResult queryResult) { - return queryResponseWriterFactory.createWriter(invocationInput, queryResult, configuration); - } - - private Void writeBadRequestError( - Throwable t, HttpServletResponse response, ListenerHandler listenerHandler) { + private Void writeErrorResponse( + GraphQLInvocationInput invocationInput, + HttpServletRequest request, + HttpServletResponse response, + ListenerHandler listenerHandler, + Throwable t) { if (!response.isCommitted()) { - response.setStatus(STATUS_BAD_REQUEST); - log.info( - "Bad request: path was not \"/schema.json\" or no query variable named \"query\" given", - t); + writeResultResponse( + invocationInput, + GraphQLQueryResult.create(toErrorResult(t)), + request, + response); listenerHandler.onError(t); } else { log.warn( @@ -119,6 +121,19 @@ private Void writeBadRequestError( return null; } + private ExecutionResult toErrorResult(Throwable t) { + String message = + t instanceof CompletionException && t.getCause() != null + ? t.getCause().getMessage() + : t.getMessage(); + return new ExecutionResultImpl(new GenericGraphQLError(message)); + } + + protected QueryResponseWriter createWriter( + GraphQLInvocationInput invocationInput, GraphQLQueryResult queryResult) { + return queryResponseWriterFactory.createWriter(invocationInput, queryResult, configuration); + } + private CompletableFuture invoke( GraphQLInvocationInput invocationInput, HttpServletRequest request, 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 index 790d8591..c75d1593 100644 --- 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 @@ -1,12 +1,15 @@ package graphql.kickstart.servlet.cache +import graphql.ExecutionResult import graphql.kickstart.execution.GraphQLInvoker +import graphql.kickstart.execution.GraphQLObjectMapper import graphql.kickstart.execution.GraphQLQueryResult import graphql.kickstart.execution.input.GraphQLSingleInvocationInput import graphql.kickstart.servlet.GraphQLConfiguration import graphql.kickstart.servlet.HttpRequestInvoker import spock.lang.Specification +import javax.servlet.ServletOutputStream import javax.servlet.http.HttpServletRequest import javax.servlet.http.HttpServletResponse import java.util.concurrent.CompletableFuture @@ -22,6 +25,8 @@ class CachingHttpRequestInvokerTest extends Specification { def httpRequestInvokerMock def graphqlInvoker def configuration + def graphqlObjectMapper + def outputStreamMock def setup() { cacheReaderMock = Mock(CacheReader) @@ -32,11 +37,17 @@ class CachingHttpRequestInvokerTest extends Specification { configuration = Mock(GraphQLConfiguration) httpRequestInvokerMock = Mock(HttpRequestInvoker) graphqlInvoker = Mock(GraphQLInvoker) + graphqlObjectMapper = Mock(GraphQLObjectMapper) + outputStreamMock = Mock(ServletOutputStream) cachingInvoker = new CachingHttpRequestInvoker(configuration, httpRequestInvokerMock, cacheReaderMock) configuration.getResponseCacheManager() >> responseCacheManagerMock configuration.getGraphQLInvoker() >> graphqlInvoker + configuration.getObjectMapper() >> graphqlObjectMapper + graphqlObjectMapper.serializeResultAsBytes(_ as ExecutionResult) >> new byte[0] graphqlInvoker.queryAsync(invocationInputMock) >> CompletableFuture.completedFuture(Mock(GraphQLQueryResult)) + + responseMock.getOutputStream() >> outputStreamMock } def "should execute regular invoker if cache not exists"() { From 43995307e75dd2fa803ae84c5437efc4a65fe994 Mon Sep 17 00:00:00 2001 From: oliemansm Date: Thu, 13 May 2021 12:18:28 +0200 Subject: [PATCH 4/6] fix(sonar): code smell unused import --- .../graphql/kickstart/servlet/HttpRequestInvokerImpl.java | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/graphql-java-servlet/src/main/java/graphql/kickstart/servlet/HttpRequestInvokerImpl.java b/graphql-java-servlet/src/main/java/graphql/kickstart/servlet/HttpRequestInvokerImpl.java index 210d2558..3565f18e 100644 --- a/graphql-java-servlet/src/main/java/graphql/kickstart/servlet/HttpRequestInvokerImpl.java +++ b/graphql-java-servlet/src/main/java/graphql/kickstart/servlet/HttpRequestInvokerImpl.java @@ -2,7 +2,6 @@ import graphql.ExecutionResult; import graphql.ExecutionResultImpl; -import graphql.GraphQLException; import graphql.kickstart.execution.GraphQLInvoker; import graphql.kickstart.execution.GraphQLQueryResult; import graphql.kickstart.execution.error.GenericGraphQLError; @@ -109,10 +108,7 @@ private Void writeErrorResponse( Throwable t) { if (!response.isCommitted()) { writeResultResponse( - invocationInput, - GraphQLQueryResult.create(toErrorResult(t)), - request, - response); + invocationInput, GraphQLQueryResult.create(toErrorResult(t)), request, response); listenerHandler.onError(t); } else { log.warn( From c0a9126077bc2f93798fe72423af07bf8294a532 Mon Sep 17 00:00:00 2001 From: oliemansm Date: Thu, 13 May 2021 13:54:09 +0200 Subject: [PATCH 5/6] fix(#469): allow graphql-java future to be canceled --- .../FutureBatchedExecutionResult.java | 26 ++++++++ .../execution/FutureErrorExecutionResult.java | 26 ++++++++ .../execution/FutureExecutionResult.java | 29 +++++++++ .../FutureSingleExecutionResult.java | 25 ++++++++ .../kickstart/execution/GraphQLInvoker.java | 9 +++ .../servlet/HttpRequestInvokerImpl.java | 59 ++++++++++++------- .../CachingHttpRequestInvokerTest.groovy | 2 + 7 files changed, 155 insertions(+), 21 deletions(-) create mode 100644 graphql-java-kickstart/src/main/java/graphql/kickstart/execution/FutureBatchedExecutionResult.java create mode 100644 graphql-java-kickstart/src/main/java/graphql/kickstart/execution/FutureErrorExecutionResult.java create mode 100644 graphql-java-kickstart/src/main/java/graphql/kickstart/execution/FutureExecutionResult.java create mode 100644 graphql-java-kickstart/src/main/java/graphql/kickstart/execution/FutureSingleExecutionResult.java diff --git a/graphql-java-kickstart/src/main/java/graphql/kickstart/execution/FutureBatchedExecutionResult.java b/graphql-java-kickstart/src/main/java/graphql/kickstart/execution/FutureBatchedExecutionResult.java new file mode 100644 index 00000000..a97c3391 --- /dev/null +++ b/graphql-java-kickstart/src/main/java/graphql/kickstart/execution/FutureBatchedExecutionResult.java @@ -0,0 +1,26 @@ +package graphql.kickstart.execution; + +import graphql.ExecutionResult; +import graphql.kickstart.execution.input.GraphQLInvocationInput; +import java.util.List; +import java.util.concurrent.CompletableFuture; +import lombok.Getter; +import lombok.RequiredArgsConstructor; + +@RequiredArgsConstructor +class FutureBatchedExecutionResult implements FutureExecutionResult { + + @Getter + private final GraphQLInvocationInput invocationInput; + private final CompletableFuture> batched; + + @Override + public CompletableFuture thenApplyQueryResult() { + return batched.thenApply(GraphQLQueryResult::create); + } + + @Override + public void cancel() { + batched.cancel(true); + } +} diff --git a/graphql-java-kickstart/src/main/java/graphql/kickstart/execution/FutureErrorExecutionResult.java b/graphql-java-kickstart/src/main/java/graphql/kickstart/execution/FutureErrorExecutionResult.java new file mode 100644 index 00000000..745eeace --- /dev/null +++ b/graphql-java-kickstart/src/main/java/graphql/kickstart/execution/FutureErrorExecutionResult.java @@ -0,0 +1,26 @@ +package graphql.kickstart.execution; + +import graphql.kickstart.execution.input.GraphQLInvocationInput; +import java.util.concurrent.CompletableFuture; +import lombok.RequiredArgsConstructor; + +@RequiredArgsConstructor +class FutureErrorExecutionResult implements FutureExecutionResult { + + private final GraphQLErrorQueryResult errorQueryResult; + + @Override + public CompletableFuture thenApplyQueryResult() { + return CompletableFuture.completedFuture(errorQueryResult); + } + + @Override + public GraphQLInvocationInput getInvocationInput() { + return null; + } + + @Override + public void cancel() { + // nothing to do + } +} diff --git a/graphql-java-kickstart/src/main/java/graphql/kickstart/execution/FutureExecutionResult.java b/graphql-java-kickstart/src/main/java/graphql/kickstart/execution/FutureExecutionResult.java new file mode 100644 index 00000000..cfbf6b06 --- /dev/null +++ b/graphql-java-kickstart/src/main/java/graphql/kickstart/execution/FutureExecutionResult.java @@ -0,0 +1,29 @@ +package graphql.kickstart.execution; + +import graphql.ExecutionResult; +import graphql.kickstart.execution.input.GraphQLInvocationInput; +import java.util.List; +import java.util.concurrent.CompletableFuture; + +public interface FutureExecutionResult { + + static FutureExecutionResult single( + GraphQLInvocationInput invocationInput, CompletableFuture single) { + return new FutureSingleExecutionResult(invocationInput, single); + } + + static FutureExecutionResult batched( + GraphQLInvocationInput invocationInput, CompletableFuture> batched) { + return new FutureBatchedExecutionResult(invocationInput, batched); + } + + static FutureExecutionResult error(GraphQLErrorQueryResult result) { + return new FutureErrorExecutionResult(result); + } + + CompletableFuture thenApplyQueryResult(); + + GraphQLInvocationInput getInvocationInput(); + + void cancel(); +} diff --git a/graphql-java-kickstart/src/main/java/graphql/kickstart/execution/FutureSingleExecutionResult.java b/graphql-java-kickstart/src/main/java/graphql/kickstart/execution/FutureSingleExecutionResult.java new file mode 100644 index 00000000..5e2e70d2 --- /dev/null +++ b/graphql-java-kickstart/src/main/java/graphql/kickstart/execution/FutureSingleExecutionResult.java @@ -0,0 +1,25 @@ +package graphql.kickstart.execution; + +import graphql.ExecutionResult; +import graphql.kickstart.execution.input.GraphQLInvocationInput; +import java.util.concurrent.CompletableFuture; +import lombok.Getter; +import lombok.RequiredArgsConstructor; + +@RequiredArgsConstructor +class FutureSingleExecutionResult implements FutureExecutionResult { + + @Getter + private final GraphQLInvocationInput invocationInput; + private final CompletableFuture single; + + @Override + public CompletableFuture thenApplyQueryResult() { + return single.thenApply(GraphQLQueryResult::create); + } + + @Override + public void cancel() { + single.cancel(true); + } +} diff --git a/graphql-java-kickstart/src/main/java/graphql/kickstart/execution/GraphQLInvoker.java b/graphql-java-kickstart/src/main/java/graphql/kickstart/execution/GraphQLInvoker.java index ca8fb832..0d566ea2 100644 --- a/graphql-java-kickstart/src/main/java/graphql/kickstart/execution/GraphQLInvoker.java +++ b/graphql-java-kickstart/src/main/java/graphql/kickstart/execution/GraphQLInvoker.java @@ -22,6 +22,15 @@ public class GraphQLInvoker { private final BatchedDataLoaderGraphQLBuilder batchedDataLoaderGraphQLBuilder; private final GraphQLInvokerProxy proxy = GraphQL::executeAsync; + public FutureExecutionResult execute(GraphQLInvocationInput invocationInput) { + if (invocationInput instanceof GraphQLSingleInvocationInput) { + return FutureExecutionResult.single( + invocationInput, executeAsync((GraphQLSingleInvocationInput) invocationInput)); + } + return FutureExecutionResult.batched( + invocationInput, executeAsync((GraphQLBatchedInvocationInput) invocationInput)); + } + public CompletableFuture executeAsync( GraphQLSingleInvocationInput invocationInput) { GraphQL graphQL = graphQLBuilder.build(invocationInput.getSchema()); diff --git a/graphql-java-servlet/src/main/java/graphql/kickstart/servlet/HttpRequestInvokerImpl.java b/graphql-java-servlet/src/main/java/graphql/kickstart/servlet/HttpRequestInvokerImpl.java index 3565f18e..f9a2260f 100644 --- a/graphql-java-servlet/src/main/java/graphql/kickstart/servlet/HttpRequestInvokerImpl.java +++ b/graphql-java-servlet/src/main/java/graphql/kickstart/servlet/HttpRequestInvokerImpl.java @@ -2,6 +2,7 @@ import graphql.ExecutionResult; import graphql.ExecutionResultImpl; +import graphql.kickstart.execution.FutureExecutionResult; import graphql.kickstart.execution.GraphQLInvoker; import graphql.kickstart.execution.GraphQLQueryResult; import graphql.kickstart.execution.error.GenericGraphQLError; @@ -13,6 +14,7 @@ import java.io.IOException; import java.io.UncheckedIOException; import java.util.Optional; +import java.util.concurrent.CancellationException; import java.util.concurrent.CompletableFuture; import java.util.concurrent.CompletionException; import java.util.concurrent.atomic.AtomicReference; @@ -40,7 +42,7 @@ public void execute( if (request.isAsyncSupported()) { invokeAndHandleAsync(invocationInput, request, response, listenerHandler); } else { - invokeAndHandle(invocationInput, request, response, listenerHandler).join(); + handle(invoke(invocationInput, request, response), request, response, listenerHandler).join(); } } @@ -54,10 +56,10 @@ private void invokeAndHandleAsync( ? request.getAsyncContext() : request.startAsync(request, response); asyncContext.setTimeout(configuration.getAsyncTimeout()); - AtomicReference> futureHolder = new AtomicReference<>(); + AtomicReference futureHolder = new AtomicReference<>(); AsyncTimeoutListener timeoutListener = event -> { - Optional.ofNullable(futureHolder.get()).ifPresent(it -> it.cancel(true)); + Optional.ofNullable(futureHolder.get()).ifPresent(FutureExecutionResult::cancel); writeResultResponse( invocationInput, GraphQLQueryResult.create( @@ -68,22 +70,27 @@ private void invokeAndHandleAsync( }; asyncContext.addListener(timeoutListener); asyncContext.start( - () -> - futureHolder.set( - invokeAndHandle(invocationInput, request, response, listenerHandler) - .thenAccept(aVoid -> asyncContext.complete()))); + () -> { + FutureExecutionResult futureResult = invoke(invocationInput, request, response); + futureHolder.set(futureResult); + handle(futureResult, request, response, listenerHandler); + }); } - private CompletableFuture invokeAndHandle( - GraphQLInvocationInput invocationInput, + private CompletableFuture handle( + FutureExecutionResult futureResult, HttpServletRequest request, HttpServletResponse response, ListenerHandler listenerHandler) { - return invoke(invocationInput, request, response) - .thenAccept(it -> writeResultResponse(invocationInput, it, request, response)) + return futureResult + .thenApplyQueryResult() + .thenAccept( + it -> writeResultResponse(futureResult.getInvocationInput(), it, request, response)) .thenAccept(it -> listenerHandler.onSuccess()) .exceptionally( - t -> writeErrorResponse(invocationInput, request, response, listenerHandler, t)) + t -> + writeErrorResponse( + futureResult.getInvocationInput(), request, response, listenerHandler, t)) .thenAccept(it -> listenerHandler.onFinally()); } @@ -106,10 +113,11 @@ private Void writeErrorResponse( HttpServletResponse response, ListenerHandler listenerHandler, Throwable t) { + Throwable cause = getCause(t); if (!response.isCommitted()) { writeResultResponse( - invocationInput, GraphQLQueryResult.create(toErrorResult(t)), request, response); - listenerHandler.onError(t); + invocationInput, GraphQLQueryResult.create(toErrorResult(cause)), request, response); + listenerHandler.onError(cause); } else { log.warn( "Cannot write GraphQL response, because the HTTP response is already committed. It most likely timed out."); @@ -117,11 +125,20 @@ private Void writeErrorResponse( return null; } + private Throwable getCause(Throwable t) { + return t instanceof CompletionException && t.getCause() != null ? t.getCause() : t; + } + private ExecutionResult toErrorResult(Throwable t) { String message = - t instanceof CompletionException && t.getCause() != null - ? t.getCause().getMessage() + t instanceof CancellationException + ? "Execution canceled because timeout of " + + configuration.getAsyncTimeout() + + " millis was reached" : t.getMessage(); + if (message == null) { + message = "Unexpected error occurred"; + } return new ExecutionResultImpl(new GenericGraphQLError(message)); } @@ -130,17 +147,17 @@ protected QueryResponseWriter createWriter( return queryResponseWriterFactory.createWriter(invocationInput, queryResult, configuration); } - private CompletableFuture invoke( + private FutureExecutionResult invoke( GraphQLInvocationInput invocationInput, HttpServletRequest request, HttpServletResponse response) { if (invocationInput instanceof GraphQLSingleInvocationInput) { - return graphQLInvoker.queryAsync(invocationInput); + return graphQLInvoker.execute(invocationInput); } return invokeBatched((GraphQLBatchedInvocationInput) invocationInput, request, response); } - private CompletableFuture invokeBatched( + private FutureExecutionResult invokeBatched( GraphQLBatchedInvocationInput batchedInvocationInput, HttpServletRequest request, HttpServletResponse response) { @@ -148,10 +165,10 @@ private CompletableFuture invokeBatched( BatchInputPreProcessResult result = preprocessor.preProcessBatch(batchedInvocationInput, request, response); if (result.isExecutable()) { - return graphQLInvoker.queryAsync(result.getBatchedInvocationInput()); + return graphQLInvoker.execute(result.getBatchedInvocationInput()); } - return CompletableFuture.completedFuture( + return FutureExecutionResult.error( GraphQLQueryResult.createError(result.getStatusCode(), result.getStatusMessage())); } } 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 index c75d1593..6305c27e 100644 --- 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 @@ -1,6 +1,7 @@ package graphql.kickstart.servlet.cache import graphql.ExecutionResult +import graphql.kickstart.execution.FutureExecutionResult import graphql.kickstart.execution.GraphQLInvoker import graphql.kickstart.execution.GraphQLObjectMapper import graphql.kickstart.execution.GraphQLQueryResult @@ -39,6 +40,7 @@ class CachingHttpRequestInvokerTest extends Specification { graphqlInvoker = Mock(GraphQLInvoker) graphqlObjectMapper = Mock(GraphQLObjectMapper) outputStreamMock = Mock(ServletOutputStream) + graphqlInvoker.execute(invocationInputMock) >> FutureExecutionResult.single(invocationInputMock, CompletableFuture.completedFuture(Mock(GraphQLQueryResult))) cachingInvoker = new CachingHttpRequestInvoker(configuration, httpRequestInvokerMock, cacheReaderMock) configuration.getResponseCacheManager() >> responseCacheManagerMock From efdb43f388fd3eea33f410dd231752aa04ab0fe6 Mon Sep 17 00:00:00 2001 From: oliemansm Date: Thu, 13 May 2021 13:56:25 +0200 Subject: [PATCH 6/6] chore: remove redundant error response --- .../kickstart/servlet/HttpRequestInvokerImpl.java | 11 +---------- 1 file changed, 1 insertion(+), 10 deletions(-) diff --git a/graphql-java-servlet/src/main/java/graphql/kickstart/servlet/HttpRequestInvokerImpl.java b/graphql-java-servlet/src/main/java/graphql/kickstart/servlet/HttpRequestInvokerImpl.java index f9a2260f..30af5490 100644 --- a/graphql-java-servlet/src/main/java/graphql/kickstart/servlet/HttpRequestInvokerImpl.java +++ b/graphql-java-servlet/src/main/java/graphql/kickstart/servlet/HttpRequestInvokerImpl.java @@ -58,16 +58,7 @@ private void invokeAndHandleAsync( asyncContext.setTimeout(configuration.getAsyncTimeout()); AtomicReference futureHolder = new AtomicReference<>(); AsyncTimeoutListener timeoutListener = - event -> { - Optional.ofNullable(futureHolder.get()).ifPresent(FutureExecutionResult::cancel); - writeResultResponse( - invocationInput, - GraphQLQueryResult.create( - new ExecutionResultImpl(new GenericGraphQLError("Timeout"))), - (HttpServletRequest) event.getAsyncContext().getRequest(), - (HttpServletResponse) event.getAsyncContext().getResponse()); - listenerHandler.onError(event.getThrowable()); - }; + event -> Optional.ofNullable(futureHolder.get()).ifPresent(FutureExecutionResult::cancel); asyncContext.addListener(timeoutListener); asyncContext.start( () -> {