Skip to content

TINKERPOP-3040 Fixed limitation in multi-line parsing. #3162

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Jul 25, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 4 additions & 3 deletions CHANGELOG.asciidoc
Original file line number Diff line number Diff line change
Expand Up @@ -44,16 +44,17 @@ 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.
* Changed `gremlin-go` Client `ReadBufferSize` and `WriteBufferSize` defaults to 1048576 (1MB) to align with DriverRemoteConnection.
* 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`

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand All @@ -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)
Expand Down Expand Up @@ -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)) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does interpretorMode no longer need to be specifically handled with the new RemoteParser?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I sense this whole section of code i removed was all unnecessary. Note the interpreterMode tests are still passing so I think it's ok.

// 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) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This MissingPropertyException handling was not ported into the new RemoteParser, was that intentional?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you can't have a MissingPropertyException anymore because this change stops doing a local evaluation of the code in the interpreter. now it just parses for correctness in the script itself.

// 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]))
Expand Down Expand Up @@ -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: ']'")
)
)
}
}
Original file line number Diff line number Diff line change
@@ -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: ']'"))
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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)
Expand Down
Loading
Loading