From b80200a363c479d00b6f38fb89d9009abcac3a1a Mon Sep 17 00:00:00 2001 From: Ryan Ernst Date: Wed, 3 Feb 2016 22:22:56 -0800 Subject: [PATCH] CliTool: Cleanup and document Terminal This change documents the Terminal abstraction that cli tools use, as well as simplifies the api to be a minimal set of methods to interact with a terminal. --- .../common/cli/CheckFileCommand.java | 8 +- .../org/elasticsearch/common/cli/CliTool.java | 18 ++- .../elasticsearch/common/cli/HelpPrinter.java | 4 +- .../elasticsearch/common/cli/Terminal.java | 132 ++++++------------ .../elasticsearch/plugins/PluginSecurity.java | 4 +- .../common/cli/TerminalTests.java | 4 +- .../common/cli/CliToolTestCase.java | 32 +---- 7 files changed, 65 insertions(+), 137 deletions(-) diff --git a/core/src/main/java/org/elasticsearch/common/cli/CheckFileCommand.java b/core/src/main/java/org/elasticsearch/common/cli/CheckFileCommand.java index 95502f16700..e2fcbe89df8 100644 --- a/core/src/main/java/org/elasticsearch/common/cli/CheckFileCommand.java +++ b/core/src/main/java/org/elasticsearch/common/cli/CheckFileCommand.java @@ -100,10 +100,10 @@ public abstract class CheckFileCommand extends CliTool.Command { Set permissionsBeforeWrite = entry.getValue(); Set permissionsAfterWrite = Files.getPosixFilePermissions(entry.getKey()); if (!permissionsBeforeWrite.equals(permissionsAfterWrite)) { - terminal.printWarn("The file permissions of [" + entry.getKey() + "] have changed " + terminal.println(Terminal.Verbosity.SILENT, "WARNING: The file permissions of [" + entry.getKey() + "] have changed " + "from [" + PosixFilePermissions.toString(permissionsBeforeWrite) + "] " + "to [" + PosixFilePermissions.toString(permissionsAfterWrite) + "]"); - terminal.printWarn("Please ensure that the user account running Elasticsearch has read access to this file!"); + terminal.println(Terminal.Verbosity.SILENT, "Please ensure that the user account running Elasticsearch has read access to this file!"); } } @@ -116,7 +116,7 @@ public abstract class CheckFileCommand extends CliTool.Command { String ownerBeforeWrite = entry.getValue(); String ownerAfterWrite = Files.getOwner(entry.getKey()).getName(); if (!ownerAfterWrite.equals(ownerBeforeWrite)) { - terminal.printWarn("WARN: Owner of file [" + entry.getKey() + "] used to be [" + ownerBeforeWrite + "], but now is [" + ownerAfterWrite + "]"); + terminal.println(Terminal.Verbosity.SILENT, "WARNING: Owner of file [" + entry.getKey() + "] used to be [" + ownerBeforeWrite + "], but now is [" + ownerAfterWrite + "]"); } } @@ -129,7 +129,7 @@ public abstract class CheckFileCommand extends CliTool.Command { String groupBeforeWrite = entry.getValue(); String groupAfterWrite = Files.readAttributes(entry.getKey(), PosixFileAttributes.class).group().getName(); if (!groupAfterWrite.equals(groupBeforeWrite)) { - terminal.printWarn("WARN: Group of file [" + entry.getKey() + "] used to be [" + groupBeforeWrite + "], but now is [" + groupAfterWrite + "]"); + terminal.println(Terminal.Verbosity.SILENT, "WARNING: Group of file [" + entry.getKey() + "] used to be [" + groupBeforeWrite + "], but now is [" + groupAfterWrite + "]"); } } 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 17994eba439..2ea01f45068 100644 --- a/core/src/main/java/org/elasticsearch/common/cli/CliTool.java +++ b/core/src/main/java/org/elasticsearch/common/cli/CliTool.java @@ -117,7 +117,7 @@ public abstract class CliTool { } else { if (args.length == 0) { - terminal.printError("command not specified"); + terminal.println(Terminal.Verbosity.SILENT, "ERROR: command not specified"); config.printUsage(terminal); return ExitStatus.USAGE; } @@ -125,7 +125,7 @@ public abstract class CliTool { String cmdName = args[0]; cmd = config.cmd(cmdName); if (cmd == null) { - terminal.printError("unknown command [" + cmdName + "]. Use [-h] option to list available commands"); + terminal.println(Terminal.Verbosity.SILENT, "ERROR: unknown command [" + cmdName + "]. Use [-h] option to list available commands"); return ExitStatus.USAGE; } @@ -142,7 +142,7 @@ public abstract class CliTool { try { return parse(cmd, args).execute(settings, env); } catch (UserError error) { - terminal.printError(error.getMessage()); + terminal.println(Terminal.Verbosity.SILENT, "ERROR: " + error.getMessage()); return error.exitStatus; } } @@ -165,8 +165,14 @@ public abstract class CliTool { // 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); + + if (cli.hasOption("v")) { + terminal.setVerbosity(Terminal.Verbosity.VERBOSE); + } else if (cli.hasOption("s")) { + terminal.setVerbosity(Terminal.Verbosity.SILENT); + } else { + terminal.setVerbosity(Terminal.Verbosity.NORMAL); + } return parse(cmd.name(), cli); } @@ -224,7 +230,7 @@ public abstract class CliTool { public ExitStatus execute(Settings settings, Environment env) throws Exception { if (msg != null) { if (status != ExitStatus.OK) { - terminal.printError(msg); + terminal.println(Terminal.Verbosity.SILENT, "ERROR: " + msg); } else { terminal.println(msg); } diff --git a/core/src/main/java/org/elasticsearch/common/cli/HelpPrinter.java b/core/src/main/java/org/elasticsearch/common/cli/HelpPrinter.java index 5a463258eb1..9be2e676831 100644 --- a/core/src/main/java/org/elasticsearch/common/cli/HelpPrinter.java +++ b/core/src/main/java/org/elasticsearch/common/cli/HelpPrinter.java @@ -41,7 +41,7 @@ public class HelpPrinter { } private static void print(Class clazz, String name, final Terminal terminal) { - terminal.println(Terminal.Verbosity.SILENT); + terminal.println(Terminal.Verbosity.SILENT, ""); try (InputStream input = clazz.getResourceAsStream(name + HELP_FILE_EXT)) { Streams.readAllLines(input, new Callback() { @Override @@ -52,6 +52,6 @@ public class HelpPrinter { } catch (IOException ioe) { throw new RuntimeException(ioe); } - terminal.println(); + terminal.println(""); } } 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 68229f69c33..9af832292d1 100644 --- a/core/src/main/java/org/elasticsearch/common/cli/Terminal.java +++ b/core/src/main/java/org/elasticsearch/common/cli/Terminal.java @@ -19,114 +19,73 @@ package org.elasticsearch.common.cli; -import org.apache.commons.cli.CommandLine; -import org.elasticsearch.common.SuppressForbidden; - import java.io.BufferedReader; import java.io.Console; import java.io.IOException; +import java.io.InputStream; import java.io.InputStreamReader; -import java.io.PrintWriter; -import java.util.Locale; +import java.io.PrintStream; +import java.nio.charset.Charset; + +import org.elasticsearch.common.SuppressForbidden; /** -* + * A Terminal wraps access to reading input and writing output for a {@link CliTool}. + * + * The available methods are similar to those of {@link Console}, with the ability + * to read either normal text or a password, and the ability to print a line + * of text. Printing is also gated by the {@link Verbosity} of the terminal, + * which allows {@link #println(Verbosity,String)} calls which act like a logger, + * only actually printing if the verbosity level of the terminal is above + * the verbosity of the message. */ -@SuppressForbidden(reason = "System#out") public abstract class Terminal { - public static final Terminal DEFAULT = ConsoleTerminal.supported() ? new ConsoleTerminal() : new SystemTerminal(); + /** The default terminal implementation, which will be a console if available, or stdout/stderr if not. */ + public static final Terminal DEFAULT = ConsoleTerminal.isSupported() ? new ConsoleTerminal() : new SystemTerminal(); - public static enum Verbosity { - SILENT(0), NORMAL(1), VERBOSE(2); - - private final int level; - - private Verbosity(int level) { - this.level = level; - } - - public boolean enabled(Verbosity verbosity) { - return level >= verbosity.level; - } - - public static Verbosity resolve(CommandLine cli) { - if (cli.hasOption("s")) { - return SILENT; - } - if (cli.hasOption("v")) { - return VERBOSE; - } - return NORMAL; - } + /** Defines the available verbosity levels of messages to be printed. */ + public enum Verbosity { + SILENT, /* always printed */ + NORMAL, /* printed when no options are given to cli */ + VERBOSE /* printed only when cli is passed verbose option */ } + /** The current verbosity for the terminal, defaulting to {@link Verbosity#NORMAL}. */ private Verbosity verbosity = Verbosity.NORMAL; - public Terminal() { - this(Verbosity.NORMAL); - } - - public Terminal(Verbosity verbosity) { + /** Sets the verbosity of the terminal. */ + void setVerbosity(Verbosity verbosity) { this.verbosity = verbosity; } - public void verbosity(Verbosity verbosity) { - this.verbosity = verbosity; - } - - public Verbosity verbosity() { - return verbosity; - } - + /** Reads clear text from the terminal input. See {@link Console#readLine()}. */ public abstract String readText(String text, Object... args); + /** Reads password text from the terminal input. See {@link Console#readPassword()}}. */ public abstract char[] readSecret(String text, Object... args); - protected abstract void printStackTrace(Throwable t); + /** Print a message directly to the terminal. */ + protected abstract void doPrint(String msg); - public void println() { - println(Verbosity.NORMAL); - } - - public void println(String msg) { + /** Prints a line to the terminal at {@link Verbosity#NORMAL} verbosity level. */ + public final void println(String msg) { println(Verbosity.NORMAL, msg); } - public void print(String msg) { - print(Verbosity.NORMAL, msg); - } - - public void println(Verbosity verbosity) { - println(verbosity, ""); - } - - public void println(Verbosity verbosity, String msg) { - print(verbosity, msg + System.lineSeparator()); - } - - public void print(Verbosity verbosity, String msg) { - if (this.verbosity.enabled(verbosity)) { - doPrint(msg); + /** Prints a line to the terminal at {@code verbosity} level. */ + public final void println(Verbosity verbosity, String msg) { + if (verbosity.ordinal() >= this.verbosity.ordinal()) { + doPrint(msg + System.lineSeparator()); } } - public void printError(String msg) { - println(Verbosity.SILENT, "ERROR: " + msg); - } - - public void printWarn(String msg) { - println(Verbosity.SILENT, "WARN: " + msg); - } - - protected abstract void doPrint(String msg); - private static class ConsoleTerminal extends Terminal { - final Console console = System.console(); + private static final Console console = System.console(); - static boolean supported() { - return System.console() != null; + static boolean isSupported() { + return console != null; } @Override @@ -144,27 +103,21 @@ public abstract class Terminal { public char[] readSecret(String text, Object... args) { return console.readPassword(text, args); } - - @Override - public void printStackTrace(Throwable t) { - t.printStackTrace(console.writer()); - } } - @SuppressForbidden(reason = "System#out") private static class SystemTerminal extends Terminal { - private final PrintWriter printWriter = new PrintWriter(System.out); - @Override + @SuppressForbidden(reason = "System#out") public void doPrint(String msg) { System.out.print(msg); + System.out.flush(); } @Override public String readText(String text, Object... args) { - print(text); - BufferedReader reader = new BufferedReader(new InputStreamReader(System.in)); + doPrint(text); + BufferedReader reader = new BufferedReader(new InputStreamReader(System.in, Charset.defaultCharset())); try { return reader.readLine(); } catch (IOException ioe) { @@ -176,10 +129,5 @@ public abstract class Terminal { public char[] readSecret(String text, Object... args) { return readText(text, args).toCharArray(); } - - @Override - public void printStackTrace(Throwable t) { - t.printStackTrace(printWriter); - } } } diff --git a/core/src/main/java/org/elasticsearch/plugins/PluginSecurity.java b/core/src/main/java/org/elasticsearch/plugins/PluginSecurity.java index 9bbafa6e16a..b14bcaf2ff3 100644 --- a/core/src/main/java/org/elasticsearch/plugins/PluginSecurity.java +++ b/core/src/main/java/org/elasticsearch/plugins/PluginSecurity.java @@ -47,7 +47,7 @@ class PluginSecurity { PermissionCollection permissions = parsePermissions(terminal, file, environment.tmpFile()); List requested = Collections.list(permissions.elements()); if (requested.isEmpty()) { - terminal.print(Verbosity.VERBOSE, "plugin has a policy file with no additional permissions"); + terminal.println(Verbosity.VERBOSE, "plugin has a policy file with no additional permissions"); return; } @@ -92,7 +92,7 @@ class PluginSecurity { terminal.println(Verbosity.NORMAL, "See http://docs.oracle.com/javase/8/docs/technotes/guides/security/permissions.html"); terminal.println(Verbosity.NORMAL, "for descriptions of what these permissions allow and the associated risks."); if (!batch) { - terminal.println(Verbosity.NORMAL); + terminal.println(Verbosity.NORMAL, ""); String text = terminal.readText("Continue with installation? [y/N]"); if (!text.equalsIgnoreCase("y")) { throw new RuntimeException("installation aborted by user"); 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 3f5562fff61..4e65b9d5792 100644 --- a/core/src/test/java/org/elasticsearch/common/cli/TerminalTests.java +++ b/core/src/test/java/org/elasticsearch/common/cli/TerminalTests.java @@ -49,14 +49,14 @@ public class TerminalTests extends CliToolTestCase { } private void assertPrinted(CaptureOutputTerminal logTerminal, Terminal.Verbosity verbosity, String text) { - logTerminal.print(verbosity, text); + logTerminal.println(verbosity, text); assertThat(logTerminal.getTerminalOutput(), hasSize(1)); assertThat(logTerminal.getTerminalOutput(), hasItem(text)); logTerminal.terminalOutput.clear(); } private void assertNotPrinted(CaptureOutputTerminal logTerminal, Terminal.Verbosity verbosity, String text) { - logTerminal.print(verbosity, text); + logTerminal.println(verbosity, text); assertThat(logTerminal.getTerminalOutput(), hasSize(0)); } } diff --git a/test/framework/src/main/java/org/elasticsearch/common/cli/CliToolTestCase.java b/test/framework/src/main/java/org/elasticsearch/common/cli/CliToolTestCase.java index 35c08977ea7..ed56c4fc57a 100644 --- a/test/framework/src/main/java/org/elasticsearch/common/cli/CliToolTestCase.java +++ b/test/framework/src/main/java/org/elasticsearch/common/cli/CliToolTestCase.java @@ -61,17 +61,8 @@ public abstract class CliToolTestCase extends ESTestCase { */ public static class MockTerminal extends Terminal { - public MockTerminal() { - super(Verbosity.NORMAL); - } - - public MockTerminal(Verbosity verbosity) { - super(verbosity); - } - @Override - protected void doPrint(String msg) { - } + protected void doPrint(String msg) {} @Override public String readText(String text, Object... args) { @@ -82,13 +73,6 @@ public abstract class CliToolTestCase extends ESTestCase { public char[] readSecret(String text, Object... args) { return new char[0]; } - - @Override - public void print(String msg) { - } - - @Override - public void printStackTrace(Throwable t) {} } /** @@ -99,11 +83,11 @@ public abstract class CliToolTestCase extends ESTestCase { List terminalOutput = new ArrayList<>(); public CaptureOutputTerminal() { - super(Verbosity.NORMAL); + this(Verbosity.NORMAL); } public CaptureOutputTerminal(Verbosity verbosity) { - super(verbosity); + setVerbosity(verbosity); } @Override @@ -111,16 +95,6 @@ public abstract class CliToolTestCase extends ESTestCase { terminalOutput.add(msg); } - @Override - public void print(String msg) { - doPrint(msg); - } - - @Override - public void printStackTrace(Throwable t) { - terminalOutput.add(ExceptionsHelper.stackTrace(t)); - } - public List getTerminalOutput() { return terminalOutput; }