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 1/5] 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; + } } From 4204d1ca3c39b03523ddb84a09276082dc350426 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Christoph=20B=C3=BCscher?= Date: Fri, 25 May 2018 13:40:30 +0200 Subject: [PATCH 2/5] Fix docs test --- docs/painless/painless-debugging.asciidoc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/painless/painless-debugging.asciidoc b/docs/painless/painless-debugging.asciidoc index a909593ff1745..8523116616d18 100644 --- a/docs/painless/painless-debugging.asciidoc +++ b/docs/painless/painless-debugging.asciidoc @@ -48,7 +48,7 @@ Which shows that the class of `doc.first` is "java_class": "org.elasticsearch.index.fielddata.ScriptDocValues$Longs", ... }, - "status": 500 + "status": 400 } --------------------------------------------------------- // TESTRESPONSE[s/\.\.\./"script_stack": $body.error.script_stack, "script": $body.error.script, "lang": $body.error.lang, "caused_by": $body.error.caused_by, "root_cause": $body.error.root_cause, "reason": $body.error.reason/] From 0f6bc12e31ec5ef53354085eea3a94aef6f327b8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Christoph=20B=C3=BCscher?= Date: Fri, 25 May 2018 15:59:15 +0200 Subject: [PATCH 3/5] Fixing another qa smoke test --- .../test/ingest/10_pipeline_with_mustache_templates.yml | 6 +++--- .../test/ingest/50_script_processor_using_painless.yml | 2 +- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/qa/smoke-test-ingest-with-all-dependencies/src/test/resources/rest-api-spec/test/ingest/10_pipeline_with_mustache_templates.yml b/qa/smoke-test-ingest-with-all-dependencies/src/test/resources/rest-api-spec/test/ingest/10_pipeline_with_mustache_templates.yml index 0e54ff0b7ad59..6bc6219bfe59d 100644 --- a/qa/smoke-test-ingest-with-all-dependencies/src/test/resources/rest-api-spec/test/ingest/10_pipeline_with_mustache_templates.yml +++ b/qa/smoke-test-ingest-with-all-dependencies/src/test/resources/rest-api-spec/test/ingest/10_pipeline_with_mustache_templates.yml @@ -332,7 +332,7 @@ wait_for_status: green - do: - catch: request + catch: bad_request ingest.put_pipeline: id: "my_pipeline_1" body: > @@ -348,5 +348,5 @@ ] } - match: { error.header.processor_type: "set" } - - match: { error.type: "general_script_exception" } - - match: { error.reason: "Failed to compile inline script [{{#join}}{{/join}}] using lang [mustache]" } + - match: { error.type: "script_exception" } + - match: { error.reason: "Mustache function [join] must contain one and only one identifier" } diff --git a/qa/smoke-test-ingest-with-all-dependencies/src/test/resources/rest-api-spec/test/ingest/50_script_processor_using_painless.yml b/qa/smoke-test-ingest-with-all-dependencies/src/test/resources/rest-api-spec/test/ingest/50_script_processor_using_painless.yml index 8c6a94b4a5c49..1c027adcc8071 100644 --- a/qa/smoke-test-ingest-with-all-dependencies/src/test/resources/rest-api-spec/test/ingest/50_script_processor_using_painless.yml +++ b/qa/smoke-test-ingest-with-all-dependencies/src/test/resources/rest-api-spec/test/ingest/50_script_processor_using_painless.yml @@ -89,7 +89,7 @@ --- "Test script processor with syntax error in inline script": - do: - catch: request + catch: bad_request ingest.put_pipeline: id: "my_pipeline" body: > From 84d2763c90b76ccda285f14fad2ad3f964f9fe7c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Christoph=20B=C3=BCscher?= Date: Sat, 26 May 2018 18:57:44 +0200 Subject: [PATCH 4/5] Fix x-pack qa test --- .../resources/rest-api-spec/test/painless/40_exception.yml | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/x-pack/qa/smoke-test-watcher/src/test/resources/rest-api-spec/test/painless/40_exception.yml b/x-pack/qa/smoke-test-watcher/src/test/resources/rest-api-spec/test/painless/40_exception.yml index a794316e97d9b..74a7d6eefb077 100644 --- a/x-pack/qa/smoke-test-watcher/src/test/resources/rest-api-spec/test/painless/40_exception.yml +++ b/x-pack/qa/smoke-test-watcher/src/test/resources/rest-api-spec/test/painless/40_exception.yml @@ -5,7 +5,7 @@ wait_for_status: green - do: - catch: request + catch: bad_request xpack.watcher.put_watch: id: "my_exe_watch" body: > @@ -33,7 +33,7 @@ } - is_true: error.script_stack - - match: { status: 500 } + - match: { status: 400 } --- "Test painless exceptions are returned when logging a broken response": From 54e4e280655241fa1eaa65cfe1dfb8cc6c9ec1c9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Christoph=20B=C3=BCscher?= Date: Wed, 30 May 2018 11:30:47 +0200 Subject: [PATCH 5/5] Add notes to migration docs --- docs/reference/migration/migrate_7_0/scripting.asciidoc | 6 ++++++ docs/reference/migration/migrate_7_0/search.asciidoc | 2 +- 2 files changed, 7 insertions(+), 1 deletion(-) diff --git a/docs/reference/migration/migrate_7_0/scripting.asciidoc b/docs/reference/migration/migrate_7_0/scripting.asciidoc index df43aaa92eadf..79380f84204ed 100644 --- a/docs/reference/migration/migrate_7_0/scripting.asciidoc +++ b/docs/reference/migration/migrate_7_0/scripting.asciidoc @@ -11,3 +11,9 @@ the getter methods for date objects were deprecated. These methods have now been removed. Instead, use `.value` on `date` fields, or explicitly parse `long` fields into a date object using `Instance.ofEpochMillis(doc["myfield"].value)`. + +==== Script errors will return as `400` error codes + +Malformed scripts, either in search templates, ingest pipelines or search +requests, return `400 - Bad request` while they would previously return +`500 - Internal Server Error`. This also applies for stored scripts. diff --git a/docs/reference/migration/migrate_7_0/search.asciidoc b/docs/reference/migration/migrate_7_0/search.asciidoc index 1fe0bc62418bb..7505b6f14d1b4 100644 --- a/docs/reference/migration/migrate_7_0/search.asciidoc +++ b/docs/reference/migration/migrate_7_0/search.asciidoc @@ -43,7 +43,7 @@ The Search API returns `400 - Bad request` while it would previously return * the number of slices is too large * keep alive for scroll is too large * number of filters in the adjacency matrix aggregation is too large - +* script compilation errors ==== Scroll queries cannot use the `request_cache` anymore