This doesn't seem to align with your implementation of ToXContentFragment (which doesn't require start/end object) as well as the code on lines 595-597 which implies the START_OBJECT is optional. You'd hit the end of the parser without getting the END_OBJECT. (Same comment on DiscoveryNodes class below.)

I believe there might be some misunderstanding here. I agree that DiscoveryNode object's XContent does not need to have a start and end object, but here we are checking for the end object which is added in line 581 above

But for DiscoveryNodes, this comment does make sense. I will modify the class to implement ToXContentObject.

I believe there might be some misunderstanding here. I agree that DiscoveryNode object's XContent does not need to have a start and end object

I'm not sure this is the desired state here. Why don't you want to extend ToXContentObject instead of ToXContentFragment?

but here we are checking for the end object which is added in line 581 above

You are also adding a start object. Which is fine as long as they're paired. But you can't guarantee (based on the interface type) that you're always receiving the same object you wrote. It's possible someone else may make a larger object and include this inline -- the purpose of ToXContentFragment.

You make the start object optional here:

        if (parser.currentToken() == XContentParser.Token.START_OBJECT) {
            parser.nextToken();
        }

So if someone else trusts your object inheritance and places the object inline without start and end objects then you'd never get to the end and would keep iterating parser.nextToken(). Eventually that will be null, and null won't equal end object, so inside the loop you'll throw an NPE on token.isValue().

So you need to either test for a null nextToken() in addition to the END_OBJECT as a loop stopping condition, or you need to implement/extend ToXContentObject which requires start and end objects. I suspect you probably want the latter.

Actually, DiscoveryNode is part of the DiscoveryNodes toXContent response and getting used as a fragment like you mentioned and I plan to use DiscoveryNodes toXContent in ClusterState going forward. That's the reason for keeping them Fragment. Anyway, I have pushed a commit modifying the toXContent method. If you still believe we can hit a NPE in the current method. Can you please provide a sample json, I would write a unit test based on that to ensure that NPE is never thrown from our function.

you are implementing ToXContentFragment but you really should implement ToXContentObject here.

I am not changing the implementing ToXContentFragment interface in the DiscoveryNode. It was already an implementation of the this interface and changing it to a ToXContentObject might causing breaking changes in our multiple API responses. As this object is already getting used as a inline object in cluster state API, and other cat APIs.

No, it's being used as an object. Your example in a comment below shows how it's used inline, and it includes th e { (START_OBJECT) and } (END_OBJECT). It's a nested complete object, not a fragment.

Even a nested object is a fragment only as there is no start and end object around it. According to our documentation here, a fragment needs to have start and end object externally.
The example of you gave around fragment being used for creating array. My use case is also similar, where instead of create a field which has a START_ARRAY, our object has START_OBJECT.

The JSON you have provided is a invalid. And we don't expect to parse XContent not created from our method. Even if our object is getting used inline, like I am using in DiscoveryNodes class, the XContent is created from our toXContent method and object is parsed from our fromXContent method.

you must match a required end object to a required start object. You require one but make the other one optional

I am matching start and end object which are being added in our toXContent method. The line 590-592 you are referring, is to enable parsing of following object. This is a complete XContent object, but even if we don't receive the complete object (i.e. without start and end), we will be able to parse that inline object.

{
  "node_1" : {
    "name" : "name_1",
    "ephemeral_id" : "3Q3xRwYKScWqBgVCrWmNCQ",
    "transport_address" : "0.0.0.0:2",
    "attributes" : {
        "custom" : "PKU"
    }
  }
}

I am not changing the implementing ToXContentFragment interface in the DiscoveryNode. It was already an implementation of the this interface and changing it to a ToXContentObject might causing breaking changes

Agreed, you shouldn't change this.

Even a nested object is a fragment only as there is no start and end object around it.

Agreed.... but then you check for the end object in the line prompting this comment chain:

 while ((token = parser.nextToken()) != XContentParser.Token.END_OBJECT) 

If there is no end object, this loop will never reach its stopping condition.

The JSON you have provided is a invalid.

I know. But the parsing error thrown would have been somewhere unrelated to the invalid JSON, in some follow-on code much more confusing to debug. I just threw up a quick example to show that in the case you do not require a start object, and thus shouldn't require an end object (because they should match) you end up with the code as it was written hitting some later end object. Or possibly getting a NPE if you're just parsing a small fragment with no start or end.

you must match a required end object to a required start object. You require one but make the other one optional

I am matching start and end object which are being added in our toXContent method.

When you create the JSON you can match them. If you are reading the JSON created elsewhere you do not control that. Someone else could be creating that JSON outside of core.

The code around the loop looks better because you're matching the start (lines 603-604) to the end (the stopping condition for the loop in 605).

But there is still an optional START_OBJECT present in lines 590-592 without a corresponding end object. I still think this is wrong.

It should be legal for anyone parsing this JSON to parse their own start/end outside of your parsing. SO taking your JSON above:

{
  "node_1" : {
    "name" : "name_1",
    "ephemeral_id" : "3Q3xRwYKScWqBgVCrWmNCQ",
    "transport_address" : "0.0.0.0:2",
    "attributes" : {
        "custom" : "PKU"
    }
  }
}

it should be legal to pass this (valid JSON) string as a fragment which completely encapsulates all the information in the object, lacking only its "field name" which is only relevant for DiscoveryNodes.

  {
    "name" : "name_1",
    "ephemeral_id" : "3Q3xRwYKScWqBgVCrWmNCQ",
    "transport_address" : "0.0.0.0:2",
    "attributes" : {
        "custom" : "PKU"
    }
  }

This fails, however, because your fragment behavior is not just the value, it's a key-value pair expected by DiscoveryNodes parsing and you require the format to be:

  "node_1" : {
    "name" : "name_1",
    "ephemeral_id" : "3Q3xRwYKScWqBgVCrWmNCQ",
    "transport_address" : "0.0.0.0:2",
    "attributes" : {
        "custom" : "PKU"
    }
  }

Essentially, the optional start object IS read when parsing a standalone object but is NOT read when parsing the specific key/value format you include in DiscoveryNodes which omits the end object.

So ultimately my chief complaint here is that having the "field name" as part of this object is wrong; I should be able to have a standalone object that works without needing DiscoveryNodes wrapped around it.

To be more clear/succinct, your representation of DiscoveryNode in JSON is this (optionally wrapped in curly braces) key value pair.

{
  "node_1" : {
    "name" : "name_1",
    "ephemeral_id" : "3Q3xRwYKScWqBgVCrWmNCQ",
    "transport_address" : "0.0.0.0:2",
    "attributes" : {
        "custom" : "PKU"
    }
  }
}

or this because the object wrapper is optional, but requires the method caller to know that it's expecting a field name and object:

  "node_1" : {
    "name" : "name_1",
    "ephemeral_id" : "3Q3xRwYKScWqBgVCrWmNCQ",
    "transport_address" : "0.0.0.0:2",
    "attributes" : {
        "custom" : "PKU"
    }
  }

This is not the expectation I've seen anywhere for XContentFragments which are usually standalone values (which may or may not be objects).

I believe it should either be:

{
  "name" : "name_1",
  "id" : "node_1",
  "ephemeral_id" : "3Q3xRwYKScWqBgVCrWmNCQ",
  "transport_address" : "0.0.0.0:2",
  "attributes" : {
      "custom" : "PKU"
  }
}

or your original format with the object wrappers mandatory (even if you keep the fragment interface).

You're mixing-and-matching pairing of start object which relies heavily on implementation details of other classes.

@dbwiddis Thanks for being patient to write the detailed comment. Got your point on that we should be able to pass the inner nested object without the fieldname. But we need to create a parser which takes id as a parameter.

DiscoveryNode fromXContent(XContentParser parser, String nodeId)

I am assuming this will help address your concern.

OK, @shiv0408 thanks for the explanation. I see now that this format is expected. It's just very confusing for a maintainer. You've obviously spent more time digging into this and I agree that your implementation is the necessary way to do it.

So my only request now is that you write a javadoc for the fromXContent() method indicating the expected format, specifically either a key-object pair standalone, or a key-object pair wrapped inside start and end objects, so the next person to come along and look at this code won't be as confused as I was.

Would this be able to handle ipv6 addresses?

Don't think we will be able to do that now. I will modify this and add a test to ensure that TransportAddress.fromString can parse both ipv4 and ipv6 addresses.