Skip to content

Commit

Permalink
Scripts: Convert template script engines to return String instead of …
Browse files Browse the repository at this point in the history
…BytesReference (#24447)

Template script engines (mustache, the only one) currently return a
BytesReference that users must know is utf8 encoded. This commit
modifies all callers and mustache to have the template engine return
String. This is much simpler, and does not require decoding in order to
use (for example, in ingest).
  • Loading branch information
rjernst committed May 16, 2017
1 parent 548a5c1 commit 6ce597a
Show file tree
Hide file tree
Showing 13 changed files with 36 additions and 78 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -105,7 +105,7 @@ public long nowInMillis() {
return nowInMillis.getAsLong();
}

public BytesReference getTemplateBytes(Script template) {
public String getTemplateBytes(Script template) {
CompiledTemplate compiledTemplate = scriptService.compileTemplate(template, ScriptContext.Standard.SEARCH);
return compiledTemplate.run(template.getParams());
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -392,7 +392,7 @@ protected final void failIfFrozen() {
}

@Override
public final BytesReference getTemplateBytes(Script template) {
public final String getTemplateBytes(Script template) {
failIfFrozen();
return super.getTemplateBytes(template);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,7 @@ public Template compile(String template) {
return new Template() {
@Override
public String execute(Map<String, Object> model) {
return compiledTemplate.run(model).utf8ToString();
return compiledTemplate.run(model);
}

@Override
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -325,7 +325,7 @@ public CompiledScript compile(Script script, ScriptContext scriptContext) {
/** Compiles a template. Note this will be moved to a separate TemplateService in the future. */
public CompiledTemplate compileTemplate(Script script, ScriptContext scriptContext) {
CompiledScript compiledScript = compile(script, scriptContext);
return params -> (BytesReference)executable(compiledScript, params).run();
return params -> (String)executable(compiledScript, params).run();
}

/**
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -116,7 +116,7 @@ public Suggestion<? extends Entry<? extends Option>> innerExecute(String name, P
vars.put(SUGGESTION_TEMPLATE_VAR_NAME, spare.toString());
QueryShardContext shardContext = suggestion.getShardContext();
final ExecutableScript executable = collateScript.apply(vars);
final BytesReference querySource = (BytesReference) executable.run();
final String querySource = (String) executable.run();
try (XContentParser parser = XContentFactory.xContent(querySource).createParser(shardContext.getXContentRegistry(),
querySource)) {
QueryBuilder innerQueryBuilder = shardContext.newParseContext(parser).parseInnerQueryBuilder();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -31,5 +31,5 @@
public interface CompiledTemplate {

/** Run a template and return the resulting string, encoded in utf8 bytes. */
BytesReference run(Map<String, Object> params);
String run(Map<String, Object> params);
}
Original file line number Diff line number Diff line change
Expand Up @@ -1156,7 +1156,7 @@ public void setNextVar(String name, Object value) {

@Override
public Object run() {
return new BytesArray(result);
return result;
}
};
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@
import org.elasticsearch.search.lookup.SearchLookup;

import java.io.Reader;
import java.io.StringWriter;
import java.lang.ref.SoftReference;
import java.security.AccessController;
import java.security.PrivilegedAction;
Expand All @@ -58,21 +59,6 @@ public final class MustacheScriptEngine implements ScriptEngine {

public static final String NAME = "mustache";

/** Thread local UTF8StreamWriter to store template execution results in, thread local to save object creation.*/
private static ThreadLocal<SoftReference<UTF8StreamWriter>> utf8StreamWriter = new ThreadLocal<>();

/** If exists, reset and return, otherwise create, reset and return a writer.*/
private static UTF8StreamWriter utf8StreamWriter() {
SoftReference<UTF8StreamWriter> ref = utf8StreamWriter.get();
UTF8StreamWriter writer = (ref == null) ? null : ref.get();
if (writer == null) {
writer = new UTF8StreamWriter(1024 * 4);
utf8StreamWriter.set(new SoftReference<>(writer));
}
writer.reset();
return writer;
}

/**
* Compile a template string to (in this case) a Mustache object than can
* later be re-used for execution to fill in missing parameter values.
Expand Down Expand Up @@ -146,8 +132,8 @@ public void setNextVar(String name, Object value) {

@Override
public Object run() {
final BytesStreamOutput result = new BytesStreamOutput();
try (UTF8StreamWriter writer = utf8StreamWriter().setOutput(result)) {
final StringWriter writer = new StringWriter();
try {
// crazy reflection here
SpecialPermission.check();
AccessController.doPrivileged((PrivilegedAction<Void>) () -> {
Expand All @@ -158,7 +144,7 @@ public Object run() {
logger.error((Supplier<?>) () -> new ParameterizedMessage("Error running {}", template), e);
throw new GeneralScriptException("Error running " + template, e);
}
return result.bytes();
return writer.toString();
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@
import org.elasticsearch.action.support.ActionFilters;
import org.elasticsearch.action.support.HandledTransportAction;
import org.elasticsearch.cluster.metadata.IndexNameExpressionResolver;
import org.elasticsearch.common.bytes.BytesArray;
import org.elasticsearch.common.bytes.BytesReference;
import org.elasticsearch.common.inject.Inject;
import org.elasticsearch.common.settings.Settings;
Expand Down Expand Up @@ -102,11 +103,10 @@ static SearchRequest convert(SearchTemplateRequest searchTemplateRequest, Search
Script script = new Script(searchTemplateRequest.getScriptType(), TEMPLATE_LANG, searchTemplateRequest.getScript(),
searchTemplateRequest.getScriptParams() == null ? Collections.emptyMap() : searchTemplateRequest.getScriptParams());
CompiledTemplate compiledScript = scriptService.compileTemplate(script, SEARCH);
BytesReference source = compiledScript.run(script.getParams());
response.setSource(source);
String source = compiledScript.run(script.getParams());
response.setSource(new BytesArray(source));

SearchRequest searchRequest = searchTemplateRequest.getRequest();
response.setSource(source);
if (searchTemplateRequest.isSimulate()) {
return null;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -68,8 +68,7 @@ public void testJsonEscapeEncoder() {
CompiledScript compiled = new CompiledScript(INLINE, null, MustacheScriptEngine.NAME, script);

ExecutableScript executable = engine.executable(compiled, singletonMap("value", "a \"value\""));
BytesReference result = (BytesReference) executable.run();
assertThat(result.utf8ToString(), equalTo("{\"field\": \"a \\\"value\\\"\"}"));
assertThat(executable.run(), equalTo("{\"field\": \"a \\\"value\\\"\"}"));
}

public void testDefaultEncoder() {
Expand All @@ -80,8 +79,7 @@ public void testDefaultEncoder() {
CompiledScript compiled = new CompiledScript(INLINE, null, MustacheScriptEngine.NAME, script);

ExecutableScript executable = engine.executable(compiled, singletonMap("value", "a \"value\""));
BytesReference result = (BytesReference) executable.run();
assertThat(result.utf8ToString(), equalTo("{\"field\": \"a \"value\"\"}"));
assertThat(executable.run(), equalTo("{\"field\": \"a \"value\"\"}"));
}

public void testUrlEncoder() {
Expand All @@ -92,7 +90,6 @@ public void testUrlEncoder() {
CompiledScript compiled = new CompiledScript(INLINE, null, MustacheScriptEngine.NAME, script);

ExecutableScript executable = engine.executable(compiled, singletonMap("value", "tilde~ AND date:[2016 FROM*]"));
BytesReference result = (BytesReference) executable.run();
assertThat(result.utf8ToString(), equalTo("{\"field\": \"tilde%7E+AND+date%3A%5B2016+FROM*%5D\"}"));
assertThat(executable.run(), equalTo("{\"field\": \"tilde%7E+AND+date%3A%5B2016+FROM*%5D\"}"));
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -58,23 +58,23 @@ public void testSimpleParameterReplace() {
+ "\"negative\": {\"term\": {\"body\": {\"value\": \"solr\"}" + "}}, \"negative_boost\": {{boost_val}} } }}";
Map<String, Object> vars = new HashMap<>();
vars.put("boost_val", "0.3");
BytesReference o = (BytesReference) qe.executable(new CompiledScript(ScriptType.INLINE, "", "mustache",
String o = (String) qe.executable(new CompiledScript(ScriptType.INLINE, "", "mustache",
qe.compile(null, template, compileParams)), vars).run();
assertEquals("GET _search {\"query\": {\"boosting\": {\"positive\": {\"match\": {\"body\": \"gift\"}},"
+ "\"negative\": {\"term\": {\"body\": {\"value\": \"solr\"}}}, \"negative_boost\": 0.3 } }}",
o.utf8ToString());
o);
}
{
String template = "GET _search {\"query\": " + "{\"boosting\": {" + "\"positive\": {\"match\": {\"body\": \"gift\"}},"
+ "\"negative\": {\"term\": {\"body\": {\"value\": \"{{body_val}}\"}" + "}}, \"negative_boost\": {{boost_val}} } }}";
Map<String, Object> vars = new HashMap<>();
vars.put("boost_val", "0.3");
vars.put("body_val", "\"quick brown\"");
BytesReference o = (BytesReference) qe.executable(new CompiledScript(ScriptType.INLINE, "", "mustache",
String o = (String) qe.executable(new CompiledScript(ScriptType.INLINE, "", "mustache",
qe.compile(null, template, compileParams)), vars).run();
assertEquals("GET _search {\"query\": {\"boosting\": {\"positive\": {\"match\": {\"body\": \"gift\"}},"
+ "\"negative\": {\"term\": {\"body\": {\"value\": \"\\\"quick brown\\\"\"}}}, \"negative_boost\": 0.3 } }}",
o.utf8ToString());
o);
}
}

Expand All @@ -89,7 +89,7 @@ public void testSimple() throws IOException {
CompiledScript compiledScript = new CompiledScript(ScriptType.INLINE, null, "mustache",
qe.compile(null, script.getIdOrCode(), Collections.emptyMap()));
ExecutableScript executableScript = qe.executable(compiledScript, script.getParams());
assertThat(((BytesReference) executableScript.run()).utf8ToString(), equalTo("{\"match_all\":{}}"));
assertThat(executableScript.run(), equalTo("{\"match_all\":{}}"));
}

public void testParseTemplateAsSingleStringWithConditionalClause() throws IOException {
Expand All @@ -105,7 +105,7 @@ public void testParseTemplateAsSingleStringWithConditionalClause() throws IOExce
CompiledScript compiledScript = new CompiledScript(ScriptType.INLINE, null, "mustache",
qe.compile(null, script.getIdOrCode(), Collections.emptyMap()));
ExecutableScript executableScript = qe.executable(compiledScript, script.getParams());
assertThat(((BytesReference) executableScript.run()).utf8ToString(), equalTo("{ \"match_all\":{} }"));
assertThat(executableScript.run(), equalTo("{ \"match_all\":{} }"));
}

public void testEscapeJson() throws IOException {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -71,7 +71,7 @@ public void testBasics() {
"Mustache templating broken",
"GET _search {\"query\": {\"boosting\": {\"positive\": {\"match\": {\"body\": \"gift\"}},"
+ "\"negative\": {\"term\": {\"body\": {\"value\": \"solr\"}}}, \"negative_boost\": 0.2 } }}",
((BytesReference) result.run()).utf8ToString()
result.run()
);
}

Expand All @@ -83,22 +83,16 @@ public void testArrayAccess() throws Exception {
new String[] { "foo", "bar" },
Arrays.asList("foo", "bar"));
vars.put("data", data);
Object output = engine.executable(mustache, vars).run();
assertThat(output, notNullValue());
assertThat(output, instanceOf(BytesReference.class));
BytesReference bytes = (BytesReference) output;
assertThat(bytes.utf8ToString(), equalTo("foo bar"));
assertThat(engine.executable(mustache, vars).run(), equalTo("foo bar"));

// Sets can come out in any order
Set<String> setData = new HashSet<>();
setData.add("foo");
setData.add("bar");
vars.put("data", setData);
output = engine.executable(mustache, vars).run();
assertThat(output, notNullValue());
assertThat(output, instanceOf(BytesReference.class));
bytes = (BytesReference) output;
assertThat(bytes.utf8ToString(), both(containsString("foo")).and(containsString("bar")));
Object output = engine.executable(mustache, vars).run();
assertThat(output, instanceOf(String.class));
assertThat((String)output, both(containsString("foo")).and(containsString("bar")));
}

public void testArrayInArrayAccess() throws Exception {
Expand All @@ -111,11 +105,7 @@ public void testArrayInArrayAccess() throws Exception {
singleton(new String[] { "foo", "bar" })
);
vars.put("data", data);
Object output = engine.executable(mustache, vars).run();
assertThat(output, notNullValue());
assertThat(output, instanceOf(BytesReference.class));
BytesReference bytes = (BytesReference) output;
assertThat(bytes.utf8ToString(), equalTo("foo bar"));
assertThat(engine.executable(mustache, vars).run(), equalTo("foo bar"));
}

public void testMapInArrayAccess() throws Exception {
Expand All @@ -126,22 +116,16 @@ public void testMapInArrayAccess() throws Exception {
new Object[] { singletonMap("key", "foo"), singletonMap("key", "bar") },
Arrays.asList(singletonMap("key", "foo"), singletonMap("key", "bar")));
vars.put("data", data);
Object output = engine.executable(mustache, vars).run();
assertThat(output, notNullValue());
assertThat(output, instanceOf(BytesReference.class));
BytesReference bytes = (BytesReference) output;
assertThat(bytes.utf8ToString(), equalTo("foo bar"));
assertThat(engine.executable(mustache, vars).run(), equalTo("foo bar"));

// HashSet iteration order isn't fixed
Set<Object> setData = new HashSet<>();
setData.add(singletonMap("key", "foo"));
setData.add(singletonMap("key", "bar"));
vars.put("data", setData);
output = engine.executable(mustache, vars).run();
assertThat(output, notNullValue());
assertThat(output, instanceOf(BytesReference.class));
bytes = (BytesReference) output;
assertThat(bytes.utf8ToString(), both(containsString("foo")).and(containsString("bar")));
Object output = engine.executable(mustache, vars).run();
assertThat(output, instanceOf(String.class));
assertThat((String)output, both(containsString("foo")).and(containsString("bar")));
}


Expand All @@ -156,14 +140,8 @@ public void testSizeAccessForCollectionsAndArrays() throws Exception {
data.put("list", randomList);
Map<String, Object> vars = new HashMap<>();
vars.put("data", data);

Object output = engine.executable(mustache, vars).run();
assertThat(output, notNullValue());
assertThat(output, instanceOf(BytesReference.class));

BytesReference bytes = (BytesReference) output;
String expectedString = String.format(Locale.ROOT, "%s %s", randomArrayValues.length, randomList.size());
assertThat(bytes.utf8ToString(), equalTo(expectedString));
assertThat(engine.executable(mustache, vars).run(), equalTo(expectedString));
}

public void testPrimitiveToJSON() throws Exception {
Expand Down Expand Up @@ -399,9 +377,7 @@ public void testUrlEncoderWithJoin() {

private void assertScript(String script, Map<String, Object> vars, Matcher<Object> matcher) {
Object result = engine.executable(new CompiledScript(INLINE, "inline", "mustache", compile(script)), vars).run();
assertThat(result, notNullValue());
assertThat(result, instanceOf(BytesReference.class));
assertThat(((BytesReference) result).utf8ToString(), matcher);
assertThat(result, matcher);
}

private Object compile(String script) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,6 @@
import org.apache.lucene.index.LeafReaderContext;
import org.apache.lucene.search.Scorer;
import org.elasticsearch.common.Nullable;
import org.elasticsearch.common.bytes.BytesArray;
import org.elasticsearch.search.lookup.LeafSearchLookup;
import org.elasticsearch.search.lookup.SearchLookup;

Expand Down Expand Up @@ -132,7 +131,7 @@ public ExecutableScript createExecutableScript(Map<String, Object> vars) {
if (vars != null) {
context.putAll(vars);
}
return new MockExecutableScript(context, script != null ? script : ctx -> new BytesArray(source));
return new MockExecutableScript(context, script != null ? script : ctx -> source);
}

public SearchScript createSearchScript(Map<String, Object> vars, SearchLookup lookup) {
Expand Down

0 comments on commit 6ce597a

Please sign in to comment.