Add get stored script and delete stored script to high level REST API by vladimirdolzhenko · Pull Request #31355 · elastic/elasticsearch

vladimirdolzhenko · 2018-06-15T09:41:26Z

Add get stored script and delete stored script to High Level REST API Client
Relates to #27205

Relates to elastic#27205

elasticmachine · 2018-06-15T09:41:28Z

Pinging @elastic/es-core-infra

javanna

left some comments, thanks @vladimirdolzhenko

javanna · 2018-06-15T12:28:03Z

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

    }

+    static Request getStoredScript(GetStoredScriptRequest getStoredScriptRequest) {
+        String endpoint = new EndpointBuilder().addPathPart("_scripts").addPathPart(getStoredScriptRequest.id()).build();


the first one should be addPathPartAsIs

javanna · 2018-06-15T12:28:15Z

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

+    }
+
+    static Request deleteStoredScript(DeleteStoredScriptRequest deleteStoredScriptRequest) {
+        String endpoint = new EndpointBuilder().addPathPart("_scripts").addPathPart(deleteStoredScriptRequest.id()).build();


here too, addPartAsIs for _scripts

javanna · 2018-06-15T12:29:13Z

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

+     * @return the response
+     * @throws IOException in case there is a problem sending the request or parsing back the response
+     */
+    public GetStoredScriptResponse getStoredScript(GetStoredScriptRequest request, RequestOptions options) throws IOException {


the SPEC have get_script, can we call these methods getScript ?

remove stored from all methods names?

javanna · 2018-06-15T12:29:29Z

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

+     * @return the response
+     * @throws IOException in case there is a problem sending the request or parsing back the response
+     */
+    public DeleteStoredScriptResponse deleteStoredScript(DeleteStoredScriptRequest request, RequestOptions options) throws IOException {


here too, remove the stored part

javanna · 2018-06-15T12:30:41Z

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

+
+        Request request = RequestConverters.getStoredScript(storedScriptRequest);
+        assertThat(request.getEndpoint(), equalTo("/_scripts/" + storedScriptRequest.id()));
+        assertThat(request.getParameters(), equalTo(expectedParams));


no need to create a new map right? you can just assert that parameters is empty.

javanna · 2018-06-15T12:33:22Z

docs/java-rest/high-level/supported-apis.asciidoc

 include::tasks/list_tasks.asciidoc[]
 include::tasks/cancel_tasks.asciidoc[]
+
+== Scripts


move them up with the top-level API? We try to align this with the API categories from the SPEC

looking again, I think that it's fine to have them grouped under scripts although the spec puts them in the top-level API. Things are a bit different in the es reference docs. Call it Scripts APIs though .

javanna · 2018-06-15T12:34:22Z

.../main/java/org/elasticsearch/action/admin/cluster/storedscripts/GetStoredScriptResponse.java

-        source.toXContent(builder, params);
+    public boolean isFragment() {
+        return true;
+    }


this method is not meant to be overridden. Please use ToXContentFragment instead of ToXContentObject if needed. Yet normally responses are objects rather than fragments, you sure about this?

javanna · 2018-06-15T12:37:35Z

server/src/main/java/org/elasticsearch/rest/action/admin/cluster/RestGetStoredScriptAction.java

-
-                    builder.endObject();
-                }
+                response.toXContent(builder, channel.request());


I think you can make the response a StatusToXContentObject and then use RestStatusToXContentListener here.

StatusToXContentObject starts to conflict with ToXContentFragment

and RestStatusToXContentListener does not work with fragment - RestGetStoredScriptAction as well adds id into response

As I said above, this probably should not be a fragment, I don't get why it would need to be.

there is a manual serialization performed in RestGetStoredScriptAction - it adds as well id of script in front of GetStoredScriptResponse - agreed to move id within GetStoredScriptResponse, make it StatusToXContentObject and drop fragment

javanna · 2018-06-15T12:41:16Z

server/src/main/java/org/elasticsearch/script/StoredScriptSource.java

-        builder.field(OPTIONS_PARSE_FIELD.getPreferredName(), options);
+        if (options.isEmpty() == false) {
+            builder.field(OPTIONS_PARSE_FIELD.getPreferredName(), options);
+        }


why do we need this change? it seems like it changes the existing StoredScriptSource output, that may be risky to do, not sure.

there is a custom serialization performed in RestGetStoredScriptAction - I use a standard StoredScriptSource.toXContent but it violates test rest-api-spec/test/painless/16_update2.yml - it expects that empty options are not shows at all. Deserialization of empty options or no-options are equal - resolves to empty map.

I traced call sites - seems no any other usages of StoredScriptSource.toXContent + other tests passes fine.

javanna · 2018-06-15T12:42:29Z

.../java/org/elasticsearch/action/admin/cluster/storedscripts/GetStoredScriptResponseTests.java

+
+    private StoredScriptSource scriptSource() {
+        return new StoredScriptSource("lang", "code",
+            Collections.singletonMap(Script.CONTENT_TYPE_OPTION, XContentType.JSON.mediaType()));


can we randomize how this instance gets created?

javanna · 2018-06-15T12:52:42Z

one more thing @vladimirdolzhenko the docs don't seem to build, can you check that please?

…i-spec; cleaned RestGetStoredScriptAction up and GetStoredScriptResponse.toXContent

javanna

left a few more comments, mostly minors.

javanna · 2018-06-18T08:57:41Z

...level/src/test/java/org/elasticsearch/client/documentation/StoredScriptsDocumentationIT.java

+        // end::delete-stored-script-execute
+
+        // tag::delete-stored-script-response
+        assertThat(deleteResponse.isAcknowledged(), equalTo(true)); // <1>


I would prefer that we don't show assertions in the docs output, but rather just assign values to variables, which is more aligned with what users will do in their applications.

javanna · 2018-06-18T08:58:16Z

...level/src/test/java/org/elasticsearch/client/documentation/StoredScriptsDocumentationIT.java

+            // end::get-stored-script-execute
+
+            // tag::get-stored-script-response
+            final StoredScriptSource source = getResponse.getSource(); // <1>


is it worth expanding on how to interact with StoredScriptSource?

javanna · 2018-06-18T08:58:53Z

docs/java-rest/high-level/delete_script.asciidoc

+["source","java",subs="attributes,callouts,macros"]
+--------------------------------------------------
+include-tagged::{doc-tests}/StoredScriptsDocumentationIT.java[delete-stored-script-execute]
+--------------------------------------------------


we are missing the optional parameters section here that describe the two supported timeouts.

javanna · 2018-06-18T08:59:33Z

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

+     * @return the response
+     * @throws IOException in case there is a problem sending the request or parsing back the response
+     */
+    public GetStoredScriptResponse getStoredScript(GetStoredScriptRequest request, RequestOptions options) throws IOException {


remove stored from all methods names?

javanna · 2018-06-18T09:01:58Z

docs/java-rest/high-level/supported-apis.asciidoc

 include::tasks/list_tasks.asciidoc[]
 include::tasks/cancel_tasks.asciidoc[]
+
+== Scripts


looking again, I think that it's fine to have them grouped under scripts although the spec puts them in the top-level API. Things are a bit different in the es reference docs. Call it Scripts APIs though .

javanna · 2018-06-18T09:04:23Z

libs/x-content/src/main/java/org/elasticsearch/common/xcontent/XContentBuilder.java

+    public XContentBuilder field(ParseField field, String value) throws IOException {
+        return field(field.getPreferredName(), value);
+    }
+


nit: I wonder what these method buy us, my concern is that we have already a lot of methods in XContentBuilder. After all callers only have to call the other variant of these methods and pass in field.getPreferredName(). I am a bit on the fence on this. Maybe let's try and do it separately and see what others think about it.

javanna · 2018-06-18T09:07:18Z

.../main/java/org/elasticsearch/action/admin/cluster/storedscripts/GetStoredScriptResponse.java

-        source.toXContent(builder, params);
+        builder.startObject();

+        builder.field(_ID_PARSE_FIELD, id);


we may have a small problem here, when toXContent is called on an object deserialized from a previous version that didn't send the _id .

nope, actually RestGetStoredScriptAction adds that _id manually (out of GetStoredScriptRequest.toXContent)

you added id as a member, which is great. It is now being serialized over the wire, but nodes on previous versions don't send it. If you are deserializing from a previous version this one will be null, and what will happen when you call toXcontent against such an object? I am not sure whether it will be NPE or the id will be printed out null, maybe just the latter, can you check?

agreed - use writeOptionalString instead of writeString (to avoid NPE on serialization) - while I don't see anything more how we can handle this case

if it prints out null it may be ok, if it gives NPE we need a null check, that's what I meant.

it prints out null :

public XContentBuilder field(String name, String value) throws IOException { if (value == null) { return nullField(name); } ensureNameNotNull(name); generator.writeStringField(name, value); return this; }

thanks for checking, that is fine then

javanna · 2018-06-18T09:08:57Z

.../main/java/org/elasticsearch/action/admin/cluster/storedscripts/GetStoredScriptResponse.java

    }

-    GetStoredScriptResponse(StoredScriptSource source) {
+    GetStoredScriptResponse(String id) {


maybe we can do without this constructor and always use the other that accepts both id and source? otherwise make this one private at least?

javanna · 2018-06-18T09:09:49Z

server/src/main/java/org/elasticsearch/script/StoredScriptSource.java

-        builder.field(OPTIONS_PARSE_FIELD.getPreferredName(), options);
+        if (options.isEmpty() == false) {
+            builder.field(OPTIONS_PARSE_FIELD.getPreferredName(), options);
+        }


javanna · 2018-06-18T09:12:51Z

.../java/org/elasticsearch/action/admin/cluster/storedscripts/GetStoredScriptResponseTests.java

+        return s -> "script.options".equals(s);
+    }
+
+    private StoredScriptSource scriptSource() {


static? and call it randomScriptSource?

…ng at REST level; code style adjustments

vladimirdolzhenko · 2018-06-19T05:59:50Z

thanks @javanna for your comments, could you pls have another look into this ?

javanna

thanks @vladimirdolzhenko I left a couple of things but LGTM, feel free to push once those are addressed.

javanna · 2018-06-19T08:17:09Z

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

        Request request = new Request(HttpDelete.METHOD_NAME, endpoint);
+        Params params = new Params(request);
+        params.withTimeout(deleteStoredScriptRequest.timeout());
+        params.withMasterTimeout(deleteStoredScriptRequest.masterNodeTimeout());


also set them to some value?

javanna · 2018-06-19T08:18:39Z

.../main/java/org/elasticsearch/action/admin/cluster/storedscripts/GetStoredScriptResponse.java

                boolean found = (Boolean)a[1];
                StoredScriptSource scriptSource = (StoredScriptSource)a[2];
-                return found ? new GetStoredScriptResponse(id, scriptSource) : new GetStoredScriptResponse(id);
+                return found ? new GetStoredScriptResponse(id, scriptSource) : new GetStoredScriptResponse();


I thought that you wanted to set the id in both cases and that the else block should become new GetStoredScriptResponse(id, null)

javanna · 2018-06-19T08:22:01Z

.../main/java/org/elasticsearch/action/admin/cluster/storedscripts/GetStoredScriptResponse.java

        }
        if (out.getVersion().onOrAfter(Version.V_7_0_0_alpha1)) {
-            out.writeString(id);
+            out.writeOptionalString(id);


You already have a conditional based on the version, if a version supports it, it will always send it. Making it optional helps in case something that is deserialized from a previous version is sent back to another node. I am not sure we ever do those multiple roundtrips with this API, hence I don't think we need to make it optional. If this is due to my previous comment on id being potentially null, I didn't mean to make you make this change. I rather meant to protect toXcontent from potential NPEs if they may happen.

javanna · 2018-06-19T08:22:37Z

server/src/main/java/org/elasticsearch/rest/action/admin/cluster/RestGetStoredScriptAction.java

        String id = request.param("id");
        GetStoredScriptRequest getRequest = new GetStoredScriptRequest(id);
-
+        getRequest.masterNodeTimeout(request.paramAsTime("master_timeout", getRequest.masterNodeTimeout()));


can you also add this to the SPEC if it's supported?

javanna · 2018-06-19T08:24:32Z

server/src/test/java/org/elasticsearch/common/xcontent/BaseXContentTestCase.java

    public void testField() throws IOException {
        expectValueException(() -> BytesReference.bytes(builder().field("foo")));
-        expectNonNullFieldException(() -> BytesReference.bytes(builder().field(null)));
+        expectNonNullFieldException(() -> BytesReference.bytes(builder().field((String)null)));


this should not be needed anymore

vladimirdolzhenko · 2018-06-19T08:34:05Z

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

+        DeleteStoredScriptRequest deleteStoredScriptRequest = new DeleteStoredScriptRequest("x-script");
+
+        Map<String, String> expectedParams = new HashMap<>();
+        setRandomTimeout(deleteStoredScriptRequest::timeout, AcknowledgedRequest.DEFAULT_ACK_TIMEOUT, expectedParams);


@javanna I guess you mean some value like this

* master: Add get stored script and delete stored script to high level REST API - post backport fix Add get stored script and delete stored script to high level REST API (#31355) Core: Combine Action and GenericAction (#31405) Fix reference to XContentBuilder.string() (#31337) Avoid sending duplicate remote failed shard requests (#31313) Fix defaults in GeoShapeFieldMapper output (#31302) RestAPI: Reject forcemerge requests with a body (#30792) Packaging: Remove windows bin files from the tar distribution (#30596) Docs: Use the default distribution to test docs (#31251) [DOCS] Adds testing for security APIs (#31345) Clarify that IP range data can be specified in CIDR notation. (#31374) Use system context for cluster state update tasks (#31241) Percentile/Ranks should return null instead of NaN when empty (#30460) REST high-level client: add validate query API (#31077) Move language analyzers from server to analysis-common module. (#31300) [Test] Fix :example-plugins:rest-handler on Windows Expose lucene's RemoveDuplicatesTokenFilter (#31275) Reload secure settings for plugins (#31383) Remove some cases in FieldTypeLookupTests that are no longer relevant. (#31381) Ensure we don't use a remote profile if cluster name matches (#31331) [TEST] Double write alias fault (#30942) [DOCS] Fix version in SQL JDBC Maven template [DOCS] Improve install and setup section for SQL JDBC SQL: Fix rest endpoint names in node stats (#31371) Support for remote path in reindex api - post backport fix Closes #22913 [ML] Put ML filter API response should contain the filter (#31362) Support for remote path in reindex api (#31290) Add byte array pooling to nio http transport (#31349) Remove trial status info from start trial doc (#31365) [DOCS] Adds links to release notes and highlights add is-write-index flag to aliases (#30942) Add rollover-creation-date setting to rolled over index (#31144) [ML] Hold ML filter items in sorted set (#31338) [Tests] Fix edge case in ScriptedMetricAggregatorTests (#31357)

get and delete stored script high level REST API

fae58be

Relates to elastic#27205

vladimirdolzhenko added >enhancement v7.0.0 :Core/Java High Level REST Client labels Jun 15, 2018

vladimirdolzhenko requested a review from javanna June 15, 2018 09:41

vladimirdolzhenko mentioned this pull request Jun 15, 2018

Add put stored script support to high-level rest client #31323

Merged

vladimirdolzhenko changed the title ~~get and delete stored script high level REST API~~ Add get stored script and delete stored script to high level REST API Jun 15, 2018

javanna mentioned this pull request Jun 15, 2018

Java high-level REST client completeness #27205

Closed

80 tasks

javanna suggested changes Jun 15, 2018

View reviewed changes

fixed broken tags in docs; REST api method names aligned with rest-ap…

e8ded37

…i-spec; cleaned RestGetStoredScriptAction up and GetStoredScriptResponse.toXContent

vladimirdolzhenko requested a review from javanna June 16, 2018 06:12

javanna suggested changes Jun 18, 2018

View reviewed changes

Vladimir Dolzhenko added 3 commits June 18, 2018 18:32

added missed timeout doc sections; added missed master-timeout handli…

48af52f

…ng at REST level; code style adjustments

fixed broken test due to handling timeouts

00f3689

fixed broken test due to handling timeouts

8c34d9e

vladimirdolzhenko requested a review from javanna June 19, 2018 05:59

javanna approved these changes Jun 19, 2018

View reviewed changes

vladimirdolzhenko commented Jun 19, 2018

View reviewed changes

couple more touches - update get_script rest api spec; minor clean ups

3dadad8

vladimirdolzhenko added the v6.4.0 label Jun 19, 2018

vladimirdolzhenko merged commit 04e4e44 into elastic:master Jun 19, 2018

vladimirdolzhenko deleted the get_delete_stored_script branch June 19, 2018 12:21

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

Conversation

vladimirdolzhenko commented Jun 15, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

elasticmachine commented Jun 15, 2018

Uh oh!

javanna left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

javanna commented Jun 15, 2018

Uh oh!

javanna left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vladimirdolzhenko commented Jun 15, 2018 •

edited

Loading