Scripting: Replace Update Context #32096
Conversation
original-brownbear
added
the
:Core/Infra/Scripting
|
Pinging @elastic/es-core-infra |
|
@rjernst can you take a look here when you have some time? |
There was a problem hiding this comment.
Overall this looks ok. We need to maintain backcompat, though, in 6.x, with having ctx available in params. The backcompat should be controlled by a sysprop and have a deprecation warning, as we have done in other similar PRs (look at scripted metric agg as an example). I would also like us to look at possibly having something other than Map, and a different name than ctx long term. The latter can happen as followups, but I want to bring it up as I think it is part of the overall work for making all script uses context aware.
| @@ -132,7 +132,7 @@ | |||
| body: | |||
| script: | |||
| lang: painless | |||
| source: "for (def key : params.keySet()) { ctx._source[key] = params[key]}" | |||
| source: "ctx._source.ctx = ctx" | |||
There was a problem hiding this comment.
Isn't this creating a cycle in _source?
There was a problem hiding this comment.
@rjernst that's the point, see what it matches a few lines down and my PR description https://github.com/elastic/elasticsearch/pull/32096/files/049606c19854ac69ee07f2ff5d772f02532f769a#diff-585f6613de46bec6547c7b2aed89db8bR140 :)
There was a problem hiding this comment.
Ah, thanks, sorry I missed that.
|
@rjernst sorry missed your other comment last night. So in general are we good with this for |
|
@original-brownbear Typically we would add the backcompat/property to the first PR which would go into master and be backported to 6.x, then remove the backcompat in master in a followup. |
|
@rjernst added a property that enables the old behaviour now. Wasn't quite sure how to exactly set it in the initial PR. |
| @@ -40,6 +40,7 @@ integTestCluster { | |||
|
|
|||
| // TODO: remove this for 7.0, this exists to allow the doc examples in 6.x to continue using the defaults | |||
| systemProperty 'es.scripting.use_java_time', 'false' | |||
| systemProperty 'es.scripting.update.ctx_in_params', 'false' | |||
There was a problem hiding this comment.
Since this is set in BuildPlugin, it shouldn't be necessary here or anywhere else?
There was a problem hiding this comment.
Right my bad that was just a random leftover from other experiments. Removing
There was a problem hiding this comment.
Ah no I needed this because it's in the integration test definition not the unit test definition that is inherited.
There was a problem hiding this comment.
Hrm, are you sure? BuildPlugin.commonTestConfig should be shared by both
There was a problem hiding this comment.
@rjernst yea def. I even debugged this locally and it failed like on Jenkins with the deprecation line.
| ExecutableScript.Factory factory = params -> executableScript; | ||
| when(scriptService.compile(any(), eq(ExecutableScript.CONTEXT))).thenReturn(factory); | ||
| when(scriptService.compile(any(), eq(ExecutableScript.UPDATE_CONTEXT))).thenReturn(factory); | ||
| UpdateScript executableScript = new UpdateScript(Collections.emptyMap()) { |
There was a problem hiding this comment.
nit: please update the variable names, as these are no longer ExecutableScript
| final Map<String, Object> params; | ||
| if (CTX_IN_PARAMS) { | ||
| params = new HashMap<>(); | ||
| params.putAll(script.getParams()); |
There was a problem hiding this comment.
You could use the ctor that takes an existing Map to copy.
| params = new HashMap<>(); | ||
| params.putAll(script.getParams()); | ||
| params.put(ContextFields.CTX, ctx); | ||
| deprecationLogger.deprecated("Exposing `ctx` under `params.ctx` is deprecated. " + |
There was a problem hiding this comment.
nit: "Exposing ctx under" -> "Using ctx via"
Also, I would use "to enforce the non-deprecated usage" instead of "to disable this behavior"
|
@rjernst thanks!, fixed all the points with the requested changes -> merging :) |
* SCRIPTING: Move Update Scripts to their own context * Added system property for backwards compatibility of change to `ctx.params`
Scripts have been moved to their own context (elastic#32096)
* Replace deprecated field `code` with `source` for stored scripts (#25127) * Replace examples using the deprecated endpoint `{index}/{type}/_search` with `{index}/_search` (#29468) * Use a system property to avoid deprecation warnings after the Update Scripts have been moved to their own context (#32096)
* Replace deprecated field `code` with `source` for stored scripts (#25127) * Replace examples using the deprecated endpoint `{index}/{type}/_search` with `{index}/_search` (#29468) * Use a system property to avoid deprecation warnings after the Update Scripts have been moved to their own context (#32096)
* Replace deprecated field `code` with `source` for stored scripts (#25127) * Replace examples using the deprecated endpoint `{index}/{type}/_search` with `{index}/_search` (#29468) * Use a system property to avoid deprecation warnings after the Update Scripts have been moved to their own context (#32096)
Same as #32068 and #32003 (fully resolves the todo together with #32068 ).
One notable point would be the change in behaviour this introduces and I'm not 100% sure about:
Previously the
paramsincludedctxunderparams.ctxbecause of the wayorg.elasticsearch.painless.ScriptImplworked (using theparamsto provide thectxindirectly).With this change, making
ctxand actual parameter this wasn't necessary anymore and I didn't add any logic that putsctxintoparamshere (seemed wrong to me since it being there was just a side effect?).This required a change to this IT:
See https://github.com/elastic/elasticsearch/compare/master...original-brownbear:replace-update-context?expand=1#diff-585f6613de46bec6547c7b2aed89db8bR135
The test forced a circular dependency in a kind of complicated way because the
forloop would hit the keyctxand then putctxtoctx.ctx. This doesn't happen anymore with the change here so I just directly forced that circular dependency.