SOLR-16829: Delegate arg parsing and help usage to java. (Round 1) #1670

epugh · 2023-05-29T23:05:56Z

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

Description

Going thorugh and attempting to remove the help usage (which seems to require also removing the arg parsing!)

I have migrated on the *nix side these commands:

bin/solr (by itself)
create
create_core
create_collection
delete
healthcheck
status

@Willdotwhite has done the same on the Windows side.

Solution

Please provide a short description of the approach taken to implement your solution.

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:

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

sonatype-lift · 2023-05-31T00:25:55Z

solr/core/src/java/org/apache/solr/cli/CreateCollectionTool.java

+
+  private String ensureConfDirExists(String confDirName, String solrInstallDir) {
+    final String fullConfDir = solrInstallDir + "/server/solr/configsets/" + confDirName;
+    if (!new File(fullConfDir).isDirectory()) {


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

ℹ️ 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.}

…st tweak text.

…o createTool works..

sonatype-lift · 2023-06-10T12:48:00Z

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

+        cli.getOptionValue("confdir", SolrCLI.DEFAULT_CONFIG_SET); // CREATE_CONFDIR
+
+    // we allow them to pass a directory instead of a configset name
+    File configsetDir = new File(confDirName);


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

❗❗ 2 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/cli/CreateCoreTool.java 127

solr/core/src/java/org/apache/solr/cli/CreateCoreTool.java 166

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.}

…t provide a solrurl...

sonatype-lift · 2023-06-10T15:15:53Z

solr/core/src/java/org/apache/solr/cli/CreateCollectionTool.java

+   * solrInstallDir.
+   */
+  private void ensureConfDirExists(String confDirName, String solrInstallDir) {
+    if (!new File(confDirName).isDirectory()) {


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

ℹ️ 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.}

sonatype-lift · 2023-06-10T15:15:55Z

solr/core/src/java/org/apache/solr/cli/CreateCollectionTool.java

+  private void ensureConfDirExists(String confDirName, String solrInstallDir) {
+    if (!new File(confDirName).isDirectory()) {
+      final String fullConfDir = solrInstallDir + "/server/solr/configsets/" + confDirName;
+      if (!new File(fullConfDir).isDirectory()) {


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

ℹ️ 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.}

…collection

epugh · 2023-06-22T12:48:43Z

@mkhludnev I've chased down the latest merge conflicts ;-). Let me know how it goes, and if you want to do a pairing session, I can jump on next week.

epugh · 2023-06-23T16:13:13Z

@Willdotwhite @mkhludnev so what is the best way to bring in the changes that both of you have made? main...mkhludnev:solr:SOLR-16829 shows some great improvments from each of you? Do you each propose a PR to the branch that I have in my github.com/epugh/solr? Just not sure how to get those commits into the PR? @Willdotwhite are the changes you've made ready to go once status is done?

epugh · 2023-06-28T19:08:17Z

@Willdotwhite I had a thought which is that maybe I'll back out the bin/solr statuscommand changes, since that was giving you some problems, and move that to the Round 2 of work, which then would make this committable?

@mkhludnev

Original source: 718c5ef curtesy of @mkhludnev Manually copied ignoring existing fixes to `healthcheck`

Update solr.cmd for round #1 migration to SolrCLI

epugh · 2023-06-28T22:18:00Z

@Willdotwhite can we delete the method status_usage ? Line 282 has:

IF "%SCRIPT_CMD%"=="status" goto status_usage

…mons cli generates

…ommands Remove status usage commands

epugh · 2023-06-29T21:45:30Z

okay @Willdotwhite I had to restore a change to StatusTool.java to make it work on Unix.. Is it possible that we have the .cmd and the .sh somewhat working at cross purposes in how we decide to output the help?

Willdotwhite · 2023-06-30T07:59:32Z

okay @Willdotwhite I had to restore a change to StatusTool.java to make it work on Unix.. Is it possible that we have the .cmd and the .sh somewhat working at cross purposes in how we decide to output the help?

Sorry @epugh , I think I was just a bit too tired yesterday and flubbed my testing - I've just put the condition back to if ((cli.getOptions().length == 0 && cli.getArgs().length == 0) || cli.hasOption("h") || cli.hasOption("help")) { on my machine and the status checks work as before; not really sure what happened there 🤔

I think NIX and Windows status checks are inline, just retested the Windows side locally from this branch and it's working as expected.

By the way, a bit of copypasta has slipped through the net on the hashbang of solr.sh just here beginning 'test_telp'

epugh · 2023-07-06T16:43:35Z

I think we are finally there... Responding to the last bits of feedback.

gerlowskija

Awesome to see so many lines in bin/solr and bin/solr.cmd get nuked, though I have some concerns around backcompat and the user experience. See inline comments for more details...

gerlowskija · 2023-07-07T18:12:05Z

solr/bin/solr

-    -help|-h)
-        print_usage ""
+    -help|-h)    
+        run_tool ""    


[Q] Does nothing in tidy/check complain about trailing whitespace? I don't care much, but I'm surprised given how we've leaned into code-formatting in recent years.

Not that I know of... Maybe because this is shell? I didn't run shellcheck after making changes...

gerlowskija · 2023-07-07T18:19:16Z

solr/core/src/java/org/apache/solr/cli/HealthcheckTool.java

@@ -60,11 +60,11 @@ public List<Option> getOptions() {
        SolrCLI.OPTION_SOLRURL,
        SolrCLI.OPTION_ZKHOST,
        Option.builder("c")
-            .argName("COLLECTION")
+            .longOpt("name")
+            .argName("NAME")
            .hasArg()
            .required(false)


[-1] AFAICT from this bin/solr code, the collection parameter should be required for the healthcheck tool.

Prior to this commit, we enforced this requirement in the bash/Windows-scripts, but now that we don't parse args or enforce required params there any more - we should update the .required(...) call here to be correct.

(Assuming I'm reading the old bin/solr code correctly at least; always possible I'm wrong.)

https://issues.apache.org/jira/browse/SOLR-16882 for fixing this... I will investigate the .bats test...

gerlowskija · 2023-07-07T18:40:49Z

solr/core/src/java/org/apache/solr/cli/HealthcheckTool.java

@@ -60,11 +60,11 @@ public List<Option> getOptions() {
        SolrCLI.OPTION_SOLRURL,
        SolrCLI.OPTION_ZKHOST,
        Option.builder("c")
-            .argName("COLLECTION")
+            .longOpt("name")


[Q] I've picked HealthcheckTool's handling of its "collection" parameter as a concrete example, but the concern below applies in a number of places.

Historically, bin/solr healthcheck has accepted the collection name using either -c or -collection. But with this PR, AFAICT it now accepts -c and --collection (note the double-dash in the longopt).

Personally, I like the new syntax better - double-dashes on longopts are pretty ubiquitous in other CLI tools, and Solr's usage of -longoptName always struck me as quirky. That said though, it's a backwards incompatible change so we need to roll it out carefully.

If you do plan on committing some form of this to branch_9x, do you have plans for addressing backcompat? (i.e. Would the branch_9x version accept both --collection and -collection as longopt forms?)

If you don't plan on committing some form of this to branch_9x, things are better from a backcompat perspective but we'd still need to call out the syntax changes in CHANGES.txt or our Upgrade Notes, presumably?Do you have a general sense or (even better) a list of the syntax changes that'd need documented?

Thanks for jumping on this... https://issues.apache.org/jira/browse/SOLR-16824 was opened to specifically track that conversion to the --collection approach. I agree on the syntax changes needing tracking in Upgrade Notes. I might start with the Upgrade Notes for 10.x, but if we back port, then bring them back to that version of Upgrade Notes.

also... one more thing is that the long name is actually --name.. I was struggling with the CreateTool which used --name since it can do core or collection. Can't use --collection for a Core!

LIkewise the same thinking with the DeleteTool

epugh added 3 commits May 29, 2023 19:00

description tweaks from the bin/solr way of phrasing things.

adc5107

small format tweaks

cc1fb9a

delegate option parsing and help output to java..

66d8165

epugh marked this pull request as draft May 29, 2023 23:06

check point the progress, partial test_create_collection.bats working

22bc51c

sonatype-lift bot reviewed May 31, 2023

View reviewed changes

epugh added 11 commits June 6, 2023 07:17

fix formatting output

2b5dfbd

two ways of changing things, lets doc both..

1ced053

formatting cleanup

b9ff4cb

this check (and therefore error message) are used by multiple commands.

8ebc915

If you start with a zkhost option you might not have a solrUrl, so ju…

7aab1b9

…st tweak text.

Use name as long option in both logic paths for core and collection s…

69e2f0d

…o createTool works..

formatting

cd97b29

for cloud stuff we are now assuming -solrUrl, does this need rethinking?

e341743

fix message

d001aa4

no longer needed

6a915d3

Checking of parameters now happens in Java code.

69b5f20

sonatype-lift bot reviewed Jun 10, 2023

View reviewed changes

epugh added 2 commits June 10, 2023 10:29

pass bats tests with some cleanups

09b93dd

restore the behavior of defaulting to localhost:8983/solr if you don'…

70beb94

…t provide a solrurl...

sonatype-lift bot reviewed Jun 10, 2023

View reviewed changes

epugh added 8 commits June 10, 2023 11:44

migrate arg parsing logic to java code

c9ec13c

update for java based help output

23d9b92

port isn't used now that we have -solrUrl, same for scheme.

7111663

use the same param name as in create and delete commands to refer to …

f2d9329

…collection

fix build

4e1b85e

migrate parsing to java for bin/solr config

c86ff77

migrate parsing logic as much as possible into java...

7d7c813

lint

a9803fa

epugh added 8 commits June 22, 2023 08:42

description tweaks from the bin/solr way of phrasing things.

6d0370c

merging

46f44c5

merging

b2ff467

migrate parsing to java for bin/solr config

63bc914

lint

65d3007

move the top level help message into the Java code

b8067ce

update test to use new parameter names

bd20ee7

Merge branch 'main' into SOLR-16829

99d5bdd

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

ed8b55b

Willdotwhite and others added 3 commits June 28, 2023 22:17

Update solr.cmd status command output

e9a79c3

Original source: 718c5ef curtesy of @mkhludnev Manually copied ignoring existing fixes to `healthcheck`

Merge pull request #2 from Willdotwhite/SOLR-16829

d9f5425

Update solr.cmd for round #1 migration to SolrCLI

lint cleanups

f388842

epugh and others added 4 commits June 28, 2023 18:25

we can remove the help text from the cmd script since we use what com…

26168f5

…mons cli generates

Remove status_usage command from solr.cmd

c375ca2

Merge pull request #3 from Willdotwhite/feature/remove_status_usage_c…

78783d6

…ommands Remove status usage commands

restore the status command in unix

4269343

epugh added 2 commits July 6, 2023 12:42

back out copypasta ;-)

0364b4b

lint

00da571

epugh added 2 commits July 6, 2023 13:16

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

e3168d6

document changes

9137ea7

epugh merged commit fb6e879 into apache:main Jul 6, 2023
3 of 5 checks passed

gerlowskija reviewed Jul 7, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

SOLR-16829: Delegate arg parsing and help usage to java. (Round 1) #1670

SOLR-16829: Delegate arg parsing and help usage to java. (Round 1) #1670

epugh commented May 29, 2023 •

edited

Loading

sonatype-lift bot May 31, 2023

sonatype-lift bot Jun 10, 2023

sonatype-lift bot Jun 10, 2023

sonatype-lift bot Jun 10, 2023

epugh commented Jun 22, 2023

epugh commented Jun 23, 2023

epugh commented Jun 28, 2023

epugh commented Jun 28, 2023

epugh commented Jun 29, 2023

Willdotwhite commented Jun 30, 2023 •

edited

Loading

epugh commented Jul 6, 2023

gerlowskija left a comment

gerlowskija Jul 7, 2023

epugh Jul 8, 2023

gerlowskija Jul 7, 2023

epugh Jul 8, 2023

gerlowskija Jul 7, 2023

epugh Jul 8, 2023

epugh Jul 8, 2023

epugh Jul 8, 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

SOLR-16829: Delegate arg parsing and help usage to java. (Round 1) #1670

SOLR-16829: Delegate arg parsing and help usage to java. (Round 1) #1670

Conversation

epugh commented May 29, 2023 • edited Loading

Description

Solution

Tests

Checklist

sonatype-lift bot May 31, 2023

Choose a reason for hiding this comment

sonatype-lift bot Jun 10, 2023

Choose a reason for hiding this comment

sonatype-lift bot Jun 10, 2023

Choose a reason for hiding this comment

sonatype-lift bot Jun 10, 2023

Choose a reason for hiding this comment

epugh commented Jun 22, 2023

epugh commented Jun 23, 2023

epugh commented Jun 28, 2023

epugh commented Jun 28, 2023

epugh commented Jun 29, 2023

Willdotwhite commented Jun 30, 2023 • edited Loading

epugh commented Jul 6, 2023

gerlowskija left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

epugh commented May 29, 2023 •

edited

Loading

Willdotwhite commented Jun 30, 2023 •

edited

Loading