Provide helpful error message if a plugin exists

Today if an older version of a plugin exists, we fail to notify the user
with a helpful error message. This happens because during plugin
verification, we attempt to read the plugin descriptors for all existing
plugins. When an older version of a plugin is sitting on disk, we will
attempt to read this old plugin descriptor and fail due to a version
mismatch. This leads to an unhelpful error message. Instead, we should
check for existence of the plugin as part of the verification phase, but
before attempting to read plugin descriptors for existing plugins. This
enables us to provide a helpful error message to the user.

Relates #22305
This commit is contained in:
Jason Tedor 2016-12-21 22:37:07 -05:00 committed by GitHub
parent 8aca504c86
commit 91cb563247
2 changed files with 25 additions and 10 deletions

View File

@ -418,6 +418,18 @@ class InstallPluginCommand extends EnvironmentAwareCommand {
private PluginInfo verify(Terminal terminal, Path pluginRoot, boolean isBatch, Environment env) throws Exception { private PluginInfo verify(Terminal terminal, Path pluginRoot, boolean isBatch, Environment env) throws Exception {
// read and validate the plugin descriptor // read and validate the plugin descriptor
PluginInfo info = PluginInfo.readFromProperties(pluginRoot); PluginInfo info = PluginInfo.readFromProperties(pluginRoot);
// checking for existing version of the plugin
final Path destination = env.pluginsFile().resolve(info.getName());
if (Files.exists(destination)) {
final String message = String.format(
Locale.ROOT,
"plugin directory [%s] already exists; if you need to update the plugin, uninstall it first using command 'remove %s'",
destination.toAbsolutePath(),
info.getName());
throw new UserException(ExitCodes.CONFIG, message);
}
terminal.println(VERBOSE, info.toString()); terminal.println(VERBOSE, info.toString());
// don't let user install plugin as a module... // don't let user install plugin as a module...
@ -471,14 +483,7 @@ class InstallPluginCommand extends EnvironmentAwareCommand {
try { try {
PluginInfo info = verify(terminal, tmpRoot, isBatch, env); PluginInfo info = verify(terminal, tmpRoot, isBatch, env);
final Path destination = env.pluginsFile().resolve(info.getName()); final Path destination = env.pluginsFile().resolve(info.getName());
if (Files.exists(destination)) {
throw new UserException(
ExitCodes.USAGE,
"plugin directory " + destination.toAbsolutePath() +
" already exists. To update the plugin, uninstall it first using 'remove " + info.getName() + "' command");
}
Path tmpBinDir = tmpRoot.resolve("bin"); Path tmpBinDir = tmpRoot.resolve("bin");
if (Files.exists(tmpBinDir)) { if (Files.exists(tmpBinDir)) {

View File

@ -60,19 +60,17 @@ import java.nio.file.attribute.PosixFileAttributes;
import java.nio.file.attribute.PosixFilePermission; import java.nio.file.attribute.PosixFilePermission;
import java.nio.file.attribute.UserPrincipal; import java.nio.file.attribute.UserPrincipal;
import java.util.ArrayList; import java.util.ArrayList;
import java.util.HashMap;
import java.util.HashSet; import java.util.HashSet;
import java.util.List; import java.util.List;
import java.util.Map;
import java.util.Set; import java.util.Set;
import java.util.function.Function; import java.util.function.Function;
import java.util.stream.Collectors; import java.util.stream.Collectors;
import java.util.zip.ZipEntry; import java.util.zip.ZipEntry;
import java.util.zip.ZipOutputStream; import java.util.zip.ZipOutputStream;
import static org.hamcrest.CoreMatchers.containsString;
import static org.hamcrest.CoreMatchers.equalTo; import static org.hamcrest.CoreMatchers.equalTo;
import static org.hamcrest.Matchers.containsInAnyOrder; import static org.hamcrest.Matchers.containsInAnyOrder;
import static org.hamcrest.Matchers.containsString;
import static org.hamcrest.Matchers.not; import static org.hamcrest.Matchers.not;
@LuceneTestCase.SuppressFileSystems("*") @LuceneTestCase.SuppressFileSystems("*")
@ -673,6 +671,18 @@ public class InstallPluginCommandTests extends ESTestCase {
assertThat(terminal.getOutput(), not(containsString("100%"))); assertThat(terminal.getOutput(), not(containsString("100%")));
} }
public void testPluginAlreadyInstalled() throws Exception {
Tuple<Path, Environment> env = createEnv(fs, temp);
Path pluginDir = createPluginDir(temp);
String pluginZip = createPlugin("fake", pluginDir);
installPlugin(pluginZip, env.v1());
final UserException e = expectThrows(UserException.class, () -> installPlugin(pluginZip, env.v1(), randomBoolean()));
assertThat(
e.getMessage(),
equalTo("plugin directory [" + env.v2().pluginsFile().resolve("fake") + "] already exists; " +
"if you need to update the plugin, uninstall it first using command 'remove fake'"));
}
private void installPlugin(MockTerminal terminal, boolean isBatch) throws Exception { private void installPlugin(MockTerminal terminal, boolean isBatch) throws Exception {
Tuple<Path, Environment> env = createEnv(fs, temp); Tuple<Path, Environment> env = createEnv(fs, temp);
Path pluginDir = createPluginDir(temp); Path pluginDir = createPluginDir(temp);