SOLR-17549: v2 SolrRequest/SolrResponse should throw exception on error

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