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.

Author: gerlowskija

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

solr/core/src/test/org/apache/solr/filestore/TestDistribFileStore.java

@@ -175,10 +175,16 @@ public void testFileStoreManagement() throws Exception {
         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"));
         }
       }
 

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.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 void testSelect() throws Exception {
     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);

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.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.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 void testMissingRequestBodyThrowsError() {
 
   @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"));
   }
 }

solr/solrj/src/java/org/apache/solr/client/solrj/response/json/JacksonDataBindResponseParser.java

@@ -22,6 +22,8 @@
 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 Collection<String> getContentTypes() {
 
   @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 NamedList<Object> processResponse(InputStream stream, String encoding) th
       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;
   }
 }

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));
+  }
+}