From a2c37c09897eead58b2d37e36a203af8aa84c687 Mon Sep 17 00:00:00 2001 From: Ryan Ernst Date: Mon, 1 Feb 2016 16:18:20 -0800 Subject: [PATCH] CliTool: Allow unexpected exceptions to propagate Cli tools currently catch all exceptions, and only print the exception message, except when a special system property is set. Even with this flag set, certain exceptions, like IOException, are captured and their stack trace is always lost. This change adds a UserError class, which can be used a cli tools to specify a message to the user, as well as an exit status. All other exceptions are propagated out of main, so java will exit with non-zero and print the stack trace. --- .../bootstrap/BootstrapCLIParser.java | 11 +-- .../org/elasticsearch/common/cli/CliTool.java | 49 ++++++------- .../elasticsearch/common/cli/Terminal.java | 11 --- .../elasticsearch/common/cli/UserError.java | 35 ++++++++++ .../org/elasticsearch/env/Environment.java | 27 -------- .../plugins/InstallPluginCommand.java | 29 ++++---- .../org/elasticsearch/plugins/PluginCli.java | 2 +- .../plugins/RemovePluginCommand.java | 5 +- .../common/cli/TerminalTests.java | 18 ----- .../elasticsearch/plugins/PluginCliTests.java | 16 +---- .../mapper/attachments/StandaloneRunner.java | 14 ++-- .../bootstrap/BootstrapCliParserTests.java | 13 ++-- .../common/cli/CliToolTests.java | 68 ++++--------------- .../plugins/InstallPluginCommandTests.java | 28 +++++--- .../plugins/RemovePluginCommandTests.java | 6 +- 15 files changed, 125 insertions(+), 207 deletions(-) create mode 100644 core/src/main/java/org/elasticsearch/common/cli/UserError.java diff --git a/core/src/main/java/org/elasticsearch/bootstrap/BootstrapCLIParser.java b/core/src/main/java/org/elasticsearch/bootstrap/BootstrapCLIParser.java index 51a16f104bc..c89ca8ea3dd 100644 --- a/core/src/main/java/org/elasticsearch/bootstrap/BootstrapCLIParser.java +++ b/core/src/main/java/org/elasticsearch/bootstrap/BootstrapCLIParser.java @@ -27,6 +27,7 @@ import org.elasticsearch.common.SuppressForbidden; import org.elasticsearch.common.cli.CliTool; import org.elasticsearch.common.cli.CliToolConfig; import org.elasticsearch.common.cli.Terminal; +import org.elasticsearch.common.cli.UserError; import org.elasticsearch.common.settings.Settings; import org.elasticsearch.env.Environment; import org.elasticsearch.monitor.jvm.JvmInfo; @@ -103,7 +104,7 @@ final class BootstrapCLIParser extends CliTool { // TODO: don't use system properties as a way to do this, its horrible... @SuppressForbidden(reason = "Sets system properties passed as CLI parameters") - public static Command parse(Terminal terminal, CommandLine cli) { + public static Command parse(Terminal terminal, CommandLine cli) throws UserError { if (cli.hasOption("V")) { return Version.parse(terminal, cli); } @@ -132,11 +133,11 @@ final class BootstrapCLIParser extends CliTool { String arg = iterator.next(); if (!arg.startsWith("--")) { if (arg.startsWith("-D") || arg.startsWith("-d") || arg.startsWith("-p")) { - throw new IllegalArgumentException( + throw new UserError(ExitStatus.USAGE, "Parameter [" + arg + "] starting with \"-D\", \"-d\" or \"-p\" must be before any parameters starting with --" ); } else { - throw new IllegalArgumentException("Parameter [" + arg + "]does not start with --"); + throw new UserError(ExitStatus.USAGE, "Parameter [" + arg + "]does not start with --"); } } // if there is no = sign, we have to get the next argu @@ -150,11 +151,11 @@ final class BootstrapCLIParser extends CliTool { if (iterator.hasNext()) { String value = iterator.next(); if (value.startsWith("--")) { - throw new IllegalArgumentException("Parameter [" + arg + "] needs value"); + throw new UserError(ExitStatus.USAGE, "Parameter [" + arg + "] needs value"); } System.setProperty("es." + arg, value); } else { - throw new IllegalArgumentException("Parameter [" + arg + "] needs value"); + throw new UserError(ExitStatus.USAGE, "Parameter [" + arg + "] needs value"); } } } diff --git a/core/src/main/java/org/elasticsearch/common/cli/CliTool.java b/core/src/main/java/org/elasticsearch/common/cli/CliTool.java index 0d32b6e2779..7e954bc85ee 100644 --- a/core/src/main/java/org/elasticsearch/common/cli/CliTool.java +++ b/core/src/main/java/org/elasticsearch/common/cli/CliTool.java @@ -19,9 +19,14 @@ package org.elasticsearch.common.cli; +import org.apache.commons.cli.AlreadySelectedException; +import org.apache.commons.cli.AmbiguousOptionException; import org.apache.commons.cli.CommandLine; import org.apache.commons.cli.CommandLineParser; import org.apache.commons.cli.DefaultParser; +import org.apache.commons.cli.MissingArgumentException; +import org.apache.commons.cli.MissingOptionException; +import org.apache.commons.cli.UnrecognizedOptionException; import org.elasticsearch.common.settings.Settings; import org.elasticsearch.env.Environment; import org.elasticsearch.node.internal.InternalSettingsPreparer; @@ -50,7 +55,7 @@ import static org.elasticsearch.common.settings.Settings.Builder.EMPTY_SETTINGS; public abstract class CliTool { // based on sysexits.h - public static enum ExitStatus { + public enum ExitStatus { OK(0), OK_AND_EXIT(0), USAGE(64), /* command line usage error */ @@ -69,23 +74,13 @@ public abstract class CliTool { final int status; - private ExitStatus(int status) { + ExitStatus(int status) { this.status = status; } public int status() { return status; } - - public static ExitStatus fromStatus(int status) { - for (ExitStatus exitStatus : values()) { - if (exitStatus.status() == status) { - return exitStatus; - } - } - - return null; - } } protected final Terminal terminal; @@ -108,7 +103,7 @@ public abstract class CliTool { settings = env.settings(); } - public final ExitStatus execute(String... args) { + public final ExitStatus execute(String... args) throws Exception { // first lets see if the user requests tool help. We're doing it only if // this is a multi-command tool. If it's a single command tool, the -h/--help @@ -146,23 +141,11 @@ public abstract class CliTool { } } - Command command = null; try { - - command = parse(cmd, args); - return command.execute(settings, env); - } catch (IOException ioe) { - terminal.printError(ioe); - return ExitStatus.IO_ERROR; - } catch (IllegalArgumentException ilae) { - terminal.printError(ilae); - return ExitStatus.USAGE; - } catch (Throwable t) { - terminal.printError(t); - if (command == null) { - return ExitStatus.USAGE; - } - return ExitStatus.CODE_ERROR; + return parse(cmd, args).execute(settings, env); + } catch (UserError error) { + terminal.printError(error.getMessage()); + return error.exitStatus; } } @@ -177,7 +160,13 @@ public abstract class CliTool { if (cli.hasOption("h")) { return helpCmd(cmd); } - cli = parser.parse(cmd.options(), args, cmd.isStopAtNonOption()); + try { + cli = parser.parse(cmd.options(), args, cmd.isStopAtNonOption()); + } catch (AlreadySelectedException|MissingArgumentException|MissingOptionException|UnrecognizedOptionException e) { + // intentionally drop the stack trace here as these are really user errors, + // the stack trace into cli parsing lib is not important + throw new UserError(ExitStatus.USAGE, e.toString()); + } Terminal.Verbosity verbosity = Terminal.Verbosity.resolve(cli); terminal.verbosity(verbosity); return parse(cmd.name(), cli); diff --git a/core/src/main/java/org/elasticsearch/common/cli/Terminal.java b/core/src/main/java/org/elasticsearch/common/cli/Terminal.java index 5e4bc09ad9f..7608c9812de 100644 --- a/core/src/main/java/org/elasticsearch/common/cli/Terminal.java +++ b/core/src/main/java/org/elasticsearch/common/cli/Terminal.java @@ -35,8 +35,6 @@ import java.util.Locale; @SuppressForbidden(reason = "System#out") public abstract class Terminal { - public static final String DEBUG_SYSTEM_PROPERTY = "es.cli.debug"; - public static final Terminal DEFAULT = ConsoleTerminal.supported() ? new ConsoleTerminal() : new SystemTerminal(); public static enum Verbosity { @@ -64,7 +62,6 @@ public abstract class Terminal { } private Verbosity verbosity = Verbosity.NORMAL; - private final boolean isDebugEnabled; public Terminal() { this(Verbosity.NORMAL); @@ -72,7 +69,6 @@ public abstract class Terminal { public Terminal(Verbosity verbosity) { this.verbosity = verbosity; - this.isDebugEnabled = "true".equals(System.getProperty(DEBUG_SYSTEM_PROPERTY, "false")); } public void verbosity(Verbosity verbosity) { @@ -119,13 +115,6 @@ public abstract class Terminal { println(Verbosity.SILENT, "ERROR: " + msg, args); } - public void printError(Throwable t) { - printError("%s", t.toString()); - if (isDebugEnabled) { - printStackTrace(t); - } - } - public void printWarn(String msg, Object... args) { println(Verbosity.SILENT, "WARN: " + msg, args); } diff --git a/core/src/main/java/org/elasticsearch/common/cli/UserError.java b/core/src/main/java/org/elasticsearch/common/cli/UserError.java new file mode 100644 index 00000000000..ad709830885 --- /dev/null +++ b/core/src/main/java/org/elasticsearch/common/cli/UserError.java @@ -0,0 +1,35 @@ +/* + * Licensed to Elasticsearch under one or more contributor + * license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright + * ownership. Elasticsearch 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.elasticsearch.common.cli; + +/** + * An exception representing a user fixable problem in {@link CliTool} usage. + */ +public class UserError extends Exception { + + /** The exist status the cli should use when catching this user error. */ + public final CliTool.ExitStatus exitStatus; + + /** Constructs a UserError with an exit status and message to show the user. */ + public UserError(CliTool.ExitStatus exitStatus, String msg) { + super(msg); + this.exitStatus = exitStatus; + } +} diff --git a/core/src/main/java/org/elasticsearch/env/Environment.java b/core/src/main/java/org/elasticsearch/env/Environment.java index 65d62bd9e33..2fc85099e8b 100644 --- a/core/src/main/java/org/elasticsearch/env/Environment.java +++ b/core/src/main/java/org/elasticsearch/env/Environment.java @@ -330,31 +330,4 @@ public class Environment { public static FileStore getFileStore(Path path) throws IOException { return ESFileStore.getMatchingFileStore(path, fileStores); } - - /** - * Returns true if the path is writable. - * Acts just like {@link Files#isWritable(Path)}, except won't - * falsely return false for paths on SUBST'd drive letters - * See https://bugs.openjdk.java.net/browse/JDK-8034057 - * Note this will set the file modification time (to its already-set value) - * to test access. - */ - @SuppressForbidden(reason = "works around https://bugs.openjdk.java.net/browse/JDK-8034057") - public static boolean isWritable(Path path) throws IOException { - boolean v = Files.isWritable(path); - if (v || Constants.WINDOWS == false) { - return v; - } - - // isWritable returned false on windows, the hack begins!!!!!! - // resetting the modification time is the least destructive/simplest - // way to check for both files and directories, and fails early just - // in getting the current value if file doesn't exist, etc - try { - Files.setLastModifiedTime(path, Files.getLastModifiedTime(path)); - return true; - } catch (Throwable e) { - return false; - } - } } diff --git a/core/src/main/java/org/elasticsearch/plugins/InstallPluginCommand.java b/core/src/main/java/org/elasticsearch/plugins/InstallPluginCommand.java index 656378b0f89..5600d407948 100644 --- a/core/src/main/java/org/elasticsearch/plugins/InstallPluginCommand.java +++ b/core/src/main/java/org/elasticsearch/plugins/InstallPluginCommand.java @@ -48,6 +48,7 @@ import org.elasticsearch.Version; import org.elasticsearch.bootstrap.JarHell; import org.elasticsearch.common.cli.CliTool; import org.elasticsearch.common.cli.Terminal; +import org.elasticsearch.common.cli.UserError; import org.elasticsearch.common.hash.MessageDigests; import org.elasticsearch.common.io.FileSystemUtils; import org.elasticsearch.common.settings.Settings; @@ -136,10 +137,6 @@ class InstallPluginCommand extends CliTool.Command { Files.createDirectory(env.pluginsFile()); } - if (Environment.isWritable(env.pluginsFile()) == false) { - throw new IOException("Plugins directory is read only: " + env.pluginsFile()); - } - Path pluginZip = download(pluginId, env.tmpFile()); Path extractedZip = unzip(pluginZip, env.pluginsFile()); install(extractedZip, env); @@ -148,7 +145,7 @@ class InstallPluginCommand extends CliTool.Command { } /** Downloads the plugin and returns the file it was downloaded to. */ - private Path download(String pluginId, Path tmpDir) throws IOException { + private Path download(String pluginId, Path tmpDir) throws Exception { if (OFFICIAL_PLUGINS.contains(pluginId)) { final String version = Version.CURRENT.toString(); final String url; @@ -189,7 +186,7 @@ class InstallPluginCommand extends CliTool.Command { } /** Downloads a zip from the url, as well as a SHA1 checksum, and checks the checksum. */ - private Path downloadZipAndChecksum(String urlString, Path tmpDir) throws IOException { + private Path downloadZipAndChecksum(String urlString, Path tmpDir) throws Exception { Path zip = downloadZip(urlString, tmpDir); URL checksumUrl = new URL(urlString + ".sha1"); @@ -198,14 +195,14 @@ class InstallPluginCommand extends CliTool.Command { BufferedReader checksumReader = new BufferedReader(new InputStreamReader(in, StandardCharsets.UTF_8)); expectedChecksum = checksumReader.readLine(); if (checksumReader.readLine() != null) { - throw new IllegalArgumentException("Invalid checksum file at " + urlString.toString()); + throw new UserError(CliTool.ExitStatus.IO_ERROR, "Invalid checksum file at " + checksumUrl); } } byte[] zipbytes = Files.readAllBytes(zip); String gotChecksum = MessageDigests.toHexString(MessageDigests.sha1().digest(zipbytes)); if (expectedChecksum.equals(gotChecksum) == false) { - throw new IllegalStateException("SHA1 mismatch, expected " + expectedChecksum + " but got " + gotChecksum); + throw new UserError(CliTool.ExitStatus.IO_ERROR, "SHA1 mismatch, expected " + expectedChecksum + " but got " + gotChecksum); } return zip; @@ -250,7 +247,7 @@ class InstallPluginCommand extends CliTool.Command { // don't let luser install plugin as a module... // they might be unavoidably in maven central and are packaged up the same way) if (MODULES.contains(info.getName())) { - throw new IOException("plugin '" + info.getName() + "' cannot be installed like this, it is a system module"); + throw new UserError(CliTool.ExitStatus.USAGE, "plugin '" + info.getName() + "' cannot be installed like this, it is a system module"); } // check for jar hell before any copying @@ -306,7 +303,7 @@ class InstallPluginCommand extends CliTool.Command { final Path destination = env.pluginsFile().resolve(info.getName()); if (Files.exists(destination)) { - throw new IOException("plugin directory " + destination.toAbsolutePath() + " already exists. To update the plugin, uninstall it first using 'remove " + info.getName() + "' command"); + throw new UserError(CliTool.ExitStatus.USAGE, "plugin directory " + destination.toAbsolutePath() + " already exists. To update the plugin, uninstall it first using 'remove " + info.getName() + "' command"); } Path tmpBinDir = tmpRoot.resolve("bin"); @@ -337,9 +334,9 @@ class InstallPluginCommand extends CliTool.Command { } /** Copies the files from {@code tmpBinDir} into {@code destBinDir}, along with permissions from dest dirs parent. */ - private void installBin(PluginInfo info, Path tmpBinDir, Path destBinDir) throws IOException { + private void installBin(PluginInfo info, Path tmpBinDir, Path destBinDir) throws Exception { if (Files.isDirectory(tmpBinDir) == false) { - throw new IOException("bin in plugin " + info.getName() + " is not a directory"); + throw new UserError(CliTool.ExitStatus.IO_ERROR, "bin in plugin " + info.getName() + " is not a directory"); } Files.createDirectory(destBinDir); @@ -357,7 +354,7 @@ class InstallPluginCommand extends CliTool.Command { try (DirectoryStream stream = Files.newDirectoryStream(tmpBinDir)) { for (Path srcFile : stream) { if (Files.isDirectory(srcFile)) { - throw new IOException("Directories not allowed in bin dir for plugin " + info.getName()); + throw new UserError(CliTool.ExitStatus.DATA_ERROR, "Directories not allowed in bin dir for plugin " + info.getName() + ", found " + srcFile.getFileName()); } Path destFile = destBinDir.resolve(tmpBinDir.relativize(srcFile)); @@ -376,9 +373,9 @@ class InstallPluginCommand extends CliTool.Command { * Copies the files from {@code tmpConfigDir} into {@code destConfigDir}. * Any files existing in both the source and destination will be skipped. */ - private void installConfig(PluginInfo info, Path tmpConfigDir, Path destConfigDir) throws IOException { + private void installConfig(PluginInfo info, Path tmpConfigDir, Path destConfigDir) throws Exception { if (Files.isDirectory(tmpConfigDir) == false) { - throw new IOException("config in plugin " + info.getName() + " is not a directory"); + throw new UserError(CliTool.ExitStatus.IO_ERROR, "config in plugin " + info.getName() + " is not a directory"); } // create the plugin's config dir "if necessary" @@ -387,7 +384,7 @@ class InstallPluginCommand extends CliTool.Command { try (DirectoryStream stream = Files.newDirectoryStream(tmpConfigDir)) { for (Path srcFile : stream) { if (Files.isDirectory(srcFile)) { - throw new IOException("Directories not allowed in config dir for plugin " + info.getName()); + throw new UserError(CliTool.ExitStatus.DATA_ERROR, "Directories not allowed in config dir for plugin " + info.getName()); } Path destFile = destConfigDir.resolve(tmpConfigDir.relativize(srcFile)); diff --git a/core/src/main/java/org/elasticsearch/plugins/PluginCli.java b/core/src/main/java/org/elasticsearch/plugins/PluginCli.java index c69c07b3d6b..30a36501a61 100644 --- a/core/src/main/java/org/elasticsearch/plugins/PluginCli.java +++ b/core/src/main/java/org/elasticsearch/plugins/PluginCli.java @@ -55,7 +55,7 @@ public class PluginCli extends CliTool { .cmds(LIST_CMD, INSTALL_CMD, REMOVE_CMD) .build(); - public static void main(String[] args) { + public static void main(String[] args) throws Exception { // initialize default for es.logger.level because we will not read the logging.yml String loggerLevel = System.getProperty("es.logger.level", "INFO"); // Set the appender for all potential log files to terminal so that other components that use the logger print out the diff --git a/core/src/main/java/org/elasticsearch/plugins/RemovePluginCommand.java b/core/src/main/java/org/elasticsearch/plugins/RemovePluginCommand.java index f5e55ba2b24..d8fd0b8f250 100644 --- a/core/src/main/java/org/elasticsearch/plugins/RemovePluginCommand.java +++ b/core/src/main/java/org/elasticsearch/plugins/RemovePluginCommand.java @@ -29,6 +29,7 @@ import org.apache.lucene.util.IOUtils; import org.elasticsearch.common.Strings; import org.elasticsearch.common.cli.CliTool; import org.elasticsearch.common.cli.Terminal; +import org.elasticsearch.common.cli.UserError; import org.elasticsearch.common.settings.Settings; import org.elasticsearch.env.Environment; @@ -51,7 +52,7 @@ class RemovePluginCommand extends CliTool.Command { Path pluginDir = env.pluginsFile().resolve(pluginName); if (Files.exists(pluginDir) == false) { - throw new IllegalArgumentException("Plugin " + pluginName + " not found. Run 'plugin list' to get list of installed plugins."); + throw new UserError(CliTool.ExitStatus.USAGE, "Plugin " + pluginName + " not found. Run 'plugin list' to get list of installed plugins."); } List pluginPaths = new ArrayList<>(); @@ -59,7 +60,7 @@ class RemovePluginCommand extends CliTool.Command { Path pluginBinDir = env.binFile().resolve(pluginName); if (Files.exists(pluginBinDir)) { if (Files.isDirectory(pluginBinDir) == false) { - throw new IllegalStateException("Bin dir for " + pluginName + " is not a directory"); + throw new UserError(CliTool.ExitStatus.IO_ERROR, "Bin dir for " + pluginName + " is not a directory"); } pluginPaths.add(pluginBinDir); terminal.println(VERBOSE, "Removing: %s", pluginBinDir); diff --git a/core/src/test/java/org/elasticsearch/common/cli/TerminalTests.java b/core/src/test/java/org/elasticsearch/common/cli/TerminalTests.java index 259ee109f0f..498dd269a23 100644 --- a/core/src/test/java/org/elasticsearch/common/cli/TerminalTests.java +++ b/core/src/test/java/org/elasticsearch/common/cli/TerminalTests.java @@ -46,24 +46,6 @@ public class TerminalTests extends CliToolTestCase { assertPrinted(terminal, Terminal.Verbosity.VERBOSE, "text"); } - public void testError() throws Exception { - try { - // actually throw so we have a stacktrace - throw new NoSuchFileException("/path/to/some/file"); - } catch (NoSuchFileException e) { - CaptureOutputTerminal terminal = new CaptureOutputTerminal(Terminal.Verbosity.NORMAL); - terminal.printError(e); - List output = terminal.getTerminalOutput(); - assertFalse(output.isEmpty()); - assertTrue(output.get(0), output.get(0).contains("NoSuchFileException")); // exception class - assertTrue(output.get(0), output.get(0).contains("/path/to/some/file")); // message - assertEquals(1, output.size()); - - // TODO: we should test stack trace is printed in debug mode...except debug is a sysprop instead of - // a command line param...maybe it should be VERBOSE instead of a separate debug prop? - } - } - private void assertPrinted(CaptureOutputTerminal logTerminal, Terminal.Verbosity verbosity, String text) { logTerminal.print(verbosity, text); assertThat(logTerminal.getTerminalOutput(), hasSize(1)); diff --git a/core/src/test/java/org/elasticsearch/plugins/PluginCliTests.java b/core/src/test/java/org/elasticsearch/plugins/PluginCliTests.java index 99d1c33821b..3a121590083 100644 --- a/core/src/test/java/org/elasticsearch/plugins/PluginCliTests.java +++ b/core/src/test/java/org/elasticsearch/plugins/PluginCliTests.java @@ -19,21 +19,15 @@ package org.elasticsearch.plugins; -import org.elasticsearch.common.cli.CliTool; import org.elasticsearch.common.cli.CliToolTestCase; -import java.io.IOException; -import java.net.MalformedURLException; -import java.nio.file.Path; - -import static org.elasticsearch.common.cli.CliTool.ExitStatus.IO_ERROR; import static org.elasticsearch.common.cli.CliTool.ExitStatus.OK_AND_EXIT; import static org.hamcrest.Matchers.containsString; import static org.hamcrest.Matchers.hasItem; import static org.hamcrest.Matchers.is; public class PluginCliTests extends CliToolTestCase { - public void testHelpWorks() throws IOException { + public void testHelpWorks() throws Exception { CliToolTestCase.CaptureOutputTerminal terminal = new CliToolTestCase.CaptureOutputTerminal(); assertThat(new PluginCli(terminal).execute(args("--help")), is(OK_AND_EXIT)); assertTerminalOutputContainsHelpFile(terminal, "/org/elasticsearch/plugins/plugin.help"); @@ -53,12 +47,4 @@ public class PluginCliTests extends CliToolTestCase { assertThat(new PluginCli(terminal).execute(args("list -h")), is(OK_AND_EXIT)); assertTerminalOutputContainsHelpFile(terminal, "/org/elasticsearch/plugins/plugin-list.help"); } - - public void testUrlSpacesInPath() throws MalformedURLException { - CliToolTestCase.CaptureOutputTerminal terminal = new CliToolTestCase.CaptureOutputTerminal(); - Path tmpDir = createTempDir().resolve("foo deps"); - String finalDir = tmpDir.toAbsolutePath().toUri().toURL().toString(); - CliTool.ExitStatus execute = new PluginCli(terminal).execute("install", finalDir); - assertThat(execute.status(), is(IO_ERROR.status())); - } } diff --git a/plugins/mapper-attachments/src/test/java/org/elasticsearch/mapper/attachments/StandaloneRunner.java b/plugins/mapper-attachments/src/test/java/org/elasticsearch/mapper/attachments/StandaloneRunner.java index 217d48a8565..c94b5e0b43a 100644 --- a/plugins/mapper-attachments/src/test/java/org/elasticsearch/mapper/attachments/StandaloneRunner.java +++ b/plugins/mapper-attachments/src/test/java/org/elasticsearch/mapper/attachments/StandaloneRunner.java @@ -139,14 +139,10 @@ public class StandaloneRunner extends CliTool { } public static byte[] copyToBytes(Path path) throws IOException { - try (InputStream is = Files.newInputStream(path)) { - if (is == null) { - throw new FileNotFoundException("Resource [" + path + "] not found in classpath"); - } - try (BytesStreamOutput out = new BytesStreamOutput()) { - copy(is, out); - return out.bytes().toBytes(); - } + try (InputStream is = Files.newInputStream(path); + BytesStreamOutput out = new BytesStreamOutput()) { + copy(is, out); + return out.bytes().toBytes(); } } @@ -177,7 +173,7 @@ public class StandaloneRunner extends CliTool { } - public static void main(String[] args) { + public static void main(String[] args) throws Exception { StandaloneRunner pluginManager = new StandaloneRunner(); pluginManager.execute(args); } diff --git a/qa/evil-tests/src/test/java/org/elasticsearch/bootstrap/BootstrapCliParserTests.java b/qa/evil-tests/src/test/java/org/elasticsearch/bootstrap/BootstrapCliParserTests.java index 78085b201a3..92c9df1845d 100644 --- a/qa/evil-tests/src/test/java/org/elasticsearch/bootstrap/BootstrapCliParserTests.java +++ b/qa/evil-tests/src/test/java/org/elasticsearch/bootstrap/BootstrapCliParserTests.java @@ -24,6 +24,7 @@ import org.elasticsearch.Version; import org.elasticsearch.common.SuppressForbidden; import org.elasticsearch.common.cli.CliTool.ExitStatus; import org.elasticsearch.common.cli.CliToolTestCase; +import org.elasticsearch.common.cli.UserError; import org.elasticsearch.common.collect.Tuple; import org.elasticsearch.monitor.jvm.JvmInfo; import org.hamcrest.Matcher; @@ -167,7 +168,7 @@ public class BootstrapCliParserTests extends CliToolTestCase { assertThatTerminalOutput(containsString("Parameter [network.host] needs value")); } - public void testParsingErrors() { + public void testParsingErrors() throws Exception { BootstrapCLIParser parser = new BootstrapCLIParser(terminal); // unknown params @@ -229,12 +230,10 @@ public class BootstrapCliParserTests extends CliToolTestCase { public void testThatHelpfulErrorMessageIsGivenWhenParametersAreOutOfOrder() throws Exception { BootstrapCLIParser parser = new BootstrapCLIParser(terminal); - try { - parser.parse("start", new String[]{"--foo=bar", "-Dbaz=qux"}); - fail("expected IllegalArgumentException for out-of-order parameters"); - } catch (IllegalArgumentException e) { - assertThat(e.getMessage(), containsString("must be before any parameters starting with --")); - } + UserError e = expectThrows(UserError.class, () -> { + parser.parse("start", new String[]{"--foo=bar", "-Dbaz=qux"}); + }); + assertThat(e.getMessage(), containsString("must be before any parameters starting with --")); } private void registerProperties(String ... systemProperties) { diff --git a/qa/evil-tests/src/test/java/org/elasticsearch/common/cli/CliToolTests.java b/qa/evil-tests/src/test/java/org/elasticsearch/common/cli/CliToolTests.java index 92ab945dfca..d5b494d9849 100644 --- a/qa/evil-tests/src/test/java/org/elasticsearch/common/cli/CliToolTests.java +++ b/qa/evil-tests/src/test/java/org/elasticsearch/common/cli/CliToolTests.java @@ -71,9 +71,9 @@ public class CliToolTests extends CliToolTestCase { final AtomicReference executed = new AtomicReference<>(false); final NamedCommand cmd = new NamedCommand("cmd", terminal) { @Override - public CliTool.ExitStatus execute(Settings settings, Environment env) { + public CliTool.ExitStatus execute(Settings settings, Environment env) throws UserError { executed.set(true); - return CliTool.ExitStatus.USAGE; + throw new UserError(CliTool.ExitStatus.USAGE, "bad usage"); } }; SingleCmdTool tool = new SingleCmdTool("tool", terminal, cmd); @@ -82,39 +82,7 @@ public class CliToolTests extends CliToolTestCase { assertCommandHasBeenExecuted(executed); } - public void testIOError() throws Exception { - Terminal terminal = new MockTerminal(); - final AtomicReference executed = new AtomicReference<>(false); - final NamedCommand cmd = new NamedCommand("cmd", terminal) { - @Override - public CliTool.ExitStatus execute(Settings settings, Environment env) throws Exception { - executed.set(true); - throw new IOException("io error"); - } - }; - SingleCmdTool tool = new SingleCmdTool("tool", terminal, cmd); - CliTool.ExitStatus status = tool.execute(); - assertStatus(status, CliTool.ExitStatus.IO_ERROR); - assertCommandHasBeenExecuted(executed); - } - - public void testCodeError() throws Exception { - Terminal terminal = new MockTerminal(); - final AtomicReference executed = new AtomicReference<>(false); - final NamedCommand cmd = new NamedCommand("cmd", terminal) { - @Override - public CliTool.ExitStatus execute(Settings settings, Environment env) throws Exception { - executed.set(true); - throw new Exception("random error"); - } - }; - SingleCmdTool tool = new SingleCmdTool("tool", terminal, cmd); - CliTool.ExitStatus status = tool.execute(); - assertStatus(status, CliTool.ExitStatus.CODE_ERROR); - assertCommandHasBeenExecuted(executed); - } - - public void testMultiCommand() { + public void testMultiCommand() throws Exception { Terminal terminal = new MockTerminal(); int count = randomIntBetween(2, 7); List> executed = new ArrayList<>(count); @@ -141,7 +109,7 @@ public class CliToolTests extends CliToolTestCase { } } - public void testMultiCommandUnknownCommand() { + public void testMultiCommandUnknownCommand() throws Exception { Terminal terminal = new MockTerminal(); int count = randomIntBetween(2, 7); List> executed = new ArrayList<>(count); @@ -184,7 +152,7 @@ public class CliToolTests extends CliToolTestCase { assertThat(terminal.getTerminalOutput(), hasItem(containsString("cmd1 help"))); } - public void testMultiCommandToolHelp() { + public void testMultiCommandToolHelp() throws Exception { CaptureOutputTerminal terminal = new CaptureOutputTerminal(); NamedCommand[] cmds = new NamedCommand[2]; cmds[0] = new NamedCommand("cmd0", terminal) { @@ -206,7 +174,7 @@ public class CliToolTests extends CliToolTestCase { assertThat(terminal.getTerminalOutput(), hasItem(containsString("tool help"))); } - public void testMultiCommandCmdHelp() { + public void testMultiCommandCmdHelp() throws Exception { CaptureOutputTerminal terminal = new CaptureOutputTerminal(); NamedCommand[] cmds = new NamedCommand[2]; cmds[0] = new NamedCommand("cmd0", terminal) { @@ -228,31 +196,19 @@ public class CliToolTests extends CliToolTestCase { assertThat(terminal.getTerminalOutput(), hasItem(containsString("cmd1 help"))); } - public void testThatThrowExceptionCanBeLogged() throws Exception { + public void testNonUserErrorPropagates() throws Exception { CaptureOutputTerminal terminal = new CaptureOutputTerminal(); NamedCommand cmd = new NamedCommand("cmd", terminal) { @Override public CliTool.ExitStatus execute(Settings settings, Environment env) throws Exception { - throw new ElasticsearchException("error message"); + throw new IOException("error message"); } }; SingleCmdTool tool = new SingleCmdTool("tool", terminal, cmd); - assertStatus(tool.execute(), CliTool.ExitStatus.CODE_ERROR); - assertThat(terminal.getTerminalOutput(), hasSize(1)); - assertThat(terminal.getTerminalOutput(), hasItem(containsString("error message"))); - - // set env... and log stack trace - try { - System.setProperty(Terminal.DEBUG_SYSTEM_PROPERTY, "true"); - terminal = new CaptureOutputTerminal(); - assertStatus(new SingleCmdTool("tool", terminal, cmd).execute(), CliTool.ExitStatus.CODE_ERROR); - assertThat(terminal.getTerminalOutput(), hasSize(2)); - assertThat(terminal.getTerminalOutput(), hasItem(containsString("error message"))); - // This class must be part of the stack strace - assertThat(terminal.getTerminalOutput(), hasItem(containsString(getClass().getName()))); - } finally { - System.clearProperty(Terminal.DEBUG_SYSTEM_PROPERTY); - } + IOException e = expectThrows(IOException.class, () -> { + tool.execute(); + }); + assertEquals("error message", e.getMessage()); } public void testMultipleLaunch() throws Exception { diff --git a/qa/evil-tests/src/test/java/org/elasticsearch/plugins/InstallPluginCommandTests.java b/qa/evil-tests/src/test/java/org/elasticsearch/plugins/InstallPluginCommandTests.java index 2905f60598d..b61ba3c13ef 100644 --- a/qa/evil-tests/src/test/java/org/elasticsearch/plugins/InstallPluginCommandTests.java +++ b/qa/evil-tests/src/test/java/org/elasticsearch/plugins/InstallPluginCommandTests.java @@ -19,8 +19,8 @@ package org.elasticsearch.plugins; -import java.io.File; import java.io.IOException; +import java.net.URL; import java.nio.charset.StandardCharsets; import java.nio.file.DirectoryStream; import java.nio.file.FileAlreadyExistsException; @@ -28,6 +28,7 @@ import java.nio.file.FileVisitResult; import java.nio.file.Files; import java.nio.file.Path; import java.nio.file.SimpleFileVisitor; +import java.nio.file.StandardCopyOption; import java.nio.file.attribute.BasicFileAttributes; import java.nio.file.attribute.PosixFileAttributeView; import java.nio.file.attribute.PosixFileAttributes; @@ -43,6 +44,7 @@ import org.elasticsearch.Version; import org.elasticsearch.common.cli.CliTool; import org.elasticsearch.common.cli.CliToolTestCase; import org.elasticsearch.common.cli.Terminal; +import org.elasticsearch.common.cli.UserError; import org.elasticsearch.common.settings.Settings; import org.elasticsearch.env.Environment; import org.elasticsearch.test.ESTestCase; @@ -193,6 +195,16 @@ public class InstallPluginCommandTests extends ESTestCase { assertPlugin("fake", pluginDir, env); } + public void testSpaceInUrl() throws Exception { + Environment env = createEnv(); + Path pluginDir = createTempDir(); + String pluginZip = createPlugin("fake", pluginDir); + Path pluginZipWithSpaces = createTempFile("foo bar", ".zip"); + Files.copy(new URL(pluginZip).openStream(), pluginZipWithSpaces, StandardCopyOption.REPLACE_EXISTING); + installPlugin(pluginZipWithSpaces.toUri().toURL().toString(), env); + assertPlugin("fake", pluginDir, env); + } + public void testPluginsDirMissing() throws Exception { Environment env = createEnv(); Files.delete(env.pluginsFile()); @@ -211,7 +223,7 @@ public class InstallPluginCommandTests extends ESTestCase { IOException e = expectThrows(IOException.class, () -> { installPlugin(pluginZip, env); }); - assertTrue(e.getMessage(), e.getMessage().contains("Plugins directory is read only")); + assertTrue(e.getMessage(), e.getMessage().contains(env.pluginsFile().toString())); } assertInstallCleaned(env); } @@ -219,7 +231,7 @@ public class InstallPluginCommandTests extends ESTestCase { public void testBuiltinModule() throws Exception { Environment env = createEnv(); String pluginZip = createPlugin("lang-groovy", createTempDir()); - IOException e = expectThrows(IOException.class, () -> { + UserError e = expectThrows(UserError.class, () -> { installPlugin(pluginZip, env); }); assertTrue(e.getMessage(), e.getMessage().contains("is a system module")); @@ -288,7 +300,7 @@ public class InstallPluginCommandTests extends ESTestCase { Environment env = createEnv(); String pluginZip = createPlugin("fake", createTempDir()); installPlugin(pluginZip, env); - IOException e = expectThrows(IOException.class, () -> { + UserError e = expectThrows(UserError.class, () -> { installPlugin(pluginZip, env); }); assertTrue(e.getMessage(), e.getMessage().contains("already exists")); @@ -312,7 +324,7 @@ public class InstallPluginCommandTests extends ESTestCase { Path binDir = pluginDir.resolve("bin"); Files.createFile(binDir); String pluginZip = createPlugin("fake", pluginDir); - IOException e = expectThrows(IOException.class, () -> { + UserError e = expectThrows(UserError.class, () -> { installPlugin(pluginZip, env); }); assertTrue(e.getMessage(), e.getMessage().contains("not a directory")); @@ -326,7 +338,7 @@ public class InstallPluginCommandTests extends ESTestCase { Files.createDirectories(dirInBinDir); Files.createFile(dirInBinDir.resolve("somescript")); String pluginZip = createPlugin("fake", pluginDir); - IOException e = expectThrows(IOException.class, () -> { + UserError e = expectThrows(UserError.class, () -> { installPlugin(pluginZip, env); }); assertTrue(e.getMessage(), e.getMessage().contains("Directories not allowed in bin dir for plugin")); @@ -401,7 +413,7 @@ public class InstallPluginCommandTests extends ESTestCase { Path configDir = pluginDir.resolve("config"); Files.createFile(configDir); String pluginZip = createPlugin("fake", pluginDir); - IOException e = expectThrows(IOException.class, () -> { + UserError e = expectThrows(UserError.class, () -> { installPlugin(pluginZip, env); }); assertTrue(e.getMessage(), e.getMessage().contains("not a directory")); @@ -415,7 +427,7 @@ public class InstallPluginCommandTests extends ESTestCase { Files.createDirectories(dirInConfigDir); Files.createFile(dirInConfigDir.resolve("myconfig.yml")); String pluginZip = createPlugin("fake", pluginDir); - IOException e = expectThrows(IOException.class, () -> { + UserError e = expectThrows(UserError.class, () -> { installPlugin(pluginZip, env); }); assertTrue(e.getMessage(), e.getMessage().contains("Directories not allowed in config dir for plugin")); diff --git a/qa/evil-tests/src/test/java/org/elasticsearch/plugins/RemovePluginCommandTests.java b/qa/evil-tests/src/test/java/org/elasticsearch/plugins/RemovePluginCommandTests.java index a643c835bd1..10fbc3c2696 100644 --- a/qa/evil-tests/src/test/java/org/elasticsearch/plugins/RemovePluginCommandTests.java +++ b/qa/evil-tests/src/test/java/org/elasticsearch/plugins/RemovePluginCommandTests.java @@ -28,6 +28,7 @@ import org.apache.lucene.util.LuceneTestCase; import org.elasticsearch.common.cli.CliTool; import org.elasticsearch.common.cli.CliToolTestCase; import org.elasticsearch.common.cli.Terminal; +import org.elasticsearch.common.cli.UserError; import org.elasticsearch.common.settings.Settings; import org.elasticsearch.env.Environment; import org.elasticsearch.test.ESTestCase; @@ -66,7 +67,7 @@ public class RemovePluginCommandTests extends ESTestCase { public void testMissing() throws Exception { Environment env = createEnv(); - IllegalArgumentException e = expectThrows(IllegalArgumentException.class, () -> { + UserError e = expectThrows(UserError.class, () -> { removePlugin("dne", env); }); assertTrue(e.getMessage(), e.getMessage().contains("Plugin dne not found")); @@ -101,9 +102,10 @@ public class RemovePluginCommandTests extends ESTestCase { public void testBinNotDir() throws Exception { Environment env = createEnv(); Files.createDirectories(env.pluginsFile().resolve("elasticsearch")); - IllegalStateException e = expectThrows(IllegalStateException.class, () -> { + UserError e = expectThrows(UserError.class, () -> { removePlugin("elasticsearch", env); }); + assertTrue(e.getMessage(), e.getMessage().contains("not a directory")); assertTrue(Files.exists(env.pluginsFile().resolve("elasticsearch"))); // did not remove assertTrue(Files.exists(env.binFile().resolve("elasticsearch"))); assertRemoveCleaned(env);