honeymoose
/
OpenSearch
mirror of https://github.com/honeymoose/OpenSearch.git
1
0
Fork 0
Code Issues Packages Projects Releases Wiki Activity

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
This commit is contained in:
Christoph Büscher 2018-05-30 14:00:07 +02:00 committed by GitHub
parent 7c5abc0a6b
commit 1ea9f11b03
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
15 changed files with 71 additions and 39 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

2
docs/painless/painless-debugging.asciidoc
View File

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

6
docs/reference/migration/migrate_7_0/scripting.asciidoc
View File

@ -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.

2
docs/reference/migration/migrate_7_0/search.asciidoc
View File

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

16
modules/lang-mustache/src/main/java/org/elasticsearch/script/mustache/MustacheScriptEngine.java
View File

@ -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.GeneralScriptException;
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 final class MustacheScriptEngine implements ScriptEngine {
}
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) {

38
modules/lang-mustache/src/test/java/org/elasticsearch/script/mustache/MustacheTests.java
View File

@ -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.Locale;
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 class MustacheTests extends ESTestCase {
}
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 class MustacheTests extends ESTestCase {
}
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() {

2
modules/lang-painless/src/test/resources/rest-api-spec/test/painless/16_update2.yml
View File

@ -35,7 +35,7 @@
id: "non_existing"
- do:
catch: request
catch: bad_request
put_script:
id: "1"
context: "search"

2
modules/lang-painless/src/test/resources/rest-api-spec/test/painless/20_scriptfield.yml
View File

@ -133,7 +133,7 @@ setup:
---
"Scripted Field with script error":
- do:
catch: request
catch: bad_request
search:
body:
script_fields:

2
modules/reindex/src/test/resources/rest-api-spec/test/reindex/35_search_failures.yml
View File

@ -17,7 +17,7 @@
indices.refresh: {}
- do:
catch: request
catch: bad_request
reindex:
body:
source:

2
modules/reindex/src/test/resources/rest-api-spec/test/reindex/85_scripting.yml
View File

@ -446,7 +446,7 @@
indices.refresh: {}
- do:
catch: request
catch: bad_request
reindex:
refresh: true
body:

2
modules/reindex/src/test/resources/rest-api-spec/test/update_by_query/35_search_failure.yml
View File

@ -17,7 +17,7 @@
indices.refresh: {}
- do:
catch: request
catch: bad_request
update_by_query:
index: source
body:

2
modules/reindex/src/test/resources/rest-api-spec/test/update_by_query/80_scripting.yml
View File

@ -434,7 +434,7 @@
indices.refresh: {}
- do:
catch: request
catch: bad_request
update_by_query:
index: twitter
refresh: true

6
qa/smoke-test-ingest-with-all-dependencies/src/test/resources/rest-api-spec/test/ingest/10_pipeline_with_mustache_templates.yml
View File

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

2
qa/smoke-test-ingest-with-all-dependencies/src/test/resources/rest-api-spec/test/ingest/50_script_processor_using_painless.yml
View File

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

22
server/src/main/java/org/elasticsearch/script/ScriptException.java
View File

@ -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.Collections;
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 class ScriptException extends ElasticsearchException {
throw new RuntimeException(e);
}
}
@Override
public RestStatus status() {
return RestStatus.BAD_REQUEST;
}
}

4
x-pack/qa/smoke-test-watcher/src/test/resources/rest-api-spec/test/painless/40_exception.yml
View File

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