Centralize codes related to 'master_timeout' deprecation for eaiser removal - in CAT Nodes API (#2670)

* Move parseDeprecatedMasterTimeoutParameter method into BaseRestHandler class to reduce duplication

Signed-off-by: Tianli Feng <ftianli@amazon.com>

* Add more comments to unit test

Signed-off-by: Tianli Feng <ftianli@amazon.com>

* Make log message key different

Signed-off-by: Tianli Feng <ftianli@amazon.com>

* Prohibit using 'master_timeout' and 'cluster_manager_timeout' parameter together

Signed-off-by: Tianli Feng <ftianli@amazon.com>

* Add separate unit tests for BaseRestHandler.parseDeprecatedMasterTimeoutParameter()

Signed-off-by: Tianli Feng <ftianli@amazon.com>

* Restore unit test for cat allocation api

Signed-off-by: Tianli Feng <ftianli@amazon.com>

* Adjust format by spotlessApply task

Signed-off-by: Tianli Feng <ftianli@amazon.com>

* Fix testBothParamsNotValid() by adding warning assertion

Signed-off-by: Tianli Feng <ftianli@amazon.com>

server/src/main/java/org/opensearch/rest/BaseRestHandler.java

@@ -36,9 +36,12 @@ import org.apache.logging.log4j.LogManager;
 import org.apache.logging.log4j.Logger;
 import org.apache.lucene.search.spell.LevenshteinDistance;
 import org.apache.lucene.util.CollectionUtil;
+import org.opensearch.OpenSearchParseException;
+import org.opensearch.action.support.master.MasterNodeRequest;
 import org.opensearch.client.node.NodeClient;
 import org.opensearch.common.CheckedConsumer;
 import org.opensearch.common.collect.Tuple;
+import org.opensearch.common.logging.DeprecationLogger;
 import org.opensearch.common.settings.Setting;
 import org.opensearch.common.settings.Setting.Property;
 import org.opensearch.plugins.ActionPlugin;
@@ -200,6 +203,34 @@ public abstract class BaseRestHandler implements RestHandler {
         return Collections.emptySet();
     }
 
+    /**
+     * Parse the deprecated request parameter 'master_timeout', and add deprecated log if the parameter is used.
+     * It also validates whether the two parameters 'master_timeout' and 'cluster_manager_timeout' are not assigned together.
+     * The method is temporarily added in 2.0 duing applying inclusive language. Remove the method along with MASTER_ROLE.
+     * @param mnr the action request
+     * @param request the REST request to handle
+     * @param logger the logger that logs deprecation notices
+     * @param logMsgKeyPrefix the key prefix of a deprecation message to avoid duplicate messages.
+     */
+    public static void parseDeprecatedMasterTimeoutParameter(
+        MasterNodeRequest mnr,
+        RestRequest request,
+        DeprecationLogger logger,
+        String logMsgKeyPrefix
+    ) {
+        final String MASTER_TIMEOUT_DEPRECATED_MESSAGE =
+            "Deprecated parameter [master_timeout] used. To promote inclusive language, please use [cluster_manager_timeout] instead. It will be unsupported in a future major version.";
+        final String DUPLICATE_PARAMETER_ERROR_MESSAGE =
+            "Please only use one of the request parameters [master_timeout, cluster_manager_timeout].";
+        if (request.hasParam("master_timeout")) {
+            logger.deprecate(logMsgKeyPrefix + "_master_timeout_parameter", MASTER_TIMEOUT_DEPRECATED_MESSAGE);
+            if (request.hasParam("cluster_manager_timeout")) {
+                throw new OpenSearchParseException(DUPLICATE_PARAMETER_ERROR_MESSAGE);
+            }
+            mnr.masterNodeTimeout(request.paramAsTime("master_timeout", mnr.masterNodeTimeout()));
+        }
+    }
+
     public static class Wrapper extends BaseRestHandler {
 
         protected final BaseRestHandler delegate;

server/src/main/java/org/opensearch/rest/RestRequest.java

@@ -54,7 +54,6 @@ import org.opensearch.http.HttpRequest;
 import java.io.IOException;
 import java.io.InputStream;
 import java.util.ArrayList;
-import java.util.Arrays;
 import java.util.Collections;
 import java.util.HashMap;
 import java.util.HashSet;
@@ -579,32 +578,6 @@ public class RestRequest implements ToXContent.Params {
         throw new IllegalArgumentException("empty Content-Type header");
     }
 
-    /**
-     * The method is only used to validate whether the values of the 2 request parameters "master_timeout" and "cluster_manager_timeout" is the same value or not.
-     * If the 2 values are not the same, throw an {@link OpenSearchParseException}.
-     * @param keys Names of the request parameters.
-     * @deprecated The method will be removed along with the request parameter "master_timeout".
-     */
-    @Deprecated
-    public void validateParamValuesAreEqual(String... keys) {
-        // Track the last seen value and ensure that every subsequent value matches it.
-        // The value to be tracked is the non-empty values of the parameters with the key.
-        String lastSeenValue = null;
-        for (String key : keys) {
-            String value = param(key);
-            if (!Strings.isNullOrEmpty(value)) {
-                if (lastSeenValue == null || value.equals(lastSeenValue)) {
-                    lastSeenValue = value;
-                } else {
-                    throw new OpenSearchParseException(
-                        "The values of the request parameters: {} are required to be equal, otherwise please only assign value to one of the request parameters.",
-                        Arrays.toString(keys)
-                    );
-                }
-            }
-        }
-    }
-
     public static class ContentTypeHeaderException extends RuntimeException {
 
         ContentTypeHeaderException(final IllegalArgumentException cause) {

server/src/main/java/org/opensearch/rest/action/cat/RestNodesAction.java

@@ -86,8 +86,6 @@ public class RestNodesAction extends AbstractCatAction {
     private static final DeprecationLogger deprecationLogger = DeprecationLogger.getLogger(RestNodesAction.class);
     static final String LOCAL_DEPRECATED_MESSAGE = "Deprecated parameter [local] used. This parameter does not cause this API to act "
         + "locally, and should not be used. It will be unsupported in version 8.0.";
-    static final String MASTER_TIMEOUT_DEPRECATED_MESSAGE =
-        "Deprecated parameter [master_timeout] used. To promote inclusive language, please use [cluster_manager_timeout] instead. It will be unsupported in a future major version.";
 
     @Override
     public List<Route> routes() {
@@ -113,7 +111,7 @@ public class RestNodesAction extends AbstractCatAction {
         }
         clusterStateRequest.local(request.paramAsBoolean("local", clusterStateRequest.local()));
         clusterStateRequest.masterNodeTimeout(request.paramAsTime("cluster_manager_timeout", clusterStateRequest.masterNodeTimeout()));
-        parseDeprecatedMasterTimeoutParameter(clusterStateRequest, request);
+        parseDeprecatedMasterTimeoutParameter(clusterStateRequest, request, deprecationLogger, getName());
         final boolean fullId = request.paramAsBoolean("full_id", false);
         return channel -> client.admin().cluster().state(clusterStateRequest, new RestActionListener<ClusterStateResponse>(channel) {
             @Override
@@ -529,20 +527,4 @@ public class RestNodesAction extends AbstractCatAction {
     private short calculatePercentage(long used, long max) {
         return max <= 0 ? 0 : (short) ((100d * used) / max);
     }
-
-    /**
-     * Parse the deprecated request parameter 'master_timeout', and add deprecated log if the parameter is used.
-     * It also validates whether the value of 'master_timeout' is the same with 'cluster_manager_timeout'.
-     * Remove the method along with MASTER_ROLE.
-     * @deprecated As of 2.0, because promoting inclusive language.
-     */
-    @Deprecated
-    private void parseDeprecatedMasterTimeoutParameter(ClusterStateRequest clusterStateRequest, RestRequest request) {
-        final String deprecatedTimeoutParam = "master_timeout";
-        if (request.hasParam(deprecatedTimeoutParam)) {
-            deprecationLogger.deprecate("cat_nodes_master_timeout_parameter", MASTER_TIMEOUT_DEPRECATED_MESSAGE);
-            request.validateParamValuesAreEqual(deprecatedTimeoutParam, "cluster_manager_timeout");
-            clusterStateRequest.masterNodeTimeout(request.paramAsTime(deprecatedTimeoutParam, clusterStateRequest.masterNodeTimeout()));
-        }
-    }
 }

server/src/test/java/org/opensearch/action/RenamedTimeoutRequestParameterTests.java

@@ -0,0 +1,113 @@
+/*
+ * SPDX-License-Identifier: Apache-2.0
+ *
+ * The OpenSearch Contributors require contributions made to
+ * this file be licensed under the Apache-2.0 license or a
+ * compatible open source license.
+ */
+
+package org.opensearch.action;
+
+import org.junit.After;
+import org.opensearch.OpenSearchParseException;
+import org.opensearch.action.support.master.MasterNodeRequest;
+import org.opensearch.client.node.NodeClient;
+import org.opensearch.common.logging.DeprecationLogger;
+import org.opensearch.common.settings.Settings;
+import org.opensearch.rest.BaseRestHandler;
+import org.opensearch.rest.action.cat.RestNodesAction;
+import org.opensearch.test.OpenSearchTestCase;
+import org.opensearch.test.rest.FakeRestRequest;
+import org.opensearch.threadpool.TestThreadPool;
+
+import static org.hamcrest.Matchers.containsString;
+
+/**
+ * As of 2.0, the request parameter 'master_timeout' in all applicable REST APIs is deprecated,
+ * and alternative parameter 'cluster_manager_timeout' is added.
+ * The tests are used to validate the behavior about the renamed request parameter.
+ * Remove the test after removing MASTER_ROLE and 'master_timeout'.
+ */
+public class RenamedTimeoutRequestParameterTests extends OpenSearchTestCase {
+    private final TestThreadPool threadPool = new TestThreadPool(RenamedTimeoutRequestParameterTests.class.getName());
+    private final NodeClient client = new NodeClient(Settings.EMPTY, threadPool);
+    private final DeprecationLogger deprecationLogger = DeprecationLogger.getLogger(RenamedTimeoutRequestParameterTests.class);
+
+    private static final String DUPLICATE_PARAMETER_ERROR_MESSAGE =
+        "Please only use one of the request parameters [master_timeout, cluster_manager_timeout].";
+    private static final String MASTER_TIMEOUT_DEPRECATED_MESSAGE =
+        "Deprecated parameter [master_timeout] used. To promote inclusive language, please use [cluster_manager_timeout] instead. It will be unsupported in a future major version.";
+
+    @After
+    public void terminateThreadPool() {
+        terminate(threadPool);
+    }
+
+    public void testNoWarningsForNewParam() {
+        BaseRestHandler.parseDeprecatedMasterTimeoutParameter(
+            getMasterNodeRequest(),
+            getRestRequestWithNewParam(),
+            deprecationLogger,
+            "test"
+        );
+    }
+
+    public void testDeprecationWarningForOldParam() {
+        BaseRestHandler.parseDeprecatedMasterTimeoutParameter(
+            getMasterNodeRequest(),
+            getRestRequestWithDeprecatedParam(),
+            deprecationLogger,
+            "test"
+        );
+        assertWarnings(MASTER_TIMEOUT_DEPRECATED_MESSAGE);
+    }
+
+    public void testBothParamsNotValid() {
+        Exception e = assertThrows(
+            OpenSearchParseException.class,
+            () -> BaseRestHandler.parseDeprecatedMasterTimeoutParameter(
+                getMasterNodeRequest(),
+                getRestRequestWithBothParams(),
+                deprecationLogger,
+                "test"
+            )
+        );
+        assertThat(e.getMessage(), containsString(DUPLICATE_PARAMETER_ERROR_MESSAGE));
+        assertWarnings(MASTER_TIMEOUT_DEPRECATED_MESSAGE);
+    }
+
+    public void testCatAllocation() {
+        RestNodesAction action = new RestNodesAction();
+        Exception e = assertThrows(OpenSearchParseException.class, () -> action.doCatRequest(getRestRequestWithBothParams(), client));
+        assertThat(e.getMessage(), containsString(DUPLICATE_PARAMETER_ERROR_MESSAGE));
+        assertWarnings(MASTER_TIMEOUT_DEPRECATED_MESSAGE);
+    }
+
+    private MasterNodeRequest getMasterNodeRequest() {
+        return new MasterNodeRequest() {
+            @Override
+            public ActionRequestValidationException validate() {
+                return null;
+            }
+        };
+    }
+
+    private FakeRestRequest getRestRequestWithBothParams() {
+        FakeRestRequest request = new FakeRestRequest();
+        request.params().put("cluster_manager_timeout", "1h");
+        request.params().put("master_timeout", "3s");
+        return request;
+    }
+
+    private FakeRestRequest getRestRequestWithDeprecatedParam() {
+        FakeRestRequest request = new FakeRestRequest();
+        request.params().put("master_timeout", "3s");
+        return request;
+    }
+
+    private FakeRestRequest getRestRequestWithNewParam() {
+        FakeRestRequest request = new FakeRestRequest();
+        request.params().put("cluster_manager_timeout", "2m");
+        return request;
+    }
+}

server/src/test/java/org/opensearch/rest/RestRequestTests.java

@@ -34,7 +34,6 @@ package org.opensearch.rest;
 
 import org.opensearch.OpenSearchParseException;
 import org.opensearch.common.CheckedConsumer;
-import org.opensearch.common.Strings;
 import org.opensearch.common.bytes.BytesArray;
 import org.opensearch.common.bytes.BytesReference;
 import org.opensearch.common.collect.MapBuilder;
@@ -51,13 +50,11 @@ import java.util.ArrayList;
 import java.util.Collections;
 import java.util.HashMap;
 import java.util.List;
-import java.util.Locale;
 import java.util.Map;
 import java.util.concurrent.atomic.AtomicReference;
 
 import static java.util.Collections.emptyMap;
 import static java.util.Collections.singletonMap;
-import static org.hamcrest.CoreMatchers.containsString;
 import static org.hamcrest.Matchers.equalTo;
 import static org.hamcrest.Matchers.instanceOf;
 import static org.mockito.Mockito.mock;
@@ -283,40 +280,6 @@ public class RestRequestTests extends OpenSearchTestCase {
         assertEquals("unknown content type", e.getMessage());
     }
 
-    /*
-     * The test is added in 2.0 when the request parameter "cluster_manager_timeout" is introduced.
-     * Remove the test along with the removal of the non-inclusive terminology "master_timeout".
-     */
-    public void testValidateParamValuesAreEqualWhenTheyAreEqual() {
-        FakeRestRequest request = new FakeRestRequest();
-        String valueForKey1 = randomFrom("value1", "", null);
-        String valueForKey2 = "value1";
-        request.params().put("key1", valueForKey1);
-        request.params().put("key2", valueForKey2);
-        request.validateParamValuesAreEqual("key1", "key2");
-        assertTrue(
-            String.format(
-                Locale.ROOT,
-                "The 2 values should be equal, or having 1 null/empty value. Value of key1: %s. Value of key2: %s",
-                valueForKey1,
-                valueForKey2
-            ),
-            Strings.isNullOrEmpty(valueForKey1) || valueForKey1.equals(valueForKey2)
-        );
-    }
-
-    /*
-     * The test is added in 2.0 when the request parameter "cluster_manager_timeout" is introduced.
-     * Remove the test along with the removal of the non-inclusive terminology "master_timeout".
-     */
-    public void testValidateParamValuesAreEqualWhenTheyAreNotEqual() {
-        FakeRestRequest request = new FakeRestRequest();
-        request.params().put("key1", "value1");
-        request.params().put("key2", "value2");
-        Exception e = assertThrows(OpenSearchParseException.class, () -> request.validateParamValuesAreEqual("key1", "key2"));
-        assertThat(e.getMessage(), containsString("The values of the request parameters: [key1, key2] are required to be equal"));
-    }
-
     private static RestRequest contentRestRequest(String content, Map<String, String> params) {
         Map<String, List<String>> headers = new HashMap<>();
         headers.put("Content-Type", Collections.singletonList("application/json"));

server/src/test/java/org/opensearch/rest/action/cat/RestNodesActionTests.java

@@ -32,7 +32,6 @@
 
 package org.opensearch.rest.action.cat;
 
-import org.opensearch.OpenSearchParseException;
 import org.opensearch.Version;
 import org.opensearch.action.admin.cluster.node.info.NodesInfoResponse;
 import org.opensearch.action.admin.cluster.node.stats.NodesStatsResponse;
@@ -52,7 +51,6 @@ import java.util.Collections;
 
 import static java.util.Collections.emptyMap;
 import static java.util.Collections.emptySet;
-import static org.hamcrest.CoreMatchers.containsString;
 import static org.mockito.Mockito.mock;
 import static org.mockito.Mockito.when;
 
@@ -91,20 +89,4 @@ public class RestNodesActionTests extends OpenSearchTestCase {
 
         terminate(threadPool);
     }
-
-    /**
-     * Validate both cluster_manager_timeout and its predecessor can be parsed correctly.
-     * Remove the test along with MASTER_ROLE. It's added in version 2.0.0.
-     */
-    public void testCatNodesWithClusterManagerTimeout() {
-        TestThreadPool threadPool = new TestThreadPool(RestNodesActionTests.class.getName());
-        NodeClient client = new NodeClient(Settings.EMPTY, threadPool);
-        FakeRestRequest request = new FakeRestRequest();
-        request.params().put("cluster_manager_timeout", randomFrom("1h", "2m"));
-        request.params().put("master_timeout", "3s");
-        Exception e = assertThrows(OpenSearchParseException.class, () -> action.doCatRequest(request, client));
-        assertThat(e.getMessage(), containsString("[master_timeout, cluster_manager_timeout] are required to be equal"));
-        assertWarnings(RestNodesAction.MASTER_TIMEOUT_DEPRECATED_MESSAGE);
-        terminate(threadPool);
-    }
 }