dsmiley commented on code in PR #4914:
URL: https://github.com/apache/solr/pull/4914#discussion_r4027881690
##########
solr/solrj/src/java/org/apache/solr/client/solrj/response/InputStreamResponseParser.java:
##########
@@ -56,6 +57,32 @@ public static String
consumeResponseToString(NamedList<Object> response) throws
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
Review Comment:
this is very important; it needs to be on the class level javadoc,
##########
solr/solrj/src/java/org/apache/solr/client/solrj/response/InputStreamResponseParser.java:
##########
@@ -56,6 +57,32 @@ public static String
consumeResponseToString(NamedList<Object> response) throws
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)) {
+ String msg = String.format(Locale.ROOT, "Unexpected HTTP status [%d] in
response", httpStatus);
+ throw new IOException(detail == null ? msg : msg + ": " + detail);
Review Comment:
if we throw an exception, it's quite possible the response InputStream is
sitting there unclosed. There have been bugs about corner cases where this
stream is forgotten, tied to a HTTP connection. We have very thorough checks
in Solr to ensure we remember to close streams always. It's important that we
try to close that stream in case the caller hasn't already done something with
it.
If we want to get a little fancy & helpful, we could even try to read the
first say 1000 chars and log it at warn level. It's probably there's
interesting info in the payload as to *why* the request failed.
--
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]
