Address code review comments from PR 123618

Author: JeremyDahlgren

rest-api-spec/src/yamlRestTest/resources/rest-api-spec/test/snapshot.get/10_basic.yml

@@ -303,3 +303,64 @@ setup:
       snapshot.delete:
         repository: test_repo_get_1
         snapshot: test_snapshot_no_repo_name
+
+---
+"Get snapshot using state parameter":
+
+  - do:
+      indices.create:
+        index: test_index
+        body:
+          settings:
+            number_of_shards: 1
+            number_of_replicas: 0
+
+  - do:
+      snapshot.create:
+        repository: test_repo_get_1
+        snapshot: test_snapshot_with_state_param
+        wait_for_completion: true
+
+  - do:
+      snapshot.get:
+        repository: test_repo_get_1
+        snapshot: test_snapshot_with_state_param
+        state: SUCCESS
+
+  - is_true: snapshots
+  - match: { snapshots.0.snapshot: test_snapshot_with_state_param }
+  - match: { snapshots.0.state: SUCCESS }
+
+  - do:
+      snapshot.get:
+        repository: test_repo_get_1
+        snapshot: test_snapshot_with_state_param
+        state: SUCCESS,PARTIAL
+
+  - is_true: snapshots
+  - match: { snapshots.0.snapshot: test_snapshot_with_state_param }
+  - match: { snapshots.0.state: SUCCESS }
+
+  - do:
+      snapshot.get:
+        repository: test_repo_get_1
+        snapshot: test_snapshot_with_state_param
+        state: FAILED
+
+  - is_true: snapshots
+  - length: { snapshots: 0 }
+
+  - do:
+      catch: bad_request
+      snapshot.get:
+        repository: test_repo_get_1
+        snapshot: test_snapshot_with_state_param
+        state: FOO
+
+  - match: { error.type: "illegal_argument_exception" }
+  - match: { error.reason: "No enum constant org.elasticsearch.snapshots.SnapshotState.FOO" }
+
+  - do:
+      snapshot.delete:
+        repository: test_repo_get_1
+        snapshot: test_snapshot_with_state_param

server/src/internalClusterTest/java/org/elasticsearch/snapshots/GetSnapshotsIT.java

@@ -53,6 +53,7 @@
 import java.nio.file.Files;
 import java.nio.file.Path;
 import java.util.ArrayList;
+import java.util.Arrays;
 import java.util.Collection;
 import java.util.Collections;
 import java.util.EnumSet;
@@ -61,6 +62,7 @@
 import java.util.Map;
 import java.util.Objects;
 import java.util.Set;
+import java.util.function.Function;
 import java.util.function.Predicate;
 import java.util.stream.Collectors;
 
@@ -70,7 +72,6 @@
 import static org.hamcrest.Matchers.hasSize;
 import static org.hamcrest.Matchers.in;
 import static org.hamcrest.Matchers.is;
-import static org.hamcrest.core.StringContains.containsString;
 
 public class GetSnapshotsIT extends AbstractSnapshotIntegTestCase {
 
@@ -641,47 +642,55 @@ public void testFilterByState() throws Exception {
         createRepository(repoName, "mock", repoPath);
 
         // Create a successful snapshot
-        String successSnapshot = "snapshot-success";
-        createFullSnapshot(repoName, successSnapshot);
+        createFullSnapshot(repoName, "snapshot-success");
+
+        final Function<EnumSet<SnapshotState>, List<SnapshotInfo>> getSnapshotsForStates = (states) -> {
+            return clusterAdmin().prepareGetSnapshots(TEST_REQUEST_TIMEOUT, repoName).setStates(states).get().getSnapshots();
+        };
 
         // Fetch snapshots with state=SUCCESS
-        GetSnapshotsResponse responseSuccess = clusterAdmin().prepareGetSnapshots(TEST_REQUEST_TIMEOUT, repoName)
-            .setState(EnumSet.of(SnapshotState.SUCCESS))
-            .get();
-        assertThat(responseSuccess.getSnapshots(), hasSize(1));
-        assertThat(responseSuccess.getSnapshots().get(0).state(), is(SnapshotState.SUCCESS));
+        var snapshots = getSnapshotsForStates.apply(EnumSet.of(SnapshotState.SUCCESS));
+        assertThat(snapshots, hasSize(1));
+        assertThat(snapshots.getFirst().state(), is(SnapshotState.SUCCESS));
 
         // Create a snapshot in progress
-        String inProgressSnapshot = "snapshot-in-progress";
         blockAllDataNodes(repoName);
-        startFullSnapshot(repoName, inProgressSnapshot);
+        startFullSnapshot(repoName, "snapshot-in-progress");
         awaitNumberOfSnapshotsInProgress(1);
 
         // Fetch snapshots with state=IN_PROGRESS
-        GetSnapshotsResponse responseInProgress = clusterAdmin().prepareGetSnapshots(TEST_REQUEST_TIMEOUT, repoName)
-            .setState(EnumSet.of(SnapshotState.IN_PROGRESS))
-            .get();
-        assertThat(responseInProgress.getSnapshots(), hasSize(1));
-        assertThat(responseInProgress.getSnapshots().get(0).state(), is(SnapshotState.IN_PROGRESS));
+        snapshots = getSnapshotsForStates.apply(EnumSet.of(SnapshotState.IN_PROGRESS));
+        assertThat(snapshots, hasSize(1));
+        assertThat(snapshots.getFirst().state(), is(SnapshotState.IN_PROGRESS));
 
         // Fetch snapshots with multiple states (SUCCESS, IN_PROGRESS)
-        GetSnapshotsResponse responseMultipleStates = clusterAdmin().prepareGetSnapshots(TEST_REQUEST_TIMEOUT, repoName)
-            .setState(EnumSet.of(SnapshotState.SUCCESS, SnapshotState.IN_PROGRESS))
-            .get();
-        assertThat(responseMultipleStates.getSnapshots(), hasSize(2));
-        assertTrue(responseMultipleStates.getSnapshots().stream().map(SnapshotInfo::state).toList().contains(SnapshotState.SUCCESS));
-        assertTrue(responseMultipleStates.getSnapshots().stream().map(SnapshotInfo::state).toList().contains(SnapshotState.IN_PROGRESS));
+        snapshots = getSnapshotsForStates.apply(EnumSet.of(SnapshotState.SUCCESS, SnapshotState.IN_PROGRESS));
+        assertThat(snapshots, hasSize(2));
+        var states = snapshots.stream().map(SnapshotInfo::state).collect(Collectors.toSet());
+        assertTrue(states.contains(SnapshotState.SUCCESS));
+        assertTrue(states.contains(SnapshotState.IN_PROGRESS));
 
         // Fetch all snapshots (without state)
-        GetSnapshotsResponse responseAll = clusterAdmin().prepareGetSnapshots(TEST_REQUEST_TIMEOUT, repoName).get();
-        assertThat(responseAll.getSnapshots(), hasSize(2));
+        snapshots = clusterAdmin().prepareGetSnapshots(TEST_REQUEST_TIMEOUT, repoName).get().getSnapshots();
+        assertThat(snapshots, hasSize(2));
 
         // Fetch snapshots with an invalid state
         IllegalArgumentException e = expectThrows(
             IllegalArgumentException.class,
-            () -> clusterAdmin().prepareGetSnapshots(TEST_REQUEST_TIMEOUT, repoName).setState(EnumSet.of(SnapshotState.of("FOO"))).get()
+            () -> getSnapshotsForStates.apply(EnumSet.of(SnapshotState.valueOf("FOO")))
         );
-        assertThat(e.getMessage(), containsString("Unknown state name [FOO]"));
+        assertThat(e.getMessage(), is("No enum constant org.elasticsearch.snapshots.SnapshotState.FOO"));
+
+        // Allow the IN_PROGRESS snapshot to finish, then verify GET using SUCCESS has results and IN_PROGRESS does not.
+        unblockAllDataNodes(repoName);
+        awaitNumberOfSnapshotsInProgress(0);
+        snapshots = clusterAdmin().prepareGetSnapshots(TEST_REQUEST_TIMEOUT, repoName).get().getSnapshots();
+        assertThat(snapshots, hasSize(2));
+        states = snapshots.stream().map(SnapshotInfo::state).collect(Collectors.toSet());
+        assertThat(states, hasSize(1));
+        assertTrue(states.contains(SnapshotState.SUCCESS));
+        snapshots = getSnapshotsForStates.apply(EnumSet.of(SnapshotState.IN_PROGRESS));
+        assertThat(snapshots, hasSize(0));
     }
 
     // Create a snapshot that is guaranteed to have a unique start time and duration for tests around ordering by either.
@@ -963,15 +972,15 @@ public void testAllFeatures() {
         // INDICES and by SHARDS. The actual sorting behaviour for these cases is tested elsewhere, here we're just checking that sorting
         // interacts correctly with the other parameters to the API.
 
-        final EnumSet<SnapshotState> state = EnumSet.of(randomFrom(SnapshotState.values()));
-        // Note: The selected state may not match any existing snapshots.
+        final EnumSet<SnapshotState> states = EnumSet.copyOf(randomNonEmptySubsetOf(Arrays.asList(SnapshotState.values())));
+        // Note: The selected state(s) may not match any existing snapshots.
         // The actual filtering behaviour for such cases is tested in the dedicated test.
-        // Here we're just checking that state interacts correctly with the other parameters to the API.
+        // Here we're just checking that states interacts correctly with the other parameters to the API.
+        snapshotInfoPredicate = snapshotInfoPredicate.and(si -> states.contains(si.state()));
 
         // compute the ordered sequence of snapshots which match the repository/snapshot name filters and SLM policy filter
         final var selectedSnapshots = snapshotInfos.stream()
             .filter(snapshotInfoPredicate)
-            .filter(s -> state.contains(s.state()))
             .sorted(sortKey.getSnapshotInfoComparator(order))
             .toList();
 
@@ -981,7 +990,7 @@ public void testAllFeatures() {
             // apply sorting params
             .sort(sortKey)
             .order(order)
-            .state(state);
+            .states(states);
 
         // sometimes use ?from_sort_value to skip some items; note that snapshots skipped in this way are subtracted from
         // GetSnapshotsResponse.totalCount whereas snapshots skipped by ?after and ?offset are not
@@ -1069,7 +1078,7 @@ public void testAllFeatures() {
                 .order(order)
                 .size(nextSize)
                 .after(SnapshotSortKey.decodeAfterQueryParam(nextRequestAfter))
-                .state(state);
+                .states(states);
             final GetSnapshotsResponse nextResponse = safeAwait(l -> client().execute(TransportGetSnapshotsAction.TYPE, nextRequest, l));
 
             assertEquals(

server/src/main/java/module-info.java

@@ -424,6 +424,7 @@
             org.elasticsearch.action.bulk.BulkFeatures,
             org.elasticsearch.features.InfrastructureFeatures,
             org.elasticsearch.rest.action.admin.cluster.ClusterRerouteFeatures,
+            org.elasticsearch.rest.action.admin.cluster.GetSnapshotsFeatures,
             org.elasticsearch.index.mapper.MapperFeatures,
             org.elasticsearch.index.IndexFeatures,
             org.elasticsearch.search.SearchFeatures,

server/src/main/java/org/elasticsearch/action/ActionModule.java

@@ -865,7 +865,7 @@ public void initRestHandlers(Supplier<DiscoveryNodes> nodesInCluster, Predicate<
         registerHandler.accept(new RestDeleteRepositoryAction());
         registerHandler.accept(new RestVerifyRepositoryAction());
         registerHandler.accept(new RestCleanupRepositoryAction());
-        registerHandler.accept(new RestGetSnapshotsAction());
+        registerHandler.accept(new RestGetSnapshotsAction(clusterSupportsFeature));
         registerHandler.accept(new RestCreateSnapshotAction());
         registerHandler.accept(new RestCloneSnapshotAction());
         registerHandler.accept(new RestRestoreSnapshotAction());

server/src/main/java/org/elasticsearch/action/admin/cluster/snapshots/get/GetSnapshotsRequest.java

@@ -28,6 +28,7 @@
 import java.util.Arrays;
 import java.util.EnumSet;
 import java.util.Map;
+import java.util.Objects;
 
 import static org.elasticsearch.action.ValidateActions.addValidationError;
 
@@ -80,7 +81,7 @@ public class GetSnapshotsRequest extends MasterNodeRequest<GetSnapshotsRequest>
 
     private boolean includeIndexNames = true;
 
-    private EnumSet<SnapshotState> state = EnumSet.noneOf(SnapshotState.class);
+    private EnumSet<SnapshotState> states = EnumSet.allOf(SnapshotState.class);
 
     public GetSnapshotsRequest(TimeValue masterNodeTimeout) {
         super(masterNodeTimeout);
@@ -124,7 +125,9 @@ public GetSnapshotsRequest(StreamInput in) throws IOException {
             includeIndexNames = in.readBoolean();
         }
         if (in.getTransportVersion().onOrAfter(STATE_FLAG_VERSION)) {
-            state = in.readEnumSet(SnapshotState.class);
+            states = in.readEnumSet(SnapshotState.class);
+        } else {
+            states = EnumSet.allOf(SnapshotState.class);
         }
     }
 
@@ -146,7 +149,11 @@ public void writeTo(StreamOutput out) throws IOException {
             out.writeBoolean(includeIndexNames);
         }
         if (out.getTransportVersion().onOrAfter(STATE_FLAG_VERSION)) {
-            out.writeEnumSet(state);
+            out.writeEnumSet(states);
+        } else if (states.equals(EnumSet.allOf(SnapshotState.class)) == false) {
+            final var errorString = "GetSnapshotsRequest [states] field is not supported on all nodes in the cluster";
+            assert false : errorString;
+            throw new IllegalStateException(errorString);
         }
     }
 
@@ -353,12 +360,12 @@ public boolean verbose() {
         return verbose;
     }
 
-    public EnumSet<SnapshotState> state() {
-        return state;
+    public EnumSet<SnapshotState> states() {
+        return states;
     }
 
-    public GetSnapshotsRequest state(EnumSet<SnapshotState> state) {
-        this.state = state;
+    public GetSnapshotsRequest states(EnumSet<SnapshotState> states) {
+        this.states = Objects.requireNonNull(states);
         return this;
     }
 

server/src/main/java/org/elasticsearch/action/admin/cluster/snapshots/get/GetSnapshotsRequestBuilder.java

@@ -153,8 +153,8 @@ public GetSnapshotsRequestBuilder setIncludeIndexNames(boolean indices) {
 
     }
 
-    public GetSnapshotsRequestBuilder setState(EnumSet<SnapshotState> state) {
-        request.state(state);
+    public GetSnapshotsRequestBuilder setStates(EnumSet<SnapshotState> states) {
+        request.states(states);
         return this;
     }
 }

server/src/main/java/org/elasticsearch/action/admin/cluster/snapshots/get/TransportGetSnapshotsAction.java

@@ -163,7 +163,7 @@ protected void masterOperation(
             SnapshotsInProgress.get(state),
             request.verbose(),
             request.includeIndexNames(),
-            request.state()
+            request.states()
         ).runOperation(listener);
     }
 
@@ -184,7 +184,7 @@ private class GetSnapshotsOperation {
         private final SnapshotNamePredicate snapshotNamePredicate;
         private final SnapshotPredicates fromSortValuePredicates;
         private final Predicate<String> slmPolicyPredicate;
-        private final EnumSet<SnapshotState> state;
+        private final EnumSet<SnapshotState> states;
 
         // snapshot ordering/pagination
         private final SnapshotSortKey sortBy;
@@ -229,7 +229,7 @@ private class GetSnapshotsOperation {
             SnapshotsInProgress snapshotsInProgress,
             boolean verbose,
             boolean indices,
-            EnumSet<SnapshotState> state
+            EnumSet<SnapshotState> states
         ) {
             this.cancellableTask = cancellableTask;
             this.repositories = repositories;
@@ -242,7 +242,7 @@ private class GetSnapshotsOperation {
             this.snapshotsInProgress = snapshotsInProgress;
             this.verbose = verbose;
             this.indices = indices;
-            this.state = state;
+            this.states = states;
 
             this.snapshotNamePredicate = SnapshotNamePredicate.forSnapshots(ignoreUnavailable, snapshots);
             this.fromSortValuePredicates = SnapshotPredicates.forFromSortValue(fromSortValue, sortBy, order);
@@ -566,7 +566,10 @@ private boolean matchesPredicates(SnapshotId snapshotId, RepositoryData reposito
 
             final var details = repositoryData.getSnapshotDetails(snapshotId);
 
-            if (!state.isEmpty() && !state.contains(details.getSnapshotState())) {
+            if (states.isEmpty() == false
+                && details != null
+                && details.getSnapshotState() != null
+                && states.contains(details.getSnapshotState()) == false) {
                 return false;
             }
 
@@ -582,6 +585,10 @@ private boolean matchesPredicates(SnapshotInfo snapshotInfo) {
                 return false;
             }
 
+            if (states.isEmpty() == false && snapshotInfo.state() != null && states.contains(snapshotInfo.state()) == false) {
+                return false;
+            }
+
             if (slmPolicyPredicate == SlmPolicyPredicate.MATCH_ALL_POLICIES) {
                 return true;
             }

server/src/main/java/org/elasticsearch/rest/action/admin/cluster/GetSnapshotsFeatures.java

@@ -0,0 +1,24 @@
+/*
+ * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
+ * or more contributor license agreements. Licensed under the "Elastic License
+ * 2.0", the "GNU Affero General Public License v3.0 only", and the "Server Side
+ * Public License v 1"; you may not use this file except in compliance with, at
+ * your election, the "Elastic License 2.0", the "GNU Affero General Public
+ * License v3.0 only", or the "Server Side Public License, v 1".
+ */
+
+package org.elasticsearch.rest.action.admin.cluster;
+
+import org.elasticsearch.features.FeatureSpecification;
+import org.elasticsearch.features.NodeFeature;
+
+import java.util.Set;
+
+public class GetSnapshotsFeatures implements FeatureSpecification {
+    public static final NodeFeature GET_SNAPSHOTS_STATE_PARAMETER = new NodeFeature("get.snapshots.state_parameter");
+
+    @Override
+    public Set<NodeFeature> getFeatures() {
+        return Set.of(GET_SNAPSHOTS_STATE_PARAMETER);
+    }
+}