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 elastic#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 elastic#12315

Author: Christoph Büscher

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> T compile(String templateName, String templateSource, ScriptContext<T
         }
         final MustacheFactory factory = createMustacheFactory(options);
         Reader reader = new StringReader(templateSource);
-        Mustache template = factory.compile(reader, "query-template");
-        TemplateScript.Factory compiled = params -> 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<String, String> options) {

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() {

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"

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:

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:

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:

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:

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

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.
  * <p>
@@ -132,4 +133,9 @@ public String toJsonString() {
             throw new RuntimeException(e);
         }
     }
+
+    @Override
+    public RestStatus status() {
+        return RestStatus.BAD_REQUEST;
+    }
 }