This is an automated email from the ASF dual-hosted git repository.
colegreer pushed a commit to branch 3.7-dev
in repository https://gitbox.apache.org/repos/asf/tinkerpop.git
The following commit(s) were added to refs/heads/3.7-dev by this push:
new 16255f5686 TINKERPOP-3040 Fixed limitation in multi-line parsing.
(#3162)
16255f5686 is described below
commit 16255f5686898862f12c40a7efbc5367702e504b
Author: Stephen Mallette <[email protected]>
AuthorDate: Fri Jul 25 15:34:26 2025 -0400
TINKERPOP-3040 Fixed limitation in multi-line parsing. (#3162)
Added a custom 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 | 81 +--------
.../gremlin/console/LocalSafeParser.groovy | 146 ++++++++++++++++
.../gremlin/console/GremlinGroovyshTest.groovy | 26 ++-
.../gremlin/console/LocalSafeParserTest.groovy | 186 +++++++++++++++++++++
5 files changed, 364 insertions(+), 82 deletions(-)
diff --git a/CHANGELOG.asciidoc b/CHANGELOG.asciidoc
index 069d52abff..4cda658fbb 100644
--- a/CHANGELOG.asciidoc
+++ b/CHANGELOG.asciidoc
@@ -44,7 +44,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.
@@ -52,8 +53,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.
* Fixed bug preventing a vertex from being dropped and then re-added in the
same `TinkerTransaction`
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..bb2f7525fd 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,11 +23,10 @@ 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
import org.codehaus.groovy.control.CompilerConfiguration
-import org.codehaus.groovy.control.MultipleCompilationErrorsException
import org.codehaus.groovy.control.customizers.ASTTransformationCustomizer
import org.codehaus.groovy.tools.shell.IO
@@ -42,6 +41,7 @@ class GremlinGroovysh extends Groovysh {
private final static CompilerConfiguration compilerConfig = new
CompilerConfiguration(CompilerConfiguration.DEFAULT) {{
addCompilationCustomizers(new
ASTTransformationCustomizer(ThreadInterrupt.class))
}}
+ private Parsing remoteParser = new LocalSafeParser()
public GremlinGroovysh(final Mediator mediator, final IO io) {
super(io, compilerConfig)
@@ -107,41 +107,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 +144,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/LocalSafeParser.groovy
b/gremlin-console/src/main/groovy/org/apache/tinkerpop/gremlin/console/LocalSafeParser.groovy
new file mode 100644
index 0000000000..9935463ef4
--- /dev/null
+++
b/gremlin-console/src/main/groovy/org/apache/tinkerpop/gremlin/console/LocalSafeParser.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 LocalSafeParser implements Parsing {
+
+ // Try to compile the script
+ private final CompilerConfiguration config = new CompilerConfiguration()
+
+ // Create a custom GroovyClassLoader that doesn't write class files
+ private final GroovyClassLoader 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/LocalSafeParserTest.groovy
b/gremlin-console/src/test/groovy/org/apache/tinkerpop/gremlin/console/LocalSafeParserTest.groovy
new file mode 100644
index 0000000000..76b35966cc
--- /dev/null
+++
b/gremlin-console/src/test/groovy/org/apache/tinkerpop/gremlin/console/LocalSafeParserTest.groovy
@@ -0,0 +1,186 @@
+/*
+ * 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
+
+/**
+ * Unit tests for {@link LocalSafeParser}.
+ */
+class LocalSafeParserTest {
+
+ private LocalSafeParser parser
+
+ @Before
+ void setUp() {
+ parser = new LocalSafeParser()
+ }
+
+ @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
