Add support for search templates to the high-level REST client. #30473

jtibshirani · 2018-05-09T06:37:20Z

No description provided.

elasticmachine · 2018-05-09T10:22:04Z

Pinging @elastic/es-core-infra

jtibshirani · 2018-05-10T18:25:42Z

docs/java-rest/high-level/search/search-template.asciidoc

+
+===== Additional References
+
+The https://www.elastic.co/guide/en/elasticsearch/reference/current/search-template.html#search-template[Search Template HTTP documentation]


I wasn't sure how to do a proper asciidoc link here, as I don't think these two sets of documentation are built together.

{ref}/search-template.html[Search Template documentation] should do it. That will also go to the proper branch. I would also remove the HTTP part from the description.

Thanks, I updated this reference.

javanna

left a few comments, mainly on testing, but this looks really good already, thanks @jtibshirani !

javanna · 2018-05-14T11:44:34Z

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

+            request = new Request(HttpGet.METHOD_NAME, "_render/template");
+        } else {
+            SearchRequest searchRequest = searchTemplateRequest.getRequest();
+            assert searchRequest != null : "When not simulating a template request, a search request must be present.";


given that the request goes through validate first, I think we could remove this assertion, this is already checked in as part of validate which will throw an error otherwise.

Oops, will remove.

javanna · 2018-05-14T11:52:23Z

docs/java-rest/high-level/search/search-template.asciidoc

+
+Search templates can be registered in advance through stored scripts API. Note that
+the stored scripts API is not yet available in the high-level REST client, so in this
+example we use the simple REST client.


s/simple/low-level ?

javanna · 2018-05-14T11:53:31Z

...t-high-level/src/test/java/org/elasticsearch/client/documentation/SearchDocumentationIT.java

+
+        // tag::register-script
+        Request scriptRequest = new Request("POST", "_scripts/title_search");
+        scriptRequest.setEntity(new NStringEntity(


you could use scriptRequest.setJsonEntity

javanna · 2018-05-14T12:07:51Z

docs/java-rest/high-level/search/search-template.asciidoc

+
+===== Additional References
+
+The https://www.elastic.co/guide/en/elasticsearch/reference/current/search-template.html#search-template[Search Template HTTP documentation]


{ref}/search-template.html[Search Template documentation] should do it. That will also go to the proper branch. I would also remove the HTTP part from the description.

javanna · 2018-05-14T12:14:29Z

docs/java-rest/high-level/search/search-template.asciidoc

+
+The https://www.elastic.co/guide/en/elasticsearch/reference/current/search-template.html#search-template[Search Template HTTP documentation]
+contains further examples of how search requests can be templated. For more detailed information on how mustache
+works and what can be done with it, please consult the http://mustache.github.io/mustache.5.html[online documentation of the mustache project].


I am a bit hesitant on the second sentence about mustache. I feel like we could do without it, here we are just documenting how to use the client.

I'm happy to remove it, especially as the mustache reference is available in the linked Search Template documentation.

javanna · 2018-05-14T12:15:36Z

...t-high-level/src/test/java/org/elasticsearch/client/documentation/SearchDocumentationIT.java

+            "  }" +
+            "}", ContentType.APPLICATION_JSON));
+        Response scriptResponse = restClient.performRequest(scriptRequest);
+        assertEquals(RestStatus.OK.getStatus(), scriptResponse.getStatusLine().getStatusCode());


can we leave the assertions out of the snippets that get published in the docs?

I removed all of the assertions. I plan to go back and fix the field capabilities documentation, since I added assertions there too!

javanna · 2018-05-14T12:17:35Z

...les/lang-mustache/src/main/java/org/elasticsearch/script/mustache/SearchTemplateRequest.java

+    public XContentBuilder toXContent(XContentBuilder builder, Params params) throws IOException {
+        builder.startObject();
+
+        if (scriptType == ScriptType.STORED) {


super minor nit: maybe this could be a switch ?

I would usually do use a switch, but here I found it harder to read because the enum values must be unqualified (case STORED instead of case ScriptType.STORED).

javanna · 2018-05-14T12:18:07Z

...les/lang-mustache/src/main/java/org/elasticsearch/script/mustache/SearchTemplateRequest.java

+        } else if (scriptType == ScriptType.INLINE) {
+            builder.field(SOURCE_FIELD.getPreferredName(), script);
+        } else {
+            throw new IllegalArgumentException("Unrecognized script type [" + scriptType + "].");


maybe this should rather be an UnsupportedOperationException ? I mean can this argument be provided with a different value than the ones supported?

Fixed, thanks.

javanna · 2018-05-14T12:27:35Z

...ang-mustache/src/test/java/org/elasticsearch/script/mustache/SearchTemplateRequestTests.java

+     * - We omit the random SearchRequest, since this component only affects the request
+     *   parameters and also isn't captured in the request's xContent.
+     */
+    public void testFromXContent() throws IOException {


Given these changes, I wonder if we should rather extend AbstractStreamableTestCase. We've had these problems in other cases, requests tend to require different equal comparisons depending on whether we are testing transport serialization or xcontent serialization. Transport serialization includes request parameters that are not part of the xcontent serialization, which is only about the http request body after all.
Got the xcontent part, maybe we could have a separate test class that extends AbstractXContentTestCase, where you can override assertEqualInstances and tweak all you need without having to override testFromXContent (hopefully)?

I ended up splitting out the xContent parts of this test into a new class, SearchTemplateRequestXContentTests. Please let me know if this is not what you had in mind.

javanna · 2018-05-14T12:28:13Z

...ng-mustache/src/test/java/org/elasticsearch/script/mustache/SearchTemplateResponseTests.java

+            skippedShards, tookInMillis, ShardSearchFailure.EMPTY_ARRAY, SearchResponse.Clusters.EMPTY);
+    }
+
+    private BytesReference createSource() {


can this be static?

jtibshirani · 2018-05-14T21:25:11Z

Thank you @javanna for the review. I've tried to address your comments.

javanna · 2018-05-15T12:09:32Z

...les/lang-mustache/src/main/java/org/elasticsearch/script/mustache/SearchTemplateRequest.java

 import org.elasticsearch.script.ScriptType;

 import java.io.IOException;
+import java.io.UnsupportedEncodingException;


wrong import?

Oops, thanks.

javanna · 2018-05-15T12:11:06Z

...ache/src/test/java/org/elasticsearch/script/mustache/SearchTemplateRequestXContentTests.java

+        expectedInstance.setRequest(null);
+        newInstance.setRequest(null);
+
+        super.assertEqualInstances(expectedInstance, newInstance);


nit: maybe instead of mutating the two instances we should have the proper comparisons? I guess it is a subset of what equals and hashcode do?

Thanks, I updated this and it looks clearer... we don't need the full equals and hashCode comparison because we're not supporting testEqualsAndHashcode.

javanna · 2018-05-15T12:12:12Z

...ache/src/test/java/org/elasticsearch/script/mustache/SearchTemplateRequestXContentTests.java

+     */
+    private XContentParser newParser(String s) throws IOException {
+        assertNotNull(s);
+        return createParser(JsonXContent.jsonXContent, s.replace("'", "\""));


you have this for readability of the snippets above? Or some other reason too?

This set-up comes from some pre-existing tests that I decided not to rewrite -- I think it's simply for readability.

javanna

left a couple of nits, LGTM otherwise, thanks a lot @jtibshirani

fbaligand · 2018-05-19T08:30:02Z

As v7 is planned far away, it would be great to back port this feature to 6.x branch !

javanna · 2018-05-22T09:00:20Z

backporting would definitely be the plan. @jtibshirani could you take care of it or was there any specific reason why this wasn't backported?

jtibshirani · 2018-05-22T16:58:19Z

Sure, I'll do that now. My mistake -- I wasn't up to speed on our backport policy.

fbaligand · 2018-05-22T17:26:56Z

Great ! 👍

(cherry picked from commit 4f9dd37)

* 6.x: [DOCS] Fixes typos in security settings Add support for indexed shape routing in geo_shape query (#30760) [DOCS] Splits auditing.asciidoc into smaller files Painless: Types Section Clean Up (#30283) [test] java tests for archive packaging (#30734) Deprecate http.pipelining setting (#30786) [DOCS] Fix more edit URLs in Stack Overview (#30704) Use correct cluster state version for node fault detection (#30810) [DOCS] Fixes broken link for native realm [DOCS] Clarified audit.index.client.hosts (#30797) Change serialization version of doc-value fields. Add a `format` option to `docvalue_fields`. (#29639) [TEST] Don't expect acks when isolating nodes Fixes UpdateSettingsRequestStreamableTests mutate bug Revert "Add more yaml tests for get alias API (#29513)" Revert "Mutes MachineLearningTests.testNoAttributes_givenSameAndMlEnabled" Only allow x-pack metadata if all nodes are ready (#30743) Mutes MachineLearningTests.testNoAttributes_givenSameAndMlEnabled Use original settings on full-cluster restart (#30780) Only ack cluster state updates successfully applied on all nodes (#30672) Replace Request#setHeaders with addHeader (#30588) [TEST] remove endless wait in RestClientTests (#30776) QA: Add xpack tests to rolling upgrade (#30795) Add support for search templates to the high-level REST client. (#30473) Reduce CLI scripts to one-liners on Windows (#30772) Fold RestGetAllSettingsAction in RestGetSettingsAction (#30561) Add more yaml tests for get alias API (#29513) [Docs] Fix script-fields snippet execution (#30693) Convert FieldCapabilitiesResponse to a ToXContentObject. (#30182) Remove assert statements from field caps documentation. (#30601) Fix a bug in FieldCapabilitiesRequest#equals and hashCode. (#30181) Add support for field capabilities to the high-level REST client. (#29664) [DOCS] Add SAML configuration information (#30548) [DOCS] Remove X-Pack references from SQL CLI (#30694) [Docs] Fix typo in circuit breaker docs (#29659) [Feature] Adding a char_group tokenizer (#24186) Increase the maximum number of filters that may be in the cache. (#30655) [Docs] Fix broken cross link in documentation Test: wait for netty threads in a JUnit ClassRule (#30763) [Security] Include an empty json object in an json array when FLS filters out all fields (#30709) [DOCS] fixed incorrect default [TEST] Wait for CS to be fully applied in testDeleteCreateInOneBulk Enable installing plugins from snapshots.elastic.co (#30765) Ignore empty completion input (#30713) Fix docs failure on language analyzers (#30722) [Docs] Fix inconsistencies in snapshot/restore doc (#30480) Add Delete Repository High Level REST API (#30666) Reduce CLI scripts to one-liners (#30759)

jtibshirani added the WIP label May 9, 2018

colings86 added the :Core/Java High Level REST Client label May 9, 2018

colings86 added the >feature label May 9, 2018

jtibshirani force-pushed the feature/rest-search-template branch from c88fb82 to c9809d8 Compare May 9, 2018 18:16

javanna mentioned this pull request May 9, 2018

Java high-level REST client completeness #27205

Closed

80 tasks

jtibshirani force-pushed the feature/rest-search-template branch from c9809d8 to 597a8c8 Compare May 10, 2018 01:55

jtibshirani added v7.0.0 and removed WIP labels May 10, 2018

jtibshirani force-pushed the feature/rest-search-template branch from 597a8c8 to b75a875 Compare May 10, 2018 02:07

jtibshirani requested a review from javanna May 10, 2018 02:07

jtibshirani force-pushed the feature/rest-search-template branch from b75a875 to de86572 Compare May 10, 2018 04:19

jtibshirani commented May 10, 2018

View reviewed changes

javanna requested changes May 14, 2018

View reviewed changes

jtibshirani force-pushed the feature/rest-search-template branch from de86572 to 6ded113 Compare May 14, 2018 21:24

jtibshirani force-pushed the feature/rest-search-template branch 2 times, most recently from aa83a0e to ac9a0e3 Compare May 15, 2018 05:56

javanna reviewed May 15, 2018

View reviewed changes

javanna approved these changes May 15, 2018

View reviewed changes

jtibshirani added 7 commits May 15, 2018 08:51

Move request parsing logic into SearchTemplateRequest.

d59ccf0

Allow for SearchTemplateRequest to be serialized to XContent.

379c400

Allow for SearchTemplateResponse to be serialized from XContent.

f5a23f0

Implement RequestConverters#searchTemplate.

5bee4c3

Add support for search templates to the high-level REST client.

0003b8e

Add documentation.

ae648c3

Address code review feedback.

776ed02

jtibshirani force-pushed the feature/rest-search-template branch from ac9a0e3 to 776ed02 Compare May 15, 2018 16:42

jtibshirani merged commit 4f9dd37 into elastic:master May 15, 2018

jtibshirani deleted the feature/rest-search-template branch May 15, 2018 20:08

jtibshirani added the v6.4.0 label May 22, 2018

jtibshirani added a commit that referenced this pull request May 22, 2018

Add support for search templates to the high-level REST client. (#30473)

b8f2f62

(cherry picked from commit 4f9dd37)

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

Add support for search templates to the high-level REST client. #30473

Add support for search templates to the high-level REST client. #30473

jtibshirani commented May 9, 2018

elasticmachine commented May 9, 2018

jtibshirani May 10, 2018

javanna May 14, 2018

jtibshirani May 14, 2018

javanna left a comment

javanna May 14, 2018

jtibshirani May 14, 2018

javanna May 14, 2018

javanna May 14, 2018

javanna May 14, 2018

javanna May 14, 2018

jtibshirani May 14, 2018 •

edited

Loading

javanna May 14, 2018

jtibshirani May 14, 2018

javanna May 14, 2018

jtibshirani May 14, 2018

javanna May 14, 2018

jtibshirani May 14, 2018

javanna May 14, 2018

jtibshirani May 14, 2018

javanna May 14, 2018

jtibshirani May 14, 2018

jtibshirani commented May 14, 2018 •

edited

Loading

javanna May 15, 2018

jtibshirani May 15, 2018

javanna May 15, 2018

jtibshirani May 15, 2018

javanna May 15, 2018

jtibshirani May 15, 2018

javanna left a comment

fbaligand commented May 19, 2018

javanna commented May 22, 2018

jtibshirani commented May 22, 2018

fbaligand commented May 22, 2018


		===== Additional References

		The https://www.elastic.co/guide/en/elasticsearch/reference/current/search-template.html#search-template[Search Template HTTP documentation]

Add support for search templates to the high-level REST client. #30473

Add support for search templates to the high-level REST client. #30473

Conversation

jtibshirani commented May 9, 2018

elasticmachine commented May 9, 2018

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jtibshirani May 14, 2018 • 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

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

jtibshirani commented May 14, 2018 • 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

Choose a reason for hiding this comment

javanna left a comment

Choose a reason for hiding this comment

fbaligand commented May 19, 2018

javanna commented May 22, 2018

jtibshirani commented May 22, 2018

fbaligand commented May 22, 2018

jtibshirani May 14, 2018 •

edited

Loading

jtibshirani commented May 14, 2018 •

edited

Loading