You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@tinkerpop.apache.org by sp...@apache.org on 2025/07/20 15:15:48 UTC
(tinkerpop) branch TINKERPOP-3040 created (now 543b5f0836)
This is an automated email from the ASF dual-hosted git repository.
spmallette pushed a change to branch TINKERPOP-3040
in repository https://gitbox.apache.org/repos/asf/tinkerpop.git
at 543b5f0836 TINKERPOP-3040 Fixed limitation in multi-line parsing.
This branch includes the following new commits:
new 543b5f0836 TINKERPOP-3040 Fixed limitation in multi-line parsing.
The 1 revisions listed above as "new" are entirely new to this
repository and will be described in separate emails. The revisions
listed as "add" were already present in the repository and have only
been added to this reference.
(tinkerpop) 01/01: TINKERPOP-3040 Fixed limitation in multi-line parsing.
Posted by sp...@apache.org.
This is an automated email from the ASF dual-hosted git repository.
spmallette pushed a commit to branch TINKERPOP-3040
in repository https://gitbox.apache.org/repos/asf/tinkerpop.git
commit 543b5f083640aaa8136b49cb93f8101faa64e03a
Author: Stephen Mallette <st...@amazon.com>
AuthorDate: Sun Jul 20 11:14:04 2025 -0400
TINKERPOP-3040 Fixed limitation in multi-line parsing.
Added a customer parser to help better detect multi-line inputs that does not involve actual evaluation of the input string which can result in scenarios where the script is not sent to the server.
---
CHANGELOG.asciidoc | 7 +-
.../gremlin/console/GremlinGroovysh.groovy | 79 +--------
.../tinkerpop/gremlin/console/RemoteParser.groovy | 146 ++++++++++++++++
.../gremlin/console/GremlinGroovyshTest.groovy | 26 ++-
.../gremlin/console/RemoteParserTest.groovy | 187 +++++++++++++++++++++
5 files changed, 365 insertions(+), 80 deletions(-)
diff --git a/CHANGELOG.asciidoc b/CHANGELOG.asciidoc
index 708e17cc72..b76b537b77 100644
--- a/CHANGELOG.asciidoc
+++ b/CHANGELOG.asciidoc
@@ -43,7 +43,8 @@ image::https://raw.githubusercontent.com/apache/tinkerpop/master/docs/static/ima
* Deprecated `gremlin_python.process.__.has_key_` in favor of `gremlin_python.process.__.has_key`.
* Added `gremlin.spark.outputRepartition` configuration to customize the partitioning of HDFS files from `OutputRDD`.
* Allowed `mergeV()` and `mergeE()` to supply `null` in `Map` values.
-* Change signature of `hasId(P<Object>)` and `hasValue(P<Object>)` to `hasId(P<?>)` and `hasValue(P<?>)`.
+* Fixed limitation in multi-line detection preventing `:remote console` scripts from being sent to the server.
+* Changed signature of `hasId(P<Object>)` and `hasValue(P<Object>)` to `hasId(P<?>)` and `hasValue(P<?>)`.
* Improved error message for when `emit()` is used without `repeat()`.
* Fixed incomplete shading of Jackson multi-release.
* Changed `PythonTranslator` to generate snake case step naming instead of camel case.
@@ -51,8 +52,8 @@ image::https://raw.githubusercontent.com/apache/tinkerpop/master/docs/static/ima
* Fixed bug in `IndexStep` which prevented Java serialization due to non-serializable lambda usage by creating serializable function classes.
* Fixed bug in `CallStep` which prevented Java serialization due to non-serializable `ServiceCallContext` and `Service` usage.
* Fixed bug in `Operator` which was caused only a single method parameter to be Collection type checked instead of all parameters.
-* Support hot reloading of SSL certificates.
-* Increase default `max_content_length`/`max_msg_size` in `gremlin-python` from 4MB to 10MB.
+* Addded support for hot reloading of SSL certificates in Gremlin Server.
+* Increased default `max_content_length`/`max_msg_size` in `gremlin-python` from 4MB to 10MB.
* Added the `PopContaining` interface designed to get label and `Pop` combinations held in a `PopInstruction` object.
[[release-3-7-3]]
diff --git a/gremlin-console/src/main/groovy/org/apache/tinkerpop/gremlin/console/GremlinGroovysh.groovy b/gremlin-console/src/main/groovy/org/apache/tinkerpop/gremlin/console/GremlinGroovysh.groovy
index f5a65e652d..85a47230f6 100644
--- a/gremlin-console/src/main/groovy/org/apache/tinkerpop/gremlin/console/GremlinGroovysh.groovy
+++ b/gremlin-console/src/main/groovy/org/apache/tinkerpop/gremlin/console/GremlinGroovysh.groovy
@@ -23,6 +23,7 @@ import org.apache.groovy.groovysh.Command
import org.apache.groovy.groovysh.Groovysh
import org.apache.groovy.groovysh.ParseCode
import org.apache.groovy.groovysh.Parser
+import org.apache.groovy.groovysh.Parsing
import org.apache.groovy.groovysh.util.CommandArgumentParser
import org.apache.groovy.groovysh.util.ScriptVariableAnalyzer
import org.apache.tinkerpop.gremlin.console.commands.GremlinSetCommand
@@ -42,6 +43,7 @@ class GremlinGroovysh extends Groovysh {
private final static CompilerConfiguration compilerConfig = new CompilerConfiguration(CompilerConfiguration.DEFAULT) {{
addCompilationCustomizers(new ASTTransformationCustomizer(ThreadInterrupt.class))
}}
+ private Parsing remoteParser = new RemoteParser()
public GremlinGroovysh(final Mediator mediator, final IO io) {
super(io, compilerConfig)
@@ -107,41 +109,10 @@ class GremlinGroovysh extends Groovysh {
String importsSpec = this.getImportStatements()
// determine if this script is complete or not - if not it's a multiline script
- def status = parser.parse([importsSpec] + current)
+ def status = remoteParser.parse([importsSpec] + current)
switch (status.code) {
case ParseCode.COMPLETE:
- if (!Boolean.valueOf(getPreference(INTERPRETER_MODE_PREFERENCE_KEY, 'false')) || isTypeOrMethodDeclaration(current)) {
- // Evaluate the current buffer w/imports and dummy statement
- List buff = [importsSpec] + [ 'true' ] + current
- try {
- interp.evaluate(buff)
- } catch(MultipleCompilationErrorsException t) {
- if (isIncompleteCaseOfAntlr4(t)) {
- // treat like INCOMPLETE case
- buffers.updateSelected(current)
- break
- }
- throw t
- } catch (MissingPropertyException mpe) {
- // Ignore any local missing properties since it doesn't affect remote execution.
- }
- } else {
- // Evaluate Buffer wrapped with code storing bounded vars
- try {
- evaluateWithStoredBoundVars(importsSpec, current)
- } catch(MultipleCompilationErrorsException t) {
- if (isIncompleteCaseOfAntlr4(t)) {
- // treat like INCOMPLETE case
- buffers.updateSelected(current)
- break
- }
- throw t
- } catch (MissingPropertyException mpe) {
- // Ignore any local missing properties since it doesn't affect remote execution.
- }
- }
-
// concat script here because commands don't support multi-line
def script = String.join(Parser.getNEWLINE(), current)
setLastResult(mediator.currentRemote().submit([script]))
@@ -175,48 +146,4 @@ class GremlinGroovysh extends Groovysh {
maybeRecordResult(result)
}
-
- private Object evaluateWithStoredBoundVars(String importsSpec, List<String> current) {
- Object result
- String variableBlocks = null
- // To make groovysh behave more like an interpreter, we need to retrieve all bound
- // vars at the end of script execution, and then update them into the groovysh Binding context.
- Set<String> boundVars = ScriptVariableAnalyzer.getBoundVars(importsSpec + Parser.NEWLINE + current.join(Parser.NEWLINE), interp.classLoader)
- if (boundVars) {
- variableBlocks = "$COLLECTED_BOUND_VARS_MAP_VARNAME = new HashMap();"
- boundVars.each({ String varname ->
- // bound vars can be in global or some local scope.
- // We discard locally scoped vars by ignoring MissingPropertyException
- variableBlocks += """
-try {$COLLECTED_BOUND_VARS_MAP_VARNAME[\"$varname\"] = $varname;
-} catch (MissingPropertyException e){}"""
- })
- }
- // Evaluate the current buffer w/imports and dummy statement
- List<String> buff
- if (variableBlocks) {
- buff = [importsSpec] + ['try {', 'true'] + current + ['} finally {' + variableBlocks + '}']
- } else {
- buff = [importsSpec] + ['true'] + current
- }
- interp.evaluate(buff)
-
- if (variableBlocks) {
- def boundVarValues = (Map<String, Object>) interp.context.getVariable(COLLECTED_BOUND_VARS_MAP_VARNAME)
- boundVarValues.each({ String name, Object value -> interp.context.setVariable(name, value) })
- }
-
- return result
- }
-
- private boolean isIncompleteCaseOfAntlr4(MultipleCompilationErrorsException t) {
- // TODO antlr4 parser errors pop out here - can we rework to be like antlr2?
- (
- (t.message.contains('Unexpected input: ') || t.message.contains('Unexpected character: ')) && !(
- t.message.contains("Unexpected input: '}'")
- || t.message.contains("Unexpected input: ')'")
- || t.message.contains("Unexpected input: ']'")
- )
- )
- }
}
diff --git a/gremlin-console/src/main/groovy/org/apache/tinkerpop/gremlin/console/RemoteParser.groovy b/gremlin-console/src/main/groovy/org/apache/tinkerpop/gremlin/console/RemoteParser.groovy
new file mode 100644
index 0000000000..b278d89e77
--- /dev/null
+++ b/gremlin-console/src/main/groovy/org/apache/tinkerpop/gremlin/console/RemoteParser.groovy
@@ -0,0 +1,146 @@
+/*
+ * 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.console
+
+import org.apache.groovy.groovysh.ParseCode
+import org.apache.groovy.groovysh.ParseStatus
+import org.apache.groovy.groovysh.Parsing
+import org.codehaus.groovy.control.CompilerConfiguration
+import org.codehaus.groovy.control.MultipleCompilationErrorsException
+import org.codehaus.groovy.control.messages.Message
+import org.codehaus.groovy.control.messages.SyntaxErrorMessage
+
+/**
+ * A {@link Parsing} implementation that detects if a Groovy script is complete or incomplete. This implementation uses
+ * the Groovy compiler to try to compile the script and analyzes any compilation errors to determine if the script is
+ * incomplete. It does not evaluate the script, which prevents local execution and potential local failure.
+ */
+class RemoteParser implements Parsing {
+
+ // Try to compile the script
+ def config = new CompilerConfiguration()
+
+ // Create a custom GroovyClassLoader that doesn't write class files
+ def gcl = new GroovyClassLoader(this.class.classLoader, config) {
+ @Override
+ public Class defineClass(String name, byte[] bytes) {
+ // Skip writing to disk, just define the class in memory
+ return super.defineClass(name, bytes, 0, bytes.length)
+ }
+ }
+ /**
+ * Parse the given buffer and determine if it's a complete Groovy script.
+ *
+ * @param buffer The list of strings representing the script lines
+ * @return A ParseStatus indicating if the script is complete, incomplete, or has errors
+ */
+ @Override
+ ParseStatus parse(Collection<String> buffer) {
+ if (!buffer) {
+ return new ParseStatus(ParseCode.COMPLETE)
+ }
+
+ // join the buffer lines into a single script
+ def script = buffer.join('\n')
+
+ try {
+ // Parse the script without generating .class files
+ gcl.parseClass(script)
+ // If we get here, the script compiled successfully
+ return new ParseStatus(ParseCode.COMPLETE)
+ } catch (MultipleCompilationErrorsException e) {
+ // Check if the errors indicate an incomplete script
+ if (isIncompleteScript(e)) {
+ return new ParseStatus(ParseCode.INCOMPLETE)
+ } else {
+ // Other compilation errors
+ return new ParseStatus(ParseCode.ERROR, e)
+ }
+ } catch (Exception e) {
+ // Any other exception is considered an error
+ return new ParseStatus(ParseCode.ERROR, e)
+ }
+ }
+
+ /**
+ * Determine if the compilation errors indicate an incomplete script.
+ *
+ * @param e The compilation errors exception
+ * @return true if the script is incomplete, false otherwise
+ */
+ private boolean isIncompleteScript(MultipleCompilationErrorsException e) {
+ def errorCollector = e.errorCollector
+
+ for (Message message : errorCollector.errors) {
+ if (message instanceof SyntaxErrorMessage) {
+ def syntaxException = message.cause
+ def errorMessage = syntaxException.message
+
+ // Check for common indicators of incomplete scripts
+ if (isUnexpectedEOF(errorMessage) ||
+ isUnclosedBracket(errorMessage) ||
+ isUnclosedString(errorMessage) ||
+ isUnexpectedInput(errorMessage)) {
+ return true
+ }
+ }
+ }
+
+ return false
+ }
+
+ /**
+ * Check if the error message indicates an unexpected end of file.
+ */
+ private boolean isUnexpectedEOF(String errorMessage) {
+ errorMessage.contains('unexpected end of file') ||
+ errorMessage.contains('Unexpected EOF')
+ }
+
+ /**
+ * Check if the error message indicates an unclosed bracket.
+ */
+ private boolean isUnclosedBracket(String errorMessage) {
+ errorMessage.contains("Missing '}'") ||
+ errorMessage.contains("Missing ')'") ||
+ errorMessage.contains("Missing ']'")
+ }
+
+ /**
+ * Check if the error message indicates an unclosed string.
+ */
+ private boolean isUnclosedString(String errorMessage) {
+ errorMessage.contains('String literal is not terminated') ||
+ errorMessage.contains('Unterminated string literal')
+ }
+
+ /**
+ * Check if the error message indicates unexpected input that might suggest
+ * an incomplete script rather than a syntax error.
+ */
+ private boolean isUnexpectedInput(String errorMessage) {
+ // Check for unexpected input but exclude cases where closing brackets are unexpected
+ // as those typically indicate syntax errors rather than incomplete scripts
+ (errorMessage.contains('Unexpected input: ') ||
+ errorMessage.contains('Unexpected character: ')) &&
+ !(errorMessage.contains("Unexpected input: '}'") ||
+ errorMessage.contains("Unexpected input: ')'") ||
+ errorMessage.contains("Unexpected input: ']'"))
+ }
+}
diff --git a/gremlin-console/src/test/groovy/org/apache/tinkerpop/gremlin/console/GremlinGroovyshTest.groovy b/gremlin-console/src/test/groovy/org/apache/tinkerpop/gremlin/console/GremlinGroovyshTest.groovy
index 4ecb5f562a..998fbbf89a 100644
--- a/gremlin-console/src/test/groovy/org/apache/tinkerpop/gremlin/console/GremlinGroovyshTest.groovy
+++ b/gremlin-console/src/test/groovy/org/apache/tinkerpop/gremlin/console/GremlinGroovyshTest.groovy
@@ -22,11 +22,14 @@ import org.apache.tinkerpop.gremlin.console.commands.RemoteCommand
import org.apache.tinkerpop.gremlin.console.jsr223.AbstractGremlinServerIntegrationTest
import org.apache.tinkerpop.gremlin.console.jsr223.DriverRemoteAcceptor
import org.apache.tinkerpop.gremlin.console.jsr223.MockGroovyGremlinShellEnvironment
+import org.apache.tinkerpop.gremlin.jsr223.console.RemoteException
import org.codehaus.groovy.tools.shell.IO
import org.junit.Test
import java.nio.file.Paths
+import static org.junit.Assert.fail;
+
class GremlinGroovyshTest extends AbstractGremlinServerIntegrationTest {
private IO testio
private ByteArrayOutputStream out
@@ -92,11 +95,32 @@ class GremlinGroovyshTest extends AbstractGremlinServerIntegrationTest {
void shouldNotSubmitIncompleteLinesFromRemoteConsole() {
setupRemote(shell)
shell.execute(":remote console")
- shell.execute("if (0 == g.V().count()) {")
+ shell.execute("if (0 == g.V().count().next()) {")
assert (0 != shell.buffers.current().size())
}
+ /**
+ * TINKERPOP-3040 - prior to 3.7.4, the console evaluated scripts locally before sending to the server which could
+ * create a situation where there were classes needed locally for that eval to succeed for the script to be sent
+ * and would therefore end in exception and not allow the send. the console shouldn't be evaluating scripts locally
+ * to determine their validity. The console only wants to determine if they are complete for multi-line submission
+ * on <enter>. This test simulates this situation by throwing an exception and asserting it is coming from the
+ * server as a RemoteException. If it had executed locally we would have just gotten a DefaultTemporaryException.
+ */
+ @Test
+ void shouldNotEvalToDetermineIncompleteLinesToSubmitForRemoteConsole() {
+ setupRemote(shell)
+ shell.execute(":remote console")
+
+ try {
+ shell.execute("throw new org.apache.tinkerpop.gremlin.server.util.DefaultTemporaryException('kaboom!!')")
+ fail("Should have thrown a remote exception")
+ } catch (RemoteException ex) {
+ assert ("kaboom!!" == ex.message)
+ }
+ }
+
@Test
void shouldGetGremlinResultFromRemoteConsoleInInterpreterMode() {
setupRemote(shell)
diff --git a/gremlin-console/src/test/groovy/org/apache/tinkerpop/gremlin/console/RemoteParserTest.groovy b/gremlin-console/src/test/groovy/org/apache/tinkerpop/gremlin/console/RemoteParserTest.groovy
new file mode 100644
index 0000000000..4bc2576a7e
--- /dev/null
+++ b/gremlin-console/src/test/groovy/org/apache/tinkerpop/gremlin/console/RemoteParserTest.groovy
@@ -0,0 +1,187 @@
+/*
+ * 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.console
+
+import org.apache.groovy.groovysh.ParseCode
+import org.apache.groovy.groovysh.ParseStatus
+import org.junit.Before
+import org.junit.Test
+
+import static org.junit.Assert.assertEquals
+import static org.junit.Assert.assertNotNull
+import static org.junit.Assert.assertTrue
+
+/**
+ * Unit tests for {@link RemoteParser}.
+ */
+class RemoteParserTest {
+
+ private RemoteParser parser
+
+ @Before
+ void setUp() {
+ parser = new RemoteParser()
+ }
+
+ @Test
+ void shouldReturnCompleteForEmptyBuffer() {
+ def status = parser.parse([])
+ assertEquals(ParseCode.COMPLETE, status.code)
+ }
+
+ @Test
+ void shouldReturnCompleteForNullBuffer() {
+ def status = parser.parse(null)
+ assertEquals(ParseCode.COMPLETE, status.code)
+ }
+
+ @Test
+ void shouldReturnCompleteForValidScript() {
+ def script = ["def x = 1", "println x"]
+ def status = parser.parse(script)
+ assertEquals(ParseCode.COMPLETE, status.code)
+ }
+
+ @Test
+ void shouldReturnCompleteForValidSingleLineScript() {
+ def script = ["def x = 1; println x"]
+ def status = parser.parse(script)
+ assertEquals(ParseCode.COMPLETE, status.code)
+ }
+
+ @Test
+ void shouldReturnIncompleteForUnclosedCurlyBracket() {
+ def script = ["if (true) {", " println 'hello'"]
+ def status = parser.parse(script)
+ assertEquals(ParseCode.INCOMPLETE, status.code)
+ }
+
+ @Test
+ void shouldReturnIncompleteForUnclosedParenthesis() {
+ def script = ["println(1 + 2"]
+ def status = parser.parse(script)
+ assertEquals(ParseCode.INCOMPLETE, status.code)
+ }
+
+ @Test
+ void shouldReturnIncompleteForUnclosedSquareBracket() {
+ def script = ["def list = [1, 2, 3"]
+ def status = parser.parse(script)
+ assertEquals(ParseCode.INCOMPLETE, status.code)
+ }
+
+ @Test
+ void shouldReturnIncompleteForUnclosedSingleQuoteString() {
+ def script = ["def s = 'unclosed string"]
+ def status = parser.parse(script)
+ assertEquals(ParseCode.INCOMPLETE, status.code)
+ }
+
+ @Test
+ void shouldReturnIncompleteForUnclosedDoubleQuoteString() {
+ def script = ["def s = \"unclosed string"]
+ def status = parser.parse(script)
+ assertEquals(ParseCode.INCOMPLETE, status.code)
+ }
+
+ @Test
+ void shouldReturnIncompleteForUnclosedTripleQuoteString() {
+ def script = ["def s = '''unclosed triple quote string"]
+ ParseStatus status = parser.parse(script)
+ assertEquals(ParseCode.INCOMPLETE, status.code)
+ }
+
+ @Test
+ void shouldReturnIncompleteForUnexpectedEOF() {
+ def script = ["def method() {"]
+ def status = parser.parse(script)
+ assertEquals(ParseCode.INCOMPLETE, status.code)
+ }
+
+ @Test
+ void shouldReturnIncompleteForUnexpectedInput() {
+ def script = ["def x = 1 +"]
+ def status = parser.parse(script)
+ assertEquals(ParseCode.INCOMPLETE, status.code)
+ }
+
+ @Test
+ void shouldReturnIncompleteForInvalidSyntax() {
+ // Variable name cannot start with a number
+ def script = ["def 1x = 1"]
+ def status = parser.parse(script)
+ assertEquals(ParseCode.INCOMPLETE, status.code)
+ }
+
+ @Test
+ void shouldReturnCompleteForSyntaxError() {
+ def script = ["def x = 1;", "x.nonExistentMethod()"]
+ def status = parser.parse(script)
+ assertEquals(ParseCode.COMPLETE, status.code)
+ }
+
+ @Test
+ void shouldReturnErrorForUnexpectedClosingBracket() {
+ def script = ["def x = 1}", "println x"]
+ def status = parser.parse(script)
+ assertEquals(ParseCode.ERROR, status.code)
+ assertNotNull(status.cause)
+ }
+
+ @Test
+ void shouldReturnIncompleteForMultilineIncompleteScript() {
+ def script = [
+ "def method() {",
+ " if (true) {",
+ " println 'hello'",
+ " }"
+ ]
+
+ def status = parser.parse(script)
+ assertEquals(ParseCode.INCOMPLETE, status.code)
+ }
+
+ @Test
+ void shouldReturnCompleteForMultilineCompleteScript() {
+ def script = [
+ "def method() {",
+ " if (true) {",
+ " println 'hello'",
+ " }",
+ "}"
+ ]
+
+ def status = parser.parse(script)
+ assertEquals(ParseCode.COMPLETE, status.code)
+ }
+
+ @Test
+ void shouldReturnIncompleteForIncompleteGremlinTraversal() {
+ def script = ["g.V().has("]
+ def status = parser.parse(script)
+ assertEquals(ParseCode.INCOMPLETE, status.code)
+ }
+
+ @Test
+ void shouldReturnCompleteForCompleteGremlinTraversal() {
+ def script = ["g.V().count()"]
+ def status = parser.parse(script)
+ assertEquals(ParseCode.COMPLETE, status.code)
+ }
+}
\ No newline at end of file