Skip to content

Commit f151e00

Browse files
shiv0408shiv0408
committed
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>
1 parent 07367ab commit f151e00

File tree

5 files changed

+119
-93
lines changed

5 files changed

+119
-93
lines changed

‎libs/core/src/main/java/org/opensearch/core/common/transport/TransportAddress.java

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -163,6 +163,11 @@ public XContentBuilder toXContent(XContentBuilder builder, Params params) throws
163163
return builder.value(toString());
164164
}
165165

166+
/**
167+
* Converts a string in the format [hostname/ip]:[port] into a transport address.
168+
* @throws UnknownHostException if the hostname or ip address is invalid
169+
* @throws IllegalArgumentException if invalid string format provided
170+
*/
166171
public static TransportAddress fromString(String address) throws UnknownHostException {
167172
String[] addressSplit = address.split(":");
168173
if (addressSplit.length != 2) {

‎server/src/main/java/org/opensearch/cluster/node/DiscoveryNode.java

Lines changed: 51 additions & 40 deletions
Original file line numberDiff line numberDiff line change
@@ -64,6 +64,7 @@
6464

6565
import static org.opensearch.cluster.metadata.Metadata.CONTEXT_MODE_API;
6666
import static org.opensearch.cluster.metadata.Metadata.CONTEXT_MODE_PARAM;
67+
import static org.opensearch.core.xcontent.XContentParserUtils.ensureExpectedToken;
6768
import static org.opensearch.node.NodeRoleSettings.NODE_ROLES_SETTING;
6869
import static org.opensearch.node.remotestore.RemoteStoreNodeAttribute.REMOTE_STORE_NODE_ATTRIBUTE_KEY_PREFIX;
6970

@@ -582,16 +583,15 @@ public XContentBuilder toXContent(XContentBuilder builder, Params params) throws
582583
return builder;
583584
}
584585

585-
public static DiscoveryNode fromXContent(XContentParser parser, String nodeId) throws IOException {
586-
if (parser.currentToken() == null) {
586+
public static DiscoveryNode fromXContent(XContentParser parser) throws IOException {
587+
if (parser.currentToken() == null) { // fresh parser? move to the first token
587588
parser.nextToken();
588589
}
589-
if (parser.currentToken() == XContentParser.Token.START_OBJECT) {
590+
if (parser.currentToken() == XContentParser.Token.START_OBJECT) { // on a start object move to next token
590591
parser.nextToken();
591592
}
592-
if (parser.currentToken() != XContentParser.Token.FIELD_NAME) {
593-
throw new IllegalArgumentException("expected field name but got a " + parser.currentToken());
594-
}
593+
ensureExpectedToken(XContentParser.Token.FIELD_NAME, parser.currentToken(), parser);
594+
String nodeId = parser.currentName();
595595
String nodeName = null;
596596
String hostName = null;
597597
String hostAddress = null;
@@ -600,45 +600,56 @@ public static DiscoveryNode fromXContent(XContentParser parser, String nodeId) t
600600
Map<String, String> attributes = new HashMap<>();
601601
Set<DiscoveryNodeRole> roles = new HashSet<>();
602602
Version version = null;
603-
String currentFieldName = parser.currentName();
604-
// token should be start object at this point
605-
// XContentParser.Token token = parser.nextToken();
606-
// if (token != XContentParser.Token.START_OBJECT) {
607-
// throw new IllegalArgumentException("expected object but got a " + token);
608-
// }
609-
XContentParser.Token token;
603+
XContentParser.Token token = parser.nextToken();
604+
ensureExpectedToken(XContentParser.Token.START_OBJECT, token, parser);
610605
while ((token = parser.nextToken()) != XContentParser.Token.END_OBJECT) {
611-
if (token == XContentParser.Token.FIELD_NAME) {
612-
currentFieldName = parser.currentName();
613-
} else if (token.isValue()) {
614-
if (KEY_NAME.equals(currentFieldName)) {
615-
nodeName = parser.text();
616-
} else if (KEY_EPHEMERAL_ID.equals(currentFieldName)) {
617-
ephemeralId = parser.text();
618-
} else if (KEY_TRANSPORT_ADDRESS.equals(currentFieldName)) {
619-
transportAddress = TransportAddress.fromString(parser.text());
620-
} else if (KEY_HOST_NAME.equals(currentFieldName)) {
621-
hostName = parser.text();
622-
} else if (KEY_HOST_ADDRESS.equals(currentFieldName)) {
623-
hostAddress = parser.text();
624-
} else if (KEY_VERSION.equals(currentFieldName)) {
625-
version = Version.fromString(parser.text());
606+
ensureExpectedToken(XContentParser.Token.FIELD_NAME, token, parser);
607+
String currentFieldName = parser.currentName();
608+
token = parser.nextToken();
609+
if (token.isValue()) {
610+
switch (currentFieldName) {
611+
case KEY_NAME:
612+
nodeName = parser.text();
613+
break;
614+
case KEY_EPHEMERAL_ID:
615+
ephemeralId = parser.text();
616+
break;
617+
case KEY_TRANSPORT_ADDRESS:
618+
transportAddress = TransportAddress.fromString(parser.text());
619+
break;
620+
case KEY_HOST_NAME:
621+
hostName = parser.text();
622+
break;
623+
case KEY_HOST_ADDRESS:
624+
hostAddress = parser.text();
625+
break;
626+
case KEY_VERSION:
627+
version = Version.fromString(parser.text());
628+
break;
629+
default:
630+
throw new IllegalArgumentException("Unexpected field [ " + currentFieldName + " ]");
626631
}
627632
} else if (token == XContentParser.Token.START_OBJECT) {
628-
if (KEY_ATTRIBUTES.equals(currentFieldName)) {
629-
while ((token = parser.nextToken()) != XContentParser.Token.END_OBJECT) {
630-
if (token == XContentParser.Token.FIELD_NAME) {
631-
currentFieldName = parser.currentName();
632-
} else if (token.isValue()) {
633-
attributes.put(currentFieldName, parser.text());
634-
}
635-
}
633+
assert currentFieldName.equals(KEY_ATTRIBUTES) : "expecting field with name ["
634+
+ KEY_ATTRIBUTES
635+
+ "] but found ["
636+
+ currentFieldName
637+
+ "]";
638+
while ((token = parser.nextToken()) != XContentParser.Token.END_OBJECT) {
639+
ensureExpectedToken(XContentParser.Token.FIELD_NAME, token, parser);
640+
currentFieldName = parser.currentName();
641+
token = parser.nextToken();
642+
ensureExpectedToken(XContentParser.Token.VALUE_STRING, token, parser);
643+
attributes.put(currentFieldName, parser.text());
636644
}
637645
} else if (token == XContentParser.Token.START_ARRAY) {
638-
if (KEY_ROLES.equals(currentFieldName)) {
639-
while ((token = parser.nextToken()) != XContentParser.Token.END_ARRAY) {
640-
roles.add(getRoleFromRoleName(parser.text()));
641-
}
646+
assert currentFieldName.equals(KEY_ROLES) : "expecting field with name ["
647+
+ KEY_ROLES
648+
+ "] but found ["
649+
+ currentFieldName
650+
+ "]";
651+
while ((token = parser.nextToken()) != XContentParser.Token.END_ARRAY) {
652+
roles.add(getRoleFromRoleName(parser.text()));
642653
}
643654
} else {
644655
throw new IllegalArgumentException("unexpected token " + token);

‎server/src/main/java/org/opensearch/cluster/node/DiscoveryNodes.java

Lines changed: 36 additions & 35 deletions
Original file line numberDiff line numberDiff line change
@@ -45,7 +45,7 @@
4545
import org.opensearch.core.common.io.stream.StreamInput;
4646
import org.opensearch.core.common.io.stream.StreamOutput;
4747
import org.opensearch.core.common.transport.TransportAddress;
48-
import org.opensearch.core.xcontent.ToXContentFragment;
48+
import org.opensearch.core.xcontent.ToXContentObject;
4949
import org.opensearch.core.xcontent.XContentBuilder;
5050
import org.opensearch.core.xcontent.XContentParser;
5151

@@ -65,6 +65,7 @@
6565

6666
import static org.opensearch.cluster.metadata.Metadata.CONTEXT_MODE_API;
6767
import static org.opensearch.cluster.metadata.Metadata.CONTEXT_MODE_PARAM;
68+
import static org.opensearch.core.xcontent.XContentParserUtils.ensureExpectedToken;
6869

6970
/**
7071
* This class holds all {@link DiscoveryNode} in the cluster and provides convenience methods to
@@ -73,9 +74,11 @@
7374
* @opensearch.api
7475
*/
7576
@PublicApi(since = "1.0.0")
76-
public class DiscoveryNodes extends AbstractDiffable<DiscoveryNodes> implements Iterable<DiscoveryNode>, ToXContentFragment {
77+
public class DiscoveryNodes extends AbstractDiffable<DiscoveryNodes> implements Iterable<DiscoveryNode>, ToXContentObject {
7778

7879
public static final DiscoveryNodes EMPTY_NODES = builder().build();
80+
public static final String KEY_NODES = "nodes";
81+
public static final String KEY_CLUSTER_MANAGER = "cluster_manager";
7982

8083
private final Map<String, DiscoveryNode> nodes;
8184
private final Map<String, DiscoveryNode> dataNodes;
@@ -575,56 +578,54 @@ public String toString() {
575578

576579
@Override
577580
public XContentBuilder toXContent(XContentBuilder builder, Params params) throws IOException {
578-
builder.startObject("nodes");
581+
builder.startObject();
582+
innerToXContent(builder, params);
583+
builder.endObject();
584+
return builder;
585+
}
586+
587+
public XContentBuilder innerToXContent(XContentBuilder builder, Params params) throws IOException {
588+
builder.startObject(KEY_NODES);
579589
for (DiscoveryNode node : this) {
580590
node.toXContent(builder, params);
581591
}
582592
builder.endObject();
583593
Metadata.XContentContext context = Metadata.XContentContext.valueOf(params.param(CONTEXT_MODE_PARAM, CONTEXT_MODE_API));
584594
if (context == Metadata.XContentContext.GATEWAY && clusterManagerNodeId != null) {
585-
builder.field("cluster_manager", clusterManagerNodeId);
595+
builder.field(KEY_CLUSTER_MANAGER, clusterManagerNodeId);
586596
}
587597
return builder;
588598
}
589599

590600
public static DiscoveryNodes fromXContent(XContentParser parser) throws IOException {
591601
Builder builder = new Builder();
592-
if (parser.currentToken() == null) {
602+
if (parser.currentToken() == null) { // fresh parser? move to the first token
593603
parser.nextToken();
594604
}
595-
if (parser.currentToken() == XContentParser.Token.START_OBJECT) {
596-
parser.nextToken();
597-
}
598-
if (parser.currentToken() != XContentParser.Token.FIELD_NAME) {
599-
throw new IllegalArgumentException("expected field name but got a " + parser.currentToken());
600-
}
601-
XContentParser.Token token;
602-
String currentFieldName = parser.currentName();
605+
XContentParser.Token token = parser.currentToken();
606+
ensureExpectedToken(XContentParser.Token.START_OBJECT, token, parser);
603607
while ((token = parser.nextToken()) != XContentParser.Token.END_OBJECT) {
604-
if (token == XContentParser.Token.FIELD_NAME) {
605-
currentFieldName = parser.currentName();
606-
} else if (token == XContentParser.Token.START_OBJECT) {
607-
if ("nodes".equals(currentFieldName)) {
608-
while ((token = parser.nextToken()) != XContentParser.Token.END_OBJECT) {
609-
if (token == XContentParser.Token.FIELD_NAME) {
610-
currentFieldName = parser.currentName();
611-
} else if (token == XContentParser.Token.START_OBJECT) {
612-
String nodeId = currentFieldName;
613-
DiscoveryNode node = DiscoveryNode.fromXContent(parser, nodeId);
614-
builder.add(node);
615-
}
616-
}
617-
} else {
618-
throw new IllegalArgumentException("unexpected object field " + currentFieldName);
608+
ensureExpectedToken(XContentParser.Token.FIELD_NAME, token, parser);
609+
String currentFieldName = parser.currentName();
610+
token = parser.nextToken();
611+
if (token == XContentParser.Token.START_OBJECT) {
612+
assert currentFieldName.equals(KEY_NODES) : "expecting field with name ["
613+
+ KEY_NODES
614+
+ "] but found ["
615+
+ currentFieldName
616+
+ "]";
617+
while ((token = parser.nextToken()) != XContentParser.Token.END_OBJECT) {
618+
builder.add(DiscoveryNode.fromXContent(parser));
619619
}
620620
} else if (token.isValue()) {
621-
if ("cluster_manager".equals(currentFieldName)) {
622-
String clusterManagerNodeId = parser.text();
623-
if (clusterManagerNodeId != null) {
624-
builder.clusterManagerNodeId(clusterManagerNodeId);
625-
}
626-
} else {
627-
throw new IllegalArgumentException("unexpected value field " + currentFieldName);
621+
assert currentFieldName.equals(KEY_CLUSTER_MANAGER) : "expecting field with name ["
622+
+ KEY_CLUSTER_MANAGER
623+
+ "] but found ["
624+
+ currentFieldName
625+
+ "]";
626+
String clusterManagerNodeId = parser.text();
627+
if (clusterManagerNodeId != null) {
628+
builder.clusterManagerNodeId(clusterManagerNodeId);
628629
}
629630
} else {
630631
throw new IllegalArgumentException("unexpected token " + token);

‎server/src/test/java/org/opensearch/cluster/node/DiscoveryNodeTests.java

Lines changed: 4 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -269,7 +269,7 @@ public void testToXContentInAPIMode() throws IOException {
269269
node.toXContent(builder, new ToXContent.MapParams(singletonMap(Metadata.CONTEXT_MODE_PARAM, CONTEXT_MODE_API)));
270270
builder.endObject();
271271

272-
String expectedNodeAPUXContent = "{\n"
272+
String expectedNodeAPIXContent = "{\n"
273273
+ " \"node_1\" : {\n"
274274
+ " \"name\" : \""
275275
+ node.getName()
@@ -282,7 +282,7 @@ public void testToXContentInAPIMode() throws IOException {
282282
+ " }\n"
283283
+ "}";
284284

285-
assertEquals(expectedNodeAPUXContent, builder.toString());
285+
assertEquals(expectedNodeAPIXContent, builder.toString());
286286
}
287287

288288
public void testToXContentInGatewayMode() throws IOException {
@@ -296,7 +296,7 @@ public void testToXContentInGatewayMode() throws IOException {
296296
node.toXContent(builder, new ToXContent.MapParams(singletonMap(Metadata.CONTEXT_MODE_PARAM, CONTEXT_MODE_GATEWAY)));
297297
builder.endObject();
298298

299-
String expectedNodeAPUXContent = "{\n"
299+
String expectedNodeAPIXContent = "{\n"
300300
+ " \"node_1\" : {\n"
301301
+ " \"name\" : \""
302302
+ node.getName()
@@ -320,7 +320,6 @@ public void testToXContentInGatewayMode() throws IOException {
320320
+ " }\n"
321321
+ "}";
322322

323-
assertEquals(expectedNodeAPUXContent, builder.toString());
324-
323+
assertEquals(expectedNodeAPIXContent, builder.toString());
325324
}
326325
}

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

Lines changed: 23 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -514,15 +514,25 @@ public void testMaxMinNodeVersion() {
514514
public void testToXContentInAPIMode() throws IOException {
515515
DiscoveryNodes nodes = buildDiscoveryNodes();
516516

517+
/*
518+
* Following format is expected to be used by API and looks like following:
519+
* "node_1" : {
520+
* "name" : "name_1",
521+
* "ephemeral_id" : "3Q3xRwYKScWqBgVCrWmNCQ",
522+
* "transport_address" : "0.0.0.0:2",
523+
* "attributes" : {
524+
* "custom" : "PKU"
525+
* }
526+
* }
527+
*
528+
* */
517529
String expectedNodeAPUXContent = "%1$s\"node_%2$d\" : {\n"
518530
+ "%1$s \"name\" : \"name_%2$d\",\n"
519531
+ "%1$s \"ephemeral_id\" : \"%3$s\",\n"
520532
+ "%1$s \"transport_address\" : \"%4$s\",\n"
521533
+ "%1$s \"attributes\" : {%5$s}\n"
522534
+ "%1$s}";
523535

524-
logger.info(nodes);
525-
526536
verifyToXContentInContextMode(
527537
CONTEXT_MODE_API,
528538
nodes,
@@ -585,26 +595,26 @@ private String getExpectedXContentInGatewayMode(DiscoveryNodes nodes) {
585595
return "{\n" + " \"nodes\" : {\n" + nodes.getNodes().entrySet().stream().map(entry -> {
586596
int id = Integer.parseInt(entry.getKey().split("_")[1]);
587597
DiscoveryNode node = entry.getValue();
588-
String indent = " ";
598+
String offset = " ";
589599
return String.format(
590600
Locale.ROOT,
591601
expectedNodeAPUXContent,
592-
indent,
602+
offset,
593603
id,
594604
node.getEphemeralId(),
595605
entry.getValue().getAddress().toString(),
596606
node.getAttributes().isEmpty()
597607
? " "
598-
: "\n" + indent + " \"custom\" : \"" + node.getAttributes().get("custom") + "\"\n " + indent,
608+
: "\n" + offset + " \"custom\" : \"" + node.getAttributes().get("custom") + "\"\n " + offset,
599609
node.getVersion(),
600610
node.getRoles().isEmpty()
601611
? " "
602612
: "\n"
603-
+ indent
613+
+ offset
604614
+ " \""
605-
+ node.getRoles().stream().map(DiscoveryNodeRole::roleName).collect(Collectors.joining("\",\n" + indent + " \""))
615+
+ node.getRoles().stream().map(DiscoveryNodeRole::roleName).collect(Collectors.joining("\",\n" + offset + " \""))
606616
+ "\"\n "
607-
+ indent
617+
+ offset
608618
);
609619
}).collect(Collectors.joining(",\n"))
610620
+ "\n"
@@ -617,9 +627,7 @@ private String getExpectedXContentInGatewayMode(DiscoveryNodes nodes) {
617627

618628
public void verifyToXContentInContextMode(String context, DiscoveryNodes nodes, String expected) throws IOException {
619629
XContentBuilder builder = JsonXContent.contentBuilder().prettyPrint();
620-
builder.startObject();
621630
nodes.toXContent(builder, new ToXContent.MapParams(singletonMap(Metadata.CONTEXT_MODE_PARAM, context)));
622-
builder.endObject();
623631

624632
assertEquals(expected, builder.toString());
625633
}
@@ -654,11 +662,13 @@ private void doFromXContentTestWithRandomFields(boolean addRandomFields) throws
654662
() -> randomAlphaOfLengthBetween(3, 10)
655663
)
656664
);
657-
IllegalArgumentException iae = expectThrows(
658-
IllegalArgumentException.class,
665+
AssertionError iae = expectThrows(
666+
AssertionError.class,
659667
() -> DiscoveryNodes.fromXContent(createParser(mediaType.xContent(), mutated))
660668
);
661-
assertEquals(iae.getMessage(), "unexpected value field " + unsupportedField);
669+
assertTrue(
670+
iae.getMessage().matches("expecting field with name \\[([a-z]+(?:_[a-z]+)*)] but found \\[" + unsupportedField + "]")
671+
);
662672
} else {
663673
try (XContentParser parser = createParser(mediaType.xContent(), originalBytes)) {
664674
DiscoveryNodes parsedNodes = DiscoveryNodes.fromXContent(parser);

0 commit comments

Comments
 (0)