Extract `SnapshotNamePredicate` (#106397)

The logic for selecting snapshots by name in
`TransportGetSnapshotsAction` is a little convoluted, not easy to test,
and as implemented today requires us to create several large lists or
sets of snapshot IDs. This commit extracts the logic, adds a dedicated
test suite, and replaces some intermediate lists with simple filtering.

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

@@ -0,0 +1,123 @@
+/*
+ * 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 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 or the Server
+ * Side Public License, v 1.
+ */
+
+package org.elasticsearch.action.admin.cluster.snapshots.get;
+
+import org.elasticsearch.common.Strings;
+import org.elasticsearch.common.regex.Regex;
+import org.elasticsearch.common.util.set.Sets;
+import org.elasticsearch.repositories.ResolvedRepositories;
+import org.elasticsearch.snapshots.SnapshotMissingException;
+
+import java.util.ArrayList;
+import java.util.Collection;
+import java.util.Collections;
+import java.util.List;
+import java.util.Set;
+
+/**
+ * Represents a filter on snapshots by name, including some special values such as {@code _all} and {@code _current}, as supported by
+ * {@link TransportGetSnapshotsAction}.
+ */
+public interface SnapshotNamePredicate {
+    /**
+     * @return Whether a snapshot with the given name should be selected.
+     */
+    boolean test(String snapshotName, boolean isCurrentSnapshot);
+
+    /**
+     * @return the snapshot names that must be present in a repository. If one of these snapshots is missing then this repository should
+     * yield a {@link SnapshotMissingException} rather than any snapshots.
+     */
+    Collection<String> requiredNames();
+
+    /**
+     * A {@link SnapshotNamePredicate} which matches all snapshots (and requires no specific names).
+     */
+    SnapshotNamePredicate MATCH_ALL = new SnapshotNamePredicate() {
+        @Override
+        public boolean test(String snapshotName, boolean isCurrentSnapshot) {
+            return true;
+        }
+
+        @Override
+        public Collection<String> requiredNames() {
+            return Collections.emptyList();
+        }
+    };
+
+    /**
+     * A {@link SnapshotNamePredicate} which matches all currently-executing snapshots (and requires no specific names).
+     */
+    SnapshotNamePredicate MATCH_CURRENT_ONLY = new SnapshotNamePredicate() {
+        @Override
+        public boolean test(String snapshotName, boolean isCurrentSnapshot) {
+            return isCurrentSnapshot;
+        }
+
+        @Override
+        public Collection<String> requiredNames() {
+            return Collections.emptyList();
+        }
+    };
+
+    /**
+     * @return a {@link SnapshotNamePredicate} from the given {@link GetSnapshotsRequest} parameters.
+     */
+    static SnapshotNamePredicate forSnapshots(boolean ignoreUnavailable, String[] snapshots) {
+        if (ResolvedRepositories.isMatchAll(snapshots)) {
+            return MATCH_ALL;
+        }
+
+        if (snapshots.length == 1 && GetSnapshotsRequest.CURRENT_SNAPSHOT.equalsIgnoreCase(snapshots[0])) {
+            return MATCH_CURRENT_ONLY;
+        }
+
+        final List<String> includesBuilder = new ArrayList<>(snapshots.length);
+        final List<String> excludesBuilder = new ArrayList<>(snapshots.length);
+        final Set<String> requiredNamesBuilder = ignoreUnavailable ? null : Sets.newHashSetWithExpectedSize(snapshots.length);
+        boolean seenCurrent = false;
+        boolean seenWildcard = false;
+        for (final var snapshot : snapshots) {
+            if (seenWildcard && snapshot.length() > 1 && snapshot.startsWith("-")) {
+                excludesBuilder.add(snapshot.substring(1));
+            } else {
+                if (Regex.isSimpleMatchPattern(snapshot)) {
+                    seenWildcard = true;
+                    includesBuilder.add(snapshot);
+                } else if (GetSnapshotsRequest.CURRENT_SNAPSHOT.equalsIgnoreCase(snapshot)) {
+                    seenCurrent = true;
+                    seenWildcard = true;
+                } else {
+                    if (ignoreUnavailable == false) {
+                        requiredNamesBuilder.add(snapshot);
+                    }
+                    includesBuilder.add(snapshot);
+                }
+            }
+        }
+
+        final boolean includeCurrent = seenCurrent;
+        final String[] includes = includesBuilder.toArray(Strings.EMPTY_ARRAY);
+        final String[] excludes = excludesBuilder.toArray(Strings.EMPTY_ARRAY);
+        final Set<String> requiredNames = requiredNamesBuilder == null ? Set.of() : Set.copyOf(requiredNamesBuilder);
+
+        return new SnapshotNamePredicate() {
+            @Override
+            public boolean test(String snapshotName, boolean isCurrentSnapshot) {
+                return ((includeCurrent && isCurrentSnapshot) || Regex.simpleMatch(includes, snapshotName))
+                    && (Regex.simpleMatch(excludes, snapshotName) == false);
+            }
+
+            @Override
+            public Collection<String> requiredNames() {
+                return requiredNames;
+            }
+        };
+    }
+}

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

@@ -153,8 +153,7 @@ public class TransportGetSnapshotsAction extends TransportMasterNodeAction<GetSn
         private final boolean isMultiRepoRequest;
 
         // snapshots selection
-        private final String[] snapshots;
-        private final boolean ignoreUnavailable;
+        private final SnapshotNamePredicate snapshotNamePredicate;
         private final SnapshotPredicates fromSortValuePredicates;
         private final Predicate<String> slmPolicyPredicate;
 
@@ -172,6 +171,7 @@ public class TransportGetSnapshotsAction extends TransportMasterNodeAction<GetSn
         private final SnapshotsInProgress snapshotsInProgress;
 
         // output detail
+        private final boolean ignoreUnavailable;
         private final boolean verbose;
         private final boolean indices;
 
@@ -204,7 +204,6 @@ public class TransportGetSnapshotsAction extends TransportMasterNodeAction<GetSn
             this.cancellableTask = cancellableTask;
             this.repositories = resolvedRepositories.repositoryMetadata();
             this.isMultiRepoRequest = isMultiRepoRequest;
-            this.snapshots = snapshots;
             this.ignoreUnavailable = ignoreUnavailable;
             this.sortBy = sortBy;
             this.order = order;
@@ -216,6 +215,7 @@ public class TransportGetSnapshotsAction extends TransportMasterNodeAction<GetSn
             this.verbose = verbose;
             this.indices = indices;
 
+            this.snapshotNamePredicate = SnapshotNamePredicate.forSnapshots(ignoreUnavailable, snapshots);
             this.fromSortValuePredicates = SnapshotPredicates.forFromSortValue(fromSortValue, sortBy, order);
             this.slmPolicyPredicate = SlmPolicyPredicate.forPolicies(policies);
 
@@ -282,113 +282,46 @@ public class TransportGetSnapshotsAction extends TransportMasterNodeAction<GetSn
         }
 
         private void getSingleRepoSnapshotInfo(String repo, ActionListener<SnapshotsInRepo> listener) {
-            final Map<String, Snapshot> allSnapshotIds = new HashMap<>();
-            final List<SnapshotInfo> currentSnapshots = new ArrayList<>();
-            for (final SnapshotInfo snapshotInfo : currentSnapshots(repo)) {
-                Snapshot snapshot = snapshotInfo.snapshot();
-                allSnapshotIds.put(snapshot.getSnapshotId().getName(), snapshot);
-                currentSnapshots.add(snapshotInfo.maybeWithoutIndices(indices));
-            }
-
             final ListenableFuture<RepositoryData> repositoryDataListener = new ListenableFuture<>();
-            if (isCurrentSnapshotsOnly()) {
+            if (snapshotNamePredicate == SnapshotNamePredicate.MATCH_CURRENT_ONLY) {
                 repositoryDataListener.onResponse(null);
             } else {
                 repositoriesService.getRepositoryData(repo, repositoryDataListener);
             }
 
             repositoryDataListener.addListener(
-                listener.delegateFailureAndWrap(
-                    (l, repositoryData) -> loadSnapshotInfos(repo, allSnapshotIds, currentSnapshots, repositoryData, l)
-                )
+                listener.delegateFailureAndWrap((l, repositoryData) -> loadSnapshotInfos(repo, repositoryData, l))
             );
         }
 
-        /**
-         * Returns a list of currently running snapshots from repository sorted by snapshot creation date
-         *
-         * @param repositoryName      repository name
-         * @return list of snapshots
-         */
-        private List<SnapshotInfo> currentSnapshots(String repositoryName) {
-            List<SnapshotInfo> snapshotList = new ArrayList<>();
-            List<SnapshotsInProgress.Entry> entries = SnapshotsService.currentSnapshots(
-                snapshotsInProgress,
-                repositoryName,
-                Collections.emptyList()
-            );
-            for (SnapshotsInProgress.Entry entry : entries) {
-                snapshotList.add(SnapshotInfo.inProgress(entry));
-            }
-            return snapshotList;
-        }
-
-        private void loadSnapshotInfos(
-            String repo,
-            Map<String, Snapshot> allSnapshotIds,
-            List<SnapshotInfo> currentSnapshots,
-            @Nullable RepositoryData repositoryData,
-            ActionListener<SnapshotsInRepo> listener
-        ) {
+        private void loadSnapshotInfos(String repo, @Nullable RepositoryData repositoryData, ActionListener<SnapshotsInRepo> listener) {
             if (cancellableTask.notifyIfCancelled(listener)) {
                 return;
             }
 
-            if (repositoryData != null) {
-                for (SnapshotId snapshotId : repositoryData.getSnapshotIds()) {
-                    if (matchesPredicates(snapshotId, repositoryData)) {
-                        allSnapshotIds.put(snapshotId.getName(), new Snapshot(repo, snapshotId));
-                    }
+            final Set<String> unmatchedRequiredNames = new HashSet<>(snapshotNamePredicate.requiredNames());
+            final Set<Snapshot> toResolve = new HashSet<>();
+
+            for (final var snapshotInProgress : snapshotsInProgress.forRepo(repo)) {
+                final var snapshotName = snapshotInProgress.snapshot().getSnapshotId().getName();
+                unmatchedRequiredNames.remove(snapshotName);
+                if (snapshotNamePredicate.test(snapshotName, true)) {
+                    toResolve.add(snapshotInProgress.snapshot());
                 }
             }
 
-            final Set<Snapshot> toResolve = new HashSet<>();
-            if (ResolvedRepositories.isMatchAll(snapshots)) {
-                toResolve.addAll(allSnapshotIds.values());
-            } else {
-                final List<String> includePatterns = new ArrayList<>();
-                final List<String> excludePatterns = new ArrayList<>();
-                boolean hasCurrent = false;
-                boolean seenWildcard = false;
-                for (String snapshotOrPattern : snapshots) {
-                    if (seenWildcard && snapshotOrPattern.length() > 1 && snapshotOrPattern.startsWith("-")) {
-                        excludePatterns.add(snapshotOrPattern.substring(1));
-                    } else {
-                        if (Regex.isSimpleMatchPattern(snapshotOrPattern)) {
-                            seenWildcard = true;
-                            includePatterns.add(snapshotOrPattern);
-                        } else if (GetSnapshotsRequest.CURRENT_SNAPSHOT.equalsIgnoreCase(snapshotOrPattern)) {
-                            hasCurrent = true;
-                            seenWildcard = true;
-                        } else {
-                            if (ignoreUnavailable == false && allSnapshotIds.containsKey(snapshotOrPattern) == false) {
-                                throw new SnapshotMissingException(repo, snapshotOrPattern);
-                            }
-                            includePatterns.add(snapshotOrPattern);
-                        }
-                    }
-                }
-                final String[] includes = includePatterns.toArray(Strings.EMPTY_ARRAY);
-                final String[] excludes = excludePatterns.toArray(Strings.EMPTY_ARRAY);
-                for (Map.Entry<String, Snapshot> entry : allSnapshotIds.entrySet()) {
-                    final Snapshot snapshot = entry.getValue();
-                    if (toResolve.contains(snapshot) == false
-                        && Regex.simpleMatch(includes, entry.getKey())
-                        && Regex.simpleMatch(excludes, entry.getKey()) == false) {
-                        toResolve.add(snapshot);
-                    }
-                }
-                if (hasCurrent) {
-                    for (SnapshotInfo snapshotInfo : currentSnapshots) {
-                        final Snapshot snapshot = snapshotInfo.snapshot();
-                        if (Regex.simpleMatch(excludes, snapshot.getSnapshotId().getName()) == false) {
-                            toResolve.add(snapshot);
-                        }
+            if (repositoryData != null) {
+                for (final var snapshotId : repositoryData.getSnapshotIds()) {
+                    final var snapshotName = snapshotId.getName();
+                    unmatchedRequiredNames.remove(snapshotName);
+                    if (snapshotNamePredicate.test(snapshotName, false) && matchesPredicates(snapshotId, repositoryData)) {
+                        toResolve.add(new Snapshot(repo, snapshotId));
                     }
                 }
-                if (toResolve.isEmpty() && ignoreUnavailable == false && isCurrentSnapshotsOnly() == false) {
-                    throw new SnapshotMissingException(repo, snapshots[0]);
-                }
+            }
+
+            if (unmatchedRequiredNames.isEmpty() == false) {
+                throw new SnapshotMissingException(repo, unmatchedRequiredNames.iterator().next());
             }
 
             if (verbose) {
@@ -396,13 +329,18 @@ public class TransportGetSnapshotsAction extends TransportMasterNodeAction<GetSn
             } else {
                 assert fromSortValuePredicates.isMatchAll() : "filtering is not supported in non-verbose mode";
                 assert slmPolicyPredicate == SlmPolicyPredicate.MATCH_ALL_POLICIES : "filtering is not supported in non-verbose mode";
+                final var currentSnapshots = snapshotsInProgress.forRepo(repo)
+                    .stream()
+                    .map(entry -> SnapshotInfo.inProgress(entry).basic())
+                    .toList();
+
                 final SnapshotsInRepo snapshotInfos;
                 if (repositoryData != null) {
                     // want non-current snapshots as well, which are found in the repository data
                     snapshotInfos = buildSimpleSnapshotInfos(toResolve, repo, repositoryData, currentSnapshots);
                 } else {
                     // only want current snapshots
-                    snapshotInfos = sortSnapshotsWithNoOffsetOrLimit(currentSnapshots.stream().map(SnapshotInfo::basic).toList());
+                    snapshotInfos = sortSnapshotsWithNoOffsetOrLimit(currentSnapshots);
                 }
                 listener.onResponse(snapshotInfos);
             }
@@ -487,10 +425,6 @@ public class TransportGetSnapshotsAction extends TransportMasterNodeAction<GetSn
             }
         }
 
-        private boolean isCurrentSnapshotsOnly() {
-            return snapshots.length == 1 && GetSnapshotsRequest.CURRENT_SNAPSHOT.equalsIgnoreCase(snapshots[0]);
-        }
-
         private SnapshotsInRepo buildSimpleSnapshotInfos(
             final Set<Snapshot> toResolve,
             final String repoName,
@@ -499,8 +433,9 @@ public class TransportGetSnapshotsAction extends TransportMasterNodeAction<GetSn
         ) {
             List<SnapshotInfo> snapshotInfos = new ArrayList<>();
             for (SnapshotInfo snapshotInfo : currentSnapshots) {
+                assert snapshotInfo.startTime() == 0L && snapshotInfo.endTime() == 0L && snapshotInfo.totalShards() == 0L : snapshotInfo;
                 if (toResolve.remove(snapshotInfo.snapshot())) {
-                    snapshotInfos.add(snapshotInfo.basic());
+                    snapshotInfos.add(snapshotInfo);
                 }
             }
             Map<SnapshotId, List<String>> snapshotsToIndices = new HashMap<>();

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

@@ -0,0 +1,137 @@
+/*
+ * 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 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 or the Server
+ * Side Public License, v 1.
+ */
+
+package org.elasticsearch.action.admin.cluster.snapshots.get;
+
+import org.elasticsearch.test.ESTestCase;
+
+import java.util.Set;
+
+import static org.hamcrest.Matchers.empty;
+import static org.hamcrest.Matchers.equalTo;
+
+public class SnapshotNamePredicateTests extends ESTestCase {
+
+    private static String allSnapshotsSelector() {
+        return randomFrom("_all", "_ALL", "_All", "_aLl");
+    }
+
+    public void testMatchAll() {
+        assertSame(SnapshotNamePredicate.MATCH_ALL, createPredicate(randomBoolean()));
+        assertSame(SnapshotNamePredicate.MATCH_ALL, createPredicate(randomBoolean(), "*"));
+        assertSame(SnapshotNamePredicate.MATCH_ALL, createPredicate(randomBoolean(), allSnapshotsSelector()));
+        assertTrue(SnapshotNamePredicate.MATCH_ALL.test(randomIdentifier(), randomBoolean()));
+    }
+
+    private static String currentSnapshotsSelector() {
+        return randomFrom("_current", "_CURRENT", "_Current", "_cUrReNt");
+    }
+
+    public void testMatchCurrent() {
+        assertSame(SnapshotNamePredicate.MATCH_CURRENT_ONLY, createPredicate(randomBoolean(), currentSnapshotsSelector()));
+        assertTrue(SnapshotNamePredicate.MATCH_CURRENT_ONLY.test(randomIdentifier(), true));
+        assertFalse(SnapshotNamePredicate.MATCH_CURRENT_ONLY.test(randomIdentifier(), false));
+    }
+
+    public void testMatchOneNameIgnoreUnavailable() {
+        final var requestName = randomIdentifier();
+        final var predicates = createPredicate(true, requestName);
+        assertTrue(predicates.test(requestName, randomBoolean()));
+        assertFalse(predicates.test(randomValueOtherThan(requestName, ESTestCase::randomIdentifier), randomBoolean()));
+        assertThat(predicates.requiredNames(), empty());
+    }
+
+    public void testMatchOneNameRequireAvailable() {
+        final var requestName = randomIdentifier();
+        final var predicates = createPredicate(false, requestName);
+        assertTrue(predicates.test(requestName, randomBoolean()));
+        assertFalse(predicates.test(randomValueOtherThan(requestName, ESTestCase::randomIdentifier), randomBoolean()));
+        assertEquals(predicates.requiredNames(), Set.of(requestName));
+    }
+
+    public void testMatchWildcard() {
+        final var predicates = createPredicate(randomBoolean(), "include-*");
+        assertTrue(predicates.test("include-" + randomIdentifier(), randomBoolean()));
+        assertFalse(predicates.test("exclude-" + randomIdentifier(), randomBoolean()));
+        assertThat(predicates.requiredNames(), empty());
+    }
+
+    public void testMatchWildcardAndName() {
+        final var requestName = randomIdentifier();
+        final var predicates = createPredicate(true, "include-*", requestName);
+        assertTrue(predicates.test("include-" + randomIdentifier(), randomBoolean()));
+        assertTrue(predicates.test(requestName, randomBoolean()));
+        assertFalse(predicates.test("exclude-" + randomIdentifier(), randomBoolean()));
+        assertThat(predicates.requiredNames(), empty());
+
+        assertEquals(createPredicate(false, "include-*", requestName).requiredNames(), Set.of(requestName));
+    }
+
+    public void testIncludeWildcardExcludeName() {
+        final var requestName = randomIdentifier();
+        final var predicates = createPredicate(randomBoolean(), "include-*", "-include-" + requestName);
+        assertTrue(predicates.test("include-" + randomValueOtherThan(requestName, ESTestCase::randomIdentifier), randomBoolean()));
+        assertFalse(predicates.test("include-" + requestName, randomBoolean()));
+        assertThat(predicates.requiredNames(), empty());
+    }
+
+    public void testIncludeWildcardExcludeWildcard() {
+        final var predicates = createPredicate(randomBoolean(), "include-*", "-include-exclude-*");
+        assertTrue(predicates.test("include-" + randomIdentifier(), randomBoolean()));
+        assertFalse(predicates.test("exclude-" + randomIdentifier(), randomBoolean()));
+        assertFalse(predicates.test("include-exclude-" + randomIdentifier(), randomBoolean()));
+        assertThat(predicates.requiredNames(), empty());
+    }
+
+    public void testIncludeCurrentExcludeWildcard() {
+        final var predicates = createPredicate(randomBoolean(), currentSnapshotsSelector(), "-exclude-*");
+        assertTrue(predicates.test(randomIdentifier(), true));
+        assertFalse(predicates.test("exclude-" + randomIdentifier(), randomBoolean()));
+        assertFalse(predicates.test(randomIdentifier(), false));
+        assertThat(predicates.requiredNames(), empty());
+    }
+
+    public void testIncludeCurrentAndWildcardExcludeName() {
+        final var requestName = randomIdentifier();
+        final var predicates = createPredicate(randomBoolean(), currentSnapshotsSelector(), "include-*", "-include-" + requestName);
+        assertTrue(predicates.test(randomIdentifier(), true));
+        assertTrue(predicates.test("include-" + randomValueOtherThan(requestName, ESTestCase::randomIdentifier), false));
+        assertFalse(predicates.test("include-" + requestName, randomBoolean()));
+        assertThat(predicates.requiredNames(), empty());
+    }
+
+    public void testInitialExclude() {
+        // NB current behaviour, but could be considered a bug?
+        final var requestName = "-" + randomIdentifier();
+        final var predicates = createPredicate(false, requestName);
+        assertTrue(predicates.test(requestName, randomBoolean()));
+        assertThat(predicates.requiredNames(), equalTo(Set.of(requestName)));
+    }
+
+    public void testHyphen() {
+        // NB current behaviour, but could be considered a bug?
+        final var predicates = createPredicate(false, "include-*", "-");
+        assertTrue(predicates.test("include-" + randomIdentifier(), randomBoolean()));
+        assertTrue(predicates.test("-", randomBoolean()));
+        assertThat(predicates.requiredNames(), equalTo(Set.of("-")));
+    }
+
+    public void testAllWithExclude() {
+        // NB current behaviour, but could be considered a bug?
+        final var requestName = randomIdentifier();
+        final var predicates = createPredicate(false, "_all", "-" + requestName);
+        assertFalse(predicates.test(randomIdentifier(), randomBoolean()));
+        assertTrue(predicates.test("_all", randomBoolean()));
+        assertTrue(predicates.test("-" + requestName, randomBoolean()));
+        assertThat(predicates.requiredNames(), equalTo(Set.of("_all", "-" + requestName)));
+    }
+
+    private static SnapshotNamePredicate createPredicate(boolean ignoreUnavailable, String... requestSnapshots) {
+        return SnapshotNamePredicate.forSnapshots(ignoreUnavailable, requestSnapshots);
+    }
+}