Improve serialization tests, address code review comments

Author: JeremyDahlgren

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

@@ -254,10 +254,11 @@ public void testCorrectCountsForDoneShards() throws Exception {
         internalCluster().restartNode(dataNodeTwo);
 
         final var snapshotStatusAfterRestart = getSnapshotStatus(repoName, snapshotOne);
-        final var snapshotShardStateAfterNodeRestart = stateFirstShard(snapshotStatusAfterRestart, indexTwo);
-        assertThat(snapshotShardStateAfterNodeRestart.getStage(), is(SnapshotIndexShardStage.DONE));
-        assertNotNull("expected a non-null description string for missing stats", snapshotShardStateAfterNodeRestart.getDescription());
-        final var missingStats = snapshotShardStateAfterNodeRestart.getStats();
+
+        final var snapshotShardStateIndexTwo = stateFirstShard(snapshotStatusAfterRestart, indexTwo);
+        assertThat(snapshotShardStateIndexTwo.getStage(), is(SnapshotIndexShardStage.DONE));
+        assertNotNull("expected a non-null description string for missing stats", snapshotShardStateIndexTwo.getDescription());
+        final var missingStats = snapshotShardStateIndexTwo.getStats();
         assertThat(missingStats.getTotalFileCount(), equalTo(-1));
         assertThat(missingStats.getTotalSize(), equalTo(-1L));
 

server/src/main/java/org/elasticsearch/action/admin/cluster/snapshots/status/SnapshotIndexShardStatus.java

@@ -101,12 +101,13 @@ public SnapshotIndexShardStatus(StreamInput in) throws IOException {
     }
 
     /**
-     * Creates an instance for scenarios where the snapshot stats are unavailable, with a non-null description of why the stats are missing.
+     * Creates an instance for scenarios where the snapshot is {@link SnapshotIndexShardStage#DONE} but the stats are unavailable, with a
+     * non-null description of why the stats are missing.
      */
-    public static SnapshotIndexShardStatus forMissingStats(ShardId shardId, SnapshotIndexShardStage stage, String description) {
+    public static SnapshotIndexShardStatus forDoneButMissingStats(ShardId shardId, String description) {
         return new SnapshotIndexShardStatus(
             shardId,
-            stage,
+            SnapshotIndexShardStage.DONE,
             SnapshotStats.forMissingStats(),
             null,
             null,

server/src/main/java/org/elasticsearch/action/admin/cluster/snapshots/status/SnapshotStats.java

@@ -39,6 +39,7 @@ public class SnapshotStats implements Writeable, ToXContentObject {
     SnapshotStats() {}
 
     SnapshotStats(StreamInput in) throws IOException {
+        // We use a boolean to indicate if the stats are present (true) or missing (false), to skip writing all the values if missing.
         if (in.getTransportVersion().onOrAfter(SNAPSHOT_INDEX_SHARD_STATUS_MISSING_STATS) && in.readBoolean() == false) {
             startTime = 0L;
             time = 0L;
@@ -158,6 +159,7 @@ public long getProcessedSize() {
     @Override
     public void writeTo(StreamOutput out) throws IOException {
         if (out.getTransportVersion().onOrAfter(SNAPSHOT_INDEX_SHARD_STATUS_MISSING_STATS)) {
+            // We use a boolean to indicate if the stats are present (true) or missing (false), to skip writing all the values if missing.
             if (isMissingStats()) {
                 out.writeBoolean(false);
                 return;
@@ -326,6 +328,11 @@ public static SnapshotStats fromXContent(XContentParser parser) throws IOExcepti
                 }
             }
         }
+        // For missing stats incrementalFileCount will be -1, and we expect processedFileCount and processedSize to be omitted (still zero).
+        if (incrementalFileCount == -1) {
+            assert processedFileCount == 0 && processedSize == 0L && incrementalSize == -1L && totalFileCount == -1 && totalSize == -1L;
+            return SnapshotStats.forMissingStats();
+        }
         return new SnapshotStats(
             startTime,
             time,

server/src/main/java/org/elasticsearch/action/admin/cluster/snapshots/status/TransportSnapshotsStatusAction.java

@@ -244,7 +244,7 @@ void buildResponse(
                         // expensive, we choose instead to provide a message to the caller explaining why the stats are missing and the API
                         // that can be used to load them once the snapshot has completed.
                         if (minTransportVersion.onOrAfter(TransportVersions.SNAPSHOT_INDEX_SHARD_STATUS_MISSING_STATS)) {
-                            shardStatus = SnapshotIndexShardStatus.forMissingStats(shardId, stage, """
+                            shardStatus = SnapshotIndexShardStatus.forDoneButMissingStats(shardId, """
                                 Snapshot shard stats missing from a currently running snapshot due to a node leaving the cluster after \
                                 completing the shard snapshot; use /_snapshot/<repository>/<snapshot>/_status to load from the repository \
                                 once the snapshot has completed.""");

server/src/test/java/org/elasticsearch/action/admin/cluster/snapshots/status/SnapshotIndexShardStatusTests.java

@@ -123,4 +123,8 @@ public static SnapshotIndexShardStatus fromXContent(XContentParser parser, Strin
         XContentParserUtils.ensureExpectedToken(XContentParser.Token.FIELD_NAME, parser.currentToken(), parser);
         return PARSER.parse(parser, indexId, parser.currentName());
     }
+
+    public void testForDoneButMissingStatsXContentSerialization() throws IOException {
+        testFromXContent(() -> SnapshotIndexShardStatus.forDoneButMissingStats(createTestInstance().getShardId(), randomAlphaOfLength(16)));
+    }
 }

server/src/test/java/org/elasticsearch/action/admin/cluster/snapshots/status/SnapshotStatsTests.java

@@ -67,7 +67,7 @@ public void testMissingStats() throws IOException {
         assertEquals(-1L, missingStats.getIncrementalSize());
         assertEquals(-1L, missingStats.getProcessedSize());
 
-        // Verify round trip serialization.
+        // Verify round trip Transport serialization.
         for (var transportVersion : List.of(
             TransportVersions.MINIMUM_COMPATIBLE,
             TransportVersions.SNAPSHOT_INDEX_SHARD_STATUS_MISSING_STATS,
@@ -95,5 +95,8 @@ public void testMissingStats() throws IOException {
                 }
             }
         }
+
+        // Verify round trip XContent serialization.
+        testFromXContent(SnapshotStats::forMissingStats);
     }
 }

test/framework/src/main/java/org/elasticsearch/test/AbstractXContentTestCase.java

@@ -284,9 +284,16 @@ public static <T extends ToXContent> void testFromXContent(
      * both for equality and asserts equality on the two queries.
      */
     public final void testFromXContent() throws IOException {
+        testFromXContent(this::createTestInstance);
+    }
+
+    /**
+     * Generic test that creates a new instance using the given supplier and verifies XContent round trip serialization.
+     */
+    public final void testFromXContent(Supplier<T> testInstanceSupplier) throws IOException {
         testFromXContent(
             NUMBER_OF_TEST_RUNS,
-            this::createTestInstance,
+            testInstanceSupplier,
             supportsUnknownFields(),
             getShuffleFieldsExceptions(),
             getRandomFieldsExcludeFilter(),