From 312eb2b8b61ae57dae386f529b9457c89ffb7de4 Mon Sep 17 00:00:00 2001 From: Chris Earle Date: Thu, 1 May 2014 23:47:11 -0400 Subject: [PATCH] PluginManager: tests for missing plugin name when passing --url Adding code to test for unset plugin names to fail fast with descriptive error messages. Also simplified the series of `if` statements checking for the commands by using a `switch` (now that it's using Java 7), added tests, and updated random exceptions with the up-to-date flag names (e.g., "--verbose" instead of "-verbose"). Closes #5976. Closes #6013. --- .../elasticsearch/plugins/PluginManager.java | 139 +++++++++++++----- .../plugin/PluginManagerTests.java | 56 ++++--- 2 files changed, 137 insertions(+), 58 deletions(-) diff --git a/src/main/java/org/elasticsearch/plugins/PluginManager.java b/src/main/java/org/elasticsearch/plugins/PluginManager.java index ab29b61956a..9dcb0ed1c1d 100644 --- a/src/main/java/org/elasticsearch/plugins/PluginManager.java +++ b/src/main/java/org/elasticsearch/plugins/PluginManager.java @@ -21,6 +21,7 @@ package org.elasticsearch.plugins; import com.google.common.base.Strings; import org.elasticsearch.ElasticsearchIllegalArgumentException; +import org.elasticsearch.ElasticsearchIllegalStateException; import org.elasticsearch.ElasticsearchTimeoutException; import org.elasticsearch.ExceptionsHelper; import org.elasticsearch.common.collect.Tuple; @@ -104,6 +105,9 @@ public class PluginManager { } public void downloadAndExtract(String name) throws IOException { + if (name == null) { + throw new ElasticsearchIllegalArgumentException("plugin name must be supplied with --install [name]."); + } HttpDownloadHelper downloadHelper = new HttpDownloadHelper(); boolean downloaded = false; HttpDownloadHelper.DownloadProgress progress; @@ -123,7 +127,7 @@ public class PluginManager { // extract the plugin File extractLocation = pluginHandle.extractedDir(environment); if (extractLocation.exists()) { - throw new IOException("plugin directory " + extractLocation.getAbsolutePath() + " already exists. To update the plugin, uninstall it first using -remove " + name + " command"); + throw new IOException("plugin directory " + extractLocation.getAbsolutePath() + " already exists. To update the plugin, uninstall it first using --remove " + name + " command"); } // first, try directly from the URL provided @@ -158,7 +162,7 @@ public class PluginManager { } if (!downloaded) { - throw new IOException("failed to download out of all possible locations..., use -verbose to get detailed information"); + throw new IOException("failed to download out of all possible locations..., use --verbose to get detailed information"); } ZipFile zipFile = null; @@ -231,6 +235,9 @@ public class PluginManager { } public void removePlugin(String name) throws IOException { + if (name == null) { + throw new ElasticsearchIllegalArgumentException("plugin name must be supplied with --remove [name]."); + } PluginHandle pluginHandle = PluginHandle.parse(name); boolean removed = false; @@ -329,38 +336,68 @@ public class PluginManager { try { for (int c = 0; c < args.length; c++) { String command = args[c]; - if ("-u".equals(command) || "--url".equals(command) - // Deprecated commands - || "url".equals(command) || "-url".equals(command)) { - url = args[++c]; - } else if ("-v".equals(command) || "--verbose".equals(command) - || "verbose".equals(command) || "-verbose".equals(command)) { - outputMode = OutputMode.VERBOSE; - } else if ("-s".equals(command) || "--silent".equals(command) - || "silent".equals(command) || "-silent".equals(command)) { - outputMode = OutputMode.SILENT; - } else if (command.equals("-i") || command.equals("--install") - // Deprecated commands - || command.equals("install") || command.equals("-install")) { - pluginName = args[++c]; - action = ACTION.INSTALL; - } else if (command.equals("-t") || command.equals("--timeout") - || command.equals("timeout") || command.equals("-timeout")) { - timeout = TimeValue.parseTimeValue(args[++c], DEFAULT_TIMEOUT); - } else if (command.equals("-r") || command.equals("--remove") - // Deprecated commands - || command.equals("remove") || command.equals("-remove")) { - pluginName = args[++c]; - - action = ACTION.REMOVE; - } else if (command.equals("-l") || command.equals("--list")) { - action = ACTION.LIST; - } else if (command.equals("-h") || command.equals("--help")) { - displayHelp(null); - } else { - displayHelp("Command [" + args[c] + "] unknown."); - // Unknown command. We break... - System.exit(EXIT_CODE_CMD_USAGE); + switch (command) { + case "-u": + case "--url": + // deprecated versions: + case "url": + case "-url": + url = getCommandValue(args, ++c, "--url"); + // Until update is supported, then supplying a URL implies installing + // By specifying this action, we also avoid silently failing without + // dubious checks. + action = ACTION.INSTALL; + break; + case "-v": + case "--verbose": + // deprecated versions: + case "verbose": + case "-verbose": + outputMode = OutputMode.VERBOSE; + break; + case "-s": + case "--silent": + // deprecated versions: + case "silent": + case "-silent": + outputMode = OutputMode.SILENT; + break; + case "-i": + case "--install": + // deprecated versions: + case "install": + case "-install": + pluginName = getCommandValue(args, ++c, "--install"); + action = ACTION.INSTALL; + break; + case "-r": + case "--remove": + // deprecated versions: + case "remove": + case "-remove": + pluginName = getCommandValue(args, ++c, "--remove"); + action = ACTION.REMOVE; + break; + case "-t": + case "--timeout": + // deprecated versions: + case "timeout": + case "-timeout": + String timeoutValue = getCommandValue(args, ++c, "--timeout"); + timeout = TimeValue.parseTimeValue(timeoutValue, DEFAULT_TIMEOUT); + break; + case "-l": + case "--list": + action = ACTION.LIST; + break; + case "-h": + case "--help": + displayHelp(null); + break; + default: + displayHelp("Command [" + command + "] unknown."); + // Unknown command. We break... + System.exit(EXIT_CODE_CMD_USAGE); } } } catch (Throwable e) { @@ -375,7 +412,7 @@ public class PluginManager { switch (action) { case ACTION.INSTALL: try { - pluginManager.log("-> Installing " + pluginName + "..."); + pluginManager.log("-> Installing " + Strings.nullToEmpty(pluginName) + "..."); pluginManager.downloadAndExtract(pluginName); exitCode = EXIT_CODE_OK; } catch (IOException e) { @@ -389,7 +426,7 @@ public class PluginManager { break; case ACTION.REMOVE: try { - pluginManager.log("-> Removing " + pluginName + " "); + pluginManager.log("-> Removing " + Strings.nullToEmpty(pluginName) + "..."); pluginManager.removePlugin(pluginName); exitCode = EXIT_CODE_OK; } catch (ElasticsearchIllegalArgumentException e) { @@ -423,6 +460,36 @@ public class PluginManager { } } + /** + * Get the value for the {@code flag} at the specified {@code arg} of the command line {@code args}. + *

+ * This is useful to avoid having to check for multiple forms of unset (e.g., " " versus "" versus {@code null}). + * @param args Incoming command line arguments. + * @param arg Expected argument containing the value. + * @param flag The flag whose value is being retrieved. + * @return Never {@code null}. The trimmed value. + * @throws NullPointerException if {@code args} is {@code null}. + * @throws ArrayIndexOutOfBoundsException if {@code arg} is negative. + * @throws ElasticsearchIllegalStateException if {@code arg} is >= {@code args.length}. + * @throws ElasticsearchIllegalArgumentException if the value evaluates to blank ({@code null} or only whitespace) + */ + private static String getCommandValue(String[] args, int arg, String flag) { + if (arg >= args.length) { + throw new ElasticsearchIllegalStateException("missing value for " + flag + ". Usage: " + flag + " [value]"); + } + + // avoid having to interpret multiple forms of unset + String trimmedValue = Strings.emptyToNull(args[arg].trim()); + + // If we had a value that is blank, then fail immediately + if (trimmedValue == null) { + throw new ElasticsearchIllegalArgumentException( + "value for " + flag + "('" + args[arg] + "') must be set. Usage: " + flag + " [value]"); + } + + return trimmedValue; + } + private static void displayHelp(String message) { System.out.println("Usage:"); System.out.println(" -u, --url [plugin location] : Set exact URL to download the plugin from"); diff --git a/src/test/java/org/elasticsearch/plugin/PluginManagerTests.java b/src/test/java/org/elasticsearch/plugin/PluginManagerTests.java index 993cb3c6d12..c52e97f874b 100644 --- a/src/test/java/org/elasticsearch/plugin/PluginManagerTests.java +++ b/src/test/java/org/elasticsearch/plugin/PluginManagerTests.java @@ -71,12 +71,16 @@ public class PluginManagerTests extends ElasticsearchIntegrationTest { deletePluginsFolder(); } + @Test(expected = ElasticsearchIllegalArgumentException.class) + public void testDownloadAndExtract_NullName_ThrowsException() throws IOException { + pluginManager(getPluginUrlForResource("plugin_single_folder.zip")).downloadAndExtract(null); + } + @Test public void testLocalPluginInstallSingleFolder() throws Exception { //When we have only a folder in top-level (no files either) we remove that folder while extracting String pluginName = "plugin-test"; - URI uri = URI.create(PluginManagerTests.class.getResource("plugin_single_folder.zip").toString()); - downloadAndExtract(pluginName, "file://" + uri.getPath()); + downloadAndExtract(pluginName, getPluginUrlForResource("plugin_single_folder.zip")); internalCluster().startNode(SETTINGS); @@ -89,10 +93,9 @@ public class PluginManagerTests extends ElasticsearchIntegrationTest { //When we have only a folder in top-level (no files either) but it's called _site, we make it work //we can either remove the folder while extracting and then re-add it manually or just leave it as it is String pluginName = "plugin-test"; - URI uri = URI.create(PluginManagerTests.class.getResource("plugin_folder_site.zip").toString()); - downloadAndExtract(pluginName, "file://" + uri.getPath()); + downloadAndExtract(pluginName, getPluginUrlForResource("plugin_folder_site.zip")); - String nodeName = internalCluster().startNode(SETTINGS); + internalCluster().startNode(SETTINGS); assertPluginLoaded(pluginName); assertPluginAvailable(pluginName); @@ -102,8 +105,7 @@ public class PluginManagerTests extends ElasticsearchIntegrationTest { public void testLocalPluginWithoutFolders() throws Exception { //When we don't have folders at all in the top-level, but only files, we don't modify anything String pluginName = "plugin-test"; - URI uri = URI.create(PluginManagerTests.class.getResource("plugin_without_folders.zip").toString()); - downloadAndExtract(pluginName, "file://" + uri.getPath()); + downloadAndExtract(pluginName, getPluginUrlForResource("plugin_without_folders.zip")); internalCluster().startNode(SETTINGS); @@ -115,8 +117,7 @@ public class PluginManagerTests extends ElasticsearchIntegrationTest { public void testLocalPluginFolderAndFile() throws Exception { //When we have a single top-level folder but also files in the top-level, we don't modify anything String pluginName = "plugin-test"; - URI uri = URI.create(PluginManagerTests.class.getResource("plugin_folder_file.zip").toString()); - downloadAndExtract(pluginName, "file://" + uri.getPath()); + downloadAndExtract(pluginName, getPluginUrlForResource("plugin_folder_file.zip")); internalCluster().startNode(SETTINGS); @@ -127,8 +128,7 @@ public class PluginManagerTests extends ElasticsearchIntegrationTest { @Test(expected = IllegalArgumentException.class) public void testSitePluginWithSourceThrows() throws Exception { String pluginName = "plugin-with-source"; - URI uri = URI.create(PluginManagerTests.class.getResource("plugin_with_sourcefiles.zip").toString()); - downloadAndExtract(pluginName, "file://" + uri.getPath()); + downloadAndExtract(pluginName, getPluginUrlForResource("plugin_with_sourcefiles.zip")); } /** @@ -202,14 +202,13 @@ public class PluginManagerTests extends ElasticsearchIntegrationTest { @Test(expected = IOException.class) public void testInstallPluginNull() throws IOException { - pluginManager(null).downloadAndExtract(""); + pluginManager(null).downloadAndExtract("plugin-test"); } @Test public void testInstallPlugin() throws IOException { - PluginManager pluginManager = pluginManager("file://".concat( - URI.create(PluginManagerTests.class.getResource("plugin_with_classfile.zip").toString()).getPath())); + PluginManager pluginManager = pluginManager(getPluginUrlForResource("plugin_with_classfile.zip")); pluginManager.downloadAndExtract("plugin"); File[] plugins = pluginManager.getListInstalledPlugins(); @@ -219,8 +218,7 @@ public class PluginManagerTests extends ElasticsearchIntegrationTest { @Test public void testInstallSitePlugin() throws IOException { - PluginManager pluginManager = pluginManager("file://".concat( - URI.create(PluginManagerTests.class.getResource("plugin_without_folders.zip").toString()).getPath())); + PluginManager pluginManager = pluginManager(getPluginUrlForResource("plugin_without_folders.zip")); pluginManager.downloadAndExtract("plugin-site"); File[] plugins = pluginManager.getListInstalledPlugins(); @@ -314,16 +312,18 @@ public class PluginManagerTests extends ElasticsearchIntegrationTest { @Test public void testRemovePlugin() throws Exception { // We want to remove plugin with plugin short name - singlePluginInstallAndRemove("plugintest", "file://".concat( - URI.create(PluginManagerTests.class.getResource("plugin_without_folders.zip").toString()).getPath())); + singlePluginInstallAndRemove("plugintest", getPluginUrlForResource("plugin_without_folders.zip")); // We want to remove plugin with groupid/artifactid/version form - singlePluginInstallAndRemove("groupid/plugintest/1.0.0", "file://".concat( - URI.create(PluginManagerTests.class.getResource("plugin_without_folders.zip").toString()).getPath())); + singlePluginInstallAndRemove("groupid/plugintest/1.0.0", getPluginUrlForResource("plugin_without_folders.zip")); // We want to remove plugin with groupid/artifactid form - singlePluginInstallAndRemove("groupid/plugintest", "file://".concat( - URI.create(PluginManagerTests.class.getResource("plugin_without_folders.zip").toString()).getPath())); + singlePluginInstallAndRemove("groupid/plugintest", getPluginUrlForResource("plugin_without_folders.zip")); + } + + @Test(expected = ElasticsearchIllegalArgumentException.class) + public void testRemovePlugin_NullName_ThrowsException() throws IOException { + pluginManager(getPluginUrlForResource("plugin_single_folder.zip")).removePlugin(null); } @Test(expected = ElasticsearchIllegalArgumentException.class) @@ -331,4 +331,16 @@ public class PluginManagerTests extends ElasticsearchIntegrationTest { PluginManager pluginManager = pluginManager(null); pluginManager.removePlugin("file://whatever"); } + + /** + * Retrieve a URL string that represents the resource with the given {@code resourceName}. + * @param resourceName The resource name relative to {@link PluginManagerTests}. + * @return Never {@code null}. + * @throws NullPointerException if {@code resourceName} does not point to a valid resource. + */ + private String getPluginUrlForResource(String resourceName) { + URI uri = URI.create(PluginManagerTests.class.getResource(resourceName).toString()); + + return "file://" + uri.getPath(); + } }