Skip to content

Commit

Permalink
Core: Pick inner most parse exception as root cause (#30270)
Browse files Browse the repository at this point in the history
Just like `ElasticsearchException`, the inner most
`XContentParseException` tends to contain the root cause of the
exception and show be show to the user in the `root_cause` field.

The effectively undoes most of the changes that #29373 made to the
`root_cause` for parsing exceptions. The `type` field still changes from
`parse_exception` to `x_content_parse_exception`, but this seems like a
fairly safe change.

`ElasticsearchWrapperException` *looks* tempting to implement this but
the behavior isn't quite right. `ElasticsearchWrapperExceptions` are
entirely unwrapped until the cause no longer
`implements ElasticsearchWrapperException` but `XContentParseException`
should be unwrapped until its cause is no longer an
`XContentParseException` but no further. In other words,
`ElasticsearchWrapperException` are unwrapped one step too far.

Closes #30261
  • Loading branch information
nik9000 committed May 1, 2018
1 parent b9e1860 commit 99b98fa
Show file tree
Hide file tree
Showing 3 changed files with 58 additions and 14 deletions.
18 changes: 18 additions & 0 deletions server/src/main/java/org/elasticsearch/ElasticsearchException.java
Expand Up @@ -31,6 +31,7 @@
import org.elasticsearch.common.logging.LoggerMessageFormat;
import org.elasticsearch.common.xcontent.ToXContentFragment;
import org.elasticsearch.common.xcontent.XContentBuilder;
import org.elasticsearch.common.xcontent.XContentParseException;
import org.elasticsearch.common.xcontent.XContentParser;
import org.elasticsearch.index.Index;
import org.elasticsearch.index.shard.ShardId;
Expand Down Expand Up @@ -635,8 +636,25 @@ public ElasticsearchException[] guessRootCauses() {
public static ElasticsearchException[] guessRootCauses(Throwable t) {
Throwable ex = ExceptionsHelper.unwrapCause(t);
if (ex instanceof ElasticsearchException) {
// ElasticsearchException knows how to guess its own root cause
return ((ElasticsearchException) ex).guessRootCauses();
}
if (ex instanceof XContentParseException) {
/*
* We'd like to unwrap parsing exceptions to the inner-most
* parsing exception because that is generally the most interesting
* exception to return to the user. If that exception is caused by
* an ElasticsearchException we'd like to keep unwrapping because
* ElasticserachExceptions tend to contain useful information for
* the user.
*/
Throwable cause = ex.getCause();
if (cause != null) {
if (cause instanceof XContentParseException || cause instanceof ElasticsearchException) {
return guessRootCauses(ex.getCause());
}
}
}
return new ElasticsearchException[]{new ElasticsearchException(t.getMessage(), t) {
@Override
protected String getExceptionName() {
Expand Down
Expand Up @@ -19,7 +19,11 @@

package org.elasticsearch;

/**
* An exception that is meant to be "unwrapped" when sent back to the user
* as an error because its is {@link #getCause() cause}, if non-null is
* <strong>always</strong> more useful to the user than the exception itself.
*/
public interface ElasticsearchWrapperException {

Throwable getCause();
}
Expand Up @@ -41,6 +41,7 @@
import org.elasticsearch.common.xcontent.XContentFactory;
import org.elasticsearch.common.xcontent.XContentHelper;
import org.elasticsearch.common.xcontent.XContentLocation;
import org.elasticsearch.common.xcontent.XContentParseException;
import org.elasticsearch.common.xcontent.XContentParser;
import org.elasticsearch.common.xcontent.XContentType;
import org.elasticsearch.discovery.DiscoverySettings;
Expand Down Expand Up @@ -78,6 +79,7 @@
import static org.hamcrest.CoreMatchers.hasItems;
import static org.hamcrest.Matchers.equalTo;
import static org.hamcrest.Matchers.hasSize;
import static org.hamcrest.Matchers.instanceOf;
import static org.hamcrest.Matchers.startsWith;

public class ElasticsearchExceptionTests extends ESTestCase {
Expand Down Expand Up @@ -124,13 +126,13 @@ public void testGuessRootCause() {
} else {
rootCauses = ElasticsearchException.guessRootCauses(randomBoolean() ? new RemoteTransportException("remoteboom", ex) : ex);
}
assertEquals(ElasticsearchException.getExceptionName(rootCauses[0]), "parsing_exception");
assertEquals(rootCauses[0].getMessage(), "foobar");
assertEquals("parsing_exception", ElasticsearchException.getExceptionName(rootCauses[0]));
assertEquals("foobar", rootCauses[0].getMessage());

ElasticsearchException oneLevel = new ElasticsearchException("foo", new RuntimeException("foobar"));
rootCauses = oneLevel.guessRootCauses();
assertEquals(ElasticsearchException.getExceptionName(rootCauses[0]), "exception");
assertEquals(rootCauses[0].getMessage(), "foo");
assertEquals("exception", ElasticsearchException.getExceptionName(rootCauses[0]));
assertEquals("foo", rootCauses[0].getMessage());
}
{
ShardSearchFailure failure = new ShardSearchFailure(
Expand All @@ -146,20 +148,40 @@ public void testGuessRootCause() {
assertEquals(rootCauses.length, 2);
assertEquals(ElasticsearchException.getExceptionName(rootCauses[0]), "parsing_exception");
assertEquals(rootCauses[0].getMessage(), "foobar");
assertEquals(((ParsingException) rootCauses[0]).getLineNumber(), 1);
assertEquals(((ParsingException) rootCauses[0]).getColumnNumber(), 2);
assertEquals(ElasticsearchException.getExceptionName(rootCauses[1]), "query_shard_exception");
assertEquals((rootCauses[1]).getIndex().getName(), "foo1");
assertEquals(rootCauses[1].getMessage(), "foobar");
assertEquals(1, ((ParsingException) rootCauses[0]).getLineNumber());
assertEquals(2, ((ParsingException) rootCauses[0]).getColumnNumber());
assertEquals("query_shard_exception", ElasticsearchException.getExceptionName(rootCauses[1]));
assertEquals("foo1", rootCauses[1].getIndex().getName());
assertEquals("foobar", rootCauses[1].getMessage());
}

{
final ElasticsearchException[] foobars = ElasticsearchException.guessRootCauses(new IllegalArgumentException("foobar"));
assertEquals(foobars.length, 1);
assertTrue(foobars[0] instanceof ElasticsearchException);
assertEquals(foobars[0].getMessage(), "foobar");
assertEquals(foobars[0].getCause().getClass(), IllegalArgumentException.class);
assertEquals(foobars[0].getExceptionName(), "illegal_argument_exception");
assertThat(foobars[0], instanceOf(ElasticsearchException.class));
assertEquals("foobar", foobars[0].getMessage());
assertEquals(IllegalArgumentException.class, foobars[0].getCause().getClass());
assertEquals("illegal_argument_exception", foobars[0].getExceptionName());
}

{
XContentParseException inner = new XContentParseException(null, "inner");
XContentParseException outer = new XContentParseException(null, "outer", inner);
final ElasticsearchException[] causes = ElasticsearchException.guessRootCauses(outer);
assertEquals(causes.length, 1);
assertThat(causes[0], instanceOf(ElasticsearchException.class));
assertEquals("inner", causes[0].getMessage());
assertEquals("x_content_parse_exception", causes[0].getExceptionName());
}

{
ElasticsearchException inner = new ElasticsearchException("inner");
XContentParseException outer = new XContentParseException(null, "outer", inner);
final ElasticsearchException[] causes = ElasticsearchException.guessRootCauses(outer);
assertEquals(causes.length, 1);
assertThat(causes[0], instanceOf(ElasticsearchException.class));
assertEquals("inner", causes[0].getMessage());
assertEquals("exception", causes[0].getExceptionName());
}
}

Expand Down

0 comments on commit 99b98fa

Please sign in to comment.