Skip to content

Commit 185be42

Browse files
committed
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.
1 parent b8f7c24 commit 185be42

File tree

5 files changed

+365
-80
lines changed

5 files changed

+365
-80
lines changed

CHANGELOG.asciidoc

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -44,16 +44,17 @@ image::https://raw.githubusercontent.com/apache/tinkerpop/master/docs/static/ima
4444
* Deprecated `gremlin_python.process.__.has_key_` in favor of `gremlin_python.process.__.has_key`.
4545
* Added `gremlin.spark.outputRepartition` configuration to customize the partitioning of HDFS files from `OutputRDD`.
4646
* Allowed `mergeV()` and `mergeE()` to supply `null` in `Map` values.
47-
* Change signature of `hasId(P<Object>)` and `hasValue(P<Object>)` to `hasId(P<?>)` and `hasValue(P<?>)`.
47+
* Fixed limitation in multi-line detection preventing `:remote console` scripts from being sent to the server.
48+
* Changed signature of `hasId(P<Object>)` and `hasValue(P<Object>)` to `hasId(P<?>)` and `hasValue(P<?>)`.
4849
* Improved error message for when `emit()` is used without `repeat()`.
4950
* Fixed incomplete shading of Jackson multi-release.
5051
* Changed `PythonTranslator` to generate snake case step naming instead of camel case.
5152
* Changed `gremlin-go` Client `ReadBufferSize` and `WriteBufferSize` defaults to 1048576 (1MB) to align with DriverRemoteConnection.
5253
* Fixed bug in `IndexStep` which prevented Java serialization due to non-serializable lambda usage by creating serializable function classes.
5354
* Fixed bug in `CallStep` which prevented Java serialization due to non-serializable `ServiceCallContext` and `Service` usage.
5455
* Fixed bug in `Operator` which was caused only a single method parameter to be Collection type checked instead of all parameters.
55-
* Support hot reloading of SSL certificates.
56-
* Increase default `max_content_length`/`max_msg_size` in `gremlin-python` from 4MB to 10MB.
56+
* Addded support for hot reloading of SSL certificates in Gremlin Server.
57+
* Increased default `max_content_length`/`max_msg_size` in `gremlin-python` from 4MB to 10MB.
5758
* Added the `PopContaining` interface designed to get label and `Pop` combinations held in a `PopInstruction` object.
5859
* Fixed bug preventing a vertex from being dropped and then re-added in the same `TinkerTransaction`
5960

gremlin-console/src/main/groovy/org/apache/tinkerpop/gremlin/console/GremlinGroovysh.groovy

Lines changed: 3 additions & 76 deletions
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,7 @@ import org.apache.groovy.groovysh.Command
2323
import org.apache.groovy.groovysh.Groovysh
2424
import org.apache.groovy.groovysh.ParseCode
2525
import org.apache.groovy.groovysh.Parser
26+
import org.apache.groovy.groovysh.Parsing
2627
import org.apache.groovy.groovysh.util.CommandArgumentParser
2728
import org.apache.groovy.groovysh.util.ScriptVariableAnalyzer
2829
import org.apache.tinkerpop.gremlin.console.commands.GremlinSetCommand
@@ -42,6 +43,7 @@ class GremlinGroovysh extends Groovysh {
4243
private final static CompilerConfiguration compilerConfig = new CompilerConfiguration(CompilerConfiguration.DEFAULT) {{
4344
addCompilationCustomizers(new ASTTransformationCustomizer(ThreadInterrupt.class))
4445
}}
46+
private Parsing remoteParser = new RemoteParser()
4547

4648
public GremlinGroovysh(final Mediator mediator, final IO io) {
4749
super(io, compilerConfig)
@@ -107,41 +109,10 @@ class GremlinGroovysh extends Groovysh {
107109
String importsSpec = this.getImportStatements()
108110

109111
// determine if this script is complete or not - if not it's a multiline script
110-
def status = parser.parse([importsSpec] + current)
112+
def status = remoteParser.parse([importsSpec] + current)
111113

112114
switch (status.code) {
113115
case ParseCode.COMPLETE:
114-
if (!Boolean.valueOf(getPreference(INTERPRETER_MODE_PREFERENCE_KEY, 'false')) || isTypeOrMethodDeclaration(current)) {
115-
// Evaluate the current buffer w/imports and dummy statement
116-
List buff = [importsSpec] + [ 'true' ] + current
117-
try {
118-
interp.evaluate(buff)
119-
} catch(MultipleCompilationErrorsException t) {
120-
if (isIncompleteCaseOfAntlr4(t)) {
121-
// treat like INCOMPLETE case
122-
buffers.updateSelected(current)
123-
break
124-
}
125-
throw t
126-
} catch (MissingPropertyException mpe) {
127-
// Ignore any local missing properties since it doesn't affect remote execution.
128-
}
129-
} else {
130-
// Evaluate Buffer wrapped with code storing bounded vars
131-
try {
132-
evaluateWithStoredBoundVars(importsSpec, current)
133-
} catch(MultipleCompilationErrorsException t) {
134-
if (isIncompleteCaseOfAntlr4(t)) {
135-
// treat like INCOMPLETE case
136-
buffers.updateSelected(current)
137-
break
138-
}
139-
throw t
140-
} catch (MissingPropertyException mpe) {
141-
// Ignore any local missing properties since it doesn't affect remote execution.
142-
}
143-
}
144-
145116
// concat script here because commands don't support multi-line
146117
def script = String.join(Parser.getNEWLINE(), current)
147118
setLastResult(mediator.currentRemote().submit([script]))
@@ -175,48 +146,4 @@ class GremlinGroovysh extends Groovysh {
175146

176147
maybeRecordResult(result)
177148
}
178-
179-
private Object evaluateWithStoredBoundVars(String importsSpec, List<String> current) {
180-
Object result
181-
String variableBlocks = null
182-
// To make groovysh behave more like an interpreter, we need to retrieve all bound
183-
// vars at the end of script execution, and then update them into the groovysh Binding context.
184-
Set<String> boundVars = ScriptVariableAnalyzer.getBoundVars(importsSpec + Parser.NEWLINE + current.join(Parser.NEWLINE), interp.classLoader)
185-
if (boundVars) {
186-
variableBlocks = "$COLLECTED_BOUND_VARS_MAP_VARNAME = new HashMap();"
187-
boundVars.each({ String varname ->
188-
// bound vars can be in global or some local scope.
189-
// We discard locally scoped vars by ignoring MissingPropertyException
190-
variableBlocks += """
191-
try {$COLLECTED_BOUND_VARS_MAP_VARNAME[\"$varname\"] = $varname;
192-
} catch (MissingPropertyException e){}"""
193-
})
194-
}
195-
// Evaluate the current buffer w/imports and dummy statement
196-
List<String> buff
197-
if (variableBlocks) {
198-
buff = [importsSpec] + ['try {', 'true'] + current + ['} finally {' + variableBlocks + '}']
199-
} else {
200-
buff = [importsSpec] + ['true'] + current
201-
}
202-
interp.evaluate(buff)
203-
204-
if (variableBlocks) {
205-
def boundVarValues = (Map<String, Object>) interp.context.getVariable(COLLECTED_BOUND_VARS_MAP_VARNAME)
206-
boundVarValues.each({ String name, Object value -> interp.context.setVariable(name, value) })
207-
}
208-
209-
return result
210-
}
211-
212-
private boolean isIncompleteCaseOfAntlr4(MultipleCompilationErrorsException t) {
213-
// TODO antlr4 parser errors pop out here - can we rework to be like antlr2?
214-
(
215-
(t.message.contains('Unexpected input: ') || t.message.contains('Unexpected character: ')) && !(
216-
t.message.contains("Unexpected input: '}'")
217-
|| t.message.contains("Unexpected input: ')'")
218-
|| t.message.contains("Unexpected input: ']'")
219-
)
220-
)
221-
}
222149
}
Lines changed: 146 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,146 @@
1+
/*
2+
* Licensed to the Apache Software Foundation (ASF) under one
3+
* or more contributor license agreements. See the NOTICE file
4+
* distributed with this work for additional information
5+
* regarding copyright ownership. The ASF licenses this file
6+
* to you under the Apache License, Version 2.0 (the
7+
* "License"); you may not use this file except in compliance
8+
* with the License. You may obtain a copy of the License at
9+
*
10+
* http://www.apache.org/licenses/LICENSE-2.0
11+
*
12+
* Unless required by applicable law or agreed to in writing,
13+
* software distributed under the License is distributed on an
14+
* "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
15+
* KIND, either express or implied. See the License for the
16+
* specific language governing permissions and limitations
17+
* under the License.
18+
*/
19+
package org.apache.tinkerpop.gremlin.console
20+
21+
import org.apache.groovy.groovysh.ParseCode
22+
import org.apache.groovy.groovysh.ParseStatus
23+
import org.apache.groovy.groovysh.Parsing
24+
import org.codehaus.groovy.control.CompilerConfiguration
25+
import org.codehaus.groovy.control.MultipleCompilationErrorsException
26+
import org.codehaus.groovy.control.messages.Message
27+
import org.codehaus.groovy.control.messages.SyntaxErrorMessage
28+
29+
/**
30+
* A {@link Parsing} implementation that detects if a Groovy script is complete or incomplete. This implementation uses
31+
* the Groovy compiler to try to compile the script and analyzes any compilation errors to determine if the script is
32+
* incomplete. It does not evaluate the script, which prevents local execution and potential local failure.
33+
*/
34+
class RemoteParser implements Parsing {
35+
36+
// Try to compile the script
37+
def config = new CompilerConfiguration()
38+
39+
// Create a custom GroovyClassLoader that doesn't write class files
40+
def gcl = new GroovyClassLoader(this.class.classLoader, config) {
41+
@Override
42+
public Class defineClass(String name, byte[] bytes) {
43+
// Skip writing to disk, just define the class in memory
44+
return super.defineClass(name, bytes, 0, bytes.length)
45+
}
46+
}
47+
/**
48+
* Parse the given buffer and determine if it's a complete Groovy script.
49+
*
50+
* @param buffer The list of strings representing the script lines
51+
* @return A ParseStatus indicating if the script is complete, incomplete, or has errors
52+
*/
53+
@Override
54+
ParseStatus parse(Collection<String> buffer) {
55+
if (!buffer) {
56+
return new ParseStatus(ParseCode.COMPLETE)
57+
}
58+
59+
// join the buffer lines into a single script
60+
def script = buffer.join('\n')
61+
62+
try {
63+
// Parse the script without generating .class files
64+
gcl.parseClass(script)
65+
// If we get here, the script compiled successfully
66+
return new ParseStatus(ParseCode.COMPLETE)
67+
} catch (MultipleCompilationErrorsException e) {
68+
// Check if the errors indicate an incomplete script
69+
if (isIncompleteScript(e)) {
70+
return new ParseStatus(ParseCode.INCOMPLETE)
71+
} else {
72+
// Other compilation errors
73+
return new ParseStatus(ParseCode.ERROR, e)
74+
}
75+
} catch (Exception e) {
76+
// Any other exception is considered an error
77+
return new ParseStatus(ParseCode.ERROR, e)
78+
}
79+
}
80+
81+
/**
82+
* Determine if the compilation errors indicate an incomplete script.
83+
*
84+
* @param e The compilation errors exception
85+
* @return true if the script is incomplete, false otherwise
86+
*/
87+
private boolean isIncompleteScript(MultipleCompilationErrorsException e) {
88+
def errorCollector = e.errorCollector
89+
90+
for (Message message : errorCollector.errors) {
91+
if (message instanceof SyntaxErrorMessage) {
92+
def syntaxException = message.cause
93+
def errorMessage = syntaxException.message
94+
95+
// Check for common indicators of incomplete scripts
96+
if (isUnexpectedEOF(errorMessage) ||
97+
isUnclosedBracket(errorMessage) ||
98+
isUnclosedString(errorMessage) ||
99+
isUnexpectedInput(errorMessage)) {
100+
return true
101+
}
102+
}
103+
}
104+
105+
return false
106+
}
107+
108+
/**
109+
* Check if the error message indicates an unexpected end of file.
110+
*/
111+
private boolean isUnexpectedEOF(String errorMessage) {
112+
errorMessage.contains('unexpected end of file') ||
113+
errorMessage.contains('Unexpected EOF')
114+
}
115+
116+
/**
117+
* Check if the error message indicates an unclosed bracket.
118+
*/
119+
private boolean isUnclosedBracket(String errorMessage) {
120+
errorMessage.contains("Missing '}'") ||
121+
errorMessage.contains("Missing ')'") ||
122+
errorMessage.contains("Missing ']'")
123+
}
124+
125+
/**
126+
* Check if the error message indicates an unclosed string.
127+
*/
128+
private boolean isUnclosedString(String errorMessage) {
129+
errorMessage.contains('String literal is not terminated') ||
130+
errorMessage.contains('Unterminated string literal')
131+
}
132+
133+
/**
134+
* Check if the error message indicates unexpected input that might suggest
135+
* an incomplete script rather than a syntax error.
136+
*/
137+
private boolean isUnexpectedInput(String errorMessage) {
138+
// Check for unexpected input but exclude cases where closing brackets are unexpected
139+
// as those typically indicate syntax errors rather than incomplete scripts
140+
(errorMessage.contains('Unexpected input: ') ||
141+
errorMessage.contains('Unexpected character: ')) &&
142+
!(errorMessage.contains("Unexpected input: '}'") ||
143+
errorMessage.contains("Unexpected input: ')'") ||
144+
errorMessage.contains("Unexpected input: ']'"))
145+
}
146+
}

gremlin-console/src/test/groovy/org/apache/tinkerpop/gremlin/console/GremlinGroovyshTest.groovy

Lines changed: 25 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -22,11 +22,14 @@ import org.apache.tinkerpop.gremlin.console.commands.RemoteCommand
2222
import org.apache.tinkerpop.gremlin.console.jsr223.AbstractGremlinServerIntegrationTest
2323
import org.apache.tinkerpop.gremlin.console.jsr223.DriverRemoteAcceptor
2424
import org.apache.tinkerpop.gremlin.console.jsr223.MockGroovyGremlinShellEnvironment
25+
import org.apache.tinkerpop.gremlin.jsr223.console.RemoteException
2526
import org.codehaus.groovy.tools.shell.IO
2627
import org.junit.Test
2728

2829
import java.nio.file.Paths
2930

31+
import static org.junit.Assert.fail;
32+
3033
class GremlinGroovyshTest extends AbstractGremlinServerIntegrationTest {
3134
private IO testio
3235
private ByteArrayOutputStream out
@@ -92,11 +95,32 @@ class GremlinGroovyshTest extends AbstractGremlinServerIntegrationTest {
9295
void shouldNotSubmitIncompleteLinesFromRemoteConsole() {
9396
setupRemote(shell)
9497
shell.execute(":remote console")
95-
shell.execute("if (0 == g.V().count()) {")
98+
shell.execute("if (0 == g.V().count().next()) {")
9699

97100
assert (0 != shell.buffers.current().size())
98101
}
99102

103+
/**
104+
* TINKERPOP-3040 - prior to 3.7.4, the console evaluated scripts locally before sending to the server which could
105+
* create a situation where there were classes needed locally for that eval to succeed for the script to be sent
106+
* and would therefore end in exception and not allow the send. the console shouldn't be evaluating scripts locally
107+
* to determine their validity. The console only wants to determine if they are complete for multi-line submission
108+
* on <enter>. This test simulates this situation by throwing an exception and asserting it is coming from the
109+
* server as a RemoteException. If it had executed locally we would have just gotten a DefaultTemporaryException.
110+
*/
111+
@Test
112+
void shouldNotEvalToDetermineIncompleteLinesToSubmitForRemoteConsole() {
113+
setupRemote(shell)
114+
shell.execute(":remote console")
115+
116+
try {
117+
shell.execute("throw new org.apache.tinkerpop.gremlin.server.util.DefaultTemporaryException('kaboom!!')")
118+
fail("Should have thrown a remote exception")
119+
} catch (RemoteException ex) {
120+
assert ("kaboom!!" == ex.message)
121+
}
122+
}
123+
100124
@Test
101125
void shouldGetGremlinResultFromRemoteConsoleInInterpreterMode() {
102126
setupRemote(shell)

0 commit comments

Comments
 (0)