Painless execute api

[martijnvg]

Adds an api that allows to execute an arbitrary script and a result to be returned.

POST /_scripts/painless/_execute
{
  "script": {
    "source": "params.var1 / params.var2",
    "params": {
      "var1": 1,
      "var2": 1
    }
  }
}

[elasticmachine]

Pinging @elastic/es-core-infra

[jasontedor]

@jdconrad Could you review this PR?

[jdconrad]

@martijnvg Thank you so much for a first pass at this! My apologies for taking so long to get to the review. I have some comments mostly related to the documentation, but a few technical points to. I would also like @rjernst to take a look at this as well as I'm hopeful he can better explain the design decisions around the context code.

[jdconrad]

Please reword this. Maybe something like "allows an arbitrary script to be executed and a result to be returned."

[jdconrad]

Should this follow something more closely to the stored script format?

"script": {
    "code":  "<code>",
    "context": "<context>"
}

[martijnvg]

I think that makes sense.

[martijnvg]

After looking at this again. Changing to format to what you suggest does make the request a bit more verbose. Example:

{
  "script": {
    "code": {
      "source": "params.count / params.total",
      "params": {
        "count": 100.0,
        "total": 1000.0
      }
    },
    "context": ...
  }
}

Compared to:

POST /_scripts/painless/_execute
{
  "script": {
    "source": "params.count / params.total",
    "params": {
      "count": 100.0,
      "total": 1000.0
    }
  },
  "context": ...
}

I don't think this adds much? And context is something specific to this api.

[jdconrad]

My apologies, I don't think I made my request clear. I thought the format was different, what I meant was change "source" to be "code" like we've done for stored scripts, but it looks like we never did that for Script, so it's confusing. Let's just roll with what you had.

[jdconrad]

Please replace "get" with "are"

[jdconrad]

Also consider rewording variables into something like parameters? I realize that's confusing too as params is often used in scripts. Variables seems to imply what can be locally created as well when really we just want to say what external data is allowed to be used.

[jdconrad]

Variables -> Parameters? It's like a hidden method call.

[jdconrad]

"script parameters" -> "user defined values" ?

[jdconrad]

Should there be a way to possibly return specified parameters as well so they the user can see side-effects? Maybe a follow up.

[martijnvg]

Not sure, whether returning the specified variables makes sense.

[jdconrad]

Yeah it probably doesn't make sense here. I guess I was thinking along the lines of how things get passed around in related to aggs, but that can be simulated anyway.

[jdconrad]

As commented on before in the documentation, consider changing this field to "code"

[jdconrad]

Does this rest path potentially limit api additions/changes moving forward given that there is no description of what action is being taken as part of the path? Something like admin/scripts/lang/painless/action/execute may be significantly more flexible.

[martijnvg]

So the rest paths are in the RestAction inner class (line 319). This NAME is kind of an unique value that is used to register this action. The reason it is name spaced, is for security, mainly for authorization reasons. I'm ok with the name change you propose here (but it needs to be prefixed with cluster:).

[jdconrad]

I assume this comment was added to remember the question and should be removed before committing. @rjernst can likely explain the necessity of the design decisions here better than I can, so I'm hopeful he will chime in :)

[rjernst]

The factory is what holds onto the params, so that they can be passed to the constructor. Think of the factory as the signature for the constructor. It's not boilerplate; it is actually needed based on current uses of scripts throughout the system. Also note that the factory signature is what allows the script instance to have arbitrary objects passed in. If ScriptService.compile were to return an instance directly, instead of a factory, we would need some way to pass in this information in a generic way, which would probably mean duck typing through a String->Object map and then require casts. With the factory, we get static type checking of the arguments a script needs to be constructed.

[martijnvg]

Thanks for the explanation.

I think I added this after looking at ExecutableScript, but I guess there setNextVar() method is used to provide params to the script engine (so params isn't implicitly provided).

[rjernst]

Maybe we could name the context (and the class) something more descriptive for it's purpose? While the rest api is for executing a script, I think this context is a generic test context? Perhaps it could be "painless_test" (and PainlessTestScript) or something like that? I like having "test" in there because it is clear this is not for production uses, but to test painless code. It would also be more clear for when we do support other contexts in the execute api.

[jdconrad]

@mvg Thanks again for doing this and considering the feeback. Please feel free to commit this whenever you're ready to.

[simianhacker]

Thank you to @martijnvg @rjernst @jdconrad !

[Mpdreamz]

@martijnvg just wondering whats the reason context takes a map and not a string[]? are we expecting per context settings in the future?

[rjernst]

This was improved in a followup PR. context is now just a string (the name of the context to use), not a map. The variables to fill in that context are in context_setup. See https://www.elastic.co/guide/en/elasticsearch/painless/6.4/painless-execute-api.html

[Mpdreamz]

Thanks for the heads up @rjernst!