Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Core: Pick inner most parse exception as root cause #30270

Merged
merged 1 commit into from May 1, 2018
Merged

Core: Pick inner most parse exception as root cause #30270

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
18 changes: 18 additions & 0 deletions server/src/main/java/org/elasticsearch/ElasticsearchException.java
View file
Open in desktop
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) {
/*
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel like if we get another exception like this one we should try to build something a little more generic and less "heuristics that make the exception look good". But XContentParseException isn't really the same thing as ElasticsearchException`. It is similar, but different enough that I don't think I could boil out the generic bits without this getting wonky.

* 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
6 changes: 5 additions & 1 deletion server/src/main/java/org/elasticsearch/ElasticsearchWrapperException.java
View file
Open in desktop
Expand Up @@ -19,7 +19,11 @@

package org.elasticsearch;

/**
* An exception that is meant to be "unwrapped" when sent back to the user
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This one turned out not to be useful but I researched it so I figured I'd add the javadocs.

* 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();
}
48 changes: 35 additions & 13 deletions server/src/test/java/org/elasticsearch/ElasticsearchExceptionTests.java
View file
Open in desktop
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");
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These were all backwards....

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