Scripting: Replace Update Context by original-brownbear · Pull Request #32096 · elastic/elasticsearch

original-brownbear · 2018-07-16T14:41:11Z

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 params included ctx under params.ctx because of the way org.elasticsearch.painless.ScriptImpl worked (using the params to provide the ctx indirectly).
With this change, making ctx and actual parameter this wasn't necessary anymore and I didn't add any logic that puts ctx into params here (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 for loop would hit the key ctx and then put ctx to ctx.ctx. This doesn't happen anymore with the change here so I just directly forced that circular dependency.

…text

elasticmachine · 2018-07-16T14:41:13Z

Pinging @elastic/es-core-infra

original-brownbear · 2018-07-17T18:52:42Z

@rjernst can you take a look here when you have some time?

rjernst

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.

rjernst · 2018-08-01T23:42:25Z

modules/lang-painless/src/test/resources/rest-api-spec/test/painless/15_update.yml

            script:
              lang:   painless
-              source: "for (def key : params.keySet()) { ctx._source[key] = params[key]}"
+              source: "ctx._source.ctx = ctx"


Isn't this creating a cycle in _source?

@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 :)

Ah, thanks, sorry I missed that.

original-brownbear · 2018-08-02T06:27:18Z

@rjernst sorry missed your other comment last night. So in general are we good with this for master/7.0 and do the property + backwards compatibility things in the back port to 6.x only or do we add the backwards compatibility here as well?

rjernst · 2018-08-03T21:51:07Z

@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.

…text

original-brownbear · 2018-08-06T21:47:12Z

@rjernst added a property that enables the old behaviour now. Wasn't quite sure how to exactly set it in the initial PR.
Defaulted it to the old behaviour (which would also cause deprecation logging by default). Is that correct?

rjernst

This looks good. A few final notes.

rjernst · 2018-08-07T19:47:52Z

docs/build.gradle


  // 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'


Since this is set in BuildPlugin, it shouldn't be necessary here or anywhere else?

Right my bad that was just a random leftover from other experiments. Removing

Ah no I needed this because it's in the integration test definition not the unit test definition that is inherited.

Hrm, are you sure? BuildPlugin.commonTestConfig should be shared by both

@rjernst yea def. I even debugged this locally and it failed like on Jenkins with the deprecation line.

rjernst · 2018-08-07T19:48:36Z

...test/java/org/elasticsearch/index/reindex/AbstractAsyncBulkByScrollActionScriptTestCase.java

-        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()) {


nit: please update the variable names, as these are no longer ExecutableScript

rjernst · 2018-08-07T20:07:19Z

server/src/main/java/org/elasticsearch/action/update/UpdateHelper.java

+                final Map<String, Object> params;
+                if (CTX_IN_PARAMS) {
+                    params = new HashMap<>();
+                    params.putAll(script.getParams());


You could use the ctor that takes an existing Map to copy.

rjernst · 2018-08-08T23:58:50Z

server/src/main/java/org/elasticsearch/action/update/UpdateHelper.java

+                    params = new HashMap<>();
+                    params.putAll(script.getParams());
+                    params.put(ContextFields.CTX, ctx);
+                    deprecationLogger.deprecated("Exposing `ctx` under `params.ctx` is deprecated. " +


nit: "Exposing ctx under" -> "Using ctx via"

Also, I would use "to enforce the non-deprecated usage" instead of "to disable this behavior"

…text

original-brownbear · 2018-08-09T12:31:55Z

@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)

original-brownbear added 12 commits July 16, 2018 09:19

SCRIPTING: Move Update Scripts to their own context

44bc7e8

SCRIPTING: Move Update Scripts to their own context

c4c572e

SCRIPTING: Move Update Scripts to their own context

a560645

SCRIPTING: Move Update Scripts to their own context

38601f9

SCRIPTING: Move Update Scripts to their own context

3bb2eea

Merge remote-tracking branch 'elastic/master' into replace-update-con…

228e465

…text

Merge remote-tracking branch 'elastic/master' into replace-update-con…

aa74ff8

…text

Merge remote-tracking branch 'elastic/master' into replace-update-con…

78d6945

…text

Merge remote-tracking branch 'elastic/master' into replace-update-con…

7700395

…text

Merge branch 'master' into replace-update-context

8473a26

SCRIPTING: Move Update Scripts to their own context

aa12c43

Merge remote-tracking branch 'elastic/master' into replace-update-con…

049606c

…text

original-brownbear added the :Core/Infra/Scripting Scripting abstractions, Painless, and Mustache label Jul 16, 2018

original-brownbear requested a review from rjernst July 16, 2018 14:41

original-brownbear added the >refactoring label Jul 17, 2018

rjernst reviewed Aug 1, 2018

View reviewed changes

original-brownbear added 8 commits August 4, 2018 17:48

merge in master

49e1875

CR: Add backwards compatibility with by putting ctx back into params

77d3e60

CR: Add backwards compatibility with by putting ctx back into params

6083196

CR: Add backwards compatibility with by putting ctx back into params

2ef1e5c

Merge remote-tracking branch 'elastic/master' into replace-update-con…

21728f4

…text

fix integ tests

699f5eb

fix integ tests

f526340

Merge remote-tracking branch 'elastic/master' into replace-update-con…

5671b5e

…text

rjernst approved these changes Aug 8, 2018

View reviewed changes

original-brownbear added 2 commits August 9, 2018 09:37

Merge remote-tracking branch 'elastic/master' into replace-update-con…

9cf2034

…text

CR nits

0df2ce0

original-brownbear added review v7.0.0 v6.5.0 labels Aug 9, 2018

original-brownbear added 2 commits August 9, 2018 11:35

Merge remote-tracking branch 'elastic/master' into replace-update-con…

35d9445

…text

CR nits

329feea

original-brownbear merged commit 79375d3 into elastic:master Aug 9, 2018

original-brownbear deleted the replace-update-context branch August 9, 2018 12:32

original-brownbear added a commit that referenced this pull request Aug 12, 2018

Scripting: Replace Update Context (#32096) (#32752)

6f3a3e1

* SCRIPTING: Move Update Scripts to their own context * Added system property for backwards compatibility of change to `ctx.params`

lipsill added a commit to lipsill/elasticsearch that referenced this pull request Sep 24, 2018

Use a system property to avoid deprecation warnings after the Update

44b354a

Scripts have been moved to their own context (elastic#32096)

colings86 added v7.0.0-beta1 and removed v7.0.0 labels Feb 7, 2019

Conversation

original-brownbear commented Jul 16, 2018

Uh oh!

elasticmachine commented Jul 16, 2018

Uh oh!

original-brownbear commented Jul 17, 2018

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!

original-brownbear commented Aug 2, 2018

Uh oh!

rjernst commented Aug 3, 2018

Uh oh!

original-brownbear commented Aug 6, 2018

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!

Choose a reason for hiding this comment

Uh oh!

original-brownbear commented Aug 9, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants