diff --git a/.github/workflows/pull-request.yml b/.github/workflows/pull-request.yml index e3a5d868..560e73da 100644 --- a/.github/workflows/pull-request.yml +++ b/.github/workflows/pull-request.yml @@ -53,27 +53,34 @@ jobs: name: Sonar analysis needs: validation runs-on: ubuntu-latest + env: + SONAR_TOKEN: ${{ secrets.SONAR_TOKEN }} steps: - uses: actions/checkout@v2 + if: env.SONAR_TOKEN != null with: fetch-depth: 0 # Shallow clones should be disabled for a better relevancy of analysis - name: Set up JDK 11 + if: env.SONAR_TOKEN != null uses: actions/setup-java@v1 with: java-version: 11 - name: Cache SonarCloud packages + if: env.SONAR_TOKEN != null uses: actions/cache@v1 with: path: ~/.sonar/cache key: ${{ runner.os }}-sonar restore-keys: ${{ runner.os }}-sonar - name: Cache Gradle packages + if: env.SONAR_TOKEN != null uses: actions/cache@v1 with: path: ~/.gradle/caches key: ${{ runner.os }}-gradle-${{ hashFiles('**/*.gradle') }} restore-keys: ${{ runner.os }}-gradle - name: Build and analyze + if: env.SONAR_TOKEN != null env: GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }} # Needed to get PR information, if any SONAR_TOKEN: ${{ secrets.SONAR_TOKEN }} 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 c6a4c3f5..fa552699 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 @@ -8,6 +8,7 @@ import graphql.kickstart.execution.input.GraphQLBatchedInvocationInput; import graphql.kickstart.execution.input.GraphQLInvocationInput; import graphql.kickstart.execution.input.GraphQLSingleInvocationInput; +import java.util.ArrayList; import java.util.List; import java.util.concurrent.CompletableFuture; import lombok.AllArgsConstructor; @@ -28,26 +29,36 @@ public CompletableFuture executeAsync( } public GraphQLQueryResult query(GraphQLInvocationInput invocationInput) { + return queryAsync(invocationInput).join(); + } + + public CompletableFuture queryAsync(GraphQLInvocationInput invocationInput) { if (invocationInput instanceof GraphQLSingleInvocationInput) { - return GraphQLQueryResult.create(query((GraphQLSingleInvocationInput) invocationInput)); + return executeAsync((GraphQLSingleInvocationInput)invocationInput).thenApply(GraphQLQueryResult::create); } GraphQLBatchedInvocationInput batchedInvocationInput = (GraphQLBatchedInvocationInput) invocationInput; - return GraphQLQueryResult.create(query(batchedInvocationInput)); + return executeAsync(batchedInvocationInput).thenApply(GraphQLQueryResult::create); } - private ExecutionResult query(GraphQLSingleInvocationInput singleInvocationInput) { - return executeAsync(singleInvocationInput).join(); + private CompletableFuture> executeAsync(GraphQLBatchedInvocationInput batchedInvocationInput) { + GraphQL graphQL = batchedDataLoaderGraphQLBuilder.newGraphQL(batchedInvocationInput, graphQLBuilder); + return sequence( + batchedInvocationInput.getExecutionInputs().stream() + .map(executionInput -> proxy.executeAsync(graphQL, executionInput)) + .collect(toList())); } - private List query(GraphQLBatchedInvocationInput batchedInvocationInput) { - GraphQL graphQL = batchedDataLoaderGraphQLBuilder - .newGraphQL(batchedInvocationInput, graphQLBuilder); - return batchedInvocationInput.getExecutionInputs().stream() - .map(executionInput -> proxy.executeAsync(graphQL, executionInput)) - .collect(toList()) - .stream() - .map(CompletableFuture::join) - .collect(toList()); + @SuppressWarnings({"unchecked", "rawtypes"}) + private CompletableFuture> sequence(List> futures) { + CompletableFuture[] futuresArray = futures.toArray(new CompletableFuture[0]); + return CompletableFuture.allOf(futuresArray).thenApply(aVoid -> { + List result = new ArrayList<>(futures.size()); + for (CompletableFuture future : futuresArray) { + assert future.isDone(); // per the API contract of allOf() + result.add((T) future.join()); + } + return result; + }); } } diff --git a/graphql-java-servlet/src/main/java/graphql/kickstart/servlet/AbstractGraphQLHttpServlet.java b/graphql-java-servlet/src/main/java/graphql/kickstart/servlet/AbstractGraphQLHttpServlet.java index 34dafb26..e7fef92e 100644 --- a/graphql-java-servlet/src/main/java/graphql/kickstart/servlet/AbstractGraphQLHttpServlet.java +++ b/graphql-java-servlet/src/main/java/graphql/kickstart/servlet/AbstractGraphQLHttpServlet.java @@ -18,7 +18,6 @@ import java.util.function.Consumer; import java.util.function.Function; import java.util.stream.Collectors; -import javax.servlet.AsyncContext; import javax.servlet.Servlet; import javax.servlet.http.HttpServlet; import javax.servlet.http.HttpServletRequest; @@ -135,59 +134,38 @@ public String executeQuery(String query) { } } - private void doRequestAsync(HttpServletRequest request, HttpServletResponse response, - HttpRequestHandler handler) { - if (configuration.isAsyncServletModeEnabled()) { - AsyncContext asyncContext = request.startAsync(request, response); - asyncContext.setTimeout(configuration.getAsyncTimeout()); - HttpServletRequest asyncRequest = (HttpServletRequest) asyncContext.getRequest(); - HttpServletResponse asyncResponse = (HttpServletResponse) asyncContext.getResponse(); - configuration.getAsyncExecutor() - .execute(() -> doRequest(asyncRequest, asyncResponse, handler, asyncContext)); - } else { - doRequest(request, response, handler, null); - } + @Override + protected void doGet(HttpServletRequest req, HttpServletResponse resp) { + doRequest(req, resp); } - private void doRequest(HttpServletRequest request, HttpServletResponse response, - HttpRequestHandler handler, - AsyncContext asyncContext) { + @Override + protected void doPost(HttpServletRequest req, HttpServletResponse resp) { + doRequest(req, resp); + } + private void doRequest(HttpServletRequest request, HttpServletResponse response) { + init(); List requestCallbacks = runListeners( l -> l.onRequest(request, response)); try { - handler.handle(request, response); + requestHandler.handle(request, response); runCallbacks(requestCallbacks, c -> c.onSuccess(request, response)); - } catch (Throwable t) { + } catch (Exception t) { log.error("Error executing GraphQL request!", t); runCallbacks(requestCallbacks, c -> c.onError(request, response, t)); } finally { runCallbacks(requestCallbacks, c -> c.onFinally(request, response)); - if (asyncContext != null) { - asyncContext.complete(); - } } } - @Override - protected void doGet(HttpServletRequest req, HttpServletResponse resp) { - init(); - doRequestAsync(req, resp, requestHandler); - } - - @Override - protected void doPost(HttpServletRequest req, HttpServletResponse resp) { - init(); - doRequestAsync(req, resp, requestHandler); - } - private List runListeners(Function action) { return configuration.getListeners().stream() .map(listener -> { try { return action.apply(listener); - } catch (Throwable t) { + } catch (Exception t) { log.error("Error running listener: {}", listener, t); return null; } @@ -200,7 +178,7 @@ private void runCallbacks(List callbacks, Consumer action) { callbacks.forEach(callback -> { try { action.accept(callback); - } catch (Throwable t) { + } catch (Exception t) { log.error("Error running callback: {}", callback, t); } }); diff --git a/graphql-java-servlet/src/main/java/graphql/kickstart/servlet/GraphQLConfiguration.java b/graphql-java-servlet/src/main/java/graphql/kickstart/servlet/GraphQLConfiguration.java index 2d13a7d9..71465a48 100644 --- a/graphql-java-servlet/src/main/java/graphql/kickstart/servlet/GraphQLConfiguration.java +++ b/graphql-java-servlet/src/main/java/graphql/kickstart/servlet/GraphQLConfiguration.java @@ -30,8 +30,19 @@ public class GraphQLConfiguration { private final GraphQLInvoker graphQLInvoker; private final GraphQLObjectMapper objectMapper; private final List listeners; + /** + * For removal + * @since 10.1.0 + */ + @Deprecated private final boolean asyncServletModeEnabled; + /** + * For removal + * @since 10.1.0 + */ + @Deprecated private final Executor asyncExecutor; + private final long subscriptionTimeout; @Getter private final long asyncTimeout; diff --git a/graphql-java-servlet/src/main/java/graphql/kickstart/servlet/HttpRequestHandlerImpl.java b/graphql-java-servlet/src/main/java/graphql/kickstart/servlet/HttpRequestHandlerImpl.java index 513c5201..7f583bd0 100644 --- a/graphql-java-servlet/src/main/java/graphql/kickstart/servlet/HttpRequestHandlerImpl.java +++ b/graphql-java-servlet/src/main/java/graphql/kickstart/servlet/HttpRequestHandlerImpl.java @@ -1,5 +1,6 @@ package graphql.kickstart.servlet; +import com.fasterxml.jackson.core.JsonProcessingException; import graphql.GraphQLException; import graphql.kickstart.execution.GraphQLInvoker; import graphql.kickstart.execution.GraphQLQueryResult; @@ -9,6 +10,9 @@ import graphql.kickstart.servlet.input.BatchInputPreProcessResult; import graphql.kickstart.servlet.input.BatchInputPreProcessor; import java.io.IOException; +import java.io.UncheckedIOException; +import java.util.concurrent.CompletableFuture; +import javax.servlet.AsyncContext; import javax.servlet.http.HttpServletRequest; import javax.servlet.http.HttpServletResponse; import lombok.extern.slf4j.Slf4j; @@ -36,11 +40,11 @@ public void handle(HttpServletRequest request, HttpServletResponse response) thr GraphQLInvocationInput invocationInput = invocationInputParser .getGraphQLInvocationInput(request, response); execute(invocationInput, request, response); - } catch (GraphQLException e) { + } catch (GraphQLException| JsonProcessingException e) { response.setStatus(STATUS_BAD_REQUEST); log.info("Bad request: cannot handle http request", e); throw e; - } catch (Throwable t) { + } catch (Exception t) { response.setStatus(500); log.error("Cannot handle http request", t); throw t; @@ -49,38 +53,65 @@ public void handle(HttpServletRequest request, HttpServletResponse response) thr protected void execute(GraphQLInvocationInput invocationInput, HttpServletRequest request, HttpServletResponse response) throws IOException { - GraphQLQueryResult queryResult = invoke(invocationInput, request, response); + if (request.isAsyncSupported()) { + AsyncContext asyncContext = request.isAsyncStarted() + ? request.getAsyncContext() + : request.startAsync(request, response); + asyncContext.setTimeout(configuration.getAsyncTimeout()); + invoke(invocationInput, request, response) + .thenAccept(result -> writeResultResponse(invocationInput, result, request, response)) + .exceptionally(t -> writeErrorResponse(t, response)) + .thenAccept(aVoid -> asyncContext.complete()); + } else { + try { + GraphQLQueryResult result = invoke(invocationInput, request, response).join(); + writeResultResponse(invocationInput, result, request, response); + } catch (Exception t) { + writeErrorResponse(t, response); + } + } + } + private void writeResultResponse(GraphQLInvocationInput invocationInput, GraphQLQueryResult queryResult, HttpServletRequest request, + HttpServletResponse response) { QueryResponseWriter queryResponseWriter = createWriter(invocationInput, queryResult); - queryResponseWriter.write(request, response); + try { + queryResponseWriter.write(request, response); + } catch (IOException e) { + throw new UncheckedIOException(e); + } } - protected QueryResponseWriter createWriter(GraphQLInvocationInput invocationInput, - GraphQLQueryResult queryResult) { + protected QueryResponseWriter createWriter(GraphQLInvocationInput invocationInput, GraphQLQueryResult queryResult) { return QueryResponseWriter.createWriter(queryResult, configuration.getObjectMapper(), configuration.getSubscriptionTimeout()); } - private GraphQLQueryResult invoke(GraphQLInvocationInput invocationInput, - HttpServletRequest request, + private Void writeErrorResponse(Throwable t, HttpServletResponse response) { + response.setStatus(STATUS_BAD_REQUEST); + log.info("Bad GET request: path was not \"/schema.json\" or no query variable named \"query\" given", t); + return null; + } + + private CompletableFuture invoke(GraphQLInvocationInput invocationInput, HttpServletRequest request, HttpServletResponse response) { if (invocationInput instanceof GraphQLSingleInvocationInput) { - return graphQLInvoker.query(invocationInput); + return graphQLInvoker.queryAsync(invocationInput); } return invokeBatched((GraphQLBatchedInvocationInput) invocationInput, request, response); } - private GraphQLQueryResult invokeBatched(GraphQLBatchedInvocationInput batchedInvocationInput, + private CompletableFuture invokeBatched(GraphQLBatchedInvocationInput batchedInvocationInput, HttpServletRequest request, HttpServletResponse response) { BatchInputPreProcessor preprocessor = configuration.getBatchInputPreProcessor(); BatchInputPreProcessResult result = preprocessor .preProcessBatch(batchedInvocationInput, request, response); if (result.isExecutable()) { - return graphQLInvoker.query(result.getBatchedInvocationInput()); + return graphQLInvoker.queryAsync(result.getBatchedInvocationInput()); } - return GraphQLQueryResult.createError(result.getStatusCode(), result.getStatusMessage()); + return CompletableFuture.completedFuture(GraphQLQueryResult.createError(result.getStatusCode(), result.getStatusMessage())); } } diff --git a/graphql-java-servlet/src/main/java/graphql/kickstart/servlet/cache/CachingHttpRequestHandlerImpl.java b/graphql-java-servlet/src/main/java/graphql/kickstart/servlet/cache/CachingHttpRequestHandlerImpl.java index 0fba9078..877e3d14 100644 --- a/graphql-java-servlet/src/main/java/graphql/kickstart/servlet/cache/CachingHttpRequestHandlerImpl.java +++ b/graphql-java-servlet/src/main/java/graphql/kickstart/servlet/cache/CachingHttpRequestHandlerImpl.java @@ -44,6 +44,7 @@ protected void execute(GraphQLInvocationInput invocationInput, HttpServletReques } } + @Override protected QueryResponseWriter createWriter(GraphQLInvocationInput invocationInput, GraphQLQueryResult queryResult) { return CachingQueryResponseWriter.createCacheWriter( diff --git a/graphql-java-servlet/src/test/groovy/graphql/kickstart/servlet/AbstractGraphQLHttpServletSpec.groovy b/graphql-java-servlet/src/test/groovy/graphql/kickstart/servlet/AbstractGraphQLHttpServletSpec.groovy index bf954e29..99e78167 100644 --- a/graphql-java-servlet/src/test/groovy/graphql/kickstart/servlet/AbstractGraphQLHttpServletSpec.groovy +++ b/graphql-java-servlet/src/test/groovy/graphql/kickstart/servlet/AbstractGraphQLHttpServletSpec.groovy @@ -127,7 +127,7 @@ class AbstractGraphQLHttpServletSpec extends Specification { getResponseContent().data.echo == "special char รก" } - def "async query over HTTP GET starts async request"() { + def "disabling async support on request over HTTP GET does not start async request"() { setup: servlet = TestUtils.createDefaultServlet({ env -> env.arguments.arg }, { env -> env.arguments.arg }, { env -> AtomicReference> publisherRef = new AtomicReference<>(); @@ -138,12 +138,13 @@ class AbstractGraphQLHttpServletSpec extends Specification { return publisherRef.get() }, true) request.addParameter('query', 'query { echo(arg:"test") }') + request.setAsyncSupported(false) when: servlet.doGet(request, response) then: - request.asyncStarted == true + request.asyncContext == null } def "query over HTTP GET with variables returns data"() { @@ -442,7 +443,7 @@ class AbstractGraphQLHttpServletSpec extends Specification { getResponseContent().data.echo == "test" } - def "async query over HTTP POST starts async request"() { + def "disabling async support on request over HTTP POST does not start async request"() { setup: servlet = TestUtils.createDefaultServlet({ env -> env.arguments.arg }, { env -> env.arguments.arg }, { env -> AtomicReference> publisherRef = new AtomicReference<>(); @@ -455,12 +456,13 @@ class AbstractGraphQLHttpServletSpec extends Specification { request.setContent(mapper.writeValueAsBytes([ query: 'query { echo(arg:"test") }' ])) + request.setAsyncSupported(false) when: servlet.doPost(request, response) then: - request.asyncStarted == true + request.asyncContext == null } def "query over HTTP POST body with graphql contentType returns data"() { diff --git a/graphql-java-servlet/src/test/groovy/graphql/kickstart/servlet/TestUtils.groovy b/graphql-java-servlet/src/test/groovy/graphql/kickstart/servlet/TestUtils.groovy index 907c56b7..e9dd2fd1 100644 --- a/graphql-java-servlet/src/test/groovy/graphql/kickstart/servlet/TestUtils.groovy +++ b/graphql-java-servlet/src/test/groovy/graphql/kickstart/servlet/TestUtils.groovy @@ -5,6 +5,9 @@ import graphql.Directives import graphql.Scalars import graphql.execution.reactive.SingleSubscriberPublisher import graphql.kickstart.execution.context.ContextSetting +import graphql.kickstart.servlet.apollo.ApolloScalars +import graphql.kickstart.servlet.context.GraphQLServletContextBuilder +import graphql.kickstart.servlet.input.BatchInputPreProcessor import graphql.schema.* import graphql.schema.idl.RuntimeWiring import graphql.schema.idl.SchemaGenerator @@ -46,7 +49,7 @@ class TestUtils { DataFetcher fieldDataFetcher = { env -> env.arguments.arg }, DataFetcher otherDataFetcher, boolean asyncServletModeEnabled = false, ContextSetting contextSetting, - graphql.kickstart.servlet.context.GraphQLServletContextBuilder contextBuilder) { + GraphQLServletContextBuilder contextBuilder) { GraphQLSchema schema = createGraphQlSchemaWithTwoLevels(queryDataFetcher, fieldDataFetcher, otherDataFetcher) GraphQLHttpServlet servlet = GraphQLHttpServlet.with(GraphQLConfiguration .with(schema) @@ -68,7 +71,7 @@ class TestUtils { })) return publisherRef.get() }, boolean asyncServletModeEnabled = false, - graphql.kickstart.servlet.input.BatchInputPreProcessor batchHandler) { + BatchInputPreProcessor batchHandler) { GraphQLHttpServlet servlet = GraphQLHttpServlet.with( graphQLConfiguration(createGraphQlSchema(queryDataFetcher, mutationDataFetcher, subscriptionDataFetcher), batchHandler, asyncServletModeEnabled)) @@ -76,7 +79,7 @@ class TestUtils { return servlet } - static def graphQLConfiguration(GraphQLSchema schema, graphql.kickstart.servlet.input.BatchInputPreProcessor batchInputPreProcessor, + static def graphQLConfiguration(GraphQLSchema schema, BatchInputPreProcessor batchInputPreProcessor, boolean asyncServletModeEnabled) { def configBuilder = GraphQLConfiguration.with(schema).with(asyncServletModeEnabled) if (batchInputPreProcessor != null) { @@ -159,7 +162,7 @@ class TestUtils { field.type(Scalars.GraphQLString) field.argument { argument -> argument.name("file") - argument.type(graphql.kickstart.servlet.apollo.ApolloScalars.Upload) + argument.type(ApolloScalars.Upload) } field.dataFetcher({ env -> new String(ByteStreams.toByteArray(env.arguments.file.getInputStream())) }) } @@ -168,7 +171,7 @@ class TestUtils { field.type(GraphQLList.list(Scalars.GraphQLString)) field.argument { argument -> argument.name("files") - argument.type(GraphQLList.list(GraphQLNonNull.nonNull(graphql.kickstart.servlet.apollo.ApolloScalars.Upload))) + argument.type(GraphQLList.list(GraphQLNonNull.nonNull(ApolloScalars.Upload))) } field.dataFetcher({ env -> env.arguments.files.collect { @@ -196,7 +199,7 @@ class TestUtils { .query(query) .mutation(mutation) .subscription(subscription) - .additionalType(graphql.kickstart.servlet.apollo.ApolloScalars.Upload) + .additionalType(ApolloScalars.Upload) .additionalDirective(Directives.DeferDirective) .build() }