Add Get Source API to the HLRC

[timoninmaxim]

Add Get Source API to the HLRC

relates: #47678

[elasticmachine]

Pinging @elastic/es-core-features (:Core/Features/Java High Level REST Client)

[martijnvg]

Thanks @timoninmaxim for working on this! I left a comment around the request/response class.

[martijnvg]

I think this api needs to use its own Request and Response class.

Although the get source api support almost all request parameters that the get api support, the get source api doesn't support stored_fields, version and version_type. The latter two may be supported in the future, but I don't see stored_fields ever be supported (that retrieves something that isn't part of the _source). Also I see that the get api may support doc_value_fields option to and this is unrelated to the _source like stored_fields is. Therefor I think that this api should have its own high level client side request class.

I also think that this api should have a dedicated response class, that just includes a Map<String, Object> field for the source instead of the generic RestResponse class.

[timoninmaxim]

Server's RestGetSourceAction returns BytesRestResponse object. Actually HLRC getSouce method returns the same, I just used more generic class for the method signature. Will it be ok to replace RestResponse with BytesRestResponse, or Map<String, Object> is preferable anyway?

[martijnvg]

Server's RestGetSourceAction returns BytesRestResponse object.

So do many RestAction classes, it is a common way for rest actions to serialize the response to a byte array, so that the networking layer can send a response.

Will it be ok to replace RestResponse with BytesRestResponse, or Map<String, Object> is preferable anyway?

I think if the getSource(...) methods return a Map<String, Object> then that is ok too, because _source is the only thing that this API returns and the exists(...) and existsSource(...) methods in RestHighLevelClient class also return just a boolean.

[timoninmaxim]

@martijnvg thanks for the clarification! Could I use new GetSourceRequest class with exists(...) method too as under the hood they use the same server's API? But it's already released with v6.6.0 (#34519), are we able break compatibility here?

[martijnvg]

Let's do that in a separate PR. We can then deprecate the old methods in favour for the new methods.

[timoninmaxim]

Ok, I've committed the requested change. Should I create an issue for change exists(...) or it's possible to link to this conversation in new PR?

[martijnvg]

No need to create an issue for this. You can just mention it in a new PR.

[timoninmaxim]

@martijnvg Create PR for that #51789

[martijnvg]

Left 2 small comments, otherwise LGTM.

[martijnvg]

Maybe add a test for this in RequestConvertersTests?

[martijnvg]

There isn't much here, but maybe also add a test for this? You can extend from AbstractResponseTestCase, this should be straight forward.

[martijnvg]

@elasticmachine test this please

[martijnvg]

@elasticmachine test this please

[martijnvg]

Thanks @timoninmaxim!
When the build is successful then I will merge this.

[martijnvg]

@elasticmachine test this please

[timoninmaxim]

Hi @martijnvg ! I don't know why those tests are failed, locally they passed successfully. Is it possible to rerun the tests?

[martijnvg]

Regarding hlrc module, this did fail:

16:32:41 > Task :client:rest-high-level:checkstyleTest FAILED
16:32:41 [ant:checkstyle] [ERROR] /dev/shm/elastic+elasticsearch+pull-request-1/client/rest-high-level/src/test/java/org/elasticsearch/client/core/GetSourceResponseTests.java:25: Using the '.*' form of import should be avoided - org.elasticsearch.common.xcontent.*. [AvoidStarImport]

If you run ./gradlew -p client/rest-high-level/ precommit or ./gradlew -p client/rest-high-level/ check (also runs all tests) then you should get the same failure as well. Can you replace the wildcard imports with the imports of the actual classes being used? If you push a fix for the checkstyle failure then I will re-run the build.

The elasticsearch-ci/default-distro ci failure is unrelated to your change and a rerun is ok in this case.

[timoninmaxim]

It's strange. I've run client:rest-high-level [check] gradle command in Intellij IDEA and it was successful, but precommit is not. Thanks for clarification!

[martijnvg]

@elasticmachine test this please

[martijnvg]

I left a few comments around the GetSourceResponseTests test, which failed in CI.
I think that after this, this pr is good to get merged 🤞

[martijnvg]

This should be replaced by:

try (XContentBuilder sourceBuilder = XContentBuilder.builder(xContentType.xContent())) {
            sourceBuilder.startObject();
            sourceBuilder.field("field", "value");
            sourceBuilder.endObject();
            return new SourceOnlyResponse(BytesReference.bytes(sourceBuilder));
        } catch (IOException ioe) {
            throw new UncheckedIOException(ioe);
        }

Sometimes the test framework uses a different xcontent type, for example yaml or cbor and this is now hardcoded to json.

[martijnvg]

Actually this is not needed, because the XContentBuilder#rawValue invocation in the SourceOnlyResponse#toXContent() method does the right conversion.

[martijnvg]

You need to overwrite isFragment() method here and let it return false or implement ToXContentObject instead.
Otherwise the test base class tries to always add a json object, which is already added in this test.

[timoninmaxim]

yes you're correct, I've just did it and send new commit

[martijnvg]

The body of this method can be replaced with: assertThat(clientInstance.getSource(), equalTo(Map.of("field", "value")));. Just checking for the expected map is sufficient here.

[timoninmaxim]

did it too

[martijnvg]

@elasticmachine test this please

[martijnvg]

@elasticmachine run elasticsearch-ci/default-distro