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

changelog/unreleased/fix-solr-18439.yml

@@ -0,0 +1,11 @@
+title: The v2 replication file-list and snapshot backup APIs no longer return HTTP 200
+  for errors. Missing index generations return HTTP 409; file-list read failures and
+  unexpected snapshot backup failures return HTTP 500. The v2 standalone node-health API returns
+  HTTP 400 for negative maxGenerationLag and HTTP 503 when followers fail the lag check.
+  V1 responses are unchanged.
+type: changed
+authors:
+  - name: Eric Pugh
+links:
+  - name: SOLR-18439
+    url: https://issues.apache.org/jira/browse/SOLR-18439

dev-docs/apis-v2-conventions.adoc

@@ -72,11 +72,16 @@ For use within the v2 API, the four "popular" HTTP methods have the following se
 
 == Errors
 
+Successful synchronous requests normally return HTTP 200.
+Report failures using the appropriate HTTP 4xx or 5xx status; use HTTP 500 for unexpected server failures.
+Do not return HTTP 200 with an error indicated only by a response-body field such as `status: ERROR`.
+
 v2 APIs should be consistent in how they report errors.  Throwing a `SolrException` will convey
 1. The error code as the HTTP response status code, as `responseHeader.status` and as `error.code`, and
 2. The error message as `error.msg`.
 
 API calls that reference a specific resource (e.g. `specificCollName`, `specificAliasName`, `specificPropertyName` and others per the above list) that do not exist should return `SolrException.ErrorCode.NOT_FOUND` (HTTP 404).
+Use `SolrException.ErrorCode.CONFLICT` (HTTP 409) when the request conflicts with the current state, such as a replication request for an index generation that is no longer available.
 
 == Exceptional Cases - "Command" APIs
 

solr/core/src/java/org/apache/solr/handler/ReplicationHandler.java

@@ -112,6 +112,7 @@
 import org.apache.solr.handler.ReplicationHandler.ReplicationHandlerConfig;
 import org.apache.solr.handler.admin.api.CoreReplication;
 import org.apache.solr.handler.admin.api.ReplicationAPIBase;
+import org.apache.solr.handler.admin.api.ReplicationAPIBase.FileListException;
 import org.apache.solr.handler.admin.api.SnapshotBackupAPI;
 import org.apache.solr.handler.api.V2ApiUtils;
 import org.apache.solr.jersey.APIConfigProvider;
@@ -308,9 +309,22 @@ public void handleRequestBody(SolrQueryRequest req, SolrQueryResponse rsp) throw
       getFileStream(solrParams, rsp, req);
     } else if (command.equals(CMD_GET_FILE_LIST)) {
       final CoreReplication coreReplicationAPI = new CoreReplication(core, req, rsp);
-      V2ApiUtils.squashIntoSolrResponseWithoutHeader(
-          rsp,
-          coreReplicationAPI.fetchFileList(Long.parseLong(solrParams.required().get(GENERATION))));
+      try {
+        V2ApiUtils.squashIntoSolrResponseWithoutHeader(
+            rsp,
+            coreReplicationAPI.fetchFileList(
+                Long.parseLong(solrParams.required().get(GENERATION))));
+      } catch (FileListException e) {
+        rsp.add(STATUS, ERR_STATUS);
+        rsp.add(
+            "message",
+            e.code() == SolrException.ErrorCode.CONFLICT.code
+                ? "invalid index generation"
+                : e.getMessage());
+        if (e.getCause() != null) {
+          rsp.add("exception", e.getCause());
+        }
+      }
     } else if (command.equalsIgnoreCase(CMD_BACKUP)) {
       doSnapShoot(new ModifiableSolrParams(solrParams), rsp, req);
     } else if (command.equalsIgnoreCase(CMD_RESTORE)) {

solr/core/src/java/org/apache/solr/handler/admin/HealthCheckHandler.java

@@ -85,7 +85,8 @@ public void handleRequestBody(SolrQueryRequest req, SolrQueryResponse rsp) throw
         req.getParams().getInt(HealthCheckRequest.PARAM_MAX_GENERATION_LAG);
     try {
       V2ApiUtils.squashIntoSolrResponseWithoutHeader(
-          rsp, new NodeHealth(coreContainer).healthcheck(requireHealthyCores, maxGenerationLag));
+          rsp,
+          new NodeHealth(coreContainer).checkHealthStatus(requireHealthyCores, maxGenerationLag));
     } catch (SolrException e) {
       final NodeHealthResponse failureResponse = new NodeHealthResponse();
       failureResponse.status = NodeHealthResponse.NodeStatus.FAILURE;

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

@@ -19,6 +19,7 @@
 
 import static org.apache.solr.client.api.model.NodeHealthResponse.NodeStatus.FAILURE;
 import static org.apache.solr.client.api.model.NodeHealthResponse.NodeStatus.OK;
+import static org.apache.solr.common.SolrException.ErrorCode.BAD_REQUEST;
 import static org.apache.solr.common.SolrException.ErrorCode.SERVER_ERROR;
 import static org.apache.solr.common.SolrException.ErrorCode.SERVICE_UNAVAILABLE;
 import static org.apache.solr.handler.admin.api.ReplicationAPIBase.GENERATION;
@@ -74,6 +75,18 @@ public NodeHealth(CoreContainer coreContainer) {
   @Override
   @PermissionName(HEALTH_PERM)
   public NodeHealthResponse healthcheck(Boolean requireHealthyCores, Integer maxGenerationLag) {
+    final NodeHealthResponse response = checkHealthStatus(requireHealthyCores, maxGenerationLag);
+    if (response.status == FAILURE) {
+      throw new SolrException(
+          maxGenerationLag != null && maxGenerationLag < 0 ? BAD_REQUEST : SERVICE_UNAVAILABLE,
+          response.message);
+    }
+    return response;
+  }
+
+  /** Returns health data, retaining standalone failure results for the v1 adapter. */
+  public NodeHealthResponse checkHealthStatus(
+      Boolean requireHealthyCores, Integer maxGenerationLag) {
     if (coreContainer == null || coreContainer.isShutDown()) {
       throw new SolrException(
           SERVER_ERROR, "CoreContainer is either not initialized or shutting down");

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;
@@ -176,6 +175,13 @@ protected DirectoryFileStream doFetchFile(
     return dfs;
   }
 
+  /** A file-list failure that the v1 handler translates into its legacy response format. */
+  public static class FileListException extends SolrException {
+    private FileListException(ErrorCode code, String message, Exception cause) {
+      super(code, message, cause);
+    }
+  }
+
   protected FileListResponse getFileList(long generation, ReplicationHandler replicationHandler) {
     final IndexDeletionPolicyWrapper delPol = solrCore.getDeletionPolicy();
     final FileListResponse filesResponse = new FileListResponse();
@@ -196,8 +202,8 @@ 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 FileListException(
+              SolrException.ErrorCode.CONFLICT, "invalid index generation: " + generation, null);
         }
       }
       assert null != commit;
@@ -246,9 +252,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 FileListException(
+            SolrException.ErrorCode.SERVER_ERROR,
+            "unable to get file names for given index generation",
+            e);
       } finally {
         if (dir != null) {
           try {
@@ -649,13 +656,4 @@ private static Path resolveWithinOrForbidden(
     }
     return resolved.normalize();
   }
-
-  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/TestReplicationHandler.java

@@ -57,6 +57,8 @@
 import org.apache.solr.client.solrj.SolrServerException;
 import org.apache.solr.client.solrj.request.CoresApi;
 import org.apache.solr.client.solrj.request.GenericSolrRequest;
+import org.apache.solr.client.solrj.request.HealthCheckRequest;
+import org.apache.solr.client.solrj.request.NodeApi;
 import org.apache.solr.client.solrj.request.QueryRequest;
 import org.apache.solr.client.solrj.request.ReplicationApi;
 import org.apache.solr.client.solrj.request.SolrQuery;
@@ -1497,6 +1499,43 @@ public void testFileListShouldReportErrorsWhenTheyOccur() throws Exception {
     assertEquals("invalid index generation", resp.get("message"));
   }
 
+  @Test
+  public void testV2FileListShouldReportErrorsWhenTheyOccur() {
+    final var request = new ReplicationApi.FetchFileList(DEFAULT_TEST_CORENAME, -2L);
+    final var error = expectThrows(SolrException.class, () -> request.process(followerClient));
+    assertEquals(SolrException.ErrorCode.CONFLICT.code, error.code());
+    assertThat(error.getMessage(), containsString("invalid index generation: -2"));
+  }
+
+  @Test
+  public void testHealthCheckGenerationLagChangesOnlyV2Status() throws Exception {
+    invokeReplicationCommand(
+        buildUrl(followerJetty.getLocalPort()) + "/" + DEFAULT_TEST_CORENAME, "disablepoll");
+    index(leaderClient, "id", "1");
+    leaderClient.commit();
+    pullFromTo(leaderJetty, followerJetty);
+
+    final var v1 = new HealthCheckRequest();
+    v1.setMaxGenerationLag(0);
+    final var v2 = new NodeApi.Healthcheck();
+    v2.setMaxGenerationLag(0);
+    assertEquals("OK", v1.process(followerClient).getNodeStatus());
+    assertEquals("OK", v2.process(followerClient).status.name());
+
+    for (int i = 2; i <= 3; i++) {
+      index(leaderClient, "id", Integer.toString(i));
+      leaderClient.commit();
+    }
+
+    final var legacyResponse = v1.process(followerClient);
+    assertEquals(0, legacyResponse.getStatus());
+    assertEquals("FAILURE", legacyResponse.getNodeStatus());
+    assertThat(legacyResponse.getMessage(), containsString("Cores violating maxGenerationLag:0"));
+    final var error = expectThrows(SolrException.class, () -> v2.process(followerClient));
+    assertEquals(SolrException.ErrorCode.SERVICE_UNAVAILABLE.code, error.code());
+    assertThat(error.getMessage(), containsString("Cores violating maxGenerationLag:0"));
+  }
+
   @Test
   public void testFollowerRestartsWhenCommitExpiresBeforeFileDownload() throws Exception {
     invokeReplicationCommand(

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

@@ -22,7 +22,10 @@
 import static org.hamcrest.Matchers.containsString;
 
 import org.apache.solr.SolrTestCaseJ4;
+import org.apache.solr.client.solrj.RemoteSolrException;
+import org.apache.solr.client.solrj.request.HealthCheckRequest;
 import org.apache.solr.client.solrj.request.NodeApi;
+import org.apache.solr.common.SolrException;
 import org.apache.solr.util.SolrJettyTestRule;
 import org.junit.BeforeClass;
 import org.junit.ClassRule;
@@ -54,13 +57,24 @@ public void testWithoutMaxGenerationLagReturnsOk() throws Exception {
   }
 
   @Test
-  public void testWithNegativeMaxGenerationLagReturnsFailure() throws Exception {
+  public void testWithNegativeMaxGenerationLagReturnsBadRequest() {
     final var request = new NodeApi.Healthcheck();
     request.setMaxGenerationLag(-1);
+    final var error =
+        expectThrows(
+            RemoteSolrException.class, () -> request.process(solrTestRule.getAdminClient()));
+    assertEquals(SolrException.ErrorCode.BAD_REQUEST.code, error.code());
+    assertThat(error.getMessage(), containsString("Invalid value of maxGenerationLag:-1"));
+  }
+
+  @Test
+  public void testV1WithNegativeMaxGenerationLagReturnsFailure() throws Exception {
+    final var request = new HealthCheckRequest();
+    request.setMaxGenerationLag(-1);
     final var response = request.process(solrTestRule.getAdminClient());
 
-    assertNotNull(response);
-    assertEquals(FAILURE, response.status);
-    assertThat(response.message, containsString("Invalid value of maxGenerationLag"));
+    assertEquals(0, response.getStatus());
+    assertEquals(FAILURE.name(), response.getNodeStatus());
+    assertEquals("Invalid value of maxGenerationLag:-1", response.getMessage());
   }
 }

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

@@ -17,10 +17,11 @@
 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 +79,44 @@ 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 {
+
+    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);
@@ -89,7 +128,6 @@ private static class TrackingSnapshotBackupAPI extends SnapshotBackupAPI {
     private static final AtomicInteger numberToKeep = new AtomicInteger();
     private static final AtomicInteger numberBackupsToKeep = new AtomicInteger();
 
-    @Inject
     public TrackingSnapshotBackupAPI(
         SolrCore solrCore, ReplicationHandlerConfig replicationHandlerConfig) {
       super(solrCore, replicationHandlerConfig);