SOLR-18439: v2 APIs use 200 on success and 500 with a error.msg key

changelog/unreleased/fix-solr-18439-schema-api.yml

@@ -0,0 +1,7 @@
+title: Fix v2 schema modification APIs reporting failures via HTTP 200 instead of a proper error status
+type: fixed
+authors:
+  - name: Eric Pugh
+links:
+  - name: SOLR-18439
+    url: https://issues.apache.org/jira/browse/SOLR-18439

changelog/unreleased/fix-solr-18439.yml

@@ -0,0 +1,7 @@
+title: Fix v2 replication file-list and backup APIs reporting failures via HTTP 200 instead of a proper error status
+type: fixed
+authors:
+  - name: Eric Pugh
+links:
+  - name: SOLR-18439
+    url: https://issues.apache.org/jira/browse/SOLR-18439

solr/core/src/java/org/apache/solr/handler/admin/api/ReplicationAPIBase.java

@@ -16,7 +16,6 @@
  */
 package org.apache.solr.handler.admin.api;
 
-import static org.apache.solr.handler.ReplicationHandler.ERR_STATUS;
 import static org.apache.solr.handler.ReplicationHandler.OK_STATUS;
 
 import jakarta.ws.rs.core.StreamingOutput;
@@ -150,8 +149,7 @@ protected FileListResponse getFileList(long generation, ReplicationHandler repli
         }
         if (null == commit) {
           // The gen they asked for either doesn't exist or has already been deleted
-          reportErrorOnResponse(filesResponse, "invalid index generation", null);
-          return filesResponse;
+          throw new SolrException(SolrException.ErrorCode.NOT_FOUND, "invalid index generation");
         }
       }
       assert null != commit;
@@ -200,9 +198,10 @@ protected FileListResponse getFileList(long generation, ReplicationHandler repli
       } catch (IOException e) {
         log.error(
             "Unable to get file names for indexCommit generation: {}", commit.getGeneration(), e);
-        reportErrorOnResponse(
-            filesResponse, "unable to get file names for given index generation", e);
-        return filesResponse;
+        throw new SolrException(
+            SolrException.ErrorCode.SERVER_ERROR,
+            "unable to get file names for given index generation",
+            e);
       } finally {
         if (dir != null) {
           try {
@@ -579,13 +578,4 @@ protected Path initFile() {
       return solrCore.getResourceLoader().getConfigPath().resolve(cfileName);
     }
   }
-
-  private void reportErrorOnResponse(
-      FileListResponse fileListResponse, String message, Exception e) {
-    fileListResponse.status = ERR_STATUS;
-    fileListResponse.message = message;
-    if (e != null) {
-      fileListResponse.exception = e;
-    }
-  }
 }

solr/core/src/java/org/apache/solr/handler/admin/api/SnapshotBackupAPI.java

@@ -16,13 +16,11 @@
  */
 package org.apache.solr.handler.admin.api;
 
-import static org.apache.solr.handler.ReplicationHandler.ERR_STATUS;
 import static org.apache.solr.security.PermissionNameProvider.Name.CORE_EDIT_PERM;
 
 import io.swagger.v3.oas.annotations.parameters.RequestBody;
 import jakarta.inject.Inject;
 import java.io.IOException;
-import java.lang.invoke.MethodHandles;
 import java.util.function.Consumer;
 import org.apache.solr.api.JerseyResource;
 import org.apache.solr.client.api.endpoint.ReplicationBackupApis;
@@ -34,8 +32,6 @@
 import org.apache.solr.handler.ReplicationHandler;
 import org.apache.solr.handler.ReplicationHandler.ReplicationHandlerConfig;
 import org.apache.solr.jersey.PermissionName;
-import org.slf4j.Logger;
-import org.slf4j.LoggerFactory;
 
 /**
  * v2 API implementation for replication-handler based backup creation.
@@ -44,7 +40,6 @@
  */
 public class SnapshotBackupAPI extends JerseyResource implements ReplicationBackupApis {
 
-  private static final Logger log = LoggerFactory.getLogger(MethodHandles.lookup().lookupClass());
   private final SolrCore solrCore;
   private final ReplicationHandlerConfig replicationHandlerConfig;
 
@@ -86,14 +81,15 @@ private ReplicationBackupResponse doBackup(
           name,
           solrCore,
           resultConsumer);
-      response.status = ReplicationHandler.OK_STATUS;
     } catch (SolrException e) {
       throw e;
     } catch (Exception e) {
-      log.error("Exception while creating a snapshot", e);
-      reportErrorOnResponse(
-          response, "Error encountered while creating a snapshot: " + e.getMessage(), e);
+      throw new SolrException(
+          SolrException.ErrorCode.SERVER_ERROR,
+          "Error encountered while creating a snapshot: " + e.getMessage(),
+          e);
     }
+    response.status = ReplicationHandler.OK_STATUS;
     return response;
   }
 
@@ -118,13 +114,4 @@ protected void doSnapShoot(
         solrCore,
         resultConsumer);
   }
-
-  private static void reportErrorOnResponse(
-      ReplicationBackupResponse response, String message, Exception e) {
-    response.status = ERR_STATUS;
-    response.message = message;
-    if (e != null) {
-      response.exception = e;
-    }
-  }
 }

solr/core/src/test/org/apache/solr/handler/admin/api/SnapshotBackupAPITest.java

@@ -17,10 +17,12 @@
 package org.apache.solr.handler.admin.api;
 
 import static org.apache.solr.SolrTestCaseJ4.assumeWorkingMockito;
+import static org.hamcrest.Matchers.containsString;
 import static org.mockito.Mockito.mock;
 import static org.mockito.Mockito.when;
 
 import jakarta.inject.Inject;
+import java.io.IOException;
 import java.util.concurrent.atomic.AtomicInteger;
 import java.util.function.Consumer;
 import org.apache.solr.SolrTestCase;
@@ -78,6 +80,45 @@ public void testSuccessfulBackupCommand() throws Exception {
     assertEquals(11, TrackingSnapshotBackupAPI.numberBackupsToKeep.get());
   }
 
+  @Test
+  public void testBackupFailureThrowsInsteadOfReportingErrorOnResponse() {
+    final var backupRequestBody = new ReplicationBackupRequestBody();
+    backupRequestBody.name = "test";
+    backupRequestBody.numberToKeep = 1;
+
+    final var thrown =
+        expectThrows(
+            SolrException.class,
+            () ->
+                new FailingSnapshotBackupAPI(solrCore, replicationHandlerConfig)
+                    .createBackup(backupRequestBody));
+    assertEquals(500, thrown.code());
+    assertThat(thrown.getMessage(), containsString("disk is full"));
+  }
+
+  private static class FailingSnapshotBackupAPI extends SnapshotBackupAPI {
+
+    @Inject
+    public FailingSnapshotBackupAPI(
+        SolrCore solrCore, ReplicationHandlerConfig replicationHandlerConfig) {
+      super(solrCore, replicationHandlerConfig);
+    }
+
+    @Override
+    protected void doSnapShoot(
+        int numberToKeep,
+        int numberBackupsToKeep,
+        String location,
+        String repoName,
+        String commitName,
+        String name,
+        SolrCore solrCore,
+        Consumer<NamedList<?>> resultConsumer)
+        throws IOException {
+      throw new IOException("disk is full");
+    }
+  }
+
   private void resetMocks() {
     solrCore = mock(SolrCore.class);
     replicationHandlerConfig = mock(ReplicationHandlerConfig.class);