Address PR comments

- Add java docs for fromString method in TransportAddress
- Cleaned up toXContent methods in DiscoveryNode, DiscoveryNodes

Signed-off-by: Shivansh Arora <hishiv@amazon.com>

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) {

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<String, String> attributes = new HashMap<>();
  Set<DiscoveryNodeRole> 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);

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<DiscoveryNodes> implements Iterable<DiscoveryNode>, ToXContentFragment {
+public class DiscoveryNodes extends AbstractDiffable<DiscoveryNodes> implements Iterable<DiscoveryNode>, 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<String, DiscoveryNode> nodes;
  private final Map<String, DiscoveryNode> 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);

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());
  }
 }

server/src/test/java/org/opensearch/cluster/node/DiscoveryNodesTests.java

@@ -514,15 +514,25 @@ 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"
  + "%1$s \"transport_address\" : \"%4$s\",\n"
  + "%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);