[
https://issues.apache.org/jira/browse/TINKERPOP-3183?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=18144058#comment-18144058
]
ASF GitHub Bot commented on TINKERPOP-3183:
-------------------------------------------
kenhuuu commented on code in PR #3773:
URL: https://github.com/apache/tinkerpop/pull/3773#discussion_r4553519033
##########
gremlin-console/src/main/groovy/org/apache/tinkerpop/gremlin/console/StatementParser.groovy:
##########
@@ -0,0 +1,174 @@
+/*
+ * 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.codehaus.groovy.control.CompilationUnit
+import org.codehaus.groovy.control.CompilerConfiguration
+import org.codehaus.groovy.control.MultipleCompilationErrorsException
+import org.codehaus.groovy.control.Phases
+import org.jline.reader.EOFError
+import org.jline.reader.ParsedLine
+import org.jline.reader.Parser
+import org.jline.reader.impl.DefaultParser
+
+import java.util.function.Predicate
+
+/**
+ * Decides when typed, piped or scripted input is a complete statement. The
line reader's own parser only waits for
+ * more input while a bracket, quote or comment is open, so a line such as
{@code g.V().} or {@code x = 1 +} would run
+ * at once and fail. This parser also waits when the statement only fails to
compile because it ends too early.
+ */
+class StatementParser implements Parser {
+ private final Parser delegate
+ private final CompilerConfiguration config
+ private final Predicate<String> isCommand
+
+ /**
+ * @param delegate the line reader's parser
+ * @param config the compiler settings of the engine the statement runs in
+ * @param isCommand whether a name such as {@code /plugin} is a console
command
+ */
+ StatementParser(final Parser delegate, final CompilerConfiguration config,
final Predicate<String> isCommand) {
+ this.delegate = delegate
+ this.config = config
+ this.isCommand = isCommand
+ }
+
+ /**
+ * Creates a parser for script files, with the line reader settings
groovysh uses for typed input.
+ */
+ static StatementParser forScripts(final CompilerConfiguration config) {
+ final DefaultParser lineParser = new DefaultParser(regexCommand:
/\/?[a-zA-Z!]\S*/, eofOnUnclosedQuote: true,
+ eofOnEscapedNewLine: true)
+ lineParser.blockCommentDelims(new
DefaultParser.BlockCommentDelims("/*", "*/"))
+ .lineCommentDelims(["//"] as String[])
+ .setEofOnUnclosedBracket(DefaultParser.Bracket.CURLY,
DefaultParser.Bracket.ROUND, DefaultParser.Bracket.SQUARE)
+ // console commands in scripts are handled before a statement is parsed
+ return new StatementParser(lineParser, config, { false } as
Predicate<String>)
+ }
+
+ @Override
+ ParsedLine parse(final String line, final int cursor, final ParseContext
context) {
+ if (context != ParseContext.ACCEPT_LINE) return delegate.parse(line,
cursor, context)
+
+ final boolean command = isCommandLine(line)
+ try {
+ final ParsedLine parsed = delegate.parse(line, cursor, context)
+ if (!command && continuesOnNextLine(line)) throw new
StatementContinues()
+ return parsed
+ } catch (EOFError e) {
+ // more input cannot fix a stray closing bracket, and a command
with an unclosed quote is reported by
+ // the command, so both are accepted as they are
+ if (e instanceof StatementContinues || !(command ||
hasStrayClosingBracket(line))) throw e
+ return delegate.parse(line, cursor, ParseContext.SPLIT_LINE)
+ }
+ }
+
+ /**
+ * Whether the text is an unfinished statement that the next line
continues.
+ */
+ boolean isUnfinished(final String text) {
+ try {
+ parse(text, text.length(), ParseContext.ACCEPT_LINE)
+ return false
+ } catch (EOFError ignored) {
+ return true
+ }
+ }
+
+ private boolean isCommandLine(final String line) {
+ final String trimmed = line.trim()
+ if (!trimmed.startsWith(":") && !trimmed.startsWith("/")) return false
+ final String name = "/" + trimmed.substring(1).split(/\s+/, 2)[0]
+ return isCommand.test(name)
+ }
+
+ /**
+ * Whether the statement fails to parse only because it ends too early,
e.g. after a trailing {@code .} or an
+ * operator: it does not parse as it is but does once a name is added
after it. The position of Groovy's error
+ * cannot be used instead, as {@code repeat(out()).} is reported at the
call rather than at the end.
+ */
+ boolean continuesOnNextLine(final String statement) {
+ final String code = statement.stripTrailing()
+ // ":" starts no Groovy statement, so an unknown command is a mistake
rather than an unfinished line
+ if (code.isEmpty() || code.startsWith(":")) return false
+
+ return !parses(code) && parses(code + "\n" + NEXT_LINE)
+ }
+
+ /**
+ * Stands in for whatever the next line holds.
+ */
+ private static final String NEXT_LINE = "_"
+
+ private boolean parses(final String code) {
+ // parse only, so no class is generated or loaded
+ final CompilationUnit unit = new CompilationUnit(config)
+ unit.addSource("Script1.groovy", code)
+ try {
+ unit.compile(Phases.CONVERSION)
+ return true
+ } catch (MultipleCompilationErrorsException ignored) {
+ return false
+ }
+ }
+
+ /**
+ * Whether a closing bracket appears before any matching opening bracket,
which no further input can fix.
+ * Brackets inside strings count too.
+ */
+ static boolean hasStrayClosingBracket(final String statement) {
Review Comment:
This method likely has an issue. For example, `foo("))"` is missing the
call-closing ), but the quoted characters drive the raw counter negative,
causing the line to execute instead of waiting for continuation.
##########
gremlin-console/src/main/groovy/org/apache/tinkerpop/gremlin/console/commands/PluginCommand.groovy:
##########
Review Comment:
Can we add a failure-path test? If `activate()` throws, shell evaluation
isn't set back to `true`.
##########
gremlin-console/src/main/groovy/org/apache/tinkerpop/gremlin/console/Console.groovy:
##########
@@ -162,85 +144,118 @@ class Console {
if (activePlugins.size() != mediator.activePlugins().size())
mediator.writePluginState()
- try {
- // if the init script contains :x command it will throw an
ExitNotification so init script execution
- // needs to appear in the try/catch
- if (scriptsAndArgs != null && !scriptsAndArgs.isEmpty())
executeInShell(scriptsAndArgs)
-
- // start iterating results to show as output
- showShellEvaluationOutput(true)
-
- runner.run()
- } catch (ExitNotification ignored) {
- // occurs on exit
- } catch (Throwable t) {
- t.printStackTrace()
- } finally {
- // shutdown hook defined above will kill any open remotes
- System.exit(0)
- }
+ commands = GremlinCommands.create(mediator, engine)
+
+ if (scriptsAndArgs != null && !scriptsAndArgs.isEmpty())
executeInShell(scriptsAndArgs, commands)
+
+ showOutput = true
}
- def showShellEvaluationOutput(final boolean show) {
- if (show)
- groovy.setResultHook(handleResultIterate)
- else
- groovy.setResultHook(handleResultShowNothing)
+ /**
+ * Runs the shell on the terminal until the session ends and returns its
exit code.
+ */
+ int start(final Terminal terminal, final Path historyFile =
Paths.get(ConsoleFs.HISTORY_FILE)) {
+ final GroovyshOptions options = GroovyshOptions.builder()
+ .engine(engine)
+ .terminal(terminal)
+ .showBanner(false)
+ .prompt({ interactive ?
Colorizer.render(Preferences.inputPromptColor, Preferences.inputPrompt + ' ') :
"" } as Supplier<String>)
+ .historyFile(historyFile)
Review Comment:
Could we add a startup test using a path that can't be opened (like a
directory)? A bad history file should not prevent console use. It seems like we
don't have testing for this now that we use the JLine history.
##########
gremlin-console/src/test/groovy/org/apache/tinkerpop/gremlin/console/AbstractConsoleSessionTest.groovy:
##########
@@ -0,0 +1,109 @@
+/*
+ * 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.jline.jansi.Ansi
+import org.junit.After
+import org.junit.Before
+
+import java.nio.file.Files
+import java.nio.file.Path
+
+/**
+ * Runs each test against consoles started in this JVM, with its own home
directory and the preferences reset.
+ */
+abstract class AbstractConsoleSessionTest {
+
+ static final String MODERN_GRAPH = "g =
traversal().with(TinkerFactory.createModern())"
+ static final String MODERN_GRAPH_RESULT =
"==>graphtraversalsource[tinkermemorygraph[vertices:6 edges:6], standard]"
+
+ // in-memory in tests (see InMemoryPreferencesFactory)
+ protected static final java.util.prefs.Preferences STORE =
+
java.util.prefs.Preferences.userRoot().node("/org/apache/tinkerpop/gremlin/console")
+
+ private static boolean preferencesLoaded = false
+
+ protected Path home
+ private String oldHome
+ private boolean oldAnsi
+ private final List<ConsoleSession> sessions = []
+
+ @Before
+ void setUpSession() {
+ // loading Console turns colors on, so load it before they are
switched off below
+ Class.forName(Console.name)
+ // main() loads the preferences and installs their listeners before it
creates the console
+ synchronized (AbstractConsoleSessionTest) {
+ if (!preferencesLoaded) {
+ Preferences.expandoMagic()
+ preferencesLoaded = true
+ }
+ }
+ resetPreferences()
+ home = Files.createTempDirectory("console-session-test")
+ oldHome = System.getProperty("user.home")
+ System.setProperty("user.home", home.toString())
+ oldAnsi = Ansi.isEnabled()
+ // as with -C
+ Ansi.enabled = false
+ }
+
+ @After
+ void tearDownSession() {
+ sessions.each { if (it != null) try { it.close() } catch (Throwable
ignored) { } }
Review Comment:
Should this be `sessions.reverseEach { ... }` ?
##########
gremlin-console/src/main/groovy/org/apache/tinkerpop/gremlin/console/Console.groovy:
##########
@@ -467,6 +504,22 @@ class Console {
if (!interactive) System.exit(0)
}
+ /**
+ * Runs a console command from a script. Only the Gremlin commands and the
exit commands are available, as
+ * groovysh's own commands do not exist until the shell starts.
+ */
+ private static void executeCommand(final String line, final CommandGroup
commands) {
+ final List<String> words = new DefaultParser().parse(line,
line.length()).words().findAll { !it.isEmpty() }
Review Comment:
Can we get some more tests in `-i/-e` mode? In particular since
`DefaultParser` is used here we should have tests to ensure that quoted values,
spaces, escapes, and multiple commands work? `:plugin unuse` alone won't catch
issues introduced by DefaultParser.
##########
gremlin-console/src/main/groovy/org/apache/tinkerpop/gremlin/console/PluggedIn.groovy:
##########
@@ -54,21 +52,15 @@ class PluggedIn {
void activate() {
plugin.getCustomizers("gremlin-groovy").get().each {
if (it instanceof ImportCustomizer) {
- if (shell instanceof GremlinGroovysh) {
- org.codehaus.groovy.control.customizers.ImportCustomizer
ic = new org.codehaus.groovy.control.customizers.ImportCustomizer()
- ic.addStarImports(it.getClassPackages().collect() {
it.getName() }.toArray(new String[0]))
- ic.addStaticStars(it.getMethodClasses().collect() {
it.getCanonicalName() }.toArray(new String[0]))
- ic.addStaticStars(it.getEnumClasses().collect() {
it.getCanonicalName() }.toArray(new String[0]))
- ((GremlinGroovysh)
shell).getCompilerConfiguration().addCompilationCustomizers(ic)
- } else {
- it.getClassPackages().collect {Mediator.IMPORT_SPACE +
it.getName() + Mediator.IMPORT_WILDCARD }.each { shell.execute(it) }
- it.getMethodClasses().collect
{Mediator.IMPORT_STATIC_SPACE + it.getCanonicalName() +
Mediator.IMPORT_WILDCARD}.each {shell.execute(it)}
- it.getEnumClasses().collect {Mediator.IMPORT_STATIC_SPACE
+ it.getCanonicalName() + Mediator.IMPORT_WILDCARD}.each {shell.execute(it)}
- }
+ org.codehaus.groovy.control.customizers.ImportCustomizer ic =
new org.codehaus.groovy.control.customizers.ImportCustomizer()
+ ic.addStarImports(it.getClassPackages().collect() {
it.getName() }.toArray(new String[0]))
+ ic.addStaticStars(it.getMethodClasses().collect() {
it.getCanonicalName() }.toArray(new String[0]))
+ ic.addStaticStars(it.getEnumClasses().collect() {
it.getCanonicalName() }.toArray(new String[0]))
+ engine.getCompilerConfiguration().addCompilationCustomizers(ic)
} else if (it instanceof ScriptCustomizer) {
- it.getScripts().collect { it.join(LINE_SEPARATOR) }.each {
shell.execute(it) }
+ it.getScripts().collect { it.join(LINE_SEPARATOR) }.each {
engine.execute(it) }
} else if (it instanceof BindingsCustomizer) {
Review Comment:
It doesn't seem like there are any test for BindingsCustomizer. Could one be
added with a test-only plugin?
> Upgrade to Groovy 6
> -------------------
>
> Key: TINKERPOP-3183
> URL: https://issues.apache.org/jira/browse/TINKERPOP-3183
> Project: TinkerPop
> Issue Type: Improvement
> Components: groovy
> Affects Versions: 3.7.4
> Reporter: Stephen Mallette
> Priority: Major
>
> -Groovy 5 recently released. While Groovy will be removed from Gremlin Server
> in 4.x it will still exist for Gremlin Console (and perhaps other use cases).
> Note that for Gremlin Console, there were supposedly significant changes to
> {{groovysh}} for Groovy 5 because of the upgrade to JLine. It likely will
> impact Gremlin Console and make the upgrade more complicated.-
> Groovy 6 has now released, which has walked back some of the changes which
> had complicated the gremlin-console groovy 5 migration. We should aim to jump
> straight to groovy 6.
--
This message was sent by Atlassian Jira
(v10.3.26#10030026)
