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.

Author: nik9000

modules/reindex/src/main/java/org/elasticsearch/index/reindex/AbstractAsyncBulkIndexByScrollAction.java

@@ -450,6 +450,8 @@ public RequestWrapper<?> apply(RequestWrapper<?> request, ScrollableHitSource.Hi
  }
  if (context == null) {
  context = new HashMap<>();
+ } else {
+ context.clear();
  }
 
  context.put(IndexFieldMapper.NAME, doc.getIndex());
@@ -485,50 +487,50 @@ public RequestWrapper<?> apply(RequestWrapper<?> request, ScrollableHitSource.Hi
  */
  request.setSource((Map<String, Object>) 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);
  }
  /*
  * Its important that routing comes after parent in case you want to
  * change them both.
  */
- newValue = context.remove(RoutingFieldMapper.NAME);
+ newValue = resultCtx.remove(RoutingFieldMapper.NAME);
  if (false == Objects.equals(oldRouting, newValue)) {
  scriptChangedRouting(request, newValue);
  }
- newValue = context.remove(TimestampFieldMapper.NAME);
+ newValue = resultCtx.remove(TimestampFieldMapper.NAME);
  if (false == Objects.equals(oldTimestamp, newValue)) {
  scriptChangedTimestamp(request, newValue);
  }
- newValue = context.remove(TTLFieldMapper.NAME);
+ newValue = resultCtx.remove(TTLFieldMapper.NAME);
  if (false == Objects.equals(oldTTL, newValue)) {
  scriptChangedTTL(request, newValue);
  }
 
  OpType newOpType = OpType.fromString(newOp);
- if (newOpType !=  oldOpType) {
+ if (newOpType != oldOpType) {
  return scriptChangedOpType(request, oldOpType, newOpType);
  }
 
- if (false == context.isEmpty()) {
- throw new IllegalArgumentException("Invalid fields added to context [" + String.join(",", context.keySet()) + ']');
+ if (false == resultCtx.isEmpty()) {
+ throw new IllegalArgumentException("Invalid fields added to context [" + String.join(",", resultCtx.keySet()) + ']');
  }
  return request;
  }

modules/reindex/src/test/java/org/elasticsearch/index/reindex/SimpleExecutableScript.java

@@ -21,9 +21,12 @@
 
 import org.elasticsearch.script.ExecutableScript;
 
+import java.util.HashMap;
 import java.util.Map;
 import java.util.function.Consumer;
 
+import static org.elasticsearch.test.ESTestCase.randomBoolean;
+
 public class SimpleExecutableScript implements ExecutableScript {
  private final Consumer<Map<String, Object>> script;
  private Map<String, Object> ctx;
@@ -50,6 +53,13 @@ public void setNextVar(String name, Object value) {
 
  @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;
  }
 }

plugins/lang-javascript/src/main/java/org/elasticsearch/script/javascript/JavaScriptScriptEngineService.java

@@ -172,14 +172,16 @@ public Object compile(String scriptName, String scriptSource, Map<String, String
  }
 
  @Override
- public ExecutableScript executable(CompiledScript compiledScript, Map<String, Object> vars) {
+ public ExecutableScript executable(CompiledScript compiledScript, @Nullable Map<String, Object> vars) {
  Context ctx = Context.enter();
  try {
  Scriptable scope = ctx.newObject(globalScope);
  scope.setPrototype(globalScope);
  scope.setParentScope(null);
- for (Map.Entry<String, Object> entry : vars.entrySet()) {
- ScriptableObject.putProperty(scope, entry.getKey(), entry.getValue());
+ if (vars != null) {
+ for (Map.Entry<String, Object> entry : vars.entrySet()) {
+ ScriptableObject.putProperty(scope, entry.getKey(), entry.getValue());
+ }
  }
 
  return new JavaScriptExecutableScript((Script) compiledScript.compiled(), scope);

plugins/lang-javascript/src/test/java/org/elasticsearch/script/javascript/JavaScriptScriptEngineTests.java

@@ -25,6 +25,8 @@
 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 void testSimpleEquation() {
  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<String, Object> vars = new HashMap<String, Object>();