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 e93f8bf70148703c33b4a8c9f5d0f15555c436e0 Author: Stephen Mallette <[email protected]> AuthorDate: Sun Jul 20 11:14:04 2025 -0400 TINKERPOP-3040 Fixed limitation in multi-line parsing. 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 | 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
