Remove unused xContent serialization logic in transport objects. #49346
Conversation
Previously, request and response objects related to index creation and mappings were used in both the transport layer and HLRC. Now that they are no longer shared, we can remove the extra xContent serialization + deserialization logic.
jtibshirani
added
:Search Foundations/Mapping
|
Pinging @elastic/es-search (:Search/Mapping) |
|
@elasticmachine run elasticsearch-ci/oss-distro-docs |
There was a problem hiding this comment.
Hi @jtibshirani, thanks for taking care of this, great to be able to remove a bunch of duplicated code.
I left a few comments around testing where I think we might be losing a bit of coverage. Let me know what you think.
| mapping.endObject(); | ||
| mapping.endObject(); | ||
| request.source(mapping); | ||
| public void testFromXContent() throws IOException { |
There was a problem hiding this comment.
Reading this I was wondering if we need this test at all anymore. The way I understand the server-side request class, is just stores the mapping source definition as a Json String, which is converted back to a map somewhere else I suppose. Let me know what you think, maybe we don't need this here.
There was a problem hiding this comment.
After looking at the logic more closely, I agree this test doesn't seem too valuable.
There was a problem hiding this comment.
It might even be worth looking into why we store the source as a json string when we later parse it to a map. But that's digressing from this PR, I can make a note of that and take a look.
| @@ -118,65 +112,4 @@ public void testMappingKeyedByType() throws IOException { | |||
| assertEquals(request1.mappings(), request2.mappings()); | |||
| } | |||
| } | |||
|
|
|||
| @Override | |||
| protected PutIndexTemplateRequest createTestInstance() { | |||
There was a problem hiding this comment.
It looks like the server side PutIndexTemplateRequest still supports internal wire serialization, which I think we should still test using random instances. Maybe changing this test to one of the AbstractWire*TestCase (tbh I'm getting confused which one would be appropriate) would be good, as long as we are able to detect faulty writeTo/readFrom implementation if we e.g. add new parameters.
There was a problem hiding this comment.
I had initially tried this out, but ended up uncovering some small bugs in PutIndexTemplateRequest. So I planned to defer the change to a follow-up PR -- is that okay with you? Currently the test just extends AbstractXContentTestCase, so it doesn't test wire serialization and I don't think we are regressing test coverage in that regard.
There was a problem hiding this comment.
No problem, I assumed AbstractXContentTestCase would also include wire serialization tests. I was wrong, but thats part of why I dislike our growing AbstractWire/XContent/*/TestCase family of base tests, because after a while its hard to remember what exactly they are doing. If you could open an issue that we should test serialization here and maybe add your findings about the "small bugs" that you found, that would be great.
|
|
||
| @Override | ||
| protected PutIndexTemplateRequest doParseInstance(XContentParser parser) throws IOException { | ||
| return new PutIndexTemplateRequest().source(parser.map()); |
There was a problem hiding this comment.
If we don't test parsing for this request any more, we should maybe unit test the source(Map<String, Object> templateSource) method directly here. There's a bunch of code in there that e.g. throws exceptions sets request properties. I think we call this in the PutIndexTemplateRequestTests in the client module, but for good unit testing coverage I think we should add independent testing to this class as well.
This could be done in this PR or in a follow up issue as far as I'm concerned.
There was a problem hiding this comment.
👍 will add a couple tests around PutIndexTemplateRequest#source.
|
@cbuescher I tried to address your comments, this is now ready for another look. |
|
I opened #49507 to track the lack of tests for wire serialization. |
Previously, request and response objects related to index creation and mappings
were used in both the transport layer and HLRC. Now that they are no longer
shared, we can remove the extra xContent serialization + deserialization logic.