This is an automated email from the ASF dual-hosted git repository. kenhuuu pushed a commit to branch stringify-params in repository https://gitbox.apache.org/repos/asf/tinkerpop.git
commit 9dc4903637f7a461a7e8420d6c921e738b7aa5c0 Author: Ken Hu <[email protected]> AuthorDate: Tue Apr 14 19:02:44 2026 -0700 Convert request bindings to gremlin-lang string format Moving parameters from binary-serialized maps to string representations makes the request side pure text, decoupling Gremlin language evolution from GraphBinary versioning. New types can be introduced in minor/patch versions without touching GraphBinary, eliminating the need for a major version bump across the ecosystem for every new request-side type. The asParameter() fallback is replaced with an unsupportedType flag that records the class name and falls back to toString(). A flag is used rather than throwing because embedded Traversals build GremlinLang as a side effect but never send it, so unknown types must not break execution. --- CHANGELOG.asciidoc | 2 + docs/src/upgrade/release-4.x.x.asciidoc | 9 + .../language/grammar/GremlinQueryParser.java | 105 +++++- .../language/grammar/ParameterMapVisitor.java | 102 +++++ .../gremlin/process/traversal/GremlinLang.java | 89 ++++- .../language/grammar/ParameterMapVisitorTest.java | 220 +++++++++++ .../gremlin/process/traversal/GremlinLangTest.java | 416 ++++++++++++++++++++- .../tinkerpop/gremlin/driver/RequestOptions.java | 49 ++- .../driver/remote/DriverRemoteConnection.java | 6 + .../driver/remote/TransactionRemoteConnection.java | 6 + .../apache/tinkerpop/gremlin/server/Context.java | 10 + .../server/handler/HttpGremlinEndpointHandler.java | 47 ++- .../server/handler/HttpRequestMessageDecoder.java | 8 +- .../gremlin/server/util/GremlinError.java | 7 + .../GremlinDriverTransactionIntegrateTest.java | 33 ++ .../server/GremlinServerHttpIntegrateTest.java | 22 +- .../gremlin/server/GremlinServerIntegrateTest.java | 50 ++- .../gremlin/server/HttpDriverIntegrateTest.java | 39 +- .../gremlin/util/message/RequestMessage.java | 25 +- .../ser/AbstractGraphSONMessageSerializerV4.java | 2 +- .../util/ser/binary/RequestMessageSerializer.java | 2 +- .../gremlin/util/message/RequestMessageTest.java | 44 ++- .../util/ser/binary/MessageSerializerTest.java | 2 +- .../tinkergraph/structure/TinkerGraphTest.java | 26 ++ 24 files changed, 1187 insertions(+), 134 deletions(-) diff --git a/CHANGELOG.asciidoc b/CHANGELOG.asciidoc index 032033ad5e..c72d06f896 100644 --- a/CHANGELOG.asciidoc +++ b/CHANGELOG.asciidoc @@ -25,6 +25,8 @@ image::https://raw.githubusercontent.com/apache/tinkerpop/master/docs/static/ima [[release-4-0-0]] === TinkerPop 4.0.0 (Release Date: NOT OFFICIALLY RELEASED YET) +* Modified request parameters from `Map<String, Object>` to gremlin-lang compatible `String`. + [[release-4-0-0-beta-2]] === TinkerPop 4.0.0-beta.2 (April 1, 2026) diff --git a/docs/src/upgrade/release-4.x.x.asciidoc b/docs/src/upgrade/release-4.x.x.asciidoc index ebc2bbb1cc..8dd1a94ddd 100644 --- a/docs/src/upgrade/release-4.x.x.asciidoc +++ b/docs/src/upgrade/release-4.x.x.asciidoc @@ -30,6 +30,15 @@ image::gremlins-wildest-dreams.png[width=185] Please see the link:https://github.com/apache/tinkerpop/blob/4.0.0/CHANGELOG.asciidoc#release-4-0-0[changelog] for a complete list of all the modifications that are part of this release. +=== Upgrading for Users + +==== Parameter Format + +Parameters have replaced bindings. Parameters are no longer `Map<String, Object>` instead they are `String`. The string is gremlin-lang map. + +Notably, because parameters now travel as maps, these maps must be gremlin-lang compatible. One of the things that aren't is +`#jsr223` control flags. This means there will be gaps in gremlin-groovy support. + == TinkerPop 4.0.0-beta.2 *Release Date: April 1, 2026* diff --git a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/language/grammar/GremlinQueryParser.java b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/language/grammar/GremlinQueryParser.java index d17fa5a972..871233fa39 100644 --- a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/language/grammar/GremlinQueryParser.java +++ b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/language/grammar/GremlinQueryParser.java @@ -18,7 +18,6 @@ */ package org.apache.tinkerpop.gremlin.language.grammar; -import org.antlr.v4.runtime.CharStream; import org.antlr.v4.runtime.CharStreams; import org.antlr.v4.runtime.CommonTokenStream; import org.antlr.v4.runtime.atn.PredictionMode; @@ -26,6 +25,10 @@ import org.apache.tinkerpop.gremlin.process.traversal.Traversal; import org.slf4j.Logger; import org.slf4j.LoggerFactory; +import java.util.Collection; +import java.util.Map; +import javax.lang.model.SourceVersion; + /** * Parses Gremlin strings to an {@code Object}, typically to a {@link Traversal}. */ @@ -44,21 +47,15 @@ public class GremlinQueryParser { * Parse Gremlin string using a specified {@link GremlinAntlrToJava} object. */ public static Object parse(final String query, final GremlinVisitor<Object> visitor) { - final CharStream in = CharStreams.fromString(query); - final GremlinLexer lexer = new GremlinLexer(in); - lexer.removeErrorListeners(); - lexer.addErrorListener(errorListener); - + final GremlinLexer lexer = createLexer(query); final CommonTokenStream tokens = new CommonTokenStream(lexer); // Setup error handler on parser - final GremlinParser parser = new GremlinParser(tokens); + final GremlinParser parser = createParser(tokens); // SLL prediction mode is faster than the LL prediction mode when parsing the grammar, // but it does not cover parsing all types of input. We use the SLL by default, and fallback // to LL mode if fails to parse the query. parser.getInterpreter().setPredictionMode(PredictionMode.SLL); - parser.removeErrorListeners(); - parser.addErrorListener(errorListener); GremlinParser.QueryListContext queryContext; try { @@ -91,4 +88,94 @@ public class GremlinQueryParser { throw new GremlinParserException("Failed to interpret Gremlin query: " + ex.getMessage(), ex); } } + + /** + * Parses a gremlin-lang map literal string into a {@code Map<String, Object>} for use as parameters. + * <p> + * Uses {@link ParameterMapVisitor} to prevent traversal injection and validates that all keys are strings + * and no values contain traversals. + * + * @param parameterMapString the gremlin-lang map literal string (e.g. {@code [x:1,y:"marko"]}) or {@code null}/empty + * @return the parsed and validated parameter map + * @throws GremlinParserException if parsing fails or validation detects invalid content + */ + public static Map<String, Object> parseParameters(final String parameterMapString) { + if (parameterMapString == null || parameterMapString.isEmpty()) { + return Map.of(); + } + + final GremlinParser parser = createParser(parameterMapString); + final GremlinParser.GenericMapLiteralContext mapCtx = parser.genericMapLiteral(); + + final ParameterMapVisitor visitor = new ParameterMapVisitor(new GremlinAntlrToJava()); + final Map<Object, Object> rawMap = (Map<Object, Object>) visitor.visitGenericMapLiteral(mapCtx); + + if (rawMap == null) { + return Map.of(); + } + + for (final Map.Entry<?, ?> entry : rawMap.entrySet()) { + if (!(entry.getKey() instanceof String)) { + throw new GremlinParserException( + String.format("Parameter map keys must be String, found: %s", + entry.getKey() == null ? "null" : entry.getKey().getClass().getSimpleName())); + } + final String key = (String) entry.getKey(); + if (!SourceVersion.isIdentifier(key)) { + throw new GremlinParserException( + String.format("Parameter map key must be a valid identifier: %s", key)); + } + validateParameterValue(entry.getValue()); + } + + return (Map<String, Object>) (Map<?, ?>) rawMap; + } + + /** + * Recursively validates that a parameter value does not contain a {@link Traversal}. Nested validation is needed + * because steps like mergeV iterate map values, so a Traversal hiding inside a nested map or collection would still + * be dangerous. + */ + private static void validateParameterValue(final Object value) { + if (value instanceof Traversal) { + throw new GremlinParserException("Traversals are not allowed as parameter values"); + } + if (value instanceof Map) { + for (final Object v : ((Map<?, ?>) value).values()) { + validateParameterValue(v); + } + } + if (value instanceof Collection) { + for (final Object v : (Collection<?>) value) { + validateParameterValue(v); + } + } + } + + /** + * Creates a {@link GremlinParser} from the given input string. + */ + private static GremlinParser createParser(final String input) { + return createParser(new CommonTokenStream(createLexer(input))); + } + + /** + * Creates a {@link GremlinParser} from the given {@link GremlinLexer}. + */ + private static GremlinParser createParser(final CommonTokenStream tokens) { + final GremlinParser parser = new GremlinParser(tokens); + parser.removeErrorListeners(); + parser.addErrorListener(errorListener); + return parser; + } + + /** + * Creates a {@link GremlinLexer} from the given input string with error listeners configured. + */ + private static GremlinLexer createLexer(final String input) { + final GremlinLexer lexer = new GremlinLexer(CharStreams.fromString(input)); + lexer.removeErrorListeners(); + lexer.addErrorListener(errorListener); + return lexer; + } } diff --git a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/language/grammar/ParameterMapVisitor.java b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/language/grammar/ParameterMapVisitor.java new file mode 100644 index 0000000000..b2566934b1 --- /dev/null +++ b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/language/grammar/ParameterMapVisitor.java @@ -0,0 +1,102 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.tinkerpop.gremlin.language.grammar; + +/** + * A visitor for parsing parameter map strings that prevents traversal injection. + * <p> + * Extends {@link GenericLiteralVisitor} and overrides traversal-related visit methods + * to throw, ensuring that no traversal can be constructed or executed during the + * parameter map parsing walk. This is critical for security because + * {@code visitTerminatedTraversal} in the base class would execute the traversal + * immediately via {@link TraversalTerminalMethodVisitor}. + */ +public class ParameterMapVisitor extends GenericLiteralVisitor { + + private static final int DEFAULT_MAX_NESTING_DEPTH = 32; + + private final int maxNestingDepth; + private int currentNestingDepth = 0; + + public ParameterMapVisitor(final GremlinAntlrToJava antlr) { + this(antlr, DEFAULT_MAX_NESTING_DEPTH); + } + + public ParameterMapVisitor(final GremlinAntlrToJava antlr, final int maxNestingDepth) { + super(antlr); + this.maxNestingDepth = maxNestingDepth; + } + + /** + * Overridden to prevent nested traversal construction in parameter maps. + */ + @Override + public Object visitNestedTraversal(final GremlinParser.NestedTraversalContext ctx) { + throw new GremlinParserException("Traversals are not allowed in parameter maps"); + } + + /** + * Overridden to prevent terminated traversal execution in parameter maps. + * This is the critical override because the base class would execute the traversal + * immediately via {@link TraversalTerminalMethodVisitor}. + */ + @Override + public Object visitTerminatedTraversal(final GremlinParser.TerminatedTraversalContext ctx) { + throw new GremlinParserException("Traversals are not allowed in parameter maps"); + } + + @Override + public Object visitGenericMapLiteral(final GremlinParser.GenericMapLiteralContext ctx) { + currentNestingDepth++; + if (currentNestingDepth > maxNestingDepth) { + throw new GremlinParserException("Parameter map nesting depth exceeds maximum of " + maxNestingDepth); + } + try { + return super.visitGenericMapLiteral(ctx); + } finally { + currentNestingDepth--; + } + } + + @Override + public Object visitGenericCollectionLiteral(final GremlinParser.GenericCollectionLiteralContext ctx) { + currentNestingDepth++; + if (currentNestingDepth > maxNestingDepth) { + throw new GremlinParserException("Parameter map nesting depth exceeds maximum of " + maxNestingDepth); + } + try { + return super.visitGenericCollectionLiteral(ctx); + } finally { + currentNestingDepth--; + } + } + + @Override + public Object visitGenericSetLiteral(final GremlinParser.GenericSetLiteralContext ctx) { + currentNestingDepth++; + if (currentNestingDepth > maxNestingDepth) { + throw new GremlinParserException("Parameter map nesting depth exceeds maximum of " + maxNestingDepth); + } + try { + return super.visitGenericSetLiteral(ctx); + } finally { + currentNestingDepth--; + } + } +} diff --git a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/GremlinLang.java b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/GremlinLang.java index fc9a147633..e50c723b5c 100644 --- a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/GremlinLang.java +++ b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/GremlinLang.java @@ -51,7 +51,6 @@ import java.util.Objects; import java.util.Set; import java.util.UUID; import java.util.Base64; -import java.util.concurrent.atomic.AtomicInteger; import static org.apache.tinkerpop.gremlin.util.DatetimeHelper.format; import static org.apache.tinkerpop.gremlin.util.iterator.IteratorUtils.asIterator; @@ -66,7 +65,7 @@ public class GremlinLang implements Cloneable, Serializable { private StringBuilder gremlin = new StringBuilder(); private Map<String, Object> parameters = new HashMap<>(); - private static final AtomicInteger paramCount = new AtomicInteger(0); + private String unsupportedType = ""; private List<OptionsStrategy> optionsStrategies = new ArrayList<>(); public GremlinLang() { @@ -197,6 +196,9 @@ public class GremlinLang implements Cloneable, Serializable { if (arg instanceof GremlinLang || arg instanceof DefaultTraversal) { final GremlinLang gremlinLang = arg instanceof GremlinLang ? (GremlinLang) arg : ((DefaultTraversal) arg).getGremlinLang(); parameters.putAll(gremlinLang.getParameters()); + if (gremlinLang.containsUnsupportedTypes()) { + unsupportedType = gremlinLang.getUnsupportedType(); + } return gremlinLang.getGremlin("__"); } @@ -239,14 +241,8 @@ public class GremlinLang implements Cloneable, Serializable { return ((Class) arg).getSimpleName(); } - return asParameter(arg); - } - - private String asParameter(final Object arg) { - final String paramName = String.format("_%d", paramCount.getAndIncrement()); - // todo: consider resetting paramCount when it's larger then 1_000_000 - parameters.put(paramName, arg); - return paramName; + unsupportedType = arg.getClass().getSimpleName(); + return arg.toString(); } private String asString(final Iterator itty) { @@ -367,17 +363,75 @@ public class GremlinLang implements Cloneable, Serializable { } /** - * The alias to set. + * Returns the simple class name of the last type encountered by {@code argAsString()} that could not be + * represented as a gremlin-lang literal. An empty string means all types were supported. + * + * @return the simple class name of the unsupported type, or empty string if none */ - public void addG(final String g) { - parameters.put("g", g); + public String getUnsupportedType() { + return unsupportedType; + } + + /** + * Returns {@code true} if {@code argAsString()} encountered at least one type that could not be represented + * as a gremlin-lang literal. In remote mode, the driver should check this before sending the request. + * <p> + * Note: this only covers types encountered while building the gremlin string. Named {@code GValue} parameters + * store their inner value directly in the parameter map without type-checking via {@code argAsString()}, so + * an unsupported type wrapped in a named {@code GValue} will not be detected by this method. Such cases are + * caught separately by {@link #convertParametersToString(Map)} when the parameter map is serialized. + * Unnamed {@code GValue} instances (null name) recurse into {@code argAsString()} and will set this flag. + * + * @return true if unsupported types were encountered in the gremlin string + */ + public boolean containsUnsupportedTypes() { + return !unsupportedType.isEmpty(); } /** - * Reset parameter naming counter. Mainly intended to make testing easier - */ - public void reset() { - paramCount.set(0); + * Serializes this instance's parameter map to a gremlin-lang map literal string. Keys are parameter names + * (identifiers), values are formatted using {@code argAsString()}. + * + * @return a gremlin-lang map literal string, e.g. {@code ["x":1,"y":"stephen"]} or {@code [:]} for empty + */ + public String getParametersAsString() { + return convertParametersToString(parameters); + } + + /** + * Converts an arbitrary parameter map to a gremlin-lang map literal string. Keys must be valid identifier strings. + * Values are formatted as gremlin-lang literals. + * + * @param params the parameter map to convert + * @return a gremlin-lang map literal string, e.g. {@code ["x":1,"y":"stephen"]} or {@code [:]} for empty + */ + public static String convertParametersToString(final Map<String, Object> params) { + if (params == null || params.isEmpty()) return "[:]"; + + final GremlinLang helper = new GremlinLang(); + final StringBuilder sb = new StringBuilder("["); + final Iterator<Map.Entry<String, Object>> itty = params.entrySet().iterator(); + while (itty.hasNext()) { + final Map.Entry<String, Object> entry = itty.next(); + sb.append(helper.argAsString(entry.getKey())).append(":").append(helper.argAsString(entry.getValue())); + if (itty.hasNext()) sb.append(","); + } + sb.append("]"); + + if (helper.containsUnsupportedTypes()) { + throw new IllegalArgumentException(String.format( + "Parameter map contains at least one type [%s] that cannot be represented as text.", + helper.getUnsupportedType())); + } + + return sb.toString(); + } + + /** + * The alias to set. + */ + public void addG(final String g) { + parameters.put("g", g); } /** @@ -499,6 +553,7 @@ public class GremlinLang implements Cloneable, Serializable { clone.gremlin = new StringBuilder(gremlin.length()); clone.gremlin.append(gremlin); clone.optionsStrategies = new ArrayList<>(this.optionsStrategies); + clone.unsupportedType = this.unsupportedType; return clone; } catch (final CloneNotSupportedException e) { throw new IllegalStateException(e.getMessage(), e); diff --git a/gremlin-core/src/test/java/org/apache/tinkerpop/gremlin/language/grammar/ParameterMapVisitorTest.java b/gremlin-core/src/test/java/org/apache/tinkerpop/gremlin/language/grammar/ParameterMapVisitorTest.java new file mode 100644 index 0000000000..68c8d9b82f --- /dev/null +++ b/gremlin-core/src/test/java/org/apache/tinkerpop/gremlin/language/grammar/ParameterMapVisitorTest.java @@ -0,0 +1,220 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.tinkerpop.gremlin.language.grammar; + +import org.junit.Test; + +import java.util.List; +import java.util.Map; +import java.util.UUID; + +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertNotNull; +import static org.junit.Assert.assertNull; +import static org.junit.Assert.assertTrue; + +/** + * Tests for parameter map parsing, validation, and security. + */ +public class ParameterMapVisitorTest { + + @Test + public void shouldParseEmptyMap() { + final Map<String, Object> result = GremlinQueryParser.parseParameters("[:]"); + assertNotNull(result); + assertTrue(result.isEmpty()); + } + + @Test + public void shouldParseNullInput() { + final Map<String, Object> result = GremlinQueryParser.parseParameters(null); + assertNotNull(result); + assertTrue(result.isEmpty()); + } + + @Test + public void shouldParseEmptyStringInput() { + final Map<String, Object> result = GremlinQueryParser.parseParameters(""); + assertNotNull(result); + assertTrue(result.isEmpty()); + } + + @Test + public void shouldParseSingleIntegerParameter() { + final Map<String, Object> result = GremlinQueryParser.parseParameters("[\"x\":1]"); + assertEquals(1, result.size()); + assertEquals(1, result.get("x")); + } + + @Test + public void shouldParseSingleStringParameter() { + final Map<String, Object> result = GremlinQueryParser.parseParameters("[\"name\":\"marko\"]"); + assertEquals(1, result.size()); + assertEquals("marko", result.get("name")); + } + + @Test + public void shouldParseSingleLongParameter() { + final Map<String, Object> result = GremlinQueryParser.parseParameters("[\"x\":1L]"); + assertEquals(1, result.size()); + assertEquals(1L, result.get("x")); + } + + @Test + public void shouldParseMultipleMixedParameters() { + final Map<String, Object> result = GremlinQueryParser.parseParameters("[\"x\":1,\"name\":\"marko\",\"flag\":true]"); + assertEquals(3, result.size()); + assertEquals(1, result.get("x")); + assertEquals("marko", result.get("name")); + assertEquals(true, result.get("flag")); + } + + @Test + public void shouldParseNullValue() { + final Map<String, Object> result = GremlinQueryParser.parseParameters("[\"x\":null]"); + assertEquals(1, result.size()); + assertNull(result.get("x")); + } + + @Test + public void shouldParseUuidValue() { + final Map<String, Object> result = GremlinQueryParser.parseParameters("[\"id\":UUID(\"bfa9bbe8-c3a3-4017-acc3-cd02dda55e3e\")]"); + assertEquals(1, result.size()); + assertEquals(UUID.fromString("bfa9bbe8-c3a3-4017-acc3-cd02dda55e3e"), result.get("id")); + } + + @Test + public void shouldParseNestedMapValue() { + final Map<String, Object> result = GremlinQueryParser.parseParameters("[\"m\":[\"name\":\"marko\"]]"); + assertEquals(1, result.size()); + assertTrue(result.get("m") instanceof Map); + assertEquals("marko", ((Map<?, ?>) result.get("m")).get("name")); + } + + @Test + public void shouldParseListValue() { + final Map<String, Object> result = GremlinQueryParser.parseParameters("[\"x\":[1,2,3]]"); + assertEquals(1, result.size()); + assertTrue(result.get("x") instanceof List); + assertEquals(3, ((List<?>) result.get("x")).size()); + } + + @Test + public void shouldParseUnicodeKey() { + final Map<String, Object> result = GremlinQueryParser.parseParameters("[\"caf\\u00E9\":1]"); + assertEquals(1, result.size()); + assertEquals(1, result.get("caf\u00e9")); + } + + @Test(expected = GremlinParserException.class) + public void shouldRejectMalformedInput() { + GremlinQueryParser.parseParameters("[\"x\":"); + } + + @Test(expected = GremlinParserException.class) + public void shouldRejectNumericKey() { + GremlinQueryParser.parseParameters("[1:\"value\"]"); + } + + @Test(expected = GremlinParserException.class) + public void shouldRejectEnumKey() { + GremlinQueryParser.parseParameters("[(T.id):\"value\"]"); + } + + @Test(expected = GremlinParserException.class) + public void shouldRejectNonIdentifierStringKey() { + GremlinQueryParser.parseParameters("[\"~id\":1]"); + } + + @Test(expected = GremlinParserException.class) + public void shouldRejectKeyWithSpaces() { + GremlinQueryParser.parseParameters("[\"my key\":1]"); + } + + @Test(expected = GremlinParserException.class) + public void shouldRejectEmptyStringKey() { + GremlinQueryParser.parseParameters("[\"\":1]"); + } + + @Test(expected = GremlinParserException.class) + public void shouldRejectExcessiveNestingDepth() { + final StringBuilder sb = new StringBuilder(); + for (int i = 0; i < 35; i++) { + sb.append("[\"a\":"); + } + sb.append("1"); + for (int i = 0; i < 35; i++) { + sb.append("]"); + } + GremlinQueryParser.parseParameters(sb.toString()); + } + + @Test(expected = GremlinParserException.class) + public void shouldRejectNestedTraversalInValue() { + GremlinQueryParser.parseParameters("[\"x\":__.out()]"); + } + + @Test(expected = GremlinParserException.class) + public void shouldRejectTerminatedTraversalInValue() { + GremlinQueryParser.parseParameters("[\"x\":g.V().drop().iterate()]"); + } + + @Test(expected = GremlinParserException.class) + public void shouldRejectTraversalInNestedList() { + GremlinQueryParser.parseParameters("[\"x\":[__.out()]]"); + } + + @Test(expected = GremlinParserException.class) + public void shouldRejectTraversalInNestedSet() { + GremlinQueryParser.parseParameters("[\"x\":{__.out()}]"); + } + + @Test(expected = GremlinParserException.class) + public void shouldRejectTraversalInNestedMapValue() { + GremlinQueryParser.parseParameters("[\"x\":[\"a\":__.out()]]"); + } + + @Test + public void shouldAcceptStringContainingGremlinLikeContent() { + final Map<String, Object> result = GremlinQueryParser.parseParameters("[\"x\":\"g.V().drop()\"]"); + assertEquals(1, result.size()); + assertEquals("g.V().drop()", result.get("x")); + } + + @Test + public void shouldAcceptStringContainingTraversalSyntax() { + final Map<String, Object> result = GremlinQueryParser.parseParameters("[\"x\":\"__.out().count()\"]"); + assertEquals(1, result.size()); + assertEquals("__.out().count()", result.get("x")); + } + + @Test + public void shouldParseUnquotedIdentifierKey() { + final Map<String, Object> result = GremlinQueryParser.parseParameters("[x:1]"); + assertEquals(1, result.size()); + assertEquals(1, result.get("x")); + } + + @Test + public void shouldParseKeywordAsMapKey() { + final Map<String, Object> result = GremlinQueryParser.parseParameters("[label:\"person\"]"); + assertEquals(1, result.size()); + assertEquals("person", result.get("label")); + } +} diff --git a/gremlin-core/src/test/java/org/apache/tinkerpop/gremlin/process/traversal/GremlinLangTest.java b/gremlin-core/src/test/java/org/apache/tinkerpop/gremlin/process/traversal/GremlinLangTest.java index e541799337..3eb4dd1891 100644 --- a/gremlin-core/src/test/java/org/apache/tinkerpop/gremlin/process/traversal/GremlinLangTest.java +++ b/gremlin-core/src/test/java/org/apache/tinkerpop/gremlin/process/traversal/GremlinLangTest.java @@ -44,6 +44,8 @@ import java.util.Collections; import java.util.Date; import java.util.HashMap; import java.util.HashSet; +import java.util.LinkedHashMap; +import java.util.Map; import java.util.UUID; import static org.apache.tinkerpop.gremlin.process.traversal.AnonymousTraversalSource.traversal; @@ -53,6 +55,8 @@ import static org.apache.tinkerpop.gremlin.process.traversal.dsl.graph.__.hasLab import static org.apache.tinkerpop.gremlin.process.traversal.dsl.graph.__.out; import static org.apache.tinkerpop.gremlin.util.CollectionUtil.asMap; import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertFalse; +import static org.junit.Assert.assertTrue; @RunWith(Parameterized.class) public class GremlinLangTest { @@ -67,14 +71,14 @@ public class GremlinLangTest { @Test public void doTest() { - final String gremlin = traversal.asAdmin().getGremlinLang().getGremlin(); - assertEquals(expected, gremlin); + final GremlinLang gremlinLang = traversal.asAdmin().getGremlinLang(); + assertEquals(expected, gremlinLang.getGremlin()); + assertFalse(gremlinLang.containsUnsupportedTypes()); + assertEquals("", gremlinLang.getUnsupportedType()); } private static GraphTraversalSource newG() { - final GraphTraversalSource g = traversal().with(EmptyGraph.instance()); - g.getGremlinLang().reset(); - return g; + return traversal().with(EmptyGraph.instance()); } @Parameterized.Parameters(name = "{0}") @@ -99,8 +103,6 @@ public class GremlinLangTest { {g.V().has(T.label, "person"), "g.V().has(T.label,\"person\")"}, {g.addE("knows").from(new DetachedVertex(1, "test1", Collections.emptyList())).to(new DetachedVertex(6, "test2", Collections.emptyList())), "g.addE(\"knows\").from(__.V(1)).to(__.V(6))"}, - {newG().E(new ReferenceEdge(1, "test label", new ReferenceVertex(1, "v1"), new ReferenceVertex(1, "v1"))), - "g.E(_0)"}, {g.V().hasId(P.within(Collections.emptyList())).count(), "g.V().hasId(P.within([])).count()"}, {g.V(1).outE().has("weight", P.inside(0.0, 0.6)), "g.V(1).outE().has(\"weight\",P.gt(0.0D).and(P.lt(0.6D)))"}, {g.withSack(1.0, Operator.sum).V(1).local(__.out("knows").barrier(SackFunctions.Barrier.normSack)).in("knows").barrier().sack(), @@ -199,4 +201,404 @@ public class GremlinLangTest { assertEquals("g.inject(Binary(\"AQID\"))", gremlin); } } + + public static class ParameterStringTests { + + @Test + public void shouldSerializeEmptyParameterMap() { + assertEquals("[:]", GremlinLang.convertParametersToString(new HashMap<>())); + assertEquals("[:]", GremlinLang.convertParametersToString(null)); + } + + @Test + public void shouldSerializeSingleStringParameter() { + final Map<String, Object> params = new HashMap<>(); + params.put("name", "marko"); + assertEquals("[\"name\":\"marko\"]", GremlinLang.convertParametersToString(params)); + } + + @Test + public void shouldSerializeSingleIntegerParameter() { + final Map<String, Object> params = new HashMap<>(); + params.put("x", 1); + assertEquals("[\"x\":1]", GremlinLang.convertParametersToString(params)); + } + + @Test + public void shouldSerializeLongParameter() { + final Map<String, Object> params = new HashMap<>(); + params.put("x", 1L); + assertEquals("[\"x\":1L]", GremlinLang.convertParametersToString(params)); + } + + @Test + public void shouldSerializeDoubleParameter() { + final Map<String, Object> params = new HashMap<>(); + params.put("x", 1.5D); + assertEquals("[\"x\":1.5D]", GremlinLang.convertParametersToString(params)); + } + + @Test + public void shouldSerializeBooleanParameter() { + final Map<String, Object> params = new HashMap<>(); + params.put("flag", true); + assertEquals("[\"flag\":true]", GremlinLang.convertParametersToString(params)); + } + + @Test + public void shouldSerializeNullParameter() { + final Map<String, Object> params = new HashMap<>(); + params.put("x", null); + assertEquals("[\"x\":null]", GremlinLang.convertParametersToString(params)); + } + + @Test + public void shouldSerializeUuidParameter() { + final Map<String, Object> params = new HashMap<>(); + final UUID uuid = UUID.fromString("bfa9bbe8-c3a3-4017-acc3-cd02dda55e3e"); + params.put("id", uuid); + assertEquals("[\"id\":UUID(\"bfa9bbe8-c3a3-4017-acc3-cd02dda55e3e\")]", GremlinLang.convertParametersToString(params)); + } + + @Test + public void shouldSerializeStringWithSpecialCharacters() { + final Map<String, Object> params = new HashMap<>(); + params.put("x", "hello \"world\""); + assertEquals("[\"x\":\"hello \\\"world\\\"\"]", GremlinLang.convertParametersToString(params)); + } + + @Test + public void shouldSerializeByteParameter() { + final Map<String, Object> params = new HashMap<>(); + params.put("x", (byte) 1); + assertEquals("[\"x\":1B]", GremlinLang.convertParametersToString(params)); + } + + @Test + public void shouldSerializeShortParameter() { + final Map<String, Object> params = new HashMap<>(); + params.put("x", (short) 1); + assertEquals("[\"x\":1S]", GremlinLang.convertParametersToString(params)); + } + + @Test + public void shouldSerializeBigIntegerParameter() { + final Map<String, Object> params = new HashMap<>(); + params.put("x", BigInteger.valueOf(123)); + assertEquals("[\"x\":123N]", GremlinLang.convertParametersToString(params)); + } + + @Test + public void shouldSerializeBigDecimalParameter() { + final Map<String, Object> params = new HashMap<>(); + params.put("x", BigDecimal.valueOf(1.5)); + assertEquals("[\"x\":1.5M]", GremlinLang.convertParametersToString(params)); + } + + @Test + public void shouldSerializeFloatParameter() { + final Map<String, Object> params = new HashMap<>(); + params.put("x", 1.5F); + assertEquals("[\"x\":1.5F]", GremlinLang.convertParametersToString(params)); + } + + @Test + public void shouldSerializeCharacterParameter() { + final Map<String, Object> params = new HashMap<>(); + params.put("x", 'a'); + assertEquals("[\"x\":\"a\"c]", GremlinLang.convertParametersToString(params)); + } + + @Test + public void shouldSerializeDurationParameter() { + final Map<String, Object> params = new HashMap<>(); + params.put("x", Duration.ofHours(2).plusMinutes(30)); + assertEquals("[\"x\":Duration(\"PT2H30M\")]", GremlinLang.convertParametersToString(params)); + } + + @Test + public void shouldSerializeBinaryParameter() { + final Map<String, Object> params = new HashMap<>(); + params.put("x", ByteBuffer.wrap(new byte[]{1, 2, 3})); + assertEquals("[\"x\":Binary(\"AQID\")]", GremlinLang.convertParametersToString(params)); + } + + @Test + public void shouldSerializeEnumParameter() { + final Map<String, Object> params = new HashMap<>(); + params.put("x", T.id); + assertEquals("[\"x\":T.id]", GremlinLang.convertParametersToString(params)); + } + + @Test + public void shouldSerializeGetParametersAsString() { + final GraphTraversalSource g2 = newG(); + final String result = g2.V(GValue.of("x", 1)). + has("name", GValue.of("name", "marko")). + asAdmin(). + getGremlinLang(). + getParametersAsString(); + // parameters are in a HashMap so order may vary, just check structure + assertTrue(result.startsWith("[")); + assertTrue(result.endsWith("]")); + assertTrue(result, result.contains("\"x\":1")); + assertTrue(result, result.contains("\"name\":\"marko\"")); + } + + @Test + public void shouldSerializeOffsetDateTimeParameter() { + final Map<String, Object> params = new HashMap<>(); + params.put("x", DatetimeHelper.parse("2018-03-21T08:35:44.741Z")); + assertEquals("[\"x\":datetime(\"2018-03-21T08:35:44.741Z\")]", GremlinLang.convertParametersToString(params)); + } + + @Test + public void shouldSerializeDateParameter() { + final Map<String, Object> params = new HashMap<>(); + params.put("x", Date.from(DatetimeHelper.parse("2018-03-21T08:35:44.741Z").toInstant())); + assertEquals("[\"x\":datetime(\"2018-03-21T08:35:44.741Z\")]", GremlinLang.convertParametersToString(params)); + } + + @Test + public void shouldSerializeMultipleParameters() { + // use a LinkedHashMap to guarantee order + final Map<String, Object> params = new LinkedHashMap<>(); + params.put("x", 1); + params.put("name", "marko"); + params.put("flag", true); + assertEquals("[\"x\":1,\"name\":\"marko\",\"flag\":true]", GremlinLang.convertParametersToString(params)); + } + + @Test + public void shouldSerializeMapValuedParameter() { + final Map<String, Object> params = new LinkedHashMap<>(); + final Map<Object, Object> nested = new LinkedHashMap<>(); + nested.put(T.label, "person"); + nested.put("name", "marko"); + params.put("m", nested); + assertEquals("[\"m\":[(T.label):\"person\",\"name\":\"marko\"]]", GremlinLang.convertParametersToString(params)); + } + + @Test + public void shouldSerializeListValuedParameter() { + final Map<String, Object> params = new HashMap<>(); + params.put("x", Arrays.asList(1, 2, 3)); + assertEquals("[\"x\":[1,2,3]]", GremlinLang.convertParametersToString(params)); + } + + @Test + public void shouldSerializeSetValuedParameter() { + // use a LinkedHashSet to guarantee order + final java.util.Set<Integer> set = new java.util.LinkedHashSet<>(Arrays.asList(1, 2)); + final Map<String, Object> params = new HashMap<>(); + params.put("x", set); + assertEquals("[\"x\":{1,2}]", GremlinLang.convertParametersToString(params)); + } + + @Test + public void shouldSerializeStringWithBackslash() { + final Map<String, Object> params = new HashMap<>(); + params.put("x", "path\\to\\file"); + assertEquals("[\"x\":\"path\\\\to\\\\file\"]", GremlinLang.convertParametersToString(params)); + } + + @Test + public void shouldSerializeStringWithUnicode() { + final Map<String, Object> params = new HashMap<>(); + params.put("x", "caf\u00E9"); + assertEquals("[\"x\":\"caf\\u00E9\"]", GremlinLang.convertParametersToString(params)); + } + + @Test + public void shouldHandleEmptyStringKey() { + final Map<String, Object> params = new HashMap<>(); + params.put("", 1); + final String result = GremlinLang.convertParametersToString(params); + assertEquals("[\"\":1]", result); + } + + @Test + public void shouldHandleKeyWithSpaces() { + final Map<String, Object> params = new HashMap<>(); + params.put("my key", 1); + final String result = GremlinLang.convertParametersToString(params); + assertEquals("[\"my key\":1]", result); + } + + @Test + public void shouldHandleNullKey() { + final Map<String, Object> params = new HashMap<>(); + params.put(null, 1); + final String result = GremlinLang.convertParametersToString(params); + assertEquals("[null:1]", result); + } + + @Test + public void shouldSerializeUnicodeKey() { + final Map<String, Object> params = new HashMap<>(); + params.put("caf\u00e9", 1); + assertEquals("[\"caf\\u00E9\":1]", GremlinLang.convertParametersToString(params)); + } + } + + public static class UnsupportedTypeTests { + + /** + * A test-only type that will never have gremlin-lang literal support. + */ + private static class UnsupportedType { + @Override + public String toString() { + return "custom-unsupported"; + } + } + + @Test + public void shouldMarkUnsupportedTypeAndUseToString() { + final UnsupportedType custom = new UnsupportedType(); + final GremlinLang gremlinLang = g.inject(custom).asAdmin().getGremlinLang(); + assertTrue(gremlinLang.containsUnsupportedTypes()); + assertEquals("UnsupportedType", gremlinLang.getUnsupportedType()); + assertTrue(gremlinLang.getGremlin().contains(custom.toString())); + } + + @Test + public void shouldNotAddUnsupportedTypeToParameterMap() { + final GremlinLang gremlinLang = g.inject(new UnsupportedType()).asAdmin().getGremlinLang(); + // unsupported type is flagged but not stored in the parameter map + assertTrue(gremlinLang.containsUnsupportedTypes()); + assertTrue(gremlinLang.getParameters().isEmpty()); + } + + @Test + public void shouldRecordLastUnsupportedType() { + final GremlinLang gremlinLang = g.inject(new UnsupportedType(), new Thread()).asAdmin().getGremlinLang(); + assertTrue(gremlinLang.containsUnsupportedTypes()); + // last one wins + assertEquals("Thread", gremlinLang.getUnsupportedType()); + } + + @Test + public void shouldPropagateFlagFromChildTraversal() { + final GremlinLang gremlinLang = g.V().where(__.has("x", P.eq(new UnsupportedType()))).asAdmin().getGremlinLang(); + assertTrue(gremlinLang.containsUnsupportedTypes()); + assertEquals("UnsupportedType", gremlinLang.getUnsupportedType()); + } + + @Test + public void shouldNotPropagateFlagFromCleanChildTraversal() { + final GremlinLang gremlinLang = g.V().where(__.has("name", "marko")).asAdmin().getGremlinLang(); + assertFalse(gremlinLang.containsUnsupportedTypes()); + assertEquals("", gremlinLang.getUnsupportedType()); + } + + @Test + public void shouldDetectUnsupportedTypeInList() { + final GremlinLang gremlinLang = g.inject(Arrays.asList(1, new UnsupportedType())).asAdmin().getGremlinLang(); + assertTrue(gremlinLang.containsUnsupportedTypes()); + assertEquals("UnsupportedType", gremlinLang.getUnsupportedType()); + } + + @Test + public void shouldDetectUnsupportedTypeInMap() { + final Map<String, Object> map = new HashMap<>(); + map.put("key", new UnsupportedType()); + final GremlinLang gremlinLang = g.inject(map).asAdmin().getGremlinLang(); + assertTrue(gremlinLang.containsUnsupportedTypes()); + assertEquals("UnsupportedType", gremlinLang.getUnsupportedType()); + } + + @Test + public void shouldDetectUnsupportedTypeInSet() { + final java.util.Set<Object> set = new java.util.LinkedHashSet<>(); + set.add(1); + set.add(new UnsupportedType()); + final GremlinLang gremlinLang = g.inject(set).asAdmin().getGremlinLang(); + assertTrue(gremlinLang.containsUnsupportedTypes()); + assertEquals("UnsupportedType", gremlinLang.getUnsupportedType()); + } + + @Test + public void shouldDetectUnsupportedTypeInPredicate() { + final GremlinLang gremlinLang = g.V().has("x", P.eq(new UnsupportedType())).asAdmin().getGremlinLang(); + assertTrue(gremlinLang.containsUnsupportedTypes()); + assertEquals("UnsupportedType", gremlinLang.getUnsupportedType()); + } + + @Test + public void shouldPreserveUnsupportedTypeOnClone() { + final GremlinLang original = g.inject(new UnsupportedType()).asAdmin().getGremlinLang(); + final GremlinLang cloned = original.clone(); + assertTrue(cloned.containsUnsupportedTypes()); + assertEquals("UnsupportedType", cloned.getUnsupportedType()); + } + + @Test + public void shouldPreserveCleanStateOnClone() { + final GremlinLang original = g.V(1).asAdmin().getGremlinLang(); + final GremlinLang cloned = original.clone(); + assertFalse(cloned.containsUnsupportedTypes()); + assertEquals("", cloned.getUnsupportedType()); + } + + @Test + public void shouldStillAddGValueToParameterMap() { + final GremlinLang gremlinLang = g.V(GValue.of("myId", 1)).asAdmin().getGremlinLang(); + assertFalse(gremlinLang.containsUnsupportedTypes()); + assertEquals(1, gremlinLang.getParameters().size()); + assertEquals(1, gremlinLang.getParameters().get("myId")); + } + + @Test + public void shouldDetectUnsupportedTypeForReferenceEdge() { + final GremlinLang gremlinLang = newG().E( + new ReferenceEdge(1, "test label", new ReferenceVertex(1, "v1"), new ReferenceVertex(1, "v1"))) + .asAdmin().getGremlinLang(); + assertTrue(gremlinLang.containsUnsupportedTypes()); + assertEquals("ReferenceEdge", gremlinLang.getUnsupportedType()); + assertEquals("g.E(e[1][1-test label->1])", gremlinLang.getGremlin()); + } + + @Test + public void shouldDetectUnsupportedTypeInMapKey() { + final Map<Object, Object> map = new LinkedHashMap<>(); + map.put(new UnsupportedType(), "value"); + final GremlinLang gremlinLang = g.inject(map).asAdmin().getGremlinLang(); + assertTrue(gremlinLang.containsUnsupportedTypes()); + assertEquals("UnsupportedType", gremlinLang.getUnsupportedType()); + } + + @Test(expected = IllegalArgumentException.class) + public void shouldRejectUnsupportedTypeInConvertParametersToString() { + final Map<String, Object> params = new HashMap<>(); + params.put("x", new UnsupportedType()); + GremlinLang.convertParametersToString(params); + } + + @Test(expected = IllegalArgumentException.class) + public void shouldRejectUnsupportedTypeInGValueViaGetParametersAsString() { + final GremlinLang gremlinLang = g.V(GValue.of("x", new UnsupportedType())).asAdmin().getGremlinLang(); + // named GValue stores the raw value in the parameter map without type-checking, + // so containsUnsupportedTypes() does not detect it + assertFalse(gremlinLang.containsUnsupportedTypes()); + // the error is caught when the parameter map is serialized + gremlinLang.getParametersAsString(); + } + + @Test + public void shouldDetectUnsupportedTypeInUnnamedGValue() { + // unnamed GValue (null name) recurses into argAsString(gValue.get()), + // which hits the unsupported-type fallback and sets the flag + final GremlinLang gremlinLang = g.inject(GValue.of(null, new UnsupportedType())).asAdmin().getGremlinLang(); + assertTrue(gremlinLang.containsUnsupportedTypes()); + assertEquals("UnsupportedType", gremlinLang.getUnsupportedType()); + } + + @Test + public void shouldPersistFlagAfterSupportedTypeFollows() { + final GremlinLang gremlinLang = g.inject(new UnsupportedType(), "hello").asAdmin().getGremlinLang(); + assertTrue(gremlinLang.containsUnsupportedTypes()); + assertEquals("UnsupportedType", gremlinLang.getUnsupportedType()); + } + } } 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 c51f2b8516..d51657f0ae 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 @@ -44,7 +44,7 @@ public final class RequestOptions { public static final RequestOptions EMPTY = RequestOptions.build().create(); private final String graphOrTraversalSource; - private final Map<String, Object> parameters; + private final String parameters; private final Integer batchSize; private final Long timeout; private final String language; @@ -54,7 +54,7 @@ public final class RequestOptions { private RequestOptions(final Builder builder) { this.graphOrTraversalSource = builder.graphOrTraversalSource; - this.parameters = builder.parameters; + this.parameters = builder.parametersString; this.batchSize = builder.batchSize; this.timeout = builder.timeout; this.language = builder.language; @@ -67,7 +67,7 @@ public final class RequestOptions { return Optional.ofNullable(graphOrTraversalSource); } - public Optional<Map<String, Object>> getParameters() { + public Optional<String> getParameters() { return Optional.ofNullable(parameters); } @@ -114,16 +114,17 @@ public final class RequestOptions { if (builder.bulkResults == null) builder.bulkResults(true); - final Map<String, Object> parameters = gremlinLang.getParameters(); - if (parameters != null && !parameters.isEmpty()) { - parameters.forEach(builder::addParameter); + final String parametersString = gremlinLang.getParametersAsString(); + if (!"[:]".equals(parametersString)) { + builder.addParametersString(parametersString); } return builder.create(); } public static final class Builder { private String graphOrTraversalSource = null; - private Map<String, Object> parameters = null; + private Map<String, Object> internalParameters = null; + private String parametersString = null; private Integer batchSize = null; private Long timeout = null; private String materializeProperties = null; @@ -139,7 +140,7 @@ public final class RequestOptions { public static Builder from(final RequestOptions options) { final Builder builder = build(); builder.graphOrTraversalSource = options.graphOrTraversalSource; - builder.parameters = options.parameters; + builder.parametersString = options.parameters; builder.batchSize = options.batchSize; builder.timeout = options.timeout; builder.materializeProperties = options.materializeProperties; @@ -158,11 +159,17 @@ public final class RequestOptions { } /** - * The parameters to pass on the request. + * Adds a parameter to the request. The parameter map is converted to a gremlin-lang map literal string when the + * request is built. This method accumulates parameters into an internal map which is converted on + * {@link #create()}. + * <p> + * Cannot be mixed with {@link #addParametersString(String)}. */ public Builder addParameter(final String name, final Object value) { - if (null == parameters) - parameters = new HashMap<>(); + if (parametersString != null) + throw new IllegalStateException("Cannot mix addParameter() with addParametersString()"); + + if (internalParameters == null) internalParameters = new HashMap<>(); if (ARGS_G.equals(name)) { this.graphOrTraversalSource = (String) value; @@ -172,7 +179,21 @@ public final class RequestOptions { this.language = (String) value; } - parameters.put(name, value); + internalParameters.put(name, value); + return this; + } + + /** + * Sets the parameters as a pre-formatted gremlin-lang map literal string. This allows users to pass parameters + * as a raw gremlin-lang string when using the Client API with a gremlin string. + * <p> + * Cannot be mixed with {@link #addParameter(String, Object)}. + */ + public Builder addParametersString(final String parametersString) { + if (internalParameters != null) + throw new IllegalStateException("Cannot mix addParametersString() with addParameter()"); + + this.parametersString = parametersString; return this; } @@ -228,6 +249,10 @@ public final class RequestOptions { } public RequestOptions create() { + // if parameters were added via addParameter(), convert the map to a string + if (internalParameters != null) { + parametersString = GremlinLang.convertParametersToString(internalParameters); + } return new RequestOptions(this); } } diff --git a/gremlin-driver/src/main/java/org/apache/tinkerpop/gremlin/driver/remote/DriverRemoteConnection.java b/gremlin-driver/src/main/java/org/apache/tinkerpop/gremlin/driver/remote/DriverRemoteConnection.java index c909e48604..baae303ce9 100644 --- a/gremlin-driver/src/main/java/org/apache/tinkerpop/gremlin/driver/remote/DriverRemoteConnection.java +++ b/gremlin-driver/src/main/java/org/apache/tinkerpop/gremlin/driver/remote/DriverRemoteConnection.java @@ -216,6 +216,12 @@ public class DriverRemoteConnection implements RemoteConnection { @Override public <E> CompletableFuture<RemoteTraversal<?, E>> submitAsync(final GremlinLang gremlinLang) throws RemoteConnectionException { + if (gremlinLang.containsUnsupportedTypes()) { + throw new IllegalArgumentException(String.format( + "GremlinLang contains at least one type [%s] that cannot be represented as text.", + gremlinLang.getUnsupportedType())); + } + try { gremlinLang.addG(remoteTraversalSourceName); return client.submitAsync(gremlinLang.getGremlin(), getRequestOptions(gremlinLang)) diff --git a/gremlin-driver/src/main/java/org/apache/tinkerpop/gremlin/driver/remote/TransactionRemoteConnection.java b/gremlin-driver/src/main/java/org/apache/tinkerpop/gremlin/driver/remote/TransactionRemoteConnection.java index 6ea9ce5e28..050ca00740 100644 --- a/gremlin-driver/src/main/java/org/apache/tinkerpop/gremlin/driver/remote/TransactionRemoteConnection.java +++ b/gremlin-driver/src/main/java/org/apache/tinkerpop/gremlin/driver/remote/TransactionRemoteConnection.java @@ -68,6 +68,12 @@ class TransactionRemoteConnection implements RemoteConnection { throw new RemoteConnectionException("Transaction is not open"); } + if (gremlinLang.containsUnsupportedTypes()) { + throw new IllegalArgumentException(String.format( + "GremlinLang contains at least one type [%s] that cannot be represented as text.", + gremlinLang.getUnsupportedType())); + } + try { // Synchronous submission through transaction final ResultSet rs = transaction.submit(gremlinLang.getGremlin(), getRequestOptions(gremlinLang)); 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 06e2507e75..10148abe08 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 @@ -30,7 +30,9 @@ import org.apache.tinkerpop.gremlin.structure.util.reference.ReferenceFactory; import org.apache.tinkerpop.gremlin.util.Tokens; import org.apache.tinkerpop.gremlin.util.message.RequestMessage; +import java.util.HashMap; import java.util.List; +import java.util.Map; import java.util.Optional; import java.util.concurrent.ScheduledExecutorService; import java.util.concurrent.ScheduledFuture; @@ -57,6 +59,7 @@ public class Context { private boolean timeoutExecutorGrabbed = false; private final Object timeoutExecutorLock = new Object(); private String transactionId; // initially null for non-transactional requests and begin() calls; set after transaction creation. + private Map<String, Object> parameters = new HashMap<>(); // only available after string parameters are parsed by grammar. public Context(final RequestMessage requestMessage, final ChannelHandlerContext ctx, final Settings settings, final GraphManager graphManager, @@ -129,6 +132,13 @@ public class Context { this.transactionId = transactionId; } + public Map<String, Object> getParameters() { + return this.parameters; + } + + public void setParameters(final Map<String, Object> parameters) { + this.parameters = parameters; + } /** * Gets the current request to Gremlin Server. 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 fa34d4d762..1e35d098d4 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 @@ -34,6 +34,7 @@ import org.apache.tinkerpop.gremlin.groovy.engine.GremlinExecutor; import org.apache.tinkerpop.gremlin.groovy.jsr223.TimedInterruptTimeoutException; import org.apache.tinkerpop.gremlin.jsr223.GremlinScriptEngine; import org.apache.tinkerpop.gremlin.language.grammar.GremlinParserException; +import org.apache.tinkerpop.gremlin.language.grammar.GremlinQueryParser; import org.apache.tinkerpop.gremlin.process.traversal.Failure; import org.apache.tinkerpop.gremlin.process.traversal.Operator; import org.apache.tinkerpop.gremlin.process.traversal.Order; @@ -238,6 +239,33 @@ public class HttpGremlinEndpointHandler extends SimpleChannelInboundHandler<Requ throw new ProcessingException(GremlinError.transactionalControlRequiresTransaction()); } + // Guard against bad parameters while trying to parse string-based bindings into a Map<String, Object> + if (requestMessage.optionalField(Tokens.ARGS_BINDINGS).isPresent()) { + Map<String, Object> bindings = null; + final String bindingsString = (String) requestMessage.getFields().get(Tokens.ARGS_BINDINGS); + try { + bindings = GremlinQueryParser.parseParameters(bindingsString); + } catch (GremlinParserException e) { + throw new ProcessingException(GremlinError.incorrectParameterFormat(bindingsString, e)); + } + + final String language = requestMessage.<String>optionalField(Tokens.ARGS_LANGUAGE).orElse("gremlin-lang"); + if ("gremlin-groovy".equals(language)) { + final Set<String> badBindings = IteratorUtils.set(IteratorUtils.<String>filter( + bindings.keySet().iterator(), + INVALID_BINDINGS_KEYS::contains)); + if (!badBindings.isEmpty()) { + throw new ProcessingException(GremlinError.binding(badBindings)); + } + } + + if (bindings.size() > settings.maxParameters) { + throw new ProcessingException(GremlinError.binding(bindings.size(), settings.maxParameters)); + } + + requestCtx.setParameters(bindings); + } + // Send back the 200 OK response header here since the response is always chunk transfer encoded. Any // failures that follow this will show up in the response body instead. sendHttpResponse(ctx, OK, createResponseHeaders(ctx, serializer, requestCtx).toArray(CharSequence[]::new)); @@ -371,23 +399,6 @@ public class HttpGremlinEndpointHandler extends SimpleChannelInboundHandler<Requ private void iterateScriptEvalResult(final Context context, MessageSerializer<?> serializer, final RequestMessage message) throws ProcessingException, InterruptedException, ScriptException { - if (message.optionalField(Tokens.ARGS_BINDINGS).isPresent()) { - final Map bindings = (Map) message.getFields().get(Tokens.ARGS_BINDINGS); - if (IteratorUtils.anyMatch(bindings.keySet().iterator(), k -> null == k || !(k instanceof String))) { - throw new ProcessingException(GremlinError.binding()); - } - - final Set<String> badBindings = IteratorUtils.set(IteratorUtils.<String>filter(bindings.keySet().iterator(), INVALID_BINDINGS_KEYS::contains)); - if (!badBindings.isEmpty()) { - throw new ProcessingException(GremlinError.binding(badBindings)); - } - - // ignore control bindings that get passed in with the "#jsr223" prefix - those aren't used in compilation - if (IteratorUtils.count(IteratorUtils.filter(bindings.keySet().iterator(), k -> !k.toString().startsWith("#jsr223"))) > settings.maxParameters) { - throw new ProcessingException(GremlinError.binding(bindings.size(), settings.maxParameters)); - } - } - final Map<String, Object> args = message.getFields(); final String language = args.containsKey(Tokens.ARGS_LANGUAGE) ? (String) args.get(Tokens.ARGS_LANGUAGE) : "gremlin-lang"; final GremlinScriptEngine scriptEngine = gremlinExecutor.getScriptEngineManager().getEngineByName(language); @@ -507,7 +518,7 @@ public class HttpGremlinEndpointHandler extends SimpleChannelInboundHandler<Requ final RequestMessage msg = ctx.getRequestMessage(); // add any bindings to override any other supplied - Optional.ofNullable((Map<String, Object>) msg.getFields().get(Tokens.ARGS_BINDINGS)).ifPresent(bindings::putAll); + bindings.putAll(ctx.getParameters()); if (msg.getFields().containsKey(Tokens.ARGS_G)) { final String aliased = msg.getField(Tokens.ARGS_G); 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 4501697475..cddff5a107 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 @@ -194,13 +194,7 @@ public class HttpRequestMessageDecoder extends MessageToMessageDecoder<FullHttpR final RequestMessage.Builder builder = RequestMessage.build(scriptNode.asText()); final JsonNode bindingsNode = body.get(Tokens.ARGS_BINDINGS); - if (bindingsNode != null && !bindingsNode.isObject()) - throw new IllegalArgumentException("bindings must be a Map"); - - final Map<String, Object> bindings = new HashMap<>(); - if (bindingsNode != null) - bindingsNode.fields().forEachRemaining(kv -> bindings.put(kv.getKey(), fromJsonNode(kv.getValue()))); - builder.addBindings(bindings); + if (bindingsNode != null) builder.addBindings(bindingsNode.asText()); final JsonNode gNode = body.get(Tokens.ARGS_G); if (null != gNode) builder.addG(gNode.asText()); 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 3014438abd..7a5b77faff 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 @@ -69,6 +69,13 @@ public class GremlinError { return new GremlinError(HttpResponseStatus.BAD_REQUEST, message, "InvalidRequestException"); } + public static GremlinError incorrectParameterFormat(final String parameters, final GremlinParserException gpe) { + final String message = String.format( + "The provided parameter string \"%s\" could not be converted into a Map. %s", + parameters, gpe.getMessage()); + return new GremlinError(HttpResponseStatus.BAD_REQUEST, message, "InvalidRequestException"); + } + public static GremlinError binding(final Set<String> badBindings) { final String message = String.format("The message supplies one or more invalid parameters key of [%s] - these are reserved names.", badBindings); diff --git a/gremlin-server/src/test/java/org/apache/tinkerpop/gremlin/server/GremlinDriverTransactionIntegrateTest.java b/gremlin-server/src/test/java/org/apache/tinkerpop/gremlin/server/GremlinDriverTransactionIntegrateTest.java index 9a88487ddf..3cb37d06a1 100644 --- a/gremlin-server/src/test/java/org/apache/tinkerpop/gremlin/server/GremlinDriverTransactionIntegrateTest.java +++ b/gremlin-server/src/test/java/org/apache/tinkerpop/gremlin/server/GremlinDriverTransactionIntegrateTest.java @@ -25,6 +25,7 @@ import org.apache.tinkerpop.gremlin.driver.RequestOptions; import org.apache.tinkerpop.gremlin.driver.Result; import org.apache.tinkerpop.gremlin.driver.remote.DriverRemoteConnection; import org.apache.tinkerpop.gremlin.process.traversal.dsl.graph.GraphTraversalSource; +import org.apache.tinkerpop.gremlin.process.traversal.step.GValue; import org.apache.tinkerpop.gremlin.server.channel.HttpChannelizer; import org.apache.tinkerpop.gremlin.structure.Transaction; import org.apache.tinkerpop.gremlin.structure.Vertex; @@ -614,4 +615,36 @@ public class GremlinDriverTransactionIntegrateTest extends AbstractGremlinServer assertEquals(1L, g.V().hasLabel("val").count().next().longValue()); } + + @Test + public void shouldRejectTransactionWithUnsupportedType() throws Exception { + final GraphTraversalSource g = traversal().with(DriverRemoteConnection.using(cluster, GTX)); + final GraphTraversalSource gtx = g.tx().begin(); + try { + gtx.inject(new Thread()).iterate(); + fail("Expected IllegalArgumentException for unsupported type"); + } catch (IllegalArgumentException ex) { + assertThat(ex.getMessage(), containsString("at least one type")); + assertThat(ex.getMessage(), containsString("Thread")); + assertThat(ex.getMessage(), containsString("cannot be represented as text")); + } finally { + gtx.tx().rollback(); + } + } + + @Test + public void shouldRejectTransactionWithUnsupportedTypeInGValue() throws Exception { + final GraphTraversalSource g = traversal().with(DriverRemoteConnection.using(cluster, GTX)); + final GraphTraversalSource gtx = g.tx().begin(); + try { + gtx.V(GValue.of("x", new Thread())).iterate(); + fail("Expected exception for unsupported type in GValue parameter"); + } catch (Exception ex) { + final Throwable inner = ExceptionHelper.getRootCause(ex); + assertThat(inner.getMessage(), containsString("at least one type")); + assertThat(inner.getMessage(), containsString("cannot be represented as text")); + } finally { + gtx.tx().rollback(); + } + } } 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 7ce01eac70..895b09b75c 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 @@ -329,7 +329,7 @@ public class GremlinServerHttpIntegrateTest extends AbstractGremlinServerIntegra public void should200OnPOSTWithGremlinQueryStringArgumentWithBindingsAndFunction() throws Exception { final CloseableHttpClient httpclient = HttpClients.createDefault(); final HttpPost httppost = new HttpPost(TestClientFactory.createURLString()); - httppost.setEntity(new StringEntity("{\"gremlin\":\"addItUp(Integer.parseInt(x),Integer.parseInt(y))\",\"language\":\"gremlin-groovy\",\"bindings\":{\"x\":\"10\", \"y\":\"10\"}}", Consts.UTF_8)); + httppost.setEntity(new StringEntity("{\"gremlin\":\"addItUp(Integer.parseInt(x),Integer.parseInt(y))\",\"language\":\"gremlin-groovy\",\"bindings\":\"[x:\\\"10\\\",y:\\\"10\\\"]\"}", Consts.UTF_8)); try (final CloseableHttpResponse response = httpclient.execute(httppost)) { assertEquals(200, response.getStatusLine().getStatusCode()); @@ -596,7 +596,7 @@ public class GremlinServerHttpIntegrateTest extends AbstractGremlinServerIntegra final CloseableHttpClient httpclient = HttpClients.createDefault(); final HttpPost httppost = new HttpPost(TestClientFactory.createURLString()); httppost.addHeader("Content-Type", "application/json"); - httppost.setEntity(new StringEntity("{\"gremlin\":\"x+y\", \"bindings\":{\"x\":10, \"y\":10}, \"language\":\"gremlin-groovy\"}", Consts.UTF_8)); + httppost.setEntity(new StringEntity("{\"gremlin\":\"x+y\", \"bindings\":\"[x:10,y:10]\", \"language\":\"gremlin-groovy\"}", Consts.UTF_8)); try (final CloseableHttpResponse response = httpclient.execute(httppost)) { assertEquals(200, response.getStatusLine().getStatusCode()); @@ -612,7 +612,7 @@ public class GremlinServerHttpIntegrateTest extends AbstractGremlinServerIntegra final CloseableHttpClient httpclient = HttpClients.createDefault(); final HttpPost httppost = new HttpPost(TestClientFactory.createURLString()); httppost.addHeader("Content-Type", "application/json"); - httppost.setEntity(new StringEntity("{\"gremlin\":\"g.inject(x)\", \"bindings\":{\"x\":10}, \"language\":\"gremlin-groovy\"}", Consts.UTF_8)); + httppost.setEntity(new StringEntity("{\"gremlin\":\"g.inject(x)\", \"bindings\":\"[x:10]\", \"language\":\"gremlin-groovy\"}", Consts.UTF_8)); try (final CloseableHttpResponse response = httpclient.execute(httppost)) { assertEquals(200, response.getStatusLine().getStatusCode()); @@ -628,7 +628,7 @@ public class GremlinServerHttpIntegrateTest extends AbstractGremlinServerIntegra final CloseableHttpClient httpclient = HttpClients.createDefault(); final HttpPost httppost = new HttpPost(TestClientFactory.createURLString()); httppost.addHeader("Content-Type", "application/json"); - httppost.setEntity(new StringEntity("{\"gremlin\":\"g.inject(x)\", \"bindings\":{\"x\":10.5}, \"language\":\"gremlin-groovy\"}", Consts.UTF_8)); + httppost.setEntity(new StringEntity("{\"gremlin\":\"g.inject(x)\", \"bindings\":\"[x:10.5]\", \"language\":\"gremlin-groovy\"}", Consts.UTF_8)); try (final CloseableHttpResponse response = httpclient.execute(httppost)) { assertEquals(200, response.getStatusLine().getStatusCode()); @@ -644,7 +644,7 @@ public class GremlinServerHttpIntegrateTest extends AbstractGremlinServerIntegra final CloseableHttpClient httpclient = HttpClients.createDefault(); final HttpPost httppost = new HttpPost(TestClientFactory.createURLString()); httppost.addHeader("Content-Type", "application/json"); - httppost.setEntity(new StringEntity("{\"gremlin\":\"g.V(1).out(x).values('name')\", \"bindings\":{\"x\":\"knows\"}, \"g\":\"gmodern\"}", Consts.UTF_8)); + httppost.setEntity(new StringEntity("{\"gremlin\":\"g.V(1).out(x).values('name')\", \"bindings\":\"[x:\\\"knows\\\"]\", \"g\":\"gmodern\"}", Consts.UTF_8)); try (final CloseableHttpResponse response = httpclient.execute(httppost)) { assertEquals(200, response.getStatusLine().getStatusCode()); @@ -660,7 +660,7 @@ public class GremlinServerHttpIntegrateTest extends AbstractGremlinServerIntegra final CloseableHttpClient httpclient = HttpClients.createDefault(); final HttpPost httppost = new HttpPost(TestClientFactory.createURLString()); httppost.addHeader("Content-Type", "application/json"); - httppost.setEntity(new StringEntity("{\"gremlin\":\"g.inject(x)\", \"bindings\":{\"x\":true}, \"language\":\"gremlin-groovy\"}", Consts.UTF_8)); + httppost.setEntity(new StringEntity("{\"gremlin\":\"g.inject(x)\", \"bindings\":\"[x:true]\", \"language\":\"gremlin-groovy\"}", Consts.UTF_8)); try (final CloseableHttpResponse response = httpclient.execute(httppost)) { assertEquals(200, response.getStatusLine().getStatusCode()); @@ -676,7 +676,7 @@ public class GremlinServerHttpIntegrateTest extends AbstractGremlinServerIntegra final CloseableHttpClient httpclient = HttpClients.createDefault(); final HttpPost httppost = new HttpPost(TestClientFactory.createURLString()); httppost.addHeader("Content-Type", "application/json"); - httppost.setEntity(new StringEntity("{\"gremlin\":\"g.inject(x)\", \"bindings\":{\"x\":null}, \"language\":\"gremlin-groovy\"}", Consts.UTF_8)); + httppost.setEntity(new StringEntity("{\"gremlin\":\"g.inject(x)\", \"bindings\":\"[x:null]\", \"language\":\"gremlin-groovy\"}", Consts.UTF_8)); try (final CloseableHttpResponse response = httpclient.execute(httppost)) { assertEquals(200, response.getStatusLine().getStatusCode()); @@ -692,7 +692,7 @@ public class GremlinServerHttpIntegrateTest extends AbstractGremlinServerIntegra final CloseableHttpClient httpclient = HttpClients.createDefault(); final HttpPost httppost = new HttpPost(TestClientFactory.createURLString()); httppost.addHeader("Content-Type", "application/json"); - httppost.setEntity(new StringEntity("{\"gremlin\":\"g.inject(x).unfold()\", \"bindings\":{\"x\":[1,2,3]}, \"language\":\"gremlin-groovy\"}", Consts.UTF_8)); + httppost.setEntity(new StringEntity("{\"gremlin\":\"g.inject(x).unfold()\", \"bindings\":\"[x:[1,2,3]]\", \"language\":\"gremlin-groovy\"}", Consts.UTF_8)); try (final CloseableHttpResponse response = httpclient.execute(httppost)) { assertEquals(200, response.getStatusLine().getStatusCode()); @@ -711,7 +711,7 @@ public class GremlinServerHttpIntegrateTest extends AbstractGremlinServerIntegra final CloseableHttpClient httpclient = HttpClients.createDefault(); final HttpPost httppost = new HttpPost(TestClientFactory.createURLString()); httppost.addHeader("Content-Type", "application/json"); - httppost.setEntity(new StringEntity("{\"gremlin\":\"g.inject(x)\", \"bindings\":{\"x\":{\"y\":1}}, \"language\":\"gremlin-groovy\"}", Consts.UTF_8)); + httppost.setEntity(new StringEntity("{\"gremlin\":\"g.inject(x)\", \"bindings\":\"[x:[\\\"y\\\":1]]\", \"language\":\"gremlin-groovy\"}", Consts.UTF_8)); try (final CloseableHttpResponse response = httpclient.execute(httppost)) { assertEquals(200, response.getStatusLine().getStatusCode()); @@ -1158,7 +1158,7 @@ public class GremlinServerHttpIntegrateTest extends AbstractGremlinServerIntegra public void should400OnPOSTWithInvalidRequestArgsWhenInvalidBindingsSupplied() throws Exception { final GraphSONMessageSerializerV4 serializer = new GraphSONMessageSerializerV4(); final ByteBuf serializedRequest = serializer.serializeRequestAsBinary( - RequestMessage.build("g.V(id)").addBinding("id", "1").create(), + RequestMessage.build("g.V(id)").addBindings("[id:1]").addLanguage("gremlin-groovy").create(), new UnpooledByteBufAllocator(false)); final CloseableHttpClient httpclient = HttpClients.createDefault(); @@ -1168,7 +1168,7 @@ public class GremlinServerHttpIntegrateTest extends AbstractGremlinServerIntegra httppost.setEntity(new ByteArrayEntity(serializedRequest.array())); try (final CloseableHttpResponse response = httpclient.execute(httppost)) { - assertEquals(200, response.getStatusLine().getStatusCode()); + assertEquals(400, response.getStatusLine().getStatusCode()); final String json = EntityUtils.toString(response.getEntity()); final JsonNode node = mapper.readTree(json); assertTrue(node.get("status").get("message").asText().contains("The message supplies one or more invalid parameters key")); 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 9f56cb2f1c..6c602df7a6 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 @@ -532,9 +532,9 @@ public class GremlinServerIntegrateTest extends AbstractGremlinServerIntegration try (SimpleClient client = TestClientFactory.createSimpleHttpClient()) { final Map<String, Object> bindings = new HashMap<>(); - bindings.put("id", "123"); + bindings.put("id", "123"); // "id" is invalid for gremlin-groovy, but not gremlin-lang final RequestMessage request = RequestMessage.build("g.inject(1,2,3,4,5,6,7,8,9,0)") - .addBindings(bindings).create(); + .addBindings(bindings).addLanguage("gremlin-groovy").create(); final CountDownLatch latch = new CountDownLatch(1); final AtomicBoolean pass = new AtomicBoolean(false); client.submit(request, result -> { @@ -717,31 +717,6 @@ public class GremlinServerIntegrateTest extends AbstractGremlinServerIntegration } } - @Test - public void shouldGarbageCollectPhantomButNotHard() throws Exception { - final Cluster cluster = TestClientFactory.open(); - final Client client = cluster.connect(); - - assertEquals(2, client.submit("addItUp(1,1)", groovyRequestOptions).all().join().get(0).getInt()); - assertEquals(0, client.submit("def subtract(x,y){x-y};subtract(1,1)", groovyRequestOptions).all().join().get(0).getInt()); - assertEquals(0, client.submit("subtract(1,1)", groovyRequestOptions).all().join().get(0).getInt()); - - RequestOptions options = RequestOptions.build() - .addParameter(GremlinGroovyScriptEngine.KEY_REFERENCE_TYPE, GremlinGroovyScriptEngine.REFERENCE_TYPE_PHANTOM) - .language("gremlin-groovy") - .create(); - assertEquals(4, client.submit("def multiply(x,y){x*y};multiply(2,2)", options).all().join().get(0).getInt()); - - try { - client.submit("multiply(2,2)", groovyRequestOptions).all().join().get(0).getInt(); - fail("Should throw an exception since reference is phantom."); - } catch (RuntimeException ignored) { - - } finally { - cluster.close(); - } - } - @SuppressWarnings("ThrowableResultOfMethodCallIgnored") @Test public void shouldBlockRequestWhenTooBig() throws Exception { @@ -1088,4 +1063,25 @@ public class GremlinServerIntegrateTest extends AbstractGremlinServerIntegration assertEquals(new BigDecimal("1.0"), client.submit("g.inject(1.0)", gremlinGroovy).one().getObject()); assertEquals(new BigDecimal("-123.456"), client.submit("g.inject(-123.456)", gremlinGroovy).one().getObject()); } + + @Test + public void shouldSubmitWithStringBindingsViaRequestMessage() throws Exception { + try (SimpleClient client = TestClientFactory.createSimpleHttpClient()) { + final RequestMessage request = RequestMessage.build("g.V(x).out(y).values('name')") + .addBindings("[\"x\":1,\"y\":\"knows\"]").addG("gmodern").create(); + final List<ResponseMessage> responses = client.submit(request); + assertEquals(HttpResponseStatus.OK, responses.get(0).getStatus().getCode()); + assertEquals("vadas", responses.get(0).getResult().getData().get(0)); + } + } + + @Test + public void shouldRejectTraversalInjectionInStringBindings() throws Exception { + try (SimpleClient client = TestClientFactory.createSimpleHttpClient()) { + final RequestMessage request = RequestMessage.build("g.V(x)") + .addBindings("[x:__.V().drop()]").addG("gmodern").create(); + final List<ResponseMessage> responses = client.submit(request); + assertEquals(HttpResponseStatus.BAD_REQUEST, responses.get(0).getStatus().getCode()); + } + } } diff --git a/gremlin-server/src/test/java/org/apache/tinkerpop/gremlin/server/HttpDriverIntegrateTest.java b/gremlin-server/src/test/java/org/apache/tinkerpop/gremlin/server/HttpDriverIntegrateTest.java index f6452abd09..b1f6423f7a 100644 --- a/gremlin-server/src/test/java/org/apache/tinkerpop/gremlin/server/HttpDriverIntegrateTest.java +++ b/gremlin-server/src/test/java/org/apache/tinkerpop/gremlin/server/HttpDriverIntegrateTest.java @@ -30,6 +30,7 @@ import org.apache.tinkerpop.gremlin.driver.remote.DriverRemoteConnection; import org.apache.tinkerpop.gremlin.process.traversal.Merge; import org.apache.tinkerpop.gremlin.process.traversal.P; import org.apache.tinkerpop.gremlin.process.traversal.dsl.graph.GraphTraversalSource; +import org.apache.tinkerpop.gremlin.process.traversal.step.GValue; import org.apache.tinkerpop.gremlin.server.channel.HttpChannelizer; import org.apache.tinkerpop.gremlin.structure.VertexProperty; import org.apache.tinkerpop.gremlin.util.CollectionUtil; @@ -238,9 +239,8 @@ public class HttpDriverIntegrateTest extends AbstractGremlinServerIntegrationTes fail("Should have thrown exception over bad serialization"); } catch (Exception ex) { final Throwable inner = ExceptionHelper.getRootCause(ex); - assertThat(inner, instanceOf(ResponseException.class)); - assertEquals(HttpResponseStatus.BAD_REQUEST, ((ResponseException) inner).getResponseStatusCode()); - assertTrue(ex.getMessage().contains("An error occurred during serialization of this request")); + assertThat(inner, instanceOf(IllegalArgumentException.class)); + assertTrue(ex.getMessage().contains("Parameter map contains at least one type [Color] that cannot be represented as text")); } // should not die completely just because we had a bad serialization error. that kind of stuff happens @@ -555,6 +555,39 @@ public class HttpDriverIntegrateTest extends AbstractGremlinServerIntegrationTes } } + @Test + public void shouldRejectTraversalWithUnsupportedType() { + final Cluster cluster = TestClientFactory.build().create(); + try { + final GraphTraversalSource g = traversal().with(DriverRemoteConnection.using(cluster)); + g.inject(new Thread()).toList(); + fail("Expected IllegalArgumentException for unsupported type"); + } catch (IllegalArgumentException ex) { + assertThat(ex.getMessage(), containsString("at least one type")); + assertThat(ex.getMessage(), containsString("Thread")); + assertThat(ex.getMessage(), containsString("cannot be represented as text")); + } finally { + cluster.close(); + } + } + + @Test + public void shouldRejectTraversalWithUnsupportedTypeInGValue() { + final Cluster cluster = TestClientFactory.build().create(); + try { + final GraphTraversalSource g = traversal().with(DriverRemoteConnection.using(cluster)); + g.V(GValue.of("x", new Thread())).toList(); + fail("Expected exception for unsupported type in GValue parameter"); + } catch (Exception ex) { + final Throwable inner = ExceptionHelper.getRootCause(ex); + assertThat(inner, instanceOf(IllegalArgumentException.class)); + assertThat(inner.getMessage(), containsString("at least one type")); + assertThat(inner.getMessage(), containsString("cannot be represented as text")); + } finally { + cluster.close(); + } + } + @Test public void shouldEvalInGremlinLang() { final Cluster cluster = TestClientFactory.build().create(); diff --git a/gremlin-util/src/main/java/org/apache/tinkerpop/gremlin/util/message/RequestMessage.java b/gremlin-util/src/main/java/org/apache/tinkerpop/gremlin/util/message/RequestMessage.java index f25aa3781c..bae392eed7 100644 --- a/gremlin-util/src/main/java/org/apache/tinkerpop/gremlin/util/message/RequestMessage.java +++ b/gremlin-util/src/main/java/org/apache/tinkerpop/gremlin/util/message/RequestMessage.java @@ -18,6 +18,7 @@ */ package org.apache.tinkerpop.gremlin.util.message; +import org.apache.tinkerpop.gremlin.process.traversal.GremlinLang; import org.apache.tinkerpop.gremlin.util.Tokens; import java.util.Collections; @@ -76,18 +77,12 @@ public final class RequestMessage { public static Builder from(final RequestMessage msg) { final Builder builder = build(msg.gremlin); builder.fields.putAll(msg.getFields()); - if (msg.getFields().containsKey(Tokens.ARGS_BINDINGS)) { - builder.addBindings((Map<String, Object>) msg.getFields().get(Tokens.ARGS_BINDINGS)); - } return builder; } public static Builder from(final RequestMessage msg, final String gremlin) { final Builder builder = build(gremlin); builder.fields.putAll(msg.getFields()); - if (msg.getFields().containsKey(Tokens.ARGS_BINDINGS)) { - builder.addBindings((Map<String, Object>) msg.getFields().get(Tokens.ARGS_BINDINGS)); - } return builder; } @@ -108,7 +103,6 @@ public final class RequestMessage { */ public static final class Builder { private final String gremlin; - private final Map<String, Object> bindings = new HashMap<>(); private final Map<String, Object> fields = new HashMap<>(); // Only allow certain items to be added to prevent breaking changes. private Builder(final String gremlin) { @@ -121,14 +115,24 @@ public final class RequestMessage { return this; } - public Builder addBinding(final String key, final Object val) { - bindings.put(key, val); + /** + * Sets the bindings as a gremlin-lang map literal string. Calling this method multiple + * times will replace the previous bindings (last-one-wins). + */ + public Builder addBindings(final String bindingsString) { + Objects.requireNonNull(bindingsString, "bindings argument cannot be null."); + this.fields.put(Tokens.ARGS_BINDINGS, bindingsString); return this; } + /** + * Sets the bindings from a map by converting it to a gremlin-lang map literal string + * using {@link GremlinLang#convertParametersToString(Map)}. Calling this method multiple + * times will replace the previous bindings (last-one-wins). + */ public Builder addBindings(final Map<String, Object> otherBindings) { Objects.requireNonNull(otherBindings, "bindings argument cannot be null."); - this.bindings.putAll(otherBindings); + this.fields.put(Tokens.ARGS_BINDINGS, GremlinLang.convertParametersToString(otherBindings)); return this; } @@ -183,7 +187,6 @@ public final class RequestMessage { * Create the request message given the settings provided to the {@link Builder}. */ public RequestMessage create() { - this.fields.put(Tokens.ARGS_BINDINGS, bindings); return new RequestMessage(gremlin, fields); } } diff --git a/gremlin-util/src/main/java/org/apache/tinkerpop/gremlin/util/ser/AbstractGraphSONMessageSerializerV4.java b/gremlin-util/src/main/java/org/apache/tinkerpop/gremlin/util/ser/AbstractGraphSONMessageSerializerV4.java index caa3def020..eae1ba50a2 100644 --- a/gremlin-util/src/main/java/org/apache/tinkerpop/gremlin/util/ser/AbstractGraphSONMessageSerializerV4.java +++ b/gremlin-util/src/main/java/org/apache/tinkerpop/gremlin/util/ser/AbstractGraphSONMessageSerializerV4.java @@ -323,7 +323,7 @@ public abstract class AbstractGraphSONMessageSerializerV4 extends AbstractMessag builder.addG(data.get(SerTokens.TOKEN_G).toString()); } if (data.containsKey(SerTokens.TOKEN_BINDINGS)) { - builder.addBindings((Map<String, Object>) data.get(SerTokens.TOKEN_BINDINGS)); + builder.addBindings(data.get(SerTokens.TOKEN_BINDINGS).toString()); } if (data.containsKey(Tokens.TIMEOUT_MS)) { // Can be int for untyped JSON and long for typed GraphSON. diff --git a/gremlin-util/src/main/java/org/apache/tinkerpop/gremlin/util/ser/binary/RequestMessageSerializer.java b/gremlin-util/src/main/java/org/apache/tinkerpop/gremlin/util/ser/binary/RequestMessageSerializer.java index 9397e5afde..fc75caad99 100644 --- a/gremlin-util/src/main/java/org/apache/tinkerpop/gremlin/util/ser/binary/RequestMessageSerializer.java +++ b/gremlin-util/src/main/java/org/apache/tinkerpop/gremlin/util/ser/binary/RequestMessageSerializer.java @@ -59,7 +59,7 @@ public class RequestMessageSerializer { builder.addG(fields.get(SerTokens.TOKEN_G).toString()); } if (fields.containsKey(SerTokens.TOKEN_BINDINGS)) { - builder.addBindings((Map<String, Object>) fields.get(SerTokens.TOKEN_BINDINGS)); + builder.addBindings(fields.get(SerTokens.TOKEN_BINDINGS).toString()); } if (fields.containsKey(Tokens.TIMEOUT_MS)) { builder.addTimeoutMillis((long) fields.get(Tokens.TIMEOUT_MS)); diff --git a/gremlin-util/src/test/java/org/apache/tinkerpop/gremlin/util/message/RequestMessageTest.java b/gremlin-util/src/test/java/org/apache/tinkerpop/gremlin/util/message/RequestMessageTest.java index d5577a48f0..13376778a8 100644 --- a/gremlin-util/src/test/java/org/apache/tinkerpop/gremlin/util/message/RequestMessageTest.java +++ b/gremlin-util/src/test/java/org/apache/tinkerpop/gremlin/util/message/RequestMessageTest.java @@ -43,19 +43,42 @@ public class RequestMessageTest { bindings.put("a", "b"); bindings.put("g", "gmodern"); final RequestMessage msg = RequestMessage.build("gremlin").addBindings(bindings).create(); - assertEquals(bindings, msg.getField(Tokens.ARGS_BINDINGS)); + // bindings are now stored as a gremlin-lang map literal string + final String bindingsString = msg.getField(Tokens.ARGS_BINDINGS); + assertTrue(bindingsString.startsWith("[")); + assertTrue(bindingsString.endsWith("]")); + assertTrue(bindingsString.contains("\"a\":\"b\"")); + assertTrue(bindingsString.contains("\"g\":\"gmodern\"")); } @Test - public void shouldSetBindingsWithKeyValue() { - final Map<String, Object> bindings = new HashMap<>(); - bindings.put("a", "b"); - bindings.put("g", "gmodern"); + public void shouldSetBindingsWithString() { final RequestMessage msg = RequestMessage.build("gremlin") - .addBinding("a", "b") - .addBinding("g", "gmodern") + .addBindings("[a:\"b\",g:\"gmodern\"]") .create(); - assertEquals(bindings, msg.getField(Tokens.ARGS_BINDINGS)); + assertEquals("[a:\"b\",g:\"gmodern\"]", msg.getField(Tokens.ARGS_BINDINGS)); + } + + @Test + public void shouldNotContainBindingsWhenNoneSet() { + final RequestMessage msg = RequestMessage.build("g.V()").create(); + assertNull(msg.getField(Tokens.ARGS_BINDINGS)); + } + + @Test + public void shouldOverwriteBindingsOnMultipleCalls() { + final RequestMessage msg = RequestMessage.build("gremlin") + .addBindings("[x:1]") + .addBindings("[y:2]") + .create(); + assertEquals("[y:2]", msg.getField(Tokens.ARGS_BINDINGS)); + } + + @Test(expected = IllegalArgumentException.class) + public void shouldRejectUnsupportedTypeInBindingsMap() { + final Map<String, Object> bindings = new HashMap<>(); + bindings.put("x", new Thread()); + RequestMessage.build("g.V()").addBindings(bindings); } @Test @@ -111,7 +134,10 @@ public class RequestMessageTest { final Map<String, Object> fields = msg.getFields(); assertEquals(g, fields.get(Tokens.ARGS_G)); assertEquals(lang, fields.get(Tokens.ARGS_LANGUAGE)); - assertEquals(bindings, fields.get(Tokens.ARGS_BINDINGS)); + // bindings are now a gremlin-lang string, not the original map + final String bindingsString = (String) fields.get(Tokens.ARGS_BINDINGS); + assertTrue(bindingsString.contains("\"b\":\"c\"")); + assertTrue(bindingsString.contains("\"g\":\"gmodern\"")); assertEquals(query, msg.getGremlin()); } diff --git a/gremlin-util/src/test/java/org/apache/tinkerpop/gremlin/util/ser/binary/MessageSerializerTest.java b/gremlin-util/src/test/java/org/apache/tinkerpop/gremlin/util/ser/binary/MessageSerializerTest.java index 6dfe98c0db..7f1e749222 100644 --- a/gremlin-util/src/test/java/org/apache/tinkerpop/gremlin/util/ser/binary/MessageSerializerTest.java +++ b/gremlin-util/src/test/java/org/apache/tinkerpop/gremlin/util/ser/binary/MessageSerializerTest.java @@ -76,7 +76,7 @@ public class MessageSerializerTest { .addTimeoutMillis(500) .addG("g1") .addLanguage("some-lang") - .addBinding("k", "v") + .addBindings("['k':'v']") .create(); final ByteBuf buffer = serializer.serializeRequestAsBinary(request, allocator); diff --git a/tinkergraph-gremlin/src/test/java/org/apache/tinkerpop/gremlin/tinkergraph/structure/TinkerGraphTest.java b/tinkergraph-gremlin/src/test/java/org/apache/tinkerpop/gremlin/tinkergraph/structure/TinkerGraphTest.java index a9ecce5600..1ef6bc4332 100644 --- a/tinkergraph-gremlin/src/test/java/org/apache/tinkerpop/gremlin/tinkergraph/structure/TinkerGraphTest.java +++ b/tinkergraph-gremlin/src/test/java/org/apache/tinkerpop/gremlin/tinkergraph/structure/TinkerGraphTest.java @@ -28,6 +28,7 @@ import org.apache.tinkerpop.gremlin.process.traversal.Traversal; import org.apache.tinkerpop.gremlin.process.traversal.TraversalStrategy; import org.apache.tinkerpop.gremlin.process.traversal.dsl.graph.GraphTraversalSource; import org.apache.tinkerpop.gremlin.process.traversal.dsl.graph.__; +import org.apache.tinkerpop.gremlin.process.traversal.step.GValue; import org.apache.tinkerpop.gremlin.process.traversal.strategy.optimization.IdentityRemovalStrategy; import org.apache.tinkerpop.gremlin.process.traversal.strategy.verification.ReservedKeysVerificationStrategy; import org.apache.tinkerpop.gremlin.process.traversal.util.Metrics; @@ -923,6 +924,31 @@ public class TinkerGraphTest { assertEquals(3, g.V(100, "1000", uuid).count().next().intValue()); } + @Test + public void shouldExecuteTraversalWithUnsupportedTypeInEmbeddedMode() { + final TinkerGraph graph = TinkerGraph.open(); + final GraphTraversalSource g = traversal().withEmbedded(graph); + + // use a type that has no gremlin-lang literal representation + final Object customValue = new Object(); + final List<Object> result = g.inject(customValue).toList(); + assertEquals(1, result.size()); + assertEquals(customValue, result.get(0)); + } + + @Test + public void shouldExecuteTraversalWithUnsupportedTypeInGValueInEmbeddedMode() { + final TinkerGraph graph = TinkerGraph.open(); + final GraphTraversalSource g = traversal().withEmbedded(graph); + graph.addVertex(T.id, 1, T.label, "person"); + + // named GValue with an unsupported type as the value works in embedded mode + // because the raw object is passed through bindings to the script engine + final Object customValue = 1; + final long count = g.V(GValue.of("myId", customValue)).count().next(); + assertEquals(1L, count); + } + /** * Coerces a {@code Color} to a {@link TinkerGraph} during serialization. Demonstrates how custom serializers * can be developed that can coerce one value to another during serialization.
