Drop dead code from get-snapshots request & response (#105608)

Removes all the now-dead code related to reading pre-7.16 get-snapshots
requests and responses, and also moves the `XContent` response parsing
out of production and into the only test suite that uses it.

qa/smoke-test-http/src/javaRestTest/java/org/elasticsearch/http/snapshots/RestGetSnapshotsIT.java

@@ -9,6 +9,7 @@
 package org.elasticsearch.http.snapshots;
 
 import org.apache.http.client.methods.HttpGet;
+import org.elasticsearch.ElasticsearchException;
 import org.elasticsearch.action.ActionFuture;
 import org.elasticsearch.action.admin.cluster.snapshots.create.CreateSnapshotResponse;
 import org.elasticsearch.action.admin.cluster.snapshots.get.GetSnapshotsRequest;
@@ -23,6 +24,8 @@ import org.elasticsearch.snapshots.AbstractSnapshotIntegTestCase;
 import org.elasticsearch.snapshots.SnapshotInfo;
 import org.elasticsearch.snapshots.SnapshotsService;
 import org.elasticsearch.threadpool.ThreadPool;
+import org.elasticsearch.xcontent.ConstructingObjectParser;
+import org.elasticsearch.xcontent.ParseField;
 import org.elasticsearch.xcontent.XContentParser;
 import org.elasticsearch.xcontent.XContentParserConfiguration;
 import org.elasticsearch.xcontent.json.JsonXContent;
@@ -31,6 +34,7 @@ import java.io.IOException;
 import java.io.InputStream;
 import java.util.ArrayList;
 import java.util.Collection;
+import java.util.HashMap;
 import java.util.HashSet;
 import java.util.List;
 import java.util.Map;
@@ -444,7 +448,7 @@ public class RestGetSnapshotsIT extends AbstractSnapshotRestTestCase {
             InputStream input = response.getEntity().getContent();
             XContentParser parser = JsonXContent.jsonXContent.createParser(XContentParserConfiguration.EMPTY, input)
         ) {
-            return GetSnapshotsResponse.fromXContent(parser);
+            return GET_SNAPSHOT_PARSER.parse(parser, null);
         }
     }
 
@@ -501,4 +505,35 @@ public class RestGetSnapshotsIT extends AbstractSnapshotRestTestCase {
         final Response response = getRestClient().performRequest(request);
         return readSnapshotInfos(response);
     }
+
+    private static final int UNKNOWN_COUNT = -1;
+
+    @SuppressWarnings("unchecked")
+    private static final ConstructingObjectParser<GetSnapshotsResponse, Void> GET_SNAPSHOT_PARSER = new ConstructingObjectParser<>(
+        GetSnapshotsResponse.class.getName(),
+        true,
+        (args) -> new GetSnapshotsResponse(
+            (List<SnapshotInfo>) args[0],
+            (Map<String, ElasticsearchException>) args[1],
+            (String) args[2],
+            args[3] == null ? UNKNOWN_COUNT : (int) args[3],
+            args[4] == null ? UNKNOWN_COUNT : (int) args[4]
+        )
+    );
+
+    static {
+        GET_SNAPSHOT_PARSER.declareObjectArray(
+            ConstructingObjectParser.constructorArg(),
+            (p, c) -> SnapshotInfo.SNAPSHOT_INFO_PARSER.apply(p, c).build(),
+            new ParseField("snapshots")
+        );
+        GET_SNAPSHOT_PARSER.declareObject(
+            ConstructingObjectParser.optionalConstructorArg(),
+            (p, c) -> p.map(HashMap::new, ElasticsearchException::fromXContent),
+            new ParseField("failures")
+        );
+        GET_SNAPSHOT_PARSER.declareStringOrNull(ConstructingObjectParser.optionalConstructorArg(), new ParseField("next"));
+        GET_SNAPSHOT_PARSER.declareIntOrNull(ConstructingObjectParser.optionalConstructorArg(), UNKNOWN_COUNT, new ParseField("total"));
+        GET_SNAPSHOT_PARSER.declareIntOrNull(ConstructingObjectParser.optionalConstructorArg(), UNKNOWN_COUNT, new ParseField("remaining"));
+    }
 }

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

@@ -41,18 +41,6 @@ public class GetSnapshotsRequest extends MasterNodeRequest<GetSnapshotsRequest>
     public static final String NO_POLICY_PATTERN = "_none";
     public static final boolean DEFAULT_VERBOSE_MODE = true;
 
-    public static final TransportVersion SLM_POLICY_FILTERING_VERSION = TransportVersions.V_7_16_0;
-
-    public static final TransportVersion FROM_SORT_VALUE_VERSION = TransportVersions.V_7_16_0;
-
-    public static final TransportVersion MULTIPLE_REPOSITORIES_SUPPORT_ADDED = TransportVersions.V_7_14_0;
-
-    public static final TransportVersion PAGINATED_GET_SNAPSHOTS_VERSION = TransportVersions.V_7_14_0;
-
-    public static final TransportVersion NUMERIC_PAGINATION_VERSION = TransportVersions.V_7_15_0;
-
-    private static final TransportVersion SORT_BY_SHARDS_OR_REPO_VERSION = TransportVersions.V_7_16_0;
-
     private static final TransportVersion INDICES_FLAG_VERSION = TransportVersions.V_8_3_0;
 
     public static final int NO_LIMIT = -1;
@@ -113,89 +101,36 @@ public class GetSnapshotsRequest extends MasterNodeRequest<GetSnapshotsRequest>
 
     public GetSnapshotsRequest(StreamInput in) throws IOException {
         super(in);
-        if (in.getTransportVersion().onOrAfter(MULTIPLE_REPOSITORIES_SUPPORT_ADDED)) {
-            repositories = in.readStringArray();
-        } else {
-            repositories = new String[] { in.readString() };
-        }
+        repositories = in.readStringArray();
         snapshots = in.readStringArray();
         ignoreUnavailable = in.readBoolean();
         verbose = in.readBoolean();
-        if (in.getTransportVersion().onOrAfter(PAGINATED_GET_SNAPSHOTS_VERSION)) {
-            after = in.readOptionalWriteable(After::new);
-            sort = in.readEnum(SortBy.class);
-            size = in.readVInt();
-            order = SortOrder.readFromStream(in);
-            if (in.getTransportVersion().onOrAfter(NUMERIC_PAGINATION_VERSION)) {
-                offset = in.readVInt();
-            }
-            if (in.getTransportVersion().onOrAfter(SLM_POLICY_FILTERING_VERSION)) {
-                policies = in.readStringArray();
-            }
-            if (in.getTransportVersion().onOrAfter(FROM_SORT_VALUE_VERSION)) {
-                fromSortValue = in.readOptionalString();
-            }
-            if (in.getTransportVersion().onOrAfter(INDICES_FLAG_VERSION)) {
-                includeIndexNames = in.readBoolean();
-            }
+        after = in.readOptionalWriteable(After::new);
+        sort = in.readEnum(SortBy.class);
+        size = in.readVInt();
+        order = SortOrder.readFromStream(in);
+        offset = in.readVInt();
+        policies = in.readStringArray();
+        fromSortValue = in.readOptionalString();
+        if (in.getTransportVersion().onOrAfter(INDICES_FLAG_VERSION)) {
+            includeIndexNames = in.readBoolean();
         }
     }
 
     @Override
     public void writeTo(StreamOutput out) throws IOException {
         super.writeTo(out);
-        if (out.getTransportVersion().onOrAfter(MULTIPLE_REPOSITORIES_SUPPORT_ADDED)) {
-            out.writeStringArray(repositories);
-        } else {
-            if (repositories.length != 1) {
-                throw new IllegalArgumentException(
-                    "Requesting snapshots from multiple repositories is not supported in versions prior "
-                        + "to "
-                        + MULTIPLE_REPOSITORIES_SUPPORT_ADDED.toString()
-                );
-            }
-            out.writeString(repositories[0]);
-        }
+        out.writeStringArray(repositories);
         out.writeStringArray(snapshots);
         out.writeBoolean(ignoreUnavailable);
         out.writeBoolean(verbose);
-        if (out.getTransportVersion().onOrAfter(PAGINATED_GET_SNAPSHOTS_VERSION)) {
-            out.writeOptionalWriteable(after);
-            if ((sort == SortBy.SHARDS || sort == SortBy.FAILED_SHARDS || sort == SortBy.REPOSITORY)
-                && out.getTransportVersion().before(SORT_BY_SHARDS_OR_REPO_VERSION)) {
-                throw new IllegalArgumentException(
-                    "can't use sort by shard count or repository name in transport version [" + out.getTransportVersion() + "]"
-                );
-            }
-            out.writeEnum(sort);
-            out.writeVInt(size);
-            order.writeTo(out);
-            if (out.getTransportVersion().onOrAfter(NUMERIC_PAGINATION_VERSION)) {
-                out.writeVInt(offset);
-            } else if (offset != 0) {
-                throw new IllegalArgumentException(
-                    "can't use numeric offset in get snapshots request in transport version [" + out.getTransportVersion() + "]"
-                );
-            }
-        } else if (sort != SortBy.START_TIME || size != NO_LIMIT || after != null || order != SortOrder.ASC) {
-            throw new IllegalArgumentException(
-                "can't use paginated get snapshots request in transport version [" + out.getTransportVersion() + "]"
-            );
-        }
-        if (out.getTransportVersion().onOrAfter(SLM_POLICY_FILTERING_VERSION)) {
-            out.writeStringArray(policies);
-        } else if (policies.length > 0) {
-            throw new IllegalArgumentException(
-                "can't use slm policy filter in snapshots request in transport version [" + out.getTransportVersion() + "]"
-            );
-        }
-        if (out.getTransportVersion().onOrAfter(FROM_SORT_VALUE_VERSION)) {
-            out.writeOptionalString(fromSortValue);
-        } else if (fromSortValue != null) {
-            throw new IllegalArgumentException(
-                "can't use after-value in snapshot request in transport version [" + out.getTransportVersion() + "]"
-            );
-        }
+        out.writeOptionalWriteable(after);
+        out.writeEnum(sort);
+        out.writeVInt(size);
+        order.writeTo(out);
+        out.writeVInt(offset);
+        out.writeStringArray(policies);
+        out.writeOptionalString(fromSortValue);
         if (out.getTransportVersion().onOrAfter(INDICES_FLAG_VERSION)) {
             out.writeBoolean(includeIndexNames);
         }

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

@@ -17,14 +17,10 @@ import org.elasticsearch.common.io.stream.StreamOutput;
 import org.elasticsearch.common.xcontent.ChunkedToXContentObject;
 import org.elasticsearch.core.Nullable;
 import org.elasticsearch.snapshots.SnapshotInfo;
-import org.elasticsearch.xcontent.ConstructingObjectParser;
-import org.elasticsearch.xcontent.ParseField;
 import org.elasticsearch.xcontent.ToXContent;
-import org.elasticsearch.xcontent.XContentParser;
 
 import java.io.IOException;
 import java.util.Collections;
-import java.util.HashMap;
 import java.util.Iterator;
 import java.util.List;
 import java.util.Map;
@@ -35,37 +31,6 @@ import java.util.Objects;
  */
 public class GetSnapshotsResponse extends ActionResponse implements ChunkedToXContentObject {
 
-    private static final int UNKNOWN_COUNT = -1;
-
-    @SuppressWarnings("unchecked")
-    private static final ConstructingObjectParser<GetSnapshotsResponse, Void> GET_SNAPSHOT_PARSER = new ConstructingObjectParser<>(
-        GetSnapshotsResponse.class.getName(),
-        true,
-        (args) -> new GetSnapshotsResponse(
-            (List<SnapshotInfo>) args[0],
-            (Map<String, ElasticsearchException>) args[1],
-            (String) args[2],
-            args[3] == null ? UNKNOWN_COUNT : (int) args[3],
-            args[4] == null ? UNKNOWN_COUNT : (int) args[4]
-        )
-    );
-
-    static {
-        GET_SNAPSHOT_PARSER.declareObjectArray(
-            ConstructingObjectParser.constructorArg(),
-            (p, c) -> SnapshotInfo.SNAPSHOT_INFO_PARSER.apply(p, c).build(),
-            new ParseField("snapshots")
-        );
-        GET_SNAPSHOT_PARSER.declareObject(
-            ConstructingObjectParser.optionalConstructorArg(),
-            (p, c) -> p.map(HashMap::new, ElasticsearchException::fromXContent),
-            new ParseField("failures")
-        );
-        GET_SNAPSHOT_PARSER.declareStringOrNull(ConstructingObjectParser.optionalConstructorArg(), new ParseField("next"));
-        GET_SNAPSHOT_PARSER.declareIntOrNull(ConstructingObjectParser.optionalConstructorArg(), UNKNOWN_COUNT, new ParseField("total"));
-        GET_SNAPSHOT_PARSER.declareIntOrNull(ConstructingObjectParser.optionalConstructorArg(), UNKNOWN_COUNT, new ParseField("remaining"));
-    }
-
     private final List<SnapshotInfo> snapshots;
 
     private final Map<String, ElasticsearchException> failures;
@@ -93,21 +58,10 @@ public class GetSnapshotsResponse extends ActionResponse implements ChunkedToXCo
 
     public GetSnapshotsResponse(StreamInput in) throws IOException {
         this.snapshots = in.readCollectionAsImmutableList(SnapshotInfo::readFrom);
-        if (in.getTransportVersion().onOrAfter(GetSnapshotsRequest.MULTIPLE_REPOSITORIES_SUPPORT_ADDED)) {
-            final Map<String, ElasticsearchException> failedResponses = in.readMap(StreamInput::readException);
-            this.failures = Collections.unmodifiableMap(failedResponses);
-            this.next = in.readOptionalString();
-        } else {
-            this.failures = Collections.emptyMap();
-            this.next = null;
-        }
-        if (in.getTransportVersion().onOrAfter(GetSnapshotsRequest.NUMERIC_PAGINATION_VERSION)) {
-            this.total = in.readVInt();
-            this.remaining = in.readVInt();
-        } else {
-            this.total = UNKNOWN_COUNT;
-            this.remaining = UNKNOWN_COUNT;
-        }
+        this.failures = Collections.unmodifiableMap(in.readMap(StreamInput::readException));
+        this.next = in.readOptionalString();
+        this.total = in.readVInt();
+        this.remaining = in.readVInt();
     }
 
     /**
@@ -149,19 +103,10 @@ public class GetSnapshotsResponse extends ActionResponse implements ChunkedToXCo
     @Override
     public void writeTo(StreamOutput out) throws IOException {
         out.writeCollection(snapshots);
-        if (out.getTransportVersion().onOrAfter(GetSnapshotsRequest.MULTIPLE_REPOSITORIES_SUPPORT_ADDED)) {
-            out.writeMap(failures, StreamOutput::writeException);
-            out.writeOptionalString(next);
-        } else {
-            if (failures.isEmpty() == false) {
-                assert false : "transport action should have thrown directly for old version but saw " + failures;
-                throw failures.values().iterator().next();
-            }
-        }
-        if (out.getTransportVersion().onOrAfter(GetSnapshotsRequest.NUMERIC_PAGINATION_VERSION)) {
-            out.writeVInt(total);
-            out.writeVInt(remaining);
-        }
+        out.writeMap(failures, StreamOutput::writeException);
+        out.writeOptionalString(next);
+        out.writeVInt(total);
+        out.writeVInt(remaining);
     }
 
     @Override
@@ -198,10 +143,6 @@ public class GetSnapshotsResponse extends ActionResponse implements ChunkedToXCo
         }));
     }
 
-    public static GetSnapshotsResponse fromXContent(XContentParser parser) throws IOException {
-        return GET_SNAPSHOT_PARSER.parse(parser, null);
-    }
-
     @Override
     public boolean equals(Object o) {
         if (this == o) return true;

server/src/main/java/org/elasticsearch/snapshots/SnapshotInfo.java

@@ -496,12 +496,7 @@ public final class SnapshotInfo implements Comparable<SnapshotInfo>, ToXContentF
      * Constructs snapshot information from stream input
      */
     public static SnapshotInfo readFrom(final StreamInput in) throws IOException {
-        final Snapshot snapshot;
-        if (in.getTransportVersion().onOrAfter(GetSnapshotsRequest.PAGINATED_GET_SNAPSHOTS_VERSION)) {
-            snapshot = new Snapshot(in);
-        } else {
-            snapshot = new Snapshot(UNKNOWN_REPO_NAME, new SnapshotId(in));
-        }
+        final Snapshot snapshot = new Snapshot(in);
         final List<String> indices = in.readStringCollectionAsImmutableList();
         final SnapshotState state = in.readBoolean() ? SnapshotState.fromValue(in.readByte()) : null;
         final String reason = in.readOptionalString();
@@ -1015,11 +1010,7 @@ public final class SnapshotInfo implements Comparable<SnapshotInfo>, ToXContentF
 
     @Override
     public void writeTo(final StreamOutput out) throws IOException {
-        if (out.getTransportVersion().onOrAfter(GetSnapshotsRequest.PAGINATED_GET_SNAPSHOTS_VERSION)) {
-            snapshot.writeTo(out);
-        } else {
-            snapshot.getSnapshotId().writeTo(out);
-        }
+        snapshot.writeTo(out);
         out.writeStringCollection(indices);
         if (state != null) {
             out.writeBoolean(true);

server/src/test/java/org/elasticsearch/action/admin/cluster/snapshots/get/GetSnapshotsResponseTests.java

@@ -10,7 +10,6 @@ package org.elasticsearch.action.admin.cluster.snapshots.get;
 
 import org.elasticsearch.ElasticsearchException;
 import org.elasticsearch.TransportVersion;
-import org.elasticsearch.common.Strings;
 import org.elasticsearch.common.UUIDs;
 import org.elasticsearch.common.io.stream.NamedWriteableRegistry;
 import org.elasticsearch.common.io.stream.StreamOutput;
@@ -24,9 +23,6 @@ import org.elasticsearch.snapshots.SnapshotInfoTestUtils;
 import org.elasticsearch.snapshots.SnapshotShardFailure;
 import org.elasticsearch.test.AbstractChunkedSerializingTestCase;
 import org.elasticsearch.test.ESTestCase;
-import org.elasticsearch.xcontent.ToXContent;
-import org.elasticsearch.xcontent.XContentParser;
-import org.elasticsearch.xcontent.XContentType;
 
 import java.io.IOException;
 import java.nio.charset.StandardCharsets;
@@ -39,11 +35,7 @@ import java.util.HashSet;
 import java.util.List;
 import java.util.Map;
 import java.util.Set;
-import java.util.function.Predicate;
-import java.util.regex.Pattern;
 
-import static org.elasticsearch.snapshots.SnapshotInfo.INDEX_DETAILS_XCONTENT_PARAM;
-import static org.elasticsearch.test.AbstractXContentTestCase.chunkedXContentTester;
 import static org.hamcrest.CoreMatchers.containsString;
 
 public class GetSnapshotsResponseTests extends ESTestCase {
@@ -53,10 +45,6 @@ public class GetSnapshotsResponseTests extends ESTestCase {
     // It does not override equals and hashCode, because it
     // contains ElasticsearchException, which does not override equals and hashCode.
 
-    private GetSnapshotsResponse doParseInstance(XContentParser parser) throws IOException {
-        return GetSnapshotsResponse.fromXContent(parser);
-    }
-
     private GetSnapshotsResponse copyInstance(GetSnapshotsResponse instance) throws IOException {
         return copyInstance(
             instance,
@@ -146,38 +134,6 @@ public class GetSnapshotsResponseTests extends ESTestCase {
         assertEqualInstances(testInstance, deserializedInstance);
     }
 
-    public void testFromXContent() throws IOException {
-        // Explicitly include the index details, excluded by default, since this is required for a faithful round-trip
-        final ToXContent.MapParams params = new ToXContent.MapParams(Map.of(INDEX_DETAILS_XCONTENT_PARAM, "true"));
-
-        // Don't inject random fields into the custom snapshot metadata, because the metadata map is equality-checked after doing a
-        // round-trip through xContent serialization/deserialization. Even though the rest of the object ignores unknown fields,
-        // `metadata` doesn't ignore unknown fields (it just includes them in the parsed object, because the keys are arbitrary),
-        // so any new fields added to the metadata before it gets deserialized that weren't in the serialized version will
-        // cause the equality check to fail.
-        //
-        // Also don't inject random junk into the index details section, since this is keyed by index name but the values
-        // are required to be a valid IndexSnapshotDetails
-        //
-        // The actual fields are nested in an array, so this regex matches fields with names of the form
-        // `snapshots.3.metadata`
-        final Predicate<String> predicate = Pattern.compile("snapshots\\.\\d+\\.metadata.*")
-            .asMatchPredicate()
-            .or(Pattern.compile("snapshots\\.\\d+\\.index_details").asMatchPredicate())
-            .or(Pattern.compile("failures\\.*").asMatchPredicate());
-        chunkedXContentTester(this::createParser, (XContentType t) -> createTestInstance(), params, this::doParseInstance).numberOfTestRuns(
-            1
-        )
-            .supportsUnknownFields(true)
-            .shuffleFieldsExceptions(Strings.EMPTY_ARRAY)
-            .randomFieldsExcludeFilter(predicate)
-            .assertEqualsConsumer(this::assertEqualInstances)
-            // We set it to false, because GetSnapshotsResponse contains
-            // ElasticsearchException, whose xContent creation/parsing are not stable.
-            .assertToXContentEquivalence(false)
-            .test();
-    }
-
     public void testChunking() {
         AbstractChunkedSerializingTestCase.assertChunkCount(createTestInstance(), response -> response.getSnapshots().size() + 2);
     }