ActionResponse should implement ToXContentObject #3889
Comments
ghost
assigned 
clintongormley
added
help wanted
|
@s1monw @javanna @cbuescher Have we made progress on this? Is there still more work to be done? |
|
We didn't make any progress, I think it's still a nice to have, not super important though. In theory it should be about moving |
|
Also some related bits here |
|
This is related to the parsing effort we have been making for the High Level REST Client. We can definitely fix it as we go. @tlrx what do you think? |
|
I agree, would be nice to fix that. |
javanna
changed the title
|
I updated the title of this issue as at this point ActionResponse should implement |
SearchScrollRequest can be created from a request body, but it doesn't support the opposite, meaning printing out its content to an XContentBuilder. This is useful to the high level REST client and allows for better testing of what we parse. Moved parsing method from RestSearchScrollAction to SearchScrollRequest so that fromXContent and toXContent sit close to each other. Added unit tests to verify that body parameters override query_string parameters when both present (there is already a yaml test for this but unit test is even better) Relates to elastic#3889
…28878) While working on #27799, we find that it might make sense to change BroadcastResponse from ToXContentFragment to ToXContentObject, seeing that it's rather a complete XContent object and also the other Responses are normally ToXContentObject. By doing this, we can also move the XContent build logic of BroadcastResponse's subclasses, from Rest Layer to the concrete classes themselves. Relates to #3889
…28878) While working on #27799, we find that it might make sense to change BroadcastResponse from ToXContentFragment to ToXContentObject, seeing that it's rather a complete XContent object and also the other Responses are normally ToXContentObject. By doing this, we can also move the XContent build logic of BroadcastResponse's subclasses, from Rest Layer to the concrete classes themselves. Relates to #3889
jaymode
added
team-discuss
and removed
needs:triage
|
We discussed this issue in core/infra today. While it is certainly an admirable goal, and may be nice to have, forcing all ActionResponses to have xcontent representations has not been a priority for over 7 years. In practice, the responses that need xcontent get it through other subclasses, and transport actions are often separated from the REST side that needs xcontent, so in many cases it is not necessary. Given that, we don't plan to work on this. |
He should across the board implement
ToXContentand all responses should implement is without surroundingstart/endObject. I started doing one of them in this PR: #3871 but as @martijnvg pointed out we should have all of them being consistent about it.The text was updated successfully, but these errors were encountered: