This is an automated email from the ASF dual-hosted git repository. kenhuuu pushed a commit to branch per-request in repository https://gitbox.apache.org/repos/asf/tinkerpop.git
commit 19da72f56337e9bb602315d0bc56f9f67631bffc Author: Ken Hu <[email protected]> AuthorDate: Thu Jul 16 14:42:08 2026 -0700 Honor per-request options embedded in a script via with() The server now applies per-request options set inside a submitted Gremlin string's with(): evaluationTimeout/timeoutMillis, materializeProperties, batchSize, bulkResults, and language, with a single precedence: script embedded with() > request field > (header, bulkResults only) > default. This fixes a regression where a script embedded timeoutMillis was silently ignored on the raw string path. Options are resolved once in Context; batchSize is parsed and validated there, returning 400 Bad Request for a non positive or out of range value rather than stalling result iteration or surfacing an uncaught 500. Validation runs before any transaction begin side effects. g, parameters, and transactionId remain request field/header only, and the obsolete requestId scrape is removed from GremlinScriptChecker. Assisted-by: Claude Code:claude-opus-4-8 --- CHANGELOG.asciidoc | 1 + docs/src/dev/provider/index.asciidoc | 12 +- .../gremlin/jsr223/GremlinScriptChecker.java | 118 +++++++++---- .../gremlin/jsr223/GremlinScriptCheckerTest.java | 188 +++++++++++++-------- .../apache/tinkerpop/gremlin/server/Context.java | 111 ++++++++++-- .../server/handler/HttpGremlinEndpointHandler.java | 45 +++-- .../gremlin/server/util/GremlinError.java | 6 + .../tinkerpop/gremlin/server/ContextTest.java | 142 +++++++++++++++- .../server/GremlinServerHttpIntegrateTest.java | 136 +++++++++++++++ .../jsr223/GremlinScriptCheckerBenchmark.java | 6 +- 10 files changed, 619 insertions(+), 146 deletions(-) diff --git a/CHANGELOG.asciidoc b/CHANGELOG.asciidoc index c0f496c0e5..44b9ad749f 100644 --- a/CHANGELOG.asciidoc +++ b/CHANGELOG.asciidoc @@ -105,6 +105,7 @@ image::https://raw.githubusercontent.com/apache/tinkerpop/master/docs/static/ima * Renamed the request `bindings` field to `parameters` across the HTTP request body, GraphBinary, and GraphSON, standardizing terminology for query parameters. * Modified HTTP API to expect gremlin-lang strings for parameters and update all GLVs to send requests in new format. * Added string parameter parsing to `GremlinServer` to prevent traversal injection and excessive nesting depths. +* Modified how the server honors per-request options embedded in a script via `with()`, extending `GremlinScriptChecker` to recognize `language`, `batchSize`, and `bulkResults` (in addition to `timeoutMillis` and `materializeProperties`) with uniform precedence. * Modified all GLVs to detect unsupported types in `GremlinLang` and throw consistent error for that case. * Extended `GValue` parameterization to all GLVs (`gremlin-python`, `gremlin-go`, `gremlin-javascript`, `gremlin-dotnet`), allowing parameters to be passed directly in traversals as in Java. * Relaxed `GValue` name validation by removing the reserved leading underscore restriction. diff --git a/docs/src/dev/provider/index.asciidoc b/docs/src/dev/provider/index.asciidoc index 773d5d6221..f2f505c368 100644 --- a/docs/src/dev/provider/index.asciidoc +++ b/docs/src/dev/provider/index.asciidoc @@ -1261,11 +1261,21 @@ the serializer specified by the `Content-Type` header. The following are the key |parameters |A gremlin-lang string that encodes a map of key/value pairs used during query execution. Its usage depends on "language". For "gremlin-groovy", these are applied as script variable bindings. For "gremlin-lang", these are the query parameters. |String containing a gremlin-lang map literal |No |g |The name of the graph traversal source to which the query applies. Default: "g" |String containing traversal source name |No |language |The name of the ScriptEngine to use to parse the gremlin query. Default: "gremlin-lang" |String containing ScriptEngine name |No +|batchSize |The number of results to include in each response chunk, overriding the server configured `resultIterationBatchSize`. |Number between 1 and 2^31-1 |No |materializeProperties |Whether to include all properties for results. One of "tokens" or "all". |String |No |bulkResults |Whether the results should be bulked by the server (only applies to GraphBinary) |Boolean |No |transactionId |A server-generated UUID that identifies an active transaction. Must be included in all non-begin requests within that transaction. Omit for non-transactional requests and for the initial begin request. |String |No |========================================================= +Several of these options may alternatively be supplied inside the Gremlin query itself using the `with()` source step +(for example `g.with('timeoutMillis', 500).V()`) when submitting a script. The server recognizes the following options +this way: `timeoutMillis`, `materializeProperties`, `batchSize`, `bulkResults`, and `language`. When the same option is +supplied both in the query via `with()` and as a request field, the value in the query takes precedence. The full +precedence order is: value embedded in the query via `with()`, then the request field, then the server/connection +default. The `parameters`, `transactionId`, `g`, and `gremlin` keys cannot be set through `with()` and must be supplied +as request fields; `g` is excluded because of the complex interactions between different script engines and +transactions. + ==== HTTP Response When Gremlin Server receives that request, it will decode it given the "mime type", and execute it using the @@ -2033,4 +2043,4 @@ in use with the Gremlin Console plugin host. Simply instantiate and return a `R include::gremlin-semantics.asciidoc[] -include::policies.asciidoc[] \ No newline at end of file +include::policies.asciidoc[] diff --git a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/jsr223/GremlinScriptChecker.java b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/jsr223/GremlinScriptChecker.java index 1879669f26..53b9eb0cbc 100644 --- a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/jsr223/GremlinScriptChecker.java +++ b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/jsr223/GremlinScriptChecker.java @@ -36,14 +36,16 @@ public class GremlinScriptChecker { /** * An empty result whose properties return as empty. */ - public static final Result EMPTY_RESULT = new Result(null, null, null); + public static final Result EMPTY_RESULT = new Result(null, null, null, null, null); /** * At least one of these tokens should be present somewhere in the Gremlin string for {@link #parse(String)} to * take any action at all. */ private static final Set<String> tokens = new HashSet<>(Arrays.asList("timeoutMillis", "TIMEOUT_MILLIS", - "requestId", "REQUEST_ID", "materializeProperties", "ARGS_MATERIALIZE_PROPERTIES")); + "materializeProperties", "ARGS_MATERIALIZE_PROPERTIES", + "language", "ARGS_LANGUAGE", "batchSize", "ARGS_BATCH_SIZE", + "bulkResults", "BULK_RESULTS")); /** * Matches single line comments, multi-line comments and space characters. @@ -85,23 +87,40 @@ public class GremlinScriptChecker { private static final String timeoutTokens = "[\"']timeoutMillis[\"']|(?:Tokens\\.)?TIMEOUT_MILLIS"; /** - * Regex fragment for the timeout tokens to look for. There are basically four: + * Regex fragment for the materializeProperties to look for. There are basically four: * <ul> - * <li>{@code requestId} which is a string value and thus single or double quoted</li> - * <li>{@code REQUEST_ID} which is a enum type of value which can be referenced with or without a {@code Tokens} qualifier</li> + * <li>{@code materializeProperties} which is a string value and thus single or double quoted</li> + * <li>{@code ARGS_MATERIALIZE_PROPERTIES} which is a enum type of value which can be referenced with or without a {@code Tokens} qualifier</li> * </ul> - * See {@link #patternWithOptions} for a full explain as this regex is embedded in there. */ - private static final String requestIdTokens = "[\"']requestId[\"']|(?:Tokens\\.)?REQUEST_ID"; + private static final String materializePropertiesTokens = "[\"']materializeProperties[\"']|(?:Tokens\\.)?ARGS_MATERIALIZE_PROPERTIES"; /** - * Regex fragment for the materializeProperties to look for. There are basically four: + * Regex fragment for the {@code language} tokens to look for: * <ul> - * <li>{@code materializeProperties} which is a string value and thus single or double quoted</li> - * <li>{@code ARGS_MATERIALIZE_PROPERTIES} which is a enum type of value which can be referenced with or without a {@code Tokens} qualifier</li> + * <li>{@code language} which is a string value and thus single or double quoted</li> + * <li>{@code ARGS_LANGUAGE} which can be referenced with or without a {@code Tokens} qualifier</li> * </ul> */ - private static final String materializePropertiesTokens = "[\"']materializeProperties[\"']|(?:Tokens\\.)?ARGS_MATERIALIZE_PROPERTIES"; + private static final String languageTokens = "[\"']language[\"']|(?:Tokens\\.)?ARGS_LANGUAGE"; + + /** + * Regex fragment for the {@code batchSize} tokens to look for: + * <ul> + * <li>{@code batchSize} which is a string value and thus single or double quoted</li> + * <li>{@code ARGS_BATCH_SIZE} which can be referenced with or without a {@code Tokens} qualifier</li> + * </ul> + */ + private static final String batchSizeTokens = "[\"']batchSize[\"']|(?:Tokens\\.)?ARGS_BATCH_SIZE"; + + /** + * Regex fragment for the {@code bulkResults} tokens to look for: + * <ul> + * <li>{@code bulkResults} which is a string value and thus single or double quoted</li> + * <li>{@code BULK_RESULTS} which can be referenced with or without a {@code Tokens} qualifier</li> + * </ul> + */ + private static final String bulkResultsTokens = "[\"']bulkResults[\"']|(?:Tokens\\.)?BULK_RESULTS"; /** * Matches {@code .with({timeout-token},{timeout})} with a matching group on the {@code timeout}. @@ -110,7 +129,7 @@ public class GremlinScriptChecker { * From https://regex101.com/ * * \.with\((((?:["']materializeProperties["']|["']scriptEvaluationTimeout["']|(?:Tokens\.)?ARGS_EVAL_TIMEOUT|(?:Tokens\.)?ARGS_SCRIPT_EVAL_TIMEOUT),(?<to>\d*)(:?L|l)?)|((?:["']materializeProperties["']|(?:Tokens\.)?ARGS_MATERIALIZE_PROPERTIES),["'](?<mp>.*?)["']?)|((?:["']requestId["']|(?:Tokens\.)?REQUEST_ID),["'](?<rid>.*?)["']))\) - * + * * gm * \. matches the character . with index 4610 (2E16 or 568) literally (case sensitive) * with matches the characters with literally (case sensitive) @@ -178,7 +197,9 @@ public class GremlinScriptChecker { Pattern.compile("\\.with\\((((?:" + timeoutTokens + "),(?<to>\\d*)(:?L|l)?)|((?:" + materializePropertiesTokens + "),[\"'](?<mp>.*?)[\"']?)|((?:" - + requestIdTokens + "),[\"'](?<rid>.*?)[\"']))\\)"); + + languageTokens + "),[\"'](?<lang>.*?)[\"'])|((?:" + + batchSizeTokens + "),(?<bs>\\d+))|((?:" + + bulkResultsTokens + "),(?<br>true|false)))\\)"); /** * Parses a Gremlin script and extracts a {@code Result} containing properties that are relevant to the checker. @@ -199,8 +220,10 @@ public class GremlinScriptChecker { // arguments given to Result class as null mean they weren't assigned (or the parser didn't find them somehow - eek!) Long timeout = null; - String requestId = null; String materializeProperties = null; + String language = null; + String batchSize = null; + Boolean bulkResults = null; do { // timeout is added up across all scripts final String to = m.group("to"); @@ -209,16 +232,26 @@ public class GremlinScriptChecker { timeout += Long.parseLong(to); } - // request id just uses the last one found - final String rid = m.group("rid"); - if (rid != null) requestId = rid; - //materializeProperties just uses the last one found final String mp = m.group("mp"); if (mp != null) materializeProperties = mp; + + // language just uses the last one found + final String lang = m.group("lang"); + if (lang != null) language = lang; + + // batchSize just uses the last one found. it is captured as the raw string (not parsed to a number) so + // that an out-of-range value does not throw here in the Context constructor - the server parses and + // validates it where a bad value can be surfaced as a bad request rather than an uncaught error. + final String bs = m.group("bs"); + if (bs != null) batchSize = bs; + + // bulkResults just uses the last one found + final String br = m.group("br"); + if (br != null) bulkResults = Boolean.parseBoolean(br); } while (m.find()); - return new Result(timeout, requestId, materializeProperties); + return new Result(timeout, materializeProperties, language, batchSize, bulkResults); } /** @@ -226,13 +259,18 @@ public class GremlinScriptChecker { */ public static class Result { private final Long timeout; - private final String requestId; private final String materializeProperties; + private final String language; + private final String batchSize; + private final Boolean bulkResults; - private Result(final Long timeout, final String requestId, final String materializeProperties) { + private Result(final Long timeout, final String materializeProperties, + final String language, final String batchSize, final Boolean bulkResults) { this.timeout = timeout; - this.requestId = requestId; this.materializeProperties = materializeProperties; + this.language = language; + this.batchSize = batchSize; + this.bulkResults = bulkResults; } /** @@ -244,27 +282,47 @@ public class GremlinScriptChecker { } /** - * Gets the value of the request identifier supplied using the {@link GraphTraversal#with(String, Object)} source step. - * If there are multiple commands using this step, the last usage should represent the id returned here. + * Gets the value of the materializeProperties supplied using the {@link GraphTraversal#with(String, Object)} source step. + * If there are multiple commands using this step, the last usage should represent the value returned here. */ - public Optional<String> getRequestId() { - return null == requestId ? Optional.empty() : Optional.of(requestId); + public Optional<String> getMaterializeProperties() { + return null == materializeProperties ? Optional.empty() : Optional.of(materializeProperties); } /** - * Gets the value of the materializeProperties supplied using the {@link GraphTraversal#with(String, Object)} source step. + * Gets the value of the language supplied using the {@link GraphTraversal#with(String, Object)} source step. * If there are multiple commands using this step, the last usage should represent the value returned here. */ - public Optional<String> getMaterializeProperties() { - return null == materializeProperties ? Optional.empty() : Optional.of(materializeProperties); + public Optional<String> getLanguage() { + return null == language ? Optional.empty() : Optional.of(language); + } + + /** + * Gets the raw, unparsed value of the batchSize supplied using the {@link GraphTraversal#with(String, Object)} + * source step. It is returned as the raw string (rather than a parsed number) so that parsing and range + * validation happen where an invalid value can be surfaced as a bad request rather than an uncaught error. + * If there are multiple commands using this step, the last usage should represent the value returned here. + */ + public Optional<String> getBatchSize() { + return null == batchSize ? Optional.empty() : Optional.of(batchSize); + } + + /** + * Gets the value of the bulkResults flag supplied using the {@link GraphTraversal#with(String, Object)} source step. + * If there are multiple commands using this step, the last usage should represent the value returned here. + */ + public Optional<Boolean> getBulkResults() { + return null == bulkResults ? Optional.empty() : Optional.of(bulkResults); } @Override public String toString() { return "GremlinScriptChecker.Result{" + "timeout=" + timeout + - ", requestId='" + requestId + '\'' + ", materializeProperties='" + materializeProperties + '\'' + + ", language='" + language + '\'' + + ", batchSize=" + batchSize + + ", bulkResults=" + bulkResults + '}'; } } diff --git a/gremlin-core/src/test/java/org/apache/tinkerpop/gremlin/jsr223/GremlinScriptCheckerTest.java b/gremlin-core/src/test/java/org/apache/tinkerpop/gremlin/jsr223/GremlinScriptCheckerTest.java index f55660d75e..bd8aa88d4c 100644 --- a/gremlin-core/src/test/java/org/apache/tinkerpop/gremlin/jsr223/GremlinScriptCheckerTest.java +++ b/gremlin-core/src/test/java/org/apache/tinkerpop/gremlin/jsr223/GremlinScriptCheckerTest.java @@ -34,7 +34,6 @@ public class GremlinScriptCheckerTest { public void shouldNotFindAResult() { final GremlinScriptChecker.Result r = GremlinScriptChecker.parse("g.with(true).V().out('knows')"); assertEquals(Optional.empty(), r.getTimeout()); - assertEquals(Optional.empty(), r.getRequestId()); assertEquals(Optional.empty(), r.getMaterializeProperties()); } @@ -43,7 +42,6 @@ public class GremlinScriptCheckerTest { final GremlinScriptChecker.Result r = GremlinScriptChecker.parse(""); assertSame(EMPTY_RESULT, r); assertEquals(Optional.empty(), r.getTimeout()); - assertEquals(Optional.empty(), r.getRequestId()); assertEquals(Optional.empty(), r.getMaterializeProperties()); } @@ -64,24 +62,6 @@ public class GremlinScriptCheckerTest { getTimeout().get().longValue()); } - @Test - public void shouldIdentifyRequestIdWithOddSpacing() { - assertEquals("4F53FB59-CFC9-4984-B477-452073A352FD", GremlinScriptChecker.parse("g.with('requestId' , '4F53FB59-CFC9-4984-B477-452073A352FD').with(true).V().out('knows')"). - getRequestId().get()); - assertEquals("4F53FB59-CFC9-4984-B477-452073A352FD", GremlinScriptChecker.parse("g.with('requestId' , '4F53FB59-CFC9-4984-B477-452073A352FD').with(true).V().out('knows')"). - getRequestId().get()); - assertEquals("4F53FB59-CFC9-4984-B477-452073A352FD", GremlinScriptChecker.parse("g.with('requestId','4F53FB59-CFC9-4984-B477-452073A352FD').with(true).V().out('knows')"). - getRequestId().get()); - } - - @Test - public void shouldIdentifyRequestIdWithEmbeddedQuote() { - assertEquals("te\"st", GremlinScriptChecker.parse("g.with('requestId','te\"st').with(true).V().out('knows')"). - getRequestId().get()); - assertEquals("te\\\"st", GremlinScriptChecker.parse("g.with('requestId', \"te\\\"st\").with(true).V().out('knows')"). - getRequestId().get()); - } - @Test public void shouldIdentifyTimeoutWithLowerL() { assertEquals(1000, GremlinScriptChecker.parse("g.with('timeoutMillis', 1000l).with(true).V().out('knows')"). @@ -106,28 +86,12 @@ public class GremlinScriptCheckerTest { getTimeout().get().longValue()); } - @Test - public void shouldIdentifyRequestIdAsStringKeySingleQuoted() { - assertEquals("db024fca-ed15-4375-95de-4c6106aef895", GremlinScriptChecker.parse("g.with('requestId', 'db024fca-ed15-4375-95de-4c6106aef895').with(true).V().out('knows')"). - getRequestId().get()); - assertEquals("db024fca-ed15-4375-95de-4c6106aef895", GremlinScriptChecker.parse("g.with(\"requestId\", 'db024fca-ed15-4375-95de-4c6106aef895').with(true).V().out('knows')"). - getRequestId().get()); - } - @Test public void shouldIdentifyTimeoutAsStringKeyDoubleQuoted() { assertEquals(1000, GremlinScriptChecker.parse("g.with(\"timeoutMillis\", 1000L).with(true).V().out('knows')"). getTimeout().get().longValue()); } - @Test - public void shouldIdentifyRequestIdAsStringKeyDoubleQuoted() { - assertEquals("db024fca-ed15-4375-95de-4c6106aef895", GremlinScriptChecker.parse("g.with(\"requestId\", \"db024fca-ed15-4375-95de-4c6106aef895\").with(true).V().out('knows')"). - getRequestId().get()); - assertEquals("db024fca-ed15-4375-95de-4c6106aef895", GremlinScriptChecker.parse("g.with(\"requestId\", \"db024fca-ed15-4375-95de-4c6106aef895\").with(true).V().out('knows')"). - getRequestId().get()); - } - @Test public void shouldIdentifyTimeoutAsTokenKey() { assertEquals(1000, GremlinScriptChecker.parse("g.with(Tokens.TIMEOUT_MILLIS, 1000L).with(true).V().out('knows')"). @@ -136,14 +100,6 @@ public class GremlinScriptCheckerTest { getTimeout().get().longValue()); } - @Test - public void shouldIdentifyRequestIdAsTokenKey() { - assertEquals("db024fca-ed15-4375-95de-4c6106aef895", GremlinScriptChecker.parse("g.with(Tokens.REQUEST_ID, \"db024fca-ed15-4375-95de-4c6106aef895\").with(true).V().out('knows')"). - getRequestId().get()); - assertEquals("db024fca-ed15-4375-95de-4c6106aef895", GremlinScriptChecker.parse("g.with(Tokens.REQUEST_ID, \"db024fca-ed15-4375-95de-4c6106aef895\").with(true).V().out('knows')"). - getRequestId().get()); - } - @Test public void shouldIdentifyTimeoutAsTokenKeyWithoutClassName() { assertEquals(1000, GremlinScriptChecker.parse("g.with(TIMEOUT_MILLIS, 1000L).with(true).V().out('knows')"). @@ -152,14 +108,6 @@ public class GremlinScriptCheckerTest { getTimeout().get().longValue()); } - @Test - public void shouldIdentifyRequestIdAsTokenKeyWithoutClassName() { - assertEquals("db024fca-ed15-4375-95de-4c6106aef895", GremlinScriptChecker.parse("g.with(REQUEST_ID, \"db024fca-ed15-4375-95de-4c6106aef895\").with(true).V().out('knows')"). - getRequestId().get()); - assertEquals("db024fca-ed15-4375-95de-4c6106aef895", GremlinScriptChecker.parse("g.with(REQUEST_ID, 'db024fca-ed15-4375-95de-4c6106aef895').with(true).V().out('knows')"). - getRequestId().get()); - } - @Test public void shouldIdentifyMultipleTimeouts() { assertEquals(6000, GremlinScriptChecker.parse("g.with('timeoutMillis', 1000L).with(true).V().out('knows');" + @@ -176,22 +124,6 @@ public class GremlinScriptCheckerTest { getTimeout().get().longValue()); } - @Test - public void shouldIdentifyMultipleRequestIds() { - assertEquals("test9", GremlinScriptChecker.parse("g.with('requestId', 'test1').with(true).V().out('knows');" + - "g.with('requestId', 'test2').with(true).V().out('knows');\n" + - " //g.with('requestId', 'test3').with(true).V().out('knows');\n" + - " /* g.with('requestId', 'test4').with(true).V().out('knows');*/\n" + - " /* \n" + - "g.with('requestId', 'test5').with(true).V().out('knows'); \n" + - "*/ \n" + - " g.with('requestId', 'test6').with(true).V().out('knows');\n" + - " g.with(Tokens.REQUEST_ID, 'test7').with(true).V().out('knows');\n" + - " g.with(REQUEST_ID, 'test8').with(true).V().out('knows');\n" + - " g.with('requestId', 'test9').with(true).V().out('knows');"). - getRequestId().get()); - } - @Test public void shouldIdentifyMaterializePropertiesSingleQuoted() { assertEquals("all", GremlinScriptChecker.parse("g.with('materializeProperties', 'all').with(true).V().out('knows')"). @@ -239,9 +171,8 @@ public class GremlinScriptCheckerTest { @Test public void shouldFindAllResults() { final GremlinScriptChecker.Result r = GremlinScriptChecker.parse( - "g.with('timeoutMillis', 1000).with(true).with(REQUEST_ID, \"db024fca-ed15-4375-95de-4c6106aef895\").with(\"materializeProperties\", 'all').V().out('knows')"); + "g.with('timeoutMillis', 1000).with(true).with(\"materializeProperties\", 'all').V().out('knows')"); assertEquals(1000, r.getTimeout().get().longValue()); - assertEquals("db024fca-ed15-4375-95de-4c6106aef895", r.getRequestId().get()); assertEquals("all", r.getMaterializeProperties().get()); } @@ -449,4 +380,121 @@ public class GremlinScriptCheckerTest { " addE('bridge').from('o').to('r').iterate()"). getTimeout().get().longValue()); } + + @Test + public void shouldIdentifyLanguageStringKey() { + assertEquals("gremlin-groovy", GremlinScriptChecker.parse("g.with('language', 'gremlin-groovy').V().out('knows')"). + getLanguage().get()); + assertEquals("gremlin-groovy", GremlinScriptChecker.parse("g.with(\"language\", \"gremlin-groovy\").V().out('knows')"). + getLanguage().get()); + } + + @Test + public void shouldIdentifyLanguageAsTokenKey() { + assertEquals("gremlin-groovy", GremlinScriptChecker.parse("g.with(ARGS_LANGUAGE, 'gremlin-groovy').V().out('knows')"). + getLanguage().get()); + assertEquals("gremlin-groovy", GremlinScriptChecker.parse("g.with(Tokens.ARGS_LANGUAGE, 'gremlin-groovy').V().out('knows')"). + getLanguage().get()); + } + + @Test + public void shouldIdentifyMultipleLanguagesUsingLast() { + assertEquals("gremlin-lang", GremlinScriptChecker.parse("g.with('language', 'gremlin-groovy').with('language', 'gremlin-lang').V()"). + getLanguage().get()); + } + + @Test + public void shouldIdentifyBatchSizeStringKey() { + assertEquals("10", GremlinScriptChecker.parse("g.with('batchSize', 10).V().out('knows')"). + getBatchSize().get()); + assertEquals("10", GremlinScriptChecker.parse("g.with(\"batchSize\", 10).V().out('knows')"). + getBatchSize().get()); + } + + @Test + public void shouldIdentifyBatchSizeAsTokenKey() { + assertEquals("25", GremlinScriptChecker.parse("g.with(ARGS_BATCH_SIZE, 25).V().out('knows')"). + getBatchSize().get()); + assertEquals("25", GremlinScriptChecker.parse("g.with(Tokens.ARGS_BATCH_SIZE, 25).V().out('knows')"). + getBatchSize().get()); + } + + @Test + public void shouldIdentifyMultipleBatchSizesUsingLast() { + assertEquals("64", GremlinScriptChecker.parse("g.with('batchSize', 10).with('batchSize', 64).V()"). + getBatchSize().get()); + } + + @Test + public void shouldCaptureBatchSizeAboveIntegerMaxUnparsed() { + // the checker captures the raw string without parsing, so a value above Integer.MAX_VALUE is returned as-is + // (rather than throwing) and the server can reject it as a bad request. + assertEquals("2147483648", GremlinScriptChecker.parse("g.with('batchSize', 2147483648).V()"). + getBatchSize().get()); + } + + @Test + public void shouldIdentifyBulkResultsStringKey() { + assertEquals(Boolean.TRUE, GremlinScriptChecker.parse("g.with('bulkResults', true).V().out('knows')"). + getBulkResults().get()); + assertEquals(Boolean.FALSE, GremlinScriptChecker.parse("g.with(\"bulkResults\", false).V().out('knows')"). + getBulkResults().get()); + } + + @Test + public void shouldIdentifyBulkResultsAsTokenKey() { + assertEquals(Boolean.TRUE, GremlinScriptChecker.parse("g.with(BULK_RESULTS, true).V().out('knows')"). + getBulkResults().get()); + assertEquals(Boolean.TRUE, GremlinScriptChecker.parse("g.with(Tokens.BULK_RESULTS, true).V().out('knows')"). + getBulkResults().get()); + } + + @Test + public void shouldIdentifyMultipleBulkResultsUsingLast() { + assertEquals(Boolean.FALSE, GremlinScriptChecker.parse("g.with('bulkResults', true).with('bulkResults', false).V()"). + getBulkResults().get()); + } + + @Test + public void shouldNotIdentifyBindingsFromScript() { + // parameters is intentionally NOT scraped from a script (field/header-only) + final GremlinScriptChecker.Result r = GremlinScriptChecker.parse("g.with('parameters', '[\"x\":1]').V(x)"); + assertEquals(Optional.empty(), r.getTimeout()); + assertEquals(Optional.empty(), r.getLanguage()); + assertEquals(Optional.empty(), r.getBatchSize()); + assertEquals(Optional.empty(), r.getBulkResults()); + } + + @Test + public void shouldNotIdentifyTransactionIdFromScript() { + // transactionId is intentionally NOT scraped from a script (field/header-only) + final GremlinScriptChecker.Result r = GremlinScriptChecker.parse("g.with('transactionId', 'abc').V()"); + assertEquals(Optional.empty(), r.getTimeout()); + assertEquals(Optional.empty(), r.getLanguage()); + assertEquals(Optional.empty(), r.getBatchSize()); + assertEquals(Optional.empty(), r.getBulkResults()); + } + + @Test + public void shouldNotIdentifyGFromScript() { + // g is intentionally NOT scraped from a script due to the complex interactions between different script + // engines and transactions - it must be supplied as a request field. + final GremlinScriptChecker.Result r = GremlinScriptChecker.parse("g.with('g', 'myGraph').V().out('knows')"); + assertEquals(Optional.empty(), r.getTimeout()); + assertEquals(Optional.empty(), r.getLanguage()); + assertEquals(Optional.empty(), r.getBatchSize()); + assertEquals(Optional.empty(), r.getBulkResults()); + } + + @Test + public void shouldFindAllFiveResults() { + final GremlinScriptChecker.Result r = GremlinScriptChecker.parse( + "g.with('timeoutMillis', 1000).with('materializeProperties', 'all').with('language', 'gremlin-groovy')." + + "with('batchSize', 32).with('bulkResults', true).V().out('knows')"); + assertEquals(1000, r.getTimeout().get().longValue()); + assertEquals("all", r.getMaterializeProperties().get()); + assertEquals("gremlin-groovy", r.getLanguage().get()); + assertEquals("32", r.getBatchSize().get()); + assertEquals(Boolean.TRUE, r.getBulkResults().get()); + } } 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 b80c5fc9e5..b080653439 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 @@ -20,9 +20,12 @@ package org.apache.tinkerpop.gremlin.server; import org.apache.tinkerpop.gremlin.process.traversal.Path; import io.netty.channel.ChannelHandlerContext; +import io.netty.handler.codec.http.HttpHeaders; import org.apache.tinkerpop.gremlin.groovy.engine.GremlinExecutor; import org.apache.tinkerpop.gremlin.jsr223.GremlinScriptChecker; import org.apache.tinkerpop.gremlin.process.traversal.traverser.util.AbstractTraverser; +import org.apache.tinkerpop.gremlin.server.handler.StateKey; +import org.apache.tinkerpop.gremlin.server.util.GremlinError; import org.apache.tinkerpop.gremlin.structure.Element; import org.apache.tinkerpop.gremlin.structure.Graph; import org.apache.tinkerpop.gremlin.structure.util.reference.ReferenceFactory; @@ -50,7 +53,9 @@ public class Context { private final ScheduledExecutorService scheduledExecutorService; private final long requestTimeout; private final String materializeProperties; - private final Object gremlinArgument; + private final String language; + private final String batchSize; + private final boolean bulkResults; private final RequestType requestType; private ScheduledFuture<?> timeoutExecutor = null; private boolean timeoutExecutorGrabbed = false; @@ -74,10 +79,18 @@ public class Context { // order of calls matter as one depends on the next final String gremlin = requestMessage.getGremlin(); - this.gremlinArgument = gremlin; this.requestType = RequestType.fromGremlin(gremlin); - this.requestTimeout = determineTimeout(); - this.materializeProperties = determineMaterializeProperties(); + + // Parse the script once and share the result across all per-request option resolution below. Every + // determine*() method applies the same precedence for its option: + // script-embedded with() > explicit request field > request header (bulkResults only) > server/connection default + // The individual methods are not re-documented with this rule; they only note behavior unique to that option. + final GremlinScriptChecker.Result scriptOptions = GremlinScriptChecker.parse(gremlin); + this.requestTimeout = determineTimeout(scriptOptions); + this.materializeProperties = determineMaterializeProperties(scriptOptions); + this.language = determineLanguage(scriptOptions); + this.batchSize = determineBatchSize(scriptOptions); + this.bulkResults = determineBulkResults(scriptOptions); this.transactionId = requestMessage.getField(Tokens.ARGS_TRANSACTION_ID); } @@ -101,10 +114,7 @@ public class Context { } /** - * The timeout for the request. If the request is a script it examines the script for a timeout setting using - * {@code with()}. If that is not found then it examines the request itself to see if the timeout is provided by - * {@link Tokens#TIMEOUT_MILLIS}. If that is not provided then the {@link Settings#timeoutMillis} is - * utilized as the default. + * The timeout in milliseconds for the request. Resolved in the constructor (see the precedence note there). */ public long getRequestTimeout() { return requestTimeout; @@ -114,6 +124,40 @@ public class Context { return materializeProperties; } + /** + * The language resolved for the request (see the precedence note in the constructor). + */ + public String getLanguage() { + return language; + } + + /** + * The batch size resolved for the request, parsed to a positive {@code int}. Parsing happens here (rather than in + * the {@link Context} constructor) so that an invalid value - non-numeric, non-positive, or greater than + * {@link Integer#MAX_VALUE} - is surfaced as a bad request via {@link ProcessingException} rather than an uncaught + * error. Callers must be on a path that turns a {@link ProcessingException} into a response (i.e. before the + * {@code 200 OK} is committed). + */ + public int getBatchSize() throws ProcessingException { + final int parsed; + try { + parsed = Integer.parseInt(batchSize); + } catch (NumberFormatException nfe) { + throw new ProcessingException(GremlinError.batchSize(batchSize)); + } + if (parsed <= 0) { + throw new ProcessingException(GremlinError.batchSize(batchSize)); + } + return parsed; + } + + /** + * Whether results should be bulked for the request (see the precedence note in the constructor). + */ + public boolean getBulkResults() { + return bulkResults; + } + public ScheduledExecutorService getScheduledExecutorService() { return scheduledExecutorService; } @@ -207,20 +251,14 @@ public class Context { return gremlinExecutor; } - private long determineTimeout() { - // per-request timeout override falls back to the server-configured default when not supplied + private long determineTimeout(final GremlinScriptChecker.Result scriptOptions) { final Long timeoutMillis = requestMessage.getField(Tokens.TIMEOUT_MILLIS); final long seto = (null != timeoutMillis) ? timeoutMillis : settings.getTimeoutMillis(); - - // 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 = GremlinScriptChecker.parse(gremlinArgument.toString()).getTimeout(); - - return timeoutDefinedInScript.orElse(seto); + return scriptOptions.getTimeout().orElse(seto); } - private String determineMaterializeProperties() { - final Optional<String> mp = GremlinScriptChecker.parse(gremlinArgument.toString()).getMaterializeProperties(); + private String determineMaterializeProperties(final GremlinScriptChecker.Result scriptOptions) { + final Optional<String> mp = scriptOptions.getMaterializeProperties(); if (mp.isPresent()) return mp.get().equals(Tokens.MATERIALIZE_PROPERTIES_TOKENS) ? Tokens.MATERIALIZE_PROPERTIES_TOKENS @@ -233,6 +271,43 @@ public class Context { : Tokens.MATERIALIZE_PROPERTIES_ALL; } + private String determineLanguage(final GremlinScriptChecker.Result scriptOptions) { + final Optional<String> lang = scriptOptions.getLanguage(); + if (lang.isPresent()) return lang.get(); + + final String language = requestMessage.getField(Tokens.ARGS_LANGUAGE); + return (null != language) ? language : "gremlin-lang"; + } + + private String determineBatchSize(final GremlinScriptChecker.Result scriptOptions) { + // the value is kept as a raw string here and only parsed/validated in getBatchSize(), so an invalid value does + // not throw while constructing Context (which runs outside the request's error-handling path). + final Optional<String> bs = scriptOptions.getBatchSize(); + if (bs.isPresent()) return bs.get(); + + final Integer batchSize = requestMessage.getField(Tokens.ARGS_BATCH_SIZE); + return String.valueOf((null != batchSize) ? batchSize : settings.resultIterationBatchSize); + } + + private boolean determineBulkResults(final GremlinScriptChecker.Result scriptOptions) { + final Optional<Boolean> br = scriptOptions.getBulkResults(); + if (br.isPresent()) return br.get(); + + final Object bulkResultsField = requestMessage.getField(Tokens.BULK_RESULTS); + if (null != bulkResultsField) return Boolean.parseBoolean(bulkResultsField.toString()); + + // the request header is a fallback below the request field. channel may be absent in unit tests. + if (null != channelHandlerContext && null != channelHandlerContext.channel()) { + final HttpHeaders headers = channelHandlerContext.channel().attr(StateKey.REQUEST_HEADERS).get(); + if (null != headers) { + final String bulkResultsHeader = headers.get(Tokens.BULK_RESULTS); + if (null != bulkResultsHeader) return Boolean.parseBoolean(bulkResultsHeader); + } + } + + return false; + } + public void handleDetachment(final List<Object> aggregate) { if (!aggregate.isEmpty() && !this.getMaterializeProperties().equals(Tokens.MATERIALIZE_PROPERTIES_ALL)) { for (int i = 0; i < aggregate.size(); i++) { 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 13531f856d..01df7e9297 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 @@ -81,7 +81,6 @@ import java.util.Iterator; import java.util.List; import java.util.Map; import java.util.NoSuchElementException; -import java.util.Objects; import java.util.Optional; import java.util.Set; import java.util.UUID; @@ -192,9 +191,9 @@ public class HttpGremlinEndpointHandler extends SimpleChannelInboundHandler<Requ final Optional<UnmanagedTransaction> txForRequest = isTransactionalOp ? transactionManager.get(requestCtx.getTransactionId()) : Optional.empty(); - // per-request timeout override falls back to the server-configured default when not supplied - final Long timeoutMillis = requestMessage.getField(Tokens.TIMEOUT_MILLIS); - final long seto = (null != timeoutMillis) ? timeoutMillis : requestCtx.getSettings().getTimeoutMillis(); + // consume the Context-resolved timeout (rather than re-reading the field) so that a timeout embedded in a + // raw-string script's with() is actually applied. + final long seto = requestCtx.getRequestTimeout(); final FutureTask<Void> evalFuture = new FutureTask<>(() -> { try { @@ -214,8 +213,14 @@ public class HttpGremlinEndpointHandler extends SimpleChannelInboundHandler<Requ } // Validate the request before any transaction lifecycle side effects. - final Map<String, Object> args = requestMessage.getFields(); - final String language = args.containsKey(Tokens.ARGS_LANGUAGE) ? (String) args.get(Tokens.ARGS_LANGUAGE) : "gremlin-lang"; + + // batchSize must parse to a positive int (1..Integer.MAX_VALUE); getBatchSize() throws a + // ProcessingException (-> bad request) for a non-numeric, non-positive, or out-of-range value. This + // runs before any transaction lifecycle side effects and before the 200 OK is committed. + requestCtx.getBatchSize(); + + // validate script engine availability. + final String language = requestCtx.getLanguage(); if (gremlinExecutor.getScriptEngineManager().getEngineByName(language) == null) { throw new ProcessingException(GremlinError.scriptEngineNotAvailable(language)); } @@ -434,8 +439,10 @@ public class HttpGremlinEndpointHandler extends SimpleChannelInboundHandler<Requ private void iterateScriptEvalResult(final Context context, MessageSerializer<?> serializer, final RequestMessage message, final HttpResponseCoordinator coordinator) throws ProcessingException, InterruptedException, ScriptException { - final Map<String, Object> args = message.getFields(); - final String language = args.containsKey(Tokens.ARGS_LANGUAGE) ? (String) args.get(Tokens.ARGS_LANGUAGE) : "gremlin-lang"; + final String language = context.getLanguage(); + // resolve batchSize here (this method declares ProcessingException) so it is not parsed again downstream in + // handleIterator; it was already validated by the pre-200 guard, so this will not throw. + final int resultIterationBatchSize = context.getBatchSize(); final GremlinScriptEngine scriptEngine = gremlinExecutor.getScriptEngineManager().getEngineByName(language); if (scriptEngine == null) { @@ -463,23 +470,19 @@ public class HttpGremlinEndpointHandler extends SimpleChannelInboundHandler<Requ try { final Object result = scriptEngine.eval(message.getGremlin(), mergedBindings); - final String bulkingSetting = context.getChannelHandlerContext().channel().attr(StateKey.REQUEST_HEADERS).get().get(Tokens.BULK_RESULTS); - // bulking only applies if it's gremlin-lang, and per request token setting takes precedence over header setting. + // bulking only applies if it's gremlin-lang and the serializer supports it. // The serializer check is temporarily needed because GraphSON hasn't been removed yet and doesn't support bulking. - final boolean bulking = language.equals("gremlin-lang") && serializer instanceof GraphBinaryMessageSerializerV4 ? - (args.containsKey(Tokens.BULK_RESULTS) ? - Objects.equals(args.get(Tokens.BULK_RESULTS), "true") : - Objects.equals(bulkingSetting, "true")) : - false; + final boolean bulking = language.equals("gremlin-lang") && serializer instanceof GraphBinaryMessageSerializerV4 && + context.getBulkResults(); if (bulking) { // optimization for driver requests ((Traversal.Admin<?, ?>) result).applyStrategies(); itty = new TraverserIterator((Traversal.Admin<?, ?>) result); - handleIterator(context, itty, coordinator, true); + handleIterator(context, itty, coordinator, true, resultIterationBatchSize); } else { itty = IteratorUtils.asIterator(result); - handleIterator(context, itty, coordinator, false); + handleIterator(context, itty, coordinator, false, resultIterationBatchSize); } if (autoCommit && graph.tx().isOpen()) graph.tx().commit(); @@ -598,10 +601,8 @@ public class HttpGremlinEndpointHandler extends SimpleChannelInboundHandler<Requ return bindings; } - private void handleIterator(final Context context, final Iterator itty, final HttpResponseCoordinator coordinator, final boolean bulking) throws InterruptedException { + private void handleIterator(final Context context, final Iterator itty, final HttpResponseCoordinator coordinator, final boolean bulking, final int resultIterationBatchSize) throws InterruptedException { final ChannelHandlerContext nettyContext = context.getChannelHandlerContext(); - final RequestMessage msg = context.getRequestMessage(); - final Settings settings = context.getSettings(); // used to limit warnings for when netty fills the buffer and hits the high watermark - prevents // over-logging of the same message. @@ -619,9 +620,7 @@ public class HttpGremlinEndpointHandler extends SimpleChannelInboundHandler<Requ return; } - // the batch size can be overridden by the request - final int resultIterationBatchSize = (Integer) msg.optionalField(Tokens.ARGS_BATCH_SIZE) - .orElse(settings.resultIterationBatchSize); + // batch size was resolved and validated by the caller. List<Object> aggregate = new ArrayList<>(resultIterationBatchSize); // use an external control to manage the loop as opposed to just checking hasNext() in the while. this diff --git a/gremlin-server/src/main/java/org/apache/tinkerpop/gremlin/server/util/GremlinError.java b/gremlin-server/src/main/java/org/apache/tinkerpop/gremlin/server/util/GremlinError.java index 4e9bc9ffe7..414e007292 100644 --- a/gremlin-server/src/main/java/org/apache/tinkerpop/gremlin/server/util/GremlinError.java +++ b/gremlin-server/src/main/java/org/apache/tinkerpop/gremlin/server/util/GremlinError.java @@ -88,6 +88,12 @@ public class GremlinError { return new GremlinError(HttpResponseStatus.BAD_REQUEST, message, "InvalidRequestException"); } + public static GremlinError batchSize(final String batchSize) { + final String message = String.format("The message specifies a batchSize of %s but it must be an integer between 1 and %s", + batchSize, Integer.MAX_VALUE); + return new GremlinError(HttpResponseStatus.BAD_REQUEST, message, "InvalidRequestException"); + } + public static GremlinError binding(final String aliased) { final String message = String.format("Could not alias [%s] to [%s] as [%s] not in the Graph or TraversalSource global bindings", Tokens.ARGS_G, aliased, aliased); diff --git a/gremlin-server/src/test/java/org/apache/tinkerpop/gremlin/server/ContextTest.java b/gremlin-server/src/test/java/org/apache/tinkerpop/gremlin/server/ContextTest.java index 6de24da96c..fdad3a3fa4 100644 --- a/gremlin-server/src/test/java/org/apache/tinkerpop/gremlin/server/ContextTest.java +++ b/gremlin-server/src/test/java/org/apache/tinkerpop/gremlin/server/ContextTest.java @@ -20,6 +20,7 @@ package org.apache.tinkerpop.gremlin.server; import io.netty.channel.ChannelHandlerContext; import nl.altindag.log.LogCaptor; +import org.apache.tinkerpop.gremlin.util.Tokens; import org.apache.tinkerpop.gremlin.util.message.RequestMessage; import org.junit.AfterClass; import org.junit.Before; @@ -28,6 +29,9 @@ import org.junit.Test; import org.mockito.Mockito; import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertFalse; +import static org.junit.Assert.assertThrows; +import static org.junit.Assert.assertTrue; public class ContextTest { @@ -76,4 +80,140 @@ public class ContextTest { // "all" is default value assertEquals("all", context.getMaterializeProperties()); } -} \ No newline at end of file + + private static Context newContext(final RequestMessage request) { + final ChannelHandlerContext ctx = Mockito.mock(ChannelHandlerContext.class); + return new Context(request, ctx, new Settings(), null, null, null); + } + + @Test + public void shouldResolveTimeoutFromScriptOverField() { + final RequestMessage request = RequestMessage.build("g.with('timeoutMillis', 1000).V()") + .addTimeoutMillis(5000).create(); + assertEquals(1000, newContext(request).getRequestTimeout()); + } + + @Test + public void shouldResolveTimeoutFromFieldWhenNoScript() { + final RequestMessage request = RequestMessage.build("g.V()").addTimeoutMillis(5000).create(); + assertEquals(5000, newContext(request).getRequestTimeout()); + } + + @Test + public void shouldResolveTimeoutFromDefaultWhenNeither() { + final RequestMessage request = RequestMessage.build("g.V()").create(); + assertEquals(new Settings().getTimeoutMillis(), newContext(request).getRequestTimeout()); + } + + @Test + public void shouldResolveMaterializePropertiesFromScriptOverField() { + final RequestMessage request = RequestMessage.build("g.with('materializeProperties', 'tokens').V()") + .addMaterializeProperties("all").create(); + assertEquals("tokens", newContext(request).getMaterializeProperties()); + } + + @Test + public void shouldResolveMaterializePropertiesFromFieldWhenNoScript() { + final RequestMessage request = RequestMessage.build("g.V()").addMaterializeProperties("tokens").create(); + assertEquals("tokens", newContext(request).getMaterializeProperties()); + } + + @Test + public void shouldResolveMaterializePropertiesFromDefaultWhenNeither() { + final RequestMessage request = RequestMessage.build("g.V()").create(); + assertEquals("all", newContext(request).getMaterializeProperties()); + } + + @Test + public void shouldResolveLanguageFromScriptOverField() { + final RequestMessage request = RequestMessage.build("g.with('language', 'gremlin-groovy').V()") + .addLanguage("gremlin-lang").create(); + assertEquals("gremlin-groovy", newContext(request).getLanguage()); + } + + @Test + public void shouldResolveLanguageFromFieldWhenNoScript() { + final RequestMessage request = RequestMessage.build("g.V()").addLanguage("gremlin-groovy").create(); + assertEquals("gremlin-groovy", newContext(request).getLanguage()); + } + + @Test + public void shouldResolveLanguageFromDefaultWhenNeither() { + final RequestMessage request = RequestMessage.build("g.V()").create(); + assertEquals("gremlin-lang", newContext(request).getLanguage()); + } + + @Test + public void shouldResolveBatchSizeFromScriptOverField() throws Exception { + final RequestMessage request = RequestMessage.build("g.with('batchSize', 10).V()") + .addChunkSize(500).create(); + assertEquals(10, newContext(request).getBatchSize()); + } + + @Test + public void shouldResolveBatchSizeFromFieldWhenNoScript() throws Exception { + final RequestMessage request = RequestMessage.build("g.V()").addChunkSize(500).create(); + assertEquals(500, newContext(request).getBatchSize()); + } + + @Test + public void shouldResolveBatchSizeFromDefaultWhenNeither() throws Exception { + final RequestMessage request = RequestMessage.build("g.V()").create(); + assertEquals(new Settings().resultIterationBatchSize, newContext(request).getBatchSize()); + } + + @Test + public void shouldThrowOnNonPositiveBatchSize() { + // getBatchSize() parses and validates, throwing a bad-request ProcessingException for a non-positive value + // (from either the script or the request field) rather than returning a value that would stall iteration. + final RequestMessage fromScript = RequestMessage.build("g.with('batchSize', 0).V()").create(); + assertThrows(ProcessingException.class, () -> newContext(fromScript).getBatchSize()); + + final RequestMessage fromField = RequestMessage.build("g.V()").addChunkSize(0).create(); + assertThrows(ProcessingException.class, () -> newContext(fromField).getBatchSize()); + } + + @Test + public void shouldThrowOnBatchSizeAboveIntegerMax() { + // a script-embedded value above Integer.MAX_VALUE cannot be applied and is rejected as a bad request rather + // than throwing an uncaught NumberFormatException while constructing Context. + final RequestMessage request = RequestMessage.build("g.with('batchSize', 2147483648).V()").create(); + assertThrows(ProcessingException.class, () -> newContext(request).getBatchSize()); + } + + @Test + public void shouldResolveBulkResultsFromScriptOverField() { + final RequestMessage request = RequestMessage.build("g.with('bulkResults', false).V()") + .addBulkResults(true).create(); + assertFalse(newContext(request).getBulkResults()); + } + + @Test + public void shouldResolveBulkResultsFromFieldWhenNoScript() { + final RequestMessage request = RequestMessage.build("g.V()").addBulkResults(true).create(); + assertTrue(newContext(request).getBulkResults()); + } + + @Test + public void shouldResolveBulkResultsFromDefaultWhenNeither() { + final RequestMessage request = RequestMessage.build("g.V()").create(); + assertFalse(newContext(request).getBulkResults()); + } + + @Test + public void shouldNotHonorParametersFromScript() { + // an embedded with('parameters', ...) must not become the request's parameters; only the field is authoritative. + final RequestMessage request = RequestMessage.build("g.with('parameters', '[\"x\":1]').V(x)") + .addParameters("[\"y\":2]").create(); + final Context context = newContext(request); + // Context does not expose parsed parameters from the script; the field value remains the source of truth. + assertEquals("[\"y\":2]", context.getRequestMessage().getField(Tokens.ARGS_PARAMETERS)); + } + + @Test + public void shouldNotHonorTransactionIdFromScript() { + final RequestMessage request = RequestMessage.build("g.with('transactionId', 'fromScript').V()") + .addTransactionId("fromField").create(); + assertEquals("fromField", newContext(request).getTransactionId()); + } +} 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 4bf10dc346..ef651cc642 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 @@ -113,6 +113,7 @@ public class GremlinServerHttpIntegrateTest extends AbstractGremlinServerIntegra case "should200OnPOSTTransactionalGraph": case "shouldRollbackOnFailedMutatingTraversal": case "shouldCommitMutatingTraversalWithEmptyResult": + case "should400OnTransactionBeginWithNonPositiveBatchSize": useTinkerTransactionGraph(settings); break; case "should200OnPOSTTransactionalGraphInStrictMode": @@ -1364,6 +1365,141 @@ public class GremlinServerHttpIntegrateTest extends AbstractGremlinServerIntegra } } + @Test(timeout = 10000) // Add test timeout to prevent incorrect timeout behavior from stopping test run. + public void shouldAcceptTimeoutMillisEmbeddedInScript() throws Exception { + // Regression: a with('timeoutMillis', ...) embedded in a raw-string script must be honored by the server. + // Previously the server computed but never applied this value, so the query ran to completion instead of + // timing out. + final String body = "{ \"gremlin\": \"" + "g.with('timeoutMillis', 100).inject(1).sideEffect{Thread.sleep(50000)}" + + "\",\"language\":\"gremlin-groovy\"}"; + 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 shouldAcceptMaterializePropertiesTokensEmbeddedInScript() throws Exception { + final String body = "{ \"gremlin\": \"" + "gmodern.with('materializeProperties', 'tokens').V().limit(1)" + + "\",\"language\":\"gremlin-groovy\"}"; + 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(TOKEN_DATA).get(GraphSONTokens.VALUEPROP).get(0).get(GraphSONTokens.VALUEPROP).get(GraphSONTokens.PROPERTIES)); + } + } + + @Test + public void shouldPreferScriptEmbeddedTimeoutOverRequestField() throws Exception { + // Precedence: script-embedded with() wins over the request field. The script sets a small timeout that fires; + // the (larger) field value must not override it. + final String body = "{ \"gremlin\": \"" + "g.with('timeoutMillis', 100).inject(1).sideEffect{Thread.sleep(5000)}" + + "\",\"language\":\"gremlin-groovy\",\"" + TIMEOUT_MILLIS + "\":\"8000\"}"; + 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 should400OnScriptEmbeddedNonPositiveBatchSize() throws Exception { + // a non-positive batchSize would stall result iteration, so it is rejected as a bad request. + final String body = "{ \"gremlin\": \"" + "g.with('batchSize', 0).inject(1)" + "\",\"language\":\"gremlin-groovy\"}"; + 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(400, response.getStatusLine().getStatusCode()); + } + } + + @Test + public void should400OnScriptEmbeddedOverflowBatchSize() throws Exception { + // a batchSize above Integer.MAX_VALUE cannot be applied; it must be a clean 400, not an uncaught error (500). + final String body = "{ \"gremlin\": \"" + "g.with('batchSize', 2147483648).inject(1)" + "\",\"language\":\"gremlin-groovy\"}"; + 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(400, response.getStatusLine().getStatusCode()); + } + } + + @Test + public void should400OnRequestFieldNonPositiveBatchSize() throws Exception { + final String body = "{ \"gremlin\": \"g.inject(1)\",\"batchSize\":0}"; + 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(400, response.getStatusLine().getStatusCode()); + } + } + + @Test + public void shouldAcceptLanguageEmbeddedInScript() throws Exception { + // a with('language', ...) embedded in the script selects the script engine. this uses a groovy-only + // construct (a closure), which only evaluates if the groovy engine was selected from the embedded option. + final String body = "{ \"gremlin\": \"" + "g.with('language', 'gremlin-groovy').inject(1).map{it.get() + 1}" + "\"}"; + 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); + assertEquals(2, node.get("result").get(TOKEN_DATA).get(GraphSONTokens.VALUEPROP).get(0).get(GraphSONTokens.VALUEPROP).intValue()); + } + } + + @Test + public void shouldAcceptBulkResultsEmbeddedInScript() throws Exception { + // bulking applies only for gremlin-lang over GraphBinary. a with('bulkResults', true) embedded in the script + // must enable it, observable via the bulked flag on the response. + final String gremlin = "g.with('bulkResults', true).inject(1, 2, 3, 2, 1)"; + final GraphBinaryMessageSerializerV4 serializer = new GraphBinaryMessageSerializerV4(); + final ByteBuf serializedRequest = serializer.serializeRequestAsBinary( + RequestMessage.build(gremlin).create(), new UnpooledByteBufAllocator(false)); + + final CloseableHttpClient httpclient = HttpClients.createDefault(); + final HttpPost httppost = new HttpPost(TestClientFactory.createURLString()); + httppost.addHeader(HttpHeaders.CONTENT_TYPE, Serializers.GRAPHBINARY_V4.getValue()); + httppost.addHeader(HttpHeaders.ACCEPT, Serializers.GRAPHBINARY_V4.getValue()); + httppost.setEntity(new ByteArrayEntity(serializedRequest.array())); + + try (final CloseableHttpResponse response = httpclient.execute(httppost)) { + assertEquals(200, response.getStatusLine().getStatusCode()); + final ResponseMessage responseMessage = serializer.readChunk(toByteBuf(response.getEntity()), true); + assertTrue(responseMessage.getResult().isBulked()); + } + } + @Test public void shouldRespectCorsAllowedOrigins() throws Exception { final CloseableHttpClient httpclient = HttpClients.createDefault(); diff --git a/gremlin-tools/gremlin-benchmark/src/main/java/org/apache/tinkerpop/jsr223/GremlinScriptCheckerBenchmark.java b/gremlin-tools/gremlin-benchmark/src/main/java/org/apache/tinkerpop/jsr223/GremlinScriptCheckerBenchmark.java index 1ffe094c60..cc174d9d5d 100644 --- a/gremlin-tools/gremlin-benchmark/src/main/java/org/apache/tinkerpop/jsr223/GremlinScriptCheckerBenchmark.java +++ b/gremlin-tools/gremlin-benchmark/src/main/java/org/apache/tinkerpop/jsr223/GremlinScriptCheckerBenchmark.java @@ -33,8 +33,8 @@ import java.util.Optional; public class GremlinScriptCheckerBenchmark extends AbstractBenchmarkBase { @Benchmark - public Optional<String> testParseRequestId() { - return GremlinScriptChecker.parse("g.with('requestId', '4F53FB59-CFC9-4984-B477-452073A352FD').with(true).V().out('knows')").getRequestId(); + public Optional<Long> testParseTimeout() { + return GremlinScriptChecker.parse("g.with('timeoutMillis', 1000L).with(true).V().out('knows')").getTimeout(); } @Benchmark @@ -44,6 +44,6 @@ public class GremlinScriptCheckerBenchmark extends AbstractBenchmarkBase { @Benchmark public GremlinScriptChecker.Result testParseAll() { - return GremlinScriptChecker.parse("g.with('timeoutMillis', 1000L).with('materializeProperties', 'all').with('requestId', '4F53FB59-CFC9-4984-B477-452073A352FD').with(true).V().out('knows')"); + return GremlinScriptChecker.parse("g.with('timeoutMillis', 1000L).with('materializeProperties', 'all').with('language', 'gremlin-lang').with('batchSize', 100).with('bulkResults', true).with(true).V().out('knows')"); } }
