janhoy commented on code in PR #5063:
URL: https://github.com/apache/solr/pull/5063#discussion_r4380710186
##########
solr/core/src/java/org/apache/solr/cli/CliDefaultValueProvider.java:
##########
@@ -16,21 +16,36 @@
*/
package org.apache.solr.cli;
+import java.lang.reflect.Field;
import org.apache.solr.common.util.EnvUtils;
import picocli.CommandLine;
-/** Provides default values for CLI arguments. */
+/**
+ * Fills picocli options from the environment (solr.in.sh variables, system
properties) when they
+ * are not given on the command line, keyed by the option's long name.
+ */
public class CliDefaultValueProvider implements
CommandLine.IDefaultValueProvider {
+
@Override
public String defaultValue(CommandLine.Model.ArgSpec argSpec) throws
Exception {
- return switch (argSpec.paramLabel()) {
- case "<zkHost>" -> EnvUtils.getProperty("zkHost");
- case "<solrConnection>" -> EnvUtils.getProperty("solr.connection");
- case "<solrUrl>" -> EnvUtils.getProperty("solr.url");
+ if (!(argSpec instanceof CommandLine.Model.OptionSpec option)) {
+ return null;
+ }
+ return switch (option.longestName()) {
+ case "--zk-host" -> EnvUtils.getProperty("zkHost");
+ case "--solr-connection" -> EnvUtils.getProperty("solr.connection");
+ // Only the shared option means a base URL; a tool's own --solr-url
(ApiTool's full endpoint
+ // URL) must not be filled from SOLR_URL
+ case "--solr-url" ->
+ declaredIn(option, ConnectionOptions.class) ?
EnvUtils.getProperty("solr.url") : null;
Review Comment:
It is needed because the provider is now keyed by the option's long name,
and `--solr-url` is not unique: this mixin's is a base URL, while `ApiTool`
declares its own `required = true` `-s/--solr-url` meaning a full endpoint URL
— without the guard a `SOLR_URL` in `solr.in.sh` would satisfy that `required`
and `bin/solr api` with no arguments would silently GET the base URL. It wasn't
needed before only because the provider keyed on `paramLabel`, and `ApiTool`
happens to set one while the shared option didn't — which is the very bug this
PR fixes, since under that keying adding a `paramLabel` to any option silently
switched its environment fallback off. `declaredIn()` isn't extra reflection on
our side either: `OptionSpec.userObject()` is picocli's own model object for an
annotated field.
##########
solr/core/src/java/org/apache/solr/cli/CliDefaultValueProvider.java:
##########
@@ -16,21 +16,36 @@
*/
package org.apache.solr.cli;
+import java.lang.reflect.Field;
import org.apache.solr.common.util.EnvUtils;
import picocli.CommandLine;
-/** Provides default values for CLI arguments. */
+/**
+ * Fills picocli options from the environment (solr.in.sh variables, system
properties) when they
+ * are not given on the command line, keyed by the option's long name.
+ */
public class CliDefaultValueProvider implements
CommandLine.IDefaultValueProvider {
+
@Override
public String defaultValue(CommandLine.Model.ArgSpec argSpec) throws
Exception {
- return switch (argSpec.paramLabel()) {
- case "<zkHost>" -> EnvUtils.getProperty("zkHost");
- case "<solrConnection>" -> EnvUtils.getProperty("solr.connection");
- case "<solrUrl>" -> EnvUtils.getProperty("solr.url");
Review Comment:
`--solr-url` is a single Solr base URL and `--zk-host` is a ZooKeeper
quorum. `--solr-connection` is the universal string parsed by
`CloudSolrClient.CloudSolrClientConnection.parse()` — contains `://` means a
comma-separated list of Solr base URLs, anything else is a ZK quorum with
optional chroot — so it is a superset, and the only one of the three that can
name more than one host:
```
# ZK quorum, same as --zk-host
bin/solr package deploy mypkg:1.0 --collections c1 -s zk1:2181,zk2:2181/solr
# HTTP quorum of two nodes, which --solr-url cannot express
bin/solr package deploy mypkg:1.0 --collections c1 -s
http://node1:8983/solr,http://node2:8983/solr
```
For the package tool all three end up in the same place, since
`runWithManagers()` needs both a URL and a ZK host and resolves whichever is
missing. The env vars map one-to-one (`SOLR_CONNECTION`, `ZK_HOST`,
`SOLR_URL`), and the point of this PR is that all three now reach every tool.
--
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]
