Copilot commented on code in PR #4695:
URL: https://github.com/apache/solr/pull/4695#discussion_r3707253782
##########
solr/core/src/java/org/apache/solr/cli/StreamTool.java:
##########
@@ -137,38 +154,46 @@ public Options getOptions() {
}
@Override
- @SuppressWarnings({"rawtypes"})
public void runImpl(CommandLine cli) throws Exception {
+ StreamParams params =
+ new StreamParams(
+ cli.getArgs(),
+ cli.getOptionValue(EXECUTION_OPTION, "remote"),
+ cli.getOptionValue(ARRAY_DELIMITER_OPTION, "|"),
+ cli.getOptionValue(DELIMITER_OPTION, " "),
+ cli.hasOption(HEADER_OPTION),
+ cli.getOptionValue(FIELDS_OPTION),
+ cli.getOptionValue(COLLECTION_OPTION),
+ cli.getOptionValue(CommonCLIOptions.CREDENTIALS_OPTION));
+
+ String expr = readExpressionFromArgs(params.args());
+ echoIfVerbose("Running Expression: " + expr);
- String expressionArgument = cli.getArgs()[0];
- String execution = cli.getOptionValue(EXECUTION_OPTION, "remote");
- String arrayDelimiter = cli.getOptionValue(ARRAY_DELIMITER_OPTION, "|");
- String delimiter = cli.getOptionValue(DELIMITER_OPTION, " ");
- boolean includeHeaders = cli.hasOption(HEADER_OPTION);
- String[] outputHeaders = getOutputFields(cli);
+ // Validate inputs before opening any connection to Solr.
+ boolean local = params.execution().equalsIgnoreCase("local");
+ validateExpressionArgs(local, params.collection(), expr);
- LineNumberReader bufferedReader = null;
- String expr;
- try {
- Reader inputStream =
- expressionArgument.toLowerCase(Locale.ROOT).endsWith(".expr")
- ? new InputStreamReader(
- new FileInputStream(expressionArgument),
Charset.defaultCharset())
- : new StringReader(expressionArgument);
-
- bufferedReader = new LineNumberReader(inputStream);
- expr = StreamTool.readExpression(bufferedReader, cli.getArgs());
- echoIfVerbose("Running Expression: " + expr);
- } finally {
- if (bufferedReader != null) {
- bufferedReader.close();
- }
+ var solrConnection = CLIUtils.getSolrConnection(cli);
+ String solrUrl = local ? null : CLIUtils.normalizeSolrUrl(cli);
+
+ runStream(params, expr, solrConnection, solrUrl);
+ }
+
+ static String readExpressionFromArgs(String[] args) throws IOException {
+ String expressionArgument = args[0];
+ try (LineNumberReader bufferedReader =
Review Comment:
`readExpressionFromArgs` assumes at least one positional arg and will throw
`ArrayIndexOutOfBoundsException` with an unhelpful stack trace when the tool is
invoked without an expression argument (e.g. `bin/solr stream` with no args).
Add an explicit length check and throw a clear `IllegalArgumentException`
instead.
##########
solr/core/src/java/org/apache/solr/cli/StreamTool.java:
##########
@@ -137,38 +154,46 @@ public Options getOptions() {
}
@Override
- @SuppressWarnings({"rawtypes"})
public void runImpl(CommandLine cli) throws Exception {
+ StreamParams params =
+ new StreamParams(
+ cli.getArgs(),
+ cli.getOptionValue(EXECUTION_OPTION, "remote"),
+ cli.getOptionValue(ARRAY_DELIMITER_OPTION, "|"),
+ cli.getOptionValue(DELIMITER_OPTION, " "),
+ cli.hasOption(HEADER_OPTION),
+ cli.getOptionValue(FIELDS_OPTION),
+ cli.getOptionValue(COLLECTION_OPTION),
+ cli.getOptionValue(CommonCLIOptions.CREDENTIALS_OPTION));
+
+ String expr = readExpressionFromArgs(params.args());
+ echoIfVerbose("Running Expression: " + expr);
- String expressionArgument = cli.getArgs()[0];
- String execution = cli.getOptionValue(EXECUTION_OPTION, "remote");
- String arrayDelimiter = cli.getOptionValue(ARRAY_DELIMITER_OPTION, "|");
- String delimiter = cli.getOptionValue(DELIMITER_OPTION, " ");
- boolean includeHeaders = cli.hasOption(HEADER_OPTION);
- String[] outputHeaders = getOutputFields(cli);
+ // Validate inputs before opening any connection to Solr.
+ boolean local = params.execution().equalsIgnoreCase("local");
+ validateExpressionArgs(local, params.collection(), expr);
- LineNumberReader bufferedReader = null;
- String expr;
- try {
- Reader inputStream =
- expressionArgument.toLowerCase(Locale.ROOT).endsWith(".expr")
- ? new InputStreamReader(
- new FileInputStream(expressionArgument),
Charset.defaultCharset())
- : new StringReader(expressionArgument);
-
- bufferedReader = new LineNumberReader(inputStream);
- expr = StreamTool.readExpression(bufferedReader, cli.getArgs());
- echoIfVerbose("Running Expression: " + expr);
- } finally {
- if (bufferedReader != null) {
- bufferedReader.close();
- }
+ var solrConnection = CLIUtils.getSolrConnection(cli);
+ String solrUrl = local ? null : CLIUtils.normalizeSolrUrl(cli);
+
Review Comment:
`CLIUtils.getSolrConnection(cli)` can return `null` when the user doesn't
supply `--solr-connection/--zk-host/--solr-url` (it does not fall back to the
default URL). Passing that null into `createStreamContext(..)` ends up calling
`streamFactory.withDefaultSolrConnection(solrConnection)` and will NPE.
Consider falling back to parsing the resolved base URL into a
`CloudSolrClientConnection` when `solrConnection` is null.
--
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]
