Skip to content

Commit

Permalink
PluginManager: tests for missing plugin name when passing --url
Browse files Browse the repository at this point in the history 
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.
  • Loading branch information
@pickypg @dadoonet
pickypg authored and dadoonet committed Jul 4, 2014
1 parent 04c5731 commit 312eb2b
Show file tree
Hide file tree
Showing 2 changed files with 137 additions and 58 deletions.
139 changes: 103 additions & 36 deletions src/main/java/org/elasticsearch/plugins/PluginManager.java
View file
Expand Up @@ -21,6 +21,7 @@

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;
Expand Down Expand Up @@ -104,6 +105,9 @@ public void checkServerTrusted(
}

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;
Expand All @@ -123,7 +127,7 @@ public void downloadAndExtract(String name) throws IOException {
// 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
Expand Down Expand Up @@ -158,7 +162,7 @@ public void downloadAndExtract(String name) throws IOException {
}

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;
Expand Down Expand Up @@ -231,6 +235,9 @@ public void downloadAndExtract(String name) throws IOException {
}

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;

Expand Down Expand Up @@ -329,38 +336,68 @@ public static void main(String[] args) {
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) {
Expand All @@ -375,7 +412,7 @@ public static void main(String[] args) {
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) {
Expand All @@ -389,7 +426,7 @@ public static void main(String[] args) {
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) {
Expand Down Expand Up @@ -423,6 +460,36 @@ public static void main(String[] args) {
}
}

/**
* Get the value for the {@code flag} at the specified {@code arg} of the command line {@code args}.
* <p />
* 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 &gt;= {@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");
Expand Down
56 changes: 34 additions & 22 deletions src/test/java/org/elasticsearch/plugin/PluginManagerTests.java
View file
Expand Up @@ -71,12 +71,16 @@ public void beforeTest() {
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);

Expand All @@ -89,10 +93,9 @@ public void testLocalPluginInstallSiteFolder() throws Exception {
//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);
Expand All @@ -102,8 +105,7 @@ public void testLocalPluginInstallSiteFolder() throws Exception {
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);

Expand All @@ -115,8 +117,7 @@ public void testLocalPluginWithoutFolders() throws Exception {
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);

Expand All @@ -127,8 +128,7 @@ public void testLocalPluginFolderAndFile() throws Exception {
@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"));
}

/**
Expand Down Expand Up @@ -202,14 +202,13 @@ public void testListInstalledEmpty() throws IOException {

@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();
Expand All @@ -219,8 +218,7 @@ public void testInstallPlugin() throws IOException {

@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();
Expand Down Expand Up @@ -314,21 +312,35 @@ private void deletePluginsFolder() {
@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)
public void testRemovePluginWithURLForm() throws Exception {
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();
}
}

0 comments on commit 312eb2b

Please sign in to comment.