From a62a709eff78ccc0c244969ffb04553428716c74 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Christoph=20B=C3=BCscher?= Date: Thu, 24 May 2018 18:25:07 +0200 Subject: [PATCH] Change ScriptException return status Currently failures to compile a script usually lead to a ScriptException, which inherits the 500 INTERNAL_SERVER_ERROR from ElasticsearchException if it does not contain another root cause. Issue #12315 suggests this should be a 400 instead for template compile errors, but I assume more generally for script compilation errors. This changes ScriptException to return 400 (bad request) as the status code and changes MustacheScriptEngine to convert any internal MustacheException to the more general ScriptException. Closes #12315 --- .../script/mustache/MustacheScriptEngine.java | 16 ++++++-- .../script/mustache/MustacheTests.java | 38 ++++++++++++------- .../test/painless/16_update2.yml | 2 +- .../test/painless/20_scriptfield.yml | 2 +- .../test/reindex/35_search_failures.yml | 2 +- .../test/reindex/85_scripting.yml | 2 +- .../update_by_query/35_search_failure.yml | 2 +- .../test/update_by_query/80_scripting.yml | 2 +- .../elasticsearch/script/ScriptException.java | 22 +++++++---- 9 files changed, 57 insertions(+), 31 deletions(-) diff --git a/modules/lang-mustache/src/main/java/org/elasticsearch/script/mustache/MustacheScriptEngine.java b/modules/lang-mustache/src/main/java/org/elasticsearch/script/mustache/MustacheScriptEngine.java index 5a0b2e15460c5..b739fc1cfd087 100644 --- a/modules/lang-mustache/src/main/java/org/elasticsearch/script/mustache/MustacheScriptEngine.java +++ b/modules/lang-mustache/src/main/java/org/elasticsearch/script/mustache/MustacheScriptEngine.java @@ -19,9 +19,9 @@ package org.elasticsearch.script.mustache; import com.github.mustachejava.Mustache; +import com.github.mustachejava.MustacheException; import com.github.mustachejava.MustacheFactory; -import java.io.StringReader; import org.apache.logging.log4j.Logger; import org.apache.logging.log4j.message.ParameterizedMessage; import org.apache.logging.log4j.util.Supplier; @@ -31,12 +31,15 @@ import org.elasticsearch.script.Script; import org.elasticsearch.script.ScriptContext; import org.elasticsearch.script.ScriptEngine; +import org.elasticsearch.script.ScriptException; import org.elasticsearch.script.TemplateScript; import java.io.Reader; +import java.io.StringReader; import java.io.StringWriter; import java.security.AccessController; import java.security.PrivilegedAction; +import java.util.Collections; import java.util.Map; /** @@ -66,9 +69,14 @@ public T compile(String templateName, String templateSource, ScriptContext new MustacheExecutableScript(template, params); - return context.factoryClazz.cast(compiled); + try { + Mustache template = factory.compile(reader, "query-template"); + TemplateScript.Factory compiled = params -> new MustacheExecutableScript(template, params); + return context.factoryClazz.cast(compiled); + } catch (MustacheException ex) { + throw new ScriptException(ex.getMessage(), ex, Collections.emptyList(), templateSource, NAME); + } + } private CustomMustacheFactory createMustacheFactory(Map options) { diff --git a/modules/lang-mustache/src/test/java/org/elasticsearch/script/mustache/MustacheTests.java b/modules/lang-mustache/src/test/java/org/elasticsearch/script/mustache/MustacheTests.java index ba59e9ccac002..354d92500904c 100644 --- a/modules/lang-mustache/src/test/java/org/elasticsearch/script/mustache/MustacheTests.java +++ b/modules/lang-mustache/src/test/java/org/elasticsearch/script/mustache/MustacheTests.java @@ -18,6 +18,15 @@ */ package org.elasticsearch.script.mustache; +import org.elasticsearch.common.bytes.BytesReference; +import org.elasticsearch.common.xcontent.XContentBuilder; +import org.elasticsearch.common.xcontent.XContentHelper; +import org.elasticsearch.script.ScriptEngine; +import org.elasticsearch.script.ScriptException; +import org.elasticsearch.script.TemplateScript; +import org.elasticsearch.test.ESTestCase; +import org.hamcrest.Matcher; + import java.net.URLEncoder; import java.nio.charset.StandardCharsets; import java.util.Arrays; @@ -29,15 +38,6 @@ import java.util.Map; import java.util.Set; -import com.github.mustachejava.MustacheException; -import org.elasticsearch.common.bytes.BytesReference; -import org.elasticsearch.common.xcontent.XContentBuilder; -import org.elasticsearch.common.xcontent.XContentHelper; -import org.elasticsearch.script.ScriptEngine; -import org.elasticsearch.script.TemplateScript; -import org.elasticsearch.test.ESTestCase; -import org.hamcrest.Matcher; - import static java.util.Collections.singleton; import static java.util.Collections.singletonMap; import static org.elasticsearch.common.xcontent.XContentFactory.jsonBuilder; @@ -225,11 +225,17 @@ public void testSimpleListToJSON() throws Exception { } public void testsUnsupportedTagsToJson() { - MustacheException e = expectThrows(MustacheException.class, () -> compile("{{#toJson}}{{foo}}{{bar}}{{/toJson}}")); + final String script = "{{#toJson}}{{foo}}{{bar}}{{/toJson}}"; + ScriptException e = expectThrows(ScriptException.class, () -> compile(script)); assertThat(e.getMessage(), containsString("Mustache function [toJson] must contain one and only one identifier")); + assertEquals(MustacheScriptEngine.NAME, e.getLang()); + assertEquals(script, e.getScript()); - e = expectThrows(MustacheException.class, () -> compile("{{#toJson}}{{/toJson}}")); + final String script2 = "{{#toJson}}{{/toJson}}"; + e = expectThrows(ScriptException.class, () -> compile(script2)); assertThat(e.getMessage(), containsString("Mustache function [toJson] must contain one and only one identifier")); + assertEquals(MustacheScriptEngine.NAME, e.getLang()); + assertEquals(script2, e.getScript()); } public void testEmbeddedToJSON() throws Exception { @@ -312,11 +318,17 @@ public void testJoinWithToJson() { } public void testsUnsupportedTagsJoin() { - MustacheException e = expectThrows(MustacheException.class, () -> compile("{{#join}}{{/join}}")); + final String script = "{{#join}}{{/join}}"; + ScriptException e = expectThrows(ScriptException.class, () -> compile(script)); assertThat(e.getMessage(), containsString("Mustache function [join] must contain one and only one identifier")); + assertEquals(MustacheScriptEngine.NAME, e.getLang()); + assertEquals(script, e.getScript()); - e = expectThrows(MustacheException.class, () -> compile("{{#join delimiter='a'}}{{/join delimiter='b'}}")); + final String script2 = "{{#join delimiter='a'}}{{/join delimiter='b'}}"; + e = expectThrows(ScriptException.class, () -> compile(script2)); assertThat(e.getMessage(), containsString("Mismatched start/end tags")); + assertEquals(MustacheScriptEngine.NAME, e.getLang()); + assertEquals(script2, e.getScript()); } public void testJoinWithCustomDelimiter() { diff --git a/modules/lang-painless/src/test/resources/rest-api-spec/test/painless/16_update2.yml b/modules/lang-painless/src/test/resources/rest-api-spec/test/painless/16_update2.yml index 3d6f603d4d806..253676bda8e38 100644 --- a/modules/lang-painless/src/test/resources/rest-api-spec/test/painless/16_update2.yml +++ b/modules/lang-painless/src/test/resources/rest-api-spec/test/painless/16_update2.yml @@ -35,7 +35,7 @@ id: "non_existing" - do: - catch: request + catch: bad_request put_script: id: "1" context: "search" diff --git a/modules/lang-painless/src/test/resources/rest-api-spec/test/painless/20_scriptfield.yml b/modules/lang-painless/src/test/resources/rest-api-spec/test/painless/20_scriptfield.yml index e498a1737576e..02c17ce0e3714 100644 --- a/modules/lang-painless/src/test/resources/rest-api-spec/test/painless/20_scriptfield.yml +++ b/modules/lang-painless/src/test/resources/rest-api-spec/test/painless/20_scriptfield.yml @@ -133,7 +133,7 @@ setup: --- "Scripted Field with script error": - do: - catch: request + catch: bad_request search: body: script_fields: diff --git a/modules/reindex/src/test/resources/rest-api-spec/test/reindex/35_search_failures.yml b/modules/reindex/src/test/resources/rest-api-spec/test/reindex/35_search_failures.yml index 70e78f7e36b37..89135449d6969 100644 --- a/modules/reindex/src/test/resources/rest-api-spec/test/reindex/35_search_failures.yml +++ b/modules/reindex/src/test/resources/rest-api-spec/test/reindex/35_search_failures.yml @@ -17,7 +17,7 @@ indices.refresh: {} - do: - catch: request + catch: bad_request reindex: body: source: diff --git a/modules/reindex/src/test/resources/rest-api-spec/test/reindex/85_scripting.yml b/modules/reindex/src/test/resources/rest-api-spec/test/reindex/85_scripting.yml index 617a46dfa66b5..901f24f022cba 100644 --- a/modules/reindex/src/test/resources/rest-api-spec/test/reindex/85_scripting.yml +++ b/modules/reindex/src/test/resources/rest-api-spec/test/reindex/85_scripting.yml @@ -446,7 +446,7 @@ indices.refresh: {} - do: - catch: request + catch: bad_request reindex: refresh: true body: diff --git a/modules/reindex/src/test/resources/rest-api-spec/test/update_by_query/35_search_failure.yml b/modules/reindex/src/test/resources/rest-api-spec/test/update_by_query/35_search_failure.yml index 17f422453ce18..e7f3a146480ff 100644 --- a/modules/reindex/src/test/resources/rest-api-spec/test/update_by_query/35_search_failure.yml +++ b/modules/reindex/src/test/resources/rest-api-spec/test/update_by_query/35_search_failure.yml @@ -17,7 +17,7 @@ indices.refresh: {} - do: - catch: request + catch: bad_request update_by_query: index: source body: diff --git a/modules/reindex/src/test/resources/rest-api-spec/test/update_by_query/80_scripting.yml b/modules/reindex/src/test/resources/rest-api-spec/test/update_by_query/80_scripting.yml index 8ed94347923d1..1a3880f3d15c1 100644 --- a/modules/reindex/src/test/resources/rest-api-spec/test/update_by_query/80_scripting.yml +++ b/modules/reindex/src/test/resources/rest-api-spec/test/update_by_query/80_scripting.yml @@ -434,7 +434,7 @@ indices.refresh: {} - do: - catch: request + catch: bad_request update_by_query: index: twitter refresh: true diff --git a/server/src/main/java/org/elasticsearch/script/ScriptException.java b/server/src/main/java/org/elasticsearch/script/ScriptException.java index 726f218610833..2d5d6841618e5 100644 --- a/server/src/main/java/org/elasticsearch/script/ScriptException.java +++ b/server/src/main/java/org/elasticsearch/script/ScriptException.java @@ -1,5 +1,14 @@ package org.elasticsearch.script; +import org.elasticsearch.ElasticsearchException; +import org.elasticsearch.common.Strings; +import org.elasticsearch.common.io.stream.StreamInput; +import org.elasticsearch.common.io.stream.StreamOutput; +import org.elasticsearch.common.xcontent.ToXContent; +import org.elasticsearch.common.xcontent.XContentBuilder; +import org.elasticsearch.common.xcontent.XContentFactory; +import org.elasticsearch.rest.RestStatus; + /* * Licensed to Elasticsearch under one or more contributor * license agreements. See the NOTICE file distributed with @@ -25,14 +34,6 @@ import java.util.List; import java.util.Objects; -import org.elasticsearch.ElasticsearchException; -import org.elasticsearch.common.Strings; -import org.elasticsearch.common.io.stream.StreamInput; -import org.elasticsearch.common.io.stream.StreamOutput; -import org.elasticsearch.common.xcontent.ToXContent; -import org.elasticsearch.common.xcontent.XContentBuilder; -import org.elasticsearch.common.xcontent.XContentFactory; - /** * Exception from a scripting engine. *

@@ -132,4 +133,9 @@ public String toJsonString() { throw new RuntimeException(e); } } + + @Override + public RestStatus status() { + return RestStatus.BAD_REQUEST; + } }