From 24cf0619a9dce235d6555034efe4bc45341499ec Mon Sep 17 00:00:00 2001 From: Tianli Feng Date: Thu, 10 Mar 2022 16:36:15 -0800 Subject: [PATCH 01/13] Add request parameter cluster_manager_timeout Signed-off-by: Tianli Feng --- .../support/master/MasterNodeRequest.java | 36 ++++++++++++++++++- .../java/org/opensearch/rest/RestRequest.java | 26 ++++++++++++++ .../rest/action/cat/RestPluginsAction.java | 3 ++ 3 files changed, 64 insertions(+), 1 deletion(-) diff --git a/server/src/main/java/org/opensearch/action/support/master/MasterNodeRequest.java b/server/src/main/java/org/opensearch/action/support/master/MasterNodeRequest.java index d5be6c48e23b8..55d5048f14d13 100644 --- a/server/src/main/java/org/opensearch/action/support/master/MasterNodeRequest.java +++ b/server/src/main/java/org/opensearch/action/support/master/MasterNodeRequest.java @@ -44,9 +44,15 @@ */ public abstract class MasterNodeRequest> extends ActionRequest { + /** + * @deprecated As of 2.0, because promoting inclusive language, replaced by {@link #DEFAULT_CLUSTER_MANAGER_NODE_TIMEOUT} + */ + @Deprecated public static final TimeValue DEFAULT_MASTER_NODE_TIMEOUT = TimeValue.timeValueSeconds(30); - protected TimeValue masterNodeTimeout = DEFAULT_MASTER_NODE_TIMEOUT; + public static final TimeValue DEFAULT_CLUSTER_MANAGER_NODE_TIMEOUT = TimeValue.timeValueSeconds(30); + + protected TimeValue masterNodeTimeout = DEFAULT_CLUSTER_MANAGER_NODE_TIMEOUT; protected MasterNodeRequest() {} @@ -63,21 +69,49 @@ public void writeTo(StreamOutput out) throws IOException { /** * A timeout value in case the master has not been discovered yet or disconnected. + * @deprecated As of 2.0, because promoting inclusive language, replaced by {@link #clusterManagerNodeTimeout} */ + @Deprecated @SuppressWarnings("unchecked") public final Request masterNodeTimeout(TimeValue timeout) { this.masterNodeTimeout = timeout; return (Request) this; } + /** + * A timeout value in case the cluster manager has not been discovered yet or disconnected. + */ + @SuppressWarnings("unchecked") + public final Request clusterManagerNodeTimeout(TimeValue timeout) { + this.masterNodeTimeout = timeout; + return (Request) this; + } + /** * A timeout value in case the master has not been discovered yet or disconnected. + * @deprecated As of 2.0, because promoting inclusive language, replaced by {@link #clusterManagerNodeTimeout} */ + @Deprecated public final Request masterNodeTimeout(String timeout) { return masterNodeTimeout(TimeValue.parseTimeValue(timeout, null, getClass().getSimpleName() + ".masterNodeTimeout")); } + /** + * A timeout value in case the cluster manager has not been discovered yet or disconnected. + */ + public final Request clusterManagerNodeTimeout(String timeout) { + return clusterManagerNodeTimeout(TimeValue.parseTimeValue(timeout, null, getClass().getSimpleName() + ".masterNodeTimeout")); + } + + /** + * @deprecated As of 2.0, because promoting inclusive language, replaced by {@link #clusterManagerNodeTimeout} + */ + @Deprecated public final TimeValue masterNodeTimeout() { return this.masterNodeTimeout; } + + public final TimeValue clusterManagerNodeTimeout() { + return this.masterNodeTimeout; + } } diff --git a/server/src/main/java/org/opensearch/rest/RestRequest.java b/server/src/main/java/org/opensearch/rest/RestRequest.java index 7d11da7e122cd..dabcea154ba1d 100644 --- a/server/src/main/java/org/opensearch/rest/RestRequest.java +++ b/server/src/main/java/org/opensearch/rest/RestRequest.java @@ -54,6 +54,7 @@ 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; @@ -578,6 +579,31 @@ public static XContentType parseContentType(List header) { 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 Keys of the request parameters. + * @deprecated The method will be removed along with the request parameter "master_timeout". + */ + @Deprecated + public void validateParamValuesAreEqual(String... keys) { + // Filter the non-null values of the parameters with the key, and get the distinct values. + Set set = new HashSet<>(); + for (String key : keys) { + String value = param(key); + if (value != null) { + set.add(value); + } + } + // If there are more than 1 distinct value of the request parameters, throw an exception. + if (set.size() > 1) { + 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) { diff --git a/server/src/main/java/org/opensearch/rest/action/cat/RestPluginsAction.java b/server/src/main/java/org/opensearch/rest/action/cat/RestPluginsAction.java index 79cac0f906c74..5058967bc55dc 100644 --- a/server/src/main/java/org/opensearch/rest/action/cat/RestPluginsAction.java +++ b/server/src/main/java/org/opensearch/rest/action/cat/RestPluginsAction.java @@ -76,6 +76,9 @@ public RestChannelConsumer doCatRequest(final RestRequest request, final NodeCli clusterStateRequest.clear().nodes(true); clusterStateRequest.local(request.paramAsBoolean("local", clusterStateRequest.local())); clusterStateRequest.masterNodeTimeout(request.paramAsTime("master_timeout", clusterStateRequest.masterNodeTimeout())); + // Add "cluster_manager_timeout" as the alternative to "master_timeout", for promoting inclusive language. + request.validateParamValuesAreEqual("master_timeout", "cluster_manager_timeout"); + clusterStateRequest.masterNodeTimeout(request.paramAsTime("cluster_manager_timeout", clusterStateRequest.masterNodeTimeout())); return channel -> client.admin().cluster().state(clusterStateRequest, new RestActionListener(channel) { @Override From 67d9e1bd9da587907c3d45da3cb36e7e6016aa9a Mon Sep 17 00:00:00 2001 From: Tianli Feng Date: Thu, 10 Mar 2022 23:03:59 -0800 Subject: [PATCH 02/13] Add deprecation notice to master_timeout Signed-off-by: Tianli Feng --- .../src/main/resources/rest-api-spec/api/cat.nodes.json | 8 ++++++++ .../org/opensearch/rest/action/cat/AbstractCatAction.java | 7 +++++++ .../org/opensearch/rest/action/cat/RestNodesAction.java | 6 ++++++ .../org/opensearch/rest/action/cat/RestPluginsAction.java | 5 +++++ 4 files changed, 26 insertions(+) diff --git a/rest-api-spec/src/main/resources/rest-api-spec/api/cat.nodes.json b/rest-api-spec/src/main/resources/rest-api-spec/api/cat.nodes.json index ba3faa92c5ec6..8edbe3c5ff634 100644 --- a/rest-api-spec/src/main/resources/rest-api-spec/api/cat.nodes.json +++ b/rest-api-spec/src/main/resources/rest-api-spec/api/cat.nodes.json @@ -50,6 +50,14 @@ } }, "master_timeout":{ + "type":"time", + "description":"Explicit operation timeout for connection to master node", + "deprecated":{ + "version":"2.0.0", + "description":"To promote inclusive language, use 'cluster_manager_timeout' instead." + } + }, + "cluster_manager_timeout":{ "type":"time", "description":"Explicit operation timeout for connection to master node" }, diff --git a/server/src/main/java/org/opensearch/rest/action/cat/AbstractCatAction.java b/server/src/main/java/org/opensearch/rest/action/cat/AbstractCatAction.java index d82a32cfc2c5d..edd556c17b596 100644 --- a/server/src/main/java/org/opensearch/rest/action/cat/AbstractCatAction.java +++ b/server/src/main/java/org/opensearch/rest/action/cat/AbstractCatAction.java @@ -58,6 +58,13 @@ public abstract class AbstractCatAction extends BaseRestHandler { protected abstract Table getTableWithHeader(RestRequest request); + /** + * @deprecated It will be removed along with the request parameter "master_timeout". + */ + @Deprecated + protected static final String DEPRECATED_MESSAGE_MASTER_TIMEOUT = "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 RestChannelConsumer prepareRequest(final RestRequest request, final NodeClient client) throws IOException { boolean helpWanted = request.paramAsBoolean("help", false); diff --git a/server/src/main/java/org/opensearch/rest/action/cat/RestNodesAction.java b/server/src/main/java/org/opensearch/rest/action/cat/RestNodesAction.java index bce9b2d6b7e9d..453f31fab643a 100644 --- a/server/src/main/java/org/opensearch/rest/action/cat/RestNodesAction.java +++ b/server/src/main/java/org/opensearch/rest/action/cat/RestNodesAction.java @@ -111,6 +111,12 @@ public RestChannelConsumer doCatRequest(final RestRequest request, final NodeCli } clusterStateRequest.local(request.paramAsBoolean("local", clusterStateRequest.local())); clusterStateRequest.masterNodeTimeout(request.paramAsTime("master_timeout", clusterStateRequest.masterNodeTimeout())); + // Add "cluster_manager_timeout" as the alternative to "master_timeout", for promoting inclusive language. + if (request.hasParam("master_timeout")) { + deprecationLogger.deprecate("cat_nodes_master_timeout_parameter", DEPRECATED_MESSAGE_MASTER_TIMEOUT); + } + request.validateParamValuesAreEqual("master_timeout", "cluster_manager_timeout"); + clusterStateRequest.masterNodeTimeout(request.paramAsTime("cluster_manager_timeout", clusterStateRequest.masterNodeTimeout())); final boolean fullId = request.paramAsBoolean("full_id", false); return channel -> client.admin().cluster().state(clusterStateRequest, new RestActionListener(channel) { @Override diff --git a/server/src/main/java/org/opensearch/rest/action/cat/RestPluginsAction.java b/server/src/main/java/org/opensearch/rest/action/cat/RestPluginsAction.java index 5058967bc55dc..971acdd7a5f88 100644 --- a/server/src/main/java/org/opensearch/rest/action/cat/RestPluginsAction.java +++ b/server/src/main/java/org/opensearch/rest/action/cat/RestPluginsAction.java @@ -42,6 +42,7 @@ import org.opensearch.cluster.node.DiscoveryNode; import org.opensearch.cluster.node.DiscoveryNodes; import org.opensearch.common.Table; +import org.opensearch.common.logging.DeprecationLogger; import org.opensearch.plugins.PluginInfo; import org.opensearch.rest.RestRequest; import org.opensearch.rest.RestResponse; @@ -54,6 +55,7 @@ import static org.opensearch.rest.RestRequest.Method.GET; public class RestPluginsAction extends AbstractCatAction { + private static final DeprecationLogger deprecationLogger = DeprecationLogger.getLogger(RestPluginsAction.class); @Override public List routes() { @@ -77,6 +79,9 @@ public RestChannelConsumer doCatRequest(final RestRequest request, final NodeCli clusterStateRequest.local(request.paramAsBoolean("local", clusterStateRequest.local())); clusterStateRequest.masterNodeTimeout(request.paramAsTime("master_timeout", clusterStateRequest.masterNodeTimeout())); // Add "cluster_manager_timeout" as the alternative to "master_timeout", for promoting inclusive language. + if (request.hasParam("master_timeout")) { + deprecationLogger.deprecate("cat_nodes_master_timeout_parameter", DEPRECATED_MESSAGE_MASTER_TIMEOUT); + } request.validateParamValuesAreEqual("master_timeout", "cluster_manager_timeout"); clusterStateRequest.masterNodeTimeout(request.paramAsTime("cluster_manager_timeout", clusterStateRequest.masterNodeTimeout())); From 8da36a95bc10a261ccf6b85b60f383f3e808a432 Mon Sep 17 00:00:00 2001 From: Tianli Feng Date: Sat, 12 Mar 2022 22:52:03 -0800 Subject: [PATCH 03/13] Revert deprecating methods in MasterNodeRequest Signed-off-by: Tianli Feng --- .../support/master/MasterNodeRequest.java | 36 +------------------ .../rest/action/cat/AbstractCatAction.java | 9 ++--- .../rest/action/cat/RestPluginsAction.java | 1 + 3 files changed, 5 insertions(+), 41 deletions(-) diff --git a/server/src/main/java/org/opensearch/action/support/master/MasterNodeRequest.java b/server/src/main/java/org/opensearch/action/support/master/MasterNodeRequest.java index 55d5048f14d13..d5be6c48e23b8 100644 --- a/server/src/main/java/org/opensearch/action/support/master/MasterNodeRequest.java +++ b/server/src/main/java/org/opensearch/action/support/master/MasterNodeRequest.java @@ -44,15 +44,9 @@ */ public abstract class MasterNodeRequest> extends ActionRequest { - /** - * @deprecated As of 2.0, because promoting inclusive language, replaced by {@link #DEFAULT_CLUSTER_MANAGER_NODE_TIMEOUT} - */ - @Deprecated public static final TimeValue DEFAULT_MASTER_NODE_TIMEOUT = TimeValue.timeValueSeconds(30); - public static final TimeValue DEFAULT_CLUSTER_MANAGER_NODE_TIMEOUT = TimeValue.timeValueSeconds(30); - - protected TimeValue masterNodeTimeout = DEFAULT_CLUSTER_MANAGER_NODE_TIMEOUT; + protected TimeValue masterNodeTimeout = DEFAULT_MASTER_NODE_TIMEOUT; protected MasterNodeRequest() {} @@ -69,49 +63,21 @@ public void writeTo(StreamOutput out) throws IOException { /** * A timeout value in case the master has not been discovered yet or disconnected. - * @deprecated As of 2.0, because promoting inclusive language, replaced by {@link #clusterManagerNodeTimeout} */ - @Deprecated @SuppressWarnings("unchecked") public final Request masterNodeTimeout(TimeValue timeout) { this.masterNodeTimeout = timeout; return (Request) this; } - /** - * A timeout value in case the cluster manager has not been discovered yet or disconnected. - */ - @SuppressWarnings("unchecked") - public final Request clusterManagerNodeTimeout(TimeValue timeout) { - this.masterNodeTimeout = timeout; - return (Request) this; - } - /** * A timeout value in case the master has not been discovered yet or disconnected. - * @deprecated As of 2.0, because promoting inclusive language, replaced by {@link #clusterManagerNodeTimeout} */ - @Deprecated public final Request masterNodeTimeout(String timeout) { return masterNodeTimeout(TimeValue.parseTimeValue(timeout, null, getClass().getSimpleName() + ".masterNodeTimeout")); } - /** - * A timeout value in case the cluster manager has not been discovered yet or disconnected. - */ - public final Request clusterManagerNodeTimeout(String timeout) { - return clusterManagerNodeTimeout(TimeValue.parseTimeValue(timeout, null, getClass().getSimpleName() + ".masterNodeTimeout")); - } - - /** - * @deprecated As of 2.0, because promoting inclusive language, replaced by {@link #clusterManagerNodeTimeout} - */ - @Deprecated public final TimeValue masterNodeTimeout() { return this.masterNodeTimeout; } - - public final TimeValue clusterManagerNodeTimeout() { - return this.masterNodeTimeout; - } } diff --git a/server/src/main/java/org/opensearch/rest/action/cat/AbstractCatAction.java b/server/src/main/java/org/opensearch/rest/action/cat/AbstractCatAction.java index edd556c17b596..1deb58aab702a 100644 --- a/server/src/main/java/org/opensearch/rest/action/cat/AbstractCatAction.java +++ b/server/src/main/java/org/opensearch/rest/action/cat/AbstractCatAction.java @@ -58,12 +58,9 @@ public abstract class AbstractCatAction extends BaseRestHandler { protected abstract Table getTableWithHeader(RestRequest request); - /** - * @deprecated It will be removed along with the request parameter "master_timeout". - */ - @Deprecated - protected static final String DEPRECATED_MESSAGE_MASTER_TIMEOUT = "Deprecated parameter [master_timeout] used. To promote inclusive language, " + - "please use [cluster_manager_timeout] instead. It will be unsupported in a future major version."; + // TODO: Remove this line after removing MASTER_ROLE. It's used for the REST CAT actions that accepts parameter 'master_timeout' + protected static final String DEPRECATED_MESSAGE_MASTER_TIMEOUT = + "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 RestChannelConsumer prepareRequest(final RestRequest request, final NodeClient client) throws IOException { diff --git a/server/src/main/java/org/opensearch/rest/action/cat/RestPluginsAction.java b/server/src/main/java/org/opensearch/rest/action/cat/RestPluginsAction.java index 971acdd7a5f88..412496ce8b23a 100644 --- a/server/src/main/java/org/opensearch/rest/action/cat/RestPluginsAction.java +++ b/server/src/main/java/org/opensearch/rest/action/cat/RestPluginsAction.java @@ -79,6 +79,7 @@ public RestChannelConsumer doCatRequest(final RestRequest request, final NodeCli clusterStateRequest.local(request.paramAsBoolean("local", clusterStateRequest.local())); clusterStateRequest.masterNodeTimeout(request.paramAsTime("master_timeout", clusterStateRequest.masterNodeTimeout())); // Add "cluster_manager_timeout" as the alternative to "master_timeout", for promoting inclusive language. + // TODO: if (request.hasParam("master_timeout")) { deprecationLogger.deprecate("cat_nodes_master_timeout_parameter", DEPRECATED_MESSAGE_MASTER_TIMEOUT); } From 3f46f4ec680e690b1528f77c30cf35db6a0201ba Mon Sep 17 00:00:00 2001 From: Tianli Feng Date: Sat, 12 Mar 2022 22:54:39 -0800 Subject: [PATCH 04/13] Add todo to remove master_timeout Signed-off-by: Tianli Feng --- .../java/org/opensearch/rest/action/cat/RestNodesAction.java | 1 + .../java/org/opensearch/rest/action/cat/RestPluginsAction.java | 2 +- 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/server/src/main/java/org/opensearch/rest/action/cat/RestNodesAction.java b/server/src/main/java/org/opensearch/rest/action/cat/RestNodesAction.java index 453f31fab643a..d137c63e7224d 100644 --- a/server/src/main/java/org/opensearch/rest/action/cat/RestNodesAction.java +++ b/server/src/main/java/org/opensearch/rest/action/cat/RestNodesAction.java @@ -110,6 +110,7 @@ public RestChannelConsumer doCatRequest(final RestRequest request, final NodeCli deprecationLogger.deprecate("cat_nodes_local_parameter", LOCAL_DEPRECATED_MESSAGE); } clusterStateRequest.local(request.paramAsBoolean("local", clusterStateRequest.local())); + // TODO: Remove below lines about 'master_timeout', after removing MASTER_ROLE. clusterStateRequest.masterNodeTimeout(request.paramAsTime("master_timeout", clusterStateRequest.masterNodeTimeout())); // Add "cluster_manager_timeout" as the alternative to "master_timeout", for promoting inclusive language. if (request.hasParam("master_timeout")) { diff --git a/server/src/main/java/org/opensearch/rest/action/cat/RestPluginsAction.java b/server/src/main/java/org/opensearch/rest/action/cat/RestPluginsAction.java index 412496ce8b23a..a378c9ffa1ffd 100644 --- a/server/src/main/java/org/opensearch/rest/action/cat/RestPluginsAction.java +++ b/server/src/main/java/org/opensearch/rest/action/cat/RestPluginsAction.java @@ -77,9 +77,9 @@ public RestChannelConsumer doCatRequest(final RestRequest request, final NodeCli final ClusterStateRequest clusterStateRequest = new ClusterStateRequest(); clusterStateRequest.clear().nodes(true); clusterStateRequest.local(request.paramAsBoolean("local", clusterStateRequest.local())); + // TODO: Remove below lines about 'master_timeout', after removing MASTER_ROLE. clusterStateRequest.masterNodeTimeout(request.paramAsTime("master_timeout", clusterStateRequest.masterNodeTimeout())); // Add "cluster_manager_timeout" as the alternative to "master_timeout", for promoting inclusive language. - // TODO: if (request.hasParam("master_timeout")) { deprecationLogger.deprecate("cat_nodes_master_timeout_parameter", DEPRECATED_MESSAGE_MASTER_TIMEOUT); } From 8e0e0744252c87e6ad86f670dc2ecc03c98f6f35 Mon Sep 17 00:00:00 2001 From: Tianli Feng Date: Mon, 14 Mar 2022 17:09:58 -0700 Subject: [PATCH 05/13] Add unit test for the temporary method ValidateParamValuesAreEqual() Signed-off-by: Tianli Feng --- .../org/opensearch/rest/RestRequestTests.java | 29 +++++++++++++++++++ 1 file changed, 29 insertions(+) diff --git a/server/src/test/java/org/opensearch/rest/RestRequestTests.java b/server/src/test/java/org/opensearch/rest/RestRequestTests.java index 7abc53e4ca610..99e6c7bf0ea70 100644 --- a/server/src/test/java/org/opensearch/rest/RestRequestTests.java +++ b/server/src/test/java/org/opensearch/rest/RestRequestTests.java @@ -47,6 +47,7 @@ import java.io.IOException; import java.util.ArrayList; +import java.util.Arrays; import java.util.Collections; import java.util.HashMap; import java.util.List; @@ -280,6 +281,34 @@ public void testRequiredContent() { 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 testValidateParamValuesAreEqual() { + FakeRestRequest request = new FakeRestRequest(); + List valueList = new ArrayList<>(Arrays.asList(null, "value1", "value2")); + String valueForKey1 = randomFrom(valueList); + String valueForKey2 = randomFrom(valueList); + request.params().put("key1", valueForKey1); + request.params().put("key2", valueForKey2); + try { + request.validateParamValuesAreEqual("key1", "key2"); + } catch (OpenSearchParseException e) { + assertEquals( + "The values of the request parameters: [key1, key2] are required to be equal, otherwise please only assign value to one of the request parameters.", + e.getMessage() + ); + assertNotEquals(valueForKey1, valueForKey2); + assertNotNull(valueForKey1); + assertNotNull(valueForKey2); + } + assertTrue( + "The 2 keys should be either equal, or having null value.", + valueForKey1 == null || valueForKey2 == null || valueForKey1.equals(valueForKey2) + ); + } + private static RestRequest contentRestRequest(String content, Map params) { Map> headers = new HashMap<>(); headers.put("Content-Type", Collections.singletonList("application/json")); From dec2597ccb88422c5f9f17024c2ac7eb414c19f6 Mon Sep 17 00:00:00 2001 From: Tianli Feng Date: Mon, 14 Mar 2022 18:08:15 -0700 Subject: [PATCH 06/13] Put codes of parsing master_timeout into a method Signed-off-by: Tianli Feng --- .../rest-api-spec/api/cat.nodes.json | 2 +- .../rest/action/cat/AbstractCatAction.java | 4 --- .../rest/action/cat/RestNodesAction.java | 31 ++++++++++++++----- .../rest/action/cat/RestPluginsAction.java | 9 ------ 4 files changed, 25 insertions(+), 21 deletions(-) diff --git a/rest-api-spec/src/main/resources/rest-api-spec/api/cat.nodes.json b/rest-api-spec/src/main/resources/rest-api-spec/api/cat.nodes.json index 8edbe3c5ff634..abb13d015b4d5 100644 --- a/rest-api-spec/src/main/resources/rest-api-spec/api/cat.nodes.json +++ b/rest-api-spec/src/main/resources/rest-api-spec/api/cat.nodes.json @@ -59,7 +59,7 @@ }, "cluster_manager_timeout":{ "type":"time", - "description":"Explicit operation timeout for connection to master node" + "description":"Explicit operation timeout for connection to cluster-manager node" }, "h":{ "type":"list", diff --git a/server/src/main/java/org/opensearch/rest/action/cat/AbstractCatAction.java b/server/src/main/java/org/opensearch/rest/action/cat/AbstractCatAction.java index 1deb58aab702a..d82a32cfc2c5d 100644 --- a/server/src/main/java/org/opensearch/rest/action/cat/AbstractCatAction.java +++ b/server/src/main/java/org/opensearch/rest/action/cat/AbstractCatAction.java @@ -58,10 +58,6 @@ public abstract class AbstractCatAction extends BaseRestHandler { protected abstract Table getTableWithHeader(RestRequest request); - // TODO: Remove this line after removing MASTER_ROLE. It's used for the REST CAT actions that accepts parameter 'master_timeout' - protected static final String DEPRECATED_MESSAGE_MASTER_TIMEOUT = - "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 RestChannelConsumer prepareRequest(final RestRequest request, final NodeClient client) throws IOException { boolean helpWanted = request.paramAsBoolean("help", false); diff --git a/server/src/main/java/org/opensearch/rest/action/cat/RestNodesAction.java b/server/src/main/java/org/opensearch/rest/action/cat/RestNodesAction.java index d137c63e7224d..07bf36e6eee80 100644 --- a/server/src/main/java/org/opensearch/rest/action/cat/RestNodesAction.java +++ b/server/src/main/java/org/opensearch/rest/action/cat/RestNodesAction.java @@ -110,14 +110,8 @@ public RestChannelConsumer doCatRequest(final RestRequest request, final NodeCli deprecationLogger.deprecate("cat_nodes_local_parameter", LOCAL_DEPRECATED_MESSAGE); } clusterStateRequest.local(request.paramAsBoolean("local", clusterStateRequest.local())); - // TODO: Remove below lines about 'master_timeout', after removing MASTER_ROLE. - clusterStateRequest.masterNodeTimeout(request.paramAsTime("master_timeout", clusterStateRequest.masterNodeTimeout())); - // Add "cluster_manager_timeout" as the alternative to "master_timeout", for promoting inclusive language. - if (request.hasParam("master_timeout")) { - deprecationLogger.deprecate("cat_nodes_master_timeout_parameter", DEPRECATED_MESSAGE_MASTER_TIMEOUT); - } - request.validateParamValuesAreEqual("master_timeout", "cluster_manager_timeout"); clusterStateRequest.masterNodeTimeout(request.paramAsTime("cluster_manager_timeout", clusterStateRequest.masterNodeTimeout())); + parseDeprecatedMasterTimeoutParam(clusterStateRequest, request); final boolean fullId = request.paramAsBoolean("full_id", false); return channel -> client.admin().cluster().state(clusterStateRequest, new RestActionListener(channel) { @Override @@ -532,4 +526,27 @@ Table buildTable( 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 parseDeprecatedMasterTimeoutParam(ClusterStateRequest clusterStateRequest, RestRequest request) { + final String deprecatedTimeoutParam = "master_timeout"; + final String clusterManagerTimeoutParam = "cluster_manager_timeout"; + final String deprecatedMessage = String.format( + Locale.US, + "Deprecated parameter [%s] used. To promote inclusive language, please use [%s] instead. It will be unsupported in a future major version.", + deprecatedTimeoutParam, + clusterManagerTimeoutParam + ); + clusterStateRequest.masterNodeTimeout(request.paramAsTime(deprecatedTimeoutParam, clusterStateRequest.masterNodeTimeout())); + if (request.hasParam(deprecatedTimeoutParam)) { + deprecationLogger.deprecate("cat_nodes_master_timeout_parameter", deprecatedMessage); + request.validateParamValuesAreEqual(deprecatedTimeoutParam, clusterManagerTimeoutParam); + } + } } diff --git a/server/src/main/java/org/opensearch/rest/action/cat/RestPluginsAction.java b/server/src/main/java/org/opensearch/rest/action/cat/RestPluginsAction.java index a378c9ffa1ffd..79cac0f906c74 100644 --- a/server/src/main/java/org/opensearch/rest/action/cat/RestPluginsAction.java +++ b/server/src/main/java/org/opensearch/rest/action/cat/RestPluginsAction.java @@ -42,7 +42,6 @@ import org.opensearch.cluster.node.DiscoveryNode; import org.opensearch.cluster.node.DiscoveryNodes; import org.opensearch.common.Table; -import org.opensearch.common.logging.DeprecationLogger; import org.opensearch.plugins.PluginInfo; import org.opensearch.rest.RestRequest; import org.opensearch.rest.RestResponse; @@ -55,7 +54,6 @@ import static org.opensearch.rest.RestRequest.Method.GET; public class RestPluginsAction extends AbstractCatAction { - private static final DeprecationLogger deprecationLogger = DeprecationLogger.getLogger(RestPluginsAction.class); @Override public List routes() { @@ -77,14 +75,7 @@ public RestChannelConsumer doCatRequest(final RestRequest request, final NodeCli final ClusterStateRequest clusterStateRequest = new ClusterStateRequest(); clusterStateRequest.clear().nodes(true); clusterStateRequest.local(request.paramAsBoolean("local", clusterStateRequest.local())); - // TODO: Remove below lines about 'master_timeout', after removing MASTER_ROLE. clusterStateRequest.masterNodeTimeout(request.paramAsTime("master_timeout", clusterStateRequest.masterNodeTimeout())); - // Add "cluster_manager_timeout" as the alternative to "master_timeout", for promoting inclusive language. - if (request.hasParam("master_timeout")) { - deprecationLogger.deprecate("cat_nodes_master_timeout_parameter", DEPRECATED_MESSAGE_MASTER_TIMEOUT); - } - request.validateParamValuesAreEqual("master_timeout", "cluster_manager_timeout"); - clusterStateRequest.masterNodeTimeout(request.paramAsTime("cluster_manager_timeout", clusterStateRequest.masterNodeTimeout())); return channel -> client.admin().cluster().state(clusterStateRequest, new RestActionListener(channel) { @Override From 7bf815c4c1fd68a775e22b60e6a6796348f41fe8 Mon Sep 17 00:00:00 2001 From: Tianli Feng Date: Mon, 14 Mar 2022 19:09:42 -0700 Subject: [PATCH 07/13] Use Strings.isNullorEmpty Signed-off-by: Tianli Feng --- .../main/java/org/opensearch/rest/RestRequest.java | 4 ++-- .../java/org/opensearch/rest/RestRequestTests.java | 11 ++++++----- 2 files changed, 8 insertions(+), 7 deletions(-) diff --git a/server/src/main/java/org/opensearch/rest/RestRequest.java b/server/src/main/java/org/opensearch/rest/RestRequest.java index dabcea154ba1d..75b0723388793 100644 --- a/server/src/main/java/org/opensearch/rest/RestRequest.java +++ b/server/src/main/java/org/opensearch/rest/RestRequest.java @@ -587,11 +587,11 @@ public static XContentType parseContentType(List header) { */ @Deprecated public void validateParamValuesAreEqual(String... keys) { - // Filter the non-null values of the parameters with the key, and get the distinct values. + // Filter the non-empty values of the parameters with the key, and get the distinct values. Set set = new HashSet<>(); for (String key : keys) { String value = param(key); - if (value != null) { + if (!Strings.isNullOrEmpty(value)) { set.add(value); } } diff --git a/server/src/test/java/org/opensearch/rest/RestRequestTests.java b/server/src/test/java/org/opensearch/rest/RestRequestTests.java index 99e6c7bf0ea70..b50049f8e7d4b 100644 --- a/server/src/test/java/org/opensearch/rest/RestRequestTests.java +++ b/server/src/test/java/org/opensearch/rest/RestRequestTests.java @@ -34,6 +34,7 @@ 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; @@ -287,7 +288,7 @@ public void testRequiredContent() { */ public void testValidateParamValuesAreEqual() { FakeRestRequest request = new FakeRestRequest(); - List valueList = new ArrayList<>(Arrays.asList(null, "value1", "value2")); + List valueList = new ArrayList<>(Arrays.asList(null, "", "value1", "value2")); String valueForKey1 = randomFrom(valueList); String valueForKey2 = randomFrom(valueList); request.params().put("key1", valueForKey1); @@ -300,12 +301,12 @@ public void testValidateParamValuesAreEqual() { e.getMessage() ); assertNotEquals(valueForKey1, valueForKey2); - assertNotNull(valueForKey1); - assertNotNull(valueForKey2); + assertFalse(Strings.isNullOrEmpty(valueForKey1)); + assertFalse(Strings.isNullOrEmpty(valueForKey2)); } assertTrue( - "The 2 keys should be either equal, or having null value.", - valueForKey1 == null || valueForKey2 == null || valueForKey1.equals(valueForKey2) + "Values of the 2 keys should be equal, or having a least 1 null value or empty String.", + Strings.isNullOrEmpty(valueForKey1) || Strings.isNullOrEmpty(valueForKey2) || valueForKey1.equals(valueForKey2) ); } From c1e429762fbadf5ed4dd54603755dee16300ed21 Mon Sep 17 00:00:00 2001 From: Tianli Feng Date: Mon, 14 Mar 2022 20:19:00 -0700 Subject: [PATCH 08/13] Add unit test for cluster timeout parameter Signed-off-by: Tianli Feng --- .../rest/action/cat/RestNodesAction.java | 14 +++++-------- .../rest/action/cat/RestNodesActionTests.java | 21 +++++++++++++++++++ 2 files changed, 26 insertions(+), 9 deletions(-) diff --git a/server/src/main/java/org/opensearch/rest/action/cat/RestNodesAction.java b/server/src/main/java/org/opensearch/rest/action/cat/RestNodesAction.java index 07bf36e6eee80..bc9e72f9638f6 100644 --- a/server/src/main/java/org/opensearch/rest/action/cat/RestNodesAction.java +++ b/server/src/main/java/org/opensearch/rest/action/cat/RestNodesAction.java @@ -86,6 +86,8 @@ 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 routes() { @@ -111,7 +113,7 @@ public RestChannelConsumer doCatRequest(final RestRequest request, final NodeCli } clusterStateRequest.local(request.paramAsBoolean("local", clusterStateRequest.local())); clusterStateRequest.masterNodeTimeout(request.paramAsTime("cluster_manager_timeout", clusterStateRequest.masterNodeTimeout())); - parseDeprecatedMasterTimeoutParam(clusterStateRequest, request); + parseDeprecatedMasterTimeoutParameter(clusterStateRequest, request); final boolean fullId = request.paramAsBoolean("full_id", false); return channel -> client.admin().cluster().state(clusterStateRequest, new RestActionListener(channel) { @Override @@ -534,18 +536,12 @@ private short calculatePercentage(long used, long max) { * @deprecated As of 2.0, because promoting inclusive language. */ @Deprecated - private void parseDeprecatedMasterTimeoutParam(ClusterStateRequest clusterStateRequest, RestRequest request) { + private void parseDeprecatedMasterTimeoutParameter(ClusterStateRequest clusterStateRequest, RestRequest request) { final String deprecatedTimeoutParam = "master_timeout"; final String clusterManagerTimeoutParam = "cluster_manager_timeout"; - final String deprecatedMessage = String.format( - Locale.US, - "Deprecated parameter [%s] used. To promote inclusive language, please use [%s] instead. It will be unsupported in a future major version.", - deprecatedTimeoutParam, - clusterManagerTimeoutParam - ); clusterStateRequest.masterNodeTimeout(request.paramAsTime(deprecatedTimeoutParam, clusterStateRequest.masterNodeTimeout())); if (request.hasParam(deprecatedTimeoutParam)) { - deprecationLogger.deprecate("cat_nodes_master_timeout_parameter", deprecatedMessage); + deprecationLogger.deprecate("cat_nodes_master_timeout_parameter", MASTER_TIMEOUT_DEPRECATED_MESSAGE); request.validateParamValuesAreEqual(deprecatedTimeoutParam, clusterManagerTimeoutParam); } } diff --git a/server/src/test/java/org/opensearch/rest/action/cat/RestNodesActionTests.java b/server/src/test/java/org/opensearch/rest/action/cat/RestNodesActionTests.java index 593ad2907797e..ec73cb97136bc 100644 --- a/server/src/test/java/org/opensearch/rest/action/cat/RestNodesActionTests.java +++ b/server/src/test/java/org/opensearch/rest/action/cat/RestNodesActionTests.java @@ -32,6 +32,7 @@ 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; @@ -51,6 +52,7 @@ 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; @@ -89,4 +91,23 @@ public void testCatNodesWithLocalDeprecationWarning() { 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"); + try { + action.doCatRequest(request, client); + } catch (OpenSearchParseException e) { + assertThat(e.getMessage(), containsString("cluster_manager_timeout")); + } + assertWarnings(RestNodesAction.MASTER_TIMEOUT_DEPRECATED_MESSAGE); + terminate(threadPool); + } } From 2d36c8fe78d6943073f8664855ef104cd41dd3fc Mon Sep 17 00:00:00 2001 From: Tianli Feng Date: Tue, 15 Mar 2022 06:23:15 +0000 Subject: [PATCH 09/13] Fix unit test by moving assert into try Signed-off-by: Tianli Feng --- .../java/org/opensearch/rest/RestRequestTests.java | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/server/src/test/java/org/opensearch/rest/RestRequestTests.java b/server/src/test/java/org/opensearch/rest/RestRequestTests.java index b50049f8e7d4b..a8a1488ea5285 100644 --- a/server/src/test/java/org/opensearch/rest/RestRequestTests.java +++ b/server/src/test/java/org/opensearch/rest/RestRequestTests.java @@ -295,6 +295,13 @@ public void testValidateParamValuesAreEqual() { request.params().put("key2", valueForKey2); try { request.validateParamValuesAreEqual("key1", "key2"); + assertTrue( + "Values of the 2 keys should be equal, or having a least 1 null value or empty String. Value of key1: " + + valueForKey1 + + ". Value of key2: " + + valueForKey2, + Strings.isNullOrEmpty(valueForKey1) || Strings.isNullOrEmpty(valueForKey2) || valueForKey1.equals(valueForKey2) + ); } catch (OpenSearchParseException e) { assertEquals( "The values of the request parameters: [key1, key2] are required to be equal, otherwise please only assign value to one of the request parameters.", @@ -304,10 +311,6 @@ public void testValidateParamValuesAreEqual() { assertFalse(Strings.isNullOrEmpty(valueForKey1)); assertFalse(Strings.isNullOrEmpty(valueForKey2)); } - assertTrue( - "Values of the 2 keys should be equal, or having a least 1 null value or empty String.", - Strings.isNullOrEmpty(valueForKey1) || Strings.isNullOrEmpty(valueForKey2) || valueForKey1.equals(valueForKey2) - ); } private static RestRequest contentRestRequest(String content, Map params) { From 02e68b59478fd9eff4e5c5c7f17560a702a8c1b5 Mon Sep 17 00:00:00 2001 From: Tianli Feng Date: Wed, 16 Mar 2022 15:30:54 -0700 Subject: [PATCH 10/13] Modify testValidateParamValuesAreEqual() and test Signed-off-by: Tianli Feng --- .../java/org/opensearch/rest/RestRequest.java | 23 +++++++------ .../org/opensearch/rest/RestRequestTests.java | 34 ++++++++++++------- 2 files changed, 33 insertions(+), 24 deletions(-) diff --git a/server/src/main/java/org/opensearch/rest/RestRequest.java b/server/src/main/java/org/opensearch/rest/RestRequest.java index 75b0723388793..e04d8faa8af39 100644 --- a/server/src/main/java/org/opensearch/rest/RestRequest.java +++ b/server/src/main/java/org/opensearch/rest/RestRequest.java @@ -582,26 +582,27 @@ public static XContentType parseContentType(List 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 Keys of the request parameters. + * @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) { - // Filter the non-empty values of the parameters with the key, and get the distinct values. - Set set = new HashSet<>(); + // 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)) { - set.add(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) + ); + } } } - // If there are more than 1 distinct value of the request parameters, throw an exception. - if (set.size() > 1) { - 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 { diff --git a/server/src/test/java/org/opensearch/rest/RestRequestTests.java b/server/src/test/java/org/opensearch/rest/RestRequestTests.java index a8a1488ea5285..eab187fe020b4 100644 --- a/server/src/test/java/org/opensearch/rest/RestRequestTests.java +++ b/server/src/test/java/org/opensearch/rest/RestRequestTests.java @@ -286,30 +286,38 @@ public void testRequiredContent() { * 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 testValidateParamValuesAreEqual() { + public void testValidateParamValuesAreEqualWhenTheyAreEqual() { FakeRestRequest request = new FakeRestRequest(); - List valueList = new ArrayList<>(Arrays.asList(null, "", "value1", "value2")); + List valueList = new ArrayList<>(Arrays.asList(null, "", "value1")); String valueForKey1 = randomFrom(valueList); - String valueForKey2 = randomFrom(valueList); + String valueForKey2 = "value1"; request.params().put("key1", valueForKey1); request.params().put("key2", valueForKey2); + request.validateParamValuesAreEqual("key1", "key2"); + assertTrue( + "Values of the 2 keys should be equal, or having a least 1 null value or empty String. Value of key1: " + + valueForKey1 + + ". Value of key2: " + + 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"); try { request.validateParamValuesAreEqual("key1", "key2"); - assertTrue( - "Values of the 2 keys should be equal, or having a least 1 null value or empty String. Value of key1: " - + valueForKey1 - + ". Value of key2: " - + valueForKey2, - Strings.isNullOrEmpty(valueForKey1) || Strings.isNullOrEmpty(valueForKey2) || valueForKey1.equals(valueForKey2) - ); } catch (OpenSearchParseException e) { assertEquals( "The values of the request parameters: [key1, key2] are required to be equal, otherwise please only assign value to one of the request parameters.", e.getMessage() ); - assertNotEquals(valueForKey1, valueForKey2); - assertFalse(Strings.isNullOrEmpty(valueForKey1)); - assertFalse(Strings.isNullOrEmpty(valueForKey2)); } } From 399f2d90568a47f42d8c65b1e306a867666a30d9 Mon Sep 17 00:00:00 2001 From: Tianli Feng Date: Wed, 16 Mar 2022 15:35:05 -0700 Subject: [PATCH 11/13] Set masterNodeTimeout when the parameter master_timeout occurs Signed-off-by: Tianli Feng --- .../java/org/opensearch/rest/action/cat/RestNodesAction.java | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/server/src/main/java/org/opensearch/rest/action/cat/RestNodesAction.java b/server/src/main/java/org/opensearch/rest/action/cat/RestNodesAction.java index bc9e72f9638f6..7899909cdebbf 100644 --- a/server/src/main/java/org/opensearch/rest/action/cat/RestNodesAction.java +++ b/server/src/main/java/org/opensearch/rest/action/cat/RestNodesAction.java @@ -538,11 +538,10 @@ private short calculatePercentage(long used, long max) { @Deprecated private void parseDeprecatedMasterTimeoutParameter(ClusterStateRequest clusterStateRequest, RestRequest request) { final String deprecatedTimeoutParam = "master_timeout"; - final String clusterManagerTimeoutParam = "cluster_manager_timeout"; - clusterStateRequest.masterNodeTimeout(request.paramAsTime(deprecatedTimeoutParam, clusterStateRequest.masterNodeTimeout())); if (request.hasParam(deprecatedTimeoutParam)) { deprecationLogger.deprecate("cat_nodes_master_timeout_parameter", MASTER_TIMEOUT_DEPRECATED_MESSAGE); - request.validateParamValuesAreEqual(deprecatedTimeoutParam, clusterManagerTimeoutParam); + request.validateParamValuesAreEqual(deprecatedTimeoutParam, "cluster_manager_timeout"); + clusterStateRequest.masterNodeTimeout(request.paramAsTime(deprecatedTimeoutParam, clusterStateRequest.masterNodeTimeout())); } } } From ae7308c369b8c6131c479e8332d5a44cfacb0a6d Mon Sep 17 00:00:00 2001 From: Tianli Feng Date: Thu, 17 Mar 2022 03:34:08 -0700 Subject: [PATCH 12/13] Use assertThrows instead of try catch Signed-off-by: Tianli Feng --- .../java/org/opensearch/rest/RestRequestTests.java | 13 ++++--------- .../rest/action/cat/RestNodesActionTests.java | 7 ++----- 2 files changed, 6 insertions(+), 14 deletions(-) diff --git a/server/src/test/java/org/opensearch/rest/RestRequestTests.java b/server/src/test/java/org/opensearch/rest/RestRequestTests.java index eab187fe020b4..cf624ed6c4c57 100644 --- a/server/src/test/java/org/opensearch/rest/RestRequestTests.java +++ b/server/src/test/java/org/opensearch/rest/RestRequestTests.java @@ -57,6 +57,7 @@ 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; @@ -295,7 +296,7 @@ public void testValidateParamValuesAreEqualWhenTheyAreEqual() { request.params().put("key2", valueForKey2); request.validateParamValuesAreEqual("key1", "key2"); assertTrue( - "Values of the 2 keys should be equal, or having a least 1 null value or empty String. Value of key1: " + "Values of the 2 keys should be equal, or having 1 null value or empty String. Value of key1: " + valueForKey1 + ". Value of key2: " + valueForKey2, @@ -311,14 +312,8 @@ public void testValidateParamValuesAreEqualWhenTheyAreNotEqual() { FakeRestRequest request = new FakeRestRequest(); request.params().put("key1", "value1"); request.params().put("key2", "value2"); - try { - request.validateParamValuesAreEqual("key1", "key2"); - } catch (OpenSearchParseException e) { - assertEquals( - "The values of the request parameters: [key1, key2] are required to be equal, otherwise please only assign value to one of the request parameters.", - e.getMessage() - ); - } + 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 params) { diff --git a/server/src/test/java/org/opensearch/rest/action/cat/RestNodesActionTests.java b/server/src/test/java/org/opensearch/rest/action/cat/RestNodesActionTests.java index ec73cb97136bc..9293d40605f42 100644 --- a/server/src/test/java/org/opensearch/rest/action/cat/RestNodesActionTests.java +++ b/server/src/test/java/org/opensearch/rest/action/cat/RestNodesActionTests.java @@ -102,11 +102,8 @@ public void testCatNodesWithClusterManagerTimeout() { FakeRestRequest request = new FakeRestRequest(); request.params().put("cluster_manager_timeout", randomFrom("1h", "2m")); request.params().put("master_timeout", "3s"); - try { - action.doCatRequest(request, client); - } catch (OpenSearchParseException e) { - assertThat(e.getMessage(), containsString("cluster_manager_timeout")); - } + 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); } From dfd260d1dbd0f981c10e6a2e1bc6a13c124e4222 Mon Sep 17 00:00:00 2001 From: Tianli Feng Date: Thu, 17 Mar 2022 15:16:33 -0700 Subject: [PATCH 13/13] put all values directly in RandomFrom in the unit test Signed-off-by: Tianli Feng --- .../org/opensearch/rest/RestRequestTests.java | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-) diff --git a/server/src/test/java/org/opensearch/rest/RestRequestTests.java b/server/src/test/java/org/opensearch/rest/RestRequestTests.java index cf624ed6c4c57..d5a915b42cf87 100644 --- a/server/src/test/java/org/opensearch/rest/RestRequestTests.java +++ b/server/src/test/java/org/opensearch/rest/RestRequestTests.java @@ -48,10 +48,10 @@ import java.io.IOException; import java.util.ArrayList; -import java.util.Arrays; 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; @@ -289,17 +289,18 @@ public void testRequiredContent() { */ public void testValidateParamValuesAreEqualWhenTheyAreEqual() { FakeRestRequest request = new FakeRestRequest(); - List valueList = new ArrayList<>(Arrays.asList(null, "", "value1")); - String valueForKey1 = randomFrom(valueList); + String valueForKey1 = randomFrom("value1", "", null); String valueForKey2 = "value1"; request.params().put("key1", valueForKey1); request.params().put("key2", valueForKey2); request.validateParamValuesAreEqual("key1", "key2"); assertTrue( - "Values of the 2 keys should be equal, or having 1 null value or empty String. Value of key1: " - + valueForKey1 - + ". Value of key2: " - + valueForKey2, + 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) ); }