Serialize NodesInfoRequest as a set of strings (#53140)

For Node Info to be pluggable, NodesInfoRequest must be able to carry
arbitrary strings. This commit reworks the internals of that class to
use a set rather than hard-coded boolean fields.

NodesInfoRequest defaults to specifying all values. We test for
this behavior as we refactor and use random testing for the
various combinations of metrics.

Add backwards compatibility for transport requests.

build.gradle

@@ -220,8 +220,8 @@ task verifyVersions {
  * after the backport of the backcompat code is complete.
  */
 
-boolean bwc_tests_enabled = true
-final String bwc_tests_disabled_issue = "" /* place a PR link here when committing bwc changes */
+boolean bwc_tests_enabled = false
+final String bwc_tests_disabled_issue = "https://github.com/elastic/elasticsearch/pull/53140" /* place a PR link here when committing bwc changes */
 if (bwc_tests_enabled == false) {
   if (bwc_tests_disabled_issue.isEmpty()) {
     throw new GradleException("bwc_tests_disabled_issue must be set when bwc_tests_enabled == false")

server/src/main/java/org/elasticsearch/action/admin/cluster/node/info/NodesInfoRequest.java

@@ -19,40 +19,48 @@
 
 package org.elasticsearch.action.admin.cluster.node.info;
 
+import org.elasticsearch.Version;
 import org.elasticsearch.action.support.nodes.BaseNodesRequest;
 import org.elasticsearch.common.io.stream.StreamInput;
 import org.elasticsearch.common.io.stream.StreamOutput;
 
 import java.io.IOException;
+import java.util.Arrays;
+import java.util.Set;
+import java.util.stream.Collectors;
 
 /**
  * A request to get node (cluster) level information.
  */
 public class NodesInfoRequest extends BaseNodesRequest<NodesInfoRequest> {
 
-    private boolean settings = true;
-    private boolean os = true;
-    private boolean process = true;
-    private boolean jvm = true;
-    private boolean threadPool = true;
-    private boolean transport = true;
-    private boolean http = true;
-    private boolean plugins = true;
-    private boolean ingest = true;
-    private boolean indices = true;
+    private Set<String> requestedMetrics = Metrics.allMetrics();
 
+    /**
+     * Create a new NodeInfoRequest from a {@link StreamInput} object.
+     *
+     * @param in A stream input object.
+     * @throws IOException if the stream cannot be deserialized.
+     */
     public NodesInfoRequest(StreamInput in) throws IOException {
         super(in);
-        settings = in.readBoolean();
-        os = in.readBoolean();
-        process = in.readBoolean();
-        jvm = in.readBoolean();
-        threadPool = in.readBoolean();
-        transport = in.readBoolean();
-        http = in.readBoolean();
-        plugins = in.readBoolean();
-        ingest = in.readBoolean();
-        indices = in.readBoolean();
+        requestedMetrics.clear();
+        if (in.getVersion().before(Version.V_7_7_0)){
+            // prior to version 8.x, a NodesInfoRequest was serialized as a list
+            // of booleans in a fixed order
+            addOrRemoveMetric(in.readBoolean(), Metrics.SETTINGS.metricName());
+            addOrRemoveMetric(in.readBoolean(), Metrics.OS.metricName());
+            addOrRemoveMetric(in.readBoolean(), Metrics.PROCESS.metricName());
+            addOrRemoveMetric(in.readBoolean(), Metrics.JVM.metricName());
+            addOrRemoveMetric(in.readBoolean(), Metrics.THREAD_POOL.metricName());
+            addOrRemoveMetric(in.readBoolean(), Metrics.TRANSPORT.metricName());
+            addOrRemoveMetric(in.readBoolean(), Metrics.HTTP.metricName());
+            addOrRemoveMetric(in.readBoolean(), Metrics.PLUGINS.metricName());
+            addOrRemoveMetric(in.readBoolean(), Metrics.INGEST.metricName());
+            addOrRemoveMetric(in.readBoolean(), Metrics.INDICES.metricName());
+        } else {
+            requestedMetrics.addAll(Arrays.asList(in.readStringArray()));
+        }
     }
 
     /**
@@ -61,22 +69,14 @@ public class NodesInfoRequest extends BaseNodesRequest<NodesInfoRequest> {
      */
     public NodesInfoRequest(String... nodesIds) {
         super(nodesIds);
+        all();
     }
 
     /**
      * Clears all info flags.
      */
     public NodesInfoRequest clear() {
-        settings = false;
-        os = false;
-        process = false;
-        jvm = false;
-        threadPool = false;
-        transport = false;
-        http = false;
-        plugins = false;
-        ingest = false;
-        indices = false;
+        requestedMetrics.clear();
         return this;
     }
 
@@ -84,16 +84,7 @@ public class NodesInfoRequest extends BaseNodesRequest<NodesInfoRequest> {
      * Sets to return all the data.
      */
     public NodesInfoRequest all() {
-        settings = true;
-        os = true;
-        process = true;
-        jvm = true;
-        threadPool = true;
-        transport = true;
-        http = true;
-        plugins = true;
-        ingest = true;
-        indices = true;
+        requestedMetrics.addAll(Metrics.allMetrics());
         return this;
     }
 
@@ -101,14 +92,14 @@ public class NodesInfoRequest extends BaseNodesRequest<NodesInfoRequest> {
      * Should the node settings be returned.
      */
     public boolean settings() {
-        return this.settings;
+        return Metrics.SETTINGS.containedIn(requestedMetrics);
     }
 
     /**
      * Should the node settings be returned.
      */
     public NodesInfoRequest settings(boolean settings) {
-        this.settings = settings;
+        addOrRemoveMetric(settings, Metrics.SETTINGS.metricName());
         return this;
     }
 
@@ -116,14 +107,14 @@ public class NodesInfoRequest extends BaseNodesRequest<NodesInfoRequest> {
      * Should the node OS be returned.
      */
     public boolean os() {
-        return this.os;
+        return Metrics.OS.containedIn(requestedMetrics);
     }
 
     /**
      * Should the node OS be returned.
      */
     public NodesInfoRequest os(boolean os) {
-        this.os = os;
+        addOrRemoveMetric(os, Metrics.OS.metricName());
         return this;
     }
 
@@ -131,14 +122,14 @@ public class NodesInfoRequest extends BaseNodesRequest<NodesInfoRequest> {
      * Should the node Process be returned.
      */
     public boolean process() {
-        return this.process;
+        return Metrics.PROCESS.containedIn(requestedMetrics);
     }
 
     /**
      * Should the node Process be returned.
      */
     public NodesInfoRequest process(boolean process) {
-        this.process = process;
+        addOrRemoveMetric(process, Metrics.PROCESS.metricName());
         return this;
     }
 
@@ -146,14 +137,14 @@ public class NodesInfoRequest extends BaseNodesRequest<NodesInfoRequest> {
      * Should the node JVM be returned.
      */
     public boolean jvm() {
-        return this.jvm;
+        return Metrics.JVM.containedIn(requestedMetrics);
     }
 
     /**
      * Should the node JVM be returned.
      */
     public NodesInfoRequest jvm(boolean jvm) {
-        this.jvm = jvm;
+        addOrRemoveMetric(jvm, Metrics.JVM.metricName());
         return this;
     }
 
@@ -161,14 +152,14 @@ public class NodesInfoRequest extends BaseNodesRequest<NodesInfoRequest> {
      * Should the node Thread Pool info be returned.
      */
     public boolean threadPool() {
-        return this.threadPool;
+        return Metrics.THREAD_POOL.containedIn(requestedMetrics);
     }
 
     /**
      * Should the node Thread Pool info be returned.
      */
     public NodesInfoRequest threadPool(boolean threadPool) {
-        this.threadPool = threadPool;
+        addOrRemoveMetric(threadPool, Metrics.THREAD_POOL.metricName());
         return this;
     }
 
@@ -176,14 +167,14 @@ public class NodesInfoRequest extends BaseNodesRequest<NodesInfoRequest> {
      * Should the node Transport be returned.
      */
     public boolean transport() {
-        return this.transport;
+        return Metrics.TRANSPORT.containedIn(requestedMetrics);
     }
 
     /**
      * Should the node Transport be returned.
      */
     public NodesInfoRequest transport(boolean transport) {
-        this.transport = transport;
+        addOrRemoveMetric(transport, Metrics.TRANSPORT.metricName());
         return this;
     }
 
@@ -191,14 +182,14 @@ public class NodesInfoRequest extends BaseNodesRequest<NodesInfoRequest> {
      * Should the node HTTP be returned.
      */
     public boolean http() {
-        return this.http;
+        return Metrics.HTTP.containedIn(requestedMetrics);
     }
 
     /**
      * Should the node HTTP be returned.
      */
     public NodesInfoRequest http(boolean http) {
-        this.http = http;
+        addOrRemoveMetric(http, Metrics.HTTP.metricName());
         return this;
     }
 
@@ -208,7 +199,7 @@ public class NodesInfoRequest extends BaseNodesRequest<NodesInfoRequest> {
      * @return The request
      */
     public NodesInfoRequest plugins(boolean plugins) {
-        this.plugins = plugins;
+        addOrRemoveMetric(plugins, Metrics.PLUGINS.metricName());
         return this;
     }
 
@@ -216,7 +207,7 @@ public class NodesInfoRequest extends BaseNodesRequest<NodesInfoRequest> {
      * @return true if information about plugins is requested
      */
     public boolean plugins() {
-        return plugins;
+        return Metrics.PLUGINS.containedIn(requestedMetrics);
     }
 
     /**
@@ -224,7 +215,7 @@ public class NodesInfoRequest extends BaseNodesRequest<NodesInfoRequest> {
      * @param ingest true if you want info
      */
     public NodesInfoRequest ingest(boolean ingest) {
-        this.ingest = ingest;
+        addOrRemoveMetric(ingest, Metrics.INGEST.metricName());
         return this;
     }
 
@@ -232,7 +223,7 @@ public class NodesInfoRequest extends BaseNodesRequest<NodesInfoRequest> {
      * @return true if information about ingest is requested
      */
     public boolean ingest() {
-        return ingest;
+        return Metrics.INGEST.containedIn(requestedMetrics);
     }
 
     /**
@@ -240,7 +231,7 @@ public class NodesInfoRequest extends BaseNodesRequest<NodesInfoRequest> {
      * @param indices true if you want info
      */
     public NodesInfoRequest indices(boolean indices) {
-        this.indices = indices;
+        addOrRemoveMetric(indices, Metrics.INDICES.metricName());
         return this;
     }
 
@@ -248,21 +239,76 @@ public class NodesInfoRequest extends BaseNodesRequest<NodesInfoRequest> {
      * @return true if information about indices (currently just indexing buffers)
      */
     public boolean indices() {
-        return indices;
+        return Metrics.INDICES.containedIn(requestedMetrics);
+    }
+
+    /**
+     * Helper method for adding and removing metrics.
+     * @param includeMetric Whether or not to include a metric.
+     * @param metricName Name of the metric to include or remove.
+     */
+    private void addOrRemoveMetric(boolean includeMetric, String metricName) {
+        if (includeMetric) {
+            requestedMetrics.add(metricName);
+        } else {
+            requestedMetrics.remove(metricName);
+        }
     }
 
     @Override
     public void writeTo(StreamOutput out) throws IOException {
         super.writeTo(out);
-        out.writeBoolean(settings);
-        out.writeBoolean(os);
-        out.writeBoolean(process);
-        out.writeBoolean(jvm);
-        out.writeBoolean(threadPool);
-        out.writeBoolean(transport);
-        out.writeBoolean(http);
-        out.writeBoolean(plugins);
-        out.writeBoolean(ingest);
-        out.writeBoolean(indices);
+        if (out.getVersion().before(Version.V_7_7_0)){
+            // prior to version 8.x, a NodesInfoRequest was serialized as a list
+            // of booleans in a fixed order
+            out.writeBoolean(Metrics.SETTINGS.containedIn(requestedMetrics));
+            out.writeBoolean(Metrics.OS.containedIn(requestedMetrics));
+            out.writeBoolean(Metrics.PROCESS.containedIn(requestedMetrics));
+            out.writeBoolean(Metrics.JVM.containedIn(requestedMetrics));
+            out.writeBoolean(Metrics.THREAD_POOL.containedIn(requestedMetrics));
+            out.writeBoolean(Metrics.TRANSPORT.containedIn(requestedMetrics));
+            out.writeBoolean(Metrics.HTTP.containedIn(requestedMetrics));
+            out.writeBoolean(Metrics.PLUGINS.containedIn(requestedMetrics));
+            out.writeBoolean(Metrics.INGEST.containedIn(requestedMetrics));
+            out.writeBoolean(Metrics.INDICES.containedIn(requestedMetrics));
+        } else {
+            out.writeStringArray(requestedMetrics.toArray(String[]::new));
+        }
+    }
+
+    /**
+     * An enumeration of the "core" sections of metrics that may be requested
+     * from the nodes information endpoint. Eventually this list list will be
+     * pluggable.
+     */
+    enum Metrics {
+        SETTINGS("settings"),
+        OS("os"),
+        PROCESS("process"),
+        JVM("jvm"),
+        THREAD_POOL("threadPool"),
+        TRANSPORT("transport"),
+        HTTP("http"),
+        PLUGINS("plugins"),
+        INGEST("ingest"),
+        INDICES("indices");
+
+        private String metricName;
+
+        Metrics(String name) {
+            this.metricName = name;
+        }
+
+        String metricName() {
+            return this.metricName;
+        }
+
+        boolean containedIn(Set<String> metricNames) {
+            return metricNames.contains(this.metricName());
+        }
+
+        static Set<String> allMetrics() {
+            return Arrays.stream(values()).map(Metrics::metricName).collect(Collectors.toSet());
+        }
     }
 }

server/src/test/java/org/elasticsearch/action/admin/cluster/node/info/NodesInfoRequestTests.java

@@ -0,0 +1,144 @@
+/*
+ * Licensed to Elasticsearch under one or more contributor
+ * license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright
+ * ownership. Elasticsearch licenses this file to you under
+ * the Apache License, Version 2.0 (the "License"); you may
+ * not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *    http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.elasticsearch.action.admin.cluster.node.info;
+
+import org.elasticsearch.common.io.stream.BytesStreamOutput;
+import org.elasticsearch.common.io.stream.StreamInput;
+import org.elasticsearch.test.ESTestCase;
+
+import static org.hamcrest.Matchers.equalTo;
+
+/**
+ * Granular tests for the {@link NodesInfoRequest} class. Higher-level tests
+ * can be found in {@link org.elasticsearch.rest.action.admin.cluster.RestNodesInfoActionTests}.
+ */
+public class NodesInfoRequestTests extends ESTestCase {
+
+    /**
+     * Make sure that we can set, serialize, and deserialize arbitrary sets
+     * of metrics.
+     *
+     * TODO: Once we can set values by string, use a collection rather than
+     *   checking each and every setter in the public API
+     */
+    public void testMetricsSetters() throws Exception {
+        NodesInfoRequest request = new NodesInfoRequest(randomAlphaOfLength(8));
+        request.settings(randomBoolean());
+        request.os(randomBoolean());
+        request.process(randomBoolean());
+        request.jvm(randomBoolean());
+        request.threadPool(randomBoolean());
+        request.transport(randomBoolean());
+        request.http(randomBoolean());
+        request.plugins(randomBoolean());
+        request.ingest(randomBoolean());
+        request.indices(randomBoolean());
+        NodesInfoRequest deserializedRequest = roundTripRequest(request);
+        assertRequestsEqual(request, deserializedRequest);
+    }
+
+    /**
+     * Test that a newly constructed NodesInfoRequestObject requests all of the
+     * possible metrics defined in {@link NodesInfoRequest.Metrics}.
+     */
+    public void testNodesInfoRequestDefaults() {
+        NodesInfoRequest defaultNodesInfoRequest = new NodesInfoRequest(randomAlphaOfLength(8));
+        NodesInfoRequest allMetricsNodesInfoRequest = new NodesInfoRequest(randomAlphaOfLength(8));
+        allMetricsNodesInfoRequest.all();
+
+        assertRequestsEqual(defaultNodesInfoRequest, allMetricsNodesInfoRequest);
+    }
+
+    /**
+     * Test that the {@link NodesInfoRequest#all()} method sets all of the
+     * metrics to {@code true}.
+     *
+     * TODO: Once we can check values by string, use a collection rather than
+     *   checking each and every getter in the public API
+     */
+    public void testNodesInfoRequestAll() throws Exception {
+        NodesInfoRequest request = new NodesInfoRequest("node");
+        request.all();
+
+        assertTrue(request.settings());
+        assertTrue(request.os());
+        assertTrue(request.process());
+        assertTrue(request.jvm());
+        assertTrue(request.threadPool());
+        assertTrue(request.transport());
+        assertTrue(request.http());
+        assertTrue(request.plugins());
+        assertTrue(request.ingest());
+        assertTrue(request.indices());
+    }
+
+    /**
+     * Test that the {@link NodesInfoRequest#clear()} method sets all of the
+     * metrics to {@code false}.
+     *
+     * TODO: Once we can check values by string, use a collection rather than
+     *   checking each and every getter in the public API
+     */
+    public void testNodesInfoRequestClear() throws Exception {
+        NodesInfoRequest request = new NodesInfoRequest("node");
+        request.clear();
+
+        assertFalse(request.settings());
+        assertFalse(request.os());
+        assertFalse(request.process());
+        assertFalse(request.jvm());
+        assertFalse(request.threadPool());
+        assertFalse(request.transport());
+        assertFalse(request.http());
+        assertFalse(request.plugins());
+        assertFalse(request.ingest());
+        assertFalse(request.indices());
+    }
+
+    /**
+     * Serialize and deserialize a request.
+     * @param request A request to serialize.
+     * @return The deserialized, "round-tripped" request.
+     */
+    private static NodesInfoRequest roundTripRequest(NodesInfoRequest request) throws Exception {
+        try (BytesStreamOutput out = new BytesStreamOutput()) {
+            request.writeTo(out);
+            try (StreamInput in = out.bytes().streamInput()) {
+                return new NodesInfoRequest(in);
+            }
+        }
+    }
+
+    private static void assertRequestsEqual(NodesInfoRequest request1, NodesInfoRequest request2) {
+
+        // TODO: Once we can check values by string, use a collection rather than
+        //   checking each and every getter in the public API
+        assertThat(request1.settings(), equalTo(request2.settings()));
+        assertThat(request1.os(), equalTo(request2.os()));
+        assertThat(request1.process(), equalTo(request2.process()));
+        assertThat(request1.jvm(), equalTo(request2.jvm()));
+        assertThat(request1.threadPool(), equalTo(request2.threadPool()));
+        assertThat(request1.transport(), equalTo(request2.transport()));
+        assertThat(request1.http(), equalTo(request2.http()));
+        assertThat(request1.plugins(), equalTo(request2.plugins()));
+        assertThat(request1.ingest(), equalTo(request2.ingest()));
+        assertThat(request1.indices(), equalTo(request2.indices()));
+    }
+}