Add level parameter validation in REST layer (#94136)

Relates #93981
elastic · Mar 21, 2023 · fc9c0ce · fc9c0ce
1 parent ab1d891
commit fc9c0ce
Show file tree

Hide file tree

Showing 21 changed files with 303 additions and 72 deletions.
diff --git a/docs/changelog/94136.yaml b/docs/changelog/94136.yaml
@@ -0,0 +1,5 @@
+pr: 94136
+summary: Add level parameter validation in REST layer 
+area: Infra/REST API
+type: bug
+issues: [93981]
diff --git a/server/src/main/java/org/elasticsearch/action/ClusterStatsLevel.java b/server/src/main/java/org/elasticsearch/action/ClusterStatsLevel.java
@@ -0,0 +1,40 @@
+/*
+ * 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;
+
+import org.elasticsearch.xcontent.ToXContent;
+
+public enum ClusterStatsLevel {
+    CLUSTER("cluster"),
+    INDICES("indices"),
+    SHARDS("shards");
+
+    private final String level;
+
+    ClusterStatsLevel(String level) {
+        this.level = level;
+    }
+
+    public String getLevel() {
+        return level;
+    }
+
+    public static ClusterStatsLevel of(String level) {
+        for (ClusterStatsLevel value : values()) {
+            if (value.getLevel().equalsIgnoreCase(level)) {
+                return value;
+            }
+        }
+        throw new IllegalArgumentException("level parameter must be one of [cluster] or [indices] or [shards] but was [" + level + "]");
+    }
+
+    public static ClusterStatsLevel of(ToXContent.Params params, ClusterStatsLevel defaultLevel) {
+        return ClusterStatsLevel.of(params.param("level", defaultLevel.getLevel()));
+    }
+}
diff --git a/server/src/main/java/org/elasticsearch/action/NodeStatsLevel.java b/server/src/main/java/org/elasticsearch/action/NodeStatsLevel.java
@@ -0,0 +1,40 @@
+/*
+ * 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;
+
+import org.elasticsearch.xcontent.ToXContent;
+
+public enum NodeStatsLevel {
+    NODE("node"),
+    INDICES("indices"),
+    SHARDS("shards");
+
+    private final String level;
+
+    NodeStatsLevel(String level) {
+        this.level = level;
+    }
+
+    public String getLevel() {
+        return level;
+    }
+
+    public static NodeStatsLevel of(String level) {
+        for (NodeStatsLevel value : values()) {
+            if (value.getLevel().equalsIgnoreCase(level)) {
+                return value;
+            }
+        }
+        throw new IllegalArgumentException("level parameter must be one of [node] or [indices] or [shards] but was [" + level + "]");
+    }
+
+    public static NodeStatsLevel of(ToXContent.Params params, NodeStatsLevel defaultLevel) {
+        return NodeStatsLevel.of(params.param("level", defaultLevel.getLevel()));
+    }
+}
diff --git a/server/src/main/java/org/elasticsearch/action/admin/cluster/health/ClusterHealthRequest.java b/server/src/main/java/org/elasticsearch/action/admin/cluster/health/ClusterHealthRequest.java
@@ -20,7 +20,6 @@
 import org.elasticsearch.core.TimeValue;
 
 import java.io.IOException;
-import java.util.Objects;
 import java.util.concurrent.TimeUnit;
 
 public class ClusterHealthRequest extends MasterNodeReadRequest<ClusterHealthRequest> implements IndicesRequest.Replaceable {
@@ -34,11 +33,6 @@ public class ClusterHealthRequest extends MasterNodeReadRequest<ClusterHealthReq
     private ActiveShardCount waitForActiveShards = ActiveShardCount.NONE;
     private String waitForNodes = "";
     private Priority waitForEvents = null;
-    /**
-     * Only used by the high-level REST Client. Controls the details level of the health information returned.
-     * The default value is 'cluster'.
-     */
-    private Level level = Level.CLUSTER;
 
     public ClusterHealthRequest() {}
 
@@ -232,30 +226,8 @@ public Priority waitForEvents() {
         return this.waitForEvents;
     }
 
-    /**
-     * Set the level of detail for the health information to be returned.
-     * Only used by the high-level REST Client.
-     */
-    public void level(Level level) {
-        this.level = Objects.requireNonNull(level, "level must not be null");
-    }
-
-    /**
-     * Get the level of detail for the health information to be returned.
-     * Only used by the high-level REST Client.
-     */
-    public Level level() {
-        return level;
-    }
-
     @Override
     public ActionRequestValidationException validate() {
         return null;
     }
-
-    public enum Level {
-        CLUSTER,
-        INDICES,
-        SHARDS
-    }
 }
diff --git a/...er/src/main/java/org/elasticsearch/action/admin/cluster/health/ClusterHealthResponse.java b/...er/src/main/java/org/elasticsearch/action/admin/cluster/health/ClusterHealthResponse.java
@@ -9,6 +9,7 @@
 package org.elasticsearch.action.admin.cluster.health;
 
 import org.elasticsearch.action.ActionResponse;
+import org.elasticsearch.action.ClusterStatsLevel;
 import org.elasticsearch.cluster.ClusterState;
 import org.elasticsearch.cluster.health.ClusterHealthStatus;
 import org.elasticsearch.cluster.health.ClusterIndexHealth;
@@ -356,8 +357,8 @@ public XContentBuilder toXContent(XContentBuilder builder, Params params) throws
         builder.humanReadableField(TASK_MAX_WAIT_TIME_IN_QUEUE_IN_MILLIS, TASK_MAX_WAIT_TIME_IN_QUEUE, getTaskMaxWaitingTime());
         builder.percentageField(ACTIVE_SHARDS_PERCENT_AS_NUMBER, ACTIVE_SHARDS_PERCENT, getActiveShardsPercent());
 
-        String level = params.param("level", "cluster");
-        boolean outputIndices = "indices".equals(level) || "shards".equals(level);
+        ClusterStatsLevel level = ClusterStatsLevel.of(params, ClusterStatsLevel.CLUSTER);
+        boolean outputIndices = level == ClusterStatsLevel.INDICES || level == ClusterStatsLevel.SHARDS;
 
         if (outputIndices) {
             builder.startObject(INDICES);

diff --git a/...er/src/main/java/org/elasticsearch/action/admin/cluster/node/stats/NodesStatsRequest.java b/...er/src/main/java/org/elasticsearch/action/admin/cluster/node/stats/NodesStatsRequest.java
@@ -178,7 +178,7 @@ public enum Metric {
         INGEST("ingest"),
         ADAPTIVE_SELECTION("adaptive_selection"),
         SCRIPT_CACHE("script_cache"),
-        INDEXING_PRESSURE("indexing_pressure"),;
+        INDEXING_PRESSURE("indexing_pressure");
 
         private String metricName;
 

diff --git a/server/src/main/java/org/elasticsearch/action/admin/indices/stats/IndicesStatsResponse.java b/server/src/main/java/org/elasticsearch/action/admin/indices/stats/IndicesStatsResponse.java
@@ -9,6 +9,7 @@
 package org.elasticsearch.action.admin.indices.stats;
 
 import org.elasticsearch.TransportVersion;
+import org.elasticsearch.action.ClusterStatsLevel;
 import org.elasticsearch.action.admin.indices.stats.IndexStats.IndexStatsBuilder;
 import org.elasticsearch.action.support.DefaultShardOperationFailedException;
 import org.elasticsearch.action.support.broadcast.ChunkedBroadcastResponse;
@@ -178,14 +179,8 @@ public void writeTo(StreamOutput out) throws IOException {
 
     @Override
     protected Iterator<ToXContent> customXContentChunks(ToXContent.Params params) {
-        final String level = params.param("level", "indices");
-        final boolean isLevelValid = "cluster".equalsIgnoreCase(level)
-            || "indices".equalsIgnoreCase(level)
-            || "shards".equalsIgnoreCase(level);
-        if (isLevelValid == false) {
-            throw new IllegalArgumentException("level parameter must be one of [cluster] or [indices] or [shards] but was [" + level + "]");
-        }
-        if ("indices".equalsIgnoreCase(level) || "shards".equalsIgnoreCase(level)) {
+        final ClusterStatsLevel level = ClusterStatsLevel.of(params, ClusterStatsLevel.INDICES);
+        if (level == ClusterStatsLevel.INDICES || level == ClusterStatsLevel.SHARDS) {
             return Iterators.concat(Iterators.single(((builder, p) -> {
                 commonStats(builder, p);
                 return builder.startObject(Fields.INDICES);
@@ -206,7 +201,7 @@ protected Iterator<ToXContent> customXContentChunks(ToXContent.Params params) {
                 indexStats.getTotal().toXContent(builder, p);
                 builder.endObject();
 
-                if ("shards".equalsIgnoreCase(level)) {
+                if (level == ClusterStatsLevel.SHARDS) {
                     builder.startObject(Fields.SHARDS);
                     for (IndexShardStats indexShardStats : indexStats) {
                         builder.startArray(Integer.toString(indexShardStats.getShardId().id()));

diff --git a/server/src/main/java/org/elasticsearch/cluster/health/ClusterIndexHealth.java b/server/src/main/java/org/elasticsearch/cluster/health/ClusterIndexHealth.java
@@ -8,6 +8,7 @@
 
 package org.elasticsearch.cluster.health;
 
+import org.elasticsearch.action.ClusterStatsLevel;
 import org.elasticsearch.cluster.metadata.IndexMetadata;
 import org.elasticsearch.cluster.routing.IndexRoutingTable;
 import org.elasticsearch.cluster.routing.IndexShardRoutingTable;
@@ -267,7 +268,8 @@ public XContentBuilder toXContent(final XContentBuilder builder, final Params pa
         builder.field(INITIALIZING_SHARDS, getInitializingShards());
         builder.field(UNASSIGNED_SHARDS, getUnassignedShards());
 
-        if ("shards".equals(params.param("level", "indices"))) {
+        ClusterStatsLevel level = ClusterStatsLevel.of(params, ClusterStatsLevel.INDICES);
+        if (level == ClusterStatsLevel.SHARDS) {
             builder.startObject(SHARDS);
             for (ClusterShardHealth shardHealth : shards.values()) {
                 shardHealth.toXContent(builder, params);

diff --git a/server/src/main/java/org/elasticsearch/indices/NodeIndicesStats.java b/server/src/main/java/org/elasticsearch/indices/NodeIndicesStats.java
@@ -9,6 +9,7 @@
 package org.elasticsearch.indices;
 
 import org.elasticsearch.TransportVersion;
+import org.elasticsearch.action.NodeStatsLevel;
 import org.elasticsearch.action.admin.indices.stats.CommonStats;
 import org.elasticsearch.action.admin.indices.stats.IndexShardStats;
 import org.elasticsearch.action.admin.indices.stats.ShardStats;
@@ -216,19 +217,13 @@ public int hashCode() {
 
     @Override
     public XContentBuilder toXContent(XContentBuilder builder, Params params) throws IOException {
-        final String level = params.param("level", "node");
-        final boolean isLevelValid = "indices".equalsIgnoreCase(level)
-            || "node".equalsIgnoreCase(level)
-            || "shards".equalsIgnoreCase(level);
-        if (isLevelValid == false) {
-            throw new IllegalArgumentException("level parameter must be one of [indices] or [node] or [shards] but was [" + level + "]");
-        }
+        final NodeStatsLevel level = NodeStatsLevel.of(params, NodeStatsLevel.NODE);
 
         // "node" level
         builder.startObject(Fields.INDICES);
         stats.toXContent(builder, params);
 
-        if ("indices".equals(level)) {
+        if (level == NodeStatsLevel.INDICES) {
             Map<Index, CommonStats> indexStats = createCommonStatsByIndex();
             builder.startObject(Fields.INDICES);
             for (Map.Entry<Index, CommonStats> entry : indexStats.entrySet()) {
@@ -237,8 +232,8 @@ public XContentBuilder toXContent(XContentBuilder builder, Params params) throws
                 builder.endObject();
             }
             builder.endObject();
-        } else if ("shards".equals(level)) {
-            builder.startObject("shards");
+        } else if (level == NodeStatsLevel.SHARDS) {
+            builder.startObject(Fields.SHARDS);
             for (Map.Entry<Index, List<IndexShardStats>> entry : statsByShard.entrySet()) {
                 builder.startArray(entry.getKey().getName());
                 for (IndexShardStats indexShardStats : entry.getValue()) {
@@ -285,5 +280,6 @@ public List<IndexShardStats> getShardStats(Index index) {
 
     static final class Fields {
         static final String INDICES = "indices";
+        static final String SHARDS = "shards";
     }
 }
diff --git a/...er/src/main/java/org/elasticsearch/rest/action/admin/cluster/RestClusterHealthAction.java b/...er/src/main/java/org/elasticsearch/rest/action/admin/cluster/RestClusterHealthAction.java
@@ -8,6 +8,7 @@
 
 package org.elasticsearch.rest.action.admin.cluster;
 
+import org.elasticsearch.action.ClusterStatsLevel;
 import org.elasticsearch.action.admin.cluster.health.ClusterHealthRequest;
 import org.elasticsearch.action.support.ActiveShardCount;
 import org.elasticsearch.action.support.IndicesOptions;
@@ -81,6 +82,8 @@ public static ClusterHealthRequest fromRequest(final RestRequest request) {
         if (request.param("wait_for_events") != null) {
             clusterHealthRequest.waitForEvents(Priority.valueOf(request.param("wait_for_events").toUpperCase(Locale.ROOT)));
         }
+        // level parameter validation
+        ClusterStatsLevel.of(request, ClusterStatsLevel.CLUSTER);
         return clusterHealthRequest;
     }
 

diff --git a/server/src/main/java/org/elasticsearch/rest/action/admin/cluster/RestNodesStatsAction.java b/server/src/main/java/org/elasticsearch/rest/action/admin/cluster/RestNodesStatsAction.java
@@ -8,6 +8,7 @@
 
 package org.elasticsearch.rest.action.admin.cluster;
 
+import org.elasticsearch.action.NodeStatsLevel;
 import org.elasticsearch.action.admin.cluster.node.stats.NodesStatsRequest;
 import org.elasticsearch.action.admin.indices.stats.CommonStatsFlags;
 import org.elasticsearch.action.admin.indices.stats.CommonStatsFlags.Flag;
@@ -86,6 +87,8 @@ public RestChannelConsumer prepareRequest(final RestRequest request, final NodeC
 
         NodesStatsRequest nodesStatsRequest = new NodesStatsRequest(nodesIds);
         nodesStatsRequest.timeout(request.param("timeout"));
+        // level parameter validation
+        NodeStatsLevel.of(request, NodeStatsLevel.NODE);
 
         if (metrics.size() == 1 && metrics.contains("_all")) {
             if (request.hasParam("index_metric")) {

diff --git a/server/src/main/java/org/elasticsearch/rest/action/admin/indices/RestIndicesStatsAction.java b/server/src/main/java/org/elasticsearch/rest/action/admin/indices/RestIndicesStatsAction.java
@@ -8,6 +8,7 @@
 
 package org.elasticsearch.rest.action.admin.indices;
 
+import org.elasticsearch.action.ClusterStatsLevel;
 import org.elasticsearch.action.admin.indices.stats.CommonStatsFlags;
 import org.elasticsearch.action.admin.indices.stats.CommonStatsFlags.Flag;
 import org.elasticsearch.action.admin.indices.stats.IndicesStatsRequest;
@@ -85,6 +86,8 @@ public RestChannelConsumer prepareRequest(final RestRequest request, final NodeC
             : "IndicesStats default indices options changed";
         indicesStatsRequest.indicesOptions(IndicesOptions.fromRequest(request, defaultIndicesOption));
         indicesStatsRequest.indices(Strings.splitStringByCommaToArray(request.param("index")));
+        // level parameter validation
+        ClusterStatsLevel.of(request, ClusterStatsLevel.INDICES);
 
         Set<String> metrics = Strings.tokenizeByCommaToSet(request.param("metric", "_all"));
         // short cut, if no metrics have been specified in URI

diff --git a/.../test/java/org/elasticsearch/action/admin/cluster/health/ClusterHealthResponsesTests.java b/.../test/java/org/elasticsearch/action/admin/cluster/health/ClusterHealthResponsesTests.java
@@ -8,6 +8,7 @@
 
 package org.elasticsearch.action.admin.cluster.health;
 
+import org.elasticsearch.action.ClusterStatsLevel;
 import org.elasticsearch.cluster.ClusterName;
 import org.elasticsearch.cluster.ClusterState;
 import org.elasticsearch.cluster.health.ClusterHealthStatus;
@@ -40,7 +41,7 @@
 import static org.hamcrest.Matchers.lessThanOrEqualTo;
 
 public class ClusterHealthResponsesTests extends AbstractXContentSerializingTestCase<ClusterHealthResponse> {
-    private final ClusterHealthRequest.Level level = randomFrom(ClusterHealthRequest.Level.values());
+    private final ClusterStatsLevel level = randomFrom(ClusterStatsLevel.values());
 
     public void testIsTimeout() {
         ClusterHealthResponse res = new ClusterHealthResponse();
@@ -109,7 +110,7 @@ protected ClusterHealthResponse doParseInstance(XContentParser parser) {
     protected ClusterHealthResponse createTestInstance() {
         int indicesSize = randomInt(20);
         Map<String, ClusterIndexHealth> indices = Maps.newMapWithExpectedSize(indicesSize);
-        if (ClusterHealthRequest.Level.INDICES.equals(level) || ClusterHealthRequest.Level.SHARDS.equals(level)) {
+        if (level == ClusterStatsLevel.INDICES || level == ClusterStatsLevel.SHARDS) {
             for (int i = 0; i < indicesSize; i++) {
                 String indexName = randomAlphaOfLengthBetween(1, 5) + i;
                 indices.put(indexName, ClusterIndexHealthTests.randomIndexHealth(indexName, level));

diff --git a/server/src/test/java/org/elasticsearch/cluster/health/ClusterIndexHealthTests.java b/server/src/test/java/org/elasticsearch/cluster/health/ClusterIndexHealthTests.java
@@ -8,7 +8,7 @@
 package org.elasticsearch.cluster.health;
 
 import org.elasticsearch.Version;
-import org.elasticsearch.action.admin.cluster.health.ClusterHealthRequest;
+import org.elasticsearch.action.ClusterStatsLevel;
 import org.elasticsearch.cluster.metadata.IndexMetadata;
 import org.elasticsearch.cluster.routing.IndexRoutingTable;
 import org.elasticsearch.cluster.routing.RoutingTableGenerator;
@@ -29,7 +29,7 @@
 import static org.hamcrest.CoreMatchers.equalTo;
 
 public class ClusterIndexHealthTests extends AbstractXContentSerializingTestCase<ClusterIndexHealth> {
-    private final ClusterHealthRequest.Level level = randomFrom(ClusterHealthRequest.Level.SHARDS, ClusterHealthRequest.Level.INDICES);
+    private final ClusterStatsLevel level = randomFrom(ClusterStatsLevel.SHARDS, ClusterStatsLevel.INDICES);
 
     public void testClusterIndexHealth() {
         RoutingTableGenerator routingTableGenerator = new RoutingTableGenerator();
@@ -73,9 +73,9 @@ protected ClusterIndexHealth createTestInstance() {
         return randomIndexHealth(randomAlphaOfLengthBetween(1, 10), level);
     }
 
-    public static ClusterIndexHealth randomIndexHealth(String indexName, ClusterHealthRequest.Level level) {
+    public static ClusterIndexHealth randomIndexHealth(String indexName, ClusterStatsLevel level) {
         Map<Integer, ClusterShardHealth> shards = new HashMap<>();
-        if (level == ClusterHealthRequest.Level.SHARDS) {
+        if (level == ClusterStatsLevel.SHARDS) {
             for (int i = 0; i < randomInt(5); i++) {
                 shards.put(i, ClusterShardHealthTests.randomShardHealth(i));
             }

diff --git a/server/src/test/java/org/elasticsearch/indices/NodeIndicesStatsTests.java b/server/src/test/java/org/elasticsearch/indices/NodeIndicesStatsTests.java
@@ -25,7 +25,7 @@ public void testInvalidLevel() {
         final IllegalArgumentException e = expectThrows(IllegalArgumentException.class, () -> stats.toXContent(null, params));
         assertThat(
             e,
-            hasToString(containsString("level parameter must be one of [indices] or [node] or [shards] but was [" + level + "]"))
+            hasToString(containsString("level parameter must be one of [node] or [indices] or [shards] but was [" + level + "]"))
         );
     }