Enhance Tests around SnapshotInfo UserMetadata (#74362)

We barely test the correct handling of user metadata directly.
With upcoming changes to how `SnapshotInfo` is stored it would be nice
to have better test coverage. This PR adds randomized coverage of serializing
user metadata to a large number of tests that all user the shared infrastructure
that is adjusted here.

Author: original-brownbear

client/rest-high-level/src/test/java/org/elasticsearch/client/SnapshotIT.java

@@ -33,12 +33,12 @@
 import org.elasticsearch.common.xcontent.XContentType;
 import org.elasticsearch.repositories.fs.FsRepository;
 import org.elasticsearch.rest.RestStatus;
+import org.elasticsearch.snapshots.AbstractSnapshotIntegTestCase;
 import org.elasticsearch.snapshots.RestoreInfo;
 import org.mockito.internal.util.collections.Sets;
 
 import java.io.IOException;
 import java.util.Collections;
-import java.util.HashMap;
 import java.util.List;
 import java.util.Map;
 
@@ -149,7 +149,7 @@ public void testCreateSnapshot() throws Exception {
         boolean waitForCompletion = randomBoolean();
         request.waitForCompletion(waitForCompletion);
         if (randomBoolean()) {
-            request.userMetadata(randomUserMetadata());
+            request.userMetadata(AbstractSnapshotIntegTestCase.randomUserMetadata());
         }
         request.partial(randomBoolean());
         request.includeGlobalState(randomBoolean());
@@ -193,7 +193,7 @@ public void testGetSnapshots() throws IOException {
         CreateSnapshotResponse putSnapshotResponse1 = createTestSnapshot(createSnapshotRequest1);
         CreateSnapshotRequest createSnapshotRequest2 = new CreateSnapshotRequest(repository2, snapshot2);
         createSnapshotRequest2.waitForCompletion(true);
-        Map<String, Object> originalMetadata = randomUserMetadata();
+        Map<String, Object> originalMetadata = AbstractSnapshotIntegTestCase.randomUserMetadata();
         createSnapshotRequest2.userMetadata(originalMetadata);
         CreateSnapshotResponse putSnapshotResponse2 = createTestSnapshot(createSnapshotRequest2);
         // check that the request went ok without parsing JSON here. When using the high level client, check acknowledgement instead.
@@ -264,7 +264,7 @@ public void testRestoreSnapshot() throws IOException {
         createSnapshotRequest.indices(testIndex);
         createSnapshotRequest.waitForCompletion(true);
         if (randomBoolean()) {
-            createSnapshotRequest.userMetadata(randomUserMetadata());
+            createSnapshotRequest.userMetadata(AbstractSnapshotIntegTestCase.randomUserMetadata());
         }
         CreateSnapshotResponse createSnapshotResponse = createTestSnapshot(createSnapshotRequest);
         assertEquals(RestStatus.OK, createSnapshotResponse.status());
@@ -311,7 +311,7 @@ public void testSnapshotHidden() throws IOException {
         createSnapshotRequest.indices("*");
         createSnapshotRequest.waitForCompletion(true);
         if (randomBoolean()) {
-            createSnapshotRequest.userMetadata(randomUserMetadata());
+            createSnapshotRequest.userMetadata(AbstractSnapshotIntegTestCase.randomUserMetadata());
         }
         CreateSnapshotResponse createSnapshotResponse = createTestSnapshot(createSnapshotRequest);
         assertEquals(RestStatus.OK, createSnapshotResponse.status());
@@ -344,7 +344,7 @@ public void testDeleteSnapshot() throws IOException {
         CreateSnapshotRequest createSnapshotRequest = new CreateSnapshotRequest(repository, snapshot);
         createSnapshotRequest.waitForCompletion(true);
         if (randomBoolean()) {
-            createSnapshotRequest.userMetadata(randomUserMetadata());
+            createSnapshotRequest.userMetadata(AbstractSnapshotIntegTestCase.randomUserMetadata());
         }
         CreateSnapshotResponse createSnapshotResponse = createTestSnapshot(createSnapshotRequest);
         // check that the request went ok without parsing JSON here. When using the high level client, check acknowledgement instead.
@@ -380,27 +380,4 @@ public void testCloneSnapshot() throws IOException {
         assertTrue(response.isAcknowledged());
     }
 
-    private static Map<String, Object> randomUserMetadata() {
-        if (randomBoolean()) {
-            return null;
-        }
-
-        Map<String, Object> metadata = new HashMap<>();
-        long fields = randomLongBetween(0, 4);
-        for (int i = 0; i < fields; i++) {
-            if (randomBoolean()) {
-                metadata.put(randomValueOtherThanMany(metadata::containsKey, () -> randomAlphaOfLengthBetween(2,10)),
-                        randomAlphaOfLengthBetween(5, 5));
-            } else {
-                Map<String, Object> nested = new HashMap<>();
-                long nestedFields = randomLongBetween(0, 4);
-                for (int j = 0; j < nestedFields; j++) {
-                    nested.put(randomValueOtherThanMany(nested::containsKey, () -> randomAlphaOfLengthBetween(2,10)),
-                            randomAlphaOfLengthBetween(5, 5));
-                }
-                metadata.put(randomValueOtherThanMany(metadata::containsKey, () -> randomAlphaOfLengthBetween(2,10)), nested);
-            }
-        }
-        return metadata;
-    }
 }

server/src/main/java/org/elasticsearch/action/admin/cluster/snapshots/create/CreateSnapshotRequest.java

@@ -22,6 +22,7 @@
 import org.elasticsearch.common.xcontent.ToXContentObject;
 import org.elasticsearch.common.xcontent.XContentBuilder;
 import org.elasticsearch.common.xcontent.XContentFactory;
+import org.elasticsearch.core.Nullable;
 
 import java.io.IOException;
 import java.util.Arrays;
@@ -74,6 +75,7 @@ public class CreateSnapshotRequest extends MasterNodeRequest<CreateSnapshotReque
 
     private boolean waitForCompletion;
 
+    @Nullable
     private Map<String, Object> userMetadata;
 
     public CreateSnapshotRequest() {}
@@ -338,11 +340,19 @@ public boolean includeGlobalState() {
         return includeGlobalState;
     }
 
+    /**
+     * @return user metadata map that should be stored with the snapshot or {@code null} if there is no user metadata to be associated with
+     *         this snapshot
+     */
+    @Nullable
     public Map<String, Object> userMetadata() {
         return userMetadata;
     }
 
-    public CreateSnapshotRequest userMetadata(Map<String, Object> userMetadata) {
+    /**
+     * @param userMetadata user metadata map that should be stored with the snapshot
+     */
+    public CreateSnapshotRequest userMetadata(@Nullable Map<String, Object> userMetadata) {
         this.userMetadata = userMetadata;
         return this;
     }
@@ -379,29 +389,35 @@ public CreateSnapshotRequest featureStates(List<String> featureStates) {
     public CreateSnapshotRequest source(Map<String, Object> source) {
         for (Map.Entry<String, Object> entry : source.entrySet()) {
             String name = entry.getKey();
-            if (name.equals("indices")) {
-                if (entry.getValue() instanceof String) {
-                    indices(Strings.splitStringByCommaToArray((String) entry.getValue()));
-                } else if (entry.getValue() instanceof List) {
-                    indices((List<String>) entry.getValue());
-                } else {
-                    throw new IllegalArgumentException("malformed indices section, should be an array of strings");
-                }
-            } else if (name.equals("feature_states")) {
-                if (entry.getValue() instanceof List) {
-                    featureStates((List<String>) entry.getValue());
-                } else {
-                    throw new IllegalArgumentException("malformed feature_states section, should be an array of strings");
-                }
-            } else if (name.equals("partial")) {
-                partial(nodeBooleanValue(entry.getValue(), "partial"));
-            } else if (name.equals("include_global_state")) {
-                includeGlobalState = nodeBooleanValue(entry.getValue(), "include_global_state");
-            } else if (name.equals("metadata")) {
-                if (entry.getValue() != null && (entry.getValue() instanceof Map == false)) {
-                    throw new IllegalArgumentException("malformed metadata, should be an object");
-                }
-                userMetadata((Map<String, Object>) entry.getValue());
+            switch (name) {
+                case "indices":
+                    if (entry.getValue() instanceof String) {
+                        indices(Strings.splitStringByCommaToArray((String) entry.getValue()));
+                    } else if (entry.getValue() instanceof List) {
+                        indices((List<String>) entry.getValue());
+                    } else {
+                        throw new IllegalArgumentException("malformed indices section, should be an array of strings");
+                    }
+                    break;
+                case "feature_states":
+                    if (entry.getValue() instanceof List) {
+                        featureStates((List<String>) entry.getValue());
+                    } else {
+                        throw new IllegalArgumentException("malformed feature_states section, should be an array of strings");
+                    }
+                    break;
+                case "partial":
+                    partial(nodeBooleanValue(entry.getValue(), "partial"));
+                    break;
+                case "include_global_state":
+                    includeGlobalState = nodeBooleanValue(entry.getValue(), "include_global_state");
+                    break;
+                case "metadata":
+                    if (entry.getValue() != null && (entry.getValue() instanceof Map == false)) {
+                        throw new IllegalArgumentException("malformed metadata, should be an object");
+                    }
+                    userMetadata((Map<String, Object>) entry.getValue());
+                    break;
             }
         }
         indicesOptions(IndicesOptions.fromMap(source, indicesOptions));

server/src/main/java/org/elasticsearch/action/admin/cluster/snapshots/create/CreateSnapshotRequestBuilder.java

@@ -11,6 +11,9 @@
 import org.elasticsearch.action.support.IndicesOptions;
 import org.elasticsearch.action.support.master.MasterNodeOperationRequestBuilder;
 import org.elasticsearch.client.ElasticsearchClient;
+import org.elasticsearch.core.Nullable;
+
+import java.util.Map;
 
 /**
  * Create snapshot request builder
@@ -124,4 +127,15 @@ public CreateSnapshotRequestBuilder setFeatureStates(String... featureStates) {
         request.featureStates(featureStates);
         return this;
     }
+
+    /**
+     * Provide a map of user metadata that should be included in the snapshot metadata.
+     *
+     * @param metadata user metadata map
+     * @return this builder
+     */
+    public CreateSnapshotRequestBuilder setUserMetadata(@Nullable Map<String, Object> metadata) {
+        request.userMetadata(metadata);
+        return this;
+    }
 }

test/framework/src/main/java/org/elasticsearch/snapshots/AbstractSnapshotIntegTestCase.java

@@ -72,6 +72,7 @@
 import java.util.Arrays;
 import java.util.Collection;
 import java.util.Collections;
+import java.util.HashMap;
 import java.util.List;
 import java.util.Locale;
 import java.util.Map;
@@ -654,7 +655,21 @@ public static List<String> createNSnapshots(Logger logger, String repoName, int
         for (int i = 0; i < count; i++) {
             final String snapshot = prefix + i;
             snapshotNames.add(snapshot);
-            client().admin().cluster().prepareCreateSnapshot(repoName, snapshot).setWaitForCompletion(true).execute(snapshotsListener);
+            final Map<String, Object> userMetadata = randomUserMetadata();
+            clusterAdmin()
+                    .prepareCreateSnapshot(repoName, snapshot)
+                    .setWaitForCompletion(true)
+                    .setUserMetadata(userMetadata)
+                    .execute(snapshotsListener.delegateFailure((l, response) -> {
+                        final SnapshotInfo snapshotInfoInResponse = response.getSnapshotInfo();
+                        assertEquals(userMetadata, snapshotInfoInResponse.userMetadata());
+                        clusterAdmin().prepareGetSnapshots(repoName)
+                                .setSnapshots(snapshot)
+                                .execute(l.delegateFailure((ll, getResponse) -> {
+                                    assertEquals(snapshotInfoInResponse, getResponse.getSnapshots(repoName).get(0));
+                                    ll.onResponse(response);
+                                }));
+                    }));
         }
         for (CreateSnapshotResponse snapshotResponse : allSnapshotsDone.get()) {
             assertThat(snapshotResponse.getSnapshotInfo().state(), is(SnapshotState.SUCCESS));
@@ -708,4 +723,34 @@ public static void assertSnapshotListSorted(List<SnapshotInfo> snapshotInfos, @N
             orderAssertion.accept(snapshotInfos.get(i), snapshotInfos.get(i + 1));
         }
     }
+
+    /**
+     * Randomly either generates some random snapshot user metadata or returns {@code null}.
+     *
+     * @return random snapshot user metadata or {@code null}
+     */
+    @Nullable
+    public static Map<String, Object> randomUserMetadata() {
+        if (randomBoolean()) {
+            return null;
+        }
+
+        Map<String, Object> metadata = new HashMap<>();
+        long fields = randomLongBetween(0, 4);
+        for (int i = 0; i < fields; i++) {
+            if (randomBoolean()) {
+                metadata.put(randomValueOtherThanMany(metadata::containsKey, () -> randomAlphaOfLengthBetween(2, 10)),
+                        randomAlphaOfLengthBetween(5, 5));
+            } else {
+                Map<String, Object> nested = new HashMap<>();
+                long nestedFields = randomLongBetween(0, 4);
+                for (int j = 0; j < nestedFields; j++) {
+                    nested.put(randomValueOtherThanMany(nested::containsKey, () -> randomAlphaOfLengthBetween(2, 10)),
+                            randomAlphaOfLengthBetween(5, 5));
+                }
+                metadata.put(randomValueOtherThanMany(metadata::containsKey, () -> randomAlphaOfLengthBetween(2, 10)), nested);
+            }
+        }
+        return metadata;
+    }
 }