SOLR-16711: Extract SolrCLI tool implementations into their own package and classes by epugh · Pull Request #1476 · apache/solr

epugh · 2023-03-20T17:47:02Z

https://issues.apache.org/jira/browse/SOLR-16711

Description

SolrCLI.java is overwhelming to work with.

Solution

Extract tools into their own package.

Tests

Please describe the tests you've developed or run to confirm this patch implements the feature or solves the problem.

Checklist

Please review the following and check all that apply:

[X ] I have reviewed the guidelines for How to Contribute and my code conforms to the standards described there to the best of my ability.
[ X] I have created a Jira issue and added the issue ID to my pull request title.
[ X] I have given Solr maintainers access to contribute to my PR branch. (optional but recommended)
[ X] I have developed this patch against the main branch.
[ X] I have run ./gradlew check.
I have added tests for my changes.
I have added documentation for the Reference Guide

Slim down the overwhelming size of the SolrCLI.java class

janhoy

Wow, this is soo over due :) Looks great!

solr/core/src/java/org/apache/solr/util/SolrCLI.java

sonatype-lift · 2023-03-20T18:44:30Z

solr/core/src/java/org/apache/solr/util/SolrCLI.java

-  } // end HealthcheckTool
-
-  private static final Option[] CREATE_COLLECTION_OPTIONS =
+  public static final Option[] CREATE_COLLECTION_OPTIONS =


MutablePublicArray: Non-empty arrays are mutable, so this public static final array is not a constant and can be modified by clients of this class. Prefer an ImmutableList, or provide an accessor method that returns a defensive copy.

ℹ️ Expand to see all @sonatype-lift commands

You can reply with the following commands. For example, reply with @sonatype-lift ignoreall to leave out all findings.

Command Usage

@sonatype-lift ignore Leave out the above finding from this PR

@sonatype-lift ignoreall Leave out all the existing findings from this PR

@sonatype-lift exclude <file|issue|path|tool> Exclude specified file|issue|path|tool from Lift findings by updating your config.toml file

Note: When talking to LiftBot, you need to refresh the page to see its response.
_{Click here to add LiftBot to another repo.}

Help us improve LIFT! (Sonatype LiftBot external survey)

Was this a good recommendation for you? _{Answering this survey will not impact your Lift settings.}

[ 🙁 Not relevant ] - [ 😕 Won't fix ] - [ 😑 Not critical, will fix ] - [ 🙂 Critical, will fix ] - [ 😊 Critical, fixing now ]

sonatype-lift · 2023-03-20T18:44:32Z

solr/core/src/java/org/apache/solr/util/cli/RunExampleTool.java

+    protected boolean isPortAvailable(int port) {
+        Socket s = null;
+        try {
+            s = new Socket("localhost", port);


UNENCRYPTED_SOCKET: Unencrypted socket to org.apache.solr.util.cli.RunExampleTool (instead of SSLSocket)

ℹ️ Expand to see all @sonatype-lift commands

You can reply with the following commands. For example, reply with @sonatype-lift ignoreall to leave out all findings.

Command Usage

@sonatype-lift ignore Leave out the above finding from this PR

@sonatype-lift ignoreall Leave out all the existing findings from this PR

@sonatype-lift exclude <file|issue|path|tool> Exclude specified file|issue|path|tool from Lift findings by updating your config.toml file

Note: When talking to LiftBot, you need to refresh the page to see its response.
_{Click here to add LiftBot to another repo.}

Help us improve LIFT! (Sonatype LiftBot external survey)

Was this a good recommendation for you? _{Answering this survey will not impact your Lift settings.}

[ 🙁 Not relevant ] - [ 😕 Won't fix ] - [ 😑 Not critical, will fix ] - [ 🙂 Critical, will fix ] - [ 😊 Critical, fixing now ]

sonatype-lift · 2023-03-20T18:44:34Z

solr/core/src/java/org/apache/solr/util/cli/ToolBase.java

+            if (excMsg != null) {
+                CLIO.err("\nERROR: " + excMsg + "\n");
+                if (verbose) {
+                    exc.printStackTrace(CLIO.getErrStream());


INFORMATION_EXPOSURE_THROUGH_AN_ERROR_MESSAGE: Possible information exposure through an error message

ℹ️ Expand to see all @sonatype-lift commands

You can reply with the following commands. For example, reply with @sonatype-lift ignoreall to leave out all findings.

Command Usage

@sonatype-lift ignore Leave out the above finding from this PR

@sonatype-lift ignoreall Leave out all the existing findings from this PR

@sonatype-lift exclude <file|issue|path|tool> Exclude specified file|issue|path|tool from Lift findings by updating your config.toml file

Note: When talking to LiftBot, you need to refresh the page to see its response.
_{Click here to add LiftBot to another repo.}

Help us improve LIFT! (Sonatype LiftBot external survey)

Was this a good recommendation for you? _{Answering this survey will not impact your Lift settings.}

[ 🙁 Not relevant ] - [ 😕 Won't fix ] - [ 😑 Not critical, will fix ] - [ 🙂 Critical, will fix ] - [ 😊 Critical, fixing now ]

sonatype-lift · 2023-03-20T18:44:37Z

solr/core/src/java/org/apache/solr/util/cli/RunExampleTool.java

+            throw new IllegalArgumentException(
+                    "Value of -serverDir option is invalid! " + zooCfg.getAbsolutePath() + " not found!");
+
+        File solrHomeDir = new File(exampleParentDir, dirName + "/solr");


PATH_TRAVERSAL_IN: This API (java/io/File.(Ljava/io/File;Ljava/lang/String;)V) reads a file whose location might be specified by user input

❗❗ 24 similar findings have been found in this PR

🔎 Expand here to view all instances of this finding

File Path Line Number

solr/core/src/java/org/apache/solr/util/cli/AssertTool.java 275

solr/core/src/java/org/apache/solr/util/cli/AuthTool.java 432

solr/core/src/java/org/apache/solr/util/cli/RunExampleTool.java 201

solr/core/src/java/org/apache/solr/util/cli/CreateCoreTool.java 71

solr/core/src/java/org/apache/solr/util/cli/AuthTool.java 440

solr/core/src/java/org/apache/solr/util/cli/ConfigSetDownloadTool.java 73

solr/core/src/java/org/apache/solr/util/cli/CreateCoreTool.java 79

solr/core/src/java/org/apache/solr/util/cli/ConfigSetDownloadTool.java 69

solr/core/src/java/org/apache/solr/util/cli/RunExampleTool.java 481

solr/core/src/java/org/apache/solr/util/cli/AuthTool.java 235

Showing 10 of 24 findings. Visit the Lift Web Console to see all.

ℹ️ Expand to see all @sonatype-lift commands

You can reply with the following commands. For example, reply with @sonatype-lift ignoreall to leave out all findings.

Command Usage

@sonatype-lift ignore Leave out the above finding from this PR

@sonatype-lift ignoreall Leave out all the existing findings from this PR

@sonatype-lift exclude <file|issue|path|tool> Exclude specified file|issue|path|tool from Lift findings by updating your config.toml file

Note: When talking to LiftBot, you need to refresh the page to see its response.
_{Click here to add LiftBot to another repo.}

Help us improve LIFT! (Sonatype LiftBot external survey)

Was this a good recommendation for you? _{Answering this survey will not impact your Lift settings.}

[ 🙁 Not relevant ] - [ 😕 Won't fix ] - [ 😑 Not critical, will fix ] - [ 🙂 Critical, will fix ] - [ 😊 Critical, fixing now ]

sonatype-lift · 2023-03-20T18:44:39Z

solr/core/src/java/org/apache/solr/util/cli/AssertTool.java

+    }
+
+    public static int assertNotRootUser() throws Exception {
+        if (currentUser().equals("root")) {


NULL_DEREFERENCE: object returned by currentUser() could be null and is dereferenced at line 308.

❗❗ 4 similar findings have been found in this PR

🔎 Expand here to view all instances of this finding

File Path Line Number

solr/core/src/java/org/apache/solr/util/cli/AssertTool.java 277

solr/core/src/java/org/apache/solr/util/cli/StatusTool.java 141

solr/core/src/java/org/apache/solr/util/cli/StatusTool.java 169

solr/core/src/java/org/apache/solr/util/cli/AssertTool.java 301

Visit the Lift Web Console to find more details in your report.

ℹ️ Expand to see all @sonatype-lift commands

You can reply with the following commands. For example, reply with @sonatype-lift ignoreall to leave out all findings.

Command Usage

@sonatype-lift ignore Leave out the above finding from this PR

@sonatype-lift ignoreall Leave out all the existing findings from this PR

@sonatype-lift exclude <file|issue|path|tool> Exclude specified file|issue|path|tool from Lift findings by updating your config.toml file

Note: When talking to LiftBot, you need to refresh the page to see its response.
_{Click here to add LiftBot to another repo.}

Help us improve LIFT! (Sonatype LiftBot external survey)

Was this a good recommendation for you? _{Answering this survey will not impact your Lift settings.}

[ 🙁 Not relevant ] - [ 😕 Won't fix ] - [ 😑 Not critical, will fix ] - [ 🙂 Critical, will fix ] - [ 😊 Critical, fixing now ]

sonatype-lift · 2023-03-20T21:20:32Z

solr/core/src/java/org/apache/solr/util/cli/ToolBase.java

+      if (excMsg != null) {
+        CLIO.err("\nERROR: " + excMsg + "\n");
+        if (verbose) {
+          exc.printStackTrace(CLIO.getErrStream());


INFORMATION_EXPOSURE_THROUGH_AN_ERROR_MESSAGE: Possible information exposure through an error message

ℹ️ Expand to see all @sonatype-lift commands

You can reply with the following commands. For example, reply with @sonatype-lift ignoreall to leave out all findings.

Command Usage

@sonatype-lift ignore Leave out the above finding from this PR

@sonatype-lift ignoreall Leave out all the existing findings from this PR

@sonatype-lift exclude <file|issue|path|tool> Exclude specified file|issue|path|tool from Lift findings by updating your config.toml file

Note: When talking to LiftBot, you need to refresh the page to see its response.
_{Click here to add LiftBot to another repo.}

Help us improve LIFT! (Sonatype LiftBot external survey)

Was this a good recommendation for you? _{Answering this survey will not impact your Lift settings.}

[ 🙁 Not relevant ] - [ 😕 Won't fix ] - [ 😑 Not critical, will fix ] - [ 🙂 Critical, will fix ] - [ 😊 Critical, fixing now ]

sonatype-lift · 2023-03-20T21:20:35Z

solr/core/src/java/org/apache/solr/util/cli/RunExampleTool.java

+  protected boolean isPortAvailable(int port) {
+    Socket s = null;
+    try {
+      s = new Socket("localhost", port);


UNENCRYPTED_SOCKET: Unencrypted socket to org.apache.solr.util.cli.RunExampleTool (instead of SSLSocket)

ℹ️ Expand to see all @sonatype-lift commands

You can reply with the following commands. For example, reply with @sonatype-lift ignoreall to leave out all findings.

Command Usage

@sonatype-lift ignore Leave out the above finding from this PR

@sonatype-lift ignoreall Leave out all the existing findings from this PR

@sonatype-lift exclude <file|issue|path|tool> Exclude specified file|issue|path|tool from Lift findings by updating your config.toml file

Note: When talking to LiftBot, you need to refresh the page to see its response.
_{Click here to add LiftBot to another repo.}

Help us improve LIFT! (Sonatype LiftBot external survey)

Was this a good recommendation for you? _{Answering this survey will not impact your Lift settings.}

[ 🙁 Not relevant ] - [ 😕 Won't fix ] - [ 😑 Not critical, will fix ] - [ 🙂 Critical, will fix ] - [ 😊 Critical, fixing now ]

sonatype-lift · 2023-03-20T21:20:37Z

solr/core/src/java/org/apache/solr/util/cli/ConfigSetDownloadTool.java

+      // we try to be nice about having the "conf" in the directory, and we create it if it's not
+      // there.
+      if (!configSetPath.endsWith("/conf")) {
+        configSetPath = Paths.get(configSetPath.toString(), "conf");


PATH_TRAVERSAL_IN: This API (java/nio/file/Paths.get(Ljava/lang/String;[Ljava/lang/String;)Ljava/nio/file/Path;) reads a file whose location might be specified by user input

❗❗ 24 similar findings have been found in this PR

🔎 Expand here to view all instances of this finding

File Path Line Number

solr/core/src/java/org/apache/solr/util/cli/RunExampleTool.java 212

solr/core/src/java/org/apache/solr/util/cli/CreateCoreTool.java 94

solr/core/src/java/org/apache/solr/util/cli/RunExampleTool.java 932

solr/core/src/java/org/apache/solr/util/cli/CreateCoreTool.java 91

solr/core/src/java/org/apache/solr/util/cli/AuthTool.java 441

solr/core/src/java/org/apache/solr/util/cli/RunExampleTool.java 628

solr/core/src/java/org/apache/solr/util/cli/ConfigSetDownloadTool.java 89

solr/core/src/java/org/apache/solr/util/cli/RunExampleTool.java 182

solr/core/src/java/org/apache/solr/util/cli/RunExampleTool.java 890

solr/core/src/java/org/apache/solr/util/cli/RunExampleTool.java 211

Showing 10 of 24 findings. Visit the Lift Web Console to see all.

ℹ️ Expand to see all @sonatype-lift commands

You can reply with the following commands. For example, reply with @sonatype-lift ignoreall to leave out all findings.

Command Usage

@sonatype-lift ignore Leave out the above finding from this PR

@sonatype-lift ignoreall Leave out all the existing findings from this PR

@sonatype-lift exclude <file|issue|path|tool> Exclude specified file|issue|path|tool from Lift findings by updating your config.toml file

Note: When talking to LiftBot, you need to refresh the page to see its response.
_{Click here to add LiftBot to another repo.}

Help us improve LIFT! (Sonatype LiftBot external survey)

Was this a good recommendation for you? _{Answering this survey will not impact your Lift settings.}

[ 🙁 Not relevant ] - [ 😕 Won't fix ] - [ 😑 Not critical, will fix ] - [ 🙂 Critical, will fix ] - [ 😊 Critical, fixing now ]

sonatype-lift · 2023-03-20T21:20:40Z

solr/core/src/java/org/apache/solr/util/cli/StatusTool.java

+    status.put("solr_home", solrHome != null ? solrHome : "?");
+    status.put("version", SolrCLI.asString("/lucene/solr-impl-version", info));
+    status.put("startTime", SolrCLI.asString("/jvm/jmx/startTime", info));
+    status.put("uptime", SolrCLI.uptime(SolrCLI.asLong("/jvm/jmx/upTimeMS", info)));


NULL_DEREFERENCE: object returned by asLong("/jvm/jmx/upTimeMS",info) could be null and is dereferenced at line 154.

❗❗ 4 similar findings have been found in this PR

🔎 Expand here to view all instances of this finding

File Path Line Number

solr/core/src/java/org/apache/solr/util/cli/AssertTool.java 318

solr/core/src/java/org/apache/solr/util/cli/AssertTool.java 294

solr/core/src/java/org/apache/solr/util/cli/AssertTool.java 325

solr/core/src/java/org/apache/solr/util/cli/StatusTool.java 182

Visit the Lift Web Console to find more details in your report.

ℹ️ Expand to see all @sonatype-lift commands

You can reply with the following commands. For example, reply with @sonatype-lift ignoreall to leave out all findings.

Command Usage

@sonatype-lift ignore Leave out the above finding from this PR

@sonatype-lift ignoreall Leave out all the existing findings from this PR

@sonatype-lift exclude <file|issue|path|tool> Exclude specified file|issue|path|tool from Lift findings by updating your config.toml file

Note: When talking to LiftBot, you need to refresh the page to see its response.
_{Click here to add LiftBot to another repo.}

Help us improve LIFT! (Sonatype LiftBot external survey)

Was this a good recommendation for you? _{Answering this survey will not impact your Lift settings.}

[ 🙁 Not relevant ] - [ 😕 Won't fix ] - [ 😑 Not critical, will fix ] - [ 🙂 Critical, will fix ] - [ 😊 Critical, fixing now ]

getCommonToolOptions appears to offer a capability that isn't used in the SolrCLI today, so simplify.

…attern

epugh · 2023-03-21T17:42:08Z

Getting to done on this... A question, should I move SolrCLI.java into the .utils.cli package and update the scripts etc? Any other changes that folks think need doing to call this "done" and ready for review/commit?

…e package.

Co-authored-by: sonatype-lift[bot] <37194012+sonatype-lift[bot]@users.noreply.github.com>

epugh · 2023-03-22T15:22:48Z

Getting to done on this... A question, should I move SolrCLI.java into the .utils.cli package and update the scripts etc? Any other changes that folks think need doing to call this "done" and ready for review/commit?

@janhoy @risdenk just wanted your opinons on making this change or not, and then I think I'm ready to commit ;-)

epugh · 2023-04-17T11:56:59Z

Merging in #1182 made this too hard to update... So I started over again in #1568. Going to close this one in favour of the new one.

epugh added 2 commits March 20, 2023 09:55

Extract various Tools into the .util.cli package

edde3bb

Slim down the overwhelming size of the SolrCLI.java class

First pass of responding to intellij suggestions.

1e4ea50

epugh requested review from HoustonPutman and risdenk March 20, 2023 17:47

janhoy approved these changes Mar 20, 2023

View reviewed changes

sonatype-lift bot reviewed Mar 20, 2023

View reviewed changes

solr/core/src/java/org/apache/solr/util/SolrCLI.java Outdated Show resolved Hide resolved

sonatype-lift bot reviewed Mar 20, 2023

View reviewed changes

epugh added 12 commits March 20, 2023 16:07

Simplify logic?

d1f2b3f

add license and update log pattern

63955bf

tweaks to pass precheck

a31d47a

Create package-info.java

e414201

tidy

37a60ac

Merge remote-tracking branch 'upstream/main' into SOLR-16711

a77a0fe

tweaks from merge of TimeUnit...

0494975

use try/with resources pattern

c44e9a1

Deal with potential for tools in two packages

4b86cc9

some missing tools in the list of help options...

6c3a4eb

remove deprecated method...

a815206

tidy

71afd55

sonatype-lift bot reviewed Mar 20, 2023

View reviewed changes

epugh added 3 commits March 21, 2023 07:49

Deprecated method

3e07675

This method is used by many tools, to genericize the error message

e6a196f

chase down some more try/with resource places

aef213c

Updates required after removing deprecated method

2db2d43

risdenk approved these changes Mar 21, 2023

View reviewed changes

epugh added 6 commits March 21, 2023 11:45

Remove unused getCommonToolOptions methods

1f1d7ea

getCommonToolOptions appears to offer a capability that isn't used in the SolrCLI today, so simplify.

Rework Option[] to be next to each other and all caps to follow the p…

401de0d

…attern

Reviewing intellij feedback on this class...

5f4bd3b

Be explicit on TimeUnit

6210b15

Match pattern of other defaults.

5426522

Let's use our TimeUnit

98973f1

epugh and others added 3 commits March 21, 2023 14:09

tidy

1b7dc8c

We actually have one test dedicated to one tool, so put it in the sam…

cbbbd6f

…e package.

Update solr/core/src/java/org/apache/solr/util/SolrCLI.java

c39c35f

Co-authored-by: sonatype-lift[bot] <37194012+sonatype-lift[bot]@users.noreply.github.com>

epugh mentioned this pull request Mar 23, 2023

SOLR-16504 Convert CLI tools to Jetty HTTP 2 client. #1182

Merged

7 tasks

epugh closed this Apr 17, 2023

Command	Usage
`@sonatype-lift ignore`	Leave out the above finding from this PR
`@sonatype-lift ignoreall`	Leave out all the existing findings from this PR
`@sonatype-lift exclude <file\|issue\|path\|tool>`	Exclude specified `file\|issue\|path\|tool` from Lift findings by updating your config.toml file

Conversation

epugh commented Mar 20, 2023

Description

Solution

Tests

Checklist

Uh oh!

janhoy left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

sonatype-lift bot Mar 20, 2023

Choose a reason for hiding this comment

Uh oh!

sonatype-lift bot Mar 20, 2023

Choose a reason for hiding this comment

Uh oh!

sonatype-lift bot Mar 20, 2023

Choose a reason for hiding this comment

Uh oh!

sonatype-lift bot Mar 20, 2023

Choose a reason for hiding this comment

Uh oh!

sonatype-lift bot Mar 20, 2023

Choose a reason for hiding this comment

Uh oh!

sonatype-lift bot Mar 20, 2023

Choose a reason for hiding this comment

Uh oh!

sonatype-lift bot Mar 20, 2023

Choose a reason for hiding this comment

Uh oh!

sonatype-lift bot Mar 20, 2023

Choose a reason for hiding this comment

Uh oh!

sonatype-lift bot Mar 20, 2023

Choose a reason for hiding this comment

Uh oh!

epugh commented Mar 21, 2023

Uh oh!

epugh commented Mar 22, 2023

Uh oh!

epugh commented Apr 17, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants