From e07e5d66faf4da6189be3b9f6e8582e28a708f3a Mon Sep 17 00:00:00 2001 From: Nik Everett Date: Wed, 10 Aug 2016 16:18:59 -0400 Subject: [PATCH] Make reindex and lang-javascript compatible Fixes two issues: 1. lang-javascript doesn't support `executable` with a `null` `vars` parameters. The parameter is quite nullable. 2. reindex didn't support script engines who's `unwrap` method wasn't a noop. This didn't come up for lang-groovy or lang-painless because both of those `unwrap`s were noops. lang-javascript copys all maps that it `unwrap`s. This adds fairly low level unit tests for these fixes but dosen't add an integration test that makes sure that reindex and lang-javascript play well together. That'd make backporting this difficult and would add a fairly significant amount of time to the build for a fairly rare interaction. Hopefully the unit tests will be enough. --- .../AbstractAsyncBulkIndexByScrollAction.java | 24 ++++++++++--------- .../index/reindex/SimpleExecutableScript.java | 10 ++++++++ .../JavaScriptScriptEngineService.java | 8 ++++--- .../JavaScriptScriptEngineTests.java | 9 +++++++ 4 files changed, 37 insertions(+), 14 deletions(-) diff --git a/modules/reindex/src/main/java/org/elasticsearch/index/reindex/AbstractAsyncBulkIndexByScrollAction.java b/modules/reindex/src/main/java/org/elasticsearch/index/reindex/AbstractAsyncBulkIndexByScrollAction.java index 73c05678651..ed5211da141 100644 --- a/modules/reindex/src/main/java/org/elasticsearch/index/reindex/AbstractAsyncBulkIndexByScrollAction.java +++ b/modules/reindex/src/main/java/org/elasticsearch/index/reindex/AbstractAsyncBulkIndexByScrollAction.java @@ -450,6 +450,8 @@ public abstract class AbstractAsyncBulkIndexByScrollAction(); + } else { + context.clear(); } context.put(IndexFieldMapper.NAME, doc.getIndex()); @@ -485,23 +487,23 @@ public abstract class AbstractAsyncBulkIndexByScrollAction) resultCtx.remove(SourceFieldMapper.NAME)); - Object newValue = context.remove(IndexFieldMapper.NAME); + Object newValue = resultCtx.remove(IndexFieldMapper.NAME); if (false == doc.getIndex().equals(newValue)) { scriptChangedIndex(request, newValue); } - newValue = context.remove(TypeFieldMapper.NAME); + newValue = resultCtx.remove(TypeFieldMapper.NAME); if (false == doc.getType().equals(newValue)) { scriptChangedType(request, newValue); } - newValue = context.remove(IdFieldMapper.NAME); + newValue = resultCtx.remove(IdFieldMapper.NAME); if (false == doc.getId().equals(newValue)) { scriptChangedId(request, newValue); } - newValue = context.remove(VersionFieldMapper.NAME); + newValue = resultCtx.remove(VersionFieldMapper.NAME); if (false == Objects.equals(oldVersion, newValue)) { scriptChangedVersion(request, newValue); } - newValue = context.remove(ParentFieldMapper.NAME); + newValue = resultCtx.remove(ParentFieldMapper.NAME); if (false == Objects.equals(oldParent, newValue)) { scriptChangedParent(request, newValue); } @@ -509,26 +511,26 @@ public abstract class AbstractAsyncBulkIndexByScrollAction> script; private Map ctx; @@ -50,6 +53,13 @@ public class SimpleExecutableScript implements ExecutableScript { @Override public Object unwrap(Object value) { + // Some script engines (javascript) copy any maps they unwrap + if (randomBoolean()) { + if (value instanceof Map) { + return new HashMap<>((Map) value); + } + } + // Others just return the objects plain (groovy, painless) return value; } } diff --git a/plugins/lang-javascript/src/main/java/org/elasticsearch/script/javascript/JavaScriptScriptEngineService.java b/plugins/lang-javascript/src/main/java/org/elasticsearch/script/javascript/JavaScriptScriptEngineService.java index 3a556ce4429..3477f62a5bc 100644 --- a/plugins/lang-javascript/src/main/java/org/elasticsearch/script/javascript/JavaScriptScriptEngineService.java +++ b/plugins/lang-javascript/src/main/java/org/elasticsearch/script/javascript/JavaScriptScriptEngineService.java @@ -172,14 +172,16 @@ public class JavaScriptScriptEngineService extends AbstractComponent implements } @Override - public ExecutableScript executable(CompiledScript compiledScript, Map vars) { + public ExecutableScript executable(CompiledScript compiledScript, @Nullable Map vars) { Context ctx = Context.enter(); try { Scriptable scope = ctx.newObject(globalScope); scope.setPrototype(globalScope); scope.setParentScope(null); - for (Map.Entry entry : vars.entrySet()) { - ScriptableObject.putProperty(scope, entry.getKey(), entry.getValue()); + if (vars != null) { + for (Map.Entry entry : vars.entrySet()) { + ScriptableObject.putProperty(scope, entry.getKey(), entry.getValue()); + } } return new JavaScriptExecutableScript((Script) compiledScript.compiled(), scope); diff --git a/plugins/lang-javascript/src/test/java/org/elasticsearch/script/javascript/JavaScriptScriptEngineTests.java b/plugins/lang-javascript/src/test/java/org/elasticsearch/script/javascript/JavaScriptScriptEngineTests.java index c3afe436c31..84bc97abfbe 100644 --- a/plugins/lang-javascript/src/test/java/org/elasticsearch/script/javascript/JavaScriptScriptEngineTests.java +++ b/plugins/lang-javascript/src/test/java/org/elasticsearch/script/javascript/JavaScriptScriptEngineTests.java @@ -25,6 +25,8 @@ import java.util.HashMap; import java.util.List; import java.util.Map; +import static java.util.Collections.emptyMap; + import org.elasticsearch.common.collect.MapBuilder; import org.elasticsearch.common.settings.Settings; import org.elasticsearch.script.CompiledScript; @@ -59,6 +61,13 @@ public class JavaScriptScriptEngineTests extends ESTestCase { assertThat(((Number) o).intValue(), equalTo(3)); } + public void testNullVars() { + CompiledScript script = new CompiledScript(ScriptService.ScriptType.INLINE, "testSimpleEquation", "js", + se.compile(null, "1 + 2", emptyMap())); + Object o = se.executable(script, null).run(); + assertThat(((Number) o).intValue(), equalTo(3)); + } + @SuppressWarnings("unchecked") public void testMapAccess() { Map vars = new HashMap();