Refactor json parsing in immutable handlers (#88492)

grcevski · web-flow · commit 001fef1d348f · 2022-07-13T10:04:24.000-04:00
diff --git a/server/src/main/java/org/elasticsearch/common/util/Maps.java b/server/src/main/java/org/elasticsearch/common/util/Maps.java
@@ -284,20 +284,6 @@ static int capacity(int expectedSize) {
         return expectedSize < 2 ? expectedSize + 1 : (int) (expectedSize / 0.75 + 1.0);
     }
 
-    /**
-     * Convenience method to convert the passed in input object as a map with String keys.
-     *
-     * @param input the input passed into the operator handler after parsing the content
-     * @return
-     */
-    @SuppressWarnings("unchecked")
-    public static Map<String, ?> asMap(Object input) {
-        if (input instanceof Map<?, ?> source) {
-            return (Map<String, Object>) source;
-        }
-        throw new IllegalStateException("Unsupported input format");
-    }
-
     /**
      * This method creates a copy of the {@code source} map using {@code copyValueFunction} to create a defensive copy of each value.
      */
diff --git a/server/src/main/java/org/elasticsearch/immutablestate/ImmutableClusterStateHandler.java b/server/src/main/java/org/elasticsearch/immutablestate/ImmutableClusterStateHandler.java
@@ -10,7 +10,9 @@
 
 import org.elasticsearch.action.ActionRequestValidationException;
 import org.elasticsearch.action.support.master.MasterNodeRequest;
+import org.elasticsearch.xcontent.XContentParser;
 
+import java.io.IOException;
 import java.util.Collection;
 import java.util.Collections;
 
@@ -35,7 +37,6 @@ public interface ImmutableClusterStateHandler<T> {
      * cluster state update and the cluster state is typically supplied as a combined content,
      * unlike the REST handlers. This name must match a desired content key name in the combined
      * cluster state update, e.g. "ilm" or "cluster_settings" (for persistent cluster settings update).
-     * </p>
      *
      * @return a String with the handler name, e.g "ilm".
      */
@@ -51,7 +52,6 @@ public interface ImmutableClusterStateHandler<T> {
      * state in one go. For that reason, we supply a wrapper class to the cluster state called
      * {@link TransformState}, which contains the current cluster state as well as any previous keys
      * set by this handler on prior invocation.
-     * </p>
      *
      * @param source The parsed information specific to this handler from the combined cluster state content
      * @param prevState The previous cluster state and keys set by this handler (if any)
@@ -70,7 +70,6 @@ public interface ImmutableClusterStateHandler<T> {
      * to any immutable handler to declare other immutable state handlers it depends on. Given dependencies exist,
      * the ImmutableClusterStateController will order those handlers such that the handlers that are dependent
      * on are processed first.
-     * </p>
      *
      * @return a collection of immutable state handler names
      */
@@ -85,7 +84,6 @@ default Collection<String> dependencies() {
      * All implementations of {@link ImmutableClusterStateHandler} should call the request validate method, by calling this default
      * implementation. To aid in any special validation logic that may need to be implemented by the immutable cluster state handler
      * we provide this convenience method.
-     * </p>
      *
      * @param request the master node request that we base this immutable state handler on
      */
@@ -95,4 +93,18 @@ default void validate(MasterNodeRequest<?> request) {
             throw new IllegalStateException("Validation error", exception);
         }
     }
+
+    /**
+     * The parse content method which is called during parsing of file based content.
+     *
+     * <p>
+     * The immutable state can be provided as XContent, which means that each handler needs
+     * to implement a method to convert an XContent to an object it can consume later in
+     * transform
+     *
+     * @param parser the XContent parser we are parsing from
+     * @return
+     * @throws IOException
+     */
+    T fromXContent(XContentParser parser) throws IOException;
 }
diff --git a/server/src/main/java/org/elasticsearch/immutablestate/action/ImmutableClusterSettingsAction.java b/server/src/main/java/org/elasticsearch/immutablestate/action/ImmutableClusterSettingsAction.java
@@ -15,22 +15,22 @@
 import org.elasticsearch.common.settings.ClusterSettings;
 import org.elasticsearch.immutablestate.ImmutableClusterStateHandler;
 import org.elasticsearch.immutablestate.TransformState;
+import org.elasticsearch.xcontent.XContentParser;
 
+import java.io.IOException;
 import java.util.HashMap;
 import java.util.HashSet;
 import java.util.Map;
 import java.util.Set;
 import java.util.stream.Collectors;
 
-import static org.elasticsearch.common.util.Maps.asMap;
-
 /**
  * This Action is the immutable state save version of RestClusterUpdateSettingsAction
  * <p>
  * It is used by the ImmutableClusterStateController to update the persistent cluster settings.
  * Since transient cluster settings are deprecated, this action doesn't support updating transient cluster settings.
  */
-public class ImmutableClusterSettingsAction implements ImmutableClusterStateHandler<ClusterUpdateSettingsRequest> {
+public class ImmutableClusterSettingsAction implements ImmutableClusterStateHandler<Map<String, Object>> {
 
     public static final String NAME = "cluster_settings";
 
@@ -49,11 +49,12 @@ public String name() {
     private ClusterUpdateSettingsRequest prepare(Object input, Set<String> previouslySet) {
         final ClusterUpdateSettingsRequest clusterUpdateSettingsRequest = Requests.clusterUpdateSettingsRequest();
 
-        Map<String, ?> source = asMap(input);
         Map<String, Object> persistentSettings = new HashMap<>();
         Set<String> toDelete = new HashSet<>(previouslySet);
 
-        source.forEach((k, v) -> {
+        Map<String, Object> settings = (Map<String, Object>) input;
+
+        settings.forEach((k, v) -> {
             persistentSettings.put(k, v);
             toDelete.remove(k);
         });
@@ -87,4 +88,9 @@ public TransformState transform(Object input, TransformState prevState) {
 
         return new TransformState(state, currentKeys);
     }
+
+    @Override
+    public Map<String, Object> fromXContent(XContentParser parser) throws IOException {
+        return parser.map();
+    }
 }
diff --git a/server/src/test/java/org/elasticsearch/action/admin/cluster/settings/ClusterUpdateSettingsRequestTests.java b/server/src/test/java/org/elasticsearch/action/admin/cluster/settings/ClusterUpdateSettingsRequestTests.java
@@ -8,9 +8,17 @@
 
 package org.elasticsearch.action.admin.cluster.settings;
 
+import org.elasticsearch.action.support.ActionFilters;
+import org.elasticsearch.cluster.metadata.IndexNameExpressionResolver;
+import org.elasticsearch.cluster.service.ClusterService;
 import org.elasticsearch.common.bytes.BytesReference;
+import org.elasticsearch.common.settings.ClusterSettings;
+import org.elasticsearch.common.settings.Settings;
+import org.elasticsearch.immutablestate.action.ImmutableClusterSettingsAction;
 import org.elasticsearch.test.ESTestCase;
 import org.elasticsearch.test.XContentTestUtils;
+import org.elasticsearch.threadpool.ThreadPool;
+import org.elasticsearch.transport.TransportService;
 import org.elasticsearch.xcontent.ToXContent;
 import org.elasticsearch.xcontent.XContentParseException;
 import org.elasticsearch.xcontent.XContentParser;
@@ -21,6 +29,8 @@
 
 import static org.hamcrest.CoreMatchers.containsString;
 import static org.hamcrest.CoreMatchers.equalTo;
+import static org.hamcrest.Matchers.containsInAnyOrder;
+import static org.mockito.Mockito.mock;
 
 public class ClusterUpdateSettingsRequestTests extends ESTestCase {
 
@@ -71,4 +81,43 @@ private static ClusterUpdateSettingsRequest createTestItem() {
         request.transientSettings(ClusterUpdateSettingsResponseTests.randomClusterSettings(0, 2));
         return request;
     }
+
+    public void testOperatorHandler() throws IOException {
+        ClusterSettings clusterSettings = new ClusterSettings(Settings.EMPTY, ClusterSettings.BUILT_IN_CLUSTER_SETTINGS);
+
+        TransportClusterUpdateSettingsAction action = new TransportClusterUpdateSettingsAction(
+            mock(TransportService.class),
+            mock(ClusterService.class),
+            mock(ThreadPool.class),
+            mock(ActionFilters.class),
+            mock(IndexNameExpressionResolver.class),
+            clusterSettings
+        );
+
+        assertEquals(ImmutableClusterSettingsAction.NAME, action.immutableStateHandlerName().get());
+
+        String oneSettingJSON = """
+            {
+                "persistent": {
+                    "indices.recovery.max_bytes_per_sec": "25mb",
+                    "cluster": {
+                         "remote": {
+                             "cluster_one": {
+                                 "seeds": [
+                                     "127.0.0.1:9300"
+                                 ]
+                             }
+                         }
+                    }
+                }
+            }""";
+
+        try (XContentParser parser = createParser(XContentType.JSON.xContent(), oneSettingJSON)) {
+            ClusterUpdateSettingsRequest parsedRequest = ClusterUpdateSettingsRequest.fromXContent(parser);
+            assertThat(
+                action.modifiedKeys(parsedRequest),
+                containsInAnyOrder("indices.recovery.max_bytes_per_sec", "cluster.remote.cluster_one.seeds")
+            );
+        }
+    }
 }
diff --git a/server/src/test/java/org/elasticsearch/immutablestate/ImmutableClusterStateHandlerTests.java b/server/src/test/java/org/elasticsearch/immutablestate/ImmutableClusterStateHandlerTests.java
@@ -10,18 +10,11 @@
 
 import org.elasticsearch.action.ActionRequestValidationException;
 import org.elasticsearch.action.support.master.MasterNodeRequest;
-import org.elasticsearch.common.util.Maps;
-import org.elasticsearch.common.xcontent.XContentHelper;
 import org.elasticsearch.indices.settings.InternalOrPrivateSettingsPlugin;
 import org.elasticsearch.test.ESTestCase;
 import org.elasticsearch.xcontent.XContentParser;
-import org.elasticsearch.xcontent.XContentParserConfiguration;
-import org.elasticsearch.xcontent.XContentType;
 
 import java.io.IOException;
-import java.util.Map;
-
-import static org.hamcrest.Matchers.containsInAnyOrder;
 
 public class ImmutableClusterStateHandlerTests extends ESTestCase {
     public void testValidation() {
@@ -35,6 +28,11 @@ public String name() {
             public TransformState transform(Object source, TransformState prevState) throws Exception {
                 return prevState;
             }
+
+            @Override
+            public ValidRequest fromXContent(XContentParser parser) throws IOException {
+                return new ValidRequest();
+            }
         };
 
         handler.validate(new ValidRequest());
@@ -44,53 +42,6 @@ public TransformState transform(Object source, TransformState prevState) throws
         );
     }
 
-    public void testAsMapAndFromMap() throws IOException {
-        String someJSON = """
-            {
-                "persistent": {
-                    "indices.recovery.max_bytes_per_sec": "25mb",
-                    "cluster": {
-                         "remote": {
-                             "cluster_one": {
-                                 "seeds": [
-                                     "127.0.0.1:9300"
-                                 ]
-                             }
-                         }
-                    }
-                }
-            }""";
-
-        ImmutableClusterStateHandler<ValidRequest> persistentHandler = new ImmutableClusterStateHandler<>() {
-            @Override
-            public String name() {
-                return "persistent";
-            }
-
-            @Override
-            public TransformState transform(Object source, TransformState prevState) throws Exception {
-                return prevState;
-            }
-        };
-
-        try (XContentParser parser = XContentType.JSON.xContent().createParser(XContentParserConfiguration.EMPTY, someJSON)) {
-            Map<String, Object> originalMap = parser.map();
-
-            Map<String, ?> internalHandlerMap = Maps.asMap(originalMap.get(persistentHandler.name()));
-            assertThat(internalHandlerMap.keySet(), containsInAnyOrder("indices.recovery.max_bytes_per_sec", "cluster"));
-            assertEquals(
-                "Unsupported input format",
-                expectThrows(IllegalStateException.class, () -> Maps.asMap(Integer.valueOf(123))).getMessage()
-            );
-
-            try (XContentParser newParser = XContentHelper.mapToXContentParser(XContentParserConfiguration.EMPTY, originalMap)) {
-                Map<String, Object> newMap = newParser.map();
-
-                assertThat(newMap.keySet(), containsInAnyOrder("persistent"));
-            }
-        }
-    }
-
     static class ValidRequest extends MasterNodeRequest<InternalOrPrivateSettingsPlugin.UpdateInternalOrPrivateAction.Request> {
         @Override
         public ActionRequestValidationException validate() {
diff --git a/x-pack/plugin/ilm/src/main/java/org/elasticsearch/xpack/ilm/action/ImmutableLifecycleAction.java b/x-pack/plugin/ilm/src/main/java/org/elasticsearch/xpack/ilm/action/ImmutableLifecycleAction.java
@@ -28,7 +28,6 @@
 import java.util.Set;
 import java.util.stream.Collectors;
 
-import static org.elasticsearch.common.util.Maps.asMap;
 import static org.elasticsearch.common.xcontent.XContentHelper.mapToXContentParser;
 
 /**
@@ -38,7 +37,7 @@
  * Internally it uses {@link TransportPutLifecycleAction} and
  * {@link TransportDeleteLifecycleAction} to add, update and delete ILM policies.
  */
-public class ImmutableLifecycleAction implements ImmutableClusterStateHandler<LifecyclePolicy> {
+public class ImmutableLifecycleAction implements ImmutableClusterStateHandler<List<LifecyclePolicy>> {
 
     private final NamedXContentRegistry xContentRegistry;
     private final Client client;
@@ -60,18 +59,12 @@ public String name() {
     @SuppressWarnings("unchecked")
     public Collection<PutLifecycleAction.Request> prepare(Object input) throws IOException {
         List<PutLifecycleAction.Request> result = new ArrayList<>();
+        List<LifecyclePolicy> policies = (List<LifecyclePolicy>) input;
 
-        Map<String, ?> source = asMap(input);
-
-        for (String name : source.keySet()) {
-            Map<String, ?> content = (Map<String, ?>) source.get(name);
-            var config = XContentParserConfiguration.EMPTY.withRegistry(LifecyclePolicyConfig.DEFAULT_X_CONTENT_REGISTRY);
-            try (XContentParser parser = mapToXContentParser(config, content)) {
-                LifecyclePolicy policy = LifecyclePolicy.parse(parser, name);
-                PutLifecycleAction.Request request = new PutLifecycleAction.Request(policy);
-                validate(request);
-                result.add(request);
-            }
+        for (var policy : policies) {
+            PutLifecycleAction.Request request = new PutLifecycleAction.Request(policy);
+            validate(request);
+            result.add(request);
         }
 
         return result;
@@ -108,4 +101,22 @@ public TransformState transform(Object source, TransformState prevState) throws
 
         return new TransformState(state, entities);
     }
+
+    @Override
+    public List<LifecyclePolicy> fromXContent(XContentParser parser) throws IOException {
+        List<LifecyclePolicy> result = new ArrayList<>();
+
+        Map<String, ?> source = parser.map();
+        var config = XContentParserConfiguration.EMPTY.withRegistry(LifecyclePolicyConfig.DEFAULT_X_CONTENT_REGISTRY);
+
+        for (String name : source.keySet()) {
+            @SuppressWarnings("unchecked")
+            Map<String, ?> content = (Map<String, ?>) source.get(name);
+            try (XContentParser policyParser = mapToXContentParser(config, content)) {
+                result.add(LifecyclePolicy.parse(policyParser, name));
+            }
+        }
+
+        return result;
+    }
 }
diff --git a/x-pack/plugin/ilm/src/main/resources/META-INF/services/org.elasticsearch.immutablestate.ImmutableClusterStateHandlerProvider b/x-pack/plugin/ilm/src/main/resources/META-INF/services/org.elasticsearch.immutablestate.ImmutableClusterStateHandlerProvider
@@ -0,0 +1,8 @@
+#
+# 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; you may not use this file except in compliance with the Elastic License
+# 2.0.
+#
+
+org.elasticsearch.xpack.ilm.ILMImmutableStateHandlerProvider
diff --git a/x-pack/plugin/ilm/src/test/java/org/elasticsearch/xpack/ilm/action/ImmutableILMStateControllerTests.java b/x-pack/plugin/ilm/src/test/java/org/elasticsearch/xpack/ilm/action/ImmutableILMStateControllerTests.java
@@ -87,7 +87,7 @@ protected NamedXContentRegistry xContentRegistry() {
 
     private TransformState processJSON(ImmutableLifecycleAction action, TransformState prevState, String json) throws Exception {
         try (XContentParser parser = XContentType.JSON.xContent().createParser(XContentParserConfiguration.EMPTY, json)) {
-            return action.transform(parser.map(), prevState);
+            return action.transform(action.fromXContent(parser), prevState);
         }
     }
 

Original file line number	Diff line number	Diff line change
`@@ -87,7 +87,7 @@ protected NamedXContentRegistry xContentRegistry() {`
`87`	`87`
`88`	`88`	`private TransformState processJSON(ImmutableLifecycleAction action, TransformState prevState, String json) throws Exception {`
`89`	`89`	`try (XContentParser parser = XContentType.JSON.xContent().createParser(XContentParserConfiguration.EMPTY, json)) {`
`90`		`- return action.transform(parser.map(), prevState);`
	`90`	`+ return action.transform(action.fromXContent(parser), prevState);`
`91`	`91`	`}`
`92`	`92`	`}`
`93`	`93`