From f03780711712b08b459f1d5d23b70c34e8e4b35f Mon Sep 17 00:00:00 2001 From: Robert Muir Date: Thu, 26 May 2016 11:43:29 -0400 Subject: [PATCH 1/2] replace ScriptException with a better one --- .../elasticsearch/ElasticsearchException.java | 41 +------ .../search/function/ScriptScoreFunction.java | 4 +- .../xcontent/json/JsonXContentGenerator.java | 2 +- .../script/GeneralScriptException.java | 51 ++++++++ .../elasticsearch/script/ScriptException.java | 116 ++++++++++++++++-- .../elasticsearch/script/ScriptService.java | 15 ++- .../org/elasticsearch/ESExceptionTests.java | 24 +++- .../ExceptionSerializationTests.java | 3 +- .../function/ScriptScoreFunctionTests.java | 4 +- .../script/ScriptContextTests.java | 4 +- .../script/ScriptExceptionTests.java | 99 +++++++++++++++ .../script/ScriptServiceTests.java | 2 +- .../ExpressionExecutableScript.java | 10 +- .../ExpressionScriptEngineService.java | 65 ++++++---- .../expression/ExpressionSearchScript.java | 6 +- .../script/expression/ExpressionTests.java | 56 +++++---- .../expression/MoreExpressionTests.java | 10 +- .../groovy/GroovyScriptEngineService.java | 10 +- .../script/groovy/GroovySecurityTests.java | 4 +- .../mustache/MustacheScriptEngineService.java | 4 +- .../org/elasticsearch/painless/Compiler.java | 46 +++---- .../elasticsearch/painless/Executable.java | 23 +++- .../elasticsearch/painless/MethodWriter.java | 32 ++++- .../elasticsearch/painless/ScriptImpl.java | 61 ++++++++- .../org/elasticsearch/painless/Writer.java | 13 +- .../painless/WriterConstants.java | 3 +- .../elasticsearch/painless/antlr/Walker.java | 2 +- .../elasticsearch/painless/node/ANode.java | 14 --- .../elasticsearch/painless/node/EBinary.java | 1 + .../elasticsearch/painless/node/ECast.java | 1 + .../elasticsearch/painless/node/EChain.java | 4 + .../elasticsearch/painless/node/EComp.java | 1 + .../painless/node/EConditional.java | 1 + .../elasticsearch/painless/node/EUnary.java | 1 + .../painless/node/LArrayLength.java | 1 + .../elasticsearch/painless/node/LBrace.java | 2 + .../elasticsearch/painless/node/LCall.java | 1 + .../elasticsearch/painless/node/LCast.java | 1 + .../painless/node/LDefArray.java | 2 + .../elasticsearch/painless/node/LDefCall.java | 1 + .../painless/node/LDefField.java | 2 + .../elasticsearch/painless/node/LField.java | 2 + .../painless/node/LListShortcut.java | 2 + .../painless/node/LMapShortcut.java | 2 + .../painless/node/LNewArray.java | 1 + .../elasticsearch/painless/node/LNewObj.java | 1 + .../painless/node/LShortcut.java | 2 + .../elasticsearch/painless/node/SBreak.java | 2 - .../elasticsearch/painless/node/SCatch.java | 3 +- .../painless/node/SContinue.java | 2 - .../painless/node/SDeclaration.java | 3 +- .../org/elasticsearch/painless/node/SDo.java | 5 +- .../painless/node/SExpression.java | 3 +- .../org/elasticsearch/painless/node/SFor.java | 7 +- .../org/elasticsearch/painless/node/SIf.java | 3 +- .../elasticsearch/painless/node/SIfElse.java | 3 +- .../elasticsearch/painless/node/SReturn.java | 3 +- .../elasticsearch/painless/node/SThrow.java | 3 +- .../org/elasticsearch/painless/node/STry.java | 3 +- .../elasticsearch/painless/node/SWhile.java | 7 +- .../painless/CompoundAssignmentTests.java | 25 ++-- .../painless/DefOperationTests.java | 8 +- .../painless/DefOptimizationTests.java | 2 +- .../elasticsearch/painless/DivisionTests.java | 28 ++--- .../elasticsearch/painless/OverloadTests.java | 2 +- .../painless/RemainderTests.java | 14 +-- .../painless/ScriptTestCase.java | 27 +++- .../elasticsearch/painless/StringTests.java | 32 ++--- .../elasticsearch/painless/TryCatchTests.java | 4 +- .../painless/WhenThingsGoWrongTests.java | 49 ++++---- .../test/reindex/40_search_failures.yaml | 2 +- .../update_by_query/40_search_failure.yaml | 2 +- 72 files changed, 688 insertions(+), 307 deletions(-) create mode 100644 core/src/main/java/org/elasticsearch/script/GeneralScriptException.java create mode 100644 core/src/test/java/org/elasticsearch/script/ScriptExceptionTests.java diff --git a/core/src/main/java/org/elasticsearch/ElasticsearchException.java b/core/src/main/java/org/elasticsearch/ElasticsearchException.java index 1faf6cd4237..ad5a2e79cd1 100644 --- a/core/src/main/java/org/elasticsearch/ElasticsearchException.java +++ b/core/src/main/java/org/elasticsearch/ElasticsearchException.java @@ -200,41 +200,6 @@ public class ElasticsearchException extends RuntimeException implements ToXConte return rootCause; } - /** - * Check whether this exception contains an exception of the given type: - * either it is of the given class itself or it contains a nested cause - * of the given type. - * - * @param exType the exception type to look for - * @return whether there is a nested exception of the specified type - */ - public boolean contains(Class exType) { - if (exType == null) { - return false; - } - if (exType.isInstance(this)) { - return true; - } - Throwable cause = getCause(); - if (cause == this) { - return false; - } - if (cause instanceof ElasticsearchException) { - return ((ElasticsearchException) cause).contains(exType); - } else { - while (cause != null) { - if (exType.isInstance(cause)) { - return true; - } - if (cause.getCause() == cause) { - break; - } - cause = cause.getCause(); - } - return false; - } - } - @Override public void writeTo(StreamOutput out) throws IOException { out.writeOptionalString(this.getMessage()); @@ -531,7 +496,8 @@ public class ElasticsearchException extends RuntimeException implements ToXConte org.elasticsearch.index.shard.IndexShardStartedException::new, 23), SEARCH_CONTEXT_MISSING_EXCEPTION(org.elasticsearch.search.SearchContextMissingException.class, org.elasticsearch.search.SearchContextMissingException::new, 24), - SCRIPT_EXCEPTION(org.elasticsearch.script.ScriptException.class, org.elasticsearch.script.ScriptException::new, 25), + GENERAL_SCRIPT_EXCEPTION(org.elasticsearch.script.GeneralScriptException.class, + org.elasticsearch.script.GeneralScriptException::new, 25), BATCH_OPERATION_EXCEPTION(org.elasticsearch.index.shard.TranslogRecoveryPerformer.BatchOperationException.class, org.elasticsearch.index.shard.TranslogRecoveryPerformer.BatchOperationException::new, 26), SNAPSHOT_CREATION_EXCEPTION(org.elasticsearch.snapshots.SnapshotCreationException.class, @@ -741,7 +707,8 @@ public class ElasticsearchException extends RuntimeException implements ToXConte QUERY_SHARD_EXCEPTION(org.elasticsearch.index.query.QueryShardException.class, org.elasticsearch.index.query.QueryShardException::new, 141), NO_LONGER_PRIMARY_SHARD_EXCEPTION(ShardStateAction.NoLongerPrimaryShardException.class, - ShardStateAction.NoLongerPrimaryShardException::new, 142); + ShardStateAction.NoLongerPrimaryShardException::new, 142), + SCRIPT_EXCEPTION(org.elasticsearch.script.ScriptException.class, org.elasticsearch.script.ScriptException::new, 143); final Class exceptionClass; diff --git a/core/src/main/java/org/elasticsearch/common/lucene/search/function/ScriptScoreFunction.java b/core/src/main/java/org/elasticsearch/common/lucene/search/function/ScriptScoreFunction.java index 82454f3dd62..47b85250735 100644 --- a/core/src/main/java/org/elasticsearch/common/lucene/search/function/ScriptScoreFunction.java +++ b/core/src/main/java/org/elasticsearch/common/lucene/search/function/ScriptScoreFunction.java @@ -26,7 +26,7 @@ import org.apache.lucene.search.Scorer; import org.elasticsearch.script.ExplainableSearchScript; import org.elasticsearch.script.LeafSearchScript; import org.elasticsearch.script.Script; -import org.elasticsearch.script.ScriptException; +import org.elasticsearch.script.GeneralScriptException; import org.elasticsearch.script.SearchScript; import java.io.IOException; @@ -87,7 +87,7 @@ public class ScriptScoreFunction extends ScoreFunction { scorer.score = subQueryScore; double result = leafScript.runAsDouble(); if (Double.isNaN(result)) { - throw new ScriptException("script_score returned NaN"); + throw new GeneralScriptException("script_score returned NaN"); } return result; } diff --git a/core/src/main/java/org/elasticsearch/common/xcontent/json/JsonXContentGenerator.java b/core/src/main/java/org/elasticsearch/common/xcontent/json/JsonXContentGenerator.java index 462288f688a..56baf30c9e0 100644 --- a/core/src/main/java/org/elasticsearch/common/xcontent/json/JsonXContentGenerator.java +++ b/core/src/main/java/org/elasticsearch/common/xcontent/json/JsonXContentGenerator.java @@ -100,7 +100,7 @@ public class JsonXContentGenerator implements XContentGenerator { @Override public final void usePrettyPrint() { - generator.setPrettyPrinter(new DefaultPrettyPrinter().withObjectIndenter(INDENTER)); + generator.setPrettyPrinter(new DefaultPrettyPrinter().withObjectIndenter(INDENTER).withArrayIndenter(INDENTER)); prettyPrint = true; } diff --git a/core/src/main/java/org/elasticsearch/script/GeneralScriptException.java b/core/src/main/java/org/elasticsearch/script/GeneralScriptException.java new file mode 100644 index 00000000000..72820622772 --- /dev/null +++ b/core/src/main/java/org/elasticsearch/script/GeneralScriptException.java @@ -0,0 +1,51 @@ +/* + * Licensed to Elasticsearch under one or more contributor + * license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright + * ownership. Elasticsearch licenses this file to you under + * the Apache License, Version 2.0 (the "License"); you may + * not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +package org.elasticsearch.script; + +import org.elasticsearch.ElasticsearchException; +import org.elasticsearch.common.io.stream.StreamInput; + +import java.io.IOException; + +/** + * Simple exception class from a script. + *

+ * Use of this exception should generally be avoided, it doesn't provide + * much context or structure to users trying to debug scripting when + * things go wrong. + * @deprecated Use ScriptException for exceptions from the scripting engine, + * otherwise use a more appropriate exception (e.g. if thrown + * from various abstractions) + */ +@Deprecated +public class GeneralScriptException extends ElasticsearchException { + + public GeneralScriptException(String msg) { + super(msg); + } + + public GeneralScriptException(String msg, Throwable cause) { + super(msg, cause); + } + + public GeneralScriptException(StreamInput in) throws IOException{ + super(in); + } +} diff --git a/core/src/main/java/org/elasticsearch/script/ScriptException.java b/core/src/main/java/org/elasticsearch/script/ScriptException.java index 3cc3560da7c..475091f9f6d 100644 --- a/core/src/main/java/org/elasticsearch/script/ScriptException.java +++ b/core/src/main/java/org/elasticsearch/script/ScriptException.java @@ -1,3 +1,5 @@ +package org.elasticsearch.script; + /* * Licensed to Elasticsearch under one or more contributor * license agreements. See the NOTICE file distributed with @@ -17,27 +19,117 @@ * under the License. */ -package org.elasticsearch.script; +import java.io.IOException; +import java.util.Arrays; +import java.util.Collections; +import java.util.List; +import java.util.Objects; import org.elasticsearch.ElasticsearchException; import org.elasticsearch.common.io.stream.StreamInput; - -import java.io.IOException; +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. + *

+ * A ScriptException has the following components: + *

*/ +@SuppressWarnings("serial") public class ScriptException extends ElasticsearchException { - - public ScriptException(String msg) { - super(msg); + private final List scriptStack; + private final String script; + private final String lang; + + /** + * Create a new ScriptException. + * @param message A short and simple summary of what happened, such as "compile error". + * Must not be {@code null}. + * @param cause The underlying cause of the exception. Must not be {@code null}. + * @param scriptStack An implementation-specific "stacktrace" for the error in the script. + * Must not be {@code null}, but can be empty (though this should be avoided if possible). + * @param script Identifier for which script failed. Must not be {@code null}. + * @param lang Scripting engine language, such as "painless". Must not be {@code null}. + * @throws NullPointerException if any parameters are {@code null}. + */ + public ScriptException(String message, Throwable cause, List scriptStack, String script, String lang) { + super(Objects.requireNonNull(message), Objects.requireNonNull(cause)); + this.scriptStack = Collections.unmodifiableList(Objects.requireNonNull(scriptStack)); + this.script = Objects.requireNonNull(script); + this.lang = Objects.requireNonNull(lang); } - public ScriptException(String msg, Throwable cause) { - super(msg, cause); - } - - public ScriptException(StreamInput in) throws IOException{ + /** + * Deserializes a ScriptException from a {@code StreamInput} + */ + public ScriptException(StreamInput in) throws IOException { super(in); + scriptStack = Arrays.asList(in.readStringArray()); + script = in.readString(); + lang = in.readString(); + } + + @Override + public void writeTo(StreamOutput out) throws IOException { + super.writeTo(out); + out.writeStringArray(scriptStack.toArray(new String[0])); + out.writeString(script); + out.writeString(lang); + } + + @Override + protected void innerToXContent(XContentBuilder builder, Params params) throws IOException { + super.innerToXContent(builder, params); + builder.field("script_stack", scriptStack); + builder.field("script", script); + builder.field("lang", lang); + } + + /** + * Returns the stacktrace for the error in the script. + * @return a read-only list of frames, which may be empty. + */ + public List getScriptStack() { + return scriptStack; + } + + /** + * Returns the identifier for which script. + * @return script's name or source text that identifies the script. + */ + public String getScript() { + return script; + } + + /** + * Returns the language of the script. + * @return the {@code lang} parameter of the scripting engine. + */ + public String getLang() { + return lang; + } + + /** + * Returns a JSON version of this exception for debugging. + */ + public String toJsonString() { + try { + XContentBuilder json = XContentFactory.jsonBuilder().prettyPrint(); + json.startObject(); + toXContent(json, ToXContent.EMPTY_PARAMS); + json.endObject(); + return json.string(); + } catch (IOException e) { + throw new RuntimeException(e); + } } } diff --git a/core/src/main/java/org/elasticsearch/script/ScriptService.java b/core/src/main/java/org/elasticsearch/script/ScriptService.java index cf0ba1ca941..f5ee456c119 100644 --- a/core/src/main/java/org/elasticsearch/script/ScriptService.java +++ b/core/src/main/java/org/elasticsearch/script/ScriptService.java @@ -31,7 +31,6 @@ import org.elasticsearch.cluster.AckedClusterStateUpdateTask; import org.elasticsearch.cluster.ClusterState; import org.elasticsearch.cluster.metadata.MetaData; import org.elasticsearch.cluster.service.ClusterService; -import org.elasticsearch.common.Nullable; import org.elasticsearch.common.ParseField; import org.elasticsearch.common.ParseFieldMatcher; import org.elasticsearch.common.Strings; @@ -236,7 +235,7 @@ public class ScriptService extends AbstractComponent implements Closeable { ScriptEngineService scriptEngineService = getScriptEngineServiceForLang(lang); if (canExecuteScript(lang, scriptEngineService, script.getType(), scriptContext) == false) { - throw new ScriptException("scripts of type [" + script.getType() + "], operation [" + scriptContext.getKey() + "] and lang [" + lang + "] are disabled"); + throw new IllegalStateException("scripts of type [" + script.getType() + "], operation [" + scriptContext.getKey() + "] and lang [" + lang + "] are disabled"); } // TODO: fix this through some API or something, that's wrong @@ -244,7 +243,7 @@ public class ScriptService extends AbstractComponent implements Closeable { boolean expression = "expression".equals(script.getLang()); boolean notSupported = scriptContext.getKey().equals(ScriptContext.Standard.UPDATE.getKey()); if (expression && notSupported) { - throw new ScriptException("scripts of type [" + script.getType() + "]," + + throw new UnsupportedOperationException("scripts of type [" + script.getType() + "]," + " operation [" + scriptContext.getKey() + "] and lang [" + lang + "] are not supported"); } @@ -307,7 +306,11 @@ public class ScriptService extends AbstractComponent implements Closeable { String actualName = (type == ScriptType.INLINE) ? null : name; compiledScript = new CompiledScript(type, name, lang, scriptEngineService.compile(actualName, code, params)); } catch (Exception exception) { - throw new ScriptException("Failed to compile " + type + " script [" + name + "] using lang [" + lang + "]", exception); + // TODO: remove this try-catch completely, when all script engines have good exceptions! + if (exception instanceof ScriptException) { + throw exception; // its already good! + } + throw new GeneralScriptException("Failed to compile " + type + " script [" + name + "] using lang [" + lang + "]", exception); } //Since the cache key is the script content itself we don't need to @@ -365,6 +368,10 @@ public class ScriptService extends AbstractComponent implements Closeable { template.getScript(), scriptLang); } } catch (Exception e) { + // TODO: remove this when all script engines have good exceptions! + if (e instanceof ScriptException) { + throw e; // its already good! + } throw new IllegalArgumentException("Unable to parse [" + template.getScript() + "] lang [" + scriptLang + "]", e); } diff --git a/core/src/test/java/org/elasticsearch/ESExceptionTests.java b/core/src/test/java/org/elasticsearch/ESExceptionTests.java index 55b8c3ed4ec..9ac8782110e 100644 --- a/core/src/test/java/org/elasticsearch/ESExceptionTests.java +++ b/core/src/test/java/org/elasticsearch/ESExceptionTests.java @@ -179,12 +179,32 @@ public class ESExceptionTests extends ESTestCase { } } + /** + * Check whether this exception contains an exception of the given type: + * either it is of the given class itself or it contains a nested cause + * of the given type. + * + * @param exType the exception type to look for + * @return whether there is a nested exception of the specified type + */ + private boolean contains(Throwable t, Class exType) { + if (exType == null) { + return false; + } + for (Throwable cause = t; t != null; t = t.getCause()) { + if (exType.isInstance(cause)) { + return true; + } + } + return false; + } + public void testGetRootCause() { Exception root = new RuntimeException("foobar"); ElasticsearchException exception = new ElasticsearchException("foo", new ElasticsearchException("bar", new IllegalArgumentException("index is closed", root))); assertEquals(root, exception.getRootCause()); - assertTrue(exception.contains(RuntimeException.class)); - assertFalse(exception.contains(EOFException.class)); + assertTrue(contains(exception, RuntimeException.class)); + assertFalse(contains(exception, EOFException.class)); } public void testToString() { diff --git a/core/src/test/java/org/elasticsearch/ExceptionSerializationTests.java b/core/src/test/java/org/elasticsearch/ExceptionSerializationTests.java index b28758ed941..96f9cbb3522 100644 --- a/core/src/test/java/org/elasticsearch/ExceptionSerializationTests.java +++ b/core/src/test/java/org/elasticsearch/ExceptionSerializationTests.java @@ -663,7 +663,7 @@ public class ExceptionSerializationTests extends ESTestCase { ids.put(22, null); // was CreateFailedEngineException ids.put(23, org.elasticsearch.index.shard.IndexShardStartedException.class); ids.put(24, org.elasticsearch.search.SearchContextMissingException.class); - ids.put(25, org.elasticsearch.script.ScriptException.class); + ids.put(25, org.elasticsearch.script.GeneralScriptException.class); ids.put(26, org.elasticsearch.index.shard.TranslogRecoveryPerformer.BatchOperationException.class); ids.put(27, org.elasticsearch.snapshots.SnapshotCreationException.class); ids.put(28, org.elasticsearch.index.engine.DeleteFailedEngineException.class); @@ -778,6 +778,7 @@ public class ExceptionSerializationTests extends ESTestCase { ids.put(140, org.elasticsearch.discovery.Discovery.FailedToCommitClusterStateException.class); ids.put(141, org.elasticsearch.index.query.QueryShardException.class); ids.put(142, ShardStateAction.NoLongerPrimaryShardException.class); + ids.put(143, org.elasticsearch.script.ScriptException.class); Map, Integer> reverse = new HashMap<>(); for (Map.Entry> entry : ids.entrySet()) { diff --git a/core/src/test/java/org/elasticsearch/common/lucene/search/function/ScriptScoreFunctionTests.java b/core/src/test/java/org/elasticsearch/common/lucene/search/function/ScriptScoreFunctionTests.java index 861476b9d47..21fbf43a133 100644 --- a/core/src/test/java/org/elasticsearch/common/lucene/search/function/ScriptScoreFunctionTests.java +++ b/core/src/test/java/org/elasticsearch/common/lucene/search/function/ScriptScoreFunctionTests.java @@ -23,7 +23,7 @@ import org.apache.lucene.index.LeafReaderContext; import org.elasticsearch.script.AbstractDoubleSearchScript; import org.elasticsearch.script.LeafSearchScript; import org.elasticsearch.script.Script; -import org.elasticsearch.script.ScriptException; +import org.elasticsearch.script.GeneralScriptException; import org.elasticsearch.script.SearchScript; import org.elasticsearch.test.ESTestCase; @@ -57,7 +57,7 @@ public class ScriptScoreFunctionTests extends ESTestCase { } }); LeafScoreFunction leafScoreFunction = scoreFunction.getLeafScoreFunction(null); - ScriptException expected = expectThrows(ScriptException.class, () -> { + GeneralScriptException expected = expectThrows(GeneralScriptException.class, () -> { leafScoreFunction.score(randomInt(), randomFloat()); }); assertTrue(expected.getMessage().contains("returned NaN")); diff --git a/core/src/test/java/org/elasticsearch/script/ScriptContextTests.java b/core/src/test/java/org/elasticsearch/script/ScriptContextTests.java index 715694fe890..bbfdb720814 100644 --- a/core/src/test/java/org/elasticsearch/script/ScriptContextTests.java +++ b/core/src/test/java/org/elasticsearch/script/ScriptContextTests.java @@ -64,7 +64,7 @@ public class ScriptContextTests extends ESTestCase { Script script = new Script("1", scriptType, MockScriptEngine.NAME, null); scriptService.compile(script, new ScriptContext.Plugin(PLUGIN_NAME, "custom_globally_disabled_op"), Collections.emptyMap(), null); fail("script compilation should have been rejected"); - } catch (ScriptException e) { + } catch (IllegalStateException e) { assertThat(e.getMessage(), containsString("scripts of type [" + scriptType + "], operation [" + PLUGIN_NAME + "_custom_globally_disabled_op] and lang [" + MockScriptEngine.NAME + "] are disabled")); } } @@ -76,7 +76,7 @@ public class ScriptContextTests extends ESTestCase { try { scriptService.compile(script, new ScriptContext.Plugin(PLUGIN_NAME, "custom_exp_disabled_op"), Collections.emptyMap(), null); fail("script compilation should have been rejected"); - } catch (ScriptException e) { + } catch (IllegalStateException e) { assertTrue(e.getMessage(), e.getMessage().contains("scripts of type [inline], operation [" + PLUGIN_NAME + "_custom_exp_disabled_op] and lang [" + MockScriptEngine.NAME + "] are disabled")); } diff --git a/core/src/test/java/org/elasticsearch/script/ScriptExceptionTests.java b/core/src/test/java/org/elasticsearch/script/ScriptExceptionTests.java new file mode 100644 index 00000000000..929c14e45e2 --- /dev/null +++ b/core/src/test/java/org/elasticsearch/script/ScriptExceptionTests.java @@ -0,0 +1,99 @@ +/* + * Licensed to Elasticsearch under one or more contributor + * license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright + * ownership. Elasticsearch licenses this file to you under + * the Apache License, Version 2.0 (the "License"); you may + * not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +package org.elasticsearch.script; + +import org.elasticsearch.common.io.stream.DataOutputStreamOutput; +import org.elasticsearch.common.io.stream.InputStreamStreamInput; +import org.elasticsearch.common.io.stream.StreamInput; +import org.elasticsearch.common.io.stream.StreamOutput; +import org.elasticsearch.test.ESTestCase; + +import java.io.ByteArrayInputStream; +import java.io.ByteArrayOutputStream; +import java.io.DataOutputStream; +import java.io.IOException; +import java.util.Arrays; +import java.util.Collections; +import java.util.List; + +/** Simple tests for {@link ScriptException} */ +public class ScriptExceptionTests extends ESTestCase { + + /** ensure we can round trip in serialization */ + public void testRoundTrip() throws IOException { + ScriptException e = new ScriptException("messageData", new Exception("causeData"), Arrays.asList("stack1", "stack2"), + "sourceData", "langData"); + + ByteArrayOutputStream bytes = new ByteArrayOutputStream(); + StreamOutput output = new DataOutputStreamOutput(new DataOutputStream(bytes)); + e.writeTo(output); + output.close(); + + StreamInput input = new InputStreamStreamInput(new ByteArrayInputStream(bytes.toByteArray())); + ScriptException e2 = new ScriptException(input); + input.close(); + + assertEquals(e.getMessage(), e2.getMessage()); + assertEquals(e.getScriptStack(), e2.getScriptStack()); + assertEquals(e.getScript(), e2.getScript()); + assertEquals(e.getLang(), e2.getLang()); + } + + /** Test that our elements are present in the json output */ + public void testJsonOutput() { + ScriptException e = new ScriptException("messageData", new Exception("causeData"), Arrays.asList("stack1", "stack2"), + "sourceData", "langData"); + String json = e.toJsonString(); + assertTrue(json.contains(e.getMessage())); + assertTrue(json.contains(e.getCause().getMessage())); + assertTrue(json.contains("stack1")); + assertTrue(json.contains("stack2")); + assertTrue(json.contains(e.getScript())); + assertTrue(json.contains(e.getLang())); + } + + /** ensure the script stack is immutable */ + public void testImmutableStack() { + ScriptException e = new ScriptException("a", new Exception(), Arrays.asList("element1", "element2"), "a", "b"); + List stack = e.getScriptStack(); + expectThrows(UnsupportedOperationException.class, () -> { + stack.add("no"); + }); + } + + /** ensure no parameters can be null */ + public void testNoLeniency() { + expectThrows(NullPointerException.class, () -> { + new ScriptException(null, new Exception(), Collections.emptyList(), "a", "b"); + }); + expectThrows(NullPointerException.class, () -> { + new ScriptException("test", null, Collections.emptyList(), "a", "b"); + }); + expectThrows(NullPointerException.class, () -> { + new ScriptException("test", new Exception(), null, "a", "b"); + }); + expectThrows(NullPointerException.class, () -> { + new ScriptException("test", new Exception(), Collections.emptyList(), null, "b"); + }); + expectThrows(NullPointerException.class, () -> { + new ScriptException("test", new Exception(), Collections.emptyList(), "a", null); + }); + } +} diff --git a/core/src/test/java/org/elasticsearch/script/ScriptServiceTests.java b/core/src/test/java/org/elasticsearch/script/ScriptServiceTests.java index 890ffccc514..c81c8b10a80 100644 --- a/core/src/test/java/org/elasticsearch/script/ScriptServiceTests.java +++ b/core/src/test/java/org/elasticsearch/script/ScriptServiceTests.java @@ -481,7 +481,7 @@ public class ScriptServiceTests extends ESTestCase { try { scriptService.compile(new Script(script, scriptType, lang, null), scriptContext, Collections.emptyMap(), emptyClusterState()); fail("compile should have been rejected for lang [" + lang + "], script_type [" + scriptType + "], scripted_op [" + scriptContext + "]"); - } catch(ScriptException e) { + } catch(IllegalStateException e) { //all good } } diff --git a/modules/lang-expression/src/main/java/org/elasticsearch/script/expression/ExpressionExecutableScript.java b/modules/lang-expression/src/main/java/org/elasticsearch/script/expression/ExpressionExecutableScript.java index 3f8bbf7ad13..85a82ac79da 100644 --- a/modules/lang-expression/src/main/java/org/elasticsearch/script/expression/ExpressionExecutableScript.java +++ b/modules/lang-expression/src/main/java/org/elasticsearch/script/expression/ExpressionExecutableScript.java @@ -25,7 +25,7 @@ import java.util.Map; import org.apache.lucene.expressions.Expression; import org.elasticsearch.script.CompiledScript; import org.elasticsearch.script.ExecutableScript; -import org.elasticsearch.script.ScriptException; +import org.elasticsearch.script.GeneralScriptException; /** * A bridge to evaluate an {@link Expression} against a map of variables in the context @@ -45,7 +45,7 @@ public class ExpressionExecutableScript implements ExecutableScript { int functionValuesLength = expression.variables.length; if (vars.size() != functionValuesLength) { - throw new ScriptException("Error using " + compiledScript + ". " + + throw new GeneralScriptException("Error using " + compiledScript + ". " + "The number of variables in an executable expression script [" + functionValuesLength + "] must match the number of variables in the variable map" + " [" + vars.size() + "]."); @@ -72,12 +72,12 @@ public class ExpressionExecutableScript implements ExecutableScript { double doubleValue = ((Number)value).doubleValue(); functionValuesMap.get(name).setValue(doubleValue); } else { - throw new ScriptException("Error using " + compiledScript + ". " + + throw new GeneralScriptException("Error using " + compiledScript + ". " + "Executable expressions scripts can only process numbers." + " The variable [" + name + "] is not a number."); } } else { - throw new ScriptException("Error using " + compiledScript + ". " + + throw new GeneralScriptException("Error using " + compiledScript + ". " + "The variable [" + name + "] does not exist in the executable expressions script."); } } @@ -87,7 +87,7 @@ public class ExpressionExecutableScript implements ExecutableScript { try { return ((Expression) compiledScript.compiled()).evaluate(NO_DOCUMENT, functionValuesArray); } catch (Exception exception) { - throw new ScriptException("Error evaluating " + compiledScript, exception); + throw new GeneralScriptException("Error evaluating " + compiledScript, exception); } } } diff --git a/modules/lang-expression/src/main/java/org/elasticsearch/script/expression/ExpressionScriptEngineService.java b/modules/lang-expression/src/main/java/org/elasticsearch/script/expression/ExpressionScriptEngineService.java index b06590a56e2..5c4ff3ca94d 100644 --- a/modules/lang-expression/src/main/java/org/elasticsearch/script/expression/ExpressionScriptEngineService.java +++ b/modules/lang-expression/src/main/java/org/elasticsearch/script/expression/ExpressionScriptEngineService.java @@ -50,7 +50,7 @@ import java.security.AccessControlContext; import java.security.AccessController; import java.security.PrivilegedAction; import java.text.ParseException; -import java.util.Collections; +import java.util.ArrayList; import java.util.List; import java.util.Map; @@ -107,7 +107,7 @@ public class ExpressionScriptEngineService extends AbstractComponent implements // NOTE: validation is delayed to allow runtime vars, and we don't have access to per index stuff here return JavascriptCompiler.compile(scriptSource, JavascriptCompiler.DEFAULT_FUNCTIONS, loader); } catch (ParseException e) { - throw new ScriptException("Failed to parse expression: " + scriptSource, e); + throw convertToScriptException("compile error", scriptSource, scriptSource, e); } } }); @@ -115,15 +115,15 @@ public class ExpressionScriptEngineService extends AbstractComponent implements @Override public SearchScript search(CompiledScript compiledScript, SearchLookup lookup, @Nullable Map vars) { - try { - Expression expr = (Expression)compiledScript.compiled(); - MapperService mapper = lookup.doc().mapperService(); - // NOTE: if we need to do anything complicated with bindings in the future, we can just extend Bindings, - // instead of complicating SimpleBindings (which should stay simple) - SimpleBindings bindings = new SimpleBindings(); - ReplaceableConstValueSource specialValue = null; - - for (String variable : expr.variables) { + Expression expr = (Expression)compiledScript.compiled(); + MapperService mapper = lookup.doc().mapperService(); + // NOTE: if we need to do anything complicated with bindings in the future, we can just extend Bindings, + // instead of complicating SimpleBindings (which should stay simple) + SimpleBindings bindings = new SimpleBindings(); + ReplaceableConstValueSource specialValue = null; + + for (String variable : expr.variables) { + try { if (variable.equals("_score")) { bindings.add(new SortField("_score", SortField.Type.SCORE)); } else if (variable.equals("_value")) { @@ -141,7 +141,7 @@ public class ExpressionScriptEngineService extends AbstractComponent implements if (value instanceof Number) { bindings.add(variable, new DoubleConstValueSource(((Number) value).doubleValue())); } else { - throw new ScriptException("Parameter [" + variable + "] must be a numeric type"); + throw new ParseException("Parameter [" + variable + "] must be a numeric type", 0); } } else { @@ -150,10 +150,10 @@ public class ExpressionScriptEngineService extends AbstractComponent implements String variablename = "value"; // .value is the default for doc['field'], its optional. VariableContext[] parts = VariableContext.parse(variable); if (parts[0].text.equals("doc") == false) { - throw new ScriptException("Unknown variable [" + parts[0].text + "] in expression"); + throw new ParseException("Unknown variable [" + parts[0].text + "]", 0); } if (parts.length < 2 || parts[1].type != VariableContext.Type.STR_INDEX) { - throw new ScriptException("Variable 'doc' in expression must be used with a specific field like: doc['myfield']"); + throw new ParseException("Variable 'doc' must be used with a specific field like: doc['myfield']", 3); } else { fieldname = parts[1].text; } @@ -163,17 +163,17 @@ public class ExpressionScriptEngineService extends AbstractComponent implements } else if (parts[2].type == VariableContext.Type.MEMBER) { variablename = parts[2].text; } else { - throw new ScriptException("Only member variables or member methods may be accessed on a field when not accessing the field directly"); + throw new IllegalArgumentException("Only member variables or member methods may be accessed on a field when not accessing the field directly"); } } if (parts.length > 3) { - throw new ScriptException("Variable [" + variable + "] does not follow an allowed format of either doc['field'] or doc['field'].method()"); + throw new IllegalArgumentException("Variable [" + variable + "] does not follow an allowed format of either doc['field'] or doc['field'].method()"); } MappedFieldType fieldType = mapper.fullName(fieldname); if (fieldType == null) { - throw new ScriptException("Field [" + fieldname + "] used in expression does not exist in mappings"); + throw new ParseException("Field [" + fieldname + "] does not exist in mappings", 5); } IndexFieldData fieldData = lookup.doc().fieldDataService().getForField(fieldType); @@ -205,18 +205,37 @@ public class ExpressionScriptEngineService extends AbstractComponent implements valueSource = NumericField.getMethod(fieldData, fieldname, methodname); } } else { - throw new ScriptException("Field [" + fieldname + "] used in expression must be numeric, date, or geopoint"); + throw new ParseException("Field [" + fieldname + "] must be numeric, date, or geopoint", 5); } bindings.add(variable, valueSource); } + } catch (Exception e) { + // we defer "binding" of variables until here: give context for that variable + throw convertToScriptException("link error", expr.sourceText, variable, e); } - - final boolean needsScores = expr.getSortField(bindings, false).needsScores(); - return new ExpressionSearchScript(compiledScript, bindings, specialValue, needsScores); - } catch (Exception exception) { - throw new ScriptException("Error during search with " + compiledScript, exception); } + + final boolean needsScores = expr.getSortField(bindings, false).needsScores(); + return new ExpressionSearchScript(compiledScript, bindings, specialValue, needsScores); + } + + /** + * converts a ParseException at compile-time or link-time to a ScriptException + */ + private ScriptException convertToScriptException(String message, String source, String portion, Throwable cause) { + List stack = new ArrayList<>(); + stack.add(portion); + StringBuilder pointer = new StringBuilder(); + if (cause instanceof ParseException) { + int offset = ((ParseException) cause).getErrorOffset(); + for (int i = 0; i < offset; i++) { + pointer.append(' '); + } + } + pointer.append("^---- HERE"); + stack.add(pointer.toString()); + throw new ScriptException(message, cause, stack, source, NAME); } @Override diff --git a/modules/lang-expression/src/main/java/org/elasticsearch/script/expression/ExpressionSearchScript.java b/modules/lang-expression/src/main/java/org/elasticsearch/script/expression/ExpressionSearchScript.java index edc6a7c3924..af89450613c 100644 --- a/modules/lang-expression/src/main/java/org/elasticsearch/script/expression/ExpressionSearchScript.java +++ b/modules/lang-expression/src/main/java/org/elasticsearch/script/expression/ExpressionSearchScript.java @@ -33,7 +33,7 @@ import org.apache.lucene.search.Scorer; import org.elasticsearch.common.lucene.Lucene; import org.elasticsearch.script.CompiledScript; import org.elasticsearch.script.LeafSearchScript; -import org.elasticsearch.script.ScriptException; +import org.elasticsearch.script.GeneralScriptException; import org.elasticsearch.script.SearchScript; /** @@ -73,7 +73,7 @@ class ExpressionSearchScript implements SearchScript { try { return values.doubleVal(docid); } catch (Exception exception) { - throw new ScriptException("Error evaluating " + compiledScript, exception); + throw new GeneralScriptException("Error evaluating " + compiledScript, exception); } } @@ -114,7 +114,7 @@ class ExpressionSearchScript implements SearchScript { if (value instanceof Number) { specialValue.setValue(((Number)value).doubleValue()); } else { - throw new ScriptException("Cannot use expression with text variable using " + compiledScript); + throw new GeneralScriptException("Cannot use expression with text variable using " + compiledScript); } } } diff --git a/modules/lang-expression/src/test/java/org/elasticsearch/script/expression/ExpressionTests.java b/modules/lang-expression/src/test/java/org/elasticsearch/script/expression/ExpressionTests.java index 209718124b6..2b74aaa9f1e 100644 --- a/modules/lang-expression/src/test/java/org/elasticsearch/script/expression/ExpressionTests.java +++ b/modules/lang-expression/src/test/java/org/elasticsearch/script/expression/ExpressionTests.java @@ -22,36 +22,50 @@ package org.elasticsearch.script.expression; import org.elasticsearch.common.settings.Settings; import org.elasticsearch.index.IndexService; import org.elasticsearch.script.CompiledScript; +import org.elasticsearch.script.ScriptException; import org.elasticsearch.script.ScriptService.ScriptType; import org.elasticsearch.script.SearchScript; import org.elasticsearch.search.lookup.SearchLookup; import org.elasticsearch.test.ESSingleNodeTestCase; +import java.text.ParseException; import java.util.Collections; public class ExpressionTests extends ESSingleNodeTestCase { - - public void testNeedsScores() { + ExpressionScriptEngineService service; + SearchLookup lookup; + + @Override + public void setUp() throws Exception { + super.setUp(); IndexService index = createIndex("test", Settings.EMPTY, "type", "d", "type=double"); - - ExpressionScriptEngineService service = new ExpressionScriptEngineService(Settings.EMPTY); - SearchLookup lookup = new SearchLookup(index.mapperService(), index.fieldData(), null); - - Object compiled = service.compile(null, "1.2", Collections.emptyMap()); - SearchScript ss = service.search(new CompiledScript(ScriptType.INLINE, "randomName", "expression", compiled), lookup, Collections.emptyMap()); - assertFalse(ss.needsScores()); - - compiled = service.compile(null, "doc['d'].value", Collections.emptyMap()); - ss = service.search(new CompiledScript(ScriptType.INLINE, "randomName", "expression", compiled), lookup, Collections.emptyMap()); - assertFalse(ss.needsScores()); - - compiled = service.compile(null, "1/_score", Collections.emptyMap()); - ss = service.search(new CompiledScript(ScriptType.INLINE, "randomName", "expression", compiled), lookup, Collections.emptyMap()); - assertTrue(ss.needsScores()); - - compiled = service.compile(null, "doc['d'].value * _score", Collections.emptyMap()); - ss = service.search(new CompiledScript(ScriptType.INLINE, "randomName", "expression", compiled), lookup, Collections.emptyMap()); - assertTrue(ss.needsScores()); + service = new ExpressionScriptEngineService(Settings.EMPTY); + lookup = new SearchLookup(index.mapperService(), index.fieldData(), null); + } + + private SearchScript compile(String expression) { + Object compiled = service.compile(null, expression, Collections.emptyMap()); + return service.search(new CompiledScript(ScriptType.INLINE, "randomName", "expression", compiled), lookup, Collections.emptyMap()); } + public void testNeedsScores() { + assertFalse(compile("1.2").needsScores()); + assertFalse(compile("doc['d'].value").needsScores()); + assertTrue(compile("1/_score").needsScores()); + assertTrue(compile("doc['d'].value * _score").needsScores()); + } + + public void testCompileError() { + ScriptException e = expectThrows(ScriptException.class, () -> { + compile("doc['d'].value * *@#)(@$*@#$ + 4"); + }); + assertTrue(e.getCause() instanceof ParseException); + } + + public void testLinkError() { + ScriptException e = expectThrows(ScriptException.class, () -> { + compile("doc['e'].value * 5"); + }); + assertTrue(e.getCause() instanceof ParseException); + } } diff --git a/modules/lang-expression/src/test/java/org/elasticsearch/script/expression/MoreExpressionTests.java b/modules/lang-expression/src/test/java/org/elasticsearch/script/expression/MoreExpressionTests.java index f023ec9f38a..3f2466e52e9 100644 --- a/modules/lang-expression/src/test/java/org/elasticsearch/script/expression/MoreExpressionTests.java +++ b/modules/lang-expression/src/test/java/org/elasticsearch/script/expression/MoreExpressionTests.java @@ -44,7 +44,7 @@ import org.elasticsearch.index.query.functionscore.ScoreFunctionBuilders; import org.elasticsearch.plugins.Plugin; import org.elasticsearch.script.CompiledScript; import org.elasticsearch.script.Script; -import org.elasticsearch.script.ScriptException; +import org.elasticsearch.script.GeneralScriptException; import org.elasticsearch.script.ScriptService.ScriptType; import org.elasticsearch.search.SearchHits; import org.elasticsearch.search.aggregations.AggregationBuilders; @@ -324,7 +324,7 @@ public class MoreExpressionTests extends ESIntegTestCase { assertThat(e.toString() + "should have contained ScriptException", e.toString().contains("ScriptException"), equalTo(true)); assertThat(e.toString() + "should have contained compilation failure", - e.toString().contains("Failed to parse expression"), equalTo(true)); + e.toString().contains("compile error"), equalTo(true)); } } @@ -494,7 +494,7 @@ public class MoreExpressionTests extends ESIntegTestCase { ees = new ExpressionExecutableScript(compiledScript, vars); ees.run(); fail("An incorrect number of variables were allowed to be used in an expression."); - } catch (ScriptException se) { + } catch (GeneralScriptException se) { message = se.getMessage(); assertThat(message + " should have contained number of variables", message.contains("number of variables"), equalTo(true)); } @@ -507,7 +507,7 @@ public class MoreExpressionTests extends ESIntegTestCase { ees = new ExpressionExecutableScript(compiledScript, vars); ees.run(); fail("A variable was allowed to be set that does not exist in the expression."); - } catch (ScriptException se) { + } catch (GeneralScriptException se) { message = se.getMessage(); assertThat(message + " should have contained does not exist in", message.contains("does not exist in"), equalTo(true)); } @@ -520,7 +520,7 @@ public class MoreExpressionTests extends ESIntegTestCase { ees = new ExpressionExecutableScript(compiledScript, vars); ees.run(); fail("A non-number was allowed to be use in the expression."); - } catch (ScriptException se) { + } catch (GeneralScriptException se) { message = se.getMessage(); assertThat(message + " should have contained process numbers", message.contains("process numbers"), equalTo(true)); } diff --git a/modules/lang-groovy/src/main/java/org/elasticsearch/script/groovy/GroovyScriptEngineService.java b/modules/lang-groovy/src/main/java/org/elasticsearch/script/groovy/GroovyScriptEngineService.java index 5d06196a83b..ef4f6fd76b6 100644 --- a/modules/lang-groovy/src/main/java/org/elasticsearch/script/groovy/GroovyScriptEngineService.java +++ b/modules/lang-groovy/src/main/java/org/elasticsearch/script/groovy/GroovyScriptEngineService.java @@ -50,7 +50,7 @@ import org.elasticsearch.script.ExecutableScript; import org.elasticsearch.script.LeafSearchScript; import org.elasticsearch.script.ScoreAccessor; import org.elasticsearch.script.ScriptEngineService; -import org.elasticsearch.script.ScriptException; +import org.elasticsearch.script.GeneralScriptException; import org.elasticsearch.script.SearchScript; import org.elasticsearch.search.lookup.LeafSearchLookup; import org.elasticsearch.search.lookup.SearchLookup; @@ -193,7 +193,7 @@ public class GroovyScriptEngineService extends AbstractComponent implements Scri if (logger.isTraceEnabled()) { logger.trace("exception compiling Groovy script:", e); } - throw new ScriptException("failed to compile groovy script", e); + throw new GeneralScriptException("failed to compile groovy script", e); } } @@ -219,7 +219,7 @@ public class GroovyScriptEngineService extends AbstractComponent implements Scri } return new GroovyScript(compiledScript, createScript(compiledScript.compiled(), allVars), this.logger); } catch (ReflectiveOperationException e) { - throw new ScriptException("failed to build executable " + compiledScript, e); + throw new GeneralScriptException("failed to build executable " + compiledScript, e); } } @@ -239,7 +239,7 @@ public class GroovyScriptEngineService extends AbstractComponent implements Scri try { scriptObject = createScript(compiledScript.compiled(), allVars); } catch (ReflectiveOperationException e) { - throw new ScriptException("failed to build search " + compiledScript, e); + throw new GeneralScriptException("failed to build search " + compiledScript, e); } return new GroovyScript(compiledScript, scriptObject, leafLookup, logger); } @@ -312,7 +312,7 @@ public class GroovyScriptEngineService extends AbstractComponent implements Scri if (logger.isTraceEnabled()) { logger.trace("failed to run {}", e, compiledScript); } - throw new ScriptException("failed to run " + compiledScript, e); + throw new GeneralScriptException("failed to run " + compiledScript, e); } } diff --git a/modules/lang-groovy/src/test/java/org/elasticsearch/script/groovy/GroovySecurityTests.java b/modules/lang-groovy/src/test/java/org/elasticsearch/script/groovy/GroovySecurityTests.java index e0e439aa6e8..7bdfe5a205e 100644 --- a/modules/lang-groovy/src/test/java/org/elasticsearch/script/groovy/GroovySecurityTests.java +++ b/modules/lang-groovy/src/test/java/org/elasticsearch/script/groovy/GroovySecurityTests.java @@ -24,7 +24,7 @@ import org.apache.lucene.util.Constants; import org.codehaus.groovy.control.MultipleCompilationErrorsException; import org.elasticsearch.common.settings.Settings; import org.elasticsearch.script.CompiledScript; -import org.elasticsearch.script.ScriptException; +import org.elasticsearch.script.GeneralScriptException; import org.elasticsearch.script.ScriptService; import org.elasticsearch.test.ESTestCase; @@ -157,7 +157,7 @@ public class GroovySecurityTests extends ESTestCase { try { doTest(script); fail("did not get expected exception"); - } catch (ScriptException expected) { + } catch (GeneralScriptException expected) { Throwable cause = expected.getCause(); assertNotNull(cause); if (exceptionClass.isAssignableFrom(cause.getClass()) == false) { diff --git a/modules/lang-mustache/src/main/java/org/elasticsearch/script/mustache/MustacheScriptEngineService.java b/modules/lang-mustache/src/main/java/org/elasticsearch/script/mustache/MustacheScriptEngineService.java index b93eaaa9b52..d565f66b8e2 100644 --- a/modules/lang-mustache/src/main/java/org/elasticsearch/script/mustache/MustacheScriptEngineService.java +++ b/modules/lang-mustache/src/main/java/org/elasticsearch/script/mustache/MustacheScriptEngineService.java @@ -31,7 +31,7 @@ import org.elasticsearch.common.settings.Settings; import org.elasticsearch.script.CompiledScript; import org.elasticsearch.script.ExecutableScript; import org.elasticsearch.script.ScriptEngineService; -import org.elasticsearch.script.ScriptException; +import org.elasticsearch.script.GeneralScriptException; import org.elasticsearch.script.SearchScript; import org.elasticsearch.search.lookup.SearchLookup; @@ -185,7 +185,7 @@ public final class MustacheScriptEngineService extends AbstractComponent impleme }); } catch (Exception e) { logger.error("Error running {}", e, template); - throw new ScriptException("Error running " + template, e); + throw new GeneralScriptException("Error running " + template, e); } return result.bytes(); } diff --git a/modules/lang-painless/src/main/java/org/elasticsearch/painless/Compiler.java b/modules/lang-painless/src/main/java/org/elasticsearch/painless/Compiler.java index 666f28bb406..36d41056013 100644 --- a/modules/lang-painless/src/main/java/org/elasticsearch/painless/Compiler.java +++ b/modules/lang-painless/src/main/java/org/elasticsearch/painless/Compiler.java @@ -29,6 +29,7 @@ import java.net.URL; import java.security.CodeSource; import java.security.SecureClassLoader; import java.security.cert.Certificate; +import java.util.BitSet; import static org.elasticsearch.painless.WriterConstants.CLASS_NAME; @@ -93,9 +94,28 @@ final class Compiler { * @return An {@link Executable} Painless script. */ static Executable compile(Loader loader, String name, String source, CompilerSettings settings) { - byte[] bytes = compile(name, source, settings); + if (source.length() > MAXIMUM_SOURCE_LENGTH) { + throw new IllegalArgumentException("Scripts may be no longer than " + MAXIMUM_SOURCE_LENGTH + + " characters. The passed in script is " + source.length() + " characters. Consider using a" + + " plugin if a script longer than this length is a requirement."); + } - return createExecutable(loader, name, source, bytes); + Reserved reserved = new Reserved(); + SSource root = Walker.buildPainlessTree(source, reserved, settings); + Variables variables = Analyzer.analyze(reserved, root); + BitSet expressions = new BitSet(source.length()); + + byte[] bytes = Writer.write(settings, name, source, variables, root, expressions); + try { + Class clazz = loader.define(CLASS_NAME, bytes); + java.lang.reflect.Constructor constructor = + clazz.getConstructor(String.class, String.class, BitSet.class); + + return constructor.newInstance(name, source, expressions); + } catch (Exception exception) { // Catch everything to let the user know this is something caused internally. + throw new IllegalStateException( + "An internal error occurred attempting to define the script [" + name + "].", exception); + } } /** @@ -115,27 +135,7 @@ final class Compiler { SSource root = Walker.buildPainlessTree(source, reserved, settings); Variables variables = Analyzer.analyze(reserved, root); - return Writer.write(settings, name, source, variables, root); - } - - /** - * Generates an {@link Executable} that can run a Painless script. - * @param loader The {@link Loader} to define the script's class file. - * @param name The name of the script. - * @param source The source text of the script. - * @param bytes The ASM generated byte code to define the class with. - * @return A Painless {@link Executable} script. - */ - private static Executable createExecutable(Loader loader, String name, String source, byte[] bytes) { - try { - Class clazz = loader.define(CLASS_NAME, bytes); - java.lang.reflect.Constructor constructor = clazz.getConstructor(String.class, String.class); - - return constructor.newInstance(name, source); - } catch (Exception exception) { // Catch everything to let the user know this is something caused internally. - throw new IllegalStateException( - "An internal error occurred attempting to define the script [" + name + "].", exception); - } + return Writer.write(settings, name, source, variables, root, new BitSet(source.length())); } /** diff --git a/modules/lang-painless/src/main/java/org/elasticsearch/painless/Executable.java b/modules/lang-painless/src/main/java/org/elasticsearch/painless/Executable.java index 7681ae8232a..c6e38374aa7 100644 --- a/modules/lang-painless/src/main/java/org/elasticsearch/painless/Executable.java +++ b/modules/lang-painless/src/main/java/org/elasticsearch/painless/Executable.java @@ -22,6 +22,7 @@ package org.elasticsearch.painless; import org.apache.lucene.search.Scorer; import org.elasticsearch.search.lookup.LeafDocLookup; +import java.util.BitSet; import java.util.Map; /** @@ -31,10 +32,12 @@ public abstract class Executable { private final String name; private final String source; + private final BitSet statements; - public Executable(final String name, final String source) { + public Executable(String name, String source, BitSet statements) { this.name = name; this.source = source; + this.statements = statements; } public String getName() { @@ -45,6 +48,24 @@ public abstract class Executable { return source; } + /** + * Finds the start of the first statement boundary that is + * on or before {@code offset}. If one is not found, {@code -1} + * is returned. + */ + public int getPreviousStatement(int offset) { + return statements.previousSetBit(offset); + } + + /** + * Finds the start of the first statement boundary that is + * after {@code offset}. If one is not found, {@code -1} + * is returned. + */ + public int getNextStatement(int offset) { + return statements.nextSetBit(offset+1); + } + public abstract Object execute( final Map params, final Scorer scorer, final LeafDocLookup doc, final Object value); } diff --git a/modules/lang-painless/src/main/java/org/elasticsearch/painless/MethodWriter.java b/modules/lang-painless/src/main/java/org/elasticsearch/painless/MethodWriter.java index 90c02b7e801..7c11c5b15bf 100644 --- a/modules/lang-painless/src/main/java/org/elasticsearch/painless/MethodWriter.java +++ b/modules/lang-painless/src/main/java/org/elasticsearch/painless/MethodWriter.java @@ -30,6 +30,7 @@ import org.objectweb.asm.commons.Method; import java.util.ArrayDeque; import java.util.ArrayList; +import java.util.BitSet; import java.util.Deque; import java.util.List; @@ -86,13 +87,15 @@ import static org.elasticsearch.painless.WriterConstants.UTILITY_TYPE; * shared by the nodes of the Painless tree. */ public final class MethodWriter extends GeneratorAdapter { + private final BitSet statements; private final Deque> stringConcatArgs = (INDY_STRING_CONCAT_BOOTSTRAP_HANDLE == null) ? null : new ArrayDeque<>(); - MethodWriter(int access, Method method, org.objectweb.asm.Type[] exceptions, ClassVisitor cv) { + MethodWriter(int access, Method method, org.objectweb.asm.Type[] exceptions, ClassVisitor cv, BitSet statements) { super(Opcodes.ASM5, cv.visitMethod(access, method.getName(), method.getDescriptor(), null, getInternalNames(exceptions)), access, method.getName(), method.getDescriptor()); + this.statements = statements; } private static String[] getInternalNames(final org.objectweb.asm.Type[] types) { @@ -106,8 +109,33 @@ public final class MethodWriter extends GeneratorAdapter { return names; } - public void writeLoopCounter(final int slot, final int count) { + /** + * Marks a new statement boundary. + *

+ * This is invoked for each statement boundary (leaf {@code S*} nodes). + */ + public void writeStatementOffset(int offset) { + // ensure we don't have duplicate stuff going in here. can catch bugs + // (e.g. nodes get assigned wrong offsets by antlr walker) + assert statements.get(offset) == false; + statements.set(offset); + } + + /** + * Encodes the offset into the line number table as {@code offset + 1}. + *

+ * This is invoked before instructions that can hit exceptions. + */ + public void writeDebugInfo(int offset) { + // TODO: maybe track these in bitsets too? this is trickier... + Label label = new Label(); + visitLabel(label); + visitLineNumber(offset + 1, label); + } + + public void writeLoopCounter(int slot, int count, int offset) { if (slot > -1) { + writeDebugInfo(offset); final Label end = new Label(); iinc(slot, -count); diff --git a/modules/lang-painless/src/main/java/org/elasticsearch/painless/ScriptImpl.java b/modules/lang-painless/src/main/java/org/elasticsearch/painless/ScriptImpl.java index 04d5a8d3d60..83ae7b664c1 100644 --- a/modules/lang-painless/src/main/java/org/elasticsearch/painless/ScriptImpl.java +++ b/modules/lang-painless/src/main/java/org/elasticsearch/painless/ScriptImpl.java @@ -22,10 +22,13 @@ package org.elasticsearch.painless; import org.apache.lucene.search.Scorer; import org.elasticsearch.script.ExecutableScript; import org.elasticsearch.script.LeafSearchScript; +import org.elasticsearch.script.ScriptException; import org.elasticsearch.search.lookup.LeafDocLookup; import org.elasticsearch.search.lookup.LeafSearchLookup; +import java.util.ArrayList; import java.util.HashMap; +import java.util.List; import java.util.Map; /** @@ -114,7 +117,63 @@ final class ScriptImpl implements ExecutableScript, LeafSearchScript { */ @Override public Object run() { - return executable.execute(variables, scorer, doc, aggregationValue); + try { + return executable.execute(variables, scorer, doc, aggregationValue); + } catch (PainlessError | Exception t) { + throw convertToScriptException(t); + } + } + + private ScriptException convertToScriptException(Throwable t) { + // create a script stack: this is just the script portion + List scriptStack = new ArrayList<>(); + for (StackTraceElement element : t.getStackTrace()) { + if (WriterConstants.CLASS_NAME.equals(element.getClassName())) { + // found the script portion + int offset = element.getLineNumber(); + if (offset == -1) { + scriptStack.add("<<< unknown portion of script >>>"); + } else { + offset--; // offset is 1 based, line numbers must be! + int startOffset = executable.getPreviousStatement(offset); + if (startOffset == -1) { + assert false; // should never happen unless we hit exc in ctor prologue... + startOffset = 0; + } + int endOffset = executable.getNextStatement(startOffset); + if (endOffset == -1) { + endOffset = executable.getSource().length(); + } + // TODO: if this is still too long, truncate and use ellipses + String snippet = executable.getSource().substring(startOffset, endOffset); + scriptStack.add(snippet); + StringBuilder pointer = new StringBuilder(); + for (int i = startOffset; i < offset; i++) { + pointer.append(' '); + } + pointer.append("^---- HERE"); + scriptStack.add(pointer.toString()); + } + break; + // but filter our own internal stacks (e.g. indy bootstrap) + } else if (!shouldFilter(element)) { + scriptStack.add(element.toString()); + } + } + // build a name for the script: + final String name; + if (PainlessScriptEngineService.INLINE_NAME.equals(executable.getName())) { + name = executable.getSource(); + } else { + name = executable.getName(); + } + throw new ScriptException("runtime error", t, scriptStack, name, PainlessScriptEngineService.NAME); + } + + /** returns true for methods that are part of the runtime */ + private static boolean shouldFilter(StackTraceElement element) { + return element.getClassName().startsWith("org.elasticsearch.painless.") || + element.getClassName().startsWith("java.lang.invoke."); } /** diff --git a/modules/lang-painless/src/main/java/org/elasticsearch/painless/Writer.java b/modules/lang-painless/src/main/java/org/elasticsearch/painless/Writer.java index 6e2d0e1431b..362d81f2479 100644 --- a/modules/lang-painless/src/main/java/org/elasticsearch/painless/Writer.java +++ b/modules/lang-painless/src/main/java/org/elasticsearch/painless/Writer.java @@ -33,15 +33,15 @@ import static org.elasticsearch.painless.WriterConstants.EXECUTE; import static org.elasticsearch.painless.WriterConstants.MAP_GET; import static org.elasticsearch.painless.WriterConstants.MAP_TYPE; +import java.util.BitSet; + /** * Runs the writing phase of compilation using the Painless AST. */ final class Writer { - static byte[] write(final CompilerSettings settings, - String name, final String source, final Variables variables, final SSource root) { - final Writer writer = new Writer(settings, name, source, variables, root); - + static byte[] write(CompilerSettings settings, String name, String source, Variables variables, SSource root, BitSet expressions) { + Writer writer = new Writer(settings, name, source, variables, root, expressions); return writer.getBytes(); } @@ -54,8 +54,7 @@ final class Writer { private final ClassWriter writer; private final MethodWriter adapter; - private Writer(final CompilerSettings settings, - String name, final String source, final Variables variables, final SSource root) { + private Writer(CompilerSettings settings, String name, String source, Variables variables, SSource root, BitSet expressions) { this.settings = settings; this.scriptName = name; this.source = source; @@ -67,7 +66,7 @@ final class Writer { writeBegin(); writeConstructor(); - adapter = new MethodWriter(Opcodes.ACC_PUBLIC, EXECUTE, null, writer); + adapter = new MethodWriter(Opcodes.ACC_PUBLIC, EXECUTE, null, writer, expressions); writeExecute(); writeEnd(); diff --git a/modules/lang-painless/src/main/java/org/elasticsearch/painless/WriterConstants.java b/modules/lang-painless/src/main/java/org/elasticsearch/painless/WriterConstants.java index 410c06e6fd7..a4804cde434 100644 --- a/modules/lang-painless/src/main/java/org/elasticsearch/painless/WriterConstants.java +++ b/modules/lang-painless/src/main/java/org/elasticsearch/painless/WriterConstants.java @@ -29,6 +29,7 @@ import org.objectweb.asm.commons.Method; import java.lang.invoke.CallSite; import java.lang.invoke.MethodHandles; import java.lang.invoke.MethodType; +import java.util.BitSet; import java.util.Map; /** @@ -42,7 +43,7 @@ public final class WriterConstants { public final static String CLASS_NAME = BASE_CLASS_NAME + "$Script"; public final static Type CLASS_TYPE = Type.getObjectType(CLASS_NAME.replace('.', '/')); - public final static Method CONSTRUCTOR = getAsmMethod(void.class, "", String.class, String.class); + public final static Method CONSTRUCTOR = getAsmMethod(void.class, "", String.class, String.class, BitSet.class); public final static Method EXECUTE = getAsmMethod(Object.class, "execute", Map.class, Scorer.class, LeafDocLookup.class, Object.class); diff --git a/modules/lang-painless/src/main/java/org/elasticsearch/painless/antlr/Walker.java b/modules/lang-painless/src/main/java/org/elasticsearch/painless/antlr/Walker.java index 3669ab24ecc..d42cb769495 100644 --- a/modules/lang-painless/src/main/java/org/elasticsearch/painless/antlr/Walker.java +++ b/modules/lang-painless/src/main/java/org/elasticsearch/painless/antlr/Walker.java @@ -380,7 +380,7 @@ public final class Walker extends PainlessParserBaseVisitor { String name = declvar.ID().getText(); AExpression expression = declvar.expression() == null ? null : (AExpression)visitExpression(declvar.expression()); - declarations.add(new SDeclaration(line(ctx), offset(ctx), location(ctx), type, name, expression)); + declarations.add(new SDeclaration(line(declvar), offset(declvar), location(declvar), type, name, expression)); } return new SDeclBlock(line(ctx), offset(ctx), location(ctx), declarations); diff --git a/modules/lang-painless/src/main/java/org/elasticsearch/painless/node/ANode.java b/modules/lang-painless/src/main/java/org/elasticsearch/painless/node/ANode.java index 414f6afe038..a27f6748086 100644 --- a/modules/lang-painless/src/main/java/org/elasticsearch/painless/node/ANode.java +++ b/modules/lang-painless/src/main/java/org/elasticsearch/painless/node/ANode.java @@ -19,9 +19,6 @@ package org.elasticsearch.painless.node; -import org.elasticsearch.painless.MethodWriter; -import org.objectweb.asm.Label; - /** * The superclass for all other nodes. */ @@ -51,15 +48,4 @@ public abstract class ANode { public String error(final String message) { return "Error " + location + ": " + message; } - - /** - * Writes line number information - *

- * Currently we emit line number data for for leaf S-nodes - */ - void writeDebugInfo(MethodWriter writer) { - Label label = new Label(); - writer.visitLabel(label); - writer.visitLineNumber(line, label); - } } diff --git a/modules/lang-painless/src/main/java/org/elasticsearch/painless/node/EBinary.java b/modules/lang-painless/src/main/java/org/elasticsearch/painless/node/EBinary.java index 0c083111d66..23179a9ed0d 100644 --- a/modules/lang-painless/src/main/java/org/elasticsearch/painless/node/EBinary.java +++ b/modules/lang-painless/src/main/java/org/elasticsearch/painless/node/EBinary.java @@ -470,6 +470,7 @@ public final class EBinary extends AExpression { @Override void write(MethodWriter writer) { + writer.writeDebugInfo(offset); if (actual.sort == Sort.STRING && operation == Operation.ADD) { if (!cat) { writer.writeNewStrings(); diff --git a/modules/lang-painless/src/main/java/org/elasticsearch/painless/node/ECast.java b/modules/lang-painless/src/main/java/org/elasticsearch/painless/node/ECast.java index e2fe5cb6d10..96e07511485 100644 --- a/modules/lang-painless/src/main/java/org/elasticsearch/painless/node/ECast.java +++ b/modules/lang-painless/src/main/java/org/elasticsearch/painless/node/ECast.java @@ -51,6 +51,7 @@ final class ECast extends AExpression { @Override void write(MethodWriter writer) { child.write(writer); + writer.writeDebugInfo(offset); writer.writeCast(cast); writer.writeBranch(tru, fals); } diff --git a/modules/lang-painless/src/main/java/org/elasticsearch/painless/node/EChain.java b/modules/lang-painless/src/main/java/org/elasticsearch/painless/node/EChain.java index 2de1a5b320e..affd1fe78c0 100644 --- a/modules/lang-painless/src/main/java/org/elasticsearch/painless/node/EChain.java +++ b/modules/lang-painless/src/main/java/org/elasticsearch/painless/node/EChain.java @@ -248,6 +248,10 @@ public final class EChain extends AExpression { @Override void write(MethodWriter writer) { + if (cat) { + writer.writeDebugInfo(offset); + } + if (cat) { writer.writeNewStrings(); } diff --git a/modules/lang-painless/src/main/java/org/elasticsearch/painless/node/EComp.java b/modules/lang-painless/src/main/java/org/elasticsearch/painless/node/EComp.java index 4521c76d911..5bc6e4ee400 100644 --- a/modules/lang-painless/src/main/java/org/elasticsearch/painless/node/EComp.java +++ b/modules/lang-painless/src/main/java/org/elasticsearch/painless/node/EComp.java @@ -399,6 +399,7 @@ public final class EComp extends AExpression { @Override void write(MethodWriter writer) { + writer.writeDebugInfo(offset); boolean branch = tru != null || fals != null; org.objectweb.asm.Type rtype = right.actual.type; Sort rsort = right.actual.sort; diff --git a/modules/lang-painless/src/main/java/org/elasticsearch/painless/node/EConditional.java b/modules/lang-painless/src/main/java/org/elasticsearch/painless/node/EConditional.java index a13176e56b7..d7f7ab7e4b6 100644 --- a/modules/lang-painless/src/main/java/org/elasticsearch/painless/node/EConditional.java +++ b/modules/lang-painless/src/main/java/org/elasticsearch/painless/node/EConditional.java @@ -78,6 +78,7 @@ public final class EConditional extends AExpression { @Override void write(MethodWriter writer) { + writer.writeDebugInfo(offset); Label localfals = new Label(); Label end = new Label(); diff --git a/modules/lang-painless/src/main/java/org/elasticsearch/painless/node/EUnary.java b/modules/lang-painless/src/main/java/org/elasticsearch/painless/node/EUnary.java index 8b02522fca9..2122174442d 100644 --- a/modules/lang-painless/src/main/java/org/elasticsearch/painless/node/EUnary.java +++ b/modules/lang-painless/src/main/java/org/elasticsearch/painless/node/EUnary.java @@ -165,6 +165,7 @@ public final class EUnary extends AExpression { @Override void write(MethodWriter writer) { + writer.writeDebugInfo(offset); if (operation == Operation.NOT) { if (tru == null && fals == null) { Label localfals = new Label(); diff --git a/modules/lang-painless/src/main/java/org/elasticsearch/painless/node/LArrayLength.java b/modules/lang-painless/src/main/java/org/elasticsearch/painless/node/LArrayLength.java index cea5c629b14..b8e2fc7739b 100644 --- a/modules/lang-painless/src/main/java/org/elasticsearch/painless/node/LArrayLength.java +++ b/modules/lang-painless/src/main/java/org/elasticsearch/painless/node/LArrayLength.java @@ -60,6 +60,7 @@ public final class LArrayLength extends ALink { @Override void load(MethodWriter writer) { + writer.writeDebugInfo(offset); writer.arrayLength(); } diff --git a/modules/lang-painless/src/main/java/org/elasticsearch/painless/node/LBrace.java b/modules/lang-painless/src/main/java/org/elasticsearch/painless/node/LBrace.java index 4411913ea07..02e8ceacd9f 100644 --- a/modules/lang-painless/src/main/java/org/elasticsearch/painless/node/LBrace.java +++ b/modules/lang-painless/src/main/java/org/elasticsearch/painless/node/LBrace.java @@ -74,11 +74,13 @@ public final class LBrace extends ALink { @Override void load(MethodWriter writer) { + writer.writeDebugInfo(offset); writer.arrayLoad(after.type); } @Override void store(MethodWriter writer) { + writer.writeDebugInfo(offset); writer.arrayStore(after.type); } diff --git a/modules/lang-painless/src/main/java/org/elasticsearch/painless/node/LCall.java b/modules/lang-painless/src/main/java/org/elasticsearch/painless/node/LCall.java index fdb369468bc..30775e3a6e7 100644 --- a/modules/lang-painless/src/main/java/org/elasticsearch/painless/node/LCall.java +++ b/modules/lang-painless/src/main/java/org/elasticsearch/painless/node/LCall.java @@ -91,6 +91,7 @@ public final class LCall extends ALink { @Override void load(MethodWriter writer) { + writer.writeDebugInfo(offset); for (AExpression argument : arguments) { argument.write(writer); } diff --git a/modules/lang-painless/src/main/java/org/elasticsearch/painless/node/LCast.java b/modules/lang-painless/src/main/java/org/elasticsearch/painless/node/LCast.java index 06dee04fca6..bce3f213a71 100644 --- a/modules/lang-painless/src/main/java/org/elasticsearch/painless/node/LCast.java +++ b/modules/lang-painless/src/main/java/org/elasticsearch/painless/node/LCast.java @@ -61,6 +61,7 @@ public final class LCast extends ALink { @Override void write(MethodWriter writer) { + writer.writeDebugInfo(offset); writer.writeCast(cast); } diff --git a/modules/lang-painless/src/main/java/org/elasticsearch/painless/node/LDefArray.java b/modules/lang-painless/src/main/java/org/elasticsearch/painless/node/LDefArray.java index 452a0732cdb..b00eafe3f9f 100644 --- a/modules/lang-painless/src/main/java/org/elasticsearch/painless/node/LDefArray.java +++ b/modules/lang-painless/src/main/java/org/elasticsearch/painless/node/LDefArray.java @@ -58,12 +58,14 @@ final class LDefArray extends ALink implements IDefLink { @Override void load(MethodWriter writer) { + writer.writeDebugInfo(offset); String desc = Type.getMethodDescriptor(after.type, Definition.DEF_TYPE.type, index.actual.type); writer.invokeDynamic("arrayLoad", desc, DEF_BOOTSTRAP_HANDLE, (Object)DefBootstrap.ARRAY_LOAD); } @Override void store(MethodWriter writer) { + writer.writeDebugInfo(offset); String desc = Type.getMethodDescriptor(Definition.VOID_TYPE.type, Definition.DEF_TYPE.type, index.actual.type, after.type); writer.invokeDynamic("arrayStore", desc, DEF_BOOTSTRAP_HANDLE, (Object)DefBootstrap.ARRAY_STORE); } diff --git a/modules/lang-painless/src/main/java/org/elasticsearch/painless/node/LDefCall.java b/modules/lang-painless/src/main/java/org/elasticsearch/painless/node/LDefCall.java index 8ddde1cbed4..0292ac3c589 100644 --- a/modules/lang-painless/src/main/java/org/elasticsearch/painless/node/LDefCall.java +++ b/modules/lang-painless/src/main/java/org/elasticsearch/painless/node/LDefCall.java @@ -67,6 +67,7 @@ final class LDefCall extends ALink implements IDefLink { @Override void load(MethodWriter writer) { + writer.writeDebugInfo(offset); StringBuilder signature = new StringBuilder(); signature.append('('); diff --git a/modules/lang-painless/src/main/java/org/elasticsearch/painless/node/LDefField.java b/modules/lang-painless/src/main/java/org/elasticsearch/painless/node/LDefField.java index b0a9d4dd0d9..e762de9e862 100644 --- a/modules/lang-painless/src/main/java/org/elasticsearch/painless/node/LDefField.java +++ b/modules/lang-painless/src/main/java/org/elasticsearch/painless/node/LDefField.java @@ -55,12 +55,14 @@ final class LDefField extends ALink implements IDefLink { @Override void load(MethodWriter writer) { + writer.writeDebugInfo(offset); String desc = Type.getMethodDescriptor(after.type, Definition.DEF_TYPE.type); writer.invokeDynamic(value, desc, DEF_BOOTSTRAP_HANDLE, (Object)DefBootstrap.LOAD); } @Override void store(MethodWriter writer) { + writer.writeDebugInfo(offset); String desc = Type.getMethodDescriptor(Definition.VOID_TYPE.type, Definition.DEF_TYPE.type, after.type); writer.invokeDynamic(value, desc, DEF_BOOTSTRAP_HANDLE, (Object)DefBootstrap.STORE); } diff --git a/modules/lang-painless/src/main/java/org/elasticsearch/painless/node/LField.java b/modules/lang-painless/src/main/java/org/elasticsearch/painless/node/LField.java index ce986413375..88173ad3f65 100644 --- a/modules/lang-painless/src/main/java/org/elasticsearch/painless/node/LField.java +++ b/modules/lang-painless/src/main/java/org/elasticsearch/painless/node/LField.java @@ -105,6 +105,7 @@ public final class LField extends ALink { @Override void load(MethodWriter writer) { + writer.writeDebugInfo(offset); if (java.lang.reflect.Modifier.isStatic(field.modifiers)) { writer.getStatic(field.owner.type, field.javaName, field.type.type); } else { @@ -114,6 +115,7 @@ public final class LField extends ALink { @Override void store(MethodWriter writer) { + writer.writeDebugInfo(offset); if (java.lang.reflect.Modifier.isStatic(field.modifiers)) { writer.putStatic(field.owner.type, field.javaName, field.type.type); } else { diff --git a/modules/lang-painless/src/main/java/org/elasticsearch/painless/node/LListShortcut.java b/modules/lang-painless/src/main/java/org/elasticsearch/painless/node/LListShortcut.java index f95c24f8592..b628fee5f37 100644 --- a/modules/lang-painless/src/main/java/org/elasticsearch/painless/node/LListShortcut.java +++ b/modules/lang-painless/src/main/java/org/elasticsearch/painless/node/LListShortcut.java @@ -79,6 +79,7 @@ final class LListShortcut extends ALink { @Override void load(MethodWriter writer) { + writer.writeDebugInfo(offset); if (java.lang.reflect.Modifier.isInterface(getter.owner.clazz.getModifiers())) { writer.invokeInterface(getter.owner.type, getter.method); } else { @@ -92,6 +93,7 @@ final class LListShortcut extends ALink { @Override void store(MethodWriter writer) { + writer.writeDebugInfo(offset); if (java.lang.reflect.Modifier.isInterface(setter.owner.clazz.getModifiers())) { writer.invokeInterface(setter.owner.type, setter.method); } else { diff --git a/modules/lang-painless/src/main/java/org/elasticsearch/painless/node/LMapShortcut.java b/modules/lang-painless/src/main/java/org/elasticsearch/painless/node/LMapShortcut.java index afe93ba1048..a500a6673fc 100644 --- a/modules/lang-painless/src/main/java/org/elasticsearch/painless/node/LMapShortcut.java +++ b/modules/lang-painless/src/main/java/org/elasticsearch/painless/node/LMapShortcut.java @@ -78,6 +78,7 @@ final class LMapShortcut extends ALink { @Override void load(MethodWriter writer) { + writer.writeDebugInfo(offset); if (java.lang.reflect.Modifier.isInterface(getter.owner.clazz.getModifiers())) { writer.invokeInterface(getter.owner.type, getter.method); } else { @@ -91,6 +92,7 @@ final class LMapShortcut extends ALink { @Override void store(MethodWriter writer) { + writer.writeDebugInfo(offset); if (java.lang.reflect.Modifier.isInterface(setter.owner.clazz.getModifiers())) { writer.invokeInterface(setter.owner.type, setter.method); } else { diff --git a/modules/lang-painless/src/main/java/org/elasticsearch/painless/node/LNewArray.java b/modules/lang-painless/src/main/java/org/elasticsearch/painless/node/LNewArray.java index ffe6168f812..c555f04ab95 100644 --- a/modules/lang-painless/src/main/java/org/elasticsearch/painless/node/LNewArray.java +++ b/modules/lang-painless/src/main/java/org/elasticsearch/painless/node/LNewArray.java @@ -79,6 +79,7 @@ public final class LNewArray extends ALink { @Override void load(MethodWriter writer) { + writer.writeDebugInfo(offset); for (AExpression argument : arguments) { argument.write(writer); } diff --git a/modules/lang-painless/src/main/java/org/elasticsearch/painless/node/LNewObj.java b/modules/lang-painless/src/main/java/org/elasticsearch/painless/node/LNewObj.java index dc8b711ee8c..44738450962 100644 --- a/modules/lang-painless/src/main/java/org/elasticsearch/painless/node/LNewObj.java +++ b/modules/lang-painless/src/main/java/org/elasticsearch/painless/node/LNewObj.java @@ -98,6 +98,7 @@ public final class LNewObj extends ALink { @Override void load(MethodWriter writer) { + writer.writeDebugInfo(offset); writer.newInstance(after.type); if (load) { diff --git a/modules/lang-painless/src/main/java/org/elasticsearch/painless/node/LShortcut.java b/modules/lang-painless/src/main/java/org/elasticsearch/painless/node/LShortcut.java index c6aafd47bff..7eb44156e55 100644 --- a/modules/lang-painless/src/main/java/org/elasticsearch/painless/node/LShortcut.java +++ b/modules/lang-painless/src/main/java/org/elasticsearch/painless/node/LShortcut.java @@ -84,6 +84,7 @@ final class LShortcut extends ALink { @Override void load(MethodWriter writer) { + writer.writeDebugInfo(offset); if (java.lang.reflect.Modifier.isInterface(getter.owner.clazz.getModifiers())) { writer.invokeInterface(getter.owner.type, getter.method); } else { @@ -97,6 +98,7 @@ final class LShortcut extends ALink { @Override void store(MethodWriter writer) { + writer.writeDebugInfo(offset); if (java.lang.reflect.Modifier.isInterface(setter.owner.clazz.getModifiers())) { writer.invokeInterface(setter.owner.type, setter.method); } else { diff --git a/modules/lang-painless/src/main/java/org/elasticsearch/painless/node/SBreak.java b/modules/lang-painless/src/main/java/org/elasticsearch/painless/node/SBreak.java index 727cd4b929c..4cd1decb26b 100644 --- a/modules/lang-painless/src/main/java/org/elasticsearch/painless/node/SBreak.java +++ b/modules/lang-painless/src/main/java/org/elasticsearch/painless/node/SBreak.java @@ -45,8 +45,6 @@ public final class SBreak extends AStatement { @Override void write(MethodWriter writer) { - writeDebugInfo(writer); - writer.goTo(brake); } } diff --git a/modules/lang-painless/src/main/java/org/elasticsearch/painless/node/SCatch.java b/modules/lang-painless/src/main/java/org/elasticsearch/painless/node/SCatch.java index b3b32d741cd..3fb8c8b4680 100644 --- a/modules/lang-painless/src/main/java/org/elasticsearch/painless/node/SCatch.java +++ b/modules/lang-painless/src/main/java/org/elasticsearch/painless/node/SCatch.java @@ -84,8 +84,7 @@ public final class SCatch extends AStatement { @Override void write(MethodWriter writer) { - writeDebugInfo(writer); - + writer.writeStatementOffset(offset); Label jump = new Label(); writer.mark(jump); diff --git a/modules/lang-painless/src/main/java/org/elasticsearch/painless/node/SContinue.java b/modules/lang-painless/src/main/java/org/elasticsearch/painless/node/SContinue.java index bfea104f23d..18ce5c81231 100644 --- a/modules/lang-painless/src/main/java/org/elasticsearch/painless/node/SContinue.java +++ b/modules/lang-painless/src/main/java/org/elasticsearch/painless/node/SContinue.java @@ -48,8 +48,6 @@ public final class SContinue extends AStatement { @Override void write(MethodWriter writer) { - writeDebugInfo(writer); - writer.goTo(continu); } } diff --git a/modules/lang-painless/src/main/java/org/elasticsearch/painless/node/SDeclaration.java b/modules/lang-painless/src/main/java/org/elasticsearch/painless/node/SDeclaration.java index 246fb992836..526d90dca26 100644 --- a/modules/lang-painless/src/main/java/org/elasticsearch/painless/node/SDeclaration.java +++ b/modules/lang-painless/src/main/java/org/elasticsearch/painless/node/SDeclaration.java @@ -66,8 +66,7 @@ public final class SDeclaration extends AStatement { @Override void write(MethodWriter writer) { - writeDebugInfo(writer); - + writer.writeStatementOffset(offset); if (expression == null) { switch (variable.type.sort) { case VOID: throw new IllegalStateException(error("Illegal tree structure.")); diff --git a/modules/lang-painless/src/main/java/org/elasticsearch/painless/node/SDo.java b/modules/lang-painless/src/main/java/org/elasticsearch/painless/node/SDo.java index 3fbc3b9fafc..3b854ea4ebf 100644 --- a/modules/lang-painless/src/main/java/org/elasticsearch/painless/node/SDo.java +++ b/modules/lang-painless/src/main/java/org/elasticsearch/painless/node/SDo.java @@ -86,8 +86,7 @@ public final class SDo extends AStatement { @Override void write(MethodWriter writer) { - writeDebugInfo(writer); - + writer.writeStatementOffset(offset); Label start = new Label(); Label begin = new Label(); Label end = new Label(); @@ -103,7 +102,7 @@ public final class SDo extends AStatement { condition.fals = end; condition.write(writer); - writer.writeLoopCounter(loopCounterSlot, Math.max(1, block.statementCount)); + writer.writeLoopCounter(loopCounterSlot, Math.max(1, block.statementCount), offset); writer.goTo(start); writer.mark(end); diff --git a/modules/lang-painless/src/main/java/org/elasticsearch/painless/node/SExpression.java b/modules/lang-painless/src/main/java/org/elasticsearch/painless/node/SExpression.java index a9f47fb6522..37f1cbeb24c 100644 --- a/modules/lang-painless/src/main/java/org/elasticsearch/painless/node/SExpression.java +++ b/modules/lang-painless/src/main/java/org/elasticsearch/painless/node/SExpression.java @@ -60,8 +60,7 @@ public final class SExpression extends AStatement { @Override void write(MethodWriter writer) { - writeDebugInfo(writer); - + writer.writeStatementOffset(offset); expression.write(writer); if (methodEscape) { diff --git a/modules/lang-painless/src/main/java/org/elasticsearch/painless/node/SFor.java b/modules/lang-painless/src/main/java/org/elasticsearch/painless/node/SFor.java index 5fb1845e551..ac8e721a1f6 100644 --- a/modules/lang-painless/src/main/java/org/elasticsearch/painless/node/SFor.java +++ b/modules/lang-painless/src/main/java/org/elasticsearch/painless/node/SFor.java @@ -127,8 +127,7 @@ public final class SFor extends AStatement { @Override void write(MethodWriter writer) { - writeDebugInfo(writer); - + writer.writeStatementOffset(offset); Label start = new Label(); Label begin = afterthought == null ? start : new Label(); Label end = new Label(); @@ -160,10 +159,10 @@ public final class SFor extends AStatement { ++statementCount; } - writer.writeLoopCounter(loopCounterSlot, statementCount); + writer.writeLoopCounter(loopCounterSlot, statementCount, offset); block.write(writer); } else { - writer.writeLoopCounter(loopCounterSlot, 1); + writer.writeLoopCounter(loopCounterSlot, 1, offset); } if (afterthought != null) { diff --git a/modules/lang-painless/src/main/java/org/elasticsearch/painless/node/SIf.java b/modules/lang-painless/src/main/java/org/elasticsearch/painless/node/SIf.java index 180b5023811..1f498215478 100644 --- a/modules/lang-painless/src/main/java/org/elasticsearch/painless/node/SIf.java +++ b/modules/lang-painless/src/main/java/org/elasticsearch/painless/node/SIf.java @@ -68,8 +68,7 @@ public final class SIf extends AStatement { @Override void write(MethodWriter writer) { - writeDebugInfo(writer); - + writer.writeStatementOffset(offset); Label fals = new Label(); condition.fals = fals; diff --git a/modules/lang-painless/src/main/java/org/elasticsearch/painless/node/SIfElse.java b/modules/lang-painless/src/main/java/org/elasticsearch/painless/node/SIfElse.java index 217584f32ab..d8fd5c0756e 100644 --- a/modules/lang-painless/src/main/java/org/elasticsearch/painless/node/SIfElse.java +++ b/modules/lang-painless/src/main/java/org/elasticsearch/painless/node/SIfElse.java @@ -89,8 +89,7 @@ public final class SIfElse extends AStatement { @Override void write(MethodWriter writer) { - writeDebugInfo(writer); - + writer.writeStatementOffset(offset); Label end = new Label(); Label fals = elseblock != null ? new Label() : end; diff --git a/modules/lang-painless/src/main/java/org/elasticsearch/painless/node/SReturn.java b/modules/lang-painless/src/main/java/org/elasticsearch/painless/node/SReturn.java index 2f342a5b7b9..d6cc5598263 100644 --- a/modules/lang-painless/src/main/java/org/elasticsearch/painless/node/SReturn.java +++ b/modules/lang-painless/src/main/java/org/elasticsearch/painless/node/SReturn.java @@ -52,8 +52,7 @@ public final class SReturn extends AStatement { @Override void write(MethodWriter writer) { - writeDebugInfo(writer); - + writer.writeStatementOffset(offset); expression.write(writer); writer.returnValue(); } diff --git a/modules/lang-painless/src/main/java/org/elasticsearch/painless/node/SThrow.java b/modules/lang-painless/src/main/java/org/elasticsearch/painless/node/SThrow.java index d002f1f9dad..7c65aafbc56 100644 --- a/modules/lang-painless/src/main/java/org/elasticsearch/painless/node/SThrow.java +++ b/modules/lang-painless/src/main/java/org/elasticsearch/painless/node/SThrow.java @@ -50,8 +50,7 @@ public final class SThrow extends AStatement { @Override void write(MethodWriter writer) { - writeDebugInfo(writer); - + writer.writeStatementOffset(offset); expression.write(writer); writer.throwException(); } diff --git a/modules/lang-painless/src/main/java/org/elasticsearch/painless/node/STry.java b/modules/lang-painless/src/main/java/org/elasticsearch/painless/node/STry.java index a4ef00ef146..0fdb70bdc82 100644 --- a/modules/lang-painless/src/main/java/org/elasticsearch/painless/node/STry.java +++ b/modules/lang-painless/src/main/java/org/elasticsearch/painless/node/STry.java @@ -86,8 +86,7 @@ public final class STry extends AStatement { @Override void write(MethodWriter writer) { - writeDebugInfo(writer); - + writer.writeStatementOffset(offset); Label begin = new Label(); Label end = new Label(); Label exception = new Label(); diff --git a/modules/lang-painless/src/main/java/org/elasticsearch/painless/node/SWhile.java b/modules/lang-painless/src/main/java/org/elasticsearch/painless/node/SWhile.java index 322ae110f66..43ac824dac7 100644 --- a/modules/lang-painless/src/main/java/org/elasticsearch/painless/node/SWhile.java +++ b/modules/lang-painless/src/main/java/org/elasticsearch/painless/node/SWhile.java @@ -92,8 +92,7 @@ public final class SWhile extends AStatement { @Override void write(MethodWriter writer) { - writeDebugInfo(writer); - + writer.writeStatementOffset(offset); Label begin = new Label(); Label end = new Label(); @@ -103,13 +102,13 @@ public final class SWhile extends AStatement { condition.write(writer); if (block != null) { - writer.writeLoopCounter(loopCounterSlot, Math.max(1, block.statementCount)); + writer.writeLoopCounter(loopCounterSlot, Math.max(1, block.statementCount), offset); block.continu = begin; block.brake = end; block.write(writer); } else { - writer.writeLoopCounter(loopCounterSlot, 1); + writer.writeLoopCounter(loopCounterSlot, 1, offset); } if (block == null || !block.allEscape) { diff --git a/modules/lang-painless/src/test/java/org/elasticsearch/painless/CompoundAssignmentTests.java b/modules/lang-painless/src/test/java/org/elasticsearch/painless/CompoundAssignmentTests.java index 03593116538..73a92edf668 100644 --- a/modules/lang-painless/src/test/java/org/elasticsearch/painless/CompoundAssignmentTests.java +++ b/modules/lang-painless/src/test/java/org/elasticsearch/painless/CompoundAssignmentTests.java @@ -120,34 +120,29 @@ public class CompoundAssignmentTests extends ScriptTestCase { public void testDivisionByZero() { // byte - try { + expectScriptThrows(ArithmeticException.class, () -> { exec("byte x = 1; x /= 0; return x;"); - fail("should have hit exception"); - } catch (ArithmeticException expected) {} + }); // short - try { + expectScriptThrows(ArithmeticException.class, () -> { exec("short x = 1; x /= 0; return x;"); - fail("should have hit exception"); - } catch (ArithmeticException expected) {} + }); // char - try { + expectScriptThrows(ArithmeticException.class, () -> { exec("char x = 1; x /= 0; return x;"); - fail("should have hit exception"); - } catch (ArithmeticException expected) {} + }); // int - try { + expectScriptThrows(ArithmeticException.class, () -> { exec("int x = 1; x /= 0; return x;"); - fail("should have hit exception"); - } catch (ArithmeticException expected) {} + }); // long - try { + expectScriptThrows(ArithmeticException.class, () -> { exec("long x = 1; x /= 0; return x;"); - fail("should have hit exception"); - } catch (ArithmeticException expected) {} + }); } public void testRemainder() { diff --git a/modules/lang-painless/src/test/java/org/elasticsearch/painless/DefOperationTests.java b/modules/lang-painless/src/test/java/org/elasticsearch/painless/DefOperationTests.java index 9f171a96889..ca611760f07 100644 --- a/modules/lang-painless/src/test/java/org/elasticsearch/painless/DefOperationTests.java +++ b/modules/lang-painless/src/test/java/org/elasticsearch/painless/DefOperationTests.java @@ -21,10 +21,14 @@ package org.elasticsearch.painless; public class DefOperationTests extends ScriptTestCase { public void testIllegalCast() { - Exception exception = expectThrows(ClassCastException.class, () -> exec("def x = 1.0; int y = x; return y;")); + Exception exception = expectScriptThrows(ClassCastException.class, () -> { + exec("def x = 1.0; int y = x; return y;"); + }); assertTrue(exception.getMessage().contains("cannot be cast")); - exception = expectThrows(ClassCastException.class, () -> exec("def x = (short)1; byte y = x; return y;")); + exception = expectScriptThrows(ClassCastException.class, () -> { + exec("def x = (short)1; byte y = x; return y;"); + }); assertTrue(exception.getMessage().contains("cannot be cast")); } diff --git a/modules/lang-painless/src/test/java/org/elasticsearch/painless/DefOptimizationTests.java b/modules/lang-painless/src/test/java/org/elasticsearch/painless/DefOptimizationTests.java index 72d08b677b9..f9771862335 100644 --- a/modules/lang-painless/src/test/java/org/elasticsearch/painless/DefOptimizationTests.java +++ b/modules/lang-painless/src/test/java/org/elasticsearch/painless/DefOptimizationTests.java @@ -173,7 +173,7 @@ public class DefOptimizationTests extends ScriptTestCase { final String script = "int x;\ndef y = new HashMap();\ny['double'] = 1.0;\nx = y.get('double');\n"; assertBytecodeExists(script, "INVOKEDYNAMIC get(Ljava/lang/Object;Ljava/lang/String;)I"); - final Exception exception = expectThrows(ClassCastException.class, () -> { + final Exception exception = expectScriptThrows(ClassCastException.class, () -> { exec(script); }); assertTrue(exception.getMessage().contains("Cannot cast java.lang.Double to java.lang.Integer")); diff --git a/modules/lang-painless/src/test/java/org/elasticsearch/painless/DivisionTests.java b/modules/lang-painless/src/test/java/org/elasticsearch/painless/DivisionTests.java index 99a48d1ee9d..61046ab59f4 100644 --- a/modules/lang-painless/src/test/java/org/elasticsearch/painless/DivisionTests.java +++ b/modules/lang-painless/src/test/java/org/elasticsearch/painless/DivisionTests.java @@ -114,34 +114,22 @@ public class DivisionTests extends ScriptTestCase { } public void testDivideByZero() throws Exception { - try { + expectScriptThrows(ArithmeticException.class, () -> { exec("int x = 1; int y = 0; return x / y;"); - fail("should have hit exception"); - } catch (ArithmeticException expected) { - // divide by zero - } + }); - try { + expectScriptThrows(ArithmeticException.class, () -> { exec("long x = 1L; long y = 0L; return x / y;"); - fail("should have hit exception"); - } catch (ArithmeticException expected) { - // divide by zero - } + }); } public void testDivideByZeroConst() throws Exception { - try { + expectThrows(ArithmeticException.class, () -> { exec("return 1/0;"); - fail("should have hit exception"); - } catch (ArithmeticException expected) { - // divide by zero - } + }); - try { + expectThrows(ArithmeticException.class, () -> { exec("return 1L/0L;"); - fail("should have hit exception"); - } catch (ArithmeticException expected) { - // divide by zero - } + }); } } diff --git a/modules/lang-painless/src/test/java/org/elasticsearch/painless/OverloadTests.java b/modules/lang-painless/src/test/java/org/elasticsearch/painless/OverloadTests.java index 3e0e4a5fc71..6662aa95fbd 100644 --- a/modules/lang-painless/src/test/java/org/elasticsearch/painless/OverloadTests.java +++ b/modules/lang-painless/src/test/java/org/elasticsearch/painless/OverloadTests.java @@ -34,7 +34,7 @@ public class OverloadTests extends ScriptTestCase { public void testMethodDynamic() { assertEquals(2, exec("def x = 'abc123abc'; return x.indexOf('c');")); assertEquals(8, exec("def x = 'abc123abc'; return x.indexOf('c', 3);")); - IllegalArgumentException expected = expectThrows(IllegalArgumentException.class, () -> { + IllegalArgumentException expected = expectScriptThrows(IllegalArgumentException.class, () -> { exec("def x = 'abc123abc'; return x.indexOf('c', 3, 'bogus');"); }); assertTrue(expected.getMessage().contains("dynamic method [indexOf] with signature [(String,int,String)")); diff --git a/modules/lang-painless/src/test/java/org/elasticsearch/painless/RemainderTests.java b/modules/lang-painless/src/test/java/org/elasticsearch/painless/RemainderTests.java index 836405a9829..b60de9233df 100644 --- a/modules/lang-painless/src/test/java/org/elasticsearch/painless/RemainderTests.java +++ b/modules/lang-painless/src/test/java/org/elasticsearch/painless/RemainderTests.java @@ -114,19 +114,13 @@ public class RemainderTests extends ScriptTestCase { } public void testDivideByZero() throws Exception { - try { + expectScriptThrows(ArithmeticException.class, () -> { exec("int x = 1; int y = 0; return x % y;"); - fail("should have hit exception"); - } catch (ArithmeticException expected) { - // divide by zero - } + }); - try { + expectScriptThrows(ArithmeticException.class, () -> { exec("long x = 1L; long y = 0L; return x % y;"); - fail("should have hit exception"); - } catch (ArithmeticException expected) { - // divide by zero - } + }); } public void testDivideByZeroConst() throws Exception { diff --git a/modules/lang-painless/src/test/java/org/elasticsearch/painless/ScriptTestCase.java b/modules/lang-painless/src/test/java/org/elasticsearch/painless/ScriptTestCase.java index 27558dc745e..b4e416ca614 100644 --- a/modules/lang-painless/src/test/java/org/elasticsearch/painless/ScriptTestCase.java +++ b/modules/lang-painless/src/test/java/org/elasticsearch/painless/ScriptTestCase.java @@ -24,11 +24,13 @@ import org.elasticsearch.common.lucene.ScorerAware; import org.elasticsearch.common.settings.Settings; import org.elasticsearch.script.CompiledScript; import org.elasticsearch.script.ExecutableScript; +import org.elasticsearch.script.ScriptException; import org.elasticsearch.script.ScriptService; import org.elasticsearch.test.ESTestCase; import org.junit.Before; -import java.util.Collections; +import junit.framework.AssertionFailedError; + import java.util.HashMap; import java.util.Map; @@ -76,4 +78,27 @@ public abstract class ScriptTestCase extends ESTestCase { final String asm = Debugger.toString(script); assertTrue("bytecode not found", asm.contains(bytecode)); } + + /** Checks a specific exception class is thrown (boxed inside ScriptException) and returns it. */ + public static T expectScriptThrows(Class expectedType, ThrowingRunnable runnable) { + try { + runnable.run(); + } catch (Throwable e) { + if (e instanceof ScriptException) { + e = e.getCause(); + if (expectedType.isInstance(e)) { + return expectedType.cast(e); + } + } else { + AssertionFailedError assertion = new AssertionFailedError("Expected boxed ScriptException"); + assertion.initCause(e); + throw assertion; + } + AssertionFailedError assertion = new AssertionFailedError("Unexpected exception type, expected " + + expectedType.getSimpleName()); + assertion.initCause(e); + throw assertion; + } + throw new AssertionFailedError("Expected exception " + expectedType.getSimpleName()); + } } diff --git a/modules/lang-painless/src/test/java/org/elasticsearch/painless/StringTests.java b/modules/lang-painless/src/test/java/org/elasticsearch/painless/StringTests.java index b06199cf903..12d275de6e5 100644 --- a/modules/lang-painless/src/test/java/org/elasticsearch/painless/StringTests.java +++ b/modules/lang-painless/src/test/java/org/elasticsearch/painless/StringTests.java @@ -162,32 +162,24 @@ public class StringTests extends ScriptTestCase { assertEquals('c', exec("String s = \"c\"; (char)s")); assertEquals('c', exec("String s = 'c'; (char)s")); - try { + ClassCastException expected = expectScriptThrows(ClassCastException.class, () -> { assertEquals("cc", exec("return (String)(char)\"cc\"")); - fail(); - } catch (final ClassCastException cce) { - assertTrue(cce.getMessage().contains("Cannot cast [String] with length greater than one to [char].")); - } + }); + assertTrue(expected.getMessage().contains("Cannot cast [String] with length greater than one to [char].")); - try { + expected = expectScriptThrows(ClassCastException.class, () -> { assertEquals("cc", exec("return (String)(char)'cc'")); - fail(); - } catch (final ClassCastException cce) { - assertTrue(cce.getMessage().contains("Cannot cast [String] with length greater than one to [char].")); - } + }); + assertTrue(expected.getMessage().contains("Cannot cast [String] with length greater than one to [char].")); - try { + expected = expectScriptThrows(ClassCastException.class, () -> { assertEquals('c', exec("String s = \"cc\"; (char)s")); - fail(); - } catch (final ClassCastException cce) { - assertTrue(cce.getMessage().contains("Cannot cast [String] with length greater than one to [char].")); - } + }); + assertTrue(expected.getMessage().contains("Cannot cast [String] with length greater than one to [char].")); - try { + expected = expectScriptThrows(ClassCastException.class, () -> { assertEquals('c', exec("String s = 'cc'; (char)s")); - fail(); - } catch (final ClassCastException cce) { - assertTrue(cce.getMessage().contains("Cannot cast [String] with length greater than one to [char].")); - } + }); + assertTrue(expected.getMessage().contains("Cannot cast [String] with length greater than one to [char].")); } } diff --git a/modules/lang-painless/src/test/java/org/elasticsearch/painless/TryCatchTests.java b/modules/lang-painless/src/test/java/org/elasticsearch/painless/TryCatchTests.java index 07439f7f42b..bf78ee0afa5 100644 --- a/modules/lang-painless/src/test/java/org/elasticsearch/painless/TryCatchTests.java +++ b/modules/lang-painless/src/test/java/org/elasticsearch/painless/TryCatchTests.java @@ -26,7 +26,7 @@ public class TryCatchTests extends ScriptTestCase { /** throws an exception */ public void testThrow() { - RuntimeException exception = expectThrows(RuntimeException.class, () -> { + RuntimeException exception = expectScriptThrows(RuntimeException.class, () -> { exec("throw new RuntimeException('test')"); }); assertEquals("test", exception.getMessage()); @@ -48,7 +48,7 @@ public class TryCatchTests extends ScriptTestCase { /** tries to catch a different type of exception */ public void testNoCatch() { - RuntimeException exception = expectThrows(RuntimeException.class, () -> { + RuntimeException exception = expectScriptThrows(RuntimeException.class, () -> { exec("try { if (params.param == 'true') throw new RuntimeException('test'); } " + "catch (ArithmeticException e) { return 1; } return 2;", Collections.singletonMap("param", "true")); diff --git a/modules/lang-painless/src/test/java/org/elasticsearch/painless/WhenThingsGoWrongTests.java b/modules/lang-painless/src/test/java/org/elasticsearch/painless/WhenThingsGoWrongTests.java index c2b40df8c9d..961c3561c48 100644 --- a/modules/lang-painless/src/test/java/org/elasticsearch/painless/WhenThingsGoWrongTests.java +++ b/modules/lang-painless/src/test/java/org/elasticsearch/painless/WhenThingsGoWrongTests.java @@ -25,36 +25,40 @@ import java.util.Collections; public class WhenThingsGoWrongTests extends ScriptTestCase { public void testNullPointer() { - expectThrows(NullPointerException.class, () -> { + expectScriptThrows(NullPointerException.class, () -> { exec("int x = params['missing']; return x;"); }); } + /** test "line numbers" in the bytecode, which are really 1-based offsets */ public void testLineNumbers() { // trigger NPE at line 1 of the script - NullPointerException exception = expectThrows(NullPointerException.class, () -> { + NullPointerException exception = expectScriptThrows(NullPointerException.class, () -> { exec("String x = null; boolean y = x.isEmpty();\n" + "return y;"); }); - assertEquals(1, exception.getStackTrace()[0].getLineNumber()); + // null deref at x.isEmpty(), the '.' is offset 30 (+1) + assertEquals(30 + 1, exception.getStackTrace()[0].getLineNumber()); // trigger NPE at line 2 of the script - exception = expectThrows(NullPointerException.class, () -> { + exception = expectScriptThrows(NullPointerException.class, () -> { exec("String x = null;\n" + "return x.isEmpty();"); }); - assertEquals(2, exception.getStackTrace()[0].getLineNumber()); + // null deref at x.isEmpty(), the '.' is offset 25 (+1) + assertEquals(25 + 1, exception.getStackTrace()[0].getLineNumber()); // trigger NPE at line 3 of the script - exception = expectThrows(NullPointerException.class, () -> { + exception = expectScriptThrows(NullPointerException.class, () -> { exec("String x = null;\n" + "String y = x;\n" + "return y.isEmpty();"); }); - assertEquals(3, exception.getStackTrace()[0].getLineNumber()); + // null deref at y.isEmpty(), the '.' is offset 39 (+1) + assertEquals(39 + 1, exception.getStackTrace()[0].getLineNumber()); // trigger NPE at line 4 in script (inside conditional) - exception = expectThrows(NullPointerException.class, () -> { + exception = expectScriptThrows(NullPointerException.class, () -> { exec("String x = null;\n" + "boolean y = false;\n" + "if (!y) {\n" + @@ -62,7 +66,8 @@ public class WhenThingsGoWrongTests extends ScriptTestCase { "}\n" + "return y;"); }); - assertEquals(4, exception.getStackTrace()[0].getLineNumber()); + // null deref at x.isEmpty(), the '.' is offset 53 (+1) + assertEquals(53 + 1, exception.getStackTrace()[0].getLineNumber()); } public void testInvalidShift() { @@ -83,46 +88,46 @@ public class WhenThingsGoWrongTests extends ScriptTestCase { } public void testInfiniteLoops() { - PainlessError expected = expectThrows(PainlessError.class, () -> { + PainlessError expected = expectScriptThrows(PainlessError.class, () -> { exec("boolean x = true; while (x) {}"); }); assertTrue(expected.getMessage().contains( "The maximum number of statements that can be executed in a loop has been reached.")); - expected = expectThrows(PainlessError.class, () -> { + expected = expectScriptThrows(PainlessError.class, () -> { exec("while (true) {int y = 5;}"); }); assertTrue(expected.getMessage().contains( "The maximum number of statements that can be executed in a loop has been reached.")); - expected = expectThrows(PainlessError.class, () -> { + expected = expectScriptThrows(PainlessError.class, () -> { exec("while (true) { boolean x = true; while (x) {} }"); }); assertTrue(expected.getMessage().contains( "The maximum number of statements that can be executed in a loop has been reached.")); - expected = expectThrows(PainlessError.class, () -> { + expected = expectScriptThrows(PainlessError.class, () -> { exec("while (true) { boolean x = false; while (x) {} }"); fail("should have hit PainlessError"); }); assertTrue(expected.getMessage().contains( "The maximum number of statements that can be executed in a loop has been reached.")); - expected = expectThrows(PainlessError.class, () -> { + expected = expectScriptThrows(PainlessError.class, () -> { exec("boolean x = true; for (;x;) {}"); fail("should have hit PainlessError"); }); assertTrue(expected.getMessage().contains( "The maximum number of statements that can be executed in a loop has been reached.")); - expected = expectThrows(PainlessError.class, () -> { + expected = expectScriptThrows(PainlessError.class, () -> { exec("for (;;) {int x = 5;}"); fail("should have hit PainlessError"); }); assertTrue(expected.getMessage().contains( "The maximum number of statements that can be executed in a loop has been reached.")); - expected = expectThrows(PainlessError.class, () -> { + expected = expectScriptThrows(PainlessError.class, () -> { exec("def x = true; do {int y = 5;} while (x)"); fail("should have hit PainlessError"); }); @@ -140,7 +145,7 @@ public class WhenThingsGoWrongTests extends ScriptTestCase { // right below limit: ok exec("for (int x = 0; x < 9999; ++x) {}"); - PainlessError expected = expectThrows(PainlessError.class, () -> { + PainlessError expected = expectScriptThrows(PainlessError.class, () -> { exec("for (int x = 0; x < 10000; ++x) {}"); }); assertTrue(expected.getMessage().contains( @@ -163,32 +168,32 @@ public class WhenThingsGoWrongTests extends ScriptTestCase { } public void testIllegalDynamicMethod() { - IllegalArgumentException expected = expectThrows(IllegalArgumentException.class, () -> { + IllegalArgumentException expected = expectScriptThrows(IllegalArgumentException.class, () -> { exec("def x = 'test'; return x.getClass().toString()"); }); assertTrue(expected.getMessage().contains("Unable to find dynamic method")); } public void testDynamicNPE() { - expectThrows(NullPointerException.class, () -> { + expectScriptThrows(NullPointerException.class, () -> { exec("def x = null; return x.toString()"); }); } public void testDynamicWrongArgs() { - expectThrows(WrongMethodTypeException.class, () -> { + expectScriptThrows(WrongMethodTypeException.class, () -> { exec("def x = new ArrayList(); return x.get('bogus');"); }); } public void testDynamicArrayWrongIndex() { - expectThrows(WrongMethodTypeException.class, () -> { + expectScriptThrows(WrongMethodTypeException.class, () -> { exec("def x = new long[1]; x[0]=1; return x['bogus'];"); }); } public void testDynamicListWrongIndex() { - expectThrows(WrongMethodTypeException.class, () -> { + expectScriptThrows(WrongMethodTypeException.class, () -> { exec("def x = new ArrayList(); x.add('foo'); return x['bogus'];"); }); } diff --git a/qa/smoke-test-reindex-with-groovy/src/test/resources/rest-api-spec/test/reindex/40_search_failures.yaml b/qa/smoke-test-reindex-with-groovy/src/test/resources/rest-api-spec/test/reindex/40_search_failures.yaml index 2c649220b75..c43374e7de2 100644 --- a/qa/smoke-test-reindex-with-groovy/src/test/resources/rest-api-spec/test/reindex/40_search_failures.yaml +++ b/qa/smoke-test-reindex-with-groovy/src/test/resources/rest-api-spec/test/reindex/40_search_failures.yaml @@ -27,7 +27,7 @@ - is_true: failures.0.shard - match: {failures.0.index: source} - is_true: failures.0.node - - match: {failures.0.reason.type: script_exception} + - match: {failures.0.reason.type: general_script_exception} - match: {failures.0.reason.reason: "failed to run inline script [1/0] using lang [groovy]"} - match: {failures.0.reason.caused_by.type: arithmetic_exception} - match: {failures.0.reason.caused_by.reason: Division by zero} diff --git a/qa/smoke-test-reindex-with-groovy/src/test/resources/rest-api-spec/test/update_by_query/40_search_failure.yaml b/qa/smoke-test-reindex-with-groovy/src/test/resources/rest-api-spec/test/update_by_query/40_search_failure.yaml index 748e5cd15e0..3aecca73332 100644 --- a/qa/smoke-test-reindex-with-groovy/src/test/resources/rest-api-spec/test/update_by_query/40_search_failure.yaml +++ b/qa/smoke-test-reindex-with-groovy/src/test/resources/rest-api-spec/test/update_by_query/40_search_failure.yaml @@ -23,7 +23,7 @@ - is_true: failures.0.shard - match: {failures.0.index: source} - is_true: failures.0.node - - match: {failures.0.reason.type: script_exception} + - match: {failures.0.reason.type: general_script_exception} - match: {failures.0.reason.reason: "failed to run inline script [1/0] using lang [groovy]"} - match: {failures.0.reason.caused_by.type: arithmetic_exception} - match: {failures.0.reason.caused_by.reason: Division by zero} From 76ca4af5612f81b7914ac13c00f54d9b1c1782c4 Mon Sep 17 00:00:00 2001 From: Robert Muir Date: Thu, 26 May 2016 15:03:13 -0400 Subject: [PATCH 2/2] move instanceof to catch block --- .../org/elasticsearch/script/ScriptService.java | 14 ++++++-------- 1 file changed, 6 insertions(+), 8 deletions(-) diff --git a/core/src/main/java/org/elasticsearch/script/ScriptService.java b/core/src/main/java/org/elasticsearch/script/ScriptService.java index f5ee456c119..646ad5e25d6 100644 --- a/core/src/main/java/org/elasticsearch/script/ScriptService.java +++ b/core/src/main/java/org/elasticsearch/script/ScriptService.java @@ -305,11 +305,10 @@ public class ScriptService extends AbstractComponent implements Closeable { // for the inline case, then its anonymous: null. String actualName = (type == ScriptType.INLINE) ? null : name; compiledScript = new CompiledScript(type, name, lang, scriptEngineService.compile(actualName, code, params)); - } catch (Exception exception) { + } catch (ScriptException good) { // TODO: remove this try-catch completely, when all script engines have good exceptions! - if (exception instanceof ScriptException) { - throw exception; // its already good! - } + throw good; // its already good + } catch (Exception exception) { throw new GeneralScriptException("Failed to compile " + type + " script [" + name + "] using lang [" + lang + "]", exception); } @@ -367,11 +366,10 @@ public class ScriptService extends AbstractComponent implements Closeable { "skipping compile of script [{}], lang [{}] as all scripted operations are disabled for indexed scripts", template.getScript(), scriptLang); } - } catch (Exception e) { + } catch (ScriptException good) { // TODO: remove this when all script engines have good exceptions! - if (e instanceof ScriptException) { - throw e; // its already good! - } + throw good; // its already good! + } catch (Exception e) { throw new IllegalArgumentException("Unable to parse [" + template.getScript() + "] lang [" + scriptLang + "]", e); }