Change ScriptException status to 400 (bad request) (#30861)

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. Instead, this should be a 400 Bad Request error.
This PR changes this more generally for script compilation errors by changing 
ScriptException to return 400 (bad request) as status code.

Closes #12315

Author: Christoph Büscher

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/]

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.

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
 

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

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" }

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:  >

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;
+    }
 }

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":