Elasticsearch: Clarify ToXContent contract

[nik9000]

ElasticsearchException implements ToXContent but it doens't look like it works with XContentBuilder#writeValue because ElasticsearchException doesn't output its start and end object markers. Is this a bug? If so we should probably fix it and add more comments around toXContent's contract and probably make some simple way to test that implementers comply with the contract.

[bleskes]

This is a good one. The reason why some ToXContent implementations don't call start and end object is that sometime you want to output within another context, where you for example have added some information for context. For an example look at org.elasticsearch.rest.BytesRestResponse#convert where we enrich the top level error. I'm not saying this is the only solution, but this is why it's like that...

[nik9000]

Thanks for clarifying. I think it'd be more clear if we had two interfaces for the different contracts. We frequently use "innerToXContent" when we want to support this. I get that we have a bazillion interfaces already but it'd be nice to know more about the things that implement ToXContent. We could even have consistent tests for them!

[javanna]

Would be great to clarify and clean up this contract for sure, in this case using a class that extends ToXContent doesn't say enough, you have to look at the implementation to see if you have to open a new object yourself or not.

[javanna]

We have been encountering this inconsistency while writing parsing code for the High Level REST client. We could fix it as we go through all the toXContent methods (in separate PRs would be wise). I see that most of the objects don't open a start object, and output essentially fragments that are not valid without an ancestor. On the other hand, our ActionResponses should always print out a complete object without the need to start and end the object externally, which some REST actions do and some others do not.

How about having ToXContent for complete objects and ToXContentFragment for fragments?

[javanna]

Discussed in FixItFriday, we agreed that this needs to be fixed. We are going to differentiate between the two contracts (self-contained objects and fragments) using two different interfaces.

[javanna]

22387 is in, so we now have ToXContent for fragments and ToXContentObject for complete objects. What is left to do is moving more classes over from ToXContent to ToXContentObject. That's why I'm keeping this issue open for now.

[andyb-elastic]

@javanna would it be helpful to have another person on this? (I see there are already a couple assignees)

[javanna]

@andy-elastic yes it would be helpful, @colings86 made quite some progress on this recently, see #25771

[elasticmachine]

Pinging @elastic/es-core-infra

[cbuescher]

@javanna I was just revisiting this old issue to see what we would need to close it. There's a great amount of classes now implementing ToXContentObject or ToXContentFragment in the code base. Its a bit hard to check what still directly implements "ToXContent" since we cannot completely eliminate the interface (in several places e.g. in generics definitions in Aggregations it makes sense to allow the generic interface), but from a quick glance at the type-hierarchy in my IDE there should be only a couple of classes left that need changing (and some in tests that we can probably ignore):

Should be put the remaining classes in a refactoring issues (checkbox list to tick off one by one), work of that list and call it done when we go a majority of the classes cut over? What else would be need to do before we can close this issue?

[javanna]

Should be put the remaining classes in a refactoring issues (checkbox list to tick off one by one), work of that list and call it done when we go a majority of the classes cut over?

+1

[pugnascotia]

Should be put the remaining classes in a refactoring issues (checkbox list to tick off one by one), work of that list and call it done when we go a majority of the classes cut over? What else would be need to do before we can close this issue?

@javanna / @cbuescher was a refactoring issue ever opened for the above?

[javanna]

not that I know. @cbuescher opened a followup PR which you can see above with some changes that were left to make. There are still some but they are not listed in an issue as far as I know.