LLClient: Add String flavored setEntity #30447

nik9000 · 2018-05-07T23:39:07Z

Adds Request#setEntity(String) which short circuits the process of
sending a json string which is super common.

Adds `Request#setEntity(String)` which short circuits the process of sending a json string which is super common.

elasticmachine · 2018-05-07T23:39:08Z

Pinging @elastic/es-core-infra

nik9000 · 2018-05-07T23:41:47Z

@javanna this follows up on some comments you've made on my recent PRs to use the Request flavored methods in the http client.

I think this extra method would be pretty useful. I'm not really sold on a byte[] version of the method though because it feels like we're adding more and more flavors for less and less common things. So I figure I'd propose this change and see where it goes.

dadoonet

I like that change! 😉
I left 2 comments.

dadoonet · 2018-05-08T00:42:30Z

modules/reindex/src/main/java/org/elasticsearch/index/reindex/remote/RemoteRequestBuilders.java

@@ -151,8 +149,7 @@ static Request initialSearch(SearchRequest searchRequest, BytesReference query,
            }

            entity.endObject();
-            BytesRef bytes = BytesReference.bytes(entity).toBytesRef();
-            request.setEntity(new ByteArrayEntity(bytes.bytes, bytes.offset, bytes.length, ContentType.APPLICATION_JSON));
+            request.setEntity(Strings.toString(entity));


Should we support as well the method for bytes?

Ha! Just saw your other comment.

on this one I am on the fence. It is odd that we have to go from bytes to string here. I would rather leave what you had, or add a method to set a byte array, even if we are the only ones using it.

dadoonet · 2018-05-08T00:43:32Z

modules/reindex/src/main/java/org/elasticsearch/index/reindex/remote/RemoteRequestBuilders.java

            return request;
        }

        try (XContentBuilder entity = JsonXContent.contentBuilder()) {
            entity.startObject()
                    .field("scroll_id", scroll)
                .endObject();
-            request.setEntity(new StringEntity(Strings.toString(entity), ContentType.APPLICATION_JSON));
+            request.setEntity(Strings.toString(entity));


Should we support the method for XContent as well?

low-level client doesn't depend on xcontent, we can't do that

javanna

left a few nits, LGTM otherwise. thanks @nik9000 !

javanna · 2018-05-08T09:53:12Z

client/rest/src/main/java/org/elasticsearch/client/Request.java

+     * If you need a different content type then use
+     * {@link #setEntity(HttpEntity)}.
+     */
+    public void setEntity(String entity) {


nit: call it setJsonEntity?

javanna · 2018-05-08T09:58:03Z

modules/reindex/src/main/java/org/elasticsearch/index/reindex/remote/RemoteRequestBuilders.java

            return request;
        }

        try (XContentBuilder entity = JsonXContent.contentBuilder()) {
            entity.startObject()
                    .field("scroll_id", scroll)
                .endObject();
-            request.setEntity(new StringEntity(Strings.toString(entity), ContentType.APPLICATION_JSON));
+            request.setEntity(Strings.toString(entity));


low-level client doesn't depend on xcontent, we can't do that

javanna · 2018-05-08T09:58:53Z

modules/reindex/src/main/java/org/elasticsearch/index/reindex/remote/RemoteRequestBuilders.java

@@ -151,8 +149,7 @@ static Request initialSearch(SearchRequest searchRequest, BytesReference query,
            }

            entity.endObject();
-            BytesRef bytes = BytesReference.bytes(entity).toBytesRef();
-            request.setEntity(new ByteArrayEntity(bytes.bytes, bytes.offset, bytes.length, ContentType.APPLICATION_JSON));
+            request.setEntity(Strings.toString(entity));


on this one I am on the fence. It is odd that we have to go from bytes to string here. I would rather leave what you had, or add a method to set a byte array, even if we are the only ones using it.

javanna · 2018-05-08T09:59:27Z

client/rest/src/main/java/org/elasticsearch/client/Request.java

+     * {@link #setEntity(HttpEntity)}.
+     */
+    public void setEntity(String entity) {
+        setEntity(entity == null ? null : new NStringEntity(entity, ContentType.APPLICATION_JSON));


can we add a small unit test for this method?

nik9000 · 2018-05-09T14:00:52Z

Yikes! I'm not sure how I let the failure through....

nik9000 · 2018-05-09T22:34:46Z

Thanks for having a look @javanna and @dadoonet! I've merged and am cherry-picking back to 6.x now.

Adds `Request#setJsonEntity(String)` which short circuits the process of sending a json string which is super common.

* master: Upgrade to Lucene-7.4-snapshot-6705632810 (#30519) add version compatibility from 6.4.0 after backport, see #30319 (#30390) Security: Simplify security index listeners (#30466) Add proper longitude validation in geo_polygon_query (#30497) Remove Discovery.AckListener.onTimeout() (#30514) Build: move generated-resources to build (#30366) Reindex: Fold "with all deps" project into reindex (#30154) Isolate REST client single host tests (#30504) Solve Gradle deprecation warnings around shadowJar (#30483) SAML: Process only signed data (#30420) Remove BWC repository test (#30500) Build: Remove xpack specific run task (#30487) AwaitsFix IntegTestZipClientYamlTestSuiteIT#indices.split tests LLClient: Add setJsonEntity (#30447) Expose CommonStatsFlags directly in IndicesStatsRequest. (#30163) Silence IndexUpgradeIT test failures. (#30430) Bump Gradle heap to 1792m (#30484) [docs] add warning for read-write indices in force merge documentation (#28869) Avoid deadlocks in cache (#30461) Test: remove hardcoded list of unconfigured ciphers (#30367) mute SplitIndexIT due to #30416 Docs: Test examples that recreate lang analyzers (#29535) BulkProcessor to retry based on status code (#29329) Add GET Repository High Level REST API (#30362) add a comment explaining the need for RetryOnReplicaException on missing mappings Add `coordinating_only` node selector (#30313) Stop forking groovyc (#30471) Avoid setting connection request timeout (#30384) Use date format in `date_range` mapping before fallback to default (#29310) Watcher: Increase HttpClient parallel sent requests (#30130) # Conflicts: # x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/LocalStateCompositeXPackPlugin.java

* 6.x: Upgrade to Lucene-7.4-snapshot-6705632810 (#30519) Remove Discovery.AckListener.onTimeout() (#30514) Build: move generated-resources to build (#30366) Reindex: Fold "with all deps" project into reindex (#30154) Isolate REST client single host tests (#30504) Remove BWC repository test (#30500) Build: Remove xpack specific run task (#30487) AwaitsFix IntegTestZipClientYamlTestSuiteIT#indices.split tests LLClient: Add setJsonEntity (#30447) [docs] add warning for read-write indices in force merge documentation (#28869) Avoid deadlocks in cache (#30461) BulkProcessor to retry based on status code (#29329) Avoid setting connection request timeout (#30384) Test: remove hardcoded list of unconfigured ciphers (#30367) Add GET Repository High Level REST API (#30362) mute SplitIndexIT due to #30416 Docs: Test examples that recreate lang analyzers (#29535) add a comment explaining the need for RetryOnReplicaException on missing mappings Pass the task to broadcast actions (#29672) Stop forking groovyc (#30471) Add `coordinating_only` node selector (#30313) Fix accidental error in changelog Use date format in `date_range` mapping before fallback to default (#29310) Watcher: Increase HttpClient parallel sent requests (#30130) [Security][Tests] Azeri(Turkish) locale tripps opensaml dependency

* es/ccr: (78 commits) Upgrade to Lucene-7.4-snapshot-6705632810 (elastic#30519) add version compatibility from 6.4.0 after backport, see elastic#30319 (elastic#30390) Security: Simplify security index listeners (elastic#30466) Add proper longitude validation in geo_polygon_query (elastic#30497) Remove Discovery.AckListener.onTimeout() (elastic#30514) Build: move generated-resources to build (elastic#30366) Reindex: Fold "with all deps" project into reindex (elastic#30154) Isolate REST client single host tests (elastic#30504) Solve Gradle deprecation warnings around shadowJar (elastic#30483) SAML: Process only signed data (elastic#30420) Remove BWC repository test (elastic#30500) Build: Remove xpack specific run task (elastic#30487) AwaitsFix IntegTestZipClientYamlTestSuiteIT#indices.split tests Enable soft-deletes in v6.4 LLClient: Add setJsonEntity (elastic#30447) [CCR] Read changes from Lucene instead of translog (elastic#30120) Expose CommonStatsFlags directly in IndicesStatsRequest. (elastic#30163) Silence IndexUpgradeIT test failures. (elastic#30430) Bump Gradle heap to 1792m (elastic#30484) [docs] add warning for read-write indices in force merge documentation (elastic#28869) ...

LLClient: Add String flavored setEntity

9e54247

Adds `Request#setEntity(String)` which short circuits the process of sending a json string which is super common.

nik9000 added >enhancement review :Clients/Java Low Level REST Client Minimal dependencies Java Client for Elasticsearch v7.0.0 v6.4.0 labels May 7, 2018

nik9000 requested a review from javanna May 7, 2018 23:39

dadoonet reviewed May 8, 2018

View reviewed changes

javanna approved these changes May 8, 2018

View reviewed changes

nik9000 added 6 commits May 8, 2018 08:28

Rename

01e983c

Merge branch 'master' into rest_client_set_entity_2

84def91

Rename and unit test

04e9700

Changelog

2fe40c5

Merge branch 'master' into rest_client_set_entity_2

ea564a1

Merge branch 'master' into rest_client_set_entity_2

6122f94

nik9000 added 2 commits May 9, 2018 10:01

Merge branch 'master' into rest_client_set_entity_2

96ea9bc

Fix unit test

d946a6f

nik9000 merged commit b4502db into elastic:master May 9, 2018

nik9000 added a commit that referenced this pull request May 9, 2018

LLClient: Add setJsonEntity (#30447)

51a38cf

Adds `Request#setJsonEntity(String)` which short circuits the process of sending a json string which is super common.

colings86 added v7.0.0-beta1 and removed v7.0.0 labels Feb 7, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

LLClient: Add String flavored setEntity #30447

LLClient: Add String flavored setEntity #30447

nik9000 commented May 7, 2018

elasticmachine commented May 7, 2018

nik9000 commented May 7, 2018

dadoonet left a comment

dadoonet May 8, 2018

dadoonet May 8, 2018

javanna May 8, 2018

dadoonet May 8, 2018

javanna May 8, 2018

javanna left a comment

javanna May 8, 2018

javanna May 8, 2018

javanna May 8, 2018

javanna May 8, 2018

nik9000 commented May 9, 2018

nik9000 commented May 9, 2018

LLClient: Add String flavored setEntity #30447

LLClient: Add String flavored setEntity #30447

Conversation

nik9000 commented May 7, 2018

elasticmachine commented May 7, 2018

nik9000 commented May 7, 2018

dadoonet 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

javanna 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

Choose a reason for hiding this comment

nik9000 commented May 9, 2018

nik9000 commented May 9, 2018