Scripting: fill in get contexts REST API by stu-elastic · Pull Request #48319 · elastic/elasticsearch

stu-elastic · 2019-10-21T20:13:07Z

Updates response for GET /_script_context, returning a contexts
object with a list of context description objects. The description
includes the context name and a list of methods available. The
methods list has the signature for the execute mathod and any
getters. eg.

{
  "contexts": [
     {
       "name" : "moving-function",
       "methods" : [
         {
           "name" : "execute",
           "return_type" : "double",
           "params" : [
             {
               "type" : "java.util.Map",
               "name" : "params"
             },
             {
               "type" : "double[]",
               "name" : "values"
             }
           ]
         }
       ]
     },
     {
       "name" : "number_sort",
       "methods" : [
         {
           "name" : "execute",
           "return_type" : "double",
           "params" : [ ]
         },
         {
           "name" : "getDoc",
           "return_type" : "java.util.Map",
           "params" : [ ]
         },
         {
           "name" : "getParams",
           "return_type" : "java.util.Map",
           "params" : [ ]
         },
         {
           "name" : "get_score",
           "return_type" : "double",
           "params" : [ ]
         }
       ]
     },
...
  ]
}

fixes: #47411

Updates response for `GET /_script_context`, returning a `contexts` object with a list of context description objects. The description includes the context name and a list of methods available. The methods list has the signature for the `execute` mathod and any getters. eg. ``` { "contexts": [ { "name" : "moving-function", "methods" : [ { "name" : "execute", "return_type" : "double", "params" : [ { "type" : "java.util.Map", "name" : "params" }, { "type" : "double[]", "name" : "values" } ] } ] }, { "name" : "number_sort", "methods" : [ { "name" : "execute", "return_type" : "double", "params" : [ ] }, { "name" : "getDoc", "return_type" : "java.util.Map", "params" : [ ] }, { "name" : "getParams", "return_type" : "java.util.Map", "params" : [ ] }, { "name" : "get_score", "return_type" : "double", "params" : [ ] } ] }, ... ] } ``` fixes: elastic#47411

elasticmachine · 2019-10-21T20:13:08Z

Pinging @elastic/es-core-infra (:Core/Infra/Scripting)

…e/47411-painless-script-context-rest_2_exec_getters_merge

…different contexts

jdconrad

@stu-elastic Thank you for the work here. This looks good to me, and I don't have any other comments as we've been chatting throughout. I'm very pleased with the changes to the structure of the response.

...a/org/elasticsearch/action/admin/cluster/storedscripts/ScriptMethodInfoSerializingTests.java

rjernst

Thanks for getting this up so quickly, and especially for the extensive serialization tests. I have a few comments.

rjernst · 2019-10-22T23:36:30Z

rest-api-spec/src/main/resources/rest-api-spec/test/scripts/20_get_script_context.yml

-  - match: { contexts.number_sort: {} }
-  - match: { contexts.processor_conditional: {} }
-  - match: { contexts.score: {} }
-  - match: { contexts.script_heuristic: {} }


We seem to have lost a lot of builtin contexts here?

Unfortunately the change to list made performing the comparison after this differ in different contexts. This will be addressed when I address your next comment.

I've cut this test down to make it more reliable, now just checking that at least one context exists, it has a execute method with return value field.

rjernst · 2019-10-22T23:37:04Z

rest-api-spec/src/main/resources/rest-api-spec/test/scripts/20_get_script_context.yml

+  - match: { contexts.0.name: "aggregation_selector" }
+  - match: { contexts.0.methods: [{"name": "execute", "return_type": "boolean", "params": []}, {"name": "getParams", "return_type": "java.util.Map", "params": []}] }
+
+  - match: { contexts.1.name: "aggs" }


Why is this structure changed to a list of objects? The order of these do not matter, so I don't think it should be a list.

Agreed, I was going with ease of using our existing parsing primitives, which have a difficulty addressing dynamic keys. Will take a pass at making it a map and we'll see how ugly it gets.

It got pretty ugly. Per offline conversation, we're gonna keep this structure for now.

It got pretty ugly doing the fragment parsing and made testing difficult. Per offline conversation, we're gonna keep this structure for now.

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

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

rjernst · 2019-10-22T23:47:23Z

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

+
+import static org.elasticsearch.common.xcontent.ConstructingObjectParser.constructorArg;
+
+public class ScriptContextInfo implements ToXContentObject, Writeable {


I still think the naming is quite confusing here, but I can live with it for now since this is an internal api and thus these can be changed without backcompat.

Happy to change when we can think of a better name.

The reason I chose this name is ScriptContextInfo is this is a description of a ScriptContext and appending Info is the naming convention we have, see PainlessContextInfo.

...a/org/elasticsearch/action/admin/cluster/storedscripts/ScriptMethodInfoSerializingTests.java

…stantize

…_2_exec_getters_merge

rjernst

LGTM

* Scripting: fill in get contexts REST API Updates response for `GET /_script_context`, returning a `contexts` object with a list of context description objects. The description includes the context name and a list of methods available. The methods list has the signature for the `execute` mathod and any getters. eg. ``` { "contexts": [ { "name" : "moving-function", "methods" : [ { "name" : "execute", "return_type" : "double", "params" : [ { "type" : "java.util.Map", "name" : "params" }, { "type" : "double[]", "name" : "values" } ] } ] }, { "name" : "number_sort", "methods" : [ { "name" : "execute", "return_type" : "double", "params" : [ ] }, { "name" : "getDoc", "return_type" : "java.util.Map", "params" : [ ] }, { "name" : "getParams", "return_type" : "java.util.Map", "params" : [ ] }, { "name" : "get_score", "return_type" : "double", "params" : [ ] } ] }, ... ] } ``` fixes: elastic#47411

Updates response for `GET /_script_context`, returning a `contexts` object with a list of context description objects. The description includes the context name and a list of methods available. The methods list has the signature for the `execute` mathod and any getters. eg. ``` { "contexts": [ { "name" : "moving-function", "methods" : [ { "name" : "execute", "return_type" : "double", "params" : [ { "type" : "java.util.Map", "name" : "params" }, { "type" : "double[]", "name" : "values" } ] } ] }, { "name" : "number_sort", "methods" : [ { "name" : "execute", "return_type" : "double", "params" : [ ] }, { "name" : "getDoc", "return_type" : "java.util.Map", "params" : [ ] }, { "name" : "getParams", "return_type" : "java.util.Map", "params" : [ ] }, { "name" : "get_score", "return_type" : "double", "params" : [ ] } ] }, ... ] } ``` fixes: #47411

stu-elastic added >enhancement :Core/Infra/Scripting Scripting abstractions, Painless, and Mustache v8.0.0 v7.6.0 labels Oct 21, 2019

stu-elastic requested review from jdconrad and rjernst October 21, 2019 20:13

stu-elastic added 2 commits October 21, 2019 14:29

Merge branch 'master' of github.com:elastic/elasticsearch into featur…

e60aff8

…e/47411-painless-script-context-rest_2_exec_getters_merge

Remove integ test after aggs_reduce to handle different responses in …

5469882

…different contexts

jdconrad approved these changes Oct 22, 2019

View reviewed changes

...a/org/elasticsearch/action/admin/cluster/storedscripts/ScriptMethodInfoSerializingTests.java Show resolved Hide resolved

rjernst requested changes Oct 22, 2019

View reviewed changes

stu-elastic added 3 commits October 23, 2019 11:35

contexts Set -> HashMap, no alloc in hashCode, boolean rearrange, con…

2c4405a

…stantize

general matching

a16a0a7

Merge branch 'master' into feature/47411-painless-script-context-rest…

1f03919

…_2_exec_getters_merge

rjernst approved these changes Oct 28, 2019

View reviewed changes

stu-elastic merged commit 0f1b076 into elastic:master Oct 28, 2019

This was referenced Feb 3, 2020

[meta] 7.6 release elastic/elasticsearch-net#4340

Closed

[meta] 7.6 release elastic/elasticsearch-net#4341

Closed

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


		import static org.elasticsearch.common.xcontent.ConstructingObjectParser.constructorArg;

		public class ScriptContextInfo implements ToXContentObject, Writeable {

Conversation

stu-elastic commented Oct 21, 2019

Uh oh!

elasticmachine commented Oct 21, 2019

Uh oh!

jdconrad left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

rjernst 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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

rjernst left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants