This is an automated email from the ASF dual-hosted git repository. kenhuuu pushed a commit to branch master-http in repository https://gitbox.apache.org/repos/asf/tinkerpop.git
The following commit(s) were added to refs/heads/master-http by this push: new 5ebc359e90 Add back evaluationTimeout and materializeProperties 5ebc359e90 is described below commit 5ebc359e908bc80da803927d04af1130a053aa76 Author: Ken Hu <106191785+kenh...@users.noreply.github.com> AuthorDate: Thu Apr 18 08:59:10 2024 -0700 Add back evaluationTimeout and materializeProperties evaluationTimeout and materializeProperties should be allowed to be set from the request body as it affects how the server should execute the query. --- .../apache/tinkerpop/gremlin/driver/Client.java | 8 +- .../tinkerpop/gremlin/driver/RequestOptions.java | 6 +- .../apache/tinkerpop/gremlin/server/Context.java | 11 +- .../server/handler/HttpGremlinEndpointHandler.java | 3 +- .../server/handler/HttpRequestMessageDecoder.java | 19 +- .../server/GremlinServerHttpIntegrateTest.java | 68 ++++++- .../gremlin/server/GremlinServerIntegrateTest.java | 224 ++++++++++----------- .../gremlin/util/message/RequestMessageV4.java | 18 ++ .../gremlin/util/message/RequestMessageV4Test.java | 26 +++ 9 files changed, 250 insertions(+), 133 deletions(-) diff --git a/gremlin-driver/src/main/java/org/apache/tinkerpop/gremlin/driver/Client.java b/gremlin-driver/src/main/java/org/apache/tinkerpop/gremlin/driver/Client.java index bc718945c0..1bf3c1dbb8 100644 --- a/gremlin-driver/src/main/java/org/apache/tinkerpop/gremlin/driver/Client.java +++ b/gremlin-driver/src/main/java/org/apache/tinkerpop/gremlin/driver/Client.java @@ -356,13 +356,13 @@ public abstract class Client { .addChunkSize(batchSize); // apply settings if they were made available -// options.getTimeout().ifPresent(timeout -> request.add(Tokens.ARGS_EVAL_TIMEOUT, timeout)); + options.getTimeout().ifPresent(timeout -> request.addTimeoutMillis(timeout)); options.getParameters().ifPresent(params -> request.addBindings(params)); options.getAliases().ifPresent(aliases -> {if (aliases.get("g") != null) request.addG(aliases.get("g")); }); // options.getOverrideRequestId().ifPresent(request::overrideRequestId); // options.getUserAgent().ifPresent(userAgent -> request.addArg(Tokens.ARGS_USER_AGENT, userAgent)); options.getLanguage().ifPresent(lang -> request.addLanguage(lang)); -// options.getMaterializeProperties().ifPresent(mp -> request.addArg(Tokens.ARGS_MATERIALIZE_PROPERTIES, mp)); + options.getMaterializeProperties().ifPresent(mp -> request.addMaterializeProperties(mp)); return submitAsync(request.create()); } @@ -655,10 +655,10 @@ public abstract class Client { // apply settings if they were made available options.getBatchSize().ifPresent(batchSize -> request.addChunkSize(batchSize)); -// options.getTimeout().ifPresent(timeout -> request.add(Tokens.ARGS_EVAL_TIMEOUT, timeout)); + options.getTimeout().ifPresent(timeout -> request.addTimeoutMillis(timeout)); // options.getOverrideRequestId().ifPresent(request::overrideRequestId); // options.getUserAgent().ifPresent(userAgent -> request.add(Tokens.ARGS_USER_AGENT, userAgent)); -// options.getMaterializeProperties().ifPresent(mp -> request.addArg(Tokens.ARGS_MATERIALIZE_PROPERTIES, mp)); + options.getMaterializeProperties().ifPresent(mp -> request.addMaterializeProperties(mp)); return submitAsync(request.create()); } catch (RuntimeException re) { diff --git a/gremlin-driver/src/main/java/org/apache/tinkerpop/gremlin/driver/RequestOptions.java b/gremlin-driver/src/main/java/org/apache/tinkerpop/gremlin/driver/RequestOptions.java index 3f66e2fbb0..63d7dad91a 100644 --- a/gremlin-driver/src/main/java/org/apache/tinkerpop/gremlin/driver/RequestOptions.java +++ b/gremlin-driver/src/main/java/org/apache/tinkerpop/gremlin/driver/RequestOptions.java @@ -41,7 +41,7 @@ public final class RequestOptions { private final UUID overrideRequestId; private final String userAgent; private final String language; -// private final String materializeProperties; + private final String materializeProperties; private RequestOptions(final Builder builder) { this.aliases = builder.aliases; @@ -51,7 +51,7 @@ public final class RequestOptions { this.overrideRequestId = builder.overrideRequestId; this.userAgent = builder.userAgent; this.language = builder.language; -// this.materializeProperties = builder.materializeProperties; + this.materializeProperties = builder.materializeProperties; } public Optional<UUID> getOverrideRequestId() { @@ -82,7 +82,7 @@ public final class RequestOptions { return Optional.ofNullable(language); } -// public Optional<String> getMaterializeProperties() { return Optional.ofNullable(materializeProperties); } + public Optional<String> getMaterializeProperties() { return Optional.ofNullable(materializeProperties); } public static Builder build() { return new Builder(); diff --git a/gremlin-server/src/main/java/org/apache/tinkerpop/gremlin/server/Context.java b/gremlin-server/src/main/java/org/apache/tinkerpop/gremlin/server/Context.java index 26513f2646..88bd88e690 100644 --- a/gremlin-server/src/main/java/org/apache/tinkerpop/gremlin/server/Context.java +++ b/gremlin-server/src/main/java/org/apache/tinkerpop/gremlin/server/Context.java @@ -200,13 +200,15 @@ public class Context { private long determineTimeout() { // timeout override - handle both deprecated and newly named configuration. earlier logic should prevent // both configurations from being submitted at the same time - // evaluationTimeout RequestMessage argument was removed starting in 4.0. + final Long timeoutMs = requestMessage.getField(Tokens.ARGS_EVAL_TIMEOUT); + final long seto = (null != timeoutMs) ? timeoutMs : settings.getEvaluationTimeout(); + // override the timeout if the lifecycle has a value assigned. if the script contains with(timeout) // options then allow that value to override what's provided on the lifecycle final Optional<Long> timeoutDefinedInScript = requestContentType == RequestContentType.SCRIPT ? GremlinScriptChecker.parse(gremlinArgument.toString()).getTimeout() : Optional.empty(); - return timeoutDefinedInScript.orElse(settings.getEvaluationTimeout()); + return timeoutDefinedInScript.orElse(seto); } private String determineMaterializeProperties() { @@ -219,8 +221,11 @@ public class Context { : Tokens.MATERIALIZE_PROPERTIES_ALL; } + final String materializeProperties = requestMessage.getField(Tokens.ARGS_MATERIALIZE_PROPERTIES); // all options except MATERIALIZE_PROPERTIES_TOKENS treated as MATERIALIZE_PROPERTIES_ALL - return Tokens.MATERIALIZE_PROPERTIES_ALL; + return Tokens.MATERIALIZE_PROPERTIES_TOKENS.equals(materializeProperties) + ? Tokens.MATERIALIZE_PROPERTIES_TOKENS + : Tokens.MATERIALIZE_PROPERTIES_ALL; } public void handleDetachment(final List<Object> aggregate) { diff --git a/gremlin-server/src/main/java/org/apache/tinkerpop/gremlin/server/handler/HttpGremlinEndpointHandler.java b/gremlin-server/src/main/java/org/apache/tinkerpop/gremlin/server/handler/HttpGremlinEndpointHandler.java index 2196795250..906792f414 100644 --- a/gremlin-server/src/main/java/org/apache/tinkerpop/gremlin/server/handler/HttpGremlinEndpointHandler.java +++ b/gremlin-server/src/main/java/org/apache/tinkerpop/gremlin/server/handler/HttpGremlinEndpointHandler.java @@ -180,7 +180,8 @@ public class HttpGremlinEndpointHandler extends SimpleChannelInboundHandler<Requ final Timer.Context timerContext = evalOpTimer.time(); // timeout override - handle both deprecated and newly named configuration. earlier logic should prevent // both configurations from being submitted at the same time - final long seto = requestCtx.getSettings().getEvaluationTimeout(); + Long timeoutMs = requestMessage.getField(Tokens.ARGS_EVAL_TIMEOUT); + final long seto = (null != timeoutMs) ? timeoutMs : requestCtx.getSettings().getEvaluationTimeout(); final FutureTask<Void> evalFuture = new FutureTask<>(() -> { requestCtx.setStartedResponse(); diff --git a/gremlin-server/src/main/java/org/apache/tinkerpop/gremlin/server/handler/HttpRequestMessageDecoder.java b/gremlin-server/src/main/java/org/apache/tinkerpop/gremlin/server/handler/HttpRequestMessageDecoder.java index cefc04f41f..1b261c3152 100644 --- a/gremlin-server/src/main/java/org/apache/tinkerpop/gremlin/server/handler/HttpRequestMessageDecoder.java +++ b/gremlin-server/src/main/java/org/apache/tinkerpop/gremlin/server/handler/HttpRequestMessageDecoder.java @@ -204,6 +204,8 @@ public class HttpRequestMessageDecoder extends MessageToMessageDecoder<FullHttpR final JsonNode scriptNode = body.get(Tokens.ARGS_GREMLIN); if (null == scriptNode) throw new IllegalArgumentException("no gremlin script supplied"); + final RequestMessageV4.Builder builder = RequestMessageV4.build(scriptNode.asText()); + final JsonNode bindingsNode = body.get(Tokens.ARGS_BINDINGS); if (bindingsNode != null && !bindingsNode.isObject()) throw new IllegalArgumentException("bindings must be a Map"); @@ -211,20 +213,23 @@ public class HttpRequestMessageDecoder extends MessageToMessageDecoder<FullHttpR final Map<String, Object> bindings = new HashMap<>(); if (bindingsNode != null) bindingsNode.fields().forEachRemaining(kv -> bindings.put(kv.getKey(), fromJsonNode(kv.getValue()))); + builder.addBindings(bindings); final JsonNode gNode = body.get(Tokens.ARGS_G); - final String g = (null == gNode) ? null : gNode.asText(); + if (null != gNode) builder.addG(gNode.asText()); final JsonNode languageNode = body.get(Tokens.ARGS_LANGUAGE); - final String language = null == languageNode ? "gremlin-groovy" : languageNode.asText(); + builder.addLanguage((null == languageNode) ? "gremlin-groovy" : languageNode.asText()); final JsonNode chunkSizeNode = body.get(Tokens.ARGS_BATCH_SIZE); - final Integer chunkSize = null == chunkSizeNode ? null : chunkSizeNode.asInt(); + if (null != chunkSizeNode) builder.addChunkSize(chunkSizeNode.asInt()); + + final JsonNode timeoutMsNode = body.get(Tokens.ARGS_EVAL_TIMEOUT); + if (null != timeoutMsNode) builder.addTimeoutMillis(timeoutMsNode.asLong()); + + final JsonNode matPropsNode = body.get(Tokens.ARGS_MATERIALIZE_PROPERTIES); + if (null != matPropsNode) builder.addMaterializeProperties(matPropsNode.asText()); - final RequestMessageV4.Builder builder = RequestMessageV4.build(scriptNode.asText()) - .addBindings(bindings).addLanguage(language); - if (null != g) builder.addG(g); - if (null != chunkSize) builder.addChunkSize(chunkSize); return builder.create(); } diff --git a/gremlin-server/src/test/java/org/apache/tinkerpop/gremlin/server/GremlinServerHttpIntegrateTest.java b/gremlin-server/src/test/java/org/apache/tinkerpop/gremlin/server/GremlinServerHttpIntegrateTest.java index bd7e85170b..96b09abc22 100644 --- a/gremlin-server/src/test/java/org/apache/tinkerpop/gremlin/server/GremlinServerHttpIntegrateTest.java +++ b/gremlin-server/src/test/java/org/apache/tinkerpop/gremlin/server/GremlinServerHttpIntegrateTest.java @@ -30,11 +30,17 @@ import org.apache.http.client.config.RequestConfig; import org.apache.http.conn.EofSensorInputStream; import org.apache.http.entity.ByteArrayEntity; import org.apache.http.impl.io.ChunkedInputStream; +import org.apache.tinkerpop.gremlin.driver.exception.ResponseException; import org.apache.tinkerpop.gremlin.process.traversal.Bytecode; +import org.apache.tinkerpop.gremlin.process.traversal.dsl.graph.GraphTraversalSource; +import org.apache.tinkerpop.gremlin.process.traversal.dsl.graph.__; import org.apache.tinkerpop.gremlin.server.handler.HttpBasicAuthenticationHandler; import org.apache.tinkerpop.gremlin.structure.util.empty.EmptyGraph; +import org.apache.tinkerpop.gremlin.util.Tokens; +import org.apache.tinkerpop.gremlin.util.function.Lambda; import org.apache.tinkerpop.gremlin.util.message.RequestMessageV4; import org.apache.tinkerpop.gremlin.util.message.ResponseMessage; +import org.apache.tinkerpop.gremlin.util.message.ResponseStatusCode; import org.apache.tinkerpop.gremlin.util.ser.GraphBinaryMessageSerializerV4; import org.apache.tinkerpop.gremlin.util.ser.GraphSONMessageSerializerV4; import org.apache.tinkerpop.gremlin.util.ser.GraphSONUntypedMessageSerializerV4; @@ -70,7 +76,12 @@ import java.util.concurrent.Future; import java.util.concurrent.ScheduledExecutorService; import java.util.concurrent.TimeUnit; +import static org.apache.tinkerpop.gremlin.process.traversal.AnonymousTraversalSource.traversal; import static org.apache.tinkerpop.gremlin.server.handler.HttpRequestIdHandler.REQUEST_ID_HEADER_NAME; +import static org.apache.tinkerpop.gremlin.util.Tokens.ARGS_EVAL_TIMEOUT; +import static org.apache.tinkerpop.gremlin.util.Tokens.ARGS_MATERIALIZE_PROPERTIES; +import static org.apache.tinkerpop.gremlin.util.Tokens.MATERIALIZE_PROPERTIES_ALL; +import static org.apache.tinkerpop.gremlin.util.Tokens.MATERIALIZE_PROPERTIES_TOKENS; import static org.hamcrest.MatcherAssert.assertThat; import static org.hamcrest.core.Is.is; import static org.hamcrest.core.IsInstanceOf.instanceOf; @@ -78,7 +89,10 @@ import static org.hamcrest.core.StringRegularExpression.matchesRegex; import static org.hamcrest.core.StringStartsWith.startsWith; import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertNotEquals; +import static org.junit.Assert.assertNotNull; +import static org.junit.Assert.assertNull; import static org.junit.Assert.assertTrue; +import static org.junit.Assert.fail; /** * Integration tests for server-side settings and processing. @@ -1058,7 +1072,7 @@ public class GremlinServerHttpIntegrateTest extends AbstractGremlinServerIntegra firstPost.setEntity(new StringEntity("{\"gremlin\":\"g.V().repeat(__.out()).until(__.outE().count().is(0)).iterate()\"}", Consts.UTF_8)); // Add a shorter timeout to the second query to ensure that its timeout is less than the first query's running time. final HttpPost secondPost = new HttpPost(TestClientFactory.createURLString()); - secondPost.setEntity(new StringEntity("{\"gremlin\":\"g.with('evaluationTimeout',1000).V().repeat(__.out()).until(__.outE().count().is(0)).iterate()\"}", Consts.UTF_8)); + secondPost.setEntity(new StringEntity("{\"gremlin\":\"g.V().repeat(__.out()).until(__.outE().count().is(0)).iterate()\"}", Consts.UTF_8)); final Callable<Integer> firstQueryWrapper = () -> { try (final CloseableHttpResponse response = firstClient.execute(firstPost)) { @@ -1416,6 +1430,58 @@ public class GremlinServerHttpIntegrateTest extends AbstractGremlinServerIntegra } } + @Test + public void shouldAcceptTimeoutInRequestBody() throws Exception { + final String body = "{ \"gremlin\": \"" + "Thread.sleep(5000)" + "\",\"language\":\"gremlin-groovy\",\"" + ARGS_EVAL_TIMEOUT + "\":\"100\"}"; + final CloseableHttpClient httpclient = HttpClients.createDefault(); + final HttpPost httppost = new HttpPost(TestClientFactory.createURLString()); + httppost.addHeader("Content-Type", "application/json"); + httppost.setEntity(new StringEntity(body, Consts.UTF_8)); + + try (final CloseableHttpResponse response = httpclient.execute(httppost)) { + assertEquals(200, response.getStatusLine().getStatusCode()); + + final String json = EntityUtils.toString(response.getEntity()); + assertTrue(json.contains("timeout occurred")); + } + } + + @Test + public void shouldAcceptMaterializePropertiesAllInRequestBody() throws Exception { + final String body = "{ \"gremlin\": \"" + "gmodern.V().limit(1)" + "\",\"language\":\"gremlin-groovy\",\"" + + ARGS_MATERIALIZE_PROPERTIES + "\":\"" + MATERIALIZE_PROPERTIES_ALL + "\"}"; + final CloseableHttpClient httpclient = HttpClients.createDefault(); + final HttpPost httppost = new HttpPost(TestClientFactory.createURLString()); + httppost.addHeader("Content-Type", "application/json"); + httppost.setEntity(new StringEntity(body, Consts.UTF_8)); + + try (final CloseableHttpResponse response = httpclient.execute(httppost)) { + assertEquals(200, response.getStatusLine().getStatusCode()); + + final String json = EntityUtils.toString(response.getEntity()); + final JsonNode node = mapper.readTree(json); + assertNotNull(node.get("result").get(GraphSONTokens.VALUEPROP).get(0).get(GraphSONTokens.VALUEPROP).get(GraphSONTokens.PROPERTIES)); + } + } + + @Test + public void shouldAcceptMaterializePropertiesTokensInRequestBody() throws Exception { + final String body = "{ \"gremlin\": \"" + "gmodern.V().limit(1)" + "\",\"language\":\"gremlin-groovy\",\"" + + ARGS_MATERIALIZE_PROPERTIES + "\":\"" + MATERIALIZE_PROPERTIES_TOKENS + "\"}"; + final CloseableHttpClient httpclient = HttpClients.createDefault(); + final HttpPost httppost = new HttpPost(TestClientFactory.createURLString()); + httppost.addHeader("Content-Type", "application/json"); + httppost.setEntity(new StringEntity(body, Consts.UTF_8)); + + try (final CloseableHttpResponse response = httpclient.execute(httppost)) { + assertEquals(200, response.getStatusLine().getStatusCode()); + + final String json = EntityUtils.toString(response.getEntity()); + final JsonNode node = mapper.readTree(json); + assertNull(node.get("result").get(GraphSONTokens.VALUEPROP).get(0).get(GraphSONTokens.VALUEPROP).get(GraphSONTokens.PROPERTIES)); + } + } + private static ByteBuf toByteBuf(final HttpEntity httpEntity) throws IOException { final byte[] asArray = EntityUtils.toByteArray(httpEntity); return Unpooled.wrappedBuffer(asArray); diff --git a/gremlin-server/src/test/java/org/apache/tinkerpop/gremlin/server/GremlinServerIntegrateTest.java b/gremlin-server/src/test/java/org/apache/tinkerpop/gremlin/server/GremlinServerIntegrateTest.java index 8f5d77efd5..5e530634b2 100644 --- a/gremlin-server/src/test/java/org/apache/tinkerpop/gremlin/server/GremlinServerIntegrateTest.java +++ b/gremlin-server/src/test/java/org/apache/tinkerpop/gremlin/server/GremlinServerIntegrateTest.java @@ -77,9 +77,11 @@ import static org.apache.tinkerpop.gremlin.groovy.jsr223.GroovyCompilerGremlinPl import static org.apache.tinkerpop.gremlin.process.remote.RemoteConnection.GREMLIN_REMOTE; import static org.apache.tinkerpop.gremlin.process.remote.RemoteConnection.GREMLIN_REMOTE_CONNECTION_CLASS; import static org.apache.tinkerpop.gremlin.process.traversal.AnonymousTraversalSource.traversal; +import static org.apache.tinkerpop.gremlin.util.Tokens.ARGS_EVAL_TIMEOUT; import static org.hamcrest.CoreMatchers.containsString; import static org.hamcrest.CoreMatchers.is; import static org.hamcrest.MatcherAssert.assertThat; +import static org.hamcrest.core.AllOf.allOf; import static org.hamcrest.core.IsInstanceOf.instanceOf; import static org.hamcrest.core.StringStartsWith.startsWith; import static org.junit.Assert.assertEquals; @@ -172,10 +174,9 @@ public class GremlinServerIntegrateTest extends AbstractGremlinServerIntegration case "shouldReturnInvalidRequestArgsWhenBindingCountExceedsAllowable": settings.maxParameters = 1; break; -// TODO: evalTimeout needs be part of script now -// case "shouldTimeOutRemoteTraversal": -// settings.evaluationTimeout = 500; -// break; + case "shouldTimeOutRemoteTraversal": + settings.evaluationTimeout = 500; + break; case "shouldBlowTheWorkQueueSize": settings.gremlinPool = 1; settings.maxWorkQueueSize = 1; @@ -334,102 +335,99 @@ public class GremlinServerIntegrateTest extends AbstractGremlinServerIntegration g.close(); } -// TODO: evalTimeout needs be part of script now -// @Test -// public void shouldTimeOutRemoteTraversal() throws Exception { -// final GraphTraversalSource g = traversal().withRemote(conf); -// -// try { -// // tests sleeping thread -// g.inject(1).sideEffect(Lambda.consumer("Thread.sleep(20000)")).iterate(); -// fail("This traversal should have timed out"); -// } catch (Exception ex) { -// final Throwable t = ex.getCause(); -// assertThat(t, instanceOf(ResponseException.class)); -// assertEquals(ResponseStatusCode.SERVER_ERROR_TIMEOUT, ((ResponseException) t).getResponseStatusCode()); -// } -// -// // make a graph with a cycle in it to force a long run traversal -// graphGetter.get().traversal().addV("person").as("p").addE("self").to("p").iterate(); -// -// try { -// // tests an "unending" traversal -// g.V().repeat(__.out()).until(__.outE().count().is(0)).iterate(); -// fail("This traversal should have timed out"); -// } catch (Exception ex) { -// final Throwable t = ex.getCause(); -// assertThat(t, instanceOf(ResponseException.class)); -// assertEquals(ResponseStatusCode.SERVER_ERROR_TIMEOUT, ((ResponseException) t).getResponseStatusCode()); -// } -// -// g.close(); -// } + @Test + public void shouldTimeOutRemoteTraversal() throws Exception { + final GraphTraversalSource g = traversal().withRemote(conf); -// TODO: driver can't set EvalTimeout anymore in RequestMessage so this won't work. -// @Test -// public void shouldTimeOutRemoteTraversalWithPerRequestOption() throws Exception { -// final GraphTraversalSource g = traversal().withRemote(conf); -// -// try { -// // tests sleeping thread -// g.with(ARGS_EVAL_TIMEOUT, 500L).inject(1).sideEffect(Lambda.consumer("Thread.sleep(10000)")).iterate(); -// fail("This traversal should have timed out"); -// } catch (Exception ex) { -// final Throwable t = ex.getCause(); -// assertThat(t, instanceOf(ResponseException.class)); -// assertEquals(ResponseStatusCode.SERVER_ERROR_TIMEOUT, ((ResponseException) t).getResponseStatusCode()); -// } -// -// // make a graph with a cycle in it to force a long run traversal -// graphGetter.get().traversal().addV("person").as("p").addE("self").to("p").iterate(); -// -// try { -// // tests an "unending" traversal -// g.with(ARGS_EVAL_TIMEOUT, 500L).V().repeat(__.out()).until(__.outE().count().is(0)).iterate(); -// fail("This traversal should have timed out"); -// } catch (Exception ex) { -// final Throwable t = ex.getCause(); -// assertThat(t, instanceOf(ResponseException.class)); -// assertEquals(ResponseStatusCode.SERVER_ERROR_TIMEOUT, ((ResponseException) t).getResponseStatusCode()); -// } -// -// g.close(); -// } + try { + // tests sleeping thread + g.inject(1).sideEffect(Lambda.consumer("Thread.sleep(10000)")).iterate(); + fail("This traversal should have timed out"); + } catch (Exception ex) { + final Throwable t = ex.getCause(); + assertThat(t, instanceOf(ResponseException.class)); + assertEquals(ResponseStatusCode.SERVER_ERROR_TIMEOUT, ((ResponseException) t).getResponseStatusCode()); + } -// TODO: can't set evalTimeout anymore -// @Test -// public void shouldProduceProperExceptionOnTimeout() throws Exception { -// final Cluster cluster = TestClientFactory.open(); -// final Client client = cluster.connect(name.getMethodName()); -// -// boolean success = false; -// // Run a short test script a few times with progressively longer timeouts. -// // Each submissions should either succeed or fail with a timeout. -// // Note: the range of timeouts is intended to cover the case when the script finishes at about the -// // same time when the timeout occurs. In this situation either a timeout response or a successful -// // response is acceptable, however no other processing errors should occur. -// // Note: the timeout of 30 ms is generally sufficient for running a simple groovy script, so using longer -// // timeouts are not likely to results in a success/timeout response collision, which is the purpose -// // of this test. -// // Note: this test may have a false negative result, but a failure would indicate a real problem. -// for(int i = 0; i < 30; i++) { -// int timeout = 1 + i; -// overrideEvaluationTimeout(timeout); -// -// try { -// client.submit("x = 1 + 1").all().get().get(0).getInt(); -// success = true; -// } catch (Exception ex) { -// final Throwable t = ex.getCause(); -// assertThat("Unexpected exception with script evaluation timeout: " + timeout, t, instanceOf(ResponseException.class)); -// assertEquals(ResponseStatusCode.SERVER_ERROR_TIMEOUT, ((ResponseException) t).getResponseStatusCode()); -// } -// } -// -// assertTrue("Some script submissions should succeed", success); -// -// cluster.close(); -// } + // make a graph with a cycle in it to force a long run traversal + graphGetter.get().traversal().addV("person").as("p").addE("self").to("p").iterate(); + + try { + // tests an "unending" traversal + g.V().repeat(__.out()).until(__.outE().count().is(0)).iterate(); + fail("This traversal should have timed out"); + } catch (Exception ex) { + final Throwable t = ex.getCause(); + assertThat(t, instanceOf(ResponseException.class)); + assertEquals(ResponseStatusCode.SERVER_ERROR_TIMEOUT, ((ResponseException) t).getResponseStatusCode()); + } + + g.close(); + } + + @Test + public void shouldTimeOutRemoteTraversalWithPerRequestOption() throws Exception { + final GraphTraversalSource g = traversal().withRemote(conf); + + try { + // tests sleeping thread + g.with(ARGS_EVAL_TIMEOUT, 500L).inject(1).sideEffect(Lambda.consumer("Thread.sleep(10000)")).iterate(); + fail("This traversal should have timed out"); + } catch (Exception ex) { + final Throwable t = ex.getCause(); + assertThat(t, instanceOf(ResponseException.class)); + assertEquals(ResponseStatusCode.SERVER_ERROR_TIMEOUT, ((ResponseException) t).getResponseStatusCode()); + } + + // make a graph with a cycle in it to force a long run traversal + graphGetter.get().traversal().addV("person").as("p").addE("self").to("p").iterate(); + + try { + // tests an "unending" traversal + g.with(ARGS_EVAL_TIMEOUT, 500L).V().repeat(__.out()).until(__.outE().count().is(0)).iterate(); + fail("This traversal should have timed out"); + } catch (Exception ex) { + final Throwable t = ex.getCause(); + assertThat(t, instanceOf(ResponseException.class)); + assertEquals(ResponseStatusCode.SERVER_ERROR_TIMEOUT, ((ResponseException) t).getResponseStatusCode()); + } + + g.close(); + } + + @Test + public void shouldProduceProperExceptionOnTimeout() throws Exception { + final Cluster cluster = TestClientFactory.open(); + final Client client = cluster.connect(name.getMethodName()); + + boolean success = false; + // Run a short test script a few times with progressively longer timeouts. + // Each submissions should either succeed or fail with a timeout. + // Note: the range of timeouts is intended to cover the case when the script finishes at about the + // same time when the timeout occurs. In this situation either a timeout response or a successful + // response is acceptable, however no other processing errors should occur. + // Note: the timeout of 30 ms is generally sufficient for running a simple groovy script, so using longer + // timeouts are not likely to results in a success/timeout response collision, which is the purpose + // of this test. + // Note: this test may have a false negative result, but a failure would indicate a real problem. + for(int i = 0; i < 30; i++) { + int timeout = 1 + i; + overrideEvaluationTimeout(timeout); + + try { + client.submit("x = 1 + 1").all().get().get(0).getInt(); + success = true; + } catch (Exception ex) { + final Throwable t = ex.getCause(); + assertThat("Unexpected exception with script evaluation timeout: " + timeout, t, instanceOf(ResponseException.class)); + assertEquals(ResponseStatusCode.SERVER_ERROR_TIMEOUT, ((ResponseException) t).getResponseStatusCode()); + } + } + + assertTrue("Some script submissions should succeed", success); + + cluster.close(); + } @Test public void shouldUseBaseScript() throws Exception { @@ -719,22 +717,20 @@ public class GremlinServerIntegrateTest extends AbstractGremlinServerIntegration } } - // TODO: evalTimeout no longer set in RequestMessage -// @Test -// @SuppressWarnings("unchecked") -// public void shouldReceiveFailureTimeOutOnEvalUsingOverride() throws Exception { -// try (SimpleClient client = TestClientFactory.createWebSocketClient()) { -// final RequestMessageV4 msg = RequestMessageV4.build("eval") -// .addArg(Tokens.ARGS_EVAL_TIMEOUT, 100L) -// .addArg(Tokens.ARGS_GREMLIN, "Thread.sleep(3000);'some-stuff-that-should not return'") -// .create(); -// final List<ResponseMessage> responses = client.submit(msg); -// assertThat(responses.get(0).getStatus().getMessage(), allOf(startsWith("Evaluation exceeded"), containsString("100 ms"))); -// -// // validate that we can still send messages to the server -// assertEquals(2, ((List<Integer>) client.submit("1+1").get(0).getResult().getData()).get(0).intValue()); -// } -// } + @Test + @SuppressWarnings("unchecked") + public void shouldReceiveFailureTimeOutOnEvalUsingOverride() throws Exception { + try (SimpleClient client = TestClientFactory.createSimpleHttpClient()) { + final RequestMessageV4 msg = RequestMessageV4.build("Thread.sleep(3000);'some-stuff-that-should not return'") + .addTimeoutMillis(100L) + .create(); + final List<ResponseMessage> responses = client.submit(msg); + assertThat(responses.get(0).getStatus().getMessage(), allOf(startsWith("Evaluation exceeded"), containsString("100 ms"))); + + // validate that we can still send messages to the server + assertEquals(2, ((List<Integer>) client.submit("1+1").get(0).getResult().getData()).get(0).intValue()); + } + } @Test public void shouldReceiveFailureTimeOutOnScriptEvalOfOutOfControlLoop() throws Exception { diff --git a/gremlin-util/src/main/java/org/apache/tinkerpop/gremlin/util/message/RequestMessageV4.java b/gremlin-util/src/main/java/org/apache/tinkerpop/gremlin/util/message/RequestMessageV4.java index 65c2b0029b..a0afeb5779 100644 --- a/gremlin-util/src/main/java/org/apache/tinkerpop/gremlin/util/message/RequestMessageV4.java +++ b/gremlin-util/src/main/java/org/apache/tinkerpop/gremlin/util/message/RequestMessageV4.java @@ -181,6 +181,24 @@ public final class RequestMessageV4 { return this; } + public Builder addMaterializeProperties(final String materializeProps) { + Objects.requireNonNull(materializeProps, "materializeProps argument cannot be null."); + if (!materializeProps.equals(Tokens.MATERIALIZE_PROPERTIES_TOKENS) && !materializeProps.equals(Tokens.MATERIALIZE_PROPERTIES_ALL)) { + throw new IllegalArgumentException("materializeProperties argument must be either token or all."); + } + + this.fields.put(Tokens.ARGS_MATERIALIZE_PROPERTIES, materializeProps); + return this; + } + + public Builder addTimeoutMillis(final long timeout) { + Objects.requireNonNull(timeout, "timeout argument cannot be null."); + if (timeout < 0) throw new IllegalArgumentException("timeout argument cannot be negative."); + + this.fields.put(Tokens.ARGS_EVAL_TIMEOUT, timeout); + return this; + } + /** * Create the request message given the settings provided to the {@link Builder}. */ diff --git a/gremlin-util/src/test/java/org/apache/tinkerpop/gremlin/util/message/RequestMessageV4Test.java b/gremlin-util/src/test/java/org/apache/tinkerpop/gremlin/util/message/RequestMessageV4Test.java index 5b17aabde6..d121524f72 100644 --- a/gremlin-util/src/test/java/org/apache/tinkerpop/gremlin/util/message/RequestMessageV4Test.java +++ b/gremlin-util/src/test/java/org/apache/tinkerpop/gremlin/util/message/RequestMessageV4Test.java @@ -83,6 +83,32 @@ public class RequestMessageV4Test { assertEquals(g, msg.getField(Tokens.ARGS_G)); } + @Test + public void shouldSetTimeout() { + final long timeout = 101L; + final RequestMessageV4 msg = RequestMessageV4.build("g").addTimeoutMillis(timeout).create(); + assertEquals(timeout, (long) msg.getField(Tokens.ARGS_EVAL_TIMEOUT)); + } + + @Test + public void shouldSetMaterializeProperties() { + final RequestMessageV4 msgWithAll = RequestMessageV4.build("g").addMaterializeProperties(Tokens.MATERIALIZE_PROPERTIES_ALL).create(); + assertEquals(Tokens.MATERIALIZE_PROPERTIES_ALL, msgWithAll.getField(Tokens.ARGS_MATERIALIZE_PROPERTIES)); + + final RequestMessageV4 msgWithTokens = RequestMessageV4.build("g").addMaterializeProperties(Tokens.MATERIALIZE_PROPERTIES_TOKENS).create(); + assertEquals(Tokens.MATERIALIZE_PROPERTIES_TOKENS, msgWithTokens.getField(Tokens.ARGS_MATERIALIZE_PROPERTIES)); + } + + @Test + public void shouldErrorSettingMaterializePropertiesWithInvalidValue() { + try { + final RequestMessageV4 msgWithTokens = RequestMessageV4.build("g").addMaterializeProperties("notToken").create(); + fail("RequestMessage shouldn't accept notToken for materializeProperties."); + } catch (Exception e) { + assertTrue(e.getMessage().contains("materializeProperties argument must be either token or all")); + } + } + @Test public void shouldGetFields() { final String g = "gmodern";