Skip to content

Commit

Permalink
Make reindex and lang-javascript compatible
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
nik9000 committed Aug 11, 2016
1 parent 69db1c1 commit 031d471
Show file tree
Hide file tree
Showing 4 changed files with 32 additions and 14 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -179,46 +179,46 @@ protected boolean applyScript(IndexRequest index, SearchHit doc, ExecutableScrip
*/
index.source((Map<String, Object>) resultCtx.remove(SourceFieldMapper.NAME));

Object newValue = ctx.remove(IndexFieldMapper.NAME);
Object newValue = resultCtx.remove(IndexFieldMapper.NAME);
if (false == doc.index().equals(newValue)) {
scriptChangedIndex(index, newValue);
}
newValue = ctx.remove(TypeFieldMapper.NAME);
newValue = resultCtx.remove(TypeFieldMapper.NAME);
if (false == doc.type().equals(newValue)) {
scriptChangedType(index, newValue);
}
newValue = ctx.remove(IdFieldMapper.NAME);
newValue = resultCtx.remove(IdFieldMapper.NAME);
if (false == doc.id().equals(newValue)) {
scriptChangedId(index, newValue);
}
newValue = ctx.remove(VersionFieldMapper.NAME);
newValue = resultCtx.remove(VersionFieldMapper.NAME);
if (false == Objects.equals(oldVersion, newValue)) {
scriptChangedVersion(index, newValue);
}
newValue = ctx.remove(ParentFieldMapper.NAME);
newValue = resultCtx.remove(ParentFieldMapper.NAME);
if (false == Objects.equals(oldParent, newValue)) {
scriptChangedParent(index, newValue);
}
/*
* Its important that routing comes after parent in case you want to
* change them both.
*/
newValue = ctx.remove(RoutingFieldMapper.NAME);
newValue = resultCtx.remove(RoutingFieldMapper.NAME);
if (false == Objects.equals(oldRouting, newValue)) {
scriptChangedRouting(index, newValue);
}
newValue = ctx.remove(TimestampFieldMapper.NAME);
newValue = resultCtx.remove(TimestampFieldMapper.NAME);
if (false == Objects.equals(oldTimestamp, newValue)) {
scriptChangedTimestamp(index, newValue);
}
newValue = ctx.remove(TTLFieldMapper.NAME);
newValue = resultCtx.remove(TTLFieldMapper.NAME);
if (false == Objects.equals(oldTTL, newValue)) {
scriptChangedTTL(index, newValue);
}
if (false == ctx.isEmpty()) {
if (false == resultCtx.isEmpty()) {
StringBuilder msg = new StringBuilder("Invalid fields added to ctx [");
boolean first = true;
for (String key : ctx.keySet()) {
for (String key : resultCtx.keySet()) {
if (first) {
first = false;
} else {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -22,8 +22,11 @@
import org.elasticsearch.common.util.Consumer;
import org.elasticsearch.script.ExecutableScript;

import java.util.HashMap;
import java.util.Map;

import static org.elasticsearch.test.ESTestCase.randomBoolean;

public class SimpleExecutableScript implements ExecutableScript {
private final Consumer<Map<String, Object>> script;
private Map<String, Object> ctx;
Expand All @@ -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;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -175,14 +175,16 @@ public Object compile(String script, Map<String, String> params) {
}

@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);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,13 @@ public void testSimpleEquation() {
assertThat(((Number) o).intValue(), equalTo(3));
}

@Test
public void testNullVars() {
CompiledScript script = new CompiledScript(ScriptService.ScriptType.INLINE, "testSimpleEquation", "js",
se.compile("1 + 2", Collections.<String, String>emptyMap()));
Object o = se.executable(script, null).run();
assertThat(((Number) o).intValue(), equalTo(3));
}

public void testMapAccess() {
Map<String, Object> vars = new HashMap<String, Object>();

Expand Down

0 comments on commit 031d471

Please sign in to comment.