Start on custom whitelists for Painless #23563

nik9000 · 2017-03-13T06:28:12Z

We'd like to be able to support context-sensitive whitelists in
Painless but we can't now because the whitelist is a static thing.
This begins to de-static the whitelist, in particular removing
the static keyword from Definition#getRuntimeClass and plumbing
the static instance into the appropriate spots as though it weren't
static. Once we de-static all the methods we should be able to
fairly simply build context-sensitive whitelists.

The only "fun" bit of this is that I added another layer in the
chain of methods that bootstraps def calls. Instead of running
invokedynamic directly on DefBootstrap we now invokedynamic
$bootstrapDef on the script itself loads the Definition that
the script was compiled against and then calls DefBootstrap.

We'd like to be able to support context-sensitive whitelists in Painless but we can't now because the whitelist is a static thing. This begins to de-static the whitelist, in particular removing the static keyword from `Definition#getRuntimeClass` and plumbing the static instance into the appropriate spots as though it weren't static. Once we de-static all the methods we should be able to fairly simply build context-sensitive whitelists. The only "fun" bit of this is that I added another layer in the chain of methods that bootstraps `def` calls. Instead of running `invokedynamic` directly on `DefBootstrap` we now `invokedynamic` `$bootstrapDef` on the script itself loads the `Definition` that the script was compiled against and then calls `DefBootstrap`.

nik9000 · 2017-03-13T06:29:22Z

modules/lang-painless/src/test/java/org/elasticsearch/painless/DefBootstrapTests.java

@@ -30,14 +30,17 @@
 import org.elasticsearch.test.ESTestCase;

 public class DefBootstrapTests extends ESTestCase {
+    private final Definition definition = Definition.INSTANCE;


I did this in the tests instead of using Definition.INSTANCE everywhere because I think we'll remove Definition.INSTANCE when we're done making context sensitive whitelists.

nik9000 · 2017-03-13T06:31:50Z

@jdconrad I mentioned this to you earlier today though I wasn't particularly clear. I figured this was one of those times when it is easier to look at code then try to describe it. What do you think about this as a first step towards context sensitive whitelists?

nik9000 · 2017-03-20T14:26:41Z

@jdconrad, what do you think of this as an approach? The next step would be to un-static the rest of Definition. The step after would be to have a look at custom definitions.

jdconrad · 2017-03-20T15:56:13Z

@nik9000 Sorry, meant to get to this last week. I see what you mean now when you mentioned all the additional plumbing going everywhere which is a bummer, but I don't see a good way around it either. No matter what happens the information for the 'binding' will have to be passed in somewhere, so it may as well be the Definition that specifies this. So far, this does indeed seem like the correct approach. I'll be curious to see how it turns once more is completed.

nik9000 · 2017-03-20T16:06:48Z

'll be curious to see how it turns once more is completed.

Do you want me to push merge this as is or de-static all the stuff and then review? I like doing this partially but if you're not sure if this shows the way to go well enough then I will keep going.

nik9000 · 2017-03-20T16:07:11Z

Also, don't worry about reviews taking time. It is all good!

jdconrad · 2017-03-20T16:46:13Z

modules/lang-painless/src/main/java/org/elasticsearch/painless/node/SSource.java

@@ -216,6 +220,19 @@ public void write() {
        visitor.visit(WriterConstants.CLASS_VERSION, classAccess, className, null, classBase, classInterfaces);
        visitor.visitSource(Location.computeSourceName(name, source), null);

+        // Write the a method to bootstrap def calls


Does this have to be in bytecode? Why not make this whole method part of the PainlessScript?

Edit: Is this to avoid passing the Definition around? Because I think it'll need to be passed into the nodes in some way for compile-time method/field look ups?

I do it this way because def is always going to need getRuntimeClass (or something like it) to be specific for each generated class. So each generated class needs its own static reference to the appropriate Definition. Lifting the method to PainlessScript would require resolving the Definition with reflection or a method reference or something.

I could also play around with making the bootstrap method non-static. If I did that I could indeed move this to PainlessScript. I didn't think that'd work when I was working on it the first time but now I think it might. Do you think I should have a look at that?

Doesn't look like it can be a non-static method given the order of the required arguments, and the reason for this method is explained below. Thanks.

jdconrad · 2017-03-20T16:51:28Z

I'd like to see a bit more please. It's so much harder to ask for changes if part of the code is already in.

nik9000 · 2017-03-20T16:59:49Z

I'd like to see a bit more please. It's so much harder to ask for changes if part of the code is already in.

🤘

jdconrad · 2017-03-20T17:50:38Z

After looking more closely at the DefBootstrap method modifications, I'm a bit surprised this works based on the spec for invokedynamic (https://docs.oracle.com/javase/specs/jvms/se8/html/jvms-6.html#jvms-6.5.invokedynamic). Sure seems like it would be illegal to have Definition as the first argument of the bootstrap method (possibly as an argument at all since it's not a constant).

nik9000 · 2017-03-20T17:53:04Z

Sure seems like it would be illegal to have Definition as the first argument of the bootstrap method.

I'll have a deeper look soon.

nik9000 · 2017-03-20T17:54:11Z

I'll have a deeper look soon.

Oh! That works because we no longer call DefBootstrap directly - we call the method in the generated script which has the old signature.

jdconrad · 2017-03-20T18:45:05Z

I understand. Thanks for the explanation.

nik9000 · 2017-03-20T18:53:31Z

Removing review as I'll be doing more work on this before the next round of review.

I chose to put `Definition` into `Locals` so I didn't have to change the signature of all the `analyze` methods. I could have do it another way, but that seems ok for now.

nik9000 · 2017-03-23T15:05:02Z

@jdconrad I de-staticed the remaining methods in Definition.

I chose to pass Definition through Locals. It was either that or changing the arguments to the analyze method. And Locals needed a Definition sometimes.

Do you think it is OK to stop here and do the actual custom Definition implementation in a followup?

jdconrad · 2017-03-23T16:48:26Z

@nik9000 I kind of figured that Definition would end up as part of Locals since it's the least disruptive way to get the necessary plumbing in there. Seems like a reasonable stopping place, and I'll try to take a good look at this review tomorrow (I have test triage today).

nik9000 · 2017-03-23T17:12:21Z

I'll try to take a good look at this review tomorrow

Thanks!

I have test triage today

Enjoy!

jdconrad · 2017-04-11T17:31:57Z

@nik9000 Sorry for the delay here. I know I still owe you a review -- haven't forgotten, will definitely happen this week. I got caught up in other issues.

jdconrad

LGTM.

nik9000 · 2017-04-14T17:12:46Z

Thanks for the review @jdconrad! I'll try and merge next week. I have a ton of test failures waiting for me when I get back on Monday. At this point I'd be surprised if I do anything other than debug those failures for a few days. That reindex one is sneaky!

…

On Fri, Apr 14, 2017, 12:53 PM Jack Conradson ***@***.***> wrote: ***@***.**** approved this pull request. LGTM. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#23563 (review)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AANLosHKutRJOyEOPV14MP_WZOgxZzh8ks5rv6RvgaJpZM4Ma47n> .

nik9000 · 2017-04-18T14:43:24Z

Thanks again for reviewing @jdconrad! I've merged to master and I'll cherry-pick to 5.x. I expect this is going to have trouble with merging with #24070. Sorry!

We'd like to be able to support context-sensitive whitelists in Painless but we can't now because the whitelist is a static thing. This begins to de-static the whitelist, in particular removing the static keyword from most of the methods on `Definition` and plumbing the static instance into the appropriate spots as though it weren't static. Once we de-static all the methods we should be able to fairly simply build context-sensitive whitelists. The only "fun" bit of this is that I added another layer in the chain of methods that bootstraps `def` calls. Instead of running `invokedynamic` directly on `DefBootstrap` we now `invokedynamic` `$bootstrapDef` on the script itself loads the `Definition` that the script was compiled against and then calls `DefBootstrap`. I chose to put `Definition` into `Locals` so I didn't have to change the signature of all the `analyze` methods. I could have do it another way, but that seems ok for now.

nik9000 · 2017-04-18T16:12:14Z

And I've backported to 5.x. Thanks again @jdconrad. I'll go and review #24070 some more now. I don't know that I'll be able to do all the requisite reading today though.

nik9000 added 2 commits March 12, 2017 23:25

Move constant

12c175d

nik9000 added :Plugin Lang Painless >enhancement v5.4.0 v6.0.0-alpha1 labels Mar 13, 2017

nik9000 requested a review from jdconrad March 13, 2017 06:28

nik9000 changed the title ~~Painless: Start on custom whitelists~~ Start on custom whitelists for Painless Mar 13, 2017

nik9000 commented Mar 13, 2017

View reviewed changes

nik9000 added the review label Mar 13, 2017

jdconrad reviewed Mar 20, 2017

View reviewed changes

nik9000 removed the review label Mar 20, 2017

nik9000 added 6 commits March 23, 2017 09:57

de-static isSimletype

f28fa03

Merge branch 'master' into painless_custom_whitelist_1

dcd7bd4

Un-static getType

1746e41

I chose to put `Definition` into `Locals` so I didn't have to change the signature of all the `analyze` methods. I could have do it another way, but that seems ok for now.

Move definition to core

0d889f5

Destatic the other getType

161a62c

Rename Definition.INSTANCE to Defintion.BUILTINS

7e98b24

Cleanup javadoc

200ba16

nik9000 added the review label Mar 23, 2017

jdconrad approved these changes Apr 14, 2017

View reviewed changes

nik9000 merged commit 0b15fde into elastic:master Apr 18, 2017

clintongormley added :Core/Infra/Scripting Scripting abstractions, Painless, and Mustache and removed :Plugin Lang Painless labels Feb 14, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Start on custom whitelists for Painless #23563

Start on custom whitelists for Painless #23563

nik9000 commented Mar 13, 2017

nik9000 Mar 13, 2017

nik9000 commented Mar 13, 2017

nik9000 commented Mar 20, 2017

jdconrad commented Mar 20, 2017

nik9000 commented Mar 20, 2017

nik9000 commented Mar 20, 2017

jdconrad Mar 20, 2017 •

edited

Loading

nik9000 Mar 20, 2017

jdconrad Mar 20, 2017

jdconrad commented Mar 20, 2017

nik9000 commented Mar 20, 2017

jdconrad commented Mar 20, 2017 •

edited

Loading

nik9000 commented Mar 20, 2017

nik9000 commented Mar 20, 2017

jdconrad commented Mar 20, 2017

nik9000 commented Mar 20, 2017

nik9000 commented Mar 23, 2017

jdconrad commented Mar 23, 2017

nik9000 commented Mar 23, 2017

jdconrad commented Apr 11, 2017

jdconrad left a comment

nik9000 commented Apr 14, 2017 via email

nik9000 commented Apr 18, 2017

nik9000 commented Apr 18, 2017

Start on custom whitelists for Painless #23563

Start on custom whitelists for Painless #23563

Conversation

nik9000 commented Mar 13, 2017

nik9000 Mar 13, 2017

Choose a reason for hiding this comment

nik9000 commented Mar 13, 2017

nik9000 commented Mar 20, 2017

jdconrad commented Mar 20, 2017

nik9000 commented Mar 20, 2017

nik9000 commented Mar 20, 2017

jdconrad Mar 20, 2017 • edited Loading

Choose a reason for hiding this comment

nik9000 Mar 20, 2017

Choose a reason for hiding this comment

jdconrad Mar 20, 2017

Choose a reason for hiding this comment

jdconrad commented Mar 20, 2017

nik9000 commented Mar 20, 2017

jdconrad commented Mar 20, 2017 • edited Loading

nik9000 commented Mar 20, 2017

nik9000 commented Mar 20, 2017

jdconrad commented Mar 20, 2017

nik9000 commented Mar 20, 2017

nik9000 commented Mar 23, 2017

jdconrad commented Mar 23, 2017

nik9000 commented Mar 23, 2017

jdconrad commented Apr 11, 2017

jdconrad left a comment

Choose a reason for hiding this comment

nik9000 commented Apr 14, 2017 via email

nik9000 commented Apr 18, 2017

nik9000 commented Apr 18, 2017

jdconrad Mar 20, 2017 •

edited

Loading

jdconrad commented Mar 20, 2017 •

edited

Loading