Add a Painless Context REST API #39382

jdconrad · 2019-02-25T23:29:44Z

This PR adds an internal REST API for querying context information about Painless whitelists.

Commands include the following:
GET /_scripts/painless/_context -- retrieves a list of contexts
GET /_scripts/painless/_context?context=%name% retrieves all available information about the API for this specific context

In the future it should be easy to expand the API for external consumption or use with a web based IDE for retrieving info about a single type/single method, etc.

The follow up for this is to add a gradle task that can process these commands and generate whitelist API docs to keep them up-to-date.

elasticmachine · 2019-02-25T23:29:46Z

Pinging @elastic/es-core-infra

martijnvg

The direction looks good. I left some comments about serialization.

martijnvg · 2019-02-26T14:11:36Z

modules/lang-painless/src/main/java/org/elasticsearch/painless/PainlessContextAction.java

+
+    public static class Request extends ActionRequest {
+
+        private String scriptContextName;


This fields needs to be serialized. This can be done by adding a constructor that accepts StreamInput and overwrite the writeTo method.

martijnvg · 2019-02-26T14:20:15Z

modules/lang-painless/src/main/java/org/elasticsearch/painless/PainlessContextAction.java

+        }
+
+        public Response(StreamInput input) {
+            throw new UnsupportedOperationException();


We should have binary serialization here too. (implement this constructor and implement writeTo() method)

martijnvg · 2019-02-26T14:31:29Z

modules/lang-painless/src/main/java/org/elasticsearch/painless/PainlessContextAction.java

+
+    public static class Response extends ActionResponse implements ToXContentObject {
+
+        private final PainlessScriptEngine painlessScriptEngine;


I think that PainlessScriptEngine should't be exposed in this response class. This something internal to painless itself. I think it makes more sense to introduce a simple pojo classes that are exposed here and that can serialize the information that users of this api need. The transport action can then built the response class with the pojo classes based on the painless script engine instance.

I considered this and wasn't sure it was the right thing to do since so much information is being copied from the PainlessLookup. But based on your feedback it makes sense for the serialization and encapsulation, so I'll work on changing it. Thanks for this feedback!

Added several POJOs to accommodate this.

jdconrad · 2019-02-26T16:02:02Z

@martijnvg Thank you for the comments! Will take a look soon.

jdconrad · 2019-03-05T18:12:59Z

@martijnvg I think I've addressed all the serialization issues. Would you please take another look when you get the chance? Thanks in advance!

jdconrad · 2019-03-08T17:04:26Z

@elasticmachine run elasticsearch-ci/1

jdconrad · 2019-03-11T19:26:56Z

@martijnvg I added comprehensive serialization tests. I also added a pair of yaml tests that execute against the two queries, but given the nature of the responses it was unrealistic to add checks against them at this time since the contains assertion will not work against lists.
Edit: Ready for re-review when you have the chance. Thanks!

martijnvg

I also added a pair of yaml tests that execute against the two queries, but given the nature of the responses it was unrealistic to add checks against them at this time since the contains assertion will not work against lists.

See my comment in PainlessContextInfo constructor, maybe we can assert the response.

martijnvg · 2019-03-12T09:04:48Z

...les/lang-painless/src/main/java/org/elasticsearch/painless/action/PainlessContextAction.java

+
+    public static class TransportAction extends HandledTransportAction<Request, Response> {
+
+        PainlessScriptEngine painlessScriptEngine;


maybe make this field final?

martijnvg · 2019-03-12T09:06:09Z

modules/lang-painless/src/main/java/org/elasticsearch/painless/action/PainlessContextInfo.java

+    public static final ParseField NAME = new ParseField("name");
+    public static final ParseField CLASSES = new ParseField("classes");
+    public static final ParseField IMPORTED_METHODS = new ParseField("imported_methods");
+    public static final ParseField ClASS_BINDINGS = new ParseField("class_bindings");


should ClASS_BINDINGS be CLASS_BINDINGS

Man, your eyes are way better than mine. Fixed.

martijnvg · 2019-03-12T13:22:14Z

modules/lang-painless/src/main/java/org/elasticsearch/painless/action/PainlessContextInfo.java

+                painlessLookup.getClasses().stream().map(
+                        javaClass -> new PainlessContextClassInfo(javaClass, painlessLookup.lookupPainlessClass(javaClass))
+                ).collect(Collectors.toList()),
+                painlessLookup.getImportedPainlessMethodsKeys().stream().map(importedPainlessMethodKey -> {


maybe add .sort(custom comparator) to the chaining here in order to generate consistent response?
This will then also allow to the test the response in the yaml test?

Sorted everything that made sense to sort.

response object.

jdconrad · 2019-03-12T18:57:47Z

@martijnvg So I changed the following:

Added a new field to PainlessContextClassInfo called imported that says if a class can be accessed through it's imported name (java.lang.Object -> Object)
Simplified the response object for the action to have data structures more in line with what's actually being returned and sort the contexts upon return for easier testing.
Sorted constructors, methods, and fields with PainlessContextClassInfo. Sorted the classes within PainlessContextInfo.
Added numerous matches against the yaml tests (thanks for the tip about lists again :) )

Ready for the next round of review.

martijnvg

LGTM - Thanks @jdconrad this looks great.
I only left a minor comment.

martijnvg · 2019-03-13T07:31:19Z

...les/lang-painless/src/main/java/org/elasticsearch/painless/action/PainlessContextAction.java

+        public Response(StreamInput in) throws IOException {
+            super(in);
+            scriptContextNames = in.readBoolean() ? in.readStringList() : null;
+            painlessContextInfo = in.readBoolean() ? new PainlessContextInfo(in) : null;


instead of the ternary expressions maybe use: readOptionalWriteable(...) and just use an empty list for scriptContextNames (if there isn't anything)?

Thank you for readOptionalWritable! That's what I was looking for, but failed to find it.

martijnvg · 2019-03-13T07:32:16Z

...les/lang-painless/src/main/java/org/elasticsearch/painless/action/PainlessContextAction.java

+                out.writeStringCollection(scriptContextNames);
+            }
+
+            if (painlessContextInfo == null) {


same as the previous comment, maybe use writeOptionalWriteable(...) and empty list for scriptContextNames ?

Same as above.

jdconrad · 2019-03-13T17:12:24Z

@martijnvg Thank you for all the iterations of review! Will commit as soon as CI passes.

martijnvg · 2019-03-14T08:18:32Z

@jdconrad Looks great!

This PR adds an internal REST API for querying context information about Painless whitelists. Commands include the following: GET /_scripts/painless/_context -- retrieves a list of contexts GET /_scripts/painless/_context?context=%name% retrieves all available information about the API for this specific context

jdconrad added 11 commits January 17, 2019 14:33

Add skeleton for context action.

7b3aacb

Store PainlessScriptEngine.t

759eb4e

Merge branch 'master' into docapi

1c1edba

Add methods to access lookups for doc api.

cf6b680

Merge branch 'master' into docapi

613cfe6

Merge branch 'master' into docapi

0c48a73

Convert to use HandledRestAction

1ae7533

Progress on API for returning methods

28b7cde

Revent prettyprint

eafa7b9

Merge branch 'master' into docapi

fed13ea

Add a Painless Context REST API

eab2c38

jdconrad added >enhancement :Core/Infra/Scripting Scripting abstractions, Painless, and Mustache v8.0.0 v7.2.0 labels Feb 25, 2019

jdconrad requested review from martijnvg and rjernst February 25, 2019 23:33

Fix style

13b3133

martijnvg reviewed Feb 26, 2019

View reviewed changes

jdconrad added 7 commits February 26, 2019 13:56

Merge branch 'master' into docapi

f519aba

Merge branch 'master' into docapi

f1f4e5d

Move to action package.

9afc790

Merge branch 'master' into docapi

1f81c2c

Merge branch 'master' into docapi

e4644d4

Add POJOs to handle serialization and response XContent.

2e40711

Update serialization.

8ad179f

Merge branch 'master' into docapi

46f7504

jdconrad added 4 commits March 7, 2019 10:20

Merge branch 'master' into docapi

c54fe76

Add serialization tests.

a74a4dd

Fix generics.

c0effbe

Merge branch 'master' into docapi

dd43131

jdconrad added 3 commits March 11, 2019 11:15

Merge branch 'master' into docapi

7f3d4a1

Add yaml tests.

7f218eb

Add rest spec

a651b4b

Fix naming convention test.

f9ac3b1

martijnvg reviewed Mar 12, 2019

View reviewed changes

jdconrad added 2 commits March 12, 2019 08:35

Merge branch 'master' into docapi

3b06a61

Add imported info to class, add matches to yaml tests, and update

79b1221

response object.

jdconrad added 2 commits March 12, 2019 12:01

Fix ignored imports in ANTLR files.

2b22fb3

Merge branch 'master' into docapi

17a3efc

martijnvg approved these changes Mar 13, 2019

View reviewed changes

jdconrad added 2 commits March 13, 2019 09:39

Merge branch 'master' into docapi

3a88b15

Response to PR comments.

5e97670

jdconrad removed the request for review from rjernst March 13, 2019 17:13

jdconrad merged commit d803676 into elastic:master Mar 14, 2019

jdconrad added the backport pending label Mar 14, 2019

jdconrad removed the backport pending label Mar 14, 2019

karmi mentioned this pull request Apr 7, 2019

Introduce stability description to the REST API specification #38413

Merged

codebrain mentioned this pull request Aug 5, 2019

[meta] 7.2 Release elastic/elasticsearch-net#3980

Closed

37 tasks

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 a Painless Context REST API #39382

Add a Painless Context REST API #39382

jdconrad commented Feb 25, 2019

elasticmachine commented Feb 25, 2019

martijnvg left a comment

martijnvg Feb 26, 2019

jdconrad Mar 5, 2019

martijnvg Feb 26, 2019

jdconrad Mar 5, 2019

martijnvg Feb 26, 2019

jdconrad Feb 26, 2019

jdconrad Mar 5, 2019

jdconrad commented Feb 26, 2019

jdconrad commented Mar 5, 2019

jdconrad commented Mar 8, 2019

jdconrad commented Mar 11, 2019 •

edited

Loading

martijnvg left a comment

martijnvg Mar 12, 2019

jdconrad Mar 12, 2019

martijnvg Mar 12, 2019

jdconrad Mar 12, 2019

martijnvg Mar 12, 2019

jdconrad Mar 12, 2019

jdconrad commented Mar 12, 2019

martijnvg left a comment

martijnvg Mar 13, 2019

jdconrad Mar 13, 2019

martijnvg Mar 13, 2019

jdconrad Mar 13, 2019

jdconrad commented Mar 13, 2019 •

edited

Loading

martijnvg commented Mar 14, 2019


		public static class Request extends ActionRequest {

		private String scriptContextName;


		public static class Response extends ActionResponse implements ToXContentObject {

		private final PainlessScriptEngine painlessScriptEngine;


		public static class TransportAction extends HandledTransportAction<Request, Response> {

		PainlessScriptEngine painlessScriptEngine;

Add a Painless Context REST API #39382

Add a Painless Context REST API #39382

Conversation

jdconrad commented Feb 25, 2019

elasticmachine commented Feb 25, 2019

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

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

jdconrad commented Feb 26, 2019

jdconrad commented Mar 5, 2019

jdconrad commented Mar 8, 2019

jdconrad commented Mar 11, 2019 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jdconrad commented Mar 12, 2019

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

Choose a reason for hiding this comment

jdconrad commented Mar 13, 2019 • edited Loading

martijnvg commented Mar 14, 2019

jdconrad commented Mar 11, 2019 •

edited

Loading

jdconrad commented Mar 13, 2019 •

edited

Loading