Copilot commented on code in PR #4267:
URL: https://github.com/apache/solr/pull/4267#discussion_r3044557996
##########
solr/core/src/java/org/apache/solr/cli/ConfigSetDownloadTool.java:
##########
@@ -74,36 +88,53 @@ public String getUsage() {
@Override
public void runImpl(CommandLine cli) throws Exception {
String zkHost = CLIUtils.getZkHost(cli);
-
String confName = cli.getOptionValue(CONF_NAME_OPTION);
String confDir = cli.getOptionValue(CONF_DIR_OPTION);
echoIfVerbose("\nConnecting to ZooKeeper at " + zkHost + " ...");
try (SolrZkClient zkClient = CLIUtils.getSolrZkClient(cli, zkHost)) {
- Path configSetPath = Path.of(confDir);
- // 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 = configSetPath.resolve("conf");
- }
- Files.createDirectories(configSetPath);
- echo(
- "Downloading configset "
- + confName
- + " from ZooKeeper at "
- + zkHost
- + " to directory "
- + configSetPath.toAbsolutePath());
-
- zkClient.downConfig(confName, configSetPath);
+ doDownconfig(zkClient, zkHost, confName, confDir);
} catch (Exception e) {
log.error("Could not complete downconfig operation for reason: ", e);
throw (e);
}
}
+ private void doDownconfig(SolrZkClient zkClient, String zkHost, String
confName, String confDir)
+ throws Exception {
+ Path configSetPath = Path.of(confDir);
+ // 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")) {
Review Comment:
`Path.endsWith("/conf")` only works for absolute paths; if `--conf-dir` is a
relative path that already ends in `conf`, this will evaluate false and append
another `conf`, creating `<dir>/conf/conf`. Use
`configSetPath.getFileName().toString().equals("conf")`, or
`configSetPath.endsWith("conf")` / `configSetPath.endsWith(Path.of("conf"))`
instead.
```suggestion
if (!configSetPath.endsWith("conf")) {
```
##########
solr/core/src/java/org/apache/solr/cli/ZkMvTool.java:
##########
@@ -70,37 +100,52 @@ public String getHeader() {
@Override
public void runImpl(CommandLine cli) throws Exception {
String zkHost = CLIUtils.getZkHost(cli);
+ String src = cli.getArgs()[0];
+ String dst = cli.getArgs()[1];
try (SolrZkClient zkClient = CLIUtils.getSolrZkClient(cli, zkHost)) {
- echoIfVerbose("\nConnecting to ZooKeeper at " + zkHost + " ...");
- String src = cli.getArgs()[0];
- String dst = cli.getArgs()[1];
-
- if (src.toLowerCase(Locale.ROOT).startsWith("file:")
- || dst.toLowerCase(Locale.ROOT).startsWith("file:")) {
- throw new SolrServerException(
- "mv command operates on znodes and 'file:' has been specified.");
- }
- String source = src;
- if (src.toLowerCase(Locale.ROOT).startsWith("zk")) {
- source = src.substring(3);
- }
-
- String dest = dst;
- if (dst.toLowerCase(Locale.ROOT).startsWith("zk")) {
- dest = dst.substring(3);
- }
-
- echo("Moving Znode " + source + " to " + dest + " on ZooKeeper at " +
zkHost);
- zkClient.moveZnode(source, dest);
+ doMv(zkClient, zkHost, src, dst);
} catch (Exception e) {
log.error("Could not complete mv operation for reason: ", e);
throw (e);
}
}
+ private void doMv(SolrZkClient zkClient, String zkHost, String src, String
dst) throws Exception {
+ echoIfVerbose("\nConnecting to ZooKeeper at " + zkHost + " ...");
+ if (src.toLowerCase(Locale.ROOT).startsWith("file:")
+ || dst.toLowerCase(Locale.ROOT).startsWith("file:")) {
+ throw new SolrServerException(
+ "mv command operates on znodes and 'file:' has been specified.");
+ }
+ String source = src;
+ if (src.toLowerCase(Locale.ROOT).startsWith("zk")) {
+ source = src.substring(3);
+ }
+
+ String dest = dst;
+ if (dst.toLowerCase(Locale.ROOT).startsWith("zk")) {
Review Comment:
The `zk` prefix stripping is too broad: `startsWith("zk")` will also match
paths like `zkfoo` and then `substring(3)` will corrupt the path (or throw).
This should check for the explicit `"zk:"` prefix before stripping.
```suggestion
if (src.toLowerCase(Locale.ROOT).startsWith("zk:")) {
source = src.substring(3);
}
String dest = dst;
if (dst.toLowerCase(Locale.ROOT).startsWith("zk:")) {
```
##########
solr/packaging/test/test_zk.bats:
##########
@@ -48,15 +48,18 @@ teardown() {
@test "long help" {
run solr zk -h
- assert_output --partial "bin/solr zk ls"
- assert_output --partial "bin/solr zk updateacls"
+ # TODO: Long help is different in picocli
+ #assert_output --partial "bin/solr zk ls"
+ #assert_output --partial "bin/solr zk updateacls"
assert_output --partial "Pass --help or -h after any COMMAND"
+ assert_output --partial "updateacls"
}
-@test "running subcommands with zk is prevented" {
- run solr ls / -z localhost:${ZK_PORT}
- assert_output --partial "You must invoke this subcommand using the zk command"
-}
+# TODO: Picocli fails in a different way when a subcommand is not invoked
using the zk command
+#@test "running subcommands with zk is prevented" {
+# run solr ls / -z localhost:${ZK_PORT}
+# assert_output --partial "You must invoke this subcommand using the zk
command"
+#}
Review Comment:
These assertions/tests are commented out, reducing coverage of `solr zk -h`
output and the “must be invoked using `zk`” behavior. Instead of disabling,
consider updating the expectations to match picocli output (or gating
assertions based on whether picocli mode is enabled) so the packaging tests
continue to validate the CLI contract.
##########
solr/core/src/test/org/apache/solr/cli/ZkSubcommandsPicocliTest.java:
##########
@@ -0,0 +1,46 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.solr.cli;
+
+import java.util.Arrays;
+
+/**
+ * Runs all {@link ZkSubcommandsTest} tests through the picocli invocation
path.
+ *
+ * <p>All {@code @Test} methods are inherited from {@link ZkSubcommandsTest}.
Only the tool
+ * invocation strategy is overridden here to use {@code
picocli.CommandLine.execute()} instead of
+ * the commons-cli path.
+ */
+public class ZkSubcommandsPicocliTest extends ZkSubcommandsTest {
+
+ @Override
+ protected int runTool(String[] args, Class<? extends ToolBase> clazz) throws
Exception {
+ // args[0] is the tool/subcommand name used by commons-cli dispatch; strip
it for picocli.
+ String[] toolArgs = Arrays.copyOfRange(args, 1, args.length);
+ ToolBase tool = clazz.getDeclaredConstructor().newInstance();
+ return new picocli.CommandLine(tool).execute(toolArgs);
+ }
+
+ @Override
+ protected int runTool(
+ String[] args, CLITestHelper.TestingRuntime runtime, Class<? extends
ToolBase> clazz)
+ throws Exception {
+ String[] toolArgs = Arrays.copyOfRange(args, 1, args.length);
+ ToolBase tool =
clazz.getDeclaredConstructor(ToolRuntime.class).newInstance(runtime);
+ return new picocli.CommandLine(tool).execute(toolArgs);
+ }
Review Comment:
The picocli test invocation doesn’t configure the same
`defaultValueProvider` as the real CLI (`SolrCLI` sets
`CliDefaultValueProvider`). Tests that rely on defaults (e.g., `testGetFile`
relies on `zkHost` system property instead of passing `-z`) will fail because
`zkOpts.zkHost` remains null and `resolveZkHost()` tries to contact the default
Solr URL. Set `new CommandLine(tool).getCommandSpec().defaultValueProvider(new
CliDefaultValueProvider())` (or equivalent) before `execute()` to mirror
production behavior.
##########
solr/core/src/test/org/apache/solr/cli/ZkSubcommandsTest.java:
##########
@@ -279,18 +310,18 @@ public void testPutFileNotExists() throws Exception {
"zk:/foo.xml"
};
- assertEquals(1, CLITestHelper.runTool(args, ZkCpTool.class));
+ assertEquals(1, runTool(args, ZkCpTool.class));
}
@Test
public void testLs() throws Exception {
zkClient.makePath("/test/path", true);
// test what happens when path arg "/" isn't the last one.
- String[] args = new String[] {"ls", "/", "-r", "true", "-z",
zkServer.getZkAddress()};
+ String[] args = new String[] {"ls", "-r", "-z", zkServer.getZkAddress(),
"/"};
Review Comment:
The comment says this test covers the case where the path argument isn’t
last, but the updated args now put the path last. Either restore the original
argument ordering to keep the intended coverage (and ensure both parsers handle
it), or update the comment/test name to match what’s actually being tested.
```suggestion
String[] args = new String[] {"ls", "/", "-r", "-z",
zkServer.getZkAddress()};
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
