This is an automated email from the ASF dual-hosted git repository. dsmiley pushed a commit to branch branch_10x in repository https://gitbox.apache.org/repos/asf/solr.git
commit 5777608633038cc57693fa8622260ef54416b687 Author: Serhiy Bzhezytskyy <[email protected]> AuthorDate: Thu Sep 17 03:12:10 2026 +0300 SOLR-17316: fix ApiTool and PackageUtils to fail on non-2xx responses (#4914) `bin/solr api` and the package manager's collection-agnostic API calls now fail on a non-2xx response instead of printing it as if it had succeeded. InputStreamResponseParser now incorporates a "reason" with the status. A checkStatus() method helps callers handle errors. Co-authored-by: Claude Sonnet 5 <[email protected]> (cherry picked from commit 10a2cb044e0320f0b63be7917f2742f5fff5e361) --- .../SOLR-17316-input-stream-status-check.yml | 10 +++++ .../core/src/java/org/apache/solr/cli/ApiTool.java | 5 ++- .../solr/cloud/api/collections/SplitShardCmd.java | 1 + .../apache/solr/packagemanager/PackageUtils.java | 5 ++- .../client/solrj/io/stream/JSONTupleStream.java | 1 + .../solr/client/solrj/impl/NodeValueFetcher.java | 1 + .../solr/client/solrj/impl/HttpSolrClient.java | 2 +- .../solrj/response/InputStreamResponseParser.java | 45 ++++++++++++++++++++++ .../response/InputStreamResponseParserTest.java | 32 +++++++++++++++ 9 files changed, 99 insertions(+), 3 deletions(-) diff --git a/changelog/unreleased/SOLR-17316-input-stream-status-check.yml b/changelog/unreleased/SOLR-17316-input-stream-status-check.yml new file mode 100644 index 00000000000..245d6342cf8 --- /dev/null +++ b/changelog/unreleased/SOLR-17316-input-stream-status-check.yml @@ -0,0 +1,10 @@ +# See https://github.com/apache/solr/blob/main/dev-docs/changelog.adoc +title: > + `bin/solr api` and the package manager's collection-agnostic API calls now fail on a non-2xx + response instead of printing it as if it had succeeded. +type: fixed +authors: + - name: Serhiy Bzhezytskyy +links: + - name: SOLR-17316 + url: https://issues.apache.org/jira/browse/SOLR-17316 diff --git a/solr/core/src/java/org/apache/solr/cli/ApiTool.java b/solr/core/src/java/org/apache/solr/cli/ApiTool.java index a86bf1f545d..51b883d507d 100644 --- a/solr/core/src/java/org/apache/solr/cli/ApiTool.java +++ b/solr/core/src/java/org/apache/solr/cli/ApiTool.java @@ -95,7 +95,10 @@ public class ApiTool extends ToolBase { // Pass the server's JSON to the user as it came; parsing and re-serialising it here only // risks changing it. req.setResponseParser(new InputStreamResponseParser("json")); - return InputStreamResponseParser.consumeResponseToString(solrClient.request(req)); + var response = solrClient.request(req); + String body = InputStreamResponseParser.consumeResponseToString(response); + InputStreamResponseParser.checkHttpStatus(response, url + ": " + body); + return body; } } diff --git a/solr/core/src/java/org/apache/solr/cloud/api/collections/SplitShardCmd.java b/solr/core/src/java/org/apache/solr/cloud/api/collections/SplitShardCmd.java index 6740c5ba046..4787e81734c 100644 --- a/solr/core/src/java/org/apache/solr/cloud/api/collections/SplitShardCmd.java +++ b/solr/core/src/java/org/apache/solr/cloud/api/collections/SplitShardCmd.java @@ -883,6 +883,7 @@ public class SplitShardCmd implements CollApiCmds.CollectionApiCommand { NamedList<Object> resp = httpClient.requestWithBaseUrl(parentShardLeader.getBaseUrl(), req, null); + InputStreamResponseParser.checkHttpStatus(resp); var indexSizeRef = new AtomicReference<Double>(-1.0); var freeSizeRef = new AtomicReference<Double>(-1.0); diff --git a/solr/core/src/java/org/apache/solr/packagemanager/PackageUtils.java b/solr/core/src/java/org/apache/solr/packagemanager/PackageUtils.java index 804d636b4d5..c97bdab3123 100644 --- a/solr/core/src/java/org/apache/solr/packagemanager/PackageUtils.java +++ b/solr/core/src/java/org/apache/solr/packagemanager/PackageUtils.java @@ -168,7 +168,10 @@ public class PackageUtils { new GenericSolrRequest(SolrRequest.METHOD.GET, path, params) .setRequiresCollection(isCollectionApi); request.setResponseParser(new InputStreamResponseParser("json")); - return InputStreamResponseParser.consumeResponseToString(client.request(request)); + var response = client.request(request); + String body = InputStreamResponseParser.consumeResponseToString(response); + InputStreamResponseParser.checkHttpStatus(response, path + ": " + body); + return body; } catch (IOException | SolrServerException e) { throw new RuntimeException(e); } diff --git a/solr/solrj-streaming/src/java/org/apache/solr/client/solrj/io/stream/JSONTupleStream.java b/solr/solrj-streaming/src/java/org/apache/solr/client/solrj/io/stream/JSONTupleStream.java index 98f02cd274c..657d82e292d 100644 --- a/solr/solrj-streaming/src/java/org/apache/solr/client/solrj/io/stream/JSONTupleStream.java +++ b/solr/solrj-streaming/src/java/org/apache/solr/client/solrj/io/stream/JSONTupleStream.java @@ -55,6 +55,7 @@ public class JSONTupleStream implements TupleStreamParser { QueryRequest query = new QueryRequest(requestParams, SolrRequest.METHOD.POST); query.setResponseParser(new InputStreamResponseParser("json")); NamedList<Object> genericResponse = server.request(query); + InputStreamResponseParser.checkHttpStatus(genericResponse); InputStream stream = (InputStream) genericResponse.get(InputStreamResponseParser.STREAM_KEY); InputStreamReader reader = new InputStreamReader(stream, StandardCharsets.UTF_8); return new JSONTupleStream(reader); diff --git a/solr/solrj-zookeeper/src/java/org/apache/solr/client/solrj/impl/NodeValueFetcher.java b/solr/solrj-zookeeper/src/java/org/apache/solr/client/solrj/impl/NodeValueFetcher.java index 917eb59ceea..6a031c1bed1 100644 --- a/solr/solrj-zookeeper/src/java/org/apache/solr/client/solrj/impl/NodeValueFetcher.java +++ b/solr/solrj-zookeeper/src/java/org/apache/solr/client/solrj/impl/NodeValueFetcher.java @@ -181,6 +181,7 @@ public class NodeValueFetcher { String baseUrl = ctx.zkClientClusterStateProvider.getZkStateReader().getBaseUrlForNodeName(ctx.getNode()); NamedList<Object> response = ctx.httpSolrClient().requestWithBaseUrl(baseUrl, req, null); + InputStreamResponseParser.checkHttpStatus(response); // TODO come up with a better solution to stream this response instead of loading in memory try (InputStream prometheusStream = (InputStream) response.get(STREAM_KEY)) { diff --git a/solr/solrj/src/java/org/apache/solr/client/solrj/impl/HttpSolrClient.java b/solr/solrj/src/java/org/apache/solr/client/solrj/impl/HttpSolrClient.java index b756506a50e..4f8918f42cb 100644 --- a/solr/solrj/src/java/org/apache/solr/client/solrj/impl/HttpSolrClient.java +++ b/solr/solrj/src/java/org/apache/solr/client/solrj/impl/HttpSolrClient.java @@ -246,7 +246,7 @@ public abstract class HttpSolrClient extends SolrClient { // Only case where stream should not be closed shouldClose = false; // no processor specified, return raw stream - return InputStreamResponseParser.createInputStreamNamedList(httpStatus, is); + return InputStreamResponseParser.createInputStreamNamedList(httpStatus, responseReason, is); } NamedList<Object> rsp; diff --git a/solr/solrj/src/java/org/apache/solr/client/solrj/response/InputStreamResponseParser.java b/solr/solrj/src/java/org/apache/solr/client/solrj/response/InputStreamResponseParser.java index 77466ef1c26..6f35a319bcc 100644 --- a/solr/solrj/src/java/org/apache/solr/client/solrj/response/InputStreamResponseParser.java +++ b/solr/solrj/src/java/org/apache/solr/client/solrj/response/InputStreamResponseParser.java @@ -20,6 +20,7 @@ import java.io.ByteArrayOutputStream; import java.io.IOException; import java.io.InputStream; import java.nio.charset.StandardCharsets; +import java.util.Locale; import java.util.Set; import org.apache.solr.common.util.NamedList; import org.apache.solr.common.util.SimpleOrderedMap; @@ -33,6 +34,7 @@ public class InputStreamResponseParser extends ResponseParser { public static String STREAM_KEY = "stream"; public static String HTTP_STATUS_KEY = "responseStatus"; + public static String HTTP_REASON_KEY = "responseReason"; private final String writerType; @@ -56,6 +58,36 @@ public class InputStreamResponseParser extends ResponseParser { return output; } + /** + * Throws if the response's HTTP status was not 2xx. + * + * <p>{@code SolrClient}s skip their usual non-2xx check when an {@link + * InputStreamResponseParser} is in use, since the raw stream is handed back regardless of + * status. Callers that read the stream under {@link #STREAM_KEY} directly -- rather than via + * {@link #consumeResponseToString}, which does not check either -- should call this first. + */ + public static void checkHttpStatus(NamedList<Object> response) throws IOException { + checkHttpStatus(response, null); + } + + /** + * As {@link #checkHttpStatus(NamedList)}, appending {@code detail} to the exception message + * when the status is not 2xx -- e.g. the request URL, or a body already consumed for another + * purpose. + */ + public static void checkHttpStatus(NamedList<Object> response, String detail) + throws IOException { + Object status = response.get(HTTP_STATUS_KEY); + if (status instanceof Integer httpStatus && (httpStatus < 200 || httpStatus >= 300)) { + Object reason = response.get(HTTP_REASON_KEY); + String msg = + reason instanceof String r && !r.isEmpty() + ? String.format(Locale.ROOT, "Unexpected HTTP status [%d %s] in response", httpStatus, r) + : String.format(Locale.ROOT, "Unexpected HTTP status [%d] in response", httpStatus); + throw new IOException(detail == null ? msg : msg + ": " + detail); + } + } + @Override public String getWriterType() { return writerType; @@ -73,9 +105,22 @@ public class InputStreamResponseParser extends ResponseParser { public static NamedList<Object> createInputStreamNamedList( int httpStatus, InputStream inputStream) { + return createInputStreamNamedList(httpStatus, null, inputStream); + } + + /** + * As {@link #createInputStreamNamedList(int, InputStream)}, also recording the HTTP reason + * phrase (e.g. "Bad Request") under {@link #HTTP_REASON_KEY}, when known, so callers building + * an error message have more to go on than the bare status code. + */ + public static NamedList<Object> createInputStreamNamedList( + int httpStatus, String reason, InputStream inputStream) { final var nl = new SimpleOrderedMap<>(); nl.add(STREAM_KEY, inputStream); nl.add(HTTP_STATUS_KEY, httpStatus); + if (reason != null) { + nl.add(HTTP_REASON_KEY, reason); + } return nl; } } diff --git a/solr/solrj/src/test/org/apache/solr/client/solrj/response/InputStreamResponseParserTest.java b/solr/solrj/src/test/org/apache/solr/client/solrj/response/InputStreamResponseParserTest.java index 68eaaf8f861..18a7bb49495 100644 --- a/solr/solrj/src/test/org/apache/solr/client/solrj/response/InputStreamResponseParserTest.java +++ b/solr/solrj/src/test/org/apache/solr/client/solrj/response/InputStreamResponseParserTest.java @@ -96,6 +96,38 @@ public class InputStreamResponseParserTest extends SolrTestCaseJ4 { assertEquals("1234", String.valueOf(solrDocument.getFieldValue("id"))); } + @Test + public void testCheckHttpStatusThrowsOnNon2xx() throws Exception { + try (InputStream is = getResponse()) { + NamedList<Object> response = InputStreamResponseParser.createInputStreamNamedList(500, is); + IOException e = + assertThrows( + IOException.class, () -> InputStreamResponseParser.checkHttpStatus(response)); + assertTrue(e.getMessage().contains("500")); + } + } + + @Test + public void testCheckHttpStatusIncludesReasonWhenPresent() throws Exception { + try (InputStream is = getResponse()) { + NamedList<Object> response = + InputStreamResponseParser.createInputStreamNamedList(400, "Bad Request", is); + IOException e = + assertThrows( + IOException.class, () -> InputStreamResponseParser.checkHttpStatus(response)); + assertTrue(e.getMessage().contains("400")); + assertTrue(e.getMessage().contains("Bad Request")); + } + } + + @Test + public void testCheckHttpStatusAllows2xx() throws Exception { + try (InputStream is = getResponse()) { + NamedList<Object> response = InputStreamResponseParser.createInputStreamNamedList(200, is); + InputStreamResponseParser.checkHttpStatus(response); // should not throw + } + } + /** Parse response from java.io.InputStream. */ @Test public void testInputStreamResponse() throws Exception {
