Add Get Source API to the HLRC #50885

timoninmaxim · 2020-01-11T13:17:06Z

Add Get Source API to the HLRC

relates: #47678

elasticmachine · 2020-01-13T09:35:27Z

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 · 2020-01-13T14:39:26Z

client/rest-high-level/src/main/java/org/elasticsearch/client/RestHighLevelClient.java

+     * @param options the request options (e.g. headers), use {@link RequestOptions#DEFAULT} if nothing needs to be customized
+     * @return the response
+     */
+    public RestResponse getSource(GetRequest getRequest, RequestOptions options) throws IOException {


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.

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?

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.

@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?

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

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?

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

@martijnvg Create PR for that #51789

martijnvg

Left 2 small comments, otherwise LGTM.

martijnvg · 2020-01-14T14:20:54Z

client/rest-high-level/src/main/java/org/elasticsearch/client/RequestConverters.java

+        return request;
+    }
+
+    static Request getSource(GetSourceRequest getSourceRequest) {


Maybe add a test for this in RequestConvertersTests?

martijnvg · 2020-01-14T14:22:20Z

client/rest-high-level/src/main/java/org/elasticsearch/client/core/GetSourceResponse.java

+import java.io.IOException;
+import java.util.Map;
+
+public final class GetSourceResponse {


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

martijnvg · 2020-01-14T14:25:50Z

@elasticmachine test this please

relates: elastic#47678

martijnvg · 2020-01-20T10:27:37Z

@elasticmachine test this please

martijnvg

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

martijnvg · 2020-01-20T14:14:29Z

@elasticmachine test this please

timoninmaxim · 2020-01-20T15:13:51Z

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

martijnvg · 2020-01-20T16:11:32Z

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 · 2020-01-20T16:37:36Z

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 · 2020-01-20T16:38:14Z

@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 · 2020-01-21T08:53:42Z

client/rest-high-level/src/test/java/org/elasticsearch/client/core/GetSourceResponseTests.java

+
+    @Override
+    protected SourceOnlyResponse createServerTestInstance(XContentType xContentType) {
+        BytesReference source = new BytesArray("{\"field\":\"value\"}");


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.

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

martijnvg · 2020-01-21T08:56:45Z

client/rest-high-level/src/test/java/org/elasticsearch/client/core/GetSourceResponseTests.java

+public final class GetSourceResponseTests extends
+    AbstractResponseTestCase<GetSourceResponseTests.SourceOnlyResponse, GetSourceResponse> {
+
+    static class SourceOnlyResponse implements ToXContent {


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.

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

martijnvg · 2020-01-21T08:57:44Z

client/rest-high-level/src/test/java/org/elasticsearch/client/core/GetSourceResponseTests.java

+        expected.put("field", "value");
+
+        assertThat(clientInstance.getSource(), equalTo(serverTestInstance.source));
+        assertThat(clientInstance.getSource(), equalTo(expected));


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.

martijnvg · 2020-01-21T09:34:20Z

@elasticmachine test this please

martijnvg · 2020-01-21T11:06:06Z

@elasticmachine run elasticsearch-ci/default-distro

Relates to elastic#47678

Backport to 7.x branch of #50885. Relates to #47678 Co-authored-by: Maxim <timonin.maksim@mail.ru>

Ref elastic#50885

#51789) Originates from #50885

elastic#51789) Originates from elastic#50885

#51789) (#51913) Originates from #50885 Co-authored-by: Maxim <timonin.maksim@mail.ru>

timoninmaxim force-pushed the feature/get_source_api branch from 8a6fe9a to c79ed70 Compare January 12, 2020 11:50

cbuescher added :Core/Features/Java High Level REST Client >enhancement labels Jan 13, 2020

timoninmaxim mentioned this pull request Jan 13, 2020

Rest only High Level APIs completeness #47678

Closed

4 tasks

martijnvg reviewed Jan 13, 2020

View reviewed changes

timoninmaxim requested a review from martijnvg January 14, 2020 13:30

martijnvg approved these changes Jan 14, 2020

View reviewed changes

timoninmaxim added 2 commits January 20, 2020 12:39

Add Get Source API to the HLRC

1689b36

relates: elastic#47678

Use GetSourceRequest, GetSourceResponse for GetSource API

3c3ed1e

timoninmaxim force-pushed the feature/get_source_api branch from b795421 to eb81a82 Compare January 20, 2020 09:49

timoninmaxim requested a review from martijnvg January 20, 2020 09:50

martijnvg approved these changes Jan 20, 2020

View reviewed changes

Add tests for GetSourceRequest, GetSourceResponse

7e7341c

timoninmaxim force-pushed the feature/get_source_api branch from eb81a82 to 7e7341c Compare January 20, 2020 13:33

timoninmaxim requested a review from martijnvg January 20, 2020 13:34

Explicit list of imports

4ccf870

martijnvg reviewed Jan 21, 2020

View reviewed changes

martijnvg added v7.7.0 v8.0.0 labels Jan 21, 2020

fix tests: Use ToXContentObject for mock server response

bfa0cd9

timoninmaxim force-pushed the feature/get_source_api branch from 1a578ac to bfa0cd9 Compare January 21, 2020 09:03

timoninmaxim requested a review from martijnvg January 21, 2020 09:17

martijnvg merged commit c019230 into elastic:master Jan 23, 2020

martijnvg pushed a commit to martijnvg/elasticsearch that referenced this pull request Jan 23, 2020

Add Get Source API to the HLRC (elastic#50885)

814d5fb

Relates to elastic#47678

martijnvg mentioned this pull request Jan 23, 2020

Backport: Add Get Source API to the HLRC #51342

Merged

martijnvg added a commit that referenced this pull request Jan 23, 2020

Add Get Source API to the HLRC (#51342)

0a8d8d7

Backport to 7.x branch of #50885. Relates to #47678 Co-authored-by: Maxim <timonin.maksim@mail.ru>

timoninmaxim deleted the feature/get_source_api branch January 24, 2020 12:30

timoninmaxim added a commit to timoninmaxim/elasticsearch that referenced this pull request Feb 2, 2020

SourceExists HLRC uses GetSourceRequest instead of GetRequest

44dc8cd

Ref elastic#50885

timoninmaxim mentioned this pull request Feb 2, 2020

SourceExists HLRC uses GetSourceRequest instead of GetRequest #51789

Merged

martijnvg pushed a commit that referenced this pull request Feb 5, 2020

Change HLRC SourceExists to use GetSourceRequest instead of GetRequest (

b7f113e

#51789) Originates from #50885

martijnvg pushed a commit to martijnvg/elasticsearch that referenced this pull request Feb 5, 2020

Change HLRC SourceExists to use GetSourceRequest instead of GetRequest (

1e7a5ec

elastic#51789) Originates from elastic#50885

martijnvg mentioned this pull request Feb 5, 2020

Change HLRC SourceExists to use GetSourceRequest instead of GetRequest #51913

Merged

martijnvg added a commit that referenced this pull request Feb 5, 2020

Change HLRC SourceExists to use GetSourceRequest instead of GetRequest (

0610eb5

#51789) (#51913) Originates from #50885 Co-authored-by: Maxim <timonin.maksim@mail.ru>

codebrain mentioned this pull request Apr 1, 2020

7.7.0 meta ticket (Part 2) elastic/elasticsearch-net#4533

Closed

jakelandis added v8.0.0-alpha1 and removed v8.0.0 labels Jul 26, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Get Source API to the HLRC #50885

Add Get Source API to the HLRC #50885

timoninmaxim commented Jan 11, 2020

elasticmachine commented Jan 13, 2020

martijnvg left a comment

martijnvg Jan 13, 2020

timoninmaxim Jan 13, 2020 •

edited

Loading

martijnvg Jan 13, 2020

timoninmaxim Jan 14, 2020 •

edited

Loading

martijnvg Jan 14, 2020

timoninmaxim Jan 14, 2020

martijnvg Jan 14, 2020

timoninmaxim Feb 2, 2020

martijnvg left a comment

martijnvg Jan 14, 2020

martijnvg Jan 14, 2020

martijnvg commented Jan 14, 2020

martijnvg commented Jan 20, 2020

martijnvg left a comment

martijnvg commented Jan 20, 2020

timoninmaxim commented Jan 20, 2020

martijnvg commented Jan 20, 2020

timoninmaxim commented Jan 20, 2020

martijnvg commented Jan 20, 2020

martijnvg left a comment

martijnvg Jan 21, 2020

martijnvg Jan 21, 2020

martijnvg Jan 21, 2020

timoninmaxim Jan 21, 2020 •

edited

Loading

martijnvg Jan 21, 2020

timoninmaxim Jan 21, 2020

martijnvg commented Jan 21, 2020

martijnvg commented Jan 21, 2020

Add Get Source API to the HLRC #50885

Add Get Source API to the HLRC #50885

Conversation

timoninmaxim commented Jan 11, 2020

elasticmachine commented Jan 13, 2020

martijnvg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timoninmaxim Jan 13, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timoninmaxim Jan 14, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

martijnvg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

martijnvg commented Jan 14, 2020

martijnvg commented Jan 20, 2020

martijnvg left a comment

Choose a reason for hiding this comment

martijnvg commented Jan 20, 2020

timoninmaxim commented Jan 20, 2020

martijnvg commented Jan 20, 2020

timoninmaxim commented Jan 20, 2020

martijnvg commented Jan 20, 2020

martijnvg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timoninmaxim Jan 21, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

martijnvg commented Jan 21, 2020

martijnvg commented Jan 21, 2020

timoninmaxim Jan 13, 2020 •

edited

Loading

timoninmaxim Jan 14, 2020 •

edited

Loading

timoninmaxim Jan 21, 2020 •

edited

Loading