This is an automated email from the ASF dual-hosted git repository.
gerlowskija pushed a commit to branch branch_10x
in repository https://gitbox.apache.org/repos/asf/solr.git
The following commit(s) were added to refs/heads/branch_10x by this push:
new d977d7eac6f SOLR-17549: v2 SolrRequest/SolrResponse should throw
exception on error (#4183)
d977d7eac6f is described below
commit d977d7eac6f8785b7a4a7595fd94f33df46185ee
Author: Jason Gerlowski <[email protected]>
AuthorDate: Fri Mar 6 10:53:54 2026 -0500
SOLR-17549: v2 SolrRequest/SolrResponse should throw exception on error
(#4183)
This commit updates our v2 SolrJ code to throw RemoteSolrException when
error-responses are encountered.
It relies on a pre-existing attempt at error detection already present in
our
SolrClient implementations. Code in many of our clients already attempts to
detect these errors by looking for a 'NamedList' key, "error", which it
(incorrectly) assumed would be present in the v2 case but which isn't
populated by the ResponseParser that our v2 SolrRequest/SolrResponse
implementations use by default.
This commit updates JacksonDatabindResponseParser to populate this
"error" key that SolrClient's were looking for, which in turn enables the
clients to correctly throw RemoteSolrException in these cases.
---
.../unreleased/SOLR-17549-v2-error-reporting.yml | 7 ++
.../solr/filestore/TestDistribFileStore.java | 14 ++-
.../apache/solr/handler/V2ApiIntegrationTest.java | 42 +++++++
.../solr/handler/admin/api/RenameCoreAPITest.java | 26 ++--
.../json/JacksonDataBindResponseParser.java | 15 ++-
.../json/JacksonDataBindResponseParserTest.java | 133 +++++++++++++++++++++
6 files changed, 215 insertions(+), 22 deletions(-)
diff --git a/changelog/unreleased/SOLR-17549-v2-error-reporting.yml
b/changelog/unreleased/SOLR-17549-v2-error-reporting.yml
new file mode 100644
index 00000000000..ef5a0860447
--- /dev/null
+++ b/changelog/unreleased/SOLR-17549-v2-error-reporting.yml
@@ -0,0 +1,7 @@
+title: SOLR-17549 - v2 SolrRequest/SolrResponse classes now report errors
identically to their v1 counterparts
+type: fixed
+authors:
+ - name: Jason Gerlowski
+links:
+ - name: SOLR-17549
+ url: https://issues.apache.org/jira/browse/SOLR-17549
diff --git
a/solr/core/src/test/org/apache/solr/filestore/TestDistribFileStore.java
b/solr/core/src/test/org/apache/solr/filestore/TestDistribFileStore.java
index c30cd2e1fc1..ed017f58839 100644
--- a/solr/core/src/test/org/apache/solr/filestore/TestDistribFileStore.java
+++ b/solr/core/src/test/org/apache/solr/filestore/TestDistribFileStore.java
@@ -175,10 +175,16 @@ public class TestDistribFileStore extends
SolrCloudTestCase {
final var fetchReq = new
FileStoreApi.FetchFile("/package/mypkg/v1.0/runtimelibs.jar2");
fetchReq.setGetFrom("someFakeSolrNode:8983_solr");
try (final var solrClient = jettySolrRunner.newClient()) {
- final var asdf = fetchReq.process(solrClient);
- assertEquals(400, asdf.responseHeader.status);
- assertThat(asdf.error.msg, containsString("File store cannot fetch
from source node"));
- assertThat(asdf.error.msg, containsString("does not appear in
live-nodes"));
+ final var expectedExc =
+ expectThrows(
+ RemoteSolrException.class,
+ () -> {
+ fetchReq.process(solrClient);
+ });
+ assertEquals(400, expectedExc.code());
+ assertThat(
+ expectedExc.getMessage(), containsString("File store cannot
fetch from source node"));
+ assertThat(expectedExc.getMessage(), containsString("does not appear
in live-nodes"));
}
}
diff --git
a/solr/core/src/test/org/apache/solr/handler/V2ApiIntegrationTest.java
b/solr/core/src/test/org/apache/solr/handler/V2ApiIntegrationTest.java
index 4ef3c7f48df..d9a2f40aecb 100644
--- a/solr/core/src/test/org/apache/solr/handler/V2ApiIntegrationTest.java
+++ b/solr/core/src/test/org/apache/solr/handler/V2ApiIntegrationTest.java
@@ -17,6 +17,8 @@
package org.apache.solr.handler;
+import static org.hamcrest.Matchers.containsString;
+
import java.io.IOException;
import java.nio.file.Path;
import java.util.HashMap;
@@ -31,7 +33,10 @@ import org.apache.solr.client.solrj.SolrRequest;
import org.apache.solr.client.solrj.SolrServerException;
import org.apache.solr.client.solrj.apache.CloudLegacySolrClient;
import org.apache.solr.client.solrj.impl.CloudSolrClient;
+import org.apache.solr.client.solrj.jetty.HttpJettySolrClient;
import org.apache.solr.client.solrj.request.CollectionAdminRequest;
+import org.apache.solr.client.solrj.request.CollectionsApi;
+import org.apache.solr.client.solrj.request.GenericV2SolrRequest;
import org.apache.solr.client.solrj.request.V2Request;
import org.apache.solr.client.solrj.response.InputStreamResponseParser;
import org.apache.solr.client.solrj.response.JavaBinResponseParser;
@@ -257,6 +262,43 @@ public class V2ApiIntegrationTest extends
SolrCloudTestCase {
assertEquals(0, ((SolrDocumentList)
v2Response.getResponse().get("response")).getNumFound());
}
+ @Test
+ public void testV2ApiErrorHandling() throws Exception {
+ final var deleteRequest = new
CollectionsApi.DeleteCollection("collection-does-not-exist");
+
+ // Test with "Http" client
+ final String baseUrl =
cluster.getJettySolrRunner(0).getBaseUrl().toString();
+ try (var httpClient = new HttpJettySolrClient.Builder(baseUrl).build()) {
+ final var ex =
+ expectThrows(RemoteSolrException.class, () ->
deleteRequest.process(httpClient));
+ assertRSECodeAndMessage(ex, 400, "Could not find collection",
"collection-does-not-exist");
+ }
+
+ // Test with "Cloud" client
+ final var ex =
+ expectThrows(
+ RemoteSolrException.class, () ->
deleteRequest.process(cluster.getSolrClient()));
+ assertRSECodeAndMessage(ex, 400, "Could not find collection",
"collection-does-not-exist");
+
+ // Test with the less desirable Generic option to make sure exception is
equivalent.
+ final var genericDeleteRequest =
+ new GenericV2SolrRequest(SolrRequest.METHOD.DELETE,
"/collections/coll-does-not-exist");
+ final var ex2 =
+ expectThrows(
+ RemoteSolrException.class, () ->
genericDeleteRequest.process(cluster.getSolrClient()));
+ assertRSECodeAndMessage(ex2, 400, "Could not find collection",
"coll-does-not-exist");
+ }
+
+ private void assertRSECodeAndMessage(
+ RemoteSolrException rse, int expectedCode, String...
expectedMessagePieces) {
+ assertEquals(expectedCode, rse.code());
+ if (expectedMessagePieces != null) {
+ for (String expectedMessageSubStr : expectedMessagePieces) {
+ assertThat(rse.getMessage(), containsString(expectedMessageSubStr));
+ }
+ }
+ }
+
private Map<?, ?> resAsMap(CloudSolrClient client, V2Request request)
throws SolrServerException, IOException {
NamedList<Object> rsp = client.request(request);
diff --git
a/solr/core/src/test/org/apache/solr/handler/admin/api/RenameCoreAPITest.java
b/solr/core/src/test/org/apache/solr/handler/admin/api/RenameCoreAPITest.java
index 9e88889ac59..b999e2b8cf5 100644
---
a/solr/core/src/test/org/apache/solr/handler/admin/api/RenameCoreAPITest.java
+++
b/solr/core/src/test/org/apache/solr/handler/admin/api/RenameCoreAPITest.java
@@ -17,6 +17,7 @@
package org.apache.solr.handler.admin.api;
import static org.apache.solr.core.CoreContainer.ALLOW_PATHS_SYSPROP;
+import static org.hamcrest.Matchers.containsString;
import java.nio.charset.StandardCharsets;
import org.apache.solr.SolrTestCaseJ4;
@@ -25,7 +26,6 @@ import org.apache.solr.client.solrj.RemoteSolrException;
import org.apache.solr.client.solrj.SolrRequest;
import org.apache.solr.client.solrj.request.CoresApi;
import org.apache.solr.client.solrj.request.GenericV2SolrRequest;
-import org.apache.solr.common.SolrException;
import org.apache.solr.common.util.EnvUtils;
import org.apache.solr.util.ExternalPaths;
import org.apache.solr.util.SolrJettyTestRule;
@@ -34,19 +34,13 @@ import org.junit.ClassRule;
import org.junit.Test;
/**
- * The success case and the missing-parameter error case use the generated
{@link
- * org.apache.solr.client.solrj.request.CoresApi.RenameCore} request type,
asserting directly on
- * {@link SolrJerseyResponse#responseHeader} status and {@link
- * org.apache.solr.client.api.model.ErrorInfo#msg} because the generated typed
client uses {@code
- * JacksonDataBindResponseParser}, which does not yet propagate errors as
exceptions (SOLR-17549).
+ * Test cases for the v2 {@link RenameCore}
*
* <p>The missing-body error case uses {@link GenericV2SolrRequest} with an
explicit {@code "null"}
* JSON body, because {@link
org.apache.solr.client.solrj.request.CoresApi.RenameCore} always
* serializes a request body and therefore cannot represent a truly absent
body. The standard
* response parser used by {@link GenericV2SolrRequest} does propagate 4xx
responses as {@link
* RemoteSolrException}s.
- *
- * <p>TODO: Deal with this when generated code does properly throw an
exception!
*/
public class RenameCoreAPITest extends SolrTestCaseJ4 {
@@ -90,14 +84,14 @@ public class RenameCoreAPITest extends SolrTestCaseJ4 {
@Test
public void testMissingToParameterThrowsError() throws Exception {
- // CoresApi.RenameCore always serializes a body, so leaving setTo()
uncalled sends a body
- // whose "to" field is null. The handler's ensureRequiredParameterProvided
guard returns a 400,
- // which JacksonDataBindResponseParser surfaces via the response fields
rather than as an
- // exception (SOLR-17549), so we assert directly on responseHeader.status
and error.msg.
CoresApi.RenameCore renameRequest = new
CoresApi.RenameCore(DEFAULT_TEST_CORENAME);
- SolrJerseyResponse response =
renameRequest.process(solrTestRule.getAdminClient());
- assertEquals(SolrException.ErrorCode.BAD_REQUEST.code,
response.responseHeader.status);
- assertNotNull(response.error);
- assertTrue(response.error.msg.contains("Missing required parameter: to"));
+ final var ex =
+ expectThrows(
+ RemoteSolrException.class,
+ () -> {
+ renameRequest.process(solrTestRule.getAdminClient());
+ });
+ assertEquals(400, ex.code());
+ assertThat(ex.getMessage(), containsString("Missing required parameter:
to"));
}
}
diff --git
a/solr/solrj/src/java/org/apache/solr/client/solrj/response/json/JacksonDataBindResponseParser.java
b/solr/solrj/src/java/org/apache/solr/client/solrj/response/json/JacksonDataBindResponseParser.java
index 58f60c50773..954db55f10c 100644
---
a/solr/solrj/src/java/org/apache/solr/client/solrj/response/json/JacksonDataBindResponseParser.java
+++
b/solr/solrj/src/java/org/apache/solr/client/solrj/response/json/JacksonDataBindResponseParser.java
@@ -22,6 +22,8 @@ import java.io.InputStream;
import java.io.InputStreamReader;
import java.util.Collection;
import java.util.List;
+import java.util.Map;
+import org.apache.solr.client.api.model.SolrJerseyResponse;
import org.apache.solr.client.solrj.request.json.JacksonContentWriter;
import org.apache.solr.client.solrj.response.ResponseParser;
import org.apache.solr.common.util.NamedList;
@@ -55,7 +57,6 @@ public class JacksonDataBindResponseParser<T> extends
ResponseParser {
@Override
public NamedList<Object> processResponse(InputStream stream, String
encoding) throws IOException {
- // TODO SOLR-17549 for error handling, implying a significant
ResponseParser API change
// TODO generalize to CBOR, Smile, ...
var mapper = JacksonContentWriter.DEFAULT_MAPPER;
@@ -66,6 +67,16 @@ public class JacksonDataBindResponseParser<T> extends
ResponseParser {
parsedVal = mapper.readValue(new InputStreamReader(stream, encoding),
typeParam);
}
- return SimpleOrderedMap.of("response", parsedVal);
+ final var result = new SimpleOrderedMap<Object>();
+ result.add("response", parsedVal);
+
+ // Surface errors from V2 response POJOs to the top-level NamedList so that
+ // HttpSolrClientBase.processErrorsAndResponse() can detect and throw
RemoteSolrException,
+ // matching the exception-throwing behavior of V1 API requests.
+ if (parsedVal instanceof SolrJerseyResponse jerseyResponse &&
jerseyResponse.error != null) {
+ result.add("error", mapper.convertValue(jerseyResponse.error,
Map.class));
+ }
+
+ return result;
}
}
diff --git
a/solr/solrj/src/test/org/apache/solr/client/solrj/response/json/JacksonDataBindResponseParserTest.java
b/solr/solrj/src/test/org/apache/solr/client/solrj/response/json/JacksonDataBindResponseParserTest.java
new file mode 100644
index 00000000000..4136927d0b3
--- /dev/null
+++
b/solr/solrj/src/test/org/apache/solr/client/solrj/response/json/JacksonDataBindResponseParserTest.java
@@ -0,0 +1,133 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.solr.client.solrj.response.json;
+
+import static org.hamcrest.Matchers.instanceOf;
+
+import java.io.ByteArrayInputStream;
+import java.io.IOException;
+import java.nio.charset.StandardCharsets;
+import java.util.Map;
+import org.apache.solr.SolrTestCase;
+import org.apache.solr.client.api.model.SolrJerseyResponse;
+import org.apache.solr.common.util.NamedList;
+import org.junit.Test;
+
+/** Unit tests for {@link JacksonDataBindResponseParser} */
+public class JacksonDataBindResponseParserTest extends SolrTestCase {
+
+ @Test
+ public void testSuccessfulResponseHasNoErrorKey() throws IOException {
+ final var parser = new
JacksonDataBindResponseParser<>(SolrJerseyResponse.class);
+ final var json = "{\"responseHeader\":{\"status\":0,\"QTime\":5}}";
+
+ final NamedList<Object> result = parser.processResponse(toStream(json),
null);
+
+ assertNotNull(result.get("response"));
+ assertThat(result.get("response"), instanceOf(SolrJerseyResponse.class));
+ assertNull("No 'error' key expected on success", result.get("error"));
+ }
+
+ @Test
+ public void testErrorResponseSurfacesErrorAtTopLevel() throws IOException {
+ final var parser = new
JacksonDataBindResponseParser<>(SolrJerseyResponse.class);
+ final var json =
+ "{"
+ + "\"responseHeader\":{\"status\":400,\"QTime\":3},"
+ + "\"error\":{"
+ + " \"code\":400,"
+ + " \"errorClass\":\"org.apache.solr.common.SolrException\","
+ + " \"msg\":\"Bad collection name\""
+ + "}"
+ + "}";
+
+ final NamedList<Object> result = parser.processResponse(toStream(json),
null);
+
+ // The deserialized POJO is present under "response"
+ assertNotNull(result.get("response"));
+ assertThat(result.get("response"), instanceOf(SolrJerseyResponse.class));
+ final var responsePojo = (SolrJerseyResponse) result.get("response");
+
+ // Error is present in the deserialized POJO
+ assertNotNull("Error should be populated in the POJO", responsePojo.error);
+
+ // Error is also present on NL for clients to detect
+ assertNotNull(result.get("error"));
+ assertThat(result.get("error"), instanceOf(Map.class));
+ @SuppressWarnings("unchecked")
+ final var topLevelError = (Map<String, Object>) result.get("error");
+ assertEquals(400, topLevelError.get("code"));
+ assertEquals("org.apache.solr.common.SolrException",
topLevelError.get("errorClass"));
+ assertEquals("Bad collection name", topLevelError.get("msg"));
+ }
+
+ @Test
+ public void testErrorResponseIncludesMetadataAtTopLevel() throws IOException
{
+ final var parser = new
JacksonDataBindResponseParser<>(SolrJerseyResponse.class);
+ final var json =
+ """
+ {
+ "responseHeader":{
+ "status":500,
+ "QTime":1
+ },
+ "error": {
+ "code":500,
+ "errorClass": "org.apache.solr.common.SolrException",
+ "msg": "Internal error",
+ "metadata":{
+ "error-class": "org.apache.solr.common.SolrException",
+ "root-error-class": "java.lang.IllegalStateException"
+ }
+ }
+ }""";
+
+ final NamedList<Object> result = parser.processResponse(toStream(json),
null);
+
+ @SuppressWarnings("unchecked")
+ final var topLevelError = (Map<String, Object>) result.get("error");
+ assertNotNull(topLevelError);
+
+ @SuppressWarnings("unchecked")
+ final var metadata = (Map<String, Object>) topLevelError.get("metadata");
+ assertNotNull("Metadata must be present for RemoteSolrException to extract
it", metadata);
+ assertEquals("org.apache.solr.common.SolrException",
metadata.get("error-class"));
+ assertEquals("java.lang.IllegalStateException",
metadata.get("root-error-class"));
+ }
+
+ @Test
+ public void testNonJerseyResponseTypeIsUnaffected() throws IOException {
+ // A parser bound to a plain POJO (i.e. not SolrJerseyResponse) must not
add an "error" key
+ final var parser = new JacksonDataBindResponseParser<>(PlainPojo.class);
+ final var json = "{\"value\":\"hello\"}";
+
+ final NamedList<Object> result = parser.processResponse(toStream(json),
null);
+
+ assertNotNull(result.get("response"));
+ assertThat(result.get("response"), instanceOf(PlainPojo.class));
+ assertNull("Error extraction only occurs for SolrJerseyResponse types",
result.get("error"));
+ }
+
+ // Minimal POJO for the non-SolrJerseyResponse test above
+ public static class PlainPojo {
+ public String value;
+ }
+
+ private static ByteArrayInputStream toStream(String json) {
+ return new ByteArrayInputStream(json.getBytes(StandardCharsets.UTF_8));
+ }
+}
