SOLR-17567: Improve Stream CLI implementation (#2872)

Cleanup and adding polish to the Stream CLI tool after getting more eyes on it. More robust handling of comments in .expr files, better use of CLI options, preventing "Strings" from being used to select an Option, in favour of an Option object. --------- Co-authored-by: Christos Malliaridis <c.malliaridis@gmail.com>
apache · Nov 22, 2024 · 621165a · 621165a
1 parent a4084e8
commit 621165a
Show file tree

Hide file tree

Showing 9 changed files with 152 additions and 108 deletions.
diff --git a/gradle/validation/forbidden-apis/commons-cli.commons-cli.all.txt b/gradle/validation/forbidden-apis/commons-cli.commons-cli.all.txt
@@ -0,0 +1,16 @@
+@defaultMessage Use a org.apache.commons.cli.Option instead of a String value
+org.apache.commons.cli.CommandLine#hasOption(java.lang.String)
+org.apache.commons.cli.CommandLine#getOptionValue(java.lang.String)
+org.apache.commons.cli.CommandLine#getOptionValue(java.lang.String, java.lang.String)
+org.apache.commons.cli.CommandLine#getParsedOptionValue(java.lang.String, java.lang.Object)
+org.apache.commons.cli.CommandLine#hasOption(char)
+org.apache.commons.cli.CommandLine#getOptionValue(char)
+org.apache.commons.cli.CommandLine#getOptionValue(char, java.lang.String)
+#org.apache.commons.cli.CommandLine#getOptionValue(char, Supplier<String>)
+org.apache.commons.cli.CommandLine#getOptionValues(char)
+org.apache.commons.cli.CommandLine#getOptionValues(java.lang.String)
+org.apache.commons.cli.CommandLine#getParsedOptionValue(char)
+# org.apache.commons.cli.CommandLine#getParsedOptionValue(char, Supplier<T>)
+org.apache.commons.cli.CommandLine#getParsedOptionValue(char, java.lang.Object)
+org.apache.commons.cli.CommandLine#getParsedOptionValue(java.lang.String)
+# org.apache.commons.cli.CommandLine#getParsedOptionValue(String, Supplier<T>)
diff --git a/solr/core/src/java/org/apache/solr/cli/SolrCLI.java b/solr/core/src/java/org/apache/solr/cli/SolrCLI.java
@@ -231,7 +231,7 @@ private static Tool newTool(String toolType) throws Exception {
    * CLI option.
    */
   public static String getOptionWithDeprecatedAndDefault(
-      CommandLine cli, String opt, String deprecated, String def) {
+      CommandLine cli, Option opt, Option deprecated, String def) {
     String val = cli.getOptionValue(opt);
     if (val == null) {
       val = cli.getOptionValue(deprecated);

diff --git a/solr/core/src/java/org/apache/solr/cli/StreamTool.java b/solr/core/src/java/org/apache/solr/cli/StreamTool.java
@@ -35,6 +35,7 @@
 import java.util.HashMap;
 import java.util.List;
 import java.util.Locale;
+import java.util.Map;
 import java.util.Set;
 import org.apache.commons.cli.CommandLine;
 import org.apache.commons.cli.Option;
@@ -77,12 +78,12 @@ public String getName() {
   @Override
   public String getUsage() {
     // Specify that the last argument is the streaming expression
-    return "bin/solr stream [--array-delimiter <CHARACTER>] [-c <NAME>] [--delimiter <CHARACTER>] [-e <ENVIRONMENT>] [-f\n"
+    return "bin/solr stream [--array-delimiter <CHARACTER>] [-c <NAME>] [--delimiter <CHARACTER>] [--execution <ENVIRONMENT>] [--fields\n"
         + "       <FIELDS>] [-h] [--header] [-s <HOST>] [-u <credentials>] [-v] [-z <HOST>]  <streaming expression OR stream_file.expr>\n";
   }
 
   private static final Option EXECUTION_OPTION =
-      Option.builder("e")
+      Option.builder()
           .longOpt("execution")
           .hasArg()
           .argName("ENVIRONMENT")
@@ -100,7 +101,7 @@ public String getUsage() {
           .build();
 
   private static final Option FIELDS_OPTION =
-      Option.builder("f")
+      Option.builder()
           .longOpt("fields")
           .argName("FIELDS")
           .hasArg()
@@ -229,11 +230,7 @@ public void runImpl(CommandLine cli) throws Exception {
         }
       }
     } finally {
-
-      if (pushBackStream != null) {
-        pushBackStream.close();
-      }
-
+      pushBackStream.close();
       solrClientCache.close();
     }
 
@@ -277,7 +274,7 @@ private PushBackStream doLocalMode(CommandLine cli, String expr) throws Exceptio
 
     Lang.register(streamFactory);
 
-    stream = StreamTool.constructStream(streamFactory, streamExpression);
+    stream = streamFactory.constructStream(streamExpression);
 
     pushBackStream = new PushBackStream(stream);
 
@@ -306,11 +303,11 @@ private PushBackStream doLocalMode(CommandLine cli, String expr) throws Exceptio
   private PushBackStream doRemoteMode(CommandLine cli, String expr) throws Exception {
 
     String solrUrl = CLIUtils.normalizeSolrUrl(cli);
-    if (!cli.hasOption("name")) {
+    if (!cli.hasOption(COLLECTION_OPTION)) {
       throw new IllegalStateException(
-          "You must provide --name COLLECTION with --worker solr parameter.");
+          "You must provide --name COLLECTION with --execution remote parameter.");
     }
-    String collection = cli.getOptionValue("name");
+    String collection = cli.getOptionValue(COLLECTION_OPTION);
 
     if (expr.toLowerCase(Locale.ROOT).contains("stdin(")) {
       throw new IllegalStateException(
@@ -371,11 +368,10 @@ public void close() throws IOException {
       }
     }
 
-    @SuppressWarnings({"unchecked", "rawtypes"})
     @Override
     public Tuple read() throws IOException {
       String line = reader.readLine();
-      HashMap map = new HashMap();
+      Map<String, ?> map = new HashMap<>();
       Tuple tuple = new Tuple(map);
       if (line != null) {
         tuple.put("line", line);
@@ -435,11 +431,15 @@ public LocalCatStream(String commaDelimitedFilepaths, int maxLines) {
 
     @Override
     public void setStreamContext(StreamContext context) {
-      // LocalCatStream has no Solr core to pull from the context
+      // LocalCatStream inherently has no Solr core to pull from the context
     }
 
     @Override
-    protected List<CrawlFile> validateAndSetFilepathsInSandbox(String commaDelimitedFilepaths) {
+    protected List<CrawlFile> validateAndSetFilepathsInSandbox() {
+      // The nature of LocalCatStream is that we are not limited to the sandboxed "userfiles"
+      // directory
+      // the way the CatStream does.
+
       final List<CrawlFile> crawlSeeds = new ArrayList<>();
       for (String crawlRootStr : commaDelimitedFilepaths.split(",")) {
         Path crawlRootPath = Paths.get(crawlRootStr).normalize();
@@ -483,11 +483,6 @@ static String listToString(List values, String internalDelim) {
     return buf.toString();
   }
 
-  private static TupleStream constructStream(
-      StreamFactory streamFactory, StreamExpression streamExpression) throws IOException {
-    return streamFactory.constructStream(streamExpression);
-  }
-
   static String readExpression(LineNumberReader bufferedReader, String[] args) throws IOException {
 
     StringBuilder exprBuff = new StringBuilder();
@@ -499,17 +494,17 @@ static String readExpression(LineNumberReader bufferedReader, String[] args) thr
         break;
       }
 
-      if (line.indexOf("/*") == 0) {
+      if (line.trim().indexOf("/*") == 0) {
         comment = true;
         continue;
       }
 
-      if (line.indexOf("*/") == 0) {
+      if (line.trim().contains("*/")) {
         comment = false;
         continue;
       }
 
-      if (comment || line.startsWith("#") || line.startsWith("//")) {
+      if (comment || line.trim().startsWith("#") || line.trim().startsWith("//")) {
         continue;
       }
 

diff --git a/solr/core/src/java/org/apache/solr/handler/CatStream.java b/solr/core/src/java/org/apache/solr/handler/CatStream.java
@@ -49,7 +49,7 @@
 public class CatStream extends TupleStream implements Expressible {
   private static final Logger log = LoggerFactory.getLogger(MethodHandles.lookup().lookupClass());
 
-  private final String commaDelimitedFilepaths;
+  protected final String commaDelimitedFilepaths;
   private final int maxLines; // -1 for no max
 
   private StreamContext context;
@@ -113,8 +113,7 @@ public List<TupleStream> children() {
 
   @Override
   public void open() throws IOException {
-    final List<CrawlFile> initialCrawlSeeds =
-        validateAndSetFilepathsInSandbox(this.commaDelimitedFilepaths);
+    final List<CrawlFile> initialCrawlSeeds = validateAndSetFilepathsInSandbox();
 
     final List<CrawlFile> filesToCrawl = new ArrayList<>();
     for (CrawlFile crawlSeed : initialCrawlSeeds) {
@@ -164,7 +163,7 @@ public Explanation toExplanation(StreamFactory factory) throws IOException {
         .withExpression(toExpression(factory).toString());
   }
 
-  protected List<CrawlFile> validateAndSetFilepathsInSandbox(String commaDelimitedFilepaths) {
+  protected List<CrawlFile> validateAndSetFilepathsInSandbox() {
     final List<CrawlFile> crawlSeeds = new ArrayList<>();
     for (String crawlRootStr : commaDelimitedFilepaths.split(",")) {
       Path crawlRootPath = chroot.resolve(crawlRootStr).normalize();

diff --git a/solr/core/src/test/org/apache/solr/cli/StreamToolTest.java b/solr/core/src/test/org/apache/solr/cli/StreamToolTest.java
@@ -100,8 +100,15 @@ public void testReadExpression() throws Exception {
     buf.println("/*");
     buf.println("Multi-line comment Comment...");
     buf.println("*/");
+    buf.println("  /*");
+    buf.println("Multi-line comment Comment...");
+    buf.println("  */");
+    buf.println("/*");
+    buf.println("Multi-line comment ending with closing chars... */");
     buf.println("// Single line comment");
     buf.println("# Single line comment");
+    buf.println(" // Single line comment");
+    buf.println(" # Single line comment");
     buf.println("let(a=$1, b=$2,");
     buf.println("search($3))");
     buf.println(")");
@@ -227,7 +234,7 @@ public void testStdInFailsWithRemoteWorker() throws Exception {
     String[] args =
         new String[] {
           "stream",
-          "-e",
+          "--execution",
           "remote",
           "--name",
           "fakeCollection",
@@ -246,7 +253,7 @@ public void testStdInSucceedsWithLocalWorker() throws Exception {
     String[] args =
         new String[] {
           "stream",
-          "-e",
+          "--execution",
           "local",
           "-v",
           "-z",
@@ -269,7 +276,7 @@ public void testRunEchoStreamLocally() throws Exception {
     // notice that we do not pass in zkHost or solrUrl for a simple echo run locally.
     String[] args = {
       "stream",
-      "-e",
+      "--execution",
       "local",
       "--verbose",
       "-zk-host",
@@ -313,7 +320,7 @@ public void testRunEchoStreamRemotely() throws Exception {
     // test passing in the file
     String[] args = {
       "stream",
-      "-e",
+      "--execution",
       "remote",
       "-c",
       collectionName,