From 79375d35bbfdfcd380049cbc59d259fc31102d00 Mon Sep 17 00:00:00 2001 From: Armin Braun Date: Thu, 9 Aug 2018 14:32:36 +0200 Subject: [PATCH] Scripting: Replace Update Context (#32096) * SCRIPTING: Move Update Scripts to their own context * Added system property for backwards compatibility of change to `ctx.params` --- .../elasticsearch/gradle/BuildPlugin.groovy | 5 +- docs/build.gradle | 1 + modules/lang-painless/build.gradle | 1 + .../rest-api-spec/test/painless/15_update.yml | 2 +- .../AbstractAsyncBulkByScrollAction.java | 9 ++-- ...AsyncBulkByScrollActionScriptTestCase.java | 16 ++++-- .../action/update/UpdateHelper.java | 34 ++++++++---- .../script/ExecutableScript.java | 3 -- .../elasticsearch/script/ScriptModule.java | 2 +- .../elasticsearch/script/ScriptService.java | 2 +- .../elasticsearch/script/UpdateScript.java | 52 +++++++++++++++++++ .../script/ScriptServiceTests.java | 4 +- .../org/elasticsearch/update/UpdateIT.java | 1 + .../script/MockScriptEngine.java | 12 +++++ 14 files changed, 116 insertions(+), 28 deletions(-) create mode 100644 server/src/main/java/org/elasticsearch/script/UpdateScript.java diff --git a/buildSrc/src/main/groovy/org/elasticsearch/gradle/BuildPlugin.groovy b/buildSrc/src/main/groovy/org/elasticsearch/gradle/BuildPlugin.groovy index f6c6d5d7fd7..05fd4784863 100644 --- a/buildSrc/src/main/groovy/org/elasticsearch/gradle/BuildPlugin.groovy +++ b/buildSrc/src/main/groovy/org/elasticsearch/gradle/BuildPlugin.groovy @@ -782,9 +782,12 @@ class BuildPlugin implements Plugin { } } - // TODO: remove this once joda time is removed from scriptin in 7.0 + // TODO: remove this once joda time is removed from scripting in 7.0 systemProperty 'es.scripting.use_java_time', 'true' + // TODO: remove this once ctx isn't added to update script params in 7.0 + systemProperty 'es.scripting.update.ctx_in_params', 'false' + // Set the system keystore/truststore password if we're running tests in a FIPS-140 JVM if (project.inFipsJvm) { systemProperty 'javax.net.ssl.trustStorePassword', 'password' diff --git a/docs/build.gradle b/docs/build.gradle index 4c0502a0e06..029147bba2f 100644 --- a/docs/build.gradle +++ b/docs/build.gradle @@ -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' } // remove when https://github.com/elastic/elasticsearch/issues/31305 is fixed diff --git a/modules/lang-painless/build.gradle b/modules/lang-painless/build.gradle index e3a7ccecae2..ed4b1d631e0 100644 --- a/modules/lang-painless/build.gradle +++ b/modules/lang-painless/build.gradle @@ -25,6 +25,7 @@ esplugin { integTestCluster { module project.project(':modules:mapper-extras') systemProperty 'es.scripting.use_java_time', 'true' + systemProperty 'es.scripting.update.ctx_in_params', 'false' } dependencies { diff --git a/modules/lang-painless/src/test/resources/rest-api-spec/test/painless/15_update.yml b/modules/lang-painless/src/test/resources/rest-api-spec/test/painless/15_update.yml index 20047e7d482..f2e1cb616b9 100644 --- a/modules/lang-painless/src/test/resources/rest-api-spec/test/painless/15_update.yml +++ b/modules/lang-painless/src/test/resources/rest-api-spec/test/painless/15_update.yml @@ -132,7 +132,7 @@ body: script: lang: painless - source: "for (def key : params.keySet()) { ctx._source[key] = params[key]}" + source: "ctx._source.ctx = ctx" params: { bar: 'xxx' } - match: { error.root_cause.0.type: "remote_transport_exception" } diff --git a/modules/reindex/src/main/java/org/elasticsearch/index/reindex/AbstractAsyncBulkByScrollAction.java b/modules/reindex/src/main/java/org/elasticsearch/index/reindex/AbstractAsyncBulkByScrollAction.java index 5bd83b6c19a..731a27aa72c 100644 --- a/modules/reindex/src/main/java/org/elasticsearch/index/reindex/AbstractAsyncBulkByScrollAction.java +++ b/modules/reindex/src/main/java/org/elasticsearch/index/reindex/AbstractAsyncBulkByScrollAction.java @@ -48,9 +48,9 @@ import org.elasticsearch.index.mapper.SourceFieldMapper; import org.elasticsearch.index.mapper.TypeFieldMapper; import org.elasticsearch.index.mapper.VersionFieldMapper; import org.elasticsearch.index.reindex.ScrollableHitSource.SearchFailure; -import org.elasticsearch.script.ExecutableScript; import org.elasticsearch.script.Script; import org.elasticsearch.script.ScriptService; +import org.elasticsearch.script.UpdateScript; import org.elasticsearch.search.sort.SortBuilder; import org.elasticsearch.threadpool.ThreadPool; @@ -746,7 +746,7 @@ public abstract class AbstractAsyncBulkByScrollAction params; - private ExecutableScript executable; + private UpdateScript executable; private Map context; public ScriptApplier(WorkerBulkByScrollTaskState taskWorker, @@ -766,7 +766,7 @@ public abstract class AbstractAsyncBulkByScrollAction T applyScript(Consumer> scriptBody) { IndexRequest index = new IndexRequest("index", "type", "1").source(singletonMap("foo", "bar")); ScrollableHitSource.Hit doc = new ScrollableHitSource.BasicHit("test", "type", "id", 0); - ExecutableScript executableScript = new SimpleExecutableScript(scriptBody); - 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 updateScript = new UpdateScript(Collections.emptyMap()) { + @Override + public void execute(Map ctx) { + scriptBody.accept(ctx); + } + }; + UpdateScript.Factory factory = params -> updateScript; + ExecutableScript simpleExecutableScript = new SimpleExecutableScript(scriptBody); + when(scriptService.compile(any(), eq(ExecutableScript.CONTEXT))).thenReturn(params -> simpleExecutableScript); + when(scriptService.compile(any(), eq(UpdateScript.CONTEXT))).thenReturn(factory); AbstractAsyncBulkByScrollAction action = action(scriptService, request().setScript(mockScript(""))); RequestWrapper result = action.buildScriptApplier().apply(AbstractAsyncBulkByScrollAction.wrap(index), doc); return (result != null) ? (T) result.self() : null; diff --git a/server/src/main/java/org/elasticsearch/action/update/UpdateHelper.java b/server/src/main/java/org/elasticsearch/action/update/UpdateHelper.java index 5212b1f3521..77485f81e58 100644 --- a/server/src/main/java/org/elasticsearch/action/update/UpdateHelper.java +++ b/server/src/main/java/org/elasticsearch/action/update/UpdateHelper.java @@ -19,6 +19,11 @@ package org.elasticsearch.action.update; +import java.io.IOException; +import java.util.Collections; +import java.util.HashMap; +import java.util.Map; +import java.util.function.LongSupplier; import org.apache.logging.log4j.Logger; import org.elasticsearch.ElasticsearchException; import org.elasticsearch.action.DocWriteResponse; @@ -42,21 +47,22 @@ import org.elasticsearch.index.get.GetResult; import org.elasticsearch.index.mapper.RoutingFieldMapper; import org.elasticsearch.index.shard.IndexShard; import org.elasticsearch.index.shard.ShardId; -import org.elasticsearch.script.ExecutableScript; import org.elasticsearch.script.Script; import org.elasticsearch.script.ScriptService; +import org.elasticsearch.script.UpdateScript; import org.elasticsearch.search.lookup.SourceLookup; -import java.io.IOException; -import java.util.Collections; -import java.util.HashMap; -import java.util.Map; -import java.util.function.LongSupplier; +import static org.elasticsearch.common.Booleans.parseBoolean; /** * Helper for translating an update request to an index, delete request or update response. */ public class UpdateHelper extends AbstractComponent { + + /** Whether scripts should add the ctx variable to the params map. */ + private static final boolean CTX_IN_PARAMS = + parseBoolean(System.getProperty("es.scripting.update.ctx_in_params"), true); + private final ScriptService scriptService; public UpdateHelper(Settings settings, ScriptService scriptService) { @@ -279,10 +285,18 @@ public class UpdateHelper extends AbstractComponent { private Map executeScript(Script script, Map ctx) { try { if (scriptService != null) { - ExecutableScript.Factory factory = scriptService.compile(script, ExecutableScript.UPDATE_CONTEXT); - ExecutableScript executableScript = factory.newInstance(script.getParams()); - executableScript.setNextVar(ContextFields.CTX, ctx); - executableScript.run(); + UpdateScript.Factory factory = scriptService.compile(script, UpdateScript.CONTEXT); + final Map params; + if (CTX_IN_PARAMS) { + params = new HashMap<>(script.getParams()); + params.put(ContextFields.CTX, ctx); + deprecationLogger.deprecated("Using `ctx` via `params.ctx` is deprecated. " + + "Use -Des.scripting.update.ctx_in_params=false to enforce non-deprecated usage."); + } else { + params = script.getParams(); + } + UpdateScript executableScript = factory.newInstance(params); + executableScript.execute(ctx); } } catch (Exception e) { throw new IllegalArgumentException("failed to execute script", e); diff --git a/server/src/main/java/org/elasticsearch/script/ExecutableScript.java b/server/src/main/java/org/elasticsearch/script/ExecutableScript.java index 1bd4c31ebf3..d0d8020371b 100644 --- a/server/src/main/java/org/elasticsearch/script/ExecutableScript.java +++ b/server/src/main/java/org/elasticsearch/script/ExecutableScript.java @@ -46,7 +46,4 @@ public interface ExecutableScript { } ScriptContext CONTEXT = new ScriptContext<>("executable", Factory.class); - - // TODO: remove these once each has its own script interface - ScriptContext UPDATE_CONTEXT = new ScriptContext<>("update", Factory.class); } diff --git a/server/src/main/java/org/elasticsearch/script/ScriptModule.java b/server/src/main/java/org/elasticsearch/script/ScriptModule.java index 3eeb26317f9..f04e690fa42 100644 --- a/server/src/main/java/org/elasticsearch/script/ScriptModule.java +++ b/server/src/main/java/org/elasticsearch/script/ScriptModule.java @@ -46,10 +46,10 @@ public class ScriptModule { SearchScript.SCRIPT_SORT_CONTEXT, SearchScript.TERMS_SET_QUERY_CONTEXT, ExecutableScript.CONTEXT, + UpdateScript.CONTEXT, BucketAggregationScript.CONTEXT, BucketAggregationSelectorScript.CONTEXT, SignificantTermsHeuristicScoreScript.CONTEXT, - ExecutableScript.UPDATE_CONTEXT, IngestScript.CONTEXT, FilterScript.CONTEXT, SimilarityScript.CONTEXT, diff --git a/server/src/main/java/org/elasticsearch/script/ScriptService.java b/server/src/main/java/org/elasticsearch/script/ScriptService.java index ca79e3b80fc..9768547b898 100644 --- a/server/src/main/java/org/elasticsearch/script/ScriptService.java +++ b/server/src/main/java/org/elasticsearch/script/ScriptService.java @@ -285,7 +285,7 @@ public class ScriptService extends AbstractComponent implements Closeable, Clust // TODO: fix this through some API or something, that's wrong // special exception to prevent expressions from compiling as update or mapping scripts boolean expression = "expression".equals(lang); - boolean notSupported = context.name.equals(ExecutableScript.UPDATE_CONTEXT.name); + boolean notSupported = context.name.equals(UpdateScript.CONTEXT.name); if (expression && notSupported) { throw new UnsupportedOperationException("scripts of type [" + script.getType() + "]," + " operation [" + context.name + "] and lang [" + lang + "] are not supported"); diff --git a/server/src/main/java/org/elasticsearch/script/UpdateScript.java b/server/src/main/java/org/elasticsearch/script/UpdateScript.java new file mode 100644 index 00000000000..c6a1d5dd9ea --- /dev/null +++ b/server/src/main/java/org/elasticsearch/script/UpdateScript.java @@ -0,0 +1,52 @@ + +/* + * Licensed to Elasticsearch under one or more contributor + * license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright + * ownership. Elasticsearch licenses this file to you under + * the Apache License, Version 2.0 (the "License"); you may + * not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +package org.elasticsearch.script; + +import java.util.Map; + +/** + * An update script. + */ +public abstract class UpdateScript { + + public static final String[] PARAMETERS = { "ctx" }; + + /** The context used to compile {@link UpdateScript} factories. */ + public static final ScriptContext CONTEXT = new ScriptContext<>("update", Factory.class); + + /** The generic runtime parameters for the script. */ + private final Map params; + + public UpdateScript(Map params) { + this.params = params; + } + + /** Return the parameters for this script. */ + public Map getParams() { + return params; + } + + public abstract void execute(Map ctx); + + public interface Factory { + UpdateScript newInstance(Map params); + } +} diff --git a/server/src/test/java/org/elasticsearch/script/ScriptServiceTests.java b/server/src/test/java/org/elasticsearch/script/ScriptServiceTests.java index 585f8601651..ea8b6a92234 100644 --- a/server/src/test/java/org/elasticsearch/script/ScriptServiceTests.java +++ b/server/src/test/java/org/elasticsearch/script/ScriptServiceTests.java @@ -167,7 +167,7 @@ public class ScriptServiceTests extends ESTestCase { assertCompileAccepted("painless", "script", ScriptType.INLINE, SearchScript.CONTEXT); assertCompileAccepted("painless", "script", ScriptType.INLINE, SearchScript.AGGS_CONTEXT); - assertCompileAccepted("painless", "script", ScriptType.INLINE, ExecutableScript.UPDATE_CONTEXT); + assertCompileAccepted("painless", "script", ScriptType.INLINE, UpdateScript.CONTEXT); assertCompileAccepted("painless", "script", ScriptType.INLINE, IngestScript.CONTEXT); } @@ -187,7 +187,7 @@ public class ScriptServiceTests extends ESTestCase { assertCompileAccepted("painless", "script", ScriptType.INLINE, SearchScript.CONTEXT); assertCompileAccepted("painless", "script", ScriptType.INLINE, SearchScript.AGGS_CONTEXT); - assertCompileRejected("painless", "script", ScriptType.INLINE, ExecutableScript.UPDATE_CONTEXT); + assertCompileRejected("painless", "script", ScriptType.INLINE, UpdateScript.CONTEXT); } public void testAllowNoScriptTypeSettings() throws IOException { diff --git a/server/src/test/java/org/elasticsearch/update/UpdateIT.java b/server/src/test/java/org/elasticsearch/update/UpdateIT.java index e4ea078b8f7..85ebf01ef28 100644 --- a/server/src/test/java/org/elasticsearch/update/UpdateIT.java +++ b/server/src/test/java/org/elasticsearch/update/UpdateIT.java @@ -93,6 +93,7 @@ public class UpdateIT extends ESIntegTestCase { } Map source = (Map) ctx.get("_source"); + params.remove("ctx"); source.putAll(params); return ctx; diff --git a/test/framework/src/main/java/org/elasticsearch/script/MockScriptEngine.java b/test/framework/src/main/java/org/elasticsearch/script/MockScriptEngine.java index 14dcac926f7..8083931e73d 100644 --- a/test/framework/src/main/java/org/elasticsearch/script/MockScriptEngine.java +++ b/test/framework/src/main/java/org/elasticsearch/script/MockScriptEngine.java @@ -96,6 +96,18 @@ public class MockScriptEngine implements ScriptEngine { } }; return context.factoryClazz.cast(factory); + } else if (context.instanceClazz.equals(UpdateScript.class)) { + UpdateScript.Factory factory = parameters -> new UpdateScript(parameters) { + @Override + public void execute(Map ctx) { + final Map vars = new HashMap<>(); + vars.put("ctx", ctx); + vars.put("params", parameters); + vars.putAll(parameters); + script.apply(vars); + } + }; + return context.factoryClazz.cast(factory); } else if (context.instanceClazz.equals(BucketAggregationScript.class)) { BucketAggregationScript.Factory factory = parameters -> new BucketAggregationScript(parameters) { @Override