Introduce `EnumSerializationTestUtils` (#111092)

Adds a test utility for fixing the ordinal-based wire protocol of enums.

Relates #107944

server/src/main/java/org/elasticsearch/common/io/stream/StreamInput.java

@@ -1295,14 +1295,16 @@ public abstract class StreamInput extends InputStream {
     }
 
     /**
-     * Reads an enum with type E that was serialized based on the value of its ordinal
+     * Reads an enum with type {@code E} that was serialized based on the value of its ordinal. Enums serialized like this must have a
+     * corresponding test which uses {@code EnumSerializationTestUtils#assertEnumSerialization} to fix the wire protocol.
      */
     public <E extends Enum<E>> E readEnum(Class<E> enumClass) throws IOException {
         return readEnum(enumClass, enumClass.getEnumConstants());
     }
 
     /**
-     * Reads an optional enum with type E that was serialized based on the value of its ordinal
+     * Reads an optional enum with type {@code E} that was serialized based on the value of its ordinal. Enums serialized like this must
+     * have a corresponding test which uses {@code EnumSerializationTestUtils#assertEnumSerialization} to fix the wire protocol.
      */
     @Nullable
     public <E extends Enum<E>> E readOptionalEnum(Class<E> enumClass) throws IOException {
@@ -1322,7 +1324,8 @@ public abstract class StreamInput extends InputStream {
     }
 
     /**
-     * Reads an enum with type E that was serialized based on the value of it's ordinal
+     * Reads a set of enums with type {@code E} that were serialized based on the value of their ordinals. Enums serialized like this must
+     * have a corresponding test which uses {@code EnumSerializationTestUtils#assertEnumSerialization} to fix the wire protocol.
      */
     public <E extends Enum<E>> EnumSet<E> readEnumSet(Class<E> enumClass) throws IOException {
         int size = readVInt();

server/src/main/java/org/elasticsearch/common/io/stream/StreamOutput.java

@@ -1135,7 +1135,8 @@ public abstract class StreamOutput extends OutputStream {
     }
 
     /**
-     * Writes an enum with type E based on its ordinal value
+     * Writes an enum with type {@code E} in terms of the value of its ordinal. Enums serialized like this must have a corresponding test
+     * which uses {@code EnumSerializationTestUtils#assertEnumSerialization} to fix the wire protocol.
      */
     public <E extends Enum<E>> void writeEnum(E enumValue) throws IOException {
         assert enumValue instanceof XContentType == false : "XContentHelper#writeTo should be used for XContentType serialisation";
@@ -1143,7 +1144,8 @@ public abstract class StreamOutput extends OutputStream {
     }
 
     /**
-     * Writes an optional enum with type E based on its ordinal value
+     * Writes an optional enum with type {@code E} in terms of the value of its ordinal. Enums serialized like this must have a
+     * corresponding test which uses {@code EnumSerializationTestUtils#assertEnumSerialization} to fix the wire protocol.
      */
     public <E extends Enum<E>> void writeOptionalEnum(@Nullable E enumValue) throws IOException {
         if (enumValue == null) {
@@ -1156,7 +1158,8 @@ public abstract class StreamOutput extends OutputStream {
     }
 
     /**
-     * Writes an EnumSet with type E that by serialized it based on it's ordinal value
+     * Writes a set of enum with type {@code E} in terms of the value of its ordinal. Enums serialized like this must have a corresponding
+     * test which uses {@code EnumSerializationTestUtils#assertEnumSerialization} to fix the wire protocol.
      */
     public <E extends Enum<E>> void writeEnumSet(EnumSet<E> enumSet) throws IOException {
         writeVInt(enumSet.size());

server/src/test/java/org/elasticsearch/cluster/coordination/CoordinationDiagnosticsServiceTests.java

@@ -23,6 +23,7 @@ import org.elasticsearch.common.util.concurrent.DeterministicTaskQueue;
 import org.elasticsearch.core.TimeValue;
 import org.elasticsearch.core.Tuple;
 import org.elasticsearch.monitor.StatusInfo;
+import org.elasticsearch.test.EnumSerializationTestUtils;
 import org.elasticsearch.test.EqualsHashCodeTestUtils;
 import org.elasticsearch.threadpool.Scheduler;
 import org.elasticsearch.threadpool.ThreadPool;
@@ -1417,4 +1418,14 @@ public class CoordinationDiagnosticsServiceTests extends AbstractCoordinatorTest
         );
         cluster.clusterNodes.add(nonMasterNode);
     }
+
+    public void testCoordinationDiagnosticsStatusSerialization() {
+        EnumSerializationTestUtils.assertEnumSerialization(
+            CoordinationDiagnosticsStatus.class,
+            CoordinationDiagnosticsStatus.GREEN,
+            CoordinationDiagnosticsStatus.UNKNOWN,
+            CoordinationDiagnosticsStatus.YELLOW,
+            CoordinationDiagnosticsStatus.RED
+        );
+    }
 }

server/src/test/java/org/elasticsearch/transport/RemoteConnectionStrategyTests.java

@@ -13,6 +13,7 @@ import org.elasticsearch.common.settings.Settings;
 import org.elasticsearch.common.util.concurrent.ThreadContext;
 import org.elasticsearch.core.TimeValue;
 import org.elasticsearch.test.ESTestCase;
+import org.elasticsearch.test.EnumSerializationTestUtils;
 
 import static org.mockito.Mockito.mock;
 
@@ -156,6 +157,14 @@ public class RemoteConnectionStrategyTests extends ESTestCase {
         }
     }
 
+    public void testConnectionStrategySerialization() {
+        EnumSerializationTestUtils.assertEnumSerialization(
+            RemoteConnectionStrategy.ConnectionStrategy.class,
+            RemoteConnectionStrategy.ConnectionStrategy.SNIFF,
+            RemoteConnectionStrategy.ConnectionStrategy.PROXY
+        );
+    }
+
     private static class FakeConnectionStrategy extends RemoteConnectionStrategy {
 
         private final ConnectionStrategy strategy;

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

@@ -0,0 +1,63 @@
+/*
+ * 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.test;
+
+import org.elasticsearch.common.io.stream.BytesStreamOutput;
+import org.elasticsearch.common.io.stream.StreamInput;
+import org.elasticsearch.common.io.stream.StreamOutput;
+
+import java.io.IOException;
+
+import static org.elasticsearch.test.ESTestCase.assertEquals;
+
+/**
+ * Enum serialization via {@link StreamOutput#writeEnum} and {@link StreamInput#readEnum(Class)} uses the enum value's ordinal on the wire.
+ * Reordering the values in an enum, or adding a new value, will change the ordinals and is therefore a wire protocol change, but it's easy
+ * to miss this fact in the context of a larger commit. To protect against this trap, any enums that we send over the wire should have a
+ * test that uses {@link #assertEnumSerialization} to assert a fixed mapping between ordinals and values. That way, a change to the ordinals
+ * will require a test change, and thus some thought about BwC.
+ */
+public class EnumSerializationTestUtils {
+    private EnumSerializationTestUtils() {/* no instances */}
+
+    /**
+     * Assert that the enum constants of the given class are exactly the ones passed in explicitly as arguments, which fixes its wire
+     * protocol when using {@link StreamOutput#writeEnum} and {@link StreamInput#readEnum(Class)}.
+     *
+     * @param value0 The enum constant with ordinal {@code 0}, passed as a separate argument to avoid prevent callers just lazily using
+     *               {@code EnumClass.values()} to pass the values of the enum, which would negate the point of this test.
+     * @param values The remaining enum constants, in ordinal order.
+     */
+    @SafeVarargs
+    public static <E extends Enum<E>> void assertEnumSerialization(Class<E> clazz, E value0, E... values) {
+        final var enumConstants = clazz.getEnumConstants();
+        assertEquals(clazz.getCanonicalName(), enumConstants.length, values.length + 1);
+        for (var ordinal = 0; ordinal < values.length + 1; ordinal++) {
+            final var enumValue = ordinal == 0 ? value0 : values[ordinal - 1];
+            final var description = clazz.getCanonicalName() + "[" + ordinal + "]";
+            assertEquals(description, enumConstants[ordinal], enumValue);
+            try (var out = new BytesStreamOutput(1)) {
+                out.writeEnum(enumValue);
+                try (var in = out.bytes().streamInput()) {
+                    assertEquals(description, ordinal, in.readVInt());
+                }
+            } catch (IOException e) {
+                ESTestCase.fail(e);
+            }
+            try (var out = new BytesStreamOutput(1)) {
+                out.writeVInt(ordinal);
+                try (var in = out.bytes().streamInput()) {
+                    assertEquals(description, enumValue, in.readEnum(clazz));
+                }
+            } catch (IOException e) {
+                ESTestCase.fail(e);
+            }
+        }
+    }
+}