Clean up XPackInfoResponse class and related tests #35547
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Response classes in Elasticsearch (and xpack) only need to implement ToXContent, which is needed to print their output put in the REST layer and return the response in json (or others) format. On the other hand, response classes that are added to the high-level REST client, need to do the opposite: parse xcontent and create a new object based on that. At the moment, both variants of XPackInfoResponse (xpack and client) do both: parsing and printing. This is redundant, it leads to code duplication, and redundant testing too. Now that we have a way to test these classes from a single test (see `AbstractHlrcStreamableXContentTestCase` base class), we should remove code duplication and only maintain the needed portions in each response class.
|
Pinging @elastic/es-core-infra |
|
You marked it as discuss, but I think this is the right way to do it. We need to make sure we don't accidentally remove tests for parsing at all, but I love that we're testing if the client can parse stuff sent from the server now. |
Agreed, please double check that I have only removed duplicated tests, that was my intention, but I may have missed some subtleties. |
|
retest this please |
nik9000
reviewed
Nov 14, 2018
And that isn't a thing you changed, either. |
|
@vladimirdolzhenko are you ok with this? |
|
@javanna sure, please go ahead 💯 |
javanna
added a commit
to javanna/elasticsearch
that referenced
this pull request
Nov 19, 2018
This commit removes the parsing code from the PostStartBasicResponse server variant, and the toXContent portion from the corresponding client variant (StartBasicResponse). Relates to elastic#35547
javanna
added a commit
to javanna/elasticsearch
that referenced
this pull request
Nov 19, 2018
This commit removes the parsing code from the PutLicenseResponse server variant, and the toXContent portion from the corresponding client variant. Relates to elastic#35547
javanna
added a commit
to javanna/elasticsearch
that referenced
this pull request
Nov 19, 2018
This commit removes the parsing code from the PutLicenseResponse server variant, and the toXContent portion from the corresponding client variant. Relates to elastic#35547
javanna
added a commit
to javanna/elasticsearch
that referenced
this pull request
Nov 19, 2018
This commit removes the parsing code from the PostStartBasicResponse server variant. It also makes the server response implement StatusToXContent which allows us to save a couple of lines of code in the corredponding REST action. Relates to elastic#35547
javanna
added a commit
that referenced
this pull request
Nov 21, 2018
This commit removes the parsing code from the PostStartBasicResponse server variant. It also makes the server response implement StatusToXContent which allows us to save a couple of lines of code in the corredponding REST action. Relates to #35547
javanna
added a commit
that referenced
this pull request
Nov 21, 2018
This commit removes the parsing code from the PutLicenseResponse server variant, and the toXContent portion from the corresponding client variant. Relates to #35547
javanna
added a commit
that referenced
this pull request
Nov 23, 2018
Response classes in Elasticsearch (and xpack) only need to implement ToXContent, which is needed to print their output put in the REST layer and return the response in json (or others) format. On the other hand, response classes that are added to the high-level REST client, need to do the opposite: parse xcontent and create a new object based on that. This commit removes the parsing code from the XPackInfoResponse server variant, and the toXContent portion from the corresponding client variant. It also removes a client specific test class that looks redundant now that we have a single test class for both classes.
javanna
added a commit
that referenced
this pull request
Nov 23, 2018
This commit removes the parsing code from the PostStartBasicResponse server variant. It also makes the server response implement StatusToXContent which allows us to save a couple of lines of code in the corredponding REST action. Relates to #35547
javanna
added a commit
that referenced
this pull request
Nov 23, 2018
This commit removes the parsing code from the PutLicenseResponse server variant, and the toXContent portion from the corresponding client variant. Relates to #35547
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is a follow-up of something that I raised in a previous PR.
Response classes in Elasticsearch (and xpack) only need to implement ToXContent, which is needed to print their output put in the REST layer and return the response in json (or others) format. On the other hand, response classes that are added to the high-level REST client, need to do the opposite: parse xcontent and create a new object based on that.
At the moment, both variants of XPackInfoResponse (xpack and client) do both: parsing and printing. This is redundant, it leads to code duplication, and redundant testing too. Now that we have a way to test these classes from a single test (see
AbstractHlrcStreamableXContentTestCasebase class), we should remove code duplication and only maintain the needed portions in each response class.This PR removes the parsing code from the
XPackInfoResponseserver variant, and thetoXContentportion from the corresponding client variant. It also removes a client specific test class that looks redundant now that we have a single test class for both classes. I will mark this for discussion to see whether this is the approach that we want to take moving forward.