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.

core/src/main/java/org/elasticsearch/common/cli/CheckFileCommand.java

@@ -100,10 +100,10 @@ public abstract class CheckFileCommand extends CliTool.Command {
             Set<PosixFilePermission> permissionsBeforeWrite = entry.getValue();
             Set<PosixFilePermission> 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 + "]");
             }
         }
 

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);
                     }

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<String>() {
                 @Override
@@ -52,6 +52,6 @@ public class HelpPrinter {
         } catch (IOException ioe) {
             throw new RuntimeException(ioe);
         }
-        terminal.println();
+        terminal.println("");
     }
 }

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.io.PrintStream;
-import java.util.Locale;
+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 {
+    /** Defines the available verbosity levels of messages to be printed. */
-        SILENT(0), NORMAL(1), VERBOSE(2);
+    public enum Verbosity {
-
+        SILENT, /* always printed */
-        private final int level;
+        NORMAL, /* printed when no options are given to cli */
-
+        VERBOSE /* printed only when cli is passed verbose option */
-        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;
-        }
     }
 
+    /** The current verbosity for the terminal, defaulting to {@link Verbosity#NORMAL}. */
     private Verbosity verbosity = Verbosity.NORMAL;
 
-    public Terminal() {
+    /** Sets the verbosity of the terminal. */
-        this(Verbosity.NORMAL);
+    void setVerbosity(Verbosity verbosity) {
-    }
-
-    public Terminal(Verbosity verbosity) {
         this.verbosity = verbosity;
     }
 
-    public void verbosity(Verbosity verbosity) {
+    /** Reads clear text from the terminal input. See {@link Console#readLine()}. */
-        this.verbosity = verbosity;
-    }
-
-    public Verbosity verbosity() {
-        return verbosity;
-    }
-
     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() {
+    /** Prints a line to the terminal at {@link Verbosity#NORMAL} verbosity level. */
-        println(Verbosity.NORMAL);
+    public final void println(String msg) {
-    }
-
-    public void println(String msg) {
         println(Verbosity.NORMAL, msg);
     }
 
-    public void print(String msg) {
+    /** Prints a line to the terminal at {@code verbosity} level. */
-        print(Verbosity.NORMAL, msg);
+    public final void println(Verbosity verbosity, String msg) {
-    }
+        if (verbosity.ordinal() >= this.verbosity.ordinal()) {
-
+            doPrint(msg + System.lineSeparator());
-    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);
         }
     }
 
-    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() {
+        static boolean isSupported() {
-            return System.console() != null;
+            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);
+            doPrint(text);
-            BufferedReader reader = new BufferedReader(new InputStreamReader(System.in));
+            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);
-        }
     }
 }

core/src/main/java/org/elasticsearch/plugins/PluginSecurity.java

@@ -47,7 +47,7 @@ class PluginSecurity {
         PermissionCollection permissions = parsePermissions(terminal, file, environment.tmpFile());
         List<Permission> 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");

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));
     }
 }

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<String> 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<String> getTerminalOutput() {
             return terminalOutput;
         }