SOLR-11178: Change error handling in AutoScalingHandler to be consistent w/ other APIs

solr/CHANGES.txt

@@ -616,6 +616,8 @@ Other Changes
 * SOLR-10033: When attempting to facet with facet.mincount=0 over points fields, raise mincount to 1
   and log a warning. (Steve Rowe)
 
+* SOLR-11178: Change error handling in AutoScalingHandler to be consistent w/ other APIs (noble)
+
 ==================  6.7.0 ==================
 
 Consult the LUCENE_CHANGES.txt file for additional, low level, changes in this release.

solr/core/src/java/org/apache/solr/cloud/autoscaling/AutoScalingHandler.java

@@ -21,6 +21,7 @@ import java.io.IOException;
 import java.lang.invoke.MethodHandles;
 import java.util.ArrayList;
 import java.util.Collection;
+import java.util.Collections;
 import java.util.HashMap;
 import java.util.List;
 import java.util.Map;
@@ -167,16 +168,31 @@ public class AutoScalingHandler extends RequestHandlerBase implements Permission
   private void handleSetClusterPolicy(SolrQueryRequest req, SolrQueryResponse rsp, CommandOperation op) throws KeeperException, InterruptedException, IOException {
     List clusterPolicy = (List) op.getCommandData();
     if (clusterPolicy == null || !(clusterPolicy instanceof List)) {
-      throw new SolrException(SolrException.ErrorCode.BAD_REQUEST, "A list of cluster policies was not found");
+      op.addError("A list of cluster policies was not found");
+      checkErr(op);
+    }
+
+    try {
+      zkSetClusterPolicy(container.getZkController().getZkStateReader(), clusterPolicy);
+    } catch (Exception e) {
+      log.warn("error persisting policies");
+      op.addError(e.getMessage());
+      checkErr(op);
+
     }
-    zkSetClusterPolicy(container.getZkController().getZkStateReader(), clusterPolicy);
     rsp.getValues().add("result", "success");
   }
 
+  private void checkErr(CommandOperation op) {
+    if (!op.hasError()) return;
+    throw new ApiBag.ExceptionWithErrObject(SolrException.ErrorCode.BAD_REQUEST, "Error in command payload", CommandOperation.captureErrors(Collections.singletonList(op)));
+  }
+
   private void handleSetClusterPreferences(SolrQueryRequest req, SolrQueryResponse rsp, CommandOperation op) throws KeeperException, InterruptedException, IOException {
     List preferences = (List) op.getCommandData();
     if (preferences == null || !(preferences instanceof List)) {
-      throw new SolrException(SolrException.ErrorCode.BAD_REQUEST, "A list of cluster preferences not found");
+      op.addError("A list of cluster preferences not found");
+      checkErr(op);
     }
     zkSetPreferences(container.getZkController().getZkStateReader(), preferences);
     rsp.getValues().add("result", "success");
@@ -185,15 +201,13 @@ public class AutoScalingHandler extends RequestHandlerBase implements Permission
   private void handleRemovePolicy(SolrQueryRequest req, SolrQueryResponse rsp, CommandOperation op) throws KeeperException, InterruptedException, IOException {
     String policyName = (String) op.getCommandData();
 
-    if (policyName.trim().length() == 0) {
+    if (op.hasError()) checkErr(op);
-      throw new SolrException(SolrException.ErrorCode.BAD_REQUEST, "The policy name cannot be empty");
-    }
     Map<String, Object> autoScalingConf = zkReadAutoScalingConf(container.getZkController().getZkStateReader());
     Map<String, Object> policies = (Map<String, Object>) autoScalingConf.get("policies");
     if (policies == null || !policies.containsKey(policyName)) {
-      throw new SolrException(SolrException.ErrorCode.BAD_REQUEST, "No policy exists with name: " + policyName);
+      op.addError("No policy exists with name: " + policyName);
     }
-
+    checkErr(op);
     zkSetPolicies(container.getZkController().getZkStateReader(), policyName, null);
     rsp.getValues().add("result", "success");
   }
@@ -203,11 +217,18 @@ public class AutoScalingHandler extends RequestHandlerBase implements Permission
     for (Map.Entry<String, Object> policy : policies.entrySet()) {
       String policyName = policy.getKey();
       if (policyName == null || policyName.trim().length() == 0) {
-        throw new SolrException(SolrException.ErrorCode.BAD_REQUEST, "The policy name cannot be null or empty");
+        op.addError("The policy name cannot be null or empty");
       }
     }
+    checkErr(op);
 
-    zkSetPolicies(container.getZkController().getZkStateReader(), null, policies);
+    try {
+      zkSetPolicies(container.getZkController().getZkStateReader(), null, policies);
+    } catch (Exception e) {
+      log.warn("error persisting policies", e);
+      op.addError(e.getMessage());
+      checkErr(op);
+    }
 
     rsp.getValues().add("result", "success");
   }

solr/core/src/java/org/apache/solr/servlet/ResponseUtils.java

@@ -15,15 +15,15 @@
  * limitations under the License.
  */
 package org.apache.solr.servlet;
-import org.apache.solr.api.ApiBag;
-import org.apache.solr.common.SolrException;
-import org.apache.solr.common.util.NamedList;
-import org.apache.solr.common.util.CommandOperation;
-import org.slf4j.Logger;
 
 import java.io.PrintWriter;
 import java.io.StringWriter;
 
+import org.apache.solr.api.ApiBag;
+import org.apache.solr.common.SolrException;
+import org.apache.solr.common.util.NamedList;
+import org.slf4j.Logger;
+
 /**
  * Response helper methods.
  */
@@ -52,7 +52,7 @@ public class ResponseUtils {
       info.add("metadata", errorMetadata);
       if (ex instanceof ApiBag.ExceptionWithErrObject) {
         ApiBag.ExceptionWithErrObject exception = (ApiBag.ExceptionWithErrObject) ex;
-        info.add(CommandOperation.ERR_MSGS, exception.getErrs() );
+        info.add("details", exception.getErrs() );
       }
     }
     

solr/core/src/test/org/apache/solr/cloud/autoscaling/AutoScalingHandlerTest.java

@@ -81,9 +81,11 @@ public class AutoScalingHandlerTest extends SolrCloudTestCase {
     try {
       solrClient.request(req);
       fail("Adding a policy with 'cores' attribute should not have succeeded.");
-    } catch (HttpSolrClient.RemoteSolrException e)  {
+    } catch (HttpSolrClient.RemoteExecutionException e)  {
+      String message = String.valueOf(Utils.getObjectByPath(e.getMetaData(), true, "error/details[0]/errorMessages[0]"));
+
       // expected
-      assertTrue(e.getMessage().contains("cores is only allowed in 'cluster-policy'"));
+      assertTrue(message.contains("cores is only allowed in 'cluster-policy'"));
     }
 
     setPolicyCommand =  "{'set-policy': {" +
@@ -175,6 +177,25 @@ public class AutoScalingHandlerTest extends SolrCloudTestCase {
     assertNotNull(clusterPolicy);
     assertEquals(3, clusterPolicy.size());
   }
+  public void testErrorHandling() throws Exception {
+    CloudSolrClient solrClient = cluster.getSolrClient();
+
+    String setClusterPolicyCommand = "{" +
+        " 'set-cluster-policy': [" +
+        "      {'cores':'<10', 'node':'#ANY'}," +
+        "      {'shard': '#EACH', 'node': '#ANY'}," +
+        "      {'nodeRole':'overseer', 'replica':0}" +
+        "    ]" +
+        "}";
+    try {
+      SolrRequest req = createAutoScalingRequest(SolrRequest.METHOD.POST, setClusterPolicyCommand);
+      solrClient.request(req);
+      fail("expect exception");
+    } catch (HttpSolrClient.RemoteExecutionException e) {
+      String message = String.valueOf(Utils.getObjectByPath(e.getMetaData(), true, "error/details[0]/errorMessages[0]"));
+      assertTrue(message.contains("replica is required in"));
+    }
+  }
 
   @Test
   public void testReadApi() throws Exception  {

solr/solrj/src/java/org/apache/solr/client/solrj/impl/HttpSolrClient.java

@@ -23,6 +23,7 @@ import java.lang.invoke.MethodHandles;
 import java.net.ConnectException;
 import java.net.SocketTimeoutException;
 import java.nio.charset.StandardCharsets;
+import java.util.Arrays;
 import java.util.Collection;
 import java.util.Collections;
 import java.util.Iterator;
@@ -498,6 +499,8 @@ public class HttpSolrClient extends SolrClient {
 
   }
 
+  private static final List<String> errPath = Arrays.asList("metadata", "error-class");//Utils.getObjectByPath(err, false,"metadata/error-class")
+
   protected NamedList<Object> executeMethod(HttpRequestBase method, final ResponseParser processor, final boolean isV2Api) throws SolrServerException {
     method.addHeader("User-Agent", AGENT);
  
@@ -596,11 +599,9 @@ public class HttpSolrClient extends SolrClient {
       } catch (Exception e) {
         throw new RemoteSolrException(baseUrl, httpStatus, e.getMessage(), e);
       }
-      if (isV2Api) {
+      Object error = rsp == null ? null : rsp.get("error");
-        Object err = rsp.get("error");
+      if (error != null && (isV2Api || String.valueOf(getObjectByPath(error, true, errPath)).endsWith("ExceptionWithErrObject"))) {
-        if (err != null) {
           throw RemoteExecutionException.create(baseUrl, rsp);
-        }
       }
       if (httpStatus != HttpStatus.SC_OK && !isV2Api) {
         NamedList<String> metadata = null;

solr/solrj/src/java/org/apache/solr/common/util/Utils.java

@@ -285,7 +285,7 @@ public class Utils {
 
   public static Object getObjectByPath(Object root, boolean onlyPrimitive, List<String> hierarchy) {
     if(root == null) return null;
-    if(!isMapLike(root)) throw new RuntimeException("must be a Map or NamedList");
+    if(!isMapLike(root)) return null;
     Object obj = root;
     for (int i = 0; i < hierarchy.size(); i++) {
       int idx = -1;