Copilot commented on code in PR #4513:
URL: https://github.com/apache/solr/pull/4513#discussion_r3436444894
##########
solr/core/src/java/org/apache/solr/cli/StreamTool.java:
##########
@@ -226,36 +240,58 @@ public void runImpl(CommandLine cli) throws Exception {
}
}
} finally {
- pushBackStream.close();
- solrClientCache.close();
+ if (pushBackStream != null) {
+ pushBackStream.close();
+ }
+ streamContext.getSolrClientCache().close();
}
echoIfVerbose("StreamTool -- Done.");
}
+ private StreamContext createStreamContext(CommandLine cli) throws Exception {
+ var jettyClientBuilder = new HttpJettySolrClient.Builder();
+ String credentials =
cli.getOptionValue(CommonCLIOptions.CREDENTIALS_OPTION);
+ if (credentials != null) {
+ String[] userPass = credentials.split(":");
+ jettyClientBuilder.withBasicAuthCredentials(userPass[0], userPass[1]);
+ }
Review Comment:
`--credentials` parsing via `split(":")` can throw (no colon) and also
breaks passwords containing ':' (only the 2nd segment is used). Prefer the
existing `withOptionalBasicAuthCredentials(credentials)` helper which validates
formatting and preserves everything after the first ':'.
##########
solr/core/src/java/org/apache/solr/cli/StreamTool.java:
##########
@@ -166,14 +164,30 @@ public void runImpl(CommandLine cli) throws Exception {
}
}
- PushBackStream pushBackStream;
- if (execution.equalsIgnoreCase("local")) {
- pushBackStream = doLocalMode(cli, expr);
- } else {
- pushBackStream = doRemoteMode(cli, expr);
+ // Validate inputs before opening any connection to Solr.
+ boolean local = execution.equalsIgnoreCase("local");
+ if (!local) {
+ if (!cli.hasOption(COLLECTION_OPTION)) {
+ throw new IllegalStateException(
+ "You must provide --name COLLECTION with --execution remote
parameter.");
+ }
+ if (expr.toLowerCase(Locale.ROOT).contains("stdin(")) {
+ throw new IllegalStateException(
+ "The stdin() expression is only usable with --worker local set
up.");
+ }
Review Comment:
The error message mentions a non-existent `--worker local` option; this tool
uses `--execution local`. This can confuse users when `stdin()` is used with
remote execution.
##########
solr/core/src/java/org/apache/solr/cli/StreamTool.java:
##########
@@ -226,36 +240,58 @@ public void runImpl(CommandLine cli) throws Exception {
}
}
} finally {
- pushBackStream.close();
- solrClientCache.close();
+ if (pushBackStream != null) {
+ pushBackStream.close();
+ }
+ streamContext.getSolrClientCache().close();
}
echoIfVerbose("StreamTool -- Done.");
}
+ private StreamContext createStreamContext(CommandLine cli) throws Exception {
+ var jettyClientBuilder = new HttpJettySolrClient.Builder();
+ String credentials =
cli.getOptionValue(CommonCLIOptions.CREDENTIALS_OPTION);
+ if (credentials != null) {
+ String[] userPass = credentials.split(":");
+ jettyClientBuilder.withBasicAuthCredentials(userPass[0], userPass[1]);
+ }
+ HttpJettySolrClient client = jettyClientBuilder.build();
+
+ // subclass so we can ensure our client is closed when the cache is closed
+ var solrClientCache =
+ new SolrClientCache(client) {
+ @Override
+ public synchronized void close() {
+ super.close();
+ client.close();
+ }
Review Comment:
`SolrClientCache.close()` is designed to be non-throwing (it uses
`IOUtils.closeQuietly`). Calling `client.close()` directly here can throw a
`RuntimeException` and potentially mask an earlier failure from the stream
execution. Use `IOUtils.closeQuietly(client)` in a `finally` to ensure cleanup
never overrides the primary exception.
##########
solr/core/src/java/org/apache/solr/cli/StreamTool.java:
##########
@@ -226,36 +240,58 @@ public void runImpl(CommandLine cli) throws Exception {
}
}
} finally {
- pushBackStream.close();
- solrClientCache.close();
+ if (pushBackStream != null) {
+ pushBackStream.close();
+ }
+ streamContext.getSolrClientCache().close();
}
echoIfVerbose("StreamTool -- Done.");
}
+ private StreamContext createStreamContext(CommandLine cli) throws Exception {
+ var jettyClientBuilder = new HttpJettySolrClient.Builder();
+ String credentials =
cli.getOptionValue(CommonCLIOptions.CREDENTIALS_OPTION);
+ if (credentials != null) {
+ String[] userPass = credentials.split(":");
+ jettyClientBuilder.withBasicAuthCredentials(userPass[0], userPass[1]);
+ }
+ HttpJettySolrClient client = jettyClientBuilder.build();
+
+ // subclass so we can ensure our client is closed when the cache is closed
+ var solrClientCache =
+ new SolrClientCache(client) {
+ @Override
+ public synchronized void close() {
+ super.close();
+ client.close();
+ }
+ };
+
+ var solrConnection = CLIUtils.getSolrConnection(cli);
+ echoIfVerbose("Connecting to Solr at " + solrConnection);
+
+ StreamContext streamContext = new StreamContext();
+ streamContext.setSolrClientCache(solrClientCache);
+
+ StreamFactory streamFactory = new DefaultStreamFactory();
+ streamFactory.withDefaultSolrConnection(solrConnection);
+ streamContext.setStreamFactory(streamFactory);
+ return streamContext;
Review Comment:
If anything throws after constructing `client`/`solrClientCache` (e.g.,
`CLIUtils.getSolrConnection(cli)` can throw), the current method will leak the
underlying Jetty client because the cache is only closed by the caller on the
success path. Wrap the remainder of the method in a try/catch and close the
cache on failure.
--
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]
