Add typless client side GetIndexRequest calls and response class

[cbuescher]

The HLRC client currently uses org.elasticsearch.action.admin.indices.get.GetIndexRequest
and org.elasticsearch.action.admin.indices.get.GetIndexResponse in its get index calls. Both request and
response are designed for the typed APIs, including some return types e.g. for getMappings() which in
the maps it returns still use a level including the type name.
In order to change this without breaking existing users of the HLRC API, this PR introduces two new request
and response objects in the org.elasticsearch.client.indices client package. These are used by the
IndicesClient#get and IndicesClient#exists calls now by default and support the type-less API. The old request
and response objects are still kept for use in similarly named, but deprecated methods.
The newly introduced client side classes are simplified versions of the server side request/response classes since
they don't need to support wire serialization, and only the response needs fromXContent parsing (but no
xContent-serialization, since this is the responsibility of the server-side class).
Also changing the return type of GetIndexResponse#getMapping to
ImmutableOpenMap<String, MappingMetaData> getMappings(), while it previously was returning another map
keyed by the type-name.

[elasticmachine]

Pinging @elastic/es-search

[elasticmachine]

Pinging @elastic/es-core-features

[cbuescher]

@elasticmachine run elasticsearch-ci/1

[cbuescher]

Needed this access for tests.

[cbuescher]

@jtibshirani thanks for the reviews, I pushed a round of changes hopefully adressing your comments.

[jtibshirani]

Sorry for the confusion in my previous review, this is now looking good to me from the types deprecation side. I just left a couple questions around tests.

[jtibshirani]

I'm not sure I really understand this TODO. For what purpose would we need a dependency on server:test?

[jtibshirani]

An update: as I was working on the same refactor for 'get mappings', I found it nice to use an xContentTester. I ended up creating a method GetMappingsResponseTests#toXContent that converts an HLRC GetMappingsResponse to a server-side one, then calls toXContent on that. Here's the test class in case you'd like to take a look: https://github.com/elastic/elasticsearch/pull/37796/files#diff-f66406b858061334c3239c8d2148b2caR41

[cbuescher]

Thanks, I'll take a look...

[cbuescher]

I see what you did there, I think its basically the same as my approach, although I have to say I really dislike using xContentTester for all the fuss around having to set up functions for everything. If you've used it several times it becomes clearer what it does, but its over-engineered from my point of view. I can change to using it if you really like though.
The main point though is to get an xContent rendering of the server side response. While you are generating a random instance of the client-side class, then convert it to server-side to call its xContent, I copied the code to create a randomized server-side response directly from org.elasticsearch.action.admin.indices.get.GetIndexResponseTests. I think it boild down to the same disadvantage, namely having to copy randomization code (which you also seem to have copied over, at least partially). By my comment I mean it would be great if we could call org.elasticsearch.action.admin.indices.get.GetIndexResponseTests#createTestInstance in the client-side test to be sure we get a "real" server side response with all its randomization variants, and call its "real" toXContent, because that is what the client is going to have to parse. In order to avoid code changing on the server side unnoticed it would be good to actually use that code, not rely on the copy being up to date which we have to do if we copy. Does this make sense?

The problem I'm running into here is that the client project can see org.elasticsearch.action.admin.indices.get.GetIndexResponse but not org.elasticsearch.action.admin.indices.get.GetIndexResponseTests. Maybe we could have a test dependency for this? But @hub-cap might know. We can probably do this in a follow up though if people think its useful to have.

[jtibshirani]

Thanks, I understand your comment now! Doing a bit more digging, I see AbstractHlrcXContentTestCase, which I think was designed for this situation. This sort of class tests both server serialization and HLRC serialization at the same time, which means there is a single place to generate random server objects. HLRC serialization is tested by generating a random server object, calling toXContent on it, and parsing it back as an client object. However, this class currently lives in the x-pack directory, which doesn't seem like a natural place to move GetMappingsResponseTests, GetIndexResponseTests, etc.

A proposal for what to do with our two open PRs:

• We can plan to follow-up with @hub-cap to discuss where to move AbstractHlrcXContentTestCase, then refactor Get*Tests to all extend this class.
• For now we can generate random server objects (as you do in this PR). I'll switch my PR to do that instead of generating a client-side one, so we are closer to modeling what actually happens during an HLRC call.
• I would prefer us to use xContentTester, because although it's a bit overkill, it is used extensively and I think being consistent with the test framework leads to overall less confusion than being inconsistent but slightly simpler in one or two tests.

[jtibshirani]

An update: after a more in-depth discussion with @hub-cap, these are his preferences for how to test these Get*Response classes that contain mappings:

• He agrees we should use an xContentTester object, to be consistent with the many other tests that have been written in this style.
• We should test serialization by first generating a random client instance, then converting it to a server object and calling toXContent, then finally parsing it back as a client object using fromXContent. Here is an example of this approach: https://github.com/elastic/elasticsearch/pull/37796/files#diff-f66406b858061334c3239c8d2148b2caR41. This approach is more straightforward and lets us test equality on the client objects.

I updated my 'get mappings' PR to be in line with the two points above, and we've made sure the tests for 'get index template' do the same. It would be great to rework this test to also follow this approach before merging, so that all of our tests are consistent.

I share your concern around duplicating the logic to generate a random instance, and having to make sure that all updates to the server-side test are also reflected in the client-side. We can plan to follow-up with a discussion on this point (that may include a discussion of AbstractHlrcXContentTestCase, as mentioned above).

[cbuescher]

@jtibshirani thanks, I adressed one of your comments, left a response to the other. Let me know if we should discuss this again later.

[jtibshirani]

I left one comment about refactoring the test to use xContent, to be consistent with our overall testing approach. However this is looking very nice and I'm approving now so as not to hold up merging after the test changes.

[cbuescher]

not to hold up merging after the test changes

@jtibshirani sorry, I'm not sure if this refers to changes you want me to still make or changes we can do as a follow re our discussion in #37778 (comment). Reading that comment I don't see any changes needed for this PR, but will wait for confirmation just before merging.

[cbuescher]

@jtibshirani I updated the test in 6a8a8a9. I have to admit I'm not very happy with the way we test this, but I tried following your suggestions from #37778 (comment) to get this PR merged. I think we should iterate on the testing later on.
Furthermore I simplified most getters for the new client-side GetIndexResponse to return plain Maps instead of the ImmutableOpenMaps that the server side response returns. I saw e.g the new client side GetMappingsResponse does the same so I adapted this approach. Let me know if this works for you.

[jtibshirani]

Thank you @cbuescher for making these updates, they look good to me. Sounds good about iterating on the approach to testing -- at least the strategy is consistent now across the different HLRC classes, which will make it easier to do a follow-up refactor.