From 08a10cb802bf0a7566827632d0a825029df2d302 Mon Sep 17 00:00:00 2001 From: Shivansh Arora Date: Tue, 7 May 2024 15:31:37 +0530 Subject: [PATCH] Address PR comments - Add java docs for fromString method in TransportAddress - Cleaned up toXContent methods in DiscoveryNode, DiscoveryNodes Signed-off-by: Shivansh Arora --- .../common/transport/TransportAddress.java | 5 + .../cluster/node/DiscoveryNode.java | 91 +++++++++++-------- .../cluster/node/DiscoveryNodes.java | 71 ++++++++------- .../cluster/node/DiscoveryNodeTests.java | 9 +- .../cluster/node/DiscoveryNodesTests.java | 36 +++++--- 5 files changed, 119 insertions(+), 93 deletions(-) diff --git a/libs/core/src/main/java/org/opensearch/core/common/transport/TransportAddress.java b/libs/core/src/main/java/org/opensearch/core/common/transport/TransportAddress.java index b7ab988ce0404..42982fefe5952 100644 --- a/libs/core/src/main/java/org/opensearch/core/common/transport/TransportAddress.java +++ b/libs/core/src/main/java/org/opensearch/core/common/transport/TransportAddress.java @@ -163,6 +163,11 @@ public XContentBuilder toXContent(XContentBuilder builder, Params params) throws return builder.value(toString()); } + /** + * Converts a string in the format [hostname/ip]:[port] into a transport address. + * @throws UnknownHostException if the hostname or ip address is invalid + * @throws IllegalArgumentException if invalid string format provided + */ public static TransportAddress fromString(String address) throws UnknownHostException { String[] addressSplit = address.split(":"); if (addressSplit.length != 2) { diff --git a/server/src/main/java/org/opensearch/cluster/node/DiscoveryNode.java b/server/src/main/java/org/opensearch/cluster/node/DiscoveryNode.java index 644e5f3de9352..04112b0efc313 100644 --- a/server/src/main/java/org/opensearch/cluster/node/DiscoveryNode.java +++ b/server/src/main/java/org/opensearch/cluster/node/DiscoveryNode.java @@ -64,6 +64,7 @@ import static org.opensearch.cluster.metadata.Metadata.CONTEXT_MODE_API; import static org.opensearch.cluster.metadata.Metadata.CONTEXT_MODE_PARAM; +import static org.opensearch.core.xcontent.XContentParserUtils.ensureExpectedToken; import static org.opensearch.node.NodeRoleSettings.NODE_ROLES_SETTING; import static org.opensearch.node.remotestore.RemoteStoreNodeAttribute.REMOTE_STORE_NODE_ATTRIBUTE_KEY_PREFIX; @@ -582,16 +583,15 @@ public XContentBuilder toXContent(XContentBuilder builder, Params params) throws return builder; } - public static DiscoveryNode fromXContent(XContentParser parser, String nodeId) throws IOException { - if (parser.currentToken() == null) { + public static DiscoveryNode fromXContent(XContentParser parser) throws IOException { + if (parser.currentToken() == null) { // fresh parser? move to the first token parser.nextToken(); } - if (parser.currentToken() == XContentParser.Token.START_OBJECT) { + if (parser.currentToken() == XContentParser.Token.START_OBJECT) { // on a start object move to next token parser.nextToken(); } - if (parser.currentToken() != XContentParser.Token.FIELD_NAME) { - throw new IllegalArgumentException("expected field name but got a " + parser.currentToken()); - } + ensureExpectedToken(XContentParser.Token.FIELD_NAME, parser.currentToken(), parser); + String nodeId = parser.currentName(); String nodeName = null; String hostName = null; String hostAddress = null; @@ -600,45 +600,56 @@ public static DiscoveryNode fromXContent(XContentParser parser, String nodeId) t Map attributes = new HashMap<>(); Set roles = new HashSet<>(); Version version = null; - String currentFieldName = parser.currentName(); - // token should be start object at this point - // XContentParser.Token token = parser.nextToken(); - // if (token != XContentParser.Token.START_OBJECT) { - // throw new IllegalArgumentException("expected object but got a " + token); - // } - XContentParser.Token token; + XContentParser.Token token = parser.nextToken(); + ensureExpectedToken(XContentParser.Token.START_OBJECT, token, parser); while ((token = parser.nextToken()) != XContentParser.Token.END_OBJECT) { - if (token == XContentParser.Token.FIELD_NAME) { - currentFieldName = parser.currentName(); - } else if (token.isValue()) { - if (KEY_NAME.equals(currentFieldName)) { - nodeName = parser.text(); - } else if (KEY_EPHEMERAL_ID.equals(currentFieldName)) { - ephemeralId = parser.text(); - } else if (KEY_TRANSPORT_ADDRESS.equals(currentFieldName)) { - transportAddress = TransportAddress.fromString(parser.text()); - } else if (KEY_HOST_NAME.equals(currentFieldName)) { - hostName = parser.text(); - } else if (KEY_HOST_ADDRESS.equals(currentFieldName)) { - hostAddress = parser.text(); - } else if (KEY_VERSION.equals(currentFieldName)) { - version = Version.fromString(parser.text()); + ensureExpectedToken(XContentParser.Token.FIELD_NAME, token, parser); + String currentFieldName = parser.currentName(); + token = parser.nextToken(); + if (token.isValue()) { + switch (currentFieldName) { + case KEY_NAME: + nodeName = parser.text(); + break; + case KEY_EPHEMERAL_ID: + ephemeralId = parser.text(); + break; + case KEY_TRANSPORT_ADDRESS: + transportAddress = TransportAddress.fromString(parser.text()); + break; + case KEY_HOST_NAME: + hostName = parser.text(); + break; + case KEY_HOST_ADDRESS: + hostAddress = parser.text(); + break; + case KEY_VERSION: + version = Version.fromString(parser.text()); + break; + default: + throw new IllegalArgumentException("Unexpected field [ " + currentFieldName + " ]"); } } else if (token == XContentParser.Token.START_OBJECT) { - if (KEY_ATTRIBUTES.equals(currentFieldName)) { - while ((token = parser.nextToken()) != XContentParser.Token.END_OBJECT) { - if (token == XContentParser.Token.FIELD_NAME) { - currentFieldName = parser.currentName(); - } else if (token.isValue()) { - attributes.put(currentFieldName, parser.text()); - } - } + assert currentFieldName.equals(KEY_ATTRIBUTES) : "expecting field with name [" + + KEY_ATTRIBUTES + + "] but found [" + + currentFieldName + + "]"; + while ((token = parser.nextToken()) != XContentParser.Token.END_OBJECT) { + ensureExpectedToken(XContentParser.Token.FIELD_NAME, token, parser); + currentFieldName = parser.currentName(); + token = parser.nextToken(); + ensureExpectedToken(XContentParser.Token.VALUE_STRING, token, parser); + attributes.put(currentFieldName, parser.text()); } } else if (token == XContentParser.Token.START_ARRAY) { - if (KEY_ROLES.equals(currentFieldName)) { - while ((token = parser.nextToken()) != XContentParser.Token.END_ARRAY) { - roles.add(getRoleFromRoleName(parser.text())); - } + assert currentFieldName.equals(KEY_ROLES) : "expecting field with name [" + + KEY_ROLES + + "] but found [" + + currentFieldName + + "]"; + while ((token = parser.nextToken()) != XContentParser.Token.END_ARRAY) { + roles.add(getRoleFromRoleName(parser.text())); } } else { throw new IllegalArgumentException("unexpected token " + token); diff --git a/server/src/main/java/org/opensearch/cluster/node/DiscoveryNodes.java b/server/src/main/java/org/opensearch/cluster/node/DiscoveryNodes.java index d281021624fd0..ba7975e126d1f 100644 --- a/server/src/main/java/org/opensearch/cluster/node/DiscoveryNodes.java +++ b/server/src/main/java/org/opensearch/cluster/node/DiscoveryNodes.java @@ -45,7 +45,7 @@ import org.opensearch.core.common.io.stream.StreamInput; import org.opensearch.core.common.io.stream.StreamOutput; import org.opensearch.core.common.transport.TransportAddress; -import org.opensearch.core.xcontent.ToXContentFragment; +import org.opensearch.core.xcontent.ToXContentObject; import org.opensearch.core.xcontent.XContentBuilder; import org.opensearch.core.xcontent.XContentParser; @@ -65,6 +65,7 @@ import static org.opensearch.cluster.metadata.Metadata.CONTEXT_MODE_API; import static org.opensearch.cluster.metadata.Metadata.CONTEXT_MODE_PARAM; +import static org.opensearch.core.xcontent.XContentParserUtils.ensureExpectedToken; /** * This class holds all {@link DiscoveryNode} in the cluster and provides convenience methods to @@ -73,9 +74,11 @@ * @opensearch.api */ @PublicApi(since = "1.0.0") -public class DiscoveryNodes extends AbstractDiffable implements Iterable, ToXContentFragment { +public class DiscoveryNodes extends AbstractDiffable implements Iterable, ToXContentObject { public static final DiscoveryNodes EMPTY_NODES = builder().build(); + public static final String KEY_NODES = "nodes"; + public static final String KEY_CLUSTER_MANAGER = "cluster_manager"; private final Map nodes; private final Map dataNodes; @@ -575,56 +578,54 @@ public String toString() { @Override public XContentBuilder toXContent(XContentBuilder builder, Params params) throws IOException { - builder.startObject("nodes"); + builder.startObject(); + innerToXContent(builder, params); + builder.endObject(); + return builder; + } + + public XContentBuilder innerToXContent(XContentBuilder builder, Params params) throws IOException { + builder.startObject(KEY_NODES); for (DiscoveryNode node : this) { node.toXContent(builder, params); } builder.endObject(); Metadata.XContentContext context = Metadata.XContentContext.valueOf(params.param(CONTEXT_MODE_PARAM, CONTEXT_MODE_API)); if (context == Metadata.XContentContext.GATEWAY && clusterManagerNodeId != null) { - builder.field("cluster_manager", clusterManagerNodeId); + builder.field(KEY_CLUSTER_MANAGER, clusterManagerNodeId); } return builder; } public static DiscoveryNodes fromXContent(XContentParser parser) throws IOException { Builder builder = new Builder(); - if (parser.currentToken() == null) { + if (parser.currentToken() == null) { // fresh parser? move to the first token parser.nextToken(); } - if (parser.currentToken() == XContentParser.Token.START_OBJECT) { - parser.nextToken(); - } - if (parser.currentToken() != XContentParser.Token.FIELD_NAME) { - throw new IllegalArgumentException("expected field name but got a " + parser.currentToken()); - } - XContentParser.Token token; - String currentFieldName = parser.currentName(); + XContentParser.Token token = parser.currentToken(); + ensureExpectedToken(XContentParser.Token.START_OBJECT, token, parser); while ((token = parser.nextToken()) != XContentParser.Token.END_OBJECT) { - if (token == XContentParser.Token.FIELD_NAME) { - currentFieldName = parser.currentName(); - } else if (token == XContentParser.Token.START_OBJECT) { - if ("nodes".equals(currentFieldName)) { - while ((token = parser.nextToken()) != XContentParser.Token.END_OBJECT) { - if (token == XContentParser.Token.FIELD_NAME) { - currentFieldName = parser.currentName(); - } else if (token == XContentParser.Token.START_OBJECT) { - String nodeId = currentFieldName; - DiscoveryNode node = DiscoveryNode.fromXContent(parser, nodeId); - builder.add(node); - } - } - } else { - throw new IllegalArgumentException("unexpected object field " + currentFieldName); + ensureExpectedToken(XContentParser.Token.FIELD_NAME, token, parser); + String currentFieldName = parser.currentName(); + token = parser.nextToken(); + if (token == XContentParser.Token.START_OBJECT) { + assert currentFieldName.equals(KEY_NODES) : "expecting field with name [" + + KEY_NODES + + "] but found [" + + currentFieldName + + "]"; + while ((token = parser.nextToken()) != XContentParser.Token.END_OBJECT) { + builder.add(DiscoveryNode.fromXContent(parser)); } } else if (token.isValue()) { - if ("cluster_manager".equals(currentFieldName)) { - String clusterManagerNodeId = parser.text(); - if (clusterManagerNodeId != null) { - builder.clusterManagerNodeId(clusterManagerNodeId); - } - } else { - throw new IllegalArgumentException("unexpected value field " + currentFieldName); + assert currentFieldName.equals(KEY_CLUSTER_MANAGER) : "expecting field with name [" + + KEY_CLUSTER_MANAGER + + "] but found [" + + currentFieldName + + "]"; + String clusterManagerNodeId = parser.text(); + if (clusterManagerNodeId != null) { + builder.clusterManagerNodeId(clusterManagerNodeId); } } else { throw new IllegalArgumentException("unexpected token " + token); diff --git a/server/src/test/java/org/opensearch/cluster/node/DiscoveryNodeTests.java b/server/src/test/java/org/opensearch/cluster/node/DiscoveryNodeTests.java index b3a4c7e34125e..f40925f5e653c 100644 --- a/server/src/test/java/org/opensearch/cluster/node/DiscoveryNodeTests.java +++ b/server/src/test/java/org/opensearch/cluster/node/DiscoveryNodeTests.java @@ -269,7 +269,7 @@ public void testToXContentInAPIMode() throws IOException { node.toXContent(builder, new ToXContent.MapParams(singletonMap(Metadata.CONTEXT_MODE_PARAM, CONTEXT_MODE_API))); builder.endObject(); - String expectedNodeAPUXContent = "{\n" + String expectedNodeAPIXContent = "{\n" + " \"node_1\" : {\n" + " \"name\" : \"" + node.getName() @@ -282,7 +282,7 @@ public void testToXContentInAPIMode() throws IOException { + " }\n" + "}"; - assertEquals(expectedNodeAPUXContent, builder.toString()); + assertEquals(expectedNodeAPIXContent, builder.toString()); } public void testToXContentInGatewayMode() throws IOException { @@ -296,7 +296,7 @@ public void testToXContentInGatewayMode() throws IOException { node.toXContent(builder, new ToXContent.MapParams(singletonMap(Metadata.CONTEXT_MODE_PARAM, CONTEXT_MODE_GATEWAY))); builder.endObject(); - String expectedNodeAPUXContent = "{\n" + String expectedNodeAPIXContent = "{\n" + " \"node_1\" : {\n" + " \"name\" : \"" + node.getName() @@ -320,7 +320,6 @@ public void testToXContentInGatewayMode() throws IOException { + " }\n" + "}"; - assertEquals(expectedNodeAPUXContent, builder.toString()); - + assertEquals(expectedNodeAPIXContent, builder.toString()); } } diff --git a/server/src/test/java/org/opensearch/cluster/node/DiscoveryNodesTests.java b/server/src/test/java/org/opensearch/cluster/node/DiscoveryNodesTests.java index 6a22601fefa93..5fed485ad75c9 100644 --- a/server/src/test/java/org/opensearch/cluster/node/DiscoveryNodesTests.java +++ b/server/src/test/java/org/opensearch/cluster/node/DiscoveryNodesTests.java @@ -514,6 +514,18 @@ public void testMaxMinNodeVersion() { public void testToXContentInAPIMode() throws IOException { DiscoveryNodes nodes = buildDiscoveryNodes(); + /* + * Following format is expected to be used by API and looks like following: + * "node_1" : { + * "name" : "name_1", + * "ephemeral_id" : "3Q3xRwYKScWqBgVCrWmNCQ", + * "transport_address" : "0.0.0.0:2", + * "attributes" : { + * "custom" : "PKU" + * } + * } + * + * */ String expectedNodeAPUXContent = "%1$s\"node_%2$d\" : {\n" + "%1$s \"name\" : \"name_%2$d\",\n" + "%1$s \"ephemeral_id\" : \"%3$s\",\n" @@ -521,8 +533,6 @@ public void testToXContentInAPIMode() throws IOException { + "%1$s \"attributes\" : {%5$s}\n" + "%1$s}"; - logger.info(nodes); - verifyToXContentInContextMode( CONTEXT_MODE_API, nodes, @@ -585,26 +595,26 @@ private String getExpectedXContentInGatewayMode(DiscoveryNodes nodes) { return "{\n" + " \"nodes\" : {\n" + nodes.getNodes().entrySet().stream().map(entry -> { int id = Integer.parseInt(entry.getKey().split("_")[1]); DiscoveryNode node = entry.getValue(); - String indent = " "; + String offset = " "; return String.format( Locale.ROOT, expectedNodeAPUXContent, - indent, + offset, id, node.getEphemeralId(), entry.getValue().getAddress().toString(), node.getAttributes().isEmpty() ? " " - : "\n" + indent + " \"custom\" : \"" + node.getAttributes().get("custom") + "\"\n " + indent, + : "\n" + offset + " \"custom\" : \"" + node.getAttributes().get("custom") + "\"\n " + offset, node.getVersion(), node.getRoles().isEmpty() ? " " : "\n" - + indent + + offset + " \"" - + node.getRoles().stream().map(DiscoveryNodeRole::roleName).collect(Collectors.joining("\",\n" + indent + " \"")) + + node.getRoles().stream().map(DiscoveryNodeRole::roleName).collect(Collectors.joining("\",\n" + offset + " \"")) + "\"\n " - + indent + + offset ); }).collect(Collectors.joining(",\n")) + "\n" @@ -617,9 +627,7 @@ private String getExpectedXContentInGatewayMode(DiscoveryNodes nodes) { public void verifyToXContentInContextMode(String context, DiscoveryNodes nodes, String expected) throws IOException { XContentBuilder builder = JsonXContent.contentBuilder().prettyPrint(); - builder.startObject(); nodes.toXContent(builder, new ToXContent.MapParams(singletonMap(Metadata.CONTEXT_MODE_PARAM, context))); - builder.endObject(); assertEquals(expected, builder.toString()); } @@ -654,11 +662,13 @@ private void doFromXContentTestWithRandomFields(boolean addRandomFields) throws () -> randomAlphaOfLengthBetween(3, 10) ) ); - IllegalArgumentException iae = expectThrows( - IllegalArgumentException.class, + AssertionError iae = expectThrows( + AssertionError.class, () -> DiscoveryNodes.fromXContent(createParser(mediaType.xContent(), mutated)) ); - assertEquals(iae.getMessage(), "unexpected value field " + unsupportedField); + assertTrue( + iae.getMessage().matches("expecting field with name \\[([a-z]+(?:_[a-z]+)*)] but found \\[" + unsupportedField + "]") + ); } else { try (XContentParser parser = createParser(mediaType.xContent(), originalBytes)) { DiscoveryNodes parsedNodes = DiscoveryNodes.fromXContent(parser);