From 9c6bb46a5f1ac650b9e9a989b7bd8323c844278d Mon Sep 17 00:00:00 2001 From: Eric Pugh Date: Tue, 25 Jul 2023 13:58:57 -0400 Subject: [PATCH 01/25] add bats test --- solr/packaging/test/test_create_collection.bats | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/solr/packaging/test/test_create_collection.bats b/solr/packaging/test/test_create_collection.bats index ff776609378..e283a9d00d7 100644 --- a/solr/packaging/test/test_create_collection.bats +++ b/solr/packaging/test/test_create_collection.bats @@ -41,13 +41,20 @@ teardown() { @test "create collection" { run solr create -c COLL_NAME assert_output --partial "Created collection 'COLL_NAME'" - assert_output --partial "assuming solrUrl is http://localhost:8983/solr" + assert_output --partial "assuming solrUrl is http://localhost:8983" } @test "create collection using solrUrl" { run solr create -c COLL_NAME -solrUrl http://localhost:8983/solr assert_output --partial "Created collection 'COLL_NAME'" - refute_output --partial "assuming solrUrl is http://localhost:8983/solr" + refute_output --partial "assuming solrUrl is http://localhost:8983" +} + +@test "create collection using legacy solrUrl" { + run solr create -c COLL_NAME -solrUrl http://localhost:8983 + assert_output --partial "Created collection 'COLL_NAME'" + assert_output --partial "The solrUrl parameter should only point to the root of Solr." + refute_output --partial "assuming solrUrl is http://localhost:8983" } @test "create collection using Zookeeper" { From 48adb632c17fea0e4691861fc14e8e89831a3b26 Mon Sep 17 00:00:00 2001 From: Eric Pugh Date: Tue, 25 Jul 2023 16:01:02 -0400 Subject: [PATCH 02/25] AssertionFailureException is only used by the AssertTool, not shared Exception --- solr/core/src/java/org/apache/solr/cli/AssertTool.java | 10 ++++++++-- solr/core/src/java/org/apache/solr/cli/SolrCLI.java | 6 ------ 2 files changed, 8 insertions(+), 8 deletions(-) diff --git a/solr/core/src/java/org/apache/solr/cli/AssertTool.java b/solr/core/src/java/org/apache/solr/cli/AssertTool.java index 014b19d123a..7036d75f831 100644 --- a/solr/core/src/java/org/apache/solr/cli/AssertTool.java +++ b/solr/core/src/java/org/apache/solr/cli/AssertTool.java @@ -348,11 +348,11 @@ public static String userForDir(Path pathToDir) { } } - private static int exitOrException(String msg) throws SolrCLI.AssertionFailureException { + private static int exitOrException(String msg) throws AssertionFailureException { if (useExitCode) { return 1; } else { - throw new SolrCLI.AssertionFailureException(message != null ? message : msg); + throw new AssertionFailureException(message != null ? message : msg); } } @@ -374,4 +374,10 @@ private static boolean runningSolrIsCloud(String url) throws Exception { return SolrCLI.isCloudMode(client); } } + + public static class AssertionFailureException extends Exception { + public AssertionFailureException(String message) { + super(message); + } + } } diff --git a/solr/core/src/java/org/apache/solr/cli/SolrCLI.java b/solr/core/src/java/org/apache/solr/cli/SolrCLI.java index e6ff82d21d4..38f92948c74 100755 --- a/solr/core/src/java/org/apache/solr/cli/SolrCLI.java +++ b/solr/core/src/java/org/apache/solr/cli/SolrCLI.java @@ -569,10 +569,4 @@ public static boolean isCloudMode(SolrClient solrClient) throws SolrServerExcept solrClient.request(new GenericSolrRequest(SolrRequest.METHOD.GET, SYSTEM_INFO_PATH)); return "solrcloud".equals(systemInfo.get("mode")); } - - public static class AssertionFailureException extends Exception { - public AssertionFailureException(String message) { - super(message); - } - } } From 42279c4c2e754d352af180ef286cc2d065a333d4 Mon Sep 17 00:00:00 2001 From: Eric Pugh Date: Tue, 25 Jul 2023 16:10:55 -0400 Subject: [PATCH 03/25] Add testing for resolving a solr url. --- .../src/java/org/apache/solr/cli/SolrCLI.java | 13 +++++++++++ ...olrCliUptimeTest.java => SolrCLITest.java} | 22 ++++++++++++++++++- 2 files changed, 34 insertions(+), 1 deletion(-) rename solr/core/src/test/org/apache/solr/cli/{SolrCliUptimeTest.java => SolrCLITest.java} (60%) diff --git a/solr/core/src/java/org/apache/solr/cli/SolrCLI.java b/solr/core/src/java/org/apache/solr/cli/SolrCLI.java index 38f92948c74..55f9aeab929 100755 --- a/solr/core/src/java/org/apache/solr/cli/SolrCLI.java +++ b/solr/core/src/java/org/apache/solr/cli/SolrCLI.java @@ -383,6 +383,10 @@ public static boolean exceptionIsAuthRelated(Exception exc) { } public static SolrClient getSolrClient(String solrUrl) { + // today we require all urls to end in /solr, however in the future we will need to support the /api url end point instead. + if (solrUrl.indexOf("/solr") == 0){ + solrUrl = solrUrl + "/solr"; + } return new Http2SolrClient.Builder(solrUrl).withMaxConnectionsPerHost(32).build(); } @@ -476,6 +480,15 @@ public static String resolveSolrUrl(CommandLine cli) throws Exception { } } } + else { + if (solrUrl.indexOf("/solr") > -1) { // warn users on the old legacy http://localhost:8983/solr pattern for Solr urls. + solrUrl = solrUrl.substring(0, solrUrl.indexOf("/solr")); + CLIO.out("The solrUrl parameter only needs to only point to the root of Solr (" + solrUrl + ")."); + } + if (solrUrl.endsWith("/")){ + solrUrl = solrUrl.substring(0,solrUrl.length()-1); + } + } return solrUrl; } diff --git a/solr/core/src/test/org/apache/solr/cli/SolrCliUptimeTest.java b/solr/core/src/test/org/apache/solr/cli/SolrCLITest.java similarity index 60% rename from solr/core/src/test/org/apache/solr/cli/SolrCliUptimeTest.java rename to solr/core/src/test/org/apache/solr/cli/SolrCLITest.java index 4589ea539ef..edb1157f34d 100644 --- a/solr/core/src/test/org/apache/solr/cli/SolrCliUptimeTest.java +++ b/solr/core/src/test/org/apache/solr/cli/SolrCLITest.java @@ -16,10 +16,30 @@ */ package org.apache.solr.cli; +import org.apache.commons.cli.CommandLine; import org.apache.solr.SolrTestCase; import org.junit.Test; -public class SolrCliUptimeTest extends SolrTestCase { +import static org.apache.solr.cli.SolrCLI.parseCmdLine; + +public class SolrCLITest extends SolrTestCase { + @Test + public void testResolveSolrUrl() throws Exception { + Tool testTool = new CreateTool(); + + String[] args = new String[] { "create", "-c","test", "-solrUrl", "http://localhost:8983/solr"}; + CommandLine cli = parseCmdLine(testTool.getName(), args, testTool.getOptions()); + assertEquals(SolrCLI.resolveSolrUrl(cli),"http://localhost:8983"); + + args = new String[] { "create", "-c","test", "-solrUrl", "http://localhost:8983"}; + cli = parseCmdLine(testTool.getName(), args, testTool.getOptions()); + assertEquals(SolrCLI.resolveSolrUrl(cli),"http://localhost:8983"); + + args = new String[] { "create", "-c","test", "-solrUrl", "http://localhost:8983/"}; + cli = parseCmdLine(testTool.getName(), args, testTool.getOptions()); + assertEquals(SolrCLI.resolveSolrUrl(cli),"http://localhost:8983"); + } + @Test public void testUptime() { assertEquals("?", SolrCLI.uptime(0)); From cd63cca24b6d9f710155dc6ac4cc20315187be6f Mon Sep 17 00:00:00 2001 From: Eric Pugh Date: Tue, 25 Jul 2023 17:16:45 -0400 Subject: [PATCH 04/25] simplify test --- .../src/java/org/apache/solr/cli/SolrCLI.java | 44 +++++++++++++------ .../test/org/apache/solr/cli/SolrCLITest.java | 21 ++------- 2 files changed, 35 insertions(+), 30 deletions(-) diff --git a/solr/core/src/java/org/apache/solr/cli/SolrCLI.java b/solr/core/src/java/org/apache/solr/cli/SolrCLI.java index 55f9aeab929..b4e6edd1979 100755 --- a/solr/core/src/java/org/apache/solr/cli/SolrCLI.java +++ b/solr/core/src/java/org/apache/solr/cli/SolrCLI.java @@ -383,8 +383,9 @@ public static boolean exceptionIsAuthRelated(Exception exc) { } public static SolrClient getSolrClient(String solrUrl) { - // today we require all urls to end in /solr, however in the future we will need to support the /api url end point instead. - if (solrUrl.indexOf("/solr") == 0){ + // today we require all urls to end in /solr, however in the future we will need to support the + // /api url end point instead. + if (!solrUrl.endsWith(("/solr"))) { solrUrl = solrUrl + "/solr"; } return new Http2SolrClient.Builder(solrUrl).withMaxConnectionsPerHost(32).build(); @@ -449,8 +450,31 @@ private static void printHelp() { print("Pass -help or -h after any COMMAND to see command-specific usage information,"); print("such as: ./solr start -help or ./solr stop -h"); } + /** - * Get the base URL of a live Solr instance from either the solrUrl command-line option from + * Strips off the end of solrUrl any /solr when a legacy solrUrl like http://localhost:8983/solr + * is used, and warns those users. In the future we'll have url's with /api as well. + * + * @param solrUrl with /solr stripped off. + * @return the truncated if need solrUrl. + */ + public static String resolveSolrUrl(String solrUrl) { + if (solrUrl != null) { + if (solrUrl.indexOf("/solr") > -1) { // + solrUrl = solrUrl.substring(0, solrUrl.indexOf("/solr")); + CLIO.out( + "The solrUrl parameter only needs to only point to the root of Solr (" + + solrUrl + + ")."); + } + if (solrUrl.endsWith("/")) { + solrUrl = solrUrl.substring(0, solrUrl.length() - 1); + } + } + return solrUrl; + } + /** + * Get the base URL of a live Solr instance from either the solrUrl command-line option or from * ZooKeeper. */ public static String resolveSolrUrl(CommandLine cli) throws Exception { @@ -479,15 +503,8 @@ public static String resolveSolrUrl(CommandLine cli) throws Exception { solrUrl = ZkStateReader.from(cloudSolrClient).getBaseUrlForNodeName(firstLiveNode); } } - } - else { - if (solrUrl.indexOf("/solr") > -1) { // warn users on the old legacy http://localhost:8983/solr pattern for Solr urls. - solrUrl = solrUrl.substring(0, solrUrl.indexOf("/solr")); - CLIO.out("The solrUrl parameter only needs to only point to the root of Solr (" + solrUrl + ")."); - } - if (solrUrl.endsWith("/")){ - solrUrl = solrUrl.substring(0,solrUrl.length()-1); - } + } else { + solrUrl = resolveSolrUrl(solrUrl); } return solrUrl; } @@ -502,7 +519,6 @@ public static String getZkHost(CommandLine cli) throws Exception { return zkHost; } - // find it using the localPort String solrUrl = cli.getOptionValue("solrUrl"); if (solrUrl == null) { solrUrl = DEFAULT_SOLR_URL; @@ -511,6 +527,8 @@ public static String getZkHost(CommandLine cli) throws Exception { "Neither -zkHost or -solrUrl parameters provided so assuming solrUrl is " + DEFAULT_SOLR_URL + "."); + } else { + solrUrl = resolveSolrUrl(solrUrl); } try (var solrClient = getSolrClient(solrUrl)) { diff --git a/solr/core/src/test/org/apache/solr/cli/SolrCLITest.java b/solr/core/src/test/org/apache/solr/cli/SolrCLITest.java index edb1157f34d..a0e0b4aac32 100644 --- a/solr/core/src/test/org/apache/solr/cli/SolrCLITest.java +++ b/solr/core/src/test/org/apache/solr/cli/SolrCLITest.java @@ -16,28 +16,15 @@ */ package org.apache.solr.cli; -import org.apache.commons.cli.CommandLine; import org.apache.solr.SolrTestCase; import org.junit.Test; -import static org.apache.solr.cli.SolrCLI.parseCmdLine; - public class SolrCLITest extends SolrTestCase { @Test - public void testResolveSolrUrl() throws Exception { - Tool testTool = new CreateTool(); - - String[] args = new String[] { "create", "-c","test", "-solrUrl", "http://localhost:8983/solr"}; - CommandLine cli = parseCmdLine(testTool.getName(), args, testTool.getOptions()); - assertEquals(SolrCLI.resolveSolrUrl(cli),"http://localhost:8983"); - - args = new String[] { "create", "-c","test", "-solrUrl", "http://localhost:8983"}; - cli = parseCmdLine(testTool.getName(), args, testTool.getOptions()); - assertEquals(SolrCLI.resolveSolrUrl(cli),"http://localhost:8983"); - - args = new String[] { "create", "-c","test", "-solrUrl", "http://localhost:8983/"}; - cli = parseCmdLine(testTool.getName(), args, testTool.getOptions()); - assertEquals(SolrCLI.resolveSolrUrl(cli),"http://localhost:8983"); + public void testResolveSolrUrl() { + assertEquals(SolrCLI.resolveSolrUrl("http://localhost:8983/solr"), "http://localhost:8983"); + assertEquals(SolrCLI.resolveSolrUrl("http://localhost:8983"), "http://localhost:8983"); + assertEquals(SolrCLI.resolveSolrUrl("http://localhost:8983/"), "http://localhost:8983"); } @Test From ade359a328b86c041cb71403b2300e9fafe56722 Mon Sep 17 00:00:00 2001 From: Eric Pugh Date: Tue, 25 Jul 2023 17:17:04 -0400 Subject: [PATCH 05/25] change up some of the urls. --- .../java/org/apache/solr/cli/AssertTool.java | 7 +++--- solr/packaging/test/test_assert.bats | 22 ++++++++++--------- 2 files changed, 15 insertions(+), 14 deletions(-) diff --git a/solr/core/src/java/org/apache/solr/cli/AssertTool.java b/solr/core/src/java/org/apache/solr/cli/AssertTool.java index 7036d75f831..21414145b5d 100644 --- a/solr/core/src/java/org/apache/solr/cli/AssertTool.java +++ b/solr/core/src/java/org/apache/solr/cli/AssertTool.java @@ -29,7 +29,6 @@ import org.apache.commons.cli.Option; import org.apache.solr.client.solrj.SolrClient; import org.apache.solr.client.solrj.SolrServerException; -import org.apache.solr.client.solrj.impl.Http2SolrClient; import org.apache.solr.client.solrj.request.HealthCheckRequest; import org.apache.solr.common.SolrException; import org.apache.solr.common.util.NamedList; @@ -198,10 +197,10 @@ protected int runAssert(CommandLine cli) throws Exception { ret += assertSolrNotRunning(cli.getOptionValue("S")); } if (cli.hasOption("c")) { - ret += assertSolrRunningInCloudMode(cli.getOptionValue("c")); + ret += assertSolrRunningInCloudMode(SolrCLI.resolveSolrUrl(cli.getOptionValue("c"))); } if (cli.hasOption("C")) { - ret += assertSolrNotRunningInCloudMode(cli.getOptionValue("C")); + ret += assertSolrNotRunningInCloudMode(SolrCLI.resolveSolrUrl(cli.getOptionValue("C"))); } return ret; } @@ -370,7 +369,7 @@ private static boolean isSolrRunningOn(String url) throws Exception { } private static boolean runningSolrIsCloud(String url) throws Exception { - try (final SolrClient client = new Http2SolrClient.Builder(url).build()) { + try (final SolrClient client = SolrCLI.getSolrClient(url)) { return SolrCLI.isCloudMode(client); } } diff --git a/solr/packaging/test/test_assert.bats b/solr/packaging/test/test_assert.bats index 991774add01..3fe126a99ce 100644 --- a/solr/packaging/test/test_assert.bats +++ b/solr/packaging/test/test_assert.bats @@ -30,24 +30,26 @@ teardown() { @test "assert for non cloud mode" { run solr start - + run solr assert --not-cloud http://localhost:8983/solr + assert_output --partial "The solrUrl parameter only needs to only point to the root of Solr (http://localhost:8983)." refute_output --partial "ERROR" - - run solr assert --cloud http://localhost:8983/solr + + run solr assert --cloud http://localhost:8983 assert_output --partial "ERROR: Solr is not running in cloud mode" - + run ! solr assert --cloud http://localhost:8983/solr -e } @test "assert for cloud mode" { - run solr start -c - - run solr assert --cloud http://localhost:8983/solr + run solr start -c + + run solr assert --cloud http://localhost:8983 refute_output --partial "ERROR" - + run solr assert --not-cloud http://localhost:8983/solr + assert_output --partial "The solrUrl parameter only needs to only point to the root of Solr (http://localhost:8983)." assert_output --partial "ERROR: Solr is not running in standalone mode" - - run ! solr assert --not-cloud http://localhost:8983/solr -e + + run ! solr assert --not-cloud http://localhost:8983 -e } From e5513eaec84d36943bdb158501fa9e40ce3220e3 Mon Sep 17 00:00:00 2001 From: Eric Pugh Date: Tue, 25 Jul 2023 17:17:43 -0400 Subject: [PATCH 06/25] changes to not need /solr, but allow it... --- .../src/java/org/apache/solr/cli/CreateTool.java | 2 +- .../org/apache/solr/cli/HealthcheckToolTest.java | 14 ++++++++++++++ solr/packaging/test/test_create_collection.bats | 10 +++++----- solr/packaging/test/test_delete_collection.bats | 8 ++++++++ 4 files changed, 28 insertions(+), 6 deletions(-) diff --git a/solr/core/src/java/org/apache/solr/cli/CreateTool.java b/solr/core/src/java/org/apache/solr/cli/CreateTool.java index e30a30f40bd..fd284196719 100644 --- a/solr/core/src/java/org/apache/solr/cli/CreateTool.java +++ b/solr/core/src/java/org/apache/solr/cli/CreateTool.java @@ -117,7 +117,7 @@ public List