From d962d33a2a2e5ab070c3fb8dc0465639130cd6d9 Mon Sep 17 00:00:00 2001 From: Nik Everett Date: Tue, 27 Feb 2018 09:24:16 -0800 Subject: [PATCH] SQL: Replace the cli fixture with in-process testing (elastic/x-pack-elasticsearch#3889) I'm really really sad to be removing the cli-fixture but I've had trouble with it leaking recently it is pretty slow. Beyond that, we'd prefer that our test fixture only fixture things that are external depndencies. So, yeah, I'm removing it. So we get faster tests and no chance of leaking processes. We lose some "realness" in the tests. Instead of interacting with the CLI like a real user we embed it in the test process. That means we don't test the forking, we don't test the executable jar, and we don't test the jLine console detection stuff. On the other hand we were kind of forcing the jLine console detection stuff in a funky way with the fixture anyway. And we test the executable jar in the packaging tests. And that'll have to do. I haven't renamed `RemoteCli` because it'd bloat this commit with mechanical changes that'd make it hard to review. I'll rename it in a followup commit. This also updates jLine so we can disable blinking to matching parentheses during testing. I have no clue why, but this wasn't happening when we used the fixture. The trouble with the blinking is that it is based on *time* so it slows things down. Worse, it works inconsistently! Sometimes it spits out sensible ascii codes and sometimes it, well, spits out weird garbage. When you use it in person it works fine though. So we keep it on when not testing. Cleans up some redundancy in when testing CLI errors. Less copy and paste good. I was tempted to disable the xterm emulation entirely while working on this because upgrading jLine changed a few things and it was a real pain to update. But If we turned that off then we'd have *nothing* testing the colors and such. That'd be a shame because we use color in the output to commicate stuff. I like it so I don't want to break it. While I was there, I replaces the cli connector's `PrintWriter` with a `BufferedWriter`. The `PrintWriter` was kind of a trap because `println` would fail to work properly on windows because we force the terminal into xterm mode and it doesn't know what to do with windows line endings. Windows..... Additionally I fixed a race condition between disabling echo when reading passwords and fast writers. We were disabling the echo shortly after sending the prompt. A fast enough writer could send us text before the echo disable kicked in. Now I delegate to `LineReader#readLine` with a special echo mask that disables echo. This is both easier to test and doesn't seem to have the race condition. This race condition was failing the tests because they are so much faster now. Yay! Original commit: elastic/x-pack-elasticsearch@d0ec0273964630a3a113ab27009fdff6a182ecb2 --- plugin/sql/sql-cli/build.gradle | 29 +- .../sql/sql-cli/licenses/jline-3.3.1.jar.sha1 | 1 - .../sql/sql-cli/licenses/jline-3.6.0.jar.sha1 | 1 + .../org/elasticsearch/xpack/sql/cli/Cli.java | 47 +-- .../xpack/sql/cli/ConnectionBuilder.java | 8 + .../xpack/sql/cli/JLineTerminal.java | 60 ++-- .../xpack/sql/cli/JLineTerminalTests.java | 54 ++++ qa/sql/build.gradle | 105 +++---- .../xpack/qa/sql/security/CliSecurityIT.java | 12 +- .../qa/sql/cli/CliIntegrationTestCase.java | 5 + .../xpack/qa/sql/cli/ErrorsTestCase.java | 55 ++-- .../xpack/qa/sql/cli/FetchSizeTestCase.java | 14 +- .../xpack/qa/sql/cli/RemoteCli.java | 294 ++++++++++++------ test/sql-cli-fixture/build.gradle | 14 - .../xpack/sql/cli/fixture/CliFixture.java | 154 --------- .../sql/cli/fixture/SuppressForbidden.java | 17 - 16 files changed, 454 insertions(+), 416 deletions(-) delete mode 100644 plugin/sql/sql-cli/licenses/jline-3.3.1.jar.sha1 create mode 100644 plugin/sql/sql-cli/licenses/jline-3.6.0.jar.sha1 create mode 100644 plugin/sql/sql-cli/src/test/java/org/elasticsearch/xpack/sql/cli/JLineTerminalTests.java delete mode 100644 test/sql-cli-fixture/build.gradle delete mode 100644 test/sql-cli-fixture/src/main/java/org/elasticsearch/xpack/sql/cli/fixture/CliFixture.java delete mode 100644 test/sql-cli-fixture/src/main/java/org/elasticsearch/xpack/sql/cli/fixture/SuppressForbidden.java diff --git a/plugin/sql/sql-cli/build.gradle b/plugin/sql/sql-cli/build.gradle index 700ba34264b..73782e25cd8 100644 --- a/plugin/sql/sql-cli/build.gradle +++ b/plugin/sql/sql-cli/build.gradle @@ -13,7 +13,7 @@ apply plugin: 'elasticsearch.build' description = 'Command line interface to Elasticsearch that speaks SQL' dependencies { - compile "org.jline:jline:3.3.1" + compile "org.jline:jline:3.6.0" compile xpackProject('plugin:sql:sql-shared-client') compile xpackProject('plugin:sql:sql-proto') compile "org.elasticsearch:elasticsearch-cli:${version}" @@ -38,19 +38,38 @@ dependencyLicenses { ignoreSha 'sql-shared-client' } +/* + * Bundle all dependencies into the main jar and mark it as executable it + * can be easilly shipped around and used. + */ jar { - // Bundle all dependencies into the jar. from { configurations.compile.collect { it.isDirectory() ? it : zipTree(it) } configurations.runtime.collect { it.isDirectory() ? it : zipTree(it) } } - // Make the jar "executable" with `java -jar` manifest { attributes 'Main-Class': 'org.elasticsearch.xpack.sql.cli.Cli' } } +/* + * Build a jar that doesn't include the dependencies bundled that we can + * include with QA tests along side Elasticsearch without breaking + * jarhell. + */ +configurations { + nodeps +} +task nodepsJar(type: Jar) { + appendix 'nodeps' + from sourceSets.main.output +} +artifacts { + nodeps nodepsJar +} + thirdPartyAudit.excludes = [ + // jLine's optional dependencies 'org.apache.sshd.client.SshClient', 'org.apache.sshd.client.auth.keyboard.UserInteraction', 'org.apache.sshd.client.channel.ChannelShell', @@ -75,7 +94,9 @@ thirdPartyAudit.excludes = [ 'org.apache.sshd.server.scp.ScpCommandFactory$Builder', 'org.apache.sshd.server.session.ServerSession', 'org.apache.sshd.server.subsystem.sftp.SftpSubsystemFactory$Builder', - 'org.mozilla.universalchardet.UniversalDetector' + 'org.mozilla.universalchardet.UniversalDetector', + 'org.fusesource.jansi.internal.Kernel32$FOCUS_EVENT_RECORD', + 'org.fusesource.jansi.internal.Kernel32$MOUSE_EVENT_RECORD', ] task runcli { diff --git a/plugin/sql/sql-cli/licenses/jline-3.3.1.jar.sha1 b/plugin/sql/sql-cli/licenses/jline-3.3.1.jar.sha1 deleted file mode 100644 index 85661fb7fda..00000000000 --- a/plugin/sql/sql-cli/licenses/jline-3.3.1.jar.sha1 +++ /dev/null @@ -1 +0,0 @@ -d8a30137fe4ee2246b71b3915baac767d348c5bb \ No newline at end of file diff --git a/plugin/sql/sql-cli/licenses/jline-3.6.0.jar.sha1 b/plugin/sql/sql-cli/licenses/jline-3.6.0.jar.sha1 new file mode 100644 index 00000000000..d6938e6d80b --- /dev/null +++ b/plugin/sql/sql-cli/licenses/jline-3.6.0.jar.sha1 @@ -0,0 +1 @@ +c8ecc302d6b7d19da41c66be7d428c17cd6b12b2 \ No newline at end of file diff --git a/plugin/sql/sql-cli/src/main/java/org/elasticsearch/xpack/sql/cli/Cli.java b/plugin/sql/sql-cli/src/main/java/org/elasticsearch/xpack/sql/cli/Cli.java index d88eae26bec..d1f59c97e5a 100644 --- a/plugin/sql/sql-cli/src/main/java/org/elasticsearch/xpack/sql/cli/Cli.java +++ b/plugin/sql/sql-cli/src/main/java/org/elasticsearch/xpack/sql/cli/Cli.java @@ -24,7 +24,7 @@ import org.elasticsearch.xpack.sql.client.HttpClient; import org.elasticsearch.xpack.sql.client.shared.ClientException; import org.elasticsearch.xpack.sql.client.shared.ConnectionConfiguration; import org.elasticsearch.xpack.sql.client.shared.Version; - +import org.jline.terminal.TerminalBuilder; import java.io.IOException; import java.net.ConnectException; import java.util.Arrays; @@ -36,22 +36,6 @@ public class Cli extends LoggingAwareCommand { private final OptionSpec checkOption; private final OptionSpec connectionString; - private Cli() { - super("Elasticsearch SQL CLI"); - parser.acceptsAll(Arrays.asList("d", "debug"), "Enable debug logging"); - this.keystoreLocation = parser.acceptsAll( - Arrays.asList("k", "keystore_location"), - "Location of a keystore to use when setting up SSL. " - + "If specified then the CLI will prompt for a keystore password. " - + "If specified when the uri isn't https then an error is thrown.") - .withRequiredArg().ofType(String.class); - this.checkOption = parser.acceptsAll(Arrays.asList("c", "check"), - "Enable initial connection check on startup") - .withRequiredArg().ofType(Boolean.class) - .defaultsTo(Boolean.parseBoolean(System.getProperty("cli.check", "true"))); - this.connectionString = parser.nonOptions("uri"); - } - /** * Use this VM Options to run in IntelliJ or Eclipse: * -Dorg.jline.terminal.type=xterm-256color @@ -61,7 +45,7 @@ public class Cli extends LoggingAwareCommand { * -Dorg.jline.terminal.dumb=true */ public static void main(String[] args) throws Exception { - final Cli cli = new Cli(); + final Cli cli = new Cli(new JLineTerminal(TerminalBuilder.builder().build(), true)); configureJLineLogging(); int status = cli.main(args, Terminal.DEFAULT); if (status != ExitCodes.OK) { @@ -79,6 +63,28 @@ public class Cli extends LoggingAwareCommand { } } + private final CliTerminal cliTerminal; + + /** + * Build the CLI. + */ + public Cli(CliTerminal cliTerminal) { + super("Elasticsearch SQL CLI"); + this.cliTerminal = cliTerminal; + parser.acceptsAll(Arrays.asList("d", "debug"), "Enable debug logging"); + this.keystoreLocation = parser.acceptsAll( + Arrays.asList("k", "keystore_location"), + "Location of a keystore to use when setting up SSL. " + + "If specified then the CLI will prompt for a keystore password. " + + "If specified when the uri isn't https then an error is thrown.") + .withRequiredArg().ofType(String.class); + this.checkOption = parser.acceptsAll(Arrays.asList("c", "check"), + "Enable initial connection check on startup") + .withRequiredArg().ofType(Boolean.class) + .defaultsTo(Boolean.parseBoolean(System.getProperty("cli.check", "true"))); + this.connectionString = parser.nonOptions("uri"); + } + @Override protected void execute(org.elasticsearch.cli.Terminal terminal, OptionSet options) throws Exception { boolean debug = options.has("d") || options.has("debug"); @@ -105,7 +111,7 @@ public class Cli extends LoggingAwareCommand { new ServerInfoCliCommand(), new ServerQueryCliCommand() ); - try (CliTerminal cliTerminal = new JLineTerminal()) { + try { ConnectionBuilder connectionBuilder = new ConnectionBuilder(cliTerminal); ConnectionConfiguration con = connectionBuilder.buildConnection(uri, keystoreLocation); CliSession cliSession = new CliSession(new HttpClient(con)); @@ -114,6 +120,8 @@ public class Cli extends LoggingAwareCommand { checkConnection(cliSession, cliTerminal, con); } new CliRepl(cliTerminal, cliSession, cliCommand).execute(); + } finally { + cliTerminal.close(); } } @@ -138,6 +146,5 @@ public class Cli extends LoggingAwareCommand { ". This version of CLI only works with Elasticsearch version " + Version.CURRENT.toString()); } } - } } diff --git a/plugin/sql/sql-cli/src/main/java/org/elasticsearch/xpack/sql/cli/ConnectionBuilder.java b/plugin/sql/sql-cli/src/main/java/org/elasticsearch/xpack/sql/cli/ConnectionBuilder.java index d3a1add7884..2e9decf9fbf 100644 --- a/plugin/sql/sql-cli/src/main/java/org/elasticsearch/xpack/sql/cli/ConnectionBuilder.java +++ b/plugin/sql/sql-cli/src/main/java/org/elasticsearch/xpack/sql/cli/ConnectionBuilder.java @@ -68,6 +68,10 @@ public class ConnectionBuilder { Path p = Paths.get(keystoreLocation); checkIfExists("keystore file", p); String keystorePassword = cliTerminal.readPassword("keystore password: "); + if (keystorePassword == null) { + throw new FatalCliException("readPassword shouldn't ever return null but [" + + cliTerminal + "#readPassword] did!"); + } /* * Set both the keystore and truststore settings which is required @@ -87,6 +91,10 @@ public class ConnectionBuilder { if (user != null) { if (password == null) { password = cliTerminal.readPassword("password: "); + if (password == null) { + throw new FatalCliException("readPassword shouldn't ever return null but [" + + cliTerminal + "#readPassword] did!"); + } } properties.setProperty(ConnectionConfiguration.AUTH_USER, user); properties.setProperty(ConnectionConfiguration.AUTH_PASS, password); diff --git a/plugin/sql/sql-cli/src/main/java/org/elasticsearch/xpack/sql/cli/JLineTerminal.java b/plugin/sql/sql-cli/src/main/java/org/elasticsearch/xpack/sql/cli/JLineTerminal.java index dc8fa73b0b9..2cdebce4e53 100644 --- a/plugin/sql/sql-cli/src/main/java/org/elasticsearch/xpack/sql/cli/JLineTerminal.java +++ b/plugin/sql/sql-cli/src/main/java/org/elasticsearch/xpack/sql/cli/JLineTerminal.java @@ -10,12 +10,10 @@ import org.jline.reader.LineReader; import org.jline.reader.LineReaderBuilder; import org.jline.reader.UserInterruptException; import org.jline.terminal.Terminal; -import org.jline.terminal.TerminalBuilder; import org.jline.utils.AttributedString; import org.jline.utils.AttributedStringBuilder; import org.jline.utils.InfoCmp; -import java.io.BufferedReader; import java.io.IOException; import static org.jline.utils.AttributedStyle.BOLD; @@ -32,15 +30,30 @@ public class JLineTerminal implements CliTerminal { private Terminal terminal; private LineReader reader; - protected JLineTerminal() { - try { - this.terminal = TerminalBuilder.builder().build(); - reader = LineReaderBuilder.builder() - .terminal(terminal) - .completer(Completers.INSTANCE) - .build(); - } catch (IOException ex) { - throw new FatalCliException("Cannot use terminal", ex); + /** + * Build the terminal. + * @param terminal the jLine terminal to work with + * @param enableMatchBracket should jLine bounce the cursor to matching brackets? + * this is disabled in tests because it very difficult to predict and + * enabled in production because it is fairly nice. + */ + public JLineTerminal(Terminal terminal, boolean enableMatchBracket) { + this(terminal, + LineReaderBuilder.builder() + .terminal(terminal) + .completer(Completers.INSTANCE) + .build(), + enableMatchBracket); + } + + /** + * Constructor for tests. + */ + JLineTerminal(Terminal terminal, LineReader reader, boolean enableMatchBracket) { + this.terminal = terminal; + this.reader = reader; + if (false == enableMatchBracket) { + reader.setVariable(LineReader.BLINK_MATCHING_PAREN, 0L); } } @@ -91,23 +104,26 @@ public class JLineTerminal implements CliTerminal { @Override public String readPassword(String prompt) { - terminal.writer().print(prompt); - terminal.writer().flush(); - terminal.echo(false); - try { - return new BufferedReader(terminal.reader()).readLine(); - } catch (IOException ex) { - throw new FatalCliException("Error reading password", ex); - } finally { - terminal.echo(true); + String line = readLine(prompt, (char) 0); + if (line == null) { + throw new FatalCliException("Error reading password, terminal is closed"); } + return line; } @Override public String readLine(String prompt) { + String attributedString = new AttributedString(prompt, DEFAULT.foreground(YELLOW)).toAnsi(terminal); + return readLine(attributedString, null); + } + + private String readLine(String prompt, Character mask) { try { - String attributedString = new AttributedString(prompt, DEFAULT.foreground(YELLOW)).toAnsi(terminal); - return reader.readLine(attributedString); + String line = reader.readLine(prompt, null, mask, null); + if (line == null) { + throw new FatalCliException("Error reading password, terminal is closed"); + } + return line; } catch (UserInterruptException ex) { return ""; } catch (EndOfFileException ex) { diff --git a/plugin/sql/sql-cli/src/test/java/org/elasticsearch/xpack/sql/cli/JLineTerminalTests.java b/plugin/sql/sql-cli/src/test/java/org/elasticsearch/xpack/sql/cli/JLineTerminalTests.java new file mode 100644 index 00000000000..32a102901c9 --- /dev/null +++ b/plugin/sql/sql-cli/src/test/java/org/elasticsearch/xpack/sql/cli/JLineTerminalTests.java @@ -0,0 +1,54 @@ +/* + * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one + * or more contributor license agreements. Licensed under the Elastic License; + * you may not use this file except in compliance with the Elastic License. + */ +package org.elasticsearch.xpack.sql.cli; + +import org.elasticsearch.test.ESTestCase; +import org.jline.reader.LineReader; +import org.jline.terminal.Terminal; +import java.io.IOException; + +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.when; +import static org.mockito.Mockito.verify; + +public class JLineTerminalTests extends ESTestCase { + public void testReadPasswordSuccess() throws IOException { + Terminal wrapped = mock(Terminal.class); + LineReader reader = mock(LineReader.class); + + try (JLineTerminal terminal = new JLineTerminal(wrapped, reader, randomBoolean())) { + String prompt = randomAlphaOfLength(5); + + String expected = randomAlphaOfLength(5); + when(reader.readLine(prompt, null, (char) 0, null)).thenReturn(expected); + String actual = terminal.readPassword(prompt); + + assertEquals(expected, actual); + verify(reader).readLine(prompt, null, (char) 0, null); + } + } + + public void testReadPasswordEof() throws IOException { + Terminal wrapped = mock(Terminal.class); + LineReader reader = mock(LineReader.class); + + try (JLineTerminal terminal = new JLineTerminal(wrapped, reader, randomBoolean())) { + String prompt = randomAlphaOfLength(5); + + Exception e = expectThrows(FatalCliException.class, () -> terminal.readPassword(prompt)); + assertEquals("Error reading password, terminal is closed", e.getMessage()); + + verify(reader).readLine(prompt, null, (char) 0, null); + } + } + + public void testDisableMatchBracket() throws IOException { + Terminal wrapped = mock(Terminal.class); + LineReader reader = mock(LineReader.class); + new JLineTerminal(wrapped, reader, false).close(); + verify(reader).setVariable(LineReader.BLINK_MATCHING_PAREN, 0L); + } +} diff --git a/qa/sql/build.gradle b/qa/sql/build.gradle index 60a2d1aa2c4..9f2db43f433 100644 --- a/qa/sql/build.gradle +++ b/qa/sql/build.gradle @@ -8,17 +8,13 @@ archivesBaseName = 'qa-sql' dependencies { compile "org.elasticsearch.test:framework:${version}" -// TODO: Restore shading when https://github.com/elastic/elasticsearch/pull/27955 gets in - testCompile "org.elasticsearch.plugin:x-pack-core:${version}" + // JDBC testing dependencies + // TODO: Restore shading when https://github.com/elastic/elasticsearch/issues/28504 is fixed compile xpackProject('plugin:sql:jdbc') compile "net.sourceforge.csvjdbc:csvjdbc:1.0.34" - runtime "com.h2database:h2:1.4.194" - // used for running debug tests - runtime 'org.antlr:antlr4-runtime:4.5.3' - /* There are *no* CLI testing dependencies because we - * communicate with a fixture to fork a new CLI process - * when we need it. */ + compile project(path: xpackModule('sql:sql-cli'), configuration: 'nodeps') + compile "org.jline:jline:3.6.0" } /* disable unit tests because these are all integration tests used @@ -34,30 +30,50 @@ forbiddenApisMain { } thirdPartyAudit.excludes = [ - // H2 dependencies that we don't actually use.... - 'javax.servlet.ServletConfig', - 'javax.servlet.ServletContext', - 'javax.servlet.ServletContextEvent', - 'javax.servlet.ServletContextListener', - 'javax.servlet.ServletOutputStream', - 'javax.servlet.http.HttpServlet', - 'javax.servlet.http.HttpServletRequest', - 'javax.servlet.http.HttpServletResponse', - 'org.apache.lucene.document.Field$Index', - 'org.apache.lucene.queryParser.QueryParser', - 'org.osgi.framework.BundleActivator', - 'org.osgi.framework.BundleContext', - 'org.osgi.service.jdbc.DataSourceFactory', - 'org.slf4j.Logger', - 'org.slf4j.LoggerFactory', - ] + // jLine's optional dependencies + 'org.apache.sshd.client.SshClient', + 'org.apache.sshd.client.auth.keyboard.UserInteraction', + 'org.apache.sshd.client.channel.ChannelShell', + 'org.apache.sshd.client.channel.ClientChannel', + 'org.apache.sshd.client.channel.ClientChannelEvent', + 'org.apache.sshd.client.future.AuthFuture', + 'org.apache.sshd.client.future.ConnectFuture', + 'org.apache.sshd.client.future.OpenFuture', + 'org.apache.sshd.client.session.ClientSession', + 'org.apache.sshd.common.Factory', + 'org.apache.sshd.common.channel.PtyMode', + 'org.apache.sshd.common.config.keys.FilePasswordProvider', + 'org.apache.sshd.common.util.io.NoCloseInputStream', + 'org.apache.sshd.common.util.io.NoCloseOutputStream', + 'org.apache.sshd.server.Command', + 'org.apache.sshd.server.Environment', + 'org.apache.sshd.server.ExitCallback', + 'org.apache.sshd.server.SessionAware', + 'org.apache.sshd.server.Signal', + 'org.apache.sshd.server.SshServer', + 'org.apache.sshd.server.keyprovider.SimpleGeneratorHostKeyProvider', + 'org.apache.sshd.server.scp.ScpCommandFactory$Builder', + 'org.apache.sshd.server.session.ServerSession', + 'org.apache.sshd.server.subsystem.sftp.SftpSubsystemFactory$Builder', + 'org.fusesource.jansi.Ansi', + 'org.fusesource.jansi.internal.CLibrary$Termios', + 'org.fusesource.jansi.internal.CLibrary$WinSize', + 'org.fusesource.jansi.internal.CLibrary', + 'org.fusesource.jansi.internal.Kernel32$CHAR_INFO', + 'org.fusesource.jansi.internal.Kernel32$CONSOLE_SCREEN_BUFFER_INFO', + 'org.fusesource.jansi.internal.Kernel32$COORD', + 'org.fusesource.jansi.internal.Kernel32$FOCUS_EVENT_RECORD', + 'org.fusesource.jansi.internal.Kernel32$INPUT_RECORD', + 'org.fusesource.jansi.internal.Kernel32$KEY_EVENT_RECORD', + 'org.fusesource.jansi.internal.Kernel32$MOUSE_EVENT_RECORD', + 'org.fusesource.jansi.internal.Kernel32$SMALL_RECT', + 'org.fusesource.jansi.internal.Kernel32', + 'org.fusesource.jansi.internal.WindowsSupport', + 'org.mozilla.universalchardet.UniversalDetector', +] subprojects { apply plugin: 'elasticsearch.standalone-rest-test' - configurations { - cliFixture - cliJar - } dependencies { /* Since we're a standalone rest test we actually get transitive * dependencies but we don't really want them because they cause @@ -69,51 +85,34 @@ subprojects { testCompile "org.elasticsearch.test:framework:${version}" // JDBC testing dependencies - testRuntime(xpackProject('plugin:sql:jdbc')) - // TODO: Restore shading when https://github.com/elastic/elasticsearch/pull/27955 gets in + // TODO: Restore shading when https://github.com/elastic/elasticsearch/issues/28504 is fixed + testRuntime xpackProject('plugin:sql:jdbc') testRuntime("net.sourceforge.csvjdbc:csvjdbc:1.0.34") { transitive = false } testRuntime("com.h2database:h2:1.4.194") { transitive = false } + + // TODO check if needed testRuntime("org.antlr:antlr4-runtime:4.5.3") { transitive = false } - cliFixture xpackProject('test:sql-cli-fixture') - cliJar(xpackProject('plugin:sql:sql-cli')) { - // We only need the jar file because it bundles all of its dependencies - transitive = false - } + testRuntime project(path: xpackModule('sql:sql-cli'), configuration: 'nodeps') + testRuntime "org.jline:jline:3.6.0" } if (project.name != 'security') { // The security project just configures its subprojects apply plugin: 'elasticsearch.rest-test' - task cliFixture(type: org.elasticsearch.gradle.test.AntFixture) { - Project cli = xpackProject('plugin:sql:sql-cli') - dependsOn project.configurations.cliFixture - dependsOn project.configurations.cliJar - executable = new File(project.runtimeJavaHome, 'bin/java') - env 'CLASSPATH', "${ -> project.configurations.cliFixture.asPath }" - args 'org.elasticsearch.xpack.sql.cli.fixture.CliFixture', - baseDir, "${ -> project.configurations.cliJar.singleFile}" - } - integTestCluster { plugin xpackProject('plugin').path setting 'xpack.monitoring.enabled', 'false' setting 'xpack.ml.enabled', 'false' setting 'xpack.watcher.enabled', 'false' setting 'script.max_compilations_rate', '1000/1m' - dependsOn cliFixture - } - - integTestRunner { - systemProperty 'tests.cli.fixture', "${ -> cliFixture.addressAndPort }" - finalizedBy cliFixture.stopTask } task runqa(type: RunTask) { @@ -122,8 +121,6 @@ subprojects { setting 'xpack.ml.enabled', 'false' setting 'xpack.watcher.enabled', 'false' setting 'script.max_compilations_rate', '1000/1m' - dependsOn cliFixture } - runqa.finalizedBy cliFixture.stopTask } } diff --git a/qa/sql/security/src/test/java/org/elasticsearch/xpack/qa/sql/security/CliSecurityIT.java b/qa/sql/security/src/test/java/org/elasticsearch/xpack/qa/sql/security/CliSecurityIT.java index aee0a473247..84d3107384c 100644 --- a/qa/sql/security/src/test/java/org/elasticsearch/xpack/qa/sql/security/CliSecurityIT.java +++ b/qa/sql/security/src/test/java/org/elasticsearch/xpack/qa/sql/security/CliSecurityIT.java @@ -7,6 +7,7 @@ package org.elasticsearch.xpack.qa.sql.security; import org.elasticsearch.common.CheckedConsumer; import org.elasticsearch.common.io.PathUtils; +import org.elasticsearch.xpack.qa.sql.cli.ErrorsTestCase; import org.elasticsearch.xpack.qa.sql.cli.RemoteCli; import org.elasticsearch.xpack.qa.sql.cli.RemoteCli.SecurityConfig; import java.io.IOException; @@ -82,8 +83,8 @@ public class CliSecurityIT extends SqlSecurityTestCase { @Override public void expectScrollMatchesAdmin(String adminSql, String user, String userSql) throws Exception { expectMatchesAdmin(adminSql, user, userSql, cli -> { - assertEquals("fetch size set to [90m1[0m", cli.command("fetch size = 1")); - assertEquals("fetch separator set to \"[90m -- fetch sep -- [0m\"", + assertEquals("[?1l>[?1000l[?2004lfetch size set to [90m1[0m", cli.command("fetch size = 1")); + assertEquals("[?1l>[?1000l[?2004lfetch separator set to \"[90m -- fetch sep -- [0m\"", cli.command("fetch separator = \" -- fetch sep -- \"")); }); } @@ -157,7 +158,7 @@ public class CliSecurityIT extends SqlSecurityTestCase { @Override public void expectUnknownIndex(String user, String sql) throws Exception { try (RemoteCli cli = new RemoteCli(elasticsearchAddress(), true, userSecurity(user))) { - assertThat(cli.command(sql), containsString("Bad request")); + ErrorsTestCase.assertFoundOneProblem(cli.command(sql)); assertThat(cli.readLine(), containsString("Unknown index")); } } @@ -176,13 +177,14 @@ public class CliSecurityIT extends SqlSecurityTestCase { @Override public void expectUnknownColumn(String user, String sql, String column) throws Exception { try (RemoteCli cli = new RemoteCli(elasticsearchAddress(), true, userSecurity(user))) { - assertThat(cli.command(sql), containsString("[1;31mBad request")); - assertThat(cli.readLine(), containsString("Unknown column [" + column + "][1;23;31m][0m")); + ErrorsTestCase.assertFoundOneProblem(cli.command(sql)); + assertThat(cli.readLine(), containsString("Unknown column [" + column + "]" + ErrorsTestCase.END)); } } @Override public void checkNoMonitorMain(String user) throws Exception { + // Building the cli will attempt the connection and run the assertion @SuppressWarnings("resource") // forceClose will close it RemoteCli cli = new RemoteCli(elasticsearchAddress(), true, userSecurity(user)) { @Override diff --git a/qa/sql/src/main/java/org/elasticsearch/xpack/qa/sql/cli/CliIntegrationTestCase.java b/qa/sql/src/main/java/org/elasticsearch/xpack/qa/sql/cli/CliIntegrationTestCase.java index 09077d63859..88407026f6c 100644 --- a/qa/sql/src/main/java/org/elasticsearch/xpack/qa/sql/cli/CliIntegrationTestCase.java +++ b/qa/sql/src/main/java/org/elasticsearch/xpack/qa/sql/cli/CliIntegrationTestCase.java @@ -70,6 +70,11 @@ public abstract class CliIntegrationTestCase extends ESRestTestCase { return cli.command(command); } + /** + * Read a line produced by the CLI. + * Note that these lines will contain {@code xterm-256color} + * escape sequences. + */ public String readLine() throws IOException { return cli.readLine(); } diff --git a/qa/sql/src/main/java/org/elasticsearch/xpack/qa/sql/cli/ErrorsTestCase.java b/qa/sql/src/main/java/org/elasticsearch/xpack/qa/sql/cli/ErrorsTestCase.java index bc7d275eab1..9a5d5b9c3ea 100644 --- a/qa/sql/src/main/java/org/elasticsearch/xpack/qa/sql/cli/ErrorsTestCase.java +++ b/qa/sql/src/main/java/org/elasticsearch/xpack/qa/sql/cli/ErrorsTestCase.java @@ -17,16 +17,25 @@ import static org.hamcrest.Matchers.startsWith; * Tests for error messages. */ public abstract class ErrorsTestCase extends CliIntegrationTestCase implements org.elasticsearch.xpack.qa.sql.ErrorsTestCase { + /** + * Starting sequence commons to lots of errors. + */ + public static final String START = "[?1l>[?1000l[?2004l[31;1m"; + /** + * Ending sequence common to lots of errors. + */ + public static final String END = "[23;31;1m][0m"; + @Override public void testSelectInvalidSql() throws Exception { - assertEquals("[1;31mBad request [[22;3;33mFound 1 problem(s)", command("SELECT * FRO")); - assertEquals("line 1:8: Cannot determine columns for *[1;23;31m][0m", readLine()); + assertFoundOneProblem(command("SELECT * FRO")); + assertEquals("line 1:8: Cannot determine columns for *" + END, readLine()); } @Override public void testSelectFromMissingIndex() throws IOException { - assertEquals("[1;31mBad request [[22;3;33mFound 1 problem(s)", command("SELECT * FROM test")); - assertEquals("line 1:15: Unknown index [test][1;23;31m][0m", readLine()); + assertFoundOneProblem(command("SELECT * FROM test")); + assertEquals("line 1:15: Unknown index [test]" + END, readLine()); } @Override @@ -34,59 +43,61 @@ public abstract class ErrorsTestCase extends CliIntegrationTestCase implements o // Create an index without any types client().performRequest("PUT", "/test", emptyMap(), new StringEntity("{}", ContentType.APPLICATION_JSON)); - assertEquals("[1;31mBad request [[22;3;33mFound 1 problem(s)", command("SELECT * FROM test")); - assertEquals("line 1:15: [test] doesn't have any types so it is incompatible with sql[1;23;31m][0m", readLine()); + assertFoundOneProblem(command("SELECT * FROM test")); + assertEquals("line 1:15: [test] doesn't have any types so it is incompatible with sql" + END, readLine()); } @Override public void testSelectMissingField() throws IOException { index("test", body -> body.field("test", "test")); - assertEquals("[1;31mBad request [[22;3;33mFound 1 problem(s)", command("SELECT missing FROM test")); - assertEquals("line 1:8: Unknown column [missing][1;23;31m][0m", readLine()); + assertFoundOneProblem(command("SELECT missing FROM test")); + assertEquals("line 1:8: Unknown column [missing]" + END, readLine()); } @Override public void testSelectMissingFunction() throws Exception { index("test", body -> body.field("foo", 1)); - assertEquals("[1;31mBad request [[22;3;33mFound 1 problem(s)", command("SELECT missing(foo) FROM test")); - assertEquals("line 1:8: Unknown function [missing][1;23;31m][0m", readLine()); + assertFoundOneProblem(command("SELECT missing(foo) FROM test")); + assertEquals("line 1:8: Unknown function [missing]" + END, readLine()); } @Override public void testSelectProjectScoreInAggContext() throws Exception { index("test", body -> body.field("foo", 1)); - assertEquals("[1;31mBad request [[22;3;33mFound 1 problem(s)", command("SELECT foo, SCORE(), COUNT(*) FROM test GROUP BY foo")); - assertEquals("line 1:13: Cannot use non-grouped column [SCORE()], expected [foo][1;23;31m][0m", readLine()); + assertFoundOneProblem(command("SELECT foo, SCORE(), COUNT(*) FROM test GROUP BY foo")); + assertEquals("line 1:13: Cannot use non-grouped column [SCORE()], expected [foo]" + END, readLine()); } @Override public void testSelectOrderByScoreInAggContext() throws Exception { index("test", body -> body.field("foo", 1)); - assertEquals("[1;31mBad request [[22;3;33mFound 1 problem(s)", - command("SELECT foo, COUNT(*) FROM test GROUP BY foo ORDER BY SCORE()")); - assertEquals("line 1:54: Cannot order by non-grouped column [SCORE()], expected [foo][1;23;31m][0m", readLine()); + assertFoundOneProblem(command("SELECT foo, COUNT(*) FROM test GROUP BY foo ORDER BY SCORE()")); + assertEquals("line 1:54: Cannot order by non-grouped column [SCORE()], expected [foo]" + END, readLine()); } @Override public void testSelectGroupByScore() throws Exception { index("test", body -> body.field("foo", 1)); - assertEquals("[1;31mBad request [[22;3;33mFound 1 problem(s)", - command("SELECT COUNT(*) FROM test GROUP BY SCORE()")); - assertEquals("line 1:36: Cannot use [SCORE()] for grouping[1;23;31m][0m", readLine()); + assertFoundOneProblem(command("SELECT COUNT(*) FROM test GROUP BY SCORE()")); + assertEquals("line 1:36: Cannot use [SCORE()] for grouping" + END, readLine()); } @Override public void testSelectScoreSubField() throws Exception { index("test", body -> body.field("foo", 1)); assertThat(command("SELECT SCORE().bar FROM test"), - startsWith("[1;31mBad request [[22;3;33mline 1:15: extraneous input '.' expecting {, ',',")); + startsWith(START + "Bad request [[3;33;22mline 1:15: extraneous input '.' expecting {, ',',")); } @Override public void testSelectScoreInScalar() throws Exception { index("test", body -> body.field("foo", 1)); - assertEquals("[1;31mBad request [[22;3;33mFound 1 problem(s)", - command("SELECT SIN(SCORE()) FROM test")); - assertEquals("line 1:12: [SCORE()] cannot be an argument to a function[1;23;31m][0m", readLine()); + assertFoundOneProblem(command("SELECT SIN(SCORE()) FROM test")); + assertEquals("line 1:12: [SCORE()] cannot be an argument to a function" + END, readLine()); } + + public static void assertFoundOneProblem(String commandResult) { + assertEquals(START + "Bad request [[3;33;22mFound 1 problem(s)", commandResult); + } + } diff --git a/qa/sql/src/main/java/org/elasticsearch/xpack/qa/sql/cli/FetchSizeTestCase.java b/qa/sql/src/main/java/org/elasticsearch/xpack/qa/sql/cli/FetchSizeTestCase.java index 92344e6cd28..dc34b9c1101 100644 --- a/qa/sql/src/main/java/org/elasticsearch/xpack/qa/sql/cli/FetchSizeTestCase.java +++ b/qa/sql/src/main/java/org/elasticsearch/xpack/qa/sql/cli/FetchSizeTestCase.java @@ -25,8 +25,9 @@ public abstract class FetchSizeTestCase extends CliIntegrationTestCase { } client().performRequest("PUT", "/test/doc/_bulk", singletonMap("refresh", "true"), new StringEntity(bulk.toString(), ContentType.APPLICATION_JSON)); - assertEquals("fetch size set to [90m4[0m", command("fetch size = 4")); - assertEquals("fetch separator set to \"[90m -- fetch sep -- [0m\"", command("fetch separator = \" -- fetch sep -- \"")); + assertEquals("[?1l>[?1000l[?2004lfetch size set to [90m4[0m", command("fetch size = 4")); + assertEquals("[?1l>[?1000l[?2004lfetch separator set to \"[90m -- fetch sep -- [0m\"", + command("fetch separator = \" -- fetch sep -- \"")); assertThat(command("SELECT * FROM test ORDER BY test_field ASC"), containsString("test_field")); assertThat(readLine(), containsString("----------")); int i = 0; @@ -41,9 +42,10 @@ public abstract class FetchSizeTestCase extends CliIntegrationTestCase { } public void testInvalidFetchSize() throws IOException { - assertEquals("[1;31mInvalid fetch size [[22;3;33mcat[1;23;31m][0m", command("fetch size = cat")); - assertEquals("[1;31mInvalid fetch size [[22;3;33m0[1;23;31m]. Must be > 0.[0m", command("fetch size = 0")); - assertEquals("[1;31mInvalid fetch size [[22;3;33m-1231[1;23;31m]. Must be > 0.[0m", command("fetch size = -1231")); - assertEquals("[1;31mInvalid fetch size [[22;3;33m" + Long.MAX_VALUE + "[1;23;31m][0m", command("fetch size = " + Long.MAX_VALUE)); + assertEquals(ErrorsTestCase.START + "Invalid fetch size [[3;33;22mcat" + ErrorsTestCase.END, command("fetch size = cat")); + assertEquals(ErrorsTestCase.START + "Invalid fetch size [[3;33;22m0[23;31;1m]. Must be > 0.[0m", command("fetch size = 0")); + assertEquals(ErrorsTestCase.START + "Invalid fetch size [[3;33;22m-1231[23;31;1m]. Must be > 0.[0m", command("fetch size = -1231")); + assertEquals(ErrorsTestCase.START + "Invalid fetch size [[3;33;22m" + Long.MAX_VALUE + ErrorsTestCase.END, + command("fetch size = " + Long.MAX_VALUE)); } } diff --git a/qa/sql/src/main/java/org/elasticsearch/xpack/qa/sql/cli/RemoteCli.java b/qa/sql/src/main/java/org/elasticsearch/xpack/qa/sql/cli/RemoteCli.java index e5852b805dc..be0c956c5d3 100644 --- a/qa/sql/src/main/java/org/elasticsearch/xpack/qa/sql/cli/RemoteCli.java +++ b/qa/sql/src/main/java/org/elasticsearch/xpack/qa/sql/cli/RemoteCli.java @@ -6,16 +6,25 @@ package org.elasticsearch.xpack.qa.sql.cli; import org.apache.logging.log4j.Logger; -import org.elasticsearch.SpecialPermission; +import org.apache.lucene.util.IOUtils; +import org.elasticsearch.cli.MockTerminal; +import org.elasticsearch.cli.Terminal; import org.elasticsearch.common.Nullable; import org.elasticsearch.common.Strings; import org.elasticsearch.common.logging.Loggers; +import org.elasticsearch.xpack.sql.cli.Cli; +import org.elasticsearch.xpack.sql.cli.CliTerminal; +import org.elasticsearch.xpack.sql.cli.JLineTerminal; +import org.jline.terminal.impl.ExternalTerminal; import java.io.BufferedReader; +import java.io.BufferedWriter; import java.io.Closeable; import java.io.IOException; import java.io.InputStreamReader; import java.io.OutputStreamWriter; +import java.io.PipedInputStream; +import java.io.PipedOutputStream; import java.io.PrintWriter; import java.net.InetAddress; import java.net.Socket; @@ -24,7 +33,13 @@ import java.nio.charset.StandardCharsets; import java.security.AccessController; import java.security.PrivilegedAction; import java.util.ArrayList; +import java.util.Arrays; +import java.util.Iterator; import java.util.List; +import java.util.concurrent.TimeUnit; +import java.util.concurrent.atomic.AtomicBoolean; +import java.util.concurrent.atomic.AtomicInteger; +import java.util.concurrent.atomic.AtomicReference; import java.util.function.Predicate; import static org.elasticsearch.test.ESTestCase.randomBoolean; @@ -34,101 +49,128 @@ import static org.hamcrest.Matchers.not; import static org.hamcrest.Matchers.startsWith; import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertThat; +import static org.junit.Assert.fail; +/** + * Wraps a CLI in as "real" a way as it can get without forking the CLI + * subprocess with the goal being integration testing of the CLI without + * breaking out security model by forking. We test the script that starts + * the CLI using packaging tests which is super "real" but not super fast + * and doesn't run super frequently. + */ public class RemoteCli implements Closeable { + // TODO rename this class to EmbeddedCli very soon private static final Logger logger = Loggers.getLogger(RemoteCli.class); - private static final InetAddress CLI_FIXTURE_ADDRESS; - private static final int CLI_FIXTURE_PORT; - static { - String addressAndPort = System.getProperty("tests.cli.fixture"); - if (addressAndPort == null) { - throw new IllegalArgumentException("Must set the [tests.cli.fixture] property. Gradle handles this for you " - + " in regular tests. In embedded mode the easiest thing to do is run " - + "`gradle :x-pack-elasticsearch:qa:sql:no-security:run` and to set the property to the contents of " - + "`qa/sql/no-security/build/fixtures/cliFixture/ports`"); - } - int split = addressAndPort.lastIndexOf(':'); - try { - CLI_FIXTURE_ADDRESS = InetAddress.getByName(addressAndPort.substring(0, split)); - } catch (UnknownHostException e) { - throw new RuntimeException(e); - } - CLI_FIXTURE_PORT = Integer.parseInt(addressAndPort.substring(split + 1)); - } - - private final Socket socket; - private final PrintWriter out; + private final Thread exec; + private final Cli cli; + private final AtomicInteger returnCode = new AtomicInteger(Integer.MIN_VALUE); + private final AtomicReference failure = new AtomicReference<>(); + private final BufferedWriter out; private final BufferedReader in; + /** + * Has the client already been closed? + */ + private boolean closed = false; public RemoteCli(String elasticsearchAddress, boolean checkConnectionOnStartup, @Nullable SecurityConfig security) throws IOException { - // Connect - SecurityManager sm = System.getSecurityManager(); - if (sm != null) { - sm.checkPermission(new SpecialPermission()); - } - logger.info("connecting to the cli fixture at {}:{}", CLI_FIXTURE_ADDRESS, CLI_FIXTURE_PORT); - socket = AccessController.doPrivileged(new PrivilegedAction() { + PipedOutputStream outgoing = new PipedOutputStream(); + PipedInputStream cliIn = new PipedInputStream(outgoing); + PipedInputStream incoming = new PipedInputStream(); + PipedOutputStream cliOut = new PipedOutputStream(incoming); + CliTerminal cliTerminal = new JLineTerminal( + new ExternalTerminal("test", "xterm-256color", cliIn, cliOut, StandardCharsets.UTF_8), + false); + cli = new Cli(cliTerminal) { @Override - public Socket run() { - try { - return new Socket(CLI_FIXTURE_ADDRESS, CLI_FIXTURE_PORT); - } catch (IOException e) { - throw new RuntimeException(e); - } + protected boolean addShutdownHook() { + return false; + } + }; + out = new BufferedWriter(new OutputStreamWriter(outgoing, StandardCharsets.UTF_8)); + in = new BufferedReader(new InputStreamReader(incoming, StandardCharsets.UTF_8)); + + List args = new ArrayList<>(); + if (security == null) { + args.add(elasticsearchAddress); + } else { + String address = security.user + "@" + elasticsearchAddress; + if (security.https) { + address = "https://" + address; + } else if (randomBoolean()) { + address = "http://" + address; + } + args.add(address); + if (security.keystoreLocation != null) { + args.add("-keystore_location"); + args.add(security.keystoreLocation); + } + } + if (false == checkConnectionOnStartup) { + args.add("-check"); + args.add("false"); + } + args.add("-debug"); + exec = new Thread(() -> { + try { + /* + * We don't really interact with the terminal because we're + * trying to test our interaction with jLine which doesn't + * support Elasticsearch's Terminal abstraction. + */ + Terminal terminal = new MockTerminal(); + int exitCode = cli.main(args.toArray(new String[0]), terminal); + returnCode.set(exitCode); + logger.info("cli exited with code [{}]", exitCode); + } catch (Exception e) { + failure.set(e); } }); - logger.info("connected"); - socket.setSoTimeout(10000); - out = new PrintWriter(new OutputStreamWriter(socket.getOutputStream(), StandardCharsets.UTF_8), true); - in = new BufferedReader(new InputStreamReader(socket.getInputStream(), StandardCharsets.UTF_8)); + exec.start(); try { - // Start the CLI - String command; - if (security == null) { - command = elasticsearchAddress; - } else { - command = security.user + "@" + elasticsearchAddress; - if (security.https) { - command = "https://" + command; - } else if (randomBoolean()) { - command = "http://" + command; - } - if (security.keystoreLocation != null) { - command = command + " -keystore_location " + security.keystoreLocation; - } - } - if (false == checkConnectionOnStartup) { - command += " -check false"; - } - /* Don't use println because it emits \r\n on windows but we put the - * terminal in unix mode to make the tests consistent. */ - out.print(command + "\n"); - out.flush(); - // Feed it passwords if needed - if (security != null && security.keystoreLocation != null) { - assertEquals("keystore password: ", readUntil(s -> s.endsWith(": "))); - out.print(security.keystorePassword + "\n"); - out.flush(); - } if (security != null) { - assertEquals("password: ", readUntil(s -> s.endsWith(": "))); - out.print(security.password + "\n"); + String passwordPrompt = "[?1h=[?2004hpassword: "; + if (security.keystoreLocation != null) { + assertEquals("[?1h=[?2004hkeystore password: ", readUntil(s -> s.endsWith(": "))); + out.write(security.keystorePassword + "\n"); + out.flush(); + logger.info("out: {}", security.keystorePassword); + // Read the newline echoed after the password prompt + assertEquals("", readLine()); + /* + * And for some reason jLine adds a second one so + * consume that too. I'm not sure why it does this + * but it looks right when a use runs the cli. + */ + assertEquals("", readLine()); + /* + * If we read the keystore password the console will + * emit some state reset escape sequences before the + * prompt for the password. + */ + passwordPrompt = "[?1l>[?1000l[?2004l[?1h=[?2004hpassword: "; + } + assertEquals(passwordPrompt, readUntil(s -> s.endsWith(": "))); + out.write(security.password + "\n"); out.flush(); + logger.info("out: {}", security.password); + // Read the newline echoed after the password prompt + assertEquals("", readLine()); } - // Throw out the logo and warnings about making a dumb terminal + // Throw out the logo while (false == readLine().contains("SQL")); - assertConnectionTest(); - } catch (AssertionError | Exception e) { - /* If there is an error during connection then try and - * force the socket shut. */ - forceClose(); - throw e; + } catch (IOException e) { + try { + forceClose(); + } catch (Exception closeException) { + e.addSuppressed(closeException); + throw e; + } } } @@ -147,15 +189,33 @@ public class RemoteCli implements Closeable { */ @Override public void close() throws IOException { + if (closed) { + return; + } try { // Try and shutdown the client normally - /* Don't use println because it enits \r\n on windows but we put the - * terminal in unix mode to make the tests consistent. */ - out.print("quit;\n"); + + /* + * Don't use command here because we want want + * to collect all the responses and report them + * as failures if there is a problem rather than + * failing on the first bad response. + */ + out.write("quit;\n"); out.flush(); List nonQuit = new ArrayList<>(); String line; - while (false == (line = readLine()).startsWith("[?1h=[33msql> [0mquit;[90mBye![0m")) { + while (true) { + line = readLine(); + if (line == null) { + fail("got EOF before [Bye!]. Extras " + nonQuit); + } + if (line.contains("quit;")) { + continue; + } + if (line.contains("Bye!")) { + break; + } if (false == line.isEmpty()) { nonQuit.add(line); } @@ -164,6 +224,7 @@ public class RemoteCli implements Closeable { } finally { forceClose(); } + assertEquals(0, returnCode.get()); } /** @@ -171,10 +232,18 @@ public class RemoteCli implements Closeable { * the remote down in an orderly way. */ public void forceClose() throws IOException { - out.close(); - in.close(); - // Most importantly, close the socket so the next test can use the fixture - socket.close(); + closed = true; + IOUtils.close(out, in, cli); + try { + exec.join(TimeUnit.SECONDS.toMillis(10)); + } catch (InterruptedException e) { + Thread.currentThread().interrupt(); + throw new RuntimeException(e); + } + Exception e = failure.get(); + if (e != null) { + throw new RuntimeException("CLI thread failed", e); + } } /** @@ -183,22 +252,42 @@ public class RemoteCli implements Closeable { public String command(String command) throws IOException { assertThat("; automatically added", command, not(endsWith(";"))); logger.info("out: {};", command); - /* Don't use println because it emits \r\n on windows but we put the - * terminal in unix mode to make the tests consistent. */ - out.print(command + ";\n"); + out.write(command + ";\n"); out.flush(); - final String firstResponse = "[?1h=[33msql> [0m" + - Strings.collectionToDelimitedString(Strings.splitSmart(command, "\n", false), "[?1h=[33m | [0m") + ";"; - String firstLine = readLine(); - assertThat(firstLine, startsWith(firstResponse)); - return firstLine.substring(firstResponse.length()); + for (String echo : expectedCommandEchos(command)) { + assertEquals(echo, readLine()); + } + return readLine(); + } + /** + * Create the "echo" that we expect jLine to send to the terminal + * while we're typing a command. + */ + private List expectedCommandEchos(String command) { + List commandLines = Strings.splitSmart(command, "\n", false); + List result = new ArrayList<>(commandLines.size() * 2); + result.add("[?1h=[?2004h[33msql> [0m" + commandLines.get(0)); + // Every line gets an extra new line because, I dunno, but it looks right in the CLI + result.add(""); + for (int i = 1; i < commandLines.size(); i++) { + result.add("[?1l>[?1000l[?2004l[?1h=[?2004h[33m | [0m" + commandLines.get(i)); + // Every line gets an extra new line because, I dunno, but it looks right in the CLI + result.add(""); + } + result.set(result.size() - 2, result.get(result.size() - 2) + ";"); + return result; } public String readLine() throws IOException { - /* Since we can't *see* esc in the error messages we just + /* + * Since we can't *see* esc in the error messages we just * remove it here and pretend it isn't required. Hopefully - * `[` is enough for us to assert on. */ - String line = in.readLine().replace("\u001B", ""); + * `[` is enough for us to assert on. + * + * `null` means EOF so we should just pass that back through. + */ + String line = in.readLine(); + line = line == null ? null : line.replace("\u001B", ""); logger.info("in : {}", line); return line; } @@ -206,14 +295,25 @@ public class RemoteCli implements Closeable { private String readUntil(Predicate end) throws IOException { StringBuilder b = new StringBuilder(); String result; - do { + while (true) { int c = in.read(); if (c == -1) { throw new IOException("got eof before end"); } + if (c == '\u001B') { + /* + * Since we can't *see* esc in the error messages we just + * remove it here and pretend it isn't required. Hopefully + * `[` is enough for us to assert on. + */ + continue; + } b.append((char) c); result = b.toString(); - } while (false == end.test(result)); + if (end.test(result)) { + break; + } + } logger.info("in : {}", result); return result; } diff --git a/test/sql-cli-fixture/build.gradle b/test/sql-cli-fixture/build.gradle deleted file mode 100644 index dcf745c89a2..00000000000 --- a/test/sql-cli-fixture/build.gradle +++ /dev/null @@ -1,14 +0,0 @@ -apply plugin: 'elasticsearch.build' - -forbiddenApisMain { - // does not depend on core, so only jdk and http signatures should be checked - signaturesURLs = [this.class.getResource('/forbidden/jdk-signatures.txt')] -} - -thirdPartyAudit.enabled = false -licenseHeaders.enabled = false -test.enabled = false -jarHell.enabled = false -// Not published so no need to assemble -tasks.remove(assemble) -build.dependsOn.remove('assemble') diff --git a/test/sql-cli-fixture/src/main/java/org/elasticsearch/xpack/sql/cli/fixture/CliFixture.java b/test/sql-cli-fixture/src/main/java/org/elasticsearch/xpack/sql/cli/fixture/CliFixture.java deleted file mode 100644 index 6367804333f..00000000000 --- a/test/sql-cli-fixture/src/main/java/org/elasticsearch/xpack/sql/cli/fixture/CliFixture.java +++ /dev/null @@ -1,154 +0,0 @@ -/* - * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one - * or more contributor license agreements. Licensed under the Elastic License; - * you may not use this file except in compliance with the Elastic License. - */ -package org.elasticsearch.xpack.sql.cli.fixture; - -import java.io.BufferedReader; -import java.io.IOException; -import java.io.InputStreamReader; -import java.lang.management.ManagementFactory; -import java.net.Inet6Address; -import java.net.InetAddress; -import java.net.InetSocketAddress; -import java.net.ServerSocket; -import java.net.Socket; -import java.nio.charset.StandardCharsets; -import java.nio.file.Files; -import java.nio.file.Path; -import java.nio.file.Paths; -import java.nio.file.StandardCopyOption; -import java.util.ArrayList; -import java.util.Arrays; -import java.util.Collections; -import java.util.List; -import java.util.Locale; - -import static java.util.Collections.singleton; - -public class CliFixture { - public static void main(String[] args) throws IOException, InterruptedException { - if (args.length < 2) { - throw new IllegalArgumentException("usage: "); - } - Path dir = Paths.get(args[0]); - Path cliJar = Paths.get(args[1]); - int port = 0; - if (args.length > 2) { - port = Integer.parseInt(args[2]); - } - if (false == Files.exists(cliJar)) { - throw new IllegalArgumentException(cliJar + " doesn't exist"); - } - if (false == Files.isRegularFile(cliJar)) { - throw new IllegalArgumentException(cliJar + " is not a regular file"); - } - String javaExec = "java"; - boolean isWindows = System.getProperty("os.name").toLowerCase(Locale.ROOT).contains("win"); - - if (isWindows) { - javaExec += ".exe"; - } - Path javaExecutable = Paths.get(System.getProperty("java.home"), "bin", javaExec); - if (false == Files.exists(javaExecutable)) { - throw new IllegalArgumentException(javaExec + " doesn't exist"); - } - if (false == Files.isExecutable(javaExecutable)) { - throw new IllegalArgumentException(javaExec + " isn't executable"); - } - - try (ServerSocket server = new ServerSocket()) { - server.bind(new InetSocketAddress(InetAddress.getLoopbackAddress(), port)); - // write pid file - Path tmp = Files.createTempFile(dir, null, null); - String pid = ManagementFactory.getRuntimeMXBean().getName().split("@")[0]; - Files.write(tmp, Collections.singleton(pid)); - Files.move(tmp, dir.resolve("pid"), StandardCopyOption.ATOMIC_MOVE); - - // write port file - tmp = Files.createTempFile(dir, null, null); - InetSocketAddress bound = (InetSocketAddress) server.getLocalSocketAddress(); - if (bound.getAddress() instanceof Inet6Address) { - Files.write(tmp, singleton("[" + bound.getHostString() + "]:" + bound.getPort())); - } - else { - Files.write(tmp, singleton(bound.getHostString() + ":" + bound.getPort())); - } - Files.move(tmp, dir.resolve("ports"), StandardCopyOption.ATOMIC_MOVE); - - boolean run = true; - // Run forever until killed - while (run) { - try { - println("accepting on localhost:" + server.getLocalPort()); - Socket s = server.accept(); - String line = new BufferedReader(new InputStreamReader(s.getInputStream(), StandardCharsets.UTF_8)).readLine(); - if (line == null || line.isEmpty()) { - continue; - } - List command = new ArrayList<>(); - command.add(javaExecutable.toString()); - // command.add("-agentlib:jdwp=transport=dt_socket,server=y,suspend=y,address=8000"); - // Force a specific terminal type so we have consistent responses for testing. - command.add("-Dorg.jline.terminal.type=xterm-256color"); - // Disable terminal types that won't work with stdin isn't actually a tty - command.add("-Dorg.jline.terminal.jna=false"); - command.add("-Dorg.jline.terminal.jansi=false"); - command.add("-Dorg.jline.terminal.exec=false"); - command.add("-Dorg.jline.terminal.dumb=true"); - command.add("-jar"); - command.add(cliJar.toString()); - command.add("-d"); - command.addAll(Arrays.asList(line.split(" "))); - ProcessBuilder cliBuilder = new ProcessBuilder(command); - // Clear the environment to drop JAVA_TOOLS which prints strange things on startup - cliBuilder.environment().clear(); - cliBuilder.redirectErrorStream(true); - Process process = cliBuilder.start(); - println("started " + command); - new Thread(() -> { - int i; - try { - while ((i = process.getInputStream().read()) != -1) { - s.getOutputStream().write(i); - s.getOutputStream().flush(); - } - } catch (IOException e) { - throw new RuntimeException("failed to copy from process to socket", e); - } finally { - process.destroyForcibly(); - } - }).start(); - new Thread(() -> { - int i; - try { - while ((i = s.getInputStream().read()) != -1) { - process.getOutputStream().write(i); - process.getOutputStream().flush(); - } - } catch (IOException e) { - throw new RuntimeException("failed to copy from socket to process", e); - } finally { - process.destroyForcibly(); - } - }).start(); - process.waitFor(); - } catch (IOException e) { - printStackTrace("error at the top level, continuing", e); - } - } - } - } - - @SuppressForbidden(reason = "cli application") - private static void println(String line) { - System.out.println(line); - } - - @SuppressForbidden(reason = "cli application") - private static void printStackTrace(String reason, Throwable t) { - System.err.println(reason); - t.printStackTrace(); - } -} diff --git a/test/sql-cli-fixture/src/main/java/org/elasticsearch/xpack/sql/cli/fixture/SuppressForbidden.java b/test/sql-cli-fixture/src/main/java/org/elasticsearch/xpack/sql/cli/fixture/SuppressForbidden.java deleted file mode 100644 index e2232d679dc..00000000000 --- a/test/sql-cli-fixture/src/main/java/org/elasticsearch/xpack/sql/cli/fixture/SuppressForbidden.java +++ /dev/null @@ -1,17 +0,0 @@ -/* - * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one - * or more contributor license agreements. Licensed under the Elastic License; - * you may not use this file except in compliance with the Elastic License. - */ -package org.elasticsearch.xpack.sql.cli.fixture; - -import java.lang.annotation.ElementType; -import java.lang.annotation.Retention; -import java.lang.annotation.RetentionPolicy; -import java.lang.annotation.Target; - -@Retention(RetentionPolicy.CLASS) -@Target({ ElementType.CONSTRUCTOR, ElementType.FIELD, ElementType.METHOD, ElementType.TYPE }) -public @interface SuppressForbidden { - String reason(); -} \ No newline at end of file