From e1a5734353ef2fbd342da972e5cbfaf98c27c094 Mon Sep 17 00:00:00 2001 From: heowc Date: Tue, 22 Feb 2022 19:27:58 +0900 Subject: [PATCH 1/6] Add test --- .../AbstractGraphQLHttpServletSpec.groovy | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) 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 022621de..946ad7f6 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 @@ -755,6 +755,22 @@ b getResponseContent().data.echoFiles == ["test", "test again"] } + def "errors while accessing file from the request"() { + setup: + request = Spy(MockHttpServletRequest) + request.setMethod("POST") + request.setContentType("multipart/form-data, boundary=test") + // See https://github.com/apache/tomcat/blob/main/java/org/apache/catalina/connector/Request.java#L2775...L2791 + request.getParts() >> { throw new IllegalStateException() } + + when: + servlet.doPost(request, response) + + then: + response.getStatus() == STATUS_ERROR + response.getContentLength() == 0 + } + def "batched query over HTTP POST body returns data"() { setup: request.setContent('[{ "query": "query { echo(arg:\\"test\\") }" }, { "query": "query { echo(arg:\\"test\\") }" }]'.bytes) From 21f96a89732ff12b82e858658fea3e2cecd4a674 Mon Sep 17 00:00:00 2001 From: heowc Date: Tue, 22 Feb 2022 19:27:58 +0900 Subject: [PATCH 2/6] Add test --- .../AbstractGraphQLHttpServletSpec.groovy | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) 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 022621de..946ad7f6 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 @@ -755,6 +755,22 @@ b getResponseContent().data.echoFiles == ["test", "test again"] } + def "errors while accessing file from the request"() { + setup: + request = Spy(MockHttpServletRequest) + request.setMethod("POST") + request.setContentType("multipart/form-data, boundary=test") + // See https://github.com/apache/tomcat/blob/main/java/org/apache/catalina/connector/Request.java#L2775...L2791 + request.getParts() >> { throw new IllegalStateException() } + + when: + servlet.doPost(request, response) + + then: + response.getStatus() == STATUS_ERROR + response.getContentLength() == 0 + } + def "batched query over HTTP POST body returns data"() { setup: request.setContent('[{ "query": "query { echo(arg:\\"test\\") }" }, { "query": "query { echo(arg:\\"test\\") }" }]'.bytes) From 065ee30fb8f7d8cfb615d805c5d54904f1158a7c Mon Sep 17 00:00:00 2001 From: heowc Date: Mon, 2 May 2022 22:54:31 +0900 Subject: [PATCH 3/6] Add listener for errors that occur in parsing `InvocationInput` --- .../servlet/HttpRequestHandlerImpl.java | 48 +++++++++++++------ .../kickstart/servlet/HttpRequestInvoker.java | 3 +- .../servlet/HttpRequestInvokerImpl.java | 5 +- .../InvocationInputParseException.java | 8 ++++ .../kickstart/servlet/ListenerHandler.java | 6 ++- .../cache/CachingHttpRequestInvoker.java | 6 ++- .../servlet/core/GraphQLServletListener.java | 8 ++++ .../AbstractGraphQLHttpServletSpec.groovy | 5 +- .../CachingHttpRequestInvokerTest.groovy | 15 +++--- 9 files changed, 73 insertions(+), 31 deletions(-) create mode 100644 graphql-java-servlet/src/main/java/graphql/kickstart/servlet/InvocationInputParseException.java 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 aa71eaf4..41b624f7 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 @@ -15,7 +15,7 @@ class HttpRequestHandlerImpl implements HttpRequestHandler { private final GraphQLConfiguration configuration; private final HttpRequestInvoker requestInvoker; - public HttpRequestHandlerImpl(GraphQLConfiguration configuration) { + HttpRequestHandlerImpl(GraphQLConfiguration configuration) { this( configuration, new HttpRequestInvokerImpl( @@ -24,7 +24,7 @@ public HttpRequestHandlerImpl(GraphQLConfiguration configuration) { new QueryResponseWriterFactoryImpl())); } - public HttpRequestHandlerImpl( + HttpRequestHandlerImpl( GraphQLConfiguration configuration, HttpRequestInvoker requestInvoker) { this.configuration = configuration; this.requestInvoker = requestInvoker; @@ -32,20 +32,22 @@ public HttpRequestHandlerImpl( @Override public void handle(HttpServletRequest request, HttpServletResponse response) throws IOException { + if (request.getCharacterEncoding() == null) { + request.setCharacterEncoding(StandardCharsets.UTF_8.name()); + } + + ListenerHandler listenerHandler = + ListenerHandler.start(request, response, configuration.getListeners()); + try { - if (request.getCharacterEncoding() == null) { - request.setCharacterEncoding(StandardCharsets.UTF_8.name()); - } - GraphQLInvocationInputParser invocationInputParser = - GraphQLInvocationInputParser.create( - request, - configuration.getInvocationInputFactory(), - configuration.getObjectMapper(), - configuration.getContextSetting()); - GraphQLInvocationInput invocationInput = - invocationInputParser. getGraphQLInvocationInput(request, response); - requestInvoker.execute(invocationInput, request, response); - } catch (GraphQLException | JsonProcessingException e) { + GraphQLInvocationInput invocationInput = parseInvocationInput(request, response); + requestInvoker.execute(invocationInput, request, response, listenerHandler); + } catch (InvocationInputParseException e) { + response.setStatus(STATUS_BAD_REQUEST); + log.info("Bad request: cannot parse http request", e); + listenerHandler.onParseError(e); + throw e; + } catch (GraphQLException e) { response.setStatus(STATUS_BAD_REQUEST); log.info("Bad request: cannot handle http request", e); throw e; @@ -55,4 +57,20 @@ public void handle(HttpServletRequest request, HttpServletResponse response) thr throw t; } } + + private GraphQLInvocationInput parseInvocationInput( + HttpServletRequest request, + HttpServletResponse response) { + try { + GraphQLInvocationInputParser invocationInputParser = + GraphQLInvocationInputParser.create( + request, + configuration.getInvocationInputFactory(), + configuration.getObjectMapper(), + configuration.getContextSetting()); + return invocationInputParser.getGraphQLInvocationInput(request, response); + } catch (Exception e) { + throw new InvocationInputParseException(e); + } + } } diff --git a/graphql-java-servlet/src/main/java/graphql/kickstart/servlet/HttpRequestInvoker.java b/graphql-java-servlet/src/main/java/graphql/kickstart/servlet/HttpRequestInvoker.java index 7cd0a740..0fbdf7f6 100644 --- a/graphql-java-servlet/src/main/java/graphql/kickstart/servlet/HttpRequestInvoker.java +++ b/graphql-java-servlet/src/main/java/graphql/kickstart/servlet/HttpRequestInvoker.java @@ -9,5 +9,6 @@ public interface HttpRequestInvoker { void execute( GraphQLInvocationInput invocationInput, HttpServletRequest request, - HttpServletResponse response); + HttpServletResponse response, + ListenerHandler listenerHandler); } 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 1b58c8ba..86a17ff0 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 @@ -39,9 +39,8 @@ public class HttpRequestInvokerImpl implements HttpRequestInvoker { public void execute( GraphQLInvocationInput invocationInput, HttpServletRequest request, - HttpServletResponse response) { - ListenerHandler listenerHandler = - ListenerHandler.start(request, response, configuration.getListeners()); + HttpServletResponse response, + ListenerHandler listenerHandler) { if (request.isAsyncSupported()) { invokeAndHandleAsync(invocationInput, request, response, listenerHandler); } else { diff --git a/graphql-java-servlet/src/main/java/graphql/kickstart/servlet/InvocationInputParseException.java b/graphql-java-servlet/src/main/java/graphql/kickstart/servlet/InvocationInputParseException.java new file mode 100644 index 00000000..ccc12940 --- /dev/null +++ b/graphql-java-servlet/src/main/java/graphql/kickstart/servlet/InvocationInputParseException.java @@ -0,0 +1,8 @@ +package graphql.kickstart.servlet; + +public class InvocationInputParseException extends RuntimeException { + + public InvocationInputParseException(Throwable t) { + super("Request parsing failed", t); + } +} diff --git a/graphql-java-servlet/src/main/java/graphql/kickstart/servlet/ListenerHandler.java b/graphql-java-servlet/src/main/java/graphql/kickstart/servlet/ListenerHandler.java index faff966d..6fe9df83 100644 --- a/graphql-java-servlet/src/main/java/graphql/kickstart/servlet/ListenerHandler.java +++ b/graphql-java-servlet/src/main/java/graphql/kickstart/servlet/ListenerHandler.java @@ -16,7 +16,7 @@ @Slf4j @RequiredArgsConstructor -class ListenerHandler { +public class ListenerHandler { private final List callbacks; private final HttpServletRequest request; @@ -60,6 +60,10 @@ void runCallbacks(Consumer action) { }); } + void onParseError(Throwable throwable) { + runCallbacks(it -> it.onParseError(request, response, throwable)); + } + void beforeFlush() { runCallbacks(it -> it.beforeFlush(request, response)); } 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 index a76ba3db..ba0d8bda 100644 --- 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 @@ -6,6 +6,7 @@ import graphql.kickstart.servlet.GraphQLConfiguration; import graphql.kickstart.servlet.HttpRequestInvoker; import graphql.kickstart.servlet.HttpRequestInvokerImpl; +import graphql.kickstart.servlet.ListenerHandler; import java.io.IOException; import javax.servlet.http.HttpServletRequest; import javax.servlet.http.HttpServletResponse; @@ -36,11 +37,12 @@ public CachingHttpRequestInvoker(GraphQLConfiguration configuration) { public void execute( GraphQLInvocationInput invocationInput, HttpServletRequest request, - HttpServletResponse response) { + HttpServletResponse response, + ListenerHandler listenerHandler) { try { if (!cacheReader.responseFromCache( invocationInput, request, response, configuration.getResponseCacheManager())) { - requestInvoker.execute(invocationInput, request, response); + requestInvoker.execute(invocationInput, request, response, listenerHandler); } } catch (IOException e) { response.setStatus(STATUS_BAD_REQUEST); diff --git a/graphql-java-servlet/src/main/java/graphql/kickstart/servlet/core/GraphQLServletListener.java b/graphql-java-servlet/src/main/java/graphql/kickstart/servlet/core/GraphQLServletListener.java index fc0feb34..7c9a7f79 100644 --- a/graphql-java-servlet/src/main/java/graphql/kickstart/servlet/core/GraphQLServletListener.java +++ b/graphql-java-servlet/src/main/java/graphql/kickstart/servlet/core/GraphQLServletListener.java @@ -21,6 +21,14 @@ default RequestCallback onRequest(HttpServletRequest request, HttpServletRespons */ interface RequestCallback { + /** + * Called when failed to parse InvocationInput and the response was not written. + * @param request http request + * @param response http response + */ + default void onParseError( + HttpServletRequest request, HttpServletResponse response, Throwable throwable) {} + /** * Called right before the response will be written and flushed. Can be used for applying some * changes to the response object, like adding response headers. 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 946ad7f6..a7f9e5da 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 @@ -767,7 +767,7 @@ b servlet.doPost(request, response) then: - response.getStatus() == STATUS_ERROR + response.getStatus() == STATUS_BAD_REQUEST response.getContentLength() == 0 } @@ -1128,8 +1128,7 @@ b servlet.doGet(request, response) then: - noExceptionThrown() - response.getStatus() == STATUS_ERROR + response.getStatus() == STATUS_BAD_REQUEST } def "errors while data fetching are masked in the response"() { 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 6305c27e..9a486036 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 @@ -8,6 +8,7 @@ import graphql.kickstart.execution.GraphQLQueryResult import graphql.kickstart.execution.input.GraphQLSingleInvocationInput import graphql.kickstart.servlet.GraphQLConfiguration import graphql.kickstart.servlet.HttpRequestInvoker +import graphql.kickstart.servlet.ListenerHandler import spock.lang.Specification import javax.servlet.ServletOutputStream @@ -28,6 +29,7 @@ class CachingHttpRequestInvokerTest extends Specification { def configuration def graphqlObjectMapper def outputStreamMock + def listenerHandlerMock def setup() { cacheReaderMock = Mock(CacheReader) @@ -42,6 +44,7 @@ class CachingHttpRequestInvokerTest extends Specification { outputStreamMock = Mock(ServletOutputStream) graphqlInvoker.execute(invocationInputMock) >> FutureExecutionResult.single(invocationInputMock, CompletableFuture.completedFuture(Mock(GraphQLQueryResult))) cachingInvoker = new CachingHttpRequestInvoker(configuration, httpRequestInvokerMock, cacheReaderMock) + listenerHandlerMock = Mock(ListenerHandler) configuration.getResponseCacheManager() >> responseCacheManagerMock configuration.getGraphQLInvoker() >> graphqlInvoker @@ -57,10 +60,10 @@ class CachingHttpRequestInvokerTest extends Specification { cacheReaderMock.responseFromCache(invocationInputMock, requestMock, responseMock, responseCacheManagerMock) >> false when: - cachingInvoker.execute(invocationInputMock, requestMock, responseMock) + cachingInvoker.execute(invocationInputMock, requestMock, responseMock, listenerHandlerMock) then: - 1 * httpRequestInvokerMock.execute(invocationInputMock, requestMock, responseMock) + 1 * httpRequestInvokerMock.execute(invocationInputMock, requestMock, responseMock, listenerHandlerMock) } def "should not execute regular invoker if cache exists"() { @@ -68,10 +71,10 @@ class CachingHttpRequestInvokerTest extends Specification { cacheReaderMock.responseFromCache(invocationInputMock, requestMock, responseMock, responseCacheManagerMock) >> true when: - cachingInvoker.execute(invocationInputMock, requestMock, responseMock) + cachingInvoker.execute(invocationInputMock, requestMock, responseMock, listenerHandlerMock) then: - 0 * httpRequestInvokerMock.execute(invocationInputMock, requestMock, responseMock) + 0 * httpRequestInvokerMock.execute(invocationInputMock, requestMock, responseMock, listenerHandlerMock) } def "should return bad request response when ioexception"() { @@ -79,7 +82,7 @@ class CachingHttpRequestInvokerTest extends Specification { cacheReaderMock.responseFromCache(invocationInputMock, requestMock, responseMock, responseCacheManagerMock) >> { throw new IOException() } when: - cachingInvoker.execute(invocationInputMock, requestMock, responseMock) + cachingInvoker.execute(invocationInputMock, requestMock, responseMock, listenerHandlerMock) then: 1 * responseMock.setStatus(400) @@ -90,7 +93,7 @@ class CachingHttpRequestInvokerTest extends Specification { def invoker = new CachingHttpRequestInvoker(configuration) when: - invoker.execute(invocationInputMock, requestMock, responseMock) + invoker.execute(invocationInputMock, requestMock, responseMock, listenerHandlerMock) then: noExceptionThrown() From 14077de6a91a07cc9c40f3d1de1cddda4fdc6397 Mon Sep 17 00:00:00 2001 From: heowc Date: Tue, 3 May 2022 09:44:39 +0900 Subject: [PATCH 4/6] Remove unused import --- .../java/graphql/kickstart/servlet/HttpRequestHandlerImpl.java | 1 - 1 file changed, 1 deletion(-) 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 41b624f7..d0af08e4 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,6 +1,5 @@ package graphql.kickstart.servlet; -import com.fasterxml.jackson.core.JsonProcessingException; import graphql.GraphQLException; import graphql.kickstart.execution.input.GraphQLInvocationInput; import java.io.IOException; From 2a5fe3fe56fb3688cea10d259fa643b3d061528f Mon Sep 17 00:00:00 2001 From: WonChul Heo Date: Wed, 25 May 2022 15:54:17 +0900 Subject: [PATCH 5/6] Indent --- .../java/graphql/kickstart/servlet/HttpRequestHandlerImpl.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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 d0af08e4..eccef4d8 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 @@ -36,7 +36,7 @@ public void handle(HttpServletRequest request, HttpServletResponse response) thr } ListenerHandler listenerHandler = - ListenerHandler.start(request, response, configuration.getListeners()); + ListenerHandler.start(request, response, configuration.getListeners()); try { GraphQLInvocationInput invocationInput = parseInvocationInput(request, response); From 99ba5e7563fb1e83768d0f61c3add9f141ce4e44 Mon Sep 17 00:00:00 2001 From: WonChul Heo Date: Wed, 25 May 2022 15:54:41 +0900 Subject: [PATCH 6/6] Indent --- .../kickstart/servlet/HttpRequestHandlerImpl.java | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) 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 eccef4d8..e99a4058 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 @@ -62,11 +62,11 @@ private GraphQLInvocationInput parseInvocationInput( HttpServletResponse response) { try { GraphQLInvocationInputParser invocationInputParser = - GraphQLInvocationInputParser.create( - request, - configuration.getInvocationInputFactory(), - configuration.getObjectMapper(), - configuration.getContextSetting()); + GraphQLInvocationInputParser.create( + request, + configuration.getInvocationInputFactory(), + configuration.getObjectMapper(), + configuration.getContextSetting()); return invocationInputParser.getGraphQLInvocationInput(request, response); } catch (Exception e) { throw new InvocationInputParseException(e);